canonicalize: more stuff

5 files changed, 70 insertions, 47 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ParcelableKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ParcelableKeyRing.java
index 5da6c4cd3..fdf561aaf 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ParcelableKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/ParcelableKeyRing.java
@@ -3,7 +3,7 @@ package org.sufficientlysecure.keychain.keyimport;
 import android.os.Parcel;
 import android.os.Parcelable;
 
-/** This is a trivial wrapper around UncachedKeyRing which implements Parcelable. It exists
+/** This is a trivial wrapper around keyring bytes which implements Parcelable. It exists
  * for the sole purpose of keeping spongycastle and android imports in separate packages.
  */
 public class ParcelableKeyRing implements Parcelable {
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 215c17590..a8e4820cf 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java
@@ -15,7 +15,6 @@ import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException;
 import org.sufficientlysecure.keychain.service.OperationResultParcel.OperationLog;
 import org.sufficientlysecure.keychain.service.OperationResultParcel.LogLevel;
 import org.sufficientlysecure.keychain.service.OperationResultParcel.LogType;
-import org.sufficientlysecure.keychain.service.OperationResults.SaveKeyringResult;
 import org.sufficientlysecure.keychain.util.IterableIterator;
 import org.sufficientlysecure.keychain.util.Log;
 
@@ -24,7 +23,6 @@ import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
@@ -229,7 +227,7 @@ public class UncachedKeyRing {
 
                 if (type != PGPSignature.KEY_REVOCATION) {
                     // Unknown type, just remove
-                    log.add(LogLevel.WARN, LogType.MSG_KC_CERT_BAD_TYPE, new String[]{
+                    log.add(LogLevel.WARN, LogType.MSG_KC_SUB_BAD_TYPE, new String[]{
                             "0x" + Integer.toString(type, 16)
                     }, indent);
                     modified = PGPPublicKey.removeCertification(modified, zert);
@@ -380,81 +378,98 @@ public class UncachedKeyRing {
         }
 
         // Process all keys
-        // NOTE we rely here on the special property that this iterator is independent from the keyring!
         for (PGPPublicKey key : new IterableIterator<PGPPublicKey>(ring.getPublicKeys())) {
-            // Only care about the master key here!
+            // Don't care about the master key here, that one gets special treatment above
             if (key.isMasterKey()) {
                 continue;
             }
-            log.add(LogLevel.DEBUG, LogType.MSG_KC_SUBKEY,
+            log.add(LogLevel.DEBUG, LogType.MSG_KC_SUB,
                     new String[]{PgpKeyHelper.convertKeyIdToHex(key.getKeyID())}, indent);
             indent += 1;
             // A subkey needs exactly one subkey binding certificate, and optionally one revocation
             // certificate.
             PGPPublicKey modified = key;
-            PGPSignature cert = null, revocation = null;
+            PGPSignature selfCert = null, revocation = null;
             for (PGPSignature zig : new IterableIterator<PGPSignature>(key.getSignatures())) {
                 // remove from keyring (for now)
                 modified = PGPPublicKey.removeCertification(modified, zig);
-                WrappedSignature sig = new WrappedSignature(zig);
-                int type = sig.getSignatureType();
+                WrappedSignature cert = new WrappedSignature(zig);
+                int type = cert.getSignatureType();
 
                 // filter out bad key types...
-                if (sig.getKeyId() != masterKey.getKeyID()) {
-                    log.add(LogLevel.WARN, LogType.MSG_KC_CERT_BAD_KEYID, null, indent);
+                if (cert.getKeyId() != masterKey.getKeyID()) {
+                    log.add(LogLevel.WARN, LogType.MSG_KC_SUB_BAD_KEYID, null, indent);
                     continue;
                 }
                 if (type != PGPSignature.SUBKEY_BINDING && type != PGPSignature.SUBKEY_REVOCATION) {
-                    log.add(LogLevel.WARN, LogType.MSG_KC_CERT_BAD_TYPE, new String[]{
+                    log.add(LogLevel.WARN, LogType.MSG_KC_SUB_BAD_TYPE, new String[]{
                             "0x" + Integer.toString(type, 16)
                     }, indent);
                     continue;
                 }
 
-                // make sure the certificate checks out
-                try {
-                    sig.init(masterKey);
-                    if (!sig.verifySignature(masterKey, key)) {
-                        log.add(LogLevel.WARN, LogType.MSG_KC_CERT_BAD, null, indent);
+                if (type == PGPSignature.SUBKEY_BINDING) {
+                    // TODO verify primary key binding signature for signing keys!
+
+                    // make sure the certificate checks out
+                    try {
+                        cert.init(masterKey);
+                        if (!cert.verifySignature(masterKey, key)) {
+                            log.add(LogLevel.WARN, LogType.MSG_KC_SUB_BAD, null, indent);
+                            log.add(LogLevel.WARN, LogType.MSG_KC_SUB, new String[] {
+                                cert.getCreationTime().toString()
+                            }, indent);
+                            continue;
+                        }
+                    } catch (PgpGeneralException e) {
+                        log.add(LogLevel.WARN, LogType.MSG_KC_SUB_BAD_ERR, null, indent);
                         continue;
                     }
-                } catch (PgpGeneralException e) {
-                    log.add(LogLevel.WARN, LogType.MSG_KC_CERT_BAD_ERR, null, indent);
-                    continue;
-                }
 
-                if (type == PGPSignature.SUBKEY_BINDING) {
-                    // TODO verify primary key binding signature for signing keys!
                     // if we already have a cert, and this one is not newer: skip it
-                    if (cert != null && cert.getCreationTime().before(sig.getCreationTime())) {
+                    if (selfCert != null && selfCert.getCreationTime().before(cert.getCreationTime())) {
                         continue;
                     }
-                    cert = zig;
+                    selfCert = zig;
                     // if this is newer than a possibly existing revocation, drop that one
-                    if (revocation != null && cert.getCreationTime().after(revocation.getCreationTime())) {
+                    if (revocation != null && selfCert.getCreationTime().after(revocation.getCreationTime())) {
                         revocation = null;
                     }
-                    // it must be a revocation, then (we made sure above)
+
+                // it must be a revocation, then (we made sure above)
                 } else {
+
+                    // make sure the certificate checks out
+                    try {
+                        cert.init(masterKey);
+                        if (!cert.verifySignature(key)) {
+                            log.add(LogLevel.WARN, LogType.MSG_KC_SUB_REVOKE_BAD, null, indent);
+                            continue;
+                        }
+                    } catch (PgpGeneralException e) {
+                        log.add(LogLevel.WARN, LogType.MSG_KC_SUB_REVOKE_BAD_ERR, null, indent);
+                        continue;
+                    }
+
                     // if there is no binding (yet), or the revocation is newer than the binding: keep it
-                    if (cert == null || cert.getCreationTime().before(sig.getCreationTime())) {
+                    if (selfCert == null || selfCert.getCreationTime().before(cert.getCreationTime())) {
                         revocation = zig;
                     }
                 }
             }
 
             // it is not properly bound? error!
-            if (cert == null) {
+            if (selfCert == null) {
                 ring = PGPPublicKeyRing.removePublicKey(ring, modified);
 
-                log.add(LogLevel.ERROR, LogType.MSG_KC_SUBKEY_NO_CERT,
+                log.add(LogLevel.ERROR, LogType.MSG_KC_SUB_NO_CERT,
                         new String[]{PgpKeyHelper.convertKeyIdToHex(key.getKeyID())}, indent);
                 indent -= 1;
                 continue;
             }
 
             // re-add certification
-            modified = PGPPublicKey.addCertification(modified, cert);
+            modified = PGPPublicKey.addCertification(modified, selfCert);
             // add revocation, if any
             if (revocation != null) {
                 modified = PGPPublicKey.addCertification(modified, revocation);
@@ -462,7 +477,7 @@ public class UncachedKeyRing {
             // replace pubkey in keyring
             ring = PGPPublicKeyRing.insertPublicKey(ring, modified);
 
-            log.add(LogLevel.DEBUG, LogType.MSG_KC_SUBKEY_SUCCESS, null, indent);
+            log.add(LogLevel.DEBUG, LogType.MSG_KC_SUB_SUCCESS, null, indent);
             indent -= 1;
         }
 
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
index 48f40026e..5c223e870 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/OperationResultParcel.java
@@ -158,18 +158,21 @@ public class OperationResultParcel implements Parcelable {
 
         // keyring canonicalization
         MSG_KC (R.string.msg_kc),
-        MSG_KC_CERT_BAD_ERR (R.string.msg_kc_cert_bad_err),
-        MSG_KC_CERT_BAD_KEYID (R.string.msg_kc_cert_bad_keyid),
-        MSG_KC_CERT_BAD (R.string.msg_kc_cert_bad),
-        MSG_KC_CERT_BAD_TYPE (R.string.msg_kc_cert_bad_type),
         MSG_KC_MASTER (R.string.msg_kc_master),
         MSG_KC_MASTER_SUCCESS (R.string.msg_kc_master_success),
         MSG_KC_REVOKE_BAD_ERR (R.string.msg_kc_revoke_bad_err),
         MSG_KC_REVOKE_BAD (R.string.msg_kc_revoke_bad),
         MSG_KC_REVOKE_DUP (R.string.msg_kc_revoke_dup),
-        MSG_KC_SUBKEY_NO_CERT (R.string.msg_kc_subkey_no_cert),
-        MSG_KC_SUBKEY (R.string.msg_kc_subkey),
-        MSG_KC_SUBKEY_SUCCESS (R.string.msg_kc_subkey_success),
+        MSG_KC_SUB (R.string.msg_kc_sub),
+        MSG_KC_SUB_BAD_ERR(R.string.msg_kc_sub_bad_err),
+        MSG_KC_SUB_BAD_KEYID(R.string.msg_kc_sub_bad_keyid),
+        MSG_KC_SUB_BAD(R.string.msg_kc_sub_bad),
+        MSG_KC_SUB_BAD_TYPE(R.string.msg_kc_sub_bad_type),
+        MSG_KC_SUB_NO_CERT(R.string.msg_kc_sub_no_cert),
+        MSG_KC_SUB_REVOKE_BAD_ERR (R.string.msg_kc_sub_revoke_bad_err),
+        MSG_KC_SUB_REVOKE_BAD (R.string.msg_kc_sub_revoke_bad),
+        MSG_KC_SUB_REVOKE_DUP (R.string.msg_kc_sub_revoke_dup),
+        MSG_KC_SUB_SUCCESS (R.string.msg_kc_sub_success),
         MSG_KC_SUCCESS_REMOVED (R.string.msg_kc_success_removed),
         MSG_KC_SUCCESS (R.string.msg_kc_success),
         MSG_KC_UID_BAD_ERR (R.string.msg_kc_uid_bad_err),
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/ImportKeysActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/ImportKeysActivity.java
index 7fa18406c..f389726ff 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/ImportKeysActivity.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/ImportKeysActivity.java
@@ -456,10 +456,12 @@ public class ImportKeysActivity extends ActionBarActivity implements ActionBar.O
                     }
                     */
 
+                    /*
                     if (ACTION_IMPORT_KEY_FROM_KEYSERVER_AND_RETURN.equals(getIntent().getAction())) {
                         ImportKeysActivity.this.setResult(Activity.RESULT_OK, mPendingIntentData);
                         finish();
                     }
+                    */
                 }
             }
         };
diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml
index 27568132b..bcf66d58f 100644
--- a/OpenKeychain/src/main/res/values/strings.xml
+++ b/OpenKeychain/src/main/res/values/strings.xml
@@ -559,18 +559,21 @@
 
     <!-- Keyring Canonicalization log entries -->
     <string name="msg_kc">Canonicalizing keyring %s</string>
-    <string name="msg_kc_cert_bad">Certificate verification failed!</string>
-    <string name="msg_kc_cert_bad_err">Certificate verification failed with an error!</string>
-    <string name="msg_kc_cert_bad_keyid">Certificate issuer id mismatch</string>
-    <string name="msg_kc_cert_bad_type">Unknown certificate type: %s</string>
     <string name="msg_kc_master">Processing master key</string>
     <string name="msg_kc_master_success">OK</string>
     <string name="msg_kc_revoke_bad_err">Removing bad keyring revocation key</string>
     <string name="msg_kc_revoke_bad">Removing bad keyring revocation key</string>
     <string name="msg_kc_revoke_dup">Removing redundant keyring revocation key</string>
-    <string name="msg_kc_subkey_no_cert">No valid certificate found for %s, removing from ring!</string>
-    <string name="msg_kc_subkey">Processing subkey %s</string>
-    <string name="msg_kc_subkey_success">OK</string>
+    <string name="msg_kc_sub">Processing subkey %s</string>
+    <string name="msg_kc_sub_bad">Subkey binding certificate verification failed!</string>
+    <string name="msg_kc_sub_bad_err">Subkey binding certificate verification failed with an error!</string>
+    <string name="msg_kc_sub_bad_keyid">Subkey binding issuer id mismatch</string>
+    <string name="msg_kc_sub_bad_type">Unknown subkey certificate type: %s</string>
+    <string name="msg_kc_sub_no_cert">No valid certificate found for %s, removing from ring!</string>
+    <string name="msg_kc_sub_revoke_bad_err">Removing bad subkey revocation key</string>
+    <string name="msg_kc_sub_revoke_bad">Removing bad subkey revocation key</string>
+    <string name="msg_kc_sub_revoke_dup">Removing redundant keyring revocation key</string>
+    <string name="msg_kc_sub_success">OK</string>
     <string name="msg_kc_success">Keyring canonicalization successful</string>
     <string name="msg_kc_success_removed">Keyring canonicalization successful, removed %s certificates</string>
     <string name="msg_kc_uid_bad_err">Removing bad self certificate for user id %s</string>