From b1ea1261425e05d7eaa803e6ea72c1f0bbb5ae32 Mon Sep 17 00:00:00 2001
From: Vincent Breitmoser <valodim@mugenguild.com>
Date: Mon, 1 Feb 2016 15:22:36 +0100
Subject: performance: avoid expensive getSecretKeyType call, use cached where
 possible

---
 .../keychain/pgp/CanonicalizedSecretKey.java       |   4 +-
 .../keychain/pgp/CanonicalizedSecretKeyRing.java   |  14 --
 .../keychain/pgp/PgpDecryptVerifyOperation.java    | 142 +++++++++++----------
 .../keychain/pgp/PgpSignEncryptOperation.java      |  19 +--
 4 files changed, 86 insertions(+), 93 deletions(-)

(limited to 'OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp')

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
index 7f2a00617..95a0d41cc 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKey.java
@@ -120,7 +120,9 @@ public class CanonicalizedSecretKey extends CanonicalizedPublicKey {
 
     }
 
-    public SecretKeyType getSecretKeyType() {
+    // This method can potentially take a LONG time (i.e. seconds), so it should only
+    // ever be called by ProviderHelper to be cached in the database.
+    public SecretKeyType getSecretKeyTypeSuperExpensive() {
         S2K s2k = mSecretKey.getS2K();
         if (s2k != null && s2k.getType() == S2K.GNU_DUMMY_S2K) {
             // divert to card is special
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java
index 97b5fa6fe..1df16254f 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java
@@ -70,20 +70,6 @@ public class CanonicalizedSecretKeyRing extends CanonicalizedKeyRing {
         return new CanonicalizedSecretKey(this, mRing.getSecretKey(id));
     }
 
-    /** Returns the key id which should be used for signing.
-     *
-     * This method returns keys which are actually available (ie. secret available, and not stripped,
-     * revoked, or expired), hence only works on keyrings where a secret key is available!
-     */
-    public long getSecretSignId() throws PgpGeneralException {
-        for(CanonicalizedSecretKey key : secretKeyIterator()) {
-            if (key.canSign() && key.isValid() && key.getSecretKeyType().isUsable()) {
-                return key.getKeyId();
-            }
-        }
-        throw new PgpGeneralException("no valid signing key available");
-    }
-
     public IterableIterator<CanonicalizedSecretKey> secretKeyIterator() {
         final Iterator<PGPSecretKey> it = mRing.getSecretKeys();
         return new IterableIterator<>(new Iterator<CanonicalizedSecretKey>() {
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java
index 79a7a8fe1..3fc020aa7 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpDecryptVerifyOperation.java
@@ -64,6 +64,8 @@ import org.sufficientlysecure.keychain.operations.results.OperationResult.LogTyp
 import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog;
 import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey.SecretKeyType;
 import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException;
+import org.sufficientlysecure.keychain.pgp.exception.PgpKeyNotFoundException;
+import org.sufficientlysecure.keychain.provider.CachedPublicKeyRing;
 import org.sufficientlysecure.keychain.provider.KeychainContract.KeyRings;
 import org.sufficientlysecure.keychain.provider.ProviderHelper;
 import org.sufficientlysecure.keychain.service.input.CryptoInputParcel;
@@ -539,7 +541,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
 
         PGPPublicKeyEncryptedData encryptedDataAsymmetric = null;
         PGPPBEEncryptedData encryptedDataSymmetric = null;
-        CanonicalizedSecretKey secretEncryptionKey = null;
+        CanonicalizedSecretKey decryptionKey = null;
 
         Passphrase passphrase = null;
 
@@ -560,85 +562,87 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
                 log.add(LogType.MSG_DC_ASYM, indent,
                         KeyFormattingUtils.convertKeyIdToHex(subKeyId));
 
-                CanonicalizedSecretKeyRing secretKeyRing;
+                CachedPublicKeyRing cachedPublicKeyRing;
                 try {
                     // get actual keyring object based on master key id
-                    secretKeyRing = mProviderHelper.getCanonicalizedSecretKeyRing(
+                    cachedPublicKeyRing = mProviderHelper.getCachedPublicKeyRing(
                             KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(subKeyId)
                     );
-                } catch (ProviderHelper.NotFoundException e) {
-                    // continue with the next packet in the while loop
-                    log.add(LogType.MSG_DC_ASKIP_NO_KEY, indent + 1);
-                    continue;
-                }
+                    long masterKeyId = cachedPublicKeyRing.getMasterKeyId();
+
+                    // allow only specific keys for decryption?
+                    if (input.getAllowedKeyIds() != null) {
+                        Log.d(Constants.TAG, "encData.getKeyID(): " + subKeyId);
+                        Log.d(Constants.TAG, "mAllowedKeyIds: " + input.getAllowedKeyIds());
+                        Log.d(Constants.TAG, "masterKeyId: " + masterKeyId);
+
+                        if (!input.getAllowedKeyIds().contains(masterKeyId)) {
+                            // this key is in our db, but NOT allowed!
+                            // continue with the next packet in the while loop
+                            result.skippedDisallowedKey = true;
+                            log.add(LogType.MSG_DC_ASKIP_NOT_ALLOWED, indent + 1);
+                            continue;
+                        }
+                    }
 
-                // allow only specific keys for decryption?
-                if (input.getAllowedKeyIds() != null) {
-                    long masterKeyId = secretKeyRing.getMasterKeyId();
-                    Log.d(Constants.TAG, "encData.getKeyID(): " + subKeyId);
-                    Log.d(Constants.TAG, "mAllowedKeyIds: " + input.getAllowedKeyIds());
-                    Log.d(Constants.TAG, "masterKeyId: " + masterKeyId);
-
-                    if (!input.getAllowedKeyIds().contains(masterKeyId)) {
-                        // this key is in our db, but NOT allowed!
-                        // continue with the next packet in the while loop
-                        result.skippedDisallowedKey = true;
-                        log.add(LogType.MSG_DC_ASKIP_NOT_ALLOWED, indent + 1);
+                    SecretKeyType secretKeyType = cachedPublicKeyRing.getSecretKeyType(subKeyId);
+                    if (!secretKeyType.isUsable()) {
+                        decryptionKey = null;
+                        log.add(LogType.MSG_DC_ASKIP_UNAVAILABLE, indent + 1);
                         continue;
                     }
-                }
-
-                // get subkey which has been used for this encryption packet
-                secretEncryptionKey = secretKeyRing.getSecretKey(subKeyId);
 
-                if (!secretEncryptionKey.canEncrypt()) {
-                    secretEncryptionKey = null;
-                    log.add(LogType.MSG_DC_ASKIP_BAD_FLAGS, indent + 1);
-                    continue;
-                }
-
-                if (!secretEncryptionKey.getSecretKeyType().isUsable()) {
-                    secretEncryptionKey = null;
-                    log.add(LogType.MSG_DC_ASKIP_UNAVAILABLE, indent + 1);
-                    continue;
-                }
+                    // get actual subkey which has been used for this encryption packet
+                    CanonicalizedSecretKeyRing canonicalizedSecretKeyRing = mProviderHelper
+                            .getCanonicalizedSecretKeyRing(masterKeyId);
+                    CanonicalizedSecretKey candidateDecryptionKey = canonicalizedSecretKeyRing.getSecretKey(subKeyId);
 
-                /* secret key exists in database and is allowed! */
-                asymmetricPacketFound = true;
+                    if (!candidateDecryptionKey.canEncrypt()) {
+                        log.add(LogType.MSG_DC_ASKIP_BAD_FLAGS, indent + 1);
+                        continue;
+                    }
 
-                encryptedDataAsymmetric = encData;
+                    if (secretKeyType == SecretKeyType.DIVERT_TO_CARD) {
+                        passphrase = null;
+                    } else if (secretKeyType == SecretKeyType.PASSPHRASE_EMPTY) {
+                        passphrase = new Passphrase("");
+                    } else if (cryptoInput.hasPassphrase()) {
+                        passphrase = cryptoInput.getPassphrase();
+                    } else {
+                        // if no passphrase was explicitly set try to get it from the cache service
+                        try {
+                            // returns "" if key has no passphrase
+                            passphrase = getCachedPassphrase(subKeyId);
+                            log.add(LogType.MSG_DC_PASS_CACHED, indent + 1);
+                        } catch (PassphraseCacheInterface.NoSecretKeyException e) {
+                            log.add(LogType.MSG_DC_ERROR_NO_KEY, indent + 1);
+                            return result.with(new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log));
+                        }
 
-                if (secretEncryptionKey.getSecretKeyType() == SecretKeyType.DIVERT_TO_CARD) {
-                    passphrase = null;
-                } else if (secretKeyType == SecretKeyType.PASSPHRASE_EMPTY) {
-                    passphrase = new Passphrase("");
-                } else if (cryptoInput.hasPassphrase()) {
-                    passphrase = cryptoInput.getPassphrase();
-                } else {
-                    // if no passphrase was explicitly set try to get it from the cache service
-                    try {
-                        // returns "" if key has no passphrase
-                        passphrase = getCachedPassphrase(subKeyId);
-                        log.add(LogType.MSG_DC_PASS_CACHED, indent + 1);
-                    } catch (PassphraseCacheInterface.NoSecretKeyException e) {
-                        log.add(LogType.MSG_DC_ERROR_NO_KEY, indent + 1);
-                        return result.with(new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log));
+                        // if passphrase was not cached, return here indicating that a passphrase is missing!
+                        if (passphrase == null) {
+                            log.add(LogType.MSG_DC_PENDING_PASSPHRASE, indent + 1);
+                            return result.with(new DecryptVerifyResult(log,
+                                    RequiredInputParcel.createRequiredDecryptPassphrase(masterKeyId, subKeyId),
+                                    cryptoInput));
+                        }
                     }
 
-                    // if passphrase was not cached, return here indicating that a passphrase is missing!
-                    if (passphrase == null) {
-                        log.add(LogType.MSG_DC_PENDING_PASSPHRASE, indent + 1);
-                        return result.with(new DecryptVerifyResult(log,
-                                RequiredInputParcel.createRequiredDecryptPassphrase(
-                                        secretKeyRing.getMasterKeyId(), secretEncryptionKey.getKeyId()),
-                                cryptoInput));
+                    // check for insecure encryption key
+                    if ( ! PgpSecurityConstants.isSecureKey(candidateDecryptionKey)) {
+                        log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1);
+                        result.insecureEncryptionKey = true;
                     }
-                }
 
-                // check for insecure encryption key
-                if ( ! PgpSecurityConstants.isSecureKey(secretEncryptionKey)) {
-                    log.add(LogType.MSG_DC_INSECURE_KEY, indent + 1);
-                    result.insecureEncryptionKey = true;
+                    // we're good, write down the data for later
+                    asymmetricPacketFound = true;
+                    encryptedDataAsymmetric = encData;
+                    decryptionKey = candidateDecryptionKey;
+
+                } catch (PgpKeyNotFoundException | ProviderHelper.NotFoundException e) {
+                    // continue with the next packet in the while loop
+                    log.add(LogType.MSG_DC_ASKIP_NO_KEY, indent + 1);
+                    continue;
                 }
 
                 // break out of while, only decrypt the first packet where we have a key
@@ -737,7 +741,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
 
             try {
                 log.add(LogType.MSG_DC_UNLOCKING, indent + 1);
-                if (!secretEncryptionKey.unlock(passphrase)) {
+                if (!decryptionKey.unlock(passphrase)) {
                     log.add(LogType.MSG_DC_ERROR_BAD_PASSPHRASE, indent + 1);
                     return result.with(new DecryptVerifyResult(DecryptVerifyResult.RESULT_ERROR, log));
                 }
@@ -750,7 +754,7 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
             updateProgress(R.string.progress_preparing_streams, currentProgress, 100);
 
             CachingDataDecryptorFactory decryptorFactory
-                    = secretEncryptionKey.getCachingDecryptorFactory(cryptoInput);
+                    = decryptionKey.getCachingDecryptorFactory(cryptoInput);
 
             // special case: if the decryptor does not have a session key cached for this encrypted
             // data, and can't actually decrypt on its own, return a pending intent
@@ -759,8 +763,8 @@ public class PgpDecryptVerifyOperation extends BaseOperation<PgpDecryptVerifyInp
 
                 log.add(LogType.MSG_DC_PENDING_NFC, indent + 1);
                 return result.with(new DecryptVerifyResult(log, RequiredInputParcel.createNfcDecryptOperation(
-                        secretEncryptionKey.getRing().getMasterKeyId(),
-                        secretEncryptionKey.getKeyId(), encryptedDataAsymmetric.getSessionKey()[0]
+                        decryptionKey.getRing().getMasterKeyId(),
+                        decryptionKey.getKeyId(), encryptedDataAsymmetric.getSessionKey()[0]
                 ), cryptoInput));
 
             }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
index 1290dcdcf..a418075c3 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpSignEncryptOperation.java
@@ -166,12 +166,13 @@ public class PgpSignEncryptOperation extends BaseOperation {
             updateProgress(R.string.progress_extracting_signature_key, 0, 100);
 
             try {
-                // fetch the indicated master key id (the one whose name we sign in)
-                CanonicalizedSecretKeyRing signingKeyRing =
-                        mProviderHelper.getCanonicalizedSecretKeyRing(input.getSignatureMasterKeyId());
-
-                // fetch the specific subkey to sign with, or just use the master key if none specified
-                signingKey = signingKeyRing.getSecretKey(input.getSignatureSubKeyId());
+                long signingMasterKeyId = input.getSignatureMasterKeyId();
+                long signingSubKeyId = input.getSignatureSubKeyId();
+                {
+                    CanonicalizedSecretKeyRing signingKeyRing =
+                            mProviderHelper.getCanonicalizedSecretKeyRing(signingMasterKeyId);
+                    signingKey = signingKeyRing.getSecretKey(input.getSignatureSubKeyId());
+                }
 
                 // Make sure we are allowed to sign here!
                 if (!signingKey.canSign()) {
@@ -179,7 +180,7 @@ public class PgpSignEncryptOperation extends BaseOperation {
                     return new PgpSignEncryptResult(PgpSignEncryptResult.RESULT_ERROR, log);
                 }
 
-                switch (signingKey.getSecretKeyType()) {
+                switch (mProviderHelper.getCachedPublicKeyRing(signingMasterKeyId).getSecretKeyType(signingSubKeyId)) {
                     case DIVERT_TO_CARD:
                     case PASSPHRASE_EMPTY: {
                         if (!signingKey.unlock(new Passphrase())) {
@@ -196,14 +197,14 @@ public class PgpSignEncryptOperation extends BaseOperation {
                         Passphrase localPassphrase = cryptoInput.getPassphrase();
                         if (localPassphrase == null) {
                             try {
-                                localPassphrase = getCachedPassphrase(signingKeyRing.getMasterKeyId(), signingKey.getKeyId());
+                                localPassphrase = getCachedPassphrase(signingMasterKeyId, signingKey.getKeyId());
                             } catch (PassphraseCacheInterface.NoSecretKeyException ignored) {
                             }
                         }
                         if (localPassphrase == null) {
                             log.add(LogType.MSG_PSE_PENDING_PASSPHRASE, indent + 1);
                             return new PgpSignEncryptResult(log, RequiredInputParcel.createRequiredSignPassphrase(
-                                    signingKeyRing.getMasterKeyId(), signingKey.getKeyId(),
+                                    signingMasterKeyId, signingKey.getKeyId(),
                                     cryptoInput.getSignatureTime()), cryptoInput);
                         }
                         if (!signingKey.unlock(localPassphrase)) {
-- 
cgit v1.2.3