aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKB Sriram <kbsriram@google.com>2016-01-15 15:28:40 -0800
committerKB Sriram <kbsriram@google.com>2016-01-15 15:28:40 -0800
commit36bffc3ef5363d51ec3fc49e6b74b593a712b80e (patch)
tree0df7aac040d3b9eb2887ab765d988723cf865c95
parentefbe9ac363132d3280f544d8377978286b828c79 (diff)
downloadopen-keychain-36bffc3ef5363d51ec3fc49e6b74b593a712b80e.zip
open-keychain-36bffc3ef5363d51ec3fc49e6b74b593a712b80e.tar.gz
open-keychain-36bffc3ef5363d51ec3fc49e6b74b593a712b80e.tar.bz2
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
-rw-r--r--.gitmodules3
-rw-r--r--OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java45
-rw-r--r--OpenKeychain/src/test/java/org/sufficientlysecure/keychain/provider/InteropTest.java274
m---------OpenKeychain/src/test/resources/openpgp-interop0
4 files changed, 310 insertions, 12 deletions
diff --git a/.gitmodules b/.gitmodules
index 0855c2f..fc4c5f5 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -22,3 +22,6 @@
path = extern/safeslinger-exchange
url = https://github.com/open-keychain/exchange-android
ignore = dirty
+[submodule "OpenKeychain/src/test/resources/openpgp-interop"]
+ path = OpenKeychain/src/test/resources/openpgp-interop
+ url = https://github.com/google/openpgp-interop
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 c967a5a..af09cf2 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;
diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/provider/InteropTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/provider/InteropTest.java
new file mode 100644
index 0000000..b48c5ac
--- /dev/null
+++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/provider/InteropTest.java
@@ -0,0 +1,274 @@
+/*
+ * Copyright (C) 2016 Google Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package org.sufficientlysecure.keychain.provider;
+
+import android.net.Uri;
+
+import org.json.JSONArray;
+import org.json.JSONObject;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.openintents.openpgp.OpenPgpMetadata;
+import org.openintents.openpgp.OpenPgpSignatureResult;
+import org.robolectric.RobolectricGradleTestRunner;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.annotation.Config;
+import org.spongycastle.jce.provider.BouncyCastleProvider;
+import org.sufficientlysecure.keychain.WorkaroundBuildConfig;
+import org.sufficientlysecure.keychain.operations.results.DecryptVerifyResult;
+import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog;
+import org.sufficientlysecure.keychain.pgp.CanonicalizedKeyRing;
+import org.sufficientlysecure.keychain.pgp.CanonicalizedPublicKey;
+import org.sufficientlysecure.keychain.pgp.CanonicalizedPublicKeyRing;
+import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKeyRing;
+import org.sufficientlysecure.keychain.pgp.PgpDecryptVerifyInputParcel;
+import org.sufficientlysecure.keychain.pgp.PgpDecryptVerifyOperation;
+import org.sufficientlysecure.keychain.pgp.UncachedKeyRing;
+import org.sufficientlysecure.keychain.provider.KeychainContract.KeyRings;
+import org.sufficientlysecure.keychain.service.input.CryptoInputParcel;
+import org.sufficientlysecure.keychain.ui.util.KeyFormattingUtils;
+import org.sufficientlysecure.keychain.util.InputData;
+import org.sufficientlysecure.keychain.util.Passphrase;
+
+import java.io.BufferedInputStream;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileInputStream;
+import java.net.URL;
+import java.security.Security;
+import java.util.ArrayList;
+
+@RunWith(RobolectricGradleTestRunner.class)
+@Config(constants = WorkaroundBuildConfig.class, sdk = 21, manifest = "src/main/AndroidManifest.xml")
+public class InteropTest {
+
+ @BeforeClass
+ public static void setUpOnce() throws Exception {
+ Security.insertProviderAt(new BouncyCastleProvider(), 1);
+ }
+
+ @Test
+ public void testInterop() throws Exception {
+ URL baseURL = InteropTest.class.getResource("/openpgp-interop/testcases");
+ Assert.assertNotNull(baseURL);
+ File baseFile = new File(baseURL.toURI());
+ walkTests(baseFile);
+ }
+
+ private void walkTests(File root) throws Exception {
+ File children[] = root.listFiles();
+ if (children == null) {
+ return;
+ }
+ for (File child : children) {
+ if (child.getName().startsWith(".")) {
+ continue;
+ }
+ if (child.isDirectory()) {
+ walkTests(child);
+ } else if (child.getName().endsWith(".json")) {
+ runTest(child);
+ }
+ }
+ }
+
+ private void runTest(File base) throws Exception {
+ JSONObject config = new JSONObject(asString(base));
+ String testType = config.getString("type");
+ if (testType.equals("import")) {
+ runImportTest(config, base);
+ } else if (testType.equals("decrypt")) {
+ runDecryptTest(config, base);
+ } else {
+ Assert.fail(base + ": unexpected test type");
+ }
+ }
+
+ private static final String asString(File json) throws Exception {
+ return new String(asBytes(json), "utf-8");
+ }
+
+ private static final byte[] asBytes(File f) throws Exception {
+ FileInputStream fin = null;
+ try {
+ fin = new FileInputStream(f);
+ byte data[] = new byte[fin.available()];
+ fin.read(data);
+ return data;
+ } finally {
+ close(fin);
+ }
+ }
+
+ private void runDecryptTest(JSONObject config, File base) throws Exception {
+ File root = base.getParentFile();
+ String baseName = getBaseName(base);
+ CanonicalizedPublicKeyRing verify;
+ if (config.has("verifyKey")) {
+ verify = (CanonicalizedPublicKeyRing)
+ readRingFromFile(new File(root, config.getString("verifyKey")));
+ } else {
+ verify = null;
+ }
+
+ CanonicalizedSecretKeyRing decrypt = (CanonicalizedSecretKeyRing)
+ readRingFromFile(new File(root, config.getString("decryptKey")));
+
+ ByteArrayOutputStream out = new ByteArrayOutputStream();
+ ByteArrayInputStream in =
+ new ByteArrayInputStream(asBytes(new File(root, baseName + ".asc")));
+
+ InputData data = new InputData(in, in.available());
+
+ Passphrase pass = new Passphrase(config.getString("passphrase"));
+
+ PgpDecryptVerifyOperation op = makeOperation(base.toString(), pass, decrypt, verify);
+ PgpDecryptVerifyInputParcel input = new PgpDecryptVerifyInputParcel();
+ CryptoInputParcel cip = new CryptoInputParcel(pass);
+ DecryptVerifyResult result = op.execute(input, cip, data, out);
+ byte[] plaintext = config.getString("textcontent").getBytes("utf-8");
+ String filename = config.getString("filename");
+ Assert.assertTrue(base + ": decryption must succeed", result.success());
+ byte[] decrypted = out.toByteArray();
+ Assert.assertArrayEquals(base + ": plaintext should be correct", decrypted, plaintext);
+ if (verify != null) {
+ // Certain keys are too short, so we check appropriately.
+ int code = result.getSignatureResult().getResult();
+ Assert.assertTrue(base + ": should have a signature",
+ (code == OpenPgpSignatureResult.RESULT_INVALID_INSECURE) ||
+ (code == OpenPgpSignatureResult.RESULT_VALID_UNCONFIRMED));
+ }
+ OpenPgpMetadata metadata = result.getDecryptionMetadata();
+ Assert.assertEquals(base + ": filesize must be correct",
+ decrypted.length, metadata.getOriginalSize());
+ Assert.assertEquals(base + ": filename must be correct",
+ filename, metadata.getFilename());
+ }
+
+ private void runImportTest(JSONObject config, File base) throws Exception {
+ File root = base.getParentFile();
+ String baseName = getBaseName(base);
+ CanonicalizedKeyRing pkr =
+ readRingFromFile(new File(root, baseName + ".asc"));
+
+ // Check we have the correct uids.
+ ArrayList<String> expected = new ArrayList<String>();
+ JSONArray uids = config.getJSONArray("expected_uids");
+ for (int i = 0; i < uids.length(); i++) {
+ expected.add(uids.getString(i));
+ }
+ check(base + ": incorrect uids", expected, pkr.getUnorderedUserIds());
+
+ // Check we have the correct main and subkey fingerprints.
+ expected.clear();
+ expected.add(config.getString("expected_fingerprint"));
+ JSONArray subkeys = config.optJSONArray("expected_subkeys");
+ if (subkeys != null) {
+ for (int i = 0; i < subkeys.length(); i++) {
+ expected.add(subkeys.getJSONObject(i).getString("expected_fingerprint"));
+ }
+ }
+ ArrayList<String> actual = new ArrayList<String>();
+ for (CanonicalizedPublicKey pk: pkr.publicKeyIterator()) {
+ if (pk.isValid()) {
+ actual.add(KeyFormattingUtils.convertFingerprintToHex(pk.getFingerprint()));
+ }
+ }
+ check(base + ": incorrect fingerprints", expected, actual);
+ }
+
+ private void check(String msg, ArrayList<String> a, ArrayList<String> b) {
+ Assert.assertEquals(msg, a.size(), b.size());
+ for (int i = 0; i < a.size(); i++) {
+ Assert.assertEquals(msg, a.get(i), b.get(i));
+ }
+ }
+
+ UncachedKeyRing readUncachedRingFromFile(File path) throws Exception {
+ BufferedInputStream bin = null;
+ try {
+ bin = new BufferedInputStream(new FileInputStream(path));
+ return UncachedKeyRing.fromStream(bin).next();
+ } finally {
+ close(bin);
+ }
+ }
+
+ CanonicalizedKeyRing readRingFromFile(File path) throws Exception {
+ UncachedKeyRing ukr = readUncachedRingFromFile(path);
+ OperationLog log = new OperationLog();
+ return ukr.canonicalize(log, 0);
+ }
+
+ private static final void close(Closeable v) {
+ if (v != null) {
+ try {
+ v.close();
+ } catch (Throwable any) {
+ }
+ }
+ }
+
+ private static final String getBaseName(File base) {
+ String name = base.getName();
+ return name.substring(0, name.length() - ".json".length());
+ }
+
+ private PgpDecryptVerifyOperation makeOperation(final String msg, final Passphrase passphrase,
+ final CanonicalizedSecretKeyRing decrypt, final CanonicalizedPublicKeyRing verify)
+ throws Exception {
+
+ final long decryptId = decrypt.getEncryptId();
+ final Uri decryptUri = KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(decryptId);
+ final Uri verifyUri = verify != null ?
+ KeyRings.buildUnifiedKeyRingsFindBySubkeyUri(verify.getMasterKeyId()) : null;
+
+ ProviderHelper helper = new ProviderHelper(RuntimeEnvironment.application) {
+ @Override
+ public CanonicalizedPublicKeyRing getCanonicalizedPublicKeyRing(Uri q)
+ throws NotFoundException {
+ Assert.assertEquals(msg + ": query should be for verification key",
+ q, verifyUri);
+ return verify;
+ }
+ @Override
+ public CanonicalizedSecretKeyRing getCanonicalizedSecretKeyRing(Uri q)
+ throws NotFoundException {
+ Assert.assertEquals(msg + ": query should be for the decryption key",
+ q, decryptUri);
+ return decrypt;
+ }
+ };
+
+ return new PgpDecryptVerifyOperation(RuntimeEnvironment.application, helper, null) {
+ @Override
+ public Passphrase getCachedPassphrase(long masterKeyId, long subKeyId)
+ throws NoSecretKeyException {
+ Assert.assertEquals(msg + ": passphrase should be for the secret key",
+ masterKeyId, decrypt.getMasterKeyId());
+ Assert.assertEquals(msg + ": passphrase should refer to the decryption subkey",
+ subKeyId, decryptId);
+ return passphrase;
+ }
+ };
+ }
+}
diff --git a/OpenKeychain/src/test/resources/openpgp-interop b/OpenKeychain/src/test/resources/openpgp-interop
new file mode 160000
+Subproject 1cf03918f0ec839015b340b1a89b12114b90c46