respect user id revocation signatures

Closes #836

4 files changed, 84 insertions, 34 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java
index 4d4c0e5d1..c0bc7769b 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSignature.java
@@ -20,6 +20,8 @@ package org.sufficientlysecure.keychain.pgp;
 
 import org.spongycastle.bcpg.SignatureSubpacket;
 import org.spongycastle.bcpg.SignatureSubpacketTags;
+import org.spongycastle.bcpg.sig.Exportable;
+import org.spongycastle.bcpg.sig.Revocable;
 import org.spongycastle.bcpg.sig.RevocationReason;
 import org.spongycastle.openpgp.PGPException;
 import org.spongycastle.openpgp.PGPObjectFactory;
@@ -218,12 +220,23 @@ public class WrappedSignature {
         return new WrappedSignature(signatures.get(0));
     }
 
+    /** Returns true if this certificate is revocable in general. */
+    public boolean isRevokable () {
+        // If nothing is specified, the packet is considered revocable
+        if (mSig.getHashedSubPackets() == null
+                || !mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.REVOCABLE)) {
+            return true;
+        }
+        SignatureSubpacket p = mSig.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.REVOCABLE);
+        return ((Revocable) p).isRevocable();
+    }
+
     public boolean isLocal() {
         if (mSig.getHashedSubPackets() == null
                 || !mSig.getHashedSubPackets().hasSubpacket(SignatureSubpacketTags.EXPORTABLE)) {
             return false;
         }
         SignatureSubpacket p = mSig.getHashedSubPackets().getSubpacket(SignatureSubpacketTags.EXPORTABLE);
-        return p.getData()[0] == 0;
+        return ((Exportable) p).isExportable();
     }
 }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
