consolidate: add key import routines with consolidation

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpImportExport.java

@@ -149,7 +149,8 @@ public class PgpImportExport {
 
                 SaveKeyringResult result;
                 if (key.isSecret()) {
-                    result = mProviderHelper.saveSecretKeyRing(key);
+                    result = mProviderHelper.saveSecretKeyRing(key,
+                            new ProgressScaler(mProgressable, position, (position+1)*progSteps, 100));
                 } else {
                     result = mProviderHelper.savePublicKeyRing(key,
                             new ProgressScaler(mProgressable, position, (position+1)*progSteps, 100));

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java

@@ -58,10 +58,18 @@ public class UncachedKeyRing {
 
     final PGPKeyRing mRing;
     final boolean mIsSecret;
+    final boolean mIsCanonicalized;
 
     UncachedKeyRing(PGPKeyRing ring) {
         mRing = ring;
         mIsSecret = ring instanceof PGPSecretKeyRing;
+        mIsCanonicalized = false;
+    }
+
+    private UncachedKeyRing(PGPKeyRing ring, boolean canonicalized) {
+        mRing = ring;
+        mIsSecret = ring instanceof PGPSecretKeyRing;
+        mIsCanonicalized = canonicalized;
     }
 
     public long getMasterKeyId() {
@@ -92,6 +100,10 @@ public class UncachedKeyRing {
         return mIsSecret;
     }
 
+    public boolean isCanonicalized() {
+        return mIsCanonicalized;
+    }
+
     public byte[] getEncoded() throws IOException {
         return mRing.getEncoded();
     }
@@ -606,7 +618,7 @@ public class UncachedKeyRing {
             log.add(LogLevel.OK, LogType.MSG_KC_SUCCESS, null, indent);
         }
 
-        return new UncachedKeyRing(ring);
+        return new UncachedKeyRing(ring, true);
     }
 
     /** This operation merges information from a different keyring, returning a combined
@@ -688,6 +700,11 @@ public class UncachedKeyRing {
                         continue;
                     }
 
+                    // Don't merge foreign stuff into secret keys
+                    if (cert.getKeyID() != masterKeyId && isSecret()) {
+                        continue;
+                    }
+
                     byte[] encoded = cert.getEncoded();
                     // Known cert, skip it
                     if (certs.contains(encoded)) {
@@ -709,6 +726,10 @@ public class UncachedKeyRing {
                 // Copy over all user id certificates
                 for (String userId : new IterableIterator<String>(key.getUserIDs())) {
                     for (PGPSignature cert : new IterableIterator<PGPSignature>(key.getSignaturesForID(userId))) {
+                        // Don't merge foreign stuff into secret keys
+                        if (cert.getKeyID() != masterKeyId && isSecret()) {
+                            continue;
+                        }
                         byte[] encoded = cert.getEncoded();
                         // Known cert, skip it
                         if (certs.contains(encoded)) {

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedKeyRing.java

@@ -101,4 +101,8 @@ public abstract class WrappedKeyRing extends KeyRing {
 
     abstract public IterableIterator<WrappedPublicKey> publicKeyIterator();
 
+    public UncachedKeyRing getUncached() {
+        return new UncachedKeyRing(getRing());
+    }
+
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedSecretKeyRing.java

@@ -154,8 +154,4 @@ public class WrappedSecretKeyRing extends WrappedKeyRing {
         });
     }
 
-    public UncachedKeyRing getUncached() {
-        return new UncachedKeyRing(mRing);
-    }
-
 }

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/ProviderHelper.java

@@ -57,6 +57,7 @@ import org.sufficientlysecure.keychain.util.Log;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
@@ -275,21 +276,15 @@ public class ProviderHelper {
             log(LogLevel.ERROR, LogType.MSG_IP_BAD_TYPE_SECRET);
             return SaveKeyringResult.RESULT_ERROR;
         }
+        if (!keyRing.isCanonicalized()) {
+            log(LogLevel.ERROR, LogType.MSG_IP_BAD_TYPE_SECRET);
+            return SaveKeyringResult.RESULT_ERROR;
+        }
 
         // start with ok result
         int result = SaveKeyringResult.SAVED_PUBLIC;
 
         long masterKeyId = keyRing.getMasterKeyId();
-        log(LogLevel.START, LogType.MSG_IP,
-                new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
-        mIndent += 1;
-
-        // Canonicalize this key, to assert a number of assumptions made about it.
-        keyRing = keyRing.canonicalize(mLog, mIndent);
-        if (keyRing == null) {
-            return SaveKeyringResult.RESULT_ERROR;
-        }
-
         UncachedPublicKey masterKey = keyRing.getPublicKey();
 
         ArrayList<ContentProviderOperation> operations;
@@ -569,11 +564,16 @@ public class ProviderHelper {
     /** Saves an UncachedKeyRing of the secret variant into the db.
      * This method will fail if no corresponding public keyring is in the database!
      */
-    private SaveKeyringResult internalSaveSecretKeyRing(UncachedKeyRing keyRing) {
+    private int internalSaveSecretKeyRing(UncachedKeyRing keyRing) {
 
         if (!keyRing.isSecret()) {
             log(LogLevel.ERROR, LogType.MSG_IS_BAD_TYPE_PUBLIC);
-            return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+            return SaveKeyringResult.RESULT_ERROR;
+        }
+
+        if (!keyRing.isCanonicalized()) {
+            log(LogLevel.ERROR, LogType.MSG_IS_BAD_TYPE_PUBLIC);
+            return SaveKeyringResult.RESULT_ERROR;
         }
 
         long masterKeyId = keyRing.getMasterKeyId();
@@ -584,7 +584,7 @@ public class ProviderHelper {
         // Canonicalize this key, to assert a number of assumptions made about it.
         keyRing = keyRing.canonicalize(mLog, mIndent);
         if (keyRing == null) {
-            return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+            return SaveKeyringResult.RESULT_ERROR;
         }
 
         // IF this is successful, it's a secret key
@@ -599,12 +599,12 @@ public class ProviderHelper {
             Uri uri = KeyRingData.buildSecretKeyRingUri(Long.toString(masterKeyId));
             if (mContentResolver.insert(uri, values) == null) {
                 log(LogLevel.ERROR, LogType.MSG_IS_DB_EXCEPTION);
-                return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                return SaveKeyringResult.RESULT_ERROR;
             }
         } catch (IOException e) {
             Log.e(Constants.TAG, "Failed to encode key!", e);
             log(LogLevel.ERROR, LogType.MSG_IS_IO_EXCPTION);
-            return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+            return SaveKeyringResult.RESULT_ERROR;
         }
 
         {
@@ -648,11 +648,12 @@ public class ProviderHelper {
         }
 
         log(LogLevel.OK, LogType.MSG_IS_SUCCESS);
-        return new SaveKeyringResult(result, mLog);
+        return result;
 
     }
 
 
+    @Deprecated
     public SaveKeyringResult savePublicKeyRing(UncachedKeyRing keyRing) {
         return savePublicKeyRing(keyRing, new Progressable() {
             @Override
@@ -669,54 +670,176 @@ public class ProviderHelper {
         });
     }
 
-    public SaveKeyringResult savePublicKeyRing(UncachedKeyRing keyRing, Progressable progress) {
+    /** Save a public keyring into the database.
+     *
+     * This is a high level method, which takes care of merging all new information into the old and
+     * keep public and secret keyrings in sync.
+     */
+    public SaveKeyringResult savePublicKeyRing(UncachedKeyRing publicRing, Progressable progress) {
 
-        // IF there is a secret key, preserve it!
-        UncachedKeyRing secretRing;
         try {
-            secretRing = getWrappedSecretKeyRing(keyRing.getMasterKeyId()).getUncached();
+            long masterKeyId = publicRing.getMasterKeyId();
-            log(LogLevel.DEBUG, LogType.MSG_IP_PRESERVING_SECRET);
+            log(LogLevel.START, LogType.MSG_IP,
-            progress.setProgress(LogType.MSG_IP_PRESERVING_SECRET.getMsgId(), 10, 100);
+                    new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
+
             mIndent += 1;
 
-            // Merge data from new public ring into secret one
+            // IF there is a secret key, preserve it!
-            secretRing = secretRing.merge(keyRing, mLog, mIndent);
+            try {
+                UncachedKeyRing oldPublicRing = getWrappedPublicKeyRing(masterKeyId).getUncached();
+
+                // Merge data from new public ring into the old one
+                publicRing = oldPublicRing.merge(publicRing, mLog, mIndent);
+
+                if (publicRing == null) {
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                }
+
+                // Early breakout if nothing changed
+                if (Arrays.hashCode(publicRing.getEncoded())
+                        == Arrays.hashCode(oldPublicRing.getEncoded())) {
+                    log(LogLevel.OK, LogType.MSG_IP,
+                            new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_OK, mLog);
+                }
+            } catch (NotFoundException e) {
+                // not an issue, just means we are dealing with a new keyring
+            }
+
+            // Canonicalize this keyring, to assert a number of assumptions made about it.
+            publicRing = publicRing.canonicalize(mLog, mIndent);
+            if (publicRing == null) {
+                return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+            }
+
+            // IF there is a secret key, preserve it!
+            UncachedKeyRing secretRing;
+            try {
+                secretRing = getWrappedSecretKeyRing(publicRing.getMasterKeyId()).getUncached();
+                log(LogLevel.DEBUG, LogType.MSG_IP_PRESERVING_SECRET);
+                progress.setProgress(LogType.MSG_IP_PRESERVING_SECRET.getMsgId(), 10, 100);
+                mIndent += 1;
+
+                // Merge data from new public ring into secret one
+                secretRing = secretRing.merge(publicRing, mLog, mIndent);
+                if (secretRing == null) {
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                }
+
+                mIndent -= 1;
+
+            } catch (NotFoundException e) {
+                secretRing = null;
+            }
+
+            int result = internalSavePublicKeyRing(publicRing, progress, secretRing != null);
+
+            // Save the saved keyring (if any)
+            if (secretRing != null) {
+                log(LogLevel.DEBUG, LogType.MSG_IP_REINSERT_SECRET);
+                progress.setProgress(LogType.MSG_IP_REINSERT_SECRET.getMsgId(), 90, 100);
+                mIndent += 1;
+                secretRing = secretRing.canonicalize(mLog, mIndent);
+                internalSaveSecretKeyRing(secretRing);
+                result |= SaveKeyringResult.SAVED_SECRET;
+                mIndent -= 1;
+            }
+
+            return new SaveKeyringResult(result, mLog);
+
+        } catch (IOException e) {
+            return null;
+        }
+
+    }
+
+    public SaveKeyringResult saveSecretKeyRing(UncachedKeyRing secretRing, Progressable progress) {
+
+        try {
+            long masterKeyId = secretRing.getMasterKeyId();
+            log(LogLevel.START, LogType.MSG_IS,
+                    new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
+
+            mIndent += 1;
+
+            // If there is a secret key, merge it.
+            try {
+                UncachedKeyRing oldSecretRing = getWrappedSecretKeyRing(masterKeyId).getUncached();
+
+                // Merge data from new public ring into the old one
+                secretRing = oldSecretRing.merge(secretRing, mLog, mIndent);
+
+                if (secretRing == null) {
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                }
+
+                // Early breakout if nothing changed
+                if (Arrays.hashCode(secretRing.getEncoded())
+                        == Arrays.hashCode(oldSecretRing.getEncoded())) {
+                    log(LogLevel.OK, LogType.MSG_IS,
+                            new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_OK, mLog);
+                }
+            } catch (NotFoundException e) {
+                // not an issue, just means we are dealing with a new keyring
+            }
+
+            // Canonicalize this keyring, to assert a number of assumptions made about it.
+            secretRing = secretRing.canonicalize(mLog, mIndent);
             if (secretRing == null) {
                 return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
             }
 
-            // NOTE that the info from this secret keyring will implicitly be merged into the
+            // Merge new data into public keyring as well, if there is any
-            // new public keyring, since that one is merged with the old public keyring.
+            try {
+                UncachedKeyRing oldPublicRing = getWrappedPublicKeyRing(masterKeyId).getUncached();
+                log(LogLevel.DEBUG, LogType.MSG_IP_PRESERVING_SECRET);
+                progress.setProgress(LogType.MSG_IP_PRESERVING_SECRET.getMsgId(), 10, 100);
+                mIndent += 1;
 
-            mIndent -= 1;
+                // Merge data from new public ring into secret one
+                UncachedKeyRing publicRing = oldPublicRing.merge(secretRing, mLog, mIndent);
+                if (publicRing == null) {
+                    return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                }
 
-        } catch (NotFoundException e) {
+                // Early breakout if nothing changed
-            secretRing = null;
+                if (Arrays.hashCode(publicRing.getEncoded())
-        }
+                        != Arrays.hashCode(oldPublicRing.getEncoded())) {
 
-        int result = internalSavePublicKeyRing(keyRing, progress, secretRing != null);
+                    log(LogLevel.OK, LogType.MSG_IS,
+                            new String[]{ PgpKeyHelper.convertKeyIdToHex(masterKeyId) });
+
+                    publicRing = publicRing.canonicalize(mLog, mIndent);
+                    if (publicRing == null) {
+                        return new SaveKeyringResult(SaveKeyringResult.RESULT_ERROR, mLog);
+                    }
+
+                    int result = internalSavePublicKeyRing(publicRing, progress, true);
+
+                }
+
+                mIndent -= 1;
+
+            } catch (NotFoundException e) {
+                // TODO, this WILL error out later because secret rings cannot be inserted without
+                // public ones
+            }
 
-        // Save the saved keyring (if any)
-        if (secretRing != null) {
-            log(LogLevel.DEBUG, LogType.MSG_IP_REINSERT_SECRET);
             progress.setProgress(LogType.MSG_IP_REINSERT_SECRET.getMsgId(), 90, 100);
-            mIndent += 1;
+            int result = internalSaveSecretKeyRing(secretRing);
-            internalSaveSecretKeyRing(secretRing);
+            return new SaveKeyringResult(result, mLog);
-            result |= SaveKeyringResult.SAVED_SECRET;
+
-            mIndent -= 1;
+        } catch (IOException e) {
+            return null;
         }
 
-        return new SaveKeyringResult(result, mLog);
-
-    }
-
-    public SaveKeyringResult saveSecretKeyRing(UncachedKeyRing keyRing) {
-        return internalSaveSecretKeyRing(keyRing);
     }
 
     /**
      * Saves (or updates) a pair of public and secret KeyRings in the database
      */
+    @Deprecated // scheduled for deletion after merge with new-edit branch
     public void savePairedKeyRing(UncachedKeyRing pubRing, UncachedKeyRing secRing) throws IOException {
         long masterKeyId = pubRing.getMasterKeyId();
 

OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java

@@ -513,7 +513,7 @@ public class KeychainIntentService extends IntentService
                     UncachedKeyRing newKeyRing =
                             keyRing.changeSecretKeyPassphrase(oldPassphrase, newPassphrase);
                     setProgress(R.string.progress_saving_key_ring, 50, 100);
-                    providerHelper.saveSecretKeyRing(newKeyRing);
+                    // providerHelper.saveSecretKeyRing(newKeyRing);
                     setProgress(R.string.progress_done, 100, 100);
                 } else {
                     PgpKeyOperation keyOperations = new PgpKeyOperation(new ProgressScaler(this, 0, 90, 100));

OpenKeychain/src/main/res/values/strings.xml

@@ -583,7 +583,7 @@
     <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_success">Keyring canonicalization successful</string>
+    <string name="msg_kc_success">Keyring canonicalization successful, no changes</string>
     <string name="msg_kc_success_bad">Keyring canonicalization successful, removed %s erroneous certificates</string>
     <string name="msg_kc_success_bad_and_red">Keyring canonicalization successful, removed %1$s erroneous and %2$s redundant certificates</string>
     <string name="msg_kc_success_redundant">Keyring canonicalization successful, removed %s redundant certificates</string>
@@ -600,8 +600,8 @@
 
 
     <!-- Keyring merging log entries -->
-    <string name="msg_mg_public">Merging into secret keyring %s</string>
+    <string name="msg_mg_public">Merging into public keyring %s</string>
-    <string name="msg_mg_secret">Merging into public keyring %s</string>
+    <string name="msg_mg_secret">Merging into secret keyring %s</string>
     <string name="msg_mg_fatal_encode">Fatal error encoding signature</string>
     <string name="msg_mg_heterogeneous">Tried to consolidate heterogeneous keyrings</string>
     <string name="msg_mg_new_subkey">Adding new subkey %s</string>