certs: properly handle primary flag and revocation of uids

OpenPGP-Keychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainContract.java

@@ -52,6 +52,7 @@ public class KeychainContract {
         String USER_ID = "user_id"; // not a database id
         String RANK = "rank"; // ONLY used for sorting! no key, no nothing!
         String IS_PRIMARY = "is_primary";
+        String IS_REVOKED = "is_revoked";
     }
 
     interface CertsColumns {
@@ -107,7 +108,8 @@ public class KeychainContract {
     public static final String PATH_ACCOUNTS = "accounts";
 
     public static class KeyRings implements BaseColumns, KeysColumns, UserIdsColumns {
-        public static final String MASTER_KEY_ID = "master_key_id";
+        public static final String MASTER_KEY_ID = KeysColumns.MASTER_KEY_ID;
+        public static final String IS_REVOKED = KeysColumns.IS_REVOKED;
         public static final String HAS_SECRET = "has_secret";
 
         public static final Uri CONTENT_URI = BASE_CONTENT_URI_INTERNAL.buildUpon()

OpenPGP-Keychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainDatabase.java

@@ -96,6 +96,7 @@ public class KeychainDatabase extends SQLiteOpenHelper {
                 + UserIdsColumns.USER_ID + " CHARMANDER, "
 
                 + UserIdsColumns.IS_PRIMARY + " BOOLEAN, "
+                + UserIdsColumns.IS_REVOKED + " BOOLEAN, "
                 + UserIdsColumns.RANK+ " INTEGER, "
 
                 + "PRIMARY KEY(" + UserIdsColumns.MASTER_KEY_ID + ", " + UserIdsColumns.USER_ID + "), "

OpenPGP-Keychain/src/main/java/org/sufficientlysecure/keychain/provider/KeychainProvider.java

@@ -249,7 +249,7 @@ public class KeychainProvider extends ContentProvider {
                 projectionMap.put(KeyRings.MASTER_KEY_ID, Tables.KEYS + "." + Keys.MASTER_KEY_ID);
                 projectionMap.put(KeyRings.KEY_ID, Keys.KEY_ID);
                 projectionMap.put(KeyRings.KEY_SIZE, Keys.KEY_SIZE);
-                projectionMap.put(KeyRings.IS_REVOKED, Keys.IS_REVOKED);
+                projectionMap.put(KeyRings.IS_REVOKED, Tables.KEYS + "." + Keys.IS_REVOKED);
                 projectionMap.put(KeyRings.CAN_CERTIFY, Keys.CAN_CERTIFY);
                 projectionMap.put(KeyRings.CAN_ENCRYPT, Keys.CAN_ENCRYPT);
                 projectionMap.put(KeyRings.CAN_SIGN, Keys.CAN_SIGN);

OpenPGP-Keychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java

@@ -32,7 +32,6 @@ import android.os.RemoteException;
 import org.spongycastle.bcpg.SignatureSubpacketTags;
 import org.spongycastle.bcpg.sig.SignatureExpirationTime;
 import org.spongycastle.openpgp.PGPException;
-import org.spongycastle.openpgp.PGPSecretKey;
 import org.spongycastle.openpgp.operator.jcajce.JcaPGPContentVerifierBuilderProvider;
 import org.spongycastle.openpgp.PGPKeyRing;
 import org.spongycastle.openpgp.PGPPublicKey;
@@ -57,8 +56,10 @@ import org.sufficientlysecure.keychain.util.Log;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.HashSet;
 import java.util.Set;
@@ -229,19 +230,40 @@ public class ProviderHelper {
         if(secretRing != null)
             allKeyRings.put(secretRing.getSecretKey().getKeyID(), secretRing);
 
-        int userIdRank = 0;
+        // classify and order user ids. primary are moved to the front, revoked to the back,
-        for (String userId : new IterableIterator<String>(masterKey.getUserIDs())) {
+        // otherwise the order in the keyfile is preserved.
-            operations.add(buildUserIdOperations(context, masterKeyId, userId, userIdRank));
+        List<UserIdItem> uids = new ArrayList<UserIdItem>();
 
-            // HashMap<Long, PGPSignature> certs = new HashMap<Long,PGPSignature>();
+        for (String userId : new IterableIterator<String>(masterKey.getUserIDs())) {
+            UserIdItem item = new UserIdItem();
+            uids.add(item);
+            item.userId = userId;
 
             // look through signatures for this specific key
             for (PGPSignature cert : new IterableIterator<PGPSignature>(
                     masterKey.getSignaturesForID(userId))) {
                 long certId = cert.getKeyID();
-                int verified = 0;
-                // verify from the key itself
                 try {
+                    // self signature
+                    if(certId == masterKeyId) {
+                        cert.init(
+                                new JcaPGPContentVerifierBuilderProvider().setProvider(
+                                        Constants.BOUNCY_CASTLE_PROVIDER_NAME),
+                                masterKey);
+                        if(!cert.verifyCertification(userId,  masterKey)) {
+                            // not verified?! dang! TODO notify user? this is kinda serious...
+                            Log.e(Constants.TAG, "Could not verify self signature for " + userId + "!");
+                            continue;
+                        }
+                        // is this the first, or a more recent certificate?
+                        if(item.selfCert == null ||
+                                item.selfCert.getCreationTime().before(cert.getCreationTime())) {
+                            item.selfCert = cert;
+                            item.isPrimary = cert.getHashedSubPackets().isPrimaryUserID();
+                            item.isRevoked =
+                                    cert.getSignatureType() == PGPSignature.CERTIFICATION_REVOCATION;
+                        }
+                    }
                     // verify signatures from known private keys
                     if(allKeyRings.containsKey(certId)) {
                         // mark them as verified
@@ -249,20 +271,9 @@ public class ProviderHelper {
                                 new JcaPGPContentVerifierBuilderProvider().setProvider(
                                         Constants.BOUNCY_CASTLE_PROVIDER_NAME),
                                 allKeyRings.get(certId).getPublicKey());
-                        verified = cert.verifyCertification(userId,  masterKey) ? Certs.VERIFIED_SECRET : 0;
+                        if(cert.verifyCertification(userId, masterKey)) {
-                        Log.d(Constants.TAG, "Verified sig for " + userId + " " + verified + " from "
+                            item.trustedCerts.add(cert);
-                                + PgpKeyHelper.convertKeyIdToHex(certId)
+                        }
-                        );
-                    // if that didn't work out, is it at least an own signature?
-                    } else if(certId == masterKeyId) {
-                        cert.init(
-                                new JcaPGPContentVerifierBuilderProvider().setProvider(
-                                    Constants.BOUNCY_CASTLE_PROVIDER_NAME),
-                                masterKey);
-                        verified = cert.verifyCertification(userId,  masterKey) ? Certs.VERIFIED_SELF : 0;
-                        Log.d(Constants.TAG, "Verified sig for " + userId + " " + verified + " from "
-                                + PgpKeyHelper.convertKeyIdToHex(certId)
-                        );
                     }
                 } catch(SignatureException e) {
                     Log.e(Constants.TAG, "Signature verification failed! "
@@ -275,15 +286,25 @@ public class ProviderHelper {
                             + " from "
                             + PgpKeyHelper.convertKeyIdToHex(cert.getKeyID()), e);
                 }
-                Log.d(Constants.TAG, "sig for " + userId + " from "
-                        + PgpKeyHelper.convertKeyIdToHex(cert.getKeyID())
-                );
-                // regardless of verification, save the certification
-                operations.add(buildCertOperations(
-                        context, masterKeyId, userIdRank, cert, verified));
             }
+        }
 
-            ++userIdRank;
+        // primary before regular before revoked (see UserIdItem.compareTo)
+        // this is a stable sort, so the order of keys is otherwise preserved.
+        Collections.sort(uids);
+        // iterate and put into db
+        for(int userIdRank = 0; userIdRank < uids.size(); userIdRank++) {
+            UserIdItem item = uids.get(userIdRank);
+            operations.add(buildUserIdOperations(masterKeyId, item, userIdRank));
+            // no self cert is bad, but allowed by the rfc...
+            if(item.selfCert != null) {
+                operations.add(buildCertOperations(
+                        masterKeyId, userIdRank, item.selfCert, Certs.VERIFIED_SELF));
+            }
+            for(int i = 0; i < item.trustedCerts.size(); i++) {
+                operations.add(buildCertOperations(
+                        masterKeyId, userIdRank, item.trustedCerts.get(i), Certs.VERIFIED_SECRET));
+            }
         }
 
         try {
@@ -301,6 +322,25 @@ public class ProviderHelper {
 
     }
 
+    private static class UserIdItem implements Comparable<UserIdItem> {
+        String userId;
+        boolean isPrimary = false;
+        boolean isRevoked = false;
+        PGPSignature selfCert;
+        List<PGPSignature> trustedCerts = new ArrayList<PGPSignature>();
+
+        @Override
+        public int compareTo(UserIdItem o) {
+            // if one key is primary but the other isn't, the primary one always comes first
+            if(isPrimary != o.isPrimary)
+                return isPrimary ? -1 : 1;
+            // revoked keys always come last!
+            if(isRevoked != o.isRevoked)
+                return isRevoked ? 1 : -1;
+            return 0;
+        }
+    }
+
     /**
      * Saves a PGPSecretKeyRing in the DB. This will only work if a corresponding public keyring
      * is already in the database!
@@ -368,11 +408,10 @@ public class ProviderHelper {
     /**
      * Build ContentProviderOperation to add PGPPublicKey to database corresponding to a keyRing
      */
-    private static ContentProviderOperation buildCertOperations(Context context,
+    private static ContentProviderOperation buildCertOperations(long masterKeyId,
-                                                                     long masterKeyId,
+                                                                int rank,
-                                                                     int rank,
+                                                                PGPSignature cert,
-                                                                     PGPSignature cert,
+                                                                int verified)
-                                                                     int verified)
             throws IOException {
         ContentValues values = new ContentValues();
         values.put(Certs.MASTER_KEY_ID, masterKeyId);
@@ -395,11 +434,13 @@ public class ProviderHelper {
     /**
      * Build ContentProviderOperation to add PublicUserIds to database corresponding to a keyRing
      */
-    private static ContentProviderOperation buildUserIdOperations(Context context,
+    private static ContentProviderOperation buildUserIdOperations(long masterKeyId, UserIdItem item,
-                                                                  long masterKeyId, String userId, int rank) {
+                                                                  int rank) {
         ContentValues values = new ContentValues();
         values.put(UserIds.MASTER_KEY_ID, masterKeyId);
-        values.put(UserIds.USER_ID, userId);
+        values.put(UserIds.USER_ID, item.userId);
+        values.put(UserIds.IS_PRIMARY, item.isPrimary);
+        values.put(UserIds.IS_REVOKED, item.isRevoked);
         values.put(UserIds.RANK, rank);
 
         Uri uri = UserIds.buildUserIdsUri(Long.toString(masterKeyId));

OpenPGP-Keychain/src/main/java/org/sufficientlysecure/keychain/ui/ViewCertActivity.java

@@ -54,18 +54,21 @@ public class ViewCertActivity extends ActionBarActivity
     static final String[] PROJECTION = new String[] {
             Certs.MASTER_KEY_ID,
             Certs.USER_ID,
+            Certs.TYPE,
             Certs.CREATION,
+            Certs.EXPIRY,
             Certs.KEY_ID_CERTIFIER,
             Certs.SIGNER_UID,
             Certs.TYPE
     };
     private static final int INDEX_MASTER_KEY_ID = 1;
     private static final int INDEX_USER_ID = 2;
-    private static final int INDEX_CREATION = 3;
+    private static final int INDEX_TYPE = 3;
-    private static final int INDEX_KEY_ID_CERTIFIER = 4;
+    private static final int INDEX_CREATION = 4;
-    private static final int INDEX_UID_CERTIFIER = 5;
+    private static final int INDEX_EXPIRY = 5;
-    private static final int INDEX_KEY_DATA = 6;
+    private static final int INDEX_KEY_ID_CERTIFIER = 6;
-    private static final int INDEX_KEY_TYPE = 6;
+    private static final int INDEX_UID_CERTIFIER = 7;
+    private static final int INDEX_KEY_TYPE = 8;
 
     private Uri mDataUri;
 
@@ -133,30 +136,26 @@ public class ViewCertActivity extends ActionBarActivity
             else
                 mSignerUid.setText(R.string.unknown_uid);
 
-            byte[] sigData = data.getBlob(INDEX_KEY_DATA);
+            // String algorithmStr = PgpKeyHelper.getAlgorithmInfo(sig.getKeyAlgorithm(), 0);
-            PGPSignature sig = PgpConversionHelper.BytesToPGPSignature(sigData);
+            // mAlgorithm.setText(algorithmStr);
-            if(sig != null) {
-                String algorithmStr = PgpKeyHelper.getAlgorithmInfo(sig.getKeyAlgorithm(), 0);
-                mAlgorithm.setText(algorithmStr);
 
-                switch(sig.getSignatureType()) {
+            switch(data.getInt(INDEX_TYPE)) {
-                    case PGPSignature.DEFAULT_CERTIFICATION:
+                case PGPSignature.DEFAULT_CERTIFICATION:
-                        mType.setText(R.string.sig_type_default); break;
+                    mType.setText(R.string.sig_type_default); break;
-                    case PGPSignature.NO_CERTIFICATION:
+                case PGPSignature.NO_CERTIFICATION:
-                        mType.setText(R.string.sig_type_none); break;
+                    mType.setText(R.string.sig_type_none); break;
-                    case PGPSignature.CASUAL_CERTIFICATION:
+                case PGPSignature.CASUAL_CERTIFICATION:
-                        mType.setText(R.string.sig_type_casual); break;
+                    mType.setText(R.string.sig_type_casual); break;
-                    case PGPSignature.POSITIVE_CERTIFICATION:
+                case PGPSignature.POSITIVE_CERTIFICATION:
-                        mType.setText(R.string.sig_type_positive); break;
+                    mType.setText(R.string.sig_type_positive); break;
-                }
+            }
 
-                long expiry = sig.getHashedSubPackets().getSignatureExpirationTime();
+            long expiry = data.getLong(INDEX_EXPIRY);
-                if(expiry == 0)
+            if(expiry == 0)
-                    mExpiry.setText("never");
+                mExpiry.setText("never");
-                else {
+            else {
-                    Date expiryDate = new Date(creationDate.getTime() + expiry * 1000);
+                Date expiryDate = new Date(creationDate.getTime() + expiry * 1000);
-                    mExpiry.setText(DateFormat.getDateFormat(getApplicationContext()).format(expiryDate));
+                mExpiry.setText(DateFormat.getDateFormat(getApplicationContext()).format(expiryDate));
-                }
             }
         }
     }