index c29a67215..8d790110d 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java
@@ -450,47 +450,67 @@ public class ProviderHelper {
                 for (WrappedSignature cert : new IterableIterator<WrappedSignature>(
                         masterKey.getSignaturesForRawId(rawUserId))) {
                     long certId = cert.getKeyId();
+                    // self signature
+                    if (certId == masterKeyId) {
+
+                        // NOTE self-certificates are already verified during canonicalization,
+                        // AND we know there is at most one cert plus at most one revocation
+                        if (!cert.isRevocation()) {
+                            item.selfCert = cert;
+                            item.isPrimary = cert.isPrimaryUserId();
+                        } else {
+                            item.isRevoked = true;
+                            log(LogType.MSG_IP_UID_REVOKED);
+                        }
+                        continue;
+
+                    }
+
+                    // do we have a trusted key for this?
+                    if (trustedKeys.indexOfKey(certId) < 0) {
+                        unknownCerts += 1;
+                        continue;
+                    }
+
+                    // verify signatures from known private keys
+                    CanonicalizedPublicKey trustedKey = trustedKeys.get(certId);
+
                     try {
-                        // self signature
-                        if (certId == masterKeyId) {
-
-                            // NOTE self-certificates are already verified during canonicalization,
-                            // AND we know there is at most one cert plus at most one revocation
-                            if (!cert.isRevocation()) {
-                                item.selfCert = cert;
-                                item.isPrimary = cert.isPrimaryUserId();
-                            } else {
-                                item.isRevoked = true;
-                                log(LogType.MSG_IP_UID_REVOKED);
-                            }
+                        cert.init(trustedKey);
+                        // if it doesn't certify, leave a note and skip
+                        if ( ! cert.verifySignature(masterKey, rawUserId)) {
+                            log(LogType.MSG_IP_UID_CERT_BAD);
                             continue;
-
                         }
 
-                        // verify signatures from known private keys
-                        if (trustedKeys.indexOfKey(certId) >= 0) {
-                            CanonicalizedPublicKey trustedKey = trustedKeys.get(certId);
-                            if (cert.isRevocation()) {
-                                // skip for now
+                        log(cert.isRevocation()
+                                        ? LogType.MSG_IP_UID_CERT_GOOD_REVOKE
+                                        : LogType.MSG_IP_UID_CERT_GOOD,
+                                KeyFormattingUtils.convertKeyIdToHexShort(trustedKey.getKeyId())
+                        );
+
+                        // check if there is a previous certificate
+                        WrappedSignature prev = item.trustedCerts.get(cert.getKeyId());
+                        if (prev != null) {
+                            // if it's newer, skip this one
+                            if (prev.getCreationTime().after(cert.getCreationTime())) {
+                                log(LogType.MSG_IP_UID_CERT_OLD);
                                 continue;
                             }
-                            cert.init(trustedKey);
-                            if (cert.verifySignature(masterKey, rawUserId)) {
-                                item.trustedCerts.add(cert);
-                                log(LogType.MSG_IP_UID_CERT_GOOD,
-                                        KeyFormattingUtils.convertKeyIdToHexShort(trustedKey.getKeyId())
-                                );
-                            } else {
-                                log(LogType.MSG_IP_UID_CERT_BAD);
+                            // if the previous one was a non-revokable certification, no need to look further
+                            if (!prev.isRevocation() && !prev.isRevokable()) {
+                                log(LogType.MSG_IP_UID_CERT_NONREVOKE);
+                                continue;
                             }
+                            log(LogType.MSG_IP_UID_CERT_NEW);
                         }
-
-                        unknownCerts += 1;
+                        item.trustedCerts.put(cert.getKeyId(), cert);
 
                     } catch (PgpGeneralException e) {
                         log(LogType.MSG_IP_UID_CERT_ERROR,
                                 KeyFormattingUtils.convertKeyIdToHex(cert.getKeyId()));
                     }
+
                 }
 
                 if (unknownCerts > 0) {
@@ -518,9 +538,18 @@ public class ProviderHelper {
                 if (item.isRevoked) {
                     continue;
                 }
-                for (int i = 0; i < item.trustedCerts.size(); i++) {
+
+                // iterate over signatures
+                for (int i = 0; i < item.trustedCerts.size() ; i++) {
+                    WrappedSignature sig = item.trustedCerts.valueAt(i);
+                    // if it's a revocation
+                    if (sig.isRevocation()) {
+                        // don't further process it
+                        continue;
+                    }
+                    // otherwise, build database operation
                     operations.add(buildCertOperations(
-                            masterKeyId, userIdRank, item.trustedCerts.get(i), Certs.VERIFIED_SECRET));
+                            masterKeyId, userIdRank, sig, Certs.VERIFIED_SECRET));
                 }
             }
 
@@ -568,7 +597,7 @@ public class ProviderHelper {
         boolean isPrimary = false;
         boolean isRevoked = false;
         WrappedSignature selfCert;
-        List<WrappedSignature> trustedCerts = new ArrayList<WrappedSignature>();
+        LongSparseArray<WrappedSignature> trustedCerts = new LongSparseArray<WrappedSignature>();
 
         @Override
         public int compareTo(UserIdItem o) {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java
index 0e883c79b..cd9c29996 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/results/OperationResult.java
@@ -264,8 +264,12 @@ public abstract class OperationResult implements Parcelable {
         MSG_IP_SUCCESS (LogLevel.OK, R.string.msg_ip_success),
         MSG_IP_SUCCESS_IDENTICAL (LogLevel.OK, R.string.msg_ip_success_identical),
         MSG_IP_UID_CERT_BAD (LogLevel.WARN, R.string.msg_ip_uid_cert_bad),
-        MSG_IP_UID_CERT_ERROR (LogLevel.ERROR, R.string.msg_ip_uid_cert_error),
+        MSG_IP_UID_CERT_ERROR (LogLevel.WARN, R.string.msg_ip_uid_cert_error),
+        MSG_IP_UID_CERT_OLD (LogLevel.DEBUG, R.string.msg_ip_uid_cert_old),
+        MSG_IP_UID_CERT_NONREVOKE (LogLevel.DEBUG, R.string.msg_ip_uid_cert_nonrevoke),
+        MSG_IP_UID_CERT_NEW (LogLevel.DEBUG, R.string.msg_ip_uid_cert_new),
         MSG_IP_UID_CERT_GOOD (LogLevel.DEBUG, R.string.msg_ip_uid_cert_good),
+        MSG_IP_UID_CERT_GOOD_REVOKE (LogLevel.DEBUG, R.string.msg_ip_uid_cert_good_revoke),
         MSG_IP_UID_CERTS_UNKNOWN (LogLevel.DEBUG, R.plurals.msg_ip_uid_certs_unknown),
         MSG_IP_UID_CLASSIFYING_ZERO (LogLevel.DEBUG, R.string.msg_ip_uid_classifying_zero),
         MSG_IP_UID_CLASSIFYING (LogLevel.DEBUG, R.plurals.msg_ip_uid_classifying),
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index 63292ce71..5d79441cf 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -580,7 +580,11 @@
     <string name="msg_ip_reinsert_secret">"Re-inserting secret key"</string>
     <string name="msg_ip_uid_cert_bad">"Encountered bad certificate!"</string>
     <string name="msg_ip_uid_cert_error">"Error processing certificate!"</string>
-    <string name="msg_ip_uid_cert_good">"User id is certified by %1$s"</string>
+    <string name="msg_ip_uid_cert_nonrevoke">"Already have a non-revokable certificate, skipping."</string>
+    <string name="msg_ip_uid_cert_old">"Certificate is older than previous, skipping."</string>
+    <string name="msg_ip_uid_cert_new">"Certificate is more recent, replacing previous."</string>
+    <string name="msg_ip_uid_cert_good">"Found good certificate by %1$s"</string>
+    <string name="msg_ip_uid_cert_good_revoke">"Found good certificate revocation by %1$s"</string>
     <plurals name="msg_ip_uid_certs_unknown">
         <item quantity="one">"Ignoring one certificate issued by an unknown public key"</item>
         <item quantity="other">"Ignoring %s certificates issued by unknown public keys"</item>