From 37cb5c4c78880b5b4737cffdaa4ead76506a3843 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Thu, 2 Oct 2014 19:23:08 +0200 Subject: [PATCH] make getSignId a secret key operation, and respect unavailable keys This one should remedy #811, but waiting for a test --- .../keychain/pgp/CanonicalizedKeyRing.java | 20 +------ .../keychain/pgp/CanonicalizedPublicKey.java | 6 ++ .../pgp/CanonicalizedSecretKeyRing.java | 47 ++++----------- .../keychain/pgp/KeyRing.java | 4 -- .../keychain/pgp/UncachedPublicKey.java | 9 +-- .../provider/CachedPublicKeyRing.java | 58 +++++-------------- .../keychain/remote/OpenPgpService.java | 4 +- .../service/KeychainIntentService.java | 2 +- 8 files changed, 38 insertions(+), 112 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java index 554899843..f43cbbeef 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedKeyRing.java @@ -78,7 +78,7 @@ public abstract class CanonicalizedKeyRing extends KeyRing { public long getEncryptId() throws PgpGeneralException { for(CanonicalizedPublicKey key : publicKeyIterator()) { - if(key.canEncrypt()) { + if (key.canEncrypt() && key.isValid()) { return key.getKeyId(); } } @@ -94,24 +94,6 @@ public abstract class CanonicalizedKeyRing extends KeyRing { } } - public long getSignId() throws PgpGeneralException { - for(CanonicalizedPublicKey key : publicKeyIterator()) { - if(key.canSign()) { - return key.getKeyId(); - } - } - throw new PgpGeneralException("No valid signing key found!"); - } - - public boolean hasSign() throws PgpGeneralException { - try { - getSignId(); - return true; - } catch (PgpGeneralException e) { - return false; - } - } - public void encode(OutputStream stream) throws IOException { getRing().encode(stream); } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java index 8fb3402b2..3539a4ceb 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedPublicKey.java @@ -104,4 +104,10 @@ public class CanonicalizedPublicKey extends UncachedPublicKey { public Integer getKeyUsage() { return super.getKeyUsage(); } + + /** Returns whether this key is valid, ie not expired or revoked. */ + public boolean isValid() { + return !isRevoked() && !isExpired(); + } + } 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 48a2aaeb6..e20155cc6 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/CanonicalizedSecretKeyRing.java @@ -30,6 +30,8 @@ import org.spongycastle.openpgp.operator.PBESecretKeyDecryptor; import org.spongycastle.openpgp.operator.jcajce.JcePBESecretKeyDecryptorBuilder; import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.pgp.exception.PgpGeneralException; +import org.sufficientlysecure.keychain.provider.KeychainContract; +import org.sufficientlysecure.keychain.provider.ProviderHelper; import org.sufficientlysecure.keychain.util.IterableIterator; import org.sufficientlysecure.keychain.util.Log; @@ -74,43 +76,18 @@ public class CanonicalizedSecretKeyRing extends CanonicalizedKeyRing { return new CanonicalizedSecretKey(this, mRing.getSecretKey(id)); } - /** Getter that returns the subkey that should be used for signing. */ - CanonicalizedSecretKey getSigningSubKey() throws PgpGeneralException { - PGPSecretKey key = mRing.getSecretKey(getSignId()); - if(key != null) { - CanonicalizedSecretKey cKey = new CanonicalizedSecretKey(this, key); - if(!cKey.canSign()) { - throw new PgpGeneralException("key error"); - } - return cKey; - } - // TODO handle with proper exception - throw new PgpGeneralException("no signing key available"); - } - - public boolean hasPassphrase() { - PGPSecretKey secretKey = null; - boolean foundValidKey = false; - for (Iterator keys = mRing.getSecretKeys(); keys.hasNext(); ) { - secretKey = (PGPSecretKey) keys.next(); - if (!secretKey.isPrivateKeyEmpty()) { - foundValidKey = true; - break; + /** 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(); } } - if(!foundValidKey) { - return false; - } - - try { - PBESecretKeyDecryptor keyDecryptor = new JcePBESecretKeyDecryptorBuilder() - .setProvider("SC").build("".toCharArray()); - PGPPrivateKey testKey = secretKey.extractPrivateKey(keyDecryptor); - return testKey == null; - } catch(PGPException e) { - // this means the crc check failed -> passphrase required - return true; - } + throw new PgpGeneralException("no valid signing key available"); } public IterableIterator secretKeyIterator() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/KeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/KeyRing.java index 3ef4b336e..b682378e9 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/KeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/KeyRing.java @@ -56,10 +56,6 @@ public abstract class KeyRing { abstract public boolean hasEncrypt() throws PgpGeneralException; - abstract public long getSignId() throws PgpGeneralException; - - abstract public boolean hasSign() throws PgpGeneralException; - abstract public int getVerified() throws PgpGeneralException; private static final Pattern USER_ID_PATTERN = Pattern.compile("^(.*?)(?: \\((.*)\\))?(?: <(.*)>)?$"); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java index bb9c7d51c..c4cacaca7 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedPublicKey.java @@ -51,12 +51,9 @@ public class UncachedPublicKey { /** The revocation signature is NOT checked here, so this may be false! */ public boolean isRevoked() { - for (PGPSignature sig : new IterableIterator( - mPublicKey.getSignaturesOfType(isMasterKey() ? PGPSignature.KEY_REVOCATION - : PGPSignature.SUBKEY_REVOCATION))) { - return true; - } - return false; + return mPublicKey.getSignaturesOfType(isMasterKey() + ? PGPSignature.KEY_REVOCATION + : PGPSignature.SUBKEY_REVOCATION).hasNext(); } public Date getCreationTime() { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/CachedPublicKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/CachedPublicKeyRing.java index cfb4a915e..ad3ebae5f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/CachedPublicKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/provider/CachedPublicKeyRing.java @@ -135,65 +135,33 @@ public class CachedPublicKeyRing extends KeyRing { @Override public long getEncryptId() throws PgpGeneralException { - try { - Cursor subkeys = getSubkeys(); - if (subkeys != null) { - try { - while (subkeys.moveToNext()) { - if (subkeys.getInt(subkeys.getColumnIndexOrThrow(KeychainContract.Keys.CAN_ENCRYPT)) != 0) { - return subkeys.getLong(subkeys.getColumnIndexOrThrow(KeychainContract.Keys.KEY_ID)); - } - } - } finally { - subkeys.close(); - } - } - } catch(Exception e) { - throw new PgpGeneralException(e); - } - throw new PgpGeneralException("No encrypt key found"); - } - - @Override - public boolean hasEncrypt() throws PgpGeneralException { try { Object data = mProviderHelper.getGenericData(mUri, - KeychainContract.KeyRings.HAS_ENCRYPT, + KeyRings.HAS_ENCRYPT, ProviderHelper.FIELD_TYPE_INTEGER); - return (Long) data > 0; + return (Long) data; } catch(ProviderHelper.NotFoundException e) { throw new PgpGeneralException(e); } } @Override - public long getSignId() throws PgpGeneralException { - try { - Cursor subkeys = getSubkeys(); - if (subkeys != null) { - try { - while (subkeys.moveToNext()) { - if (subkeys.getInt(subkeys.getColumnIndexOrThrow(KeychainContract.Keys.CAN_SIGN)) != 0) { - return subkeys.getLong(subkeys.getColumnIndexOrThrow(KeychainContract.Keys.KEY_ID)); - } - } - } finally { - subkeys.close(); - } - } - } catch(Exception e) { - throw new PgpGeneralException(e); - } - throw new PgpGeneralException("No sign key found"); + public boolean hasEncrypt() throws PgpGeneralException { + return getEncryptId() != 0; } - @Override - public boolean hasSign() throws PgpGeneralException { + /** 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 { try { Object data = mProviderHelper.getGenericData(mUri, - KeychainContract.KeyRings.HAS_SIGN, + KeyRings.HAS_SIGN, ProviderHelper.FIELD_TYPE_INTEGER); - return (Long) data > 0; + return (Long) data; } catch(ProviderHelper.NotFoundException e) { throw new PgpGeneralException(e); } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java index b7ee453d5..ed1f19dbd 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/remote/OpenPgpService.java @@ -293,7 +293,7 @@ public class OpenPgpService extends RemoteService { // Find the appropriate subkey to sign with CachedPublicKeyRing signingRing = new ProviderHelper(this).getCachedPublicKeyRing(accSettings.getKeyId()); - final long sigSubKeyId = signingRing.getSignId(); + final long sigSubKeyId = signingRing.getSecretSignId(); // sign-only PgpSignEncrypt.Builder builder = new PgpSignEncrypt.Builder( @@ -405,7 +405,7 @@ public class OpenPgpService extends RemoteService { // Find the appropriate subkey to sign with CachedPublicKeyRing signingRing = new ProviderHelper(this).getCachedPublicKeyRing(accSettings.getKeyId()); - final long sigSubKeyId = signingRing.getSignId(); + final long sigSubKeyId = signingRing.getSecretSignId(); String passphrase; if (data.hasExtra(OpenPgpApi.EXTRA_PASSPHRASE)) { diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java index bb966fec8..0ee3c7093 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/KeychainIntentService.java @@ -294,7 +294,7 @@ public class KeychainIntentService extends IntentService implements Progressable // Find the appropriate subkey to sign with CachedPublicKeyRing signingRing = new ProviderHelper(this).getCachedPublicKeyRing(sigMasterKeyId); - long sigSubKeyId = signingRing.getSignId(); + long sigSubKeyId = signingRing.getSecretSignId(); // Set signature settings builder.setSignatureMasterKeyId(sigMasterKeyId)