Always revoke subkeys with a revocation signature.

Unlike UID revocations which are "reversible" by newer UID
self-signatures, a subkey revocation should be "permanent" even if
followed by a newer self-signature.

The RFC is ambiguous on this, but this is the convention used by (e.g.)
GnuPG. The rationale for this behaviour is available as comments within
the GnuPG source.

UID signatures:
https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1668-L1674
Subkey signatures:
https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1990-L1997

1 files changed, 33 insertions, 12 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
index c967a5abc..af09cf235 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
@@ -250,8 +250,21 @@ public class UncachedKeyRing {
      *  - Remove all non-verifying self-certificates
      *  - Remove all "future" self-certificates
      *  - Remove all certificates flagged as "local"
-     *  - Remove all certificates which are superseded by a newer one on the same target,
-     *      including revocations with later re-certifications.
+     *  - For UID certificates, remove all certificates which are
+     *      superseded by a newer one on the same target, including
+     *      revocations with later re-certifications.
+     *  - For subkey certifications, remove all certificates which
+     *      are superseded by a newer one on the same target, unless
+     *      it encounters a revocation certificate. The revocation
+     *      certificate is considered to permanently revoke the key,
+     *      even if contains later re-certifications.
+     *  This is the "behavior in practice" used by (e.g.) GnuPG, and
+     *  the rationale for both can be found as comments in the GnuPG
+     *  source.
+     *  UID signatures:
+     *  https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1668-L1674
+     *  Subkey signatures:
+     *  https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1990-L1997
      *  - Remove all certificates in other positions if not of known type:
      *   - key revocation signatures on the master key
      *   - subkey binding signatures for subkeys
@@ -278,8 +291,21 @@ public class UncachedKeyRing {
      *  - Remove all non-verifying self-certificates
      *  - Remove all "future" self-certificates
      *  - Remove all certificates flagged as "local"
-     *  - Remove all certificates which are superseded by a newer one on the same target,
-     *      including revocations with later re-certifications.
+     *  - For UID certificates, remove all certificates which are
+     *      superseded by a newer one on the same target, including
+     *      revocations with later re-certifications.
+     *  - For subkey certifications, remove all certificates which
+     *      are superseded by a newer one on the same target, unless
+     *      it encounters a revocation certificate. The revocation
+     *      certificate is considered to permanently revoke the key,
+     *      even if contains later re-certifications.
+     *  This is the "behavior in practice" used by (e.g.) GnuPG, and
+     *  the rationale for both can be found as comments in the GnuPG
+     *  source.
+     *  UID signatures:
+     *  https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1668-L1674
+     *  Subkey signatures:
+     *  https://github.com/mtigas/gnupg/blob/50c98c7ed6b542857ee2f902eca36cda37407737/g10/getkey.c#L1990-L1997
      *  - Remove all certificates in other positions if not of known type:
      *   - key revocation signatures on the master key
      *   - subkey binding signatures for subkeys
@@ -950,12 +976,6 @@ public class UncachedKeyRing {
                     }
 
                     selfCert = zert;
-                    // if this is newer than a possibly existing revocation, drop that one
-                    if (revocation != null && selfCert.getCreationTime().after(revocation.getCreationTime())) {
-                        log.add(LogType.MSG_KC_SUB_REVOKE_DUP, indent);
-                        redundantCerts += 1;
-                        revocation = null;
-                    }
 
                 // it must be a revocation, then (we made sure above)
                 } else {
@@ -974,8 +994,9 @@ public class UncachedKeyRing {
                         continue;
                     }
 
-                    // if there is a certification that is newer than this revocation, don't bother
-                    if (selfCert != null && selfCert.getCreationTime().after(cert.getCreationTime())) {
+                    // If we already have a newer revocation cert, skip this one.
+                    if (revocation != null &&
+                        revocation.getCreationTime().after(cert.getCreationTime())) {
                         log.add(LogType.MSG_KC_SUB_REVOKE_DUP, indent);
                         redundantCerts += 1;
                         continue;