From 0e0970c347f0a2f78f190d6d26205178037f5095 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Sat, 24 Jan 2015 23:05:50 +0100 Subject: [PATCH] move key stripping into ChangeSubkey, support divert-to-card --- .../keychain/pgp/PgpKeyOperationTest.java | 4 +- .../keychain/pgp/PgpKeyOperation.java | 43 ++++++++----------- .../keychain/pgp/UncachedKeyRing.java | 4 +- .../keychain/service/SaveKeyringParcel.java | 29 +++++++++---- .../keychain/ui/EditKeyFragment.java | 12 +++--- .../keychain/ui/adapter/SubkeysAdapter.java | 7 ++- extern/spongycastle | 2 +- 7 files changed, 59 insertions(+), 42 deletions(-) diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java index 3ea88aac6..d4fdf14d7 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperationTest.java @@ -701,7 +701,7 @@ public class PgpKeyOperationTest { public void testSubkeyStrip() throws Exception { long keyId = KeyringTestingHelper.getSubkeyId(ring, 1); - parcel.mStripSubKeys.add(keyId); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); applyModificationWithChecks(parcel, ring, onlyA, onlyB); Assert.assertEquals("one extra packet in original", 1, onlyA.size()); @@ -727,7 +727,7 @@ public class PgpKeyOperationTest { public void testMasterStrip() throws Exception { long keyId = ring.getMasterKeyId(); - parcel.mStripSubKeys.add(keyId); + parcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); applyModificationWithChecks(parcel, ring, onlyA, onlyB); Assert.assertEquals("one extra packet in original", 1, onlyA.size()); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java index 4bab7f2b9..4cf5fc459 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -20,6 +20,7 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.CompressionAlgorithmTags; import org.spongycastle.bcpg.HashAlgorithmTags; +import org.spongycastle.bcpg.S2K; import org.spongycastle.bcpg.SymmetricKeyAlgorithmTags; import org.spongycastle.bcpg.sig.Features; import org.spongycastle.bcpg.sig.KeyFlags; @@ -715,6 +716,24 @@ public class PgpKeyOperation { return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); } + if (change.mDummyStrip || change.mDummyDivert) { + // IT'S DANGEROUS~ + // no really, it is. this operation irrevocably removes the private key data from the key + if (change.mDummyStrip) { + sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), + S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY); + } else { + sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey(), + S2K.GNU_PROTECTION_MODE_DIVERT_TO_CARD); + } + sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey); + } + + // This doesn't concern us any further + if (change.mExpiry == null && change.mFlags == null) { + continue; + } + // expiry must not be in the past if (change.mExpiry != null && change.mExpiry != 0 && new Date(change.mExpiry*1000).before(new Date())) { @@ -805,30 +824,6 @@ public class PgpKeyOperation { } subProgressPop(); - // 4c. For each subkey to be stripped... do so - subProgressPush(65, 70); - for (int i = 0; i < saveParcel.mStripSubKeys.size(); i++) { - - progress(R.string.progress_modify_subkeystrip, (i-1) * (100 / saveParcel.mStripSubKeys.size())); - long strip = saveParcel.mStripSubKeys.get(i); - log.add(LogType.MSG_MF_SUBKEY_STRIP, - indent, KeyFormattingUtils.convertKeyIdToHex(strip)); - - PGPSecretKey sKey = sKR.getSecretKey(strip); - if (sKey == null) { - log.add(LogType.MSG_MF_ERROR_SUBKEY_MISSING, - indent+1, KeyFormattingUtils.convertKeyIdToHex(strip)); - return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); - } - - // IT'S DANGEROUS~ - // no really, it is. this operation irrevocably removes the private key data from the key - sKey = PGPSecretKey.constructGnuDummyKey(sKey.getPublicKey()); - sKR = PGPSecretKeyRing.insertSecretKey(sKR, sKey); - - } - subProgressPop(); - // 5. Generate and add new subkeys subProgressPush(70, 90); for (int i = 0; i < saveParcel.mAddSubKeys.size(); i++) { 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 04fb955fa..df333553b 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -20,6 +20,7 @@ package org.sufficientlysecure.keychain.pgp; import org.spongycastle.bcpg.ArmoredOutputStream; import org.spongycastle.bcpg.PublicKeyAlgorithmTags; +import org.spongycastle.bcpg.S2K; import org.spongycastle.bcpg.SignatureSubpacketTags; import org.spongycastle.bcpg.UserAttributeSubpacketTags; import org.spongycastle.bcpg.sig.KeyFlags; @@ -1221,7 +1222,8 @@ public class UncachedKeyRing { // if this is a secret key which does not yet occur in the secret ring if (sKey == null) { // generate a stripped secret (sub)key - sKey = PGPSecretKey.constructGnuDummyKey(key); + sKey = PGPSecretKey.constructGnuDummyKey(key, + S2K.GNU_PROTECTION_MODE_NO_PRIVATE_KEY); } sKey = PGPSecretKey.replacePublicKey(sKey, key); return PGPSecretKeyRing.insertSecretKey(secRing, sKey); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java index a314c8768..b8ee750b7 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/service/SaveKeyringParcel.java @@ -58,7 +58,6 @@ public class SaveKeyringParcel implements Parcelable { public ArrayList mRevokeUserIds; public ArrayList mRevokeSubKeys; - public ArrayList mStripSubKeys; public SaveKeyringParcel() { reset(); @@ -79,7 +78,6 @@ public class SaveKeyringParcel implements Parcelable { mChangeSubKeys = new ArrayList(); mRevokeUserIds = new ArrayList(); mRevokeSubKeys = new ArrayList(); - mStripSubKeys = new ArrayList(); } // performance gain for using Parcelable here would probably be negligible, @@ -112,10 +110,14 @@ public class SaveKeyringParcel implements Parcelable { } public static class SubkeyChange implements Serializable { - public long mKeyId; + public final long mKeyId; public Integer mFlags; // this is a long unix timestamp, in seconds (NOT MILLISECONDS!) public Long mExpiry; + // if this flag is true, the subkey should be changed to a stripped key + public boolean mDummyStrip; + // if this flag is true, the subkey should be changed to a divert-to-card key + public boolean mDummyDivert; public SubkeyChange(long keyId) { mKeyId = keyId; @@ -127,11 +129,25 @@ public class SaveKeyringParcel implements Parcelable { mExpiry = expiry; } + public SubkeyChange(long keyId, boolean dummyStrip, boolean dummyDivert) { + this(keyId, null, null); + + // these flags are mutually exclusive! + if (dummyStrip && dummyDivert) { + throw new AssertionError( + "cannot set strip and divert flags at the same time - this is a bug!"); + } + mDummyStrip = dummyStrip; + mDummyDivert = dummyDivert; + } + @Override public String toString() { String out = "mKeyId: " + mKeyId + ", "; out += "mFlags: " + mFlags + ", "; - out += "mExpiry: " + mExpiry; + out += "mExpiry: " + mExpiry + ", "; + out += "mDummyStrip: " + mDummyStrip + ", "; + out += "mDummyDivert: " + mDummyDivert; return out; } @@ -173,7 +189,6 @@ public class SaveKeyringParcel implements Parcelable { mRevokeUserIds = source.createStringArrayList(); mRevokeSubKeys = (ArrayList) source.readSerializable(); - mStripSubKeys = (ArrayList) source.readSerializable(); } @Override @@ -196,7 +211,6 @@ public class SaveKeyringParcel implements Parcelable { destination.writeStringList(mRevokeUserIds); destination.writeSerializable(mRevokeSubKeys); - destination.writeSerializable(mStripSubKeys); } public static final Creator CREATOR = new Creator() { @@ -224,8 +238,7 @@ public class SaveKeyringParcel implements Parcelable { out += "mChangeSubKeys: " + mChangeSubKeys + "\n"; out += "mChangePrimaryUserId: " + mChangePrimaryUserId + "\n"; out += "mRevokeUserIds: " + mRevokeUserIds + "\n"; - out += "mRevokeSubKeys: " + mRevokeSubKeys + "\n"; - out += "mStripSubKeys: " + mStripSubKeys; + out += "mRevokeSubKeys: " + mRevokeSubKeys; return out; } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java index afe6afb3c..330589c7c 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/EditKeyFragment.java @@ -55,6 +55,7 @@ import org.sufficientlysecure.keychain.service.KeychainIntentServiceHandler; import org.sufficientlysecure.keychain.service.PassphraseCacheService; import org.sufficientlysecure.keychain.service.SaveKeyringParcel; import org.sufficientlysecure.keychain.service.SaveKeyringParcel.ChangeUnlockParcel; +import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyChange; import org.sufficientlysecure.keychain.ui.adapter.SubkeysAdapter; import org.sufficientlysecure.keychain.ui.adapter.SubkeysAddedAdapter; import org.sufficientlysecure.keychain.ui.adapter.UserIdsAdapter; @@ -478,12 +479,13 @@ public class EditKeyFragment extends LoaderFragment implements } break; case EditSubkeyDialogFragment.MESSAGE_STRIP: - // toggle - if (mSaveKeyringParcel.mStripSubKeys.contains(keyId)) { - mSaveKeyringParcel.mStripSubKeys.remove(keyId); - } else { - mSaveKeyringParcel.mStripSubKeys.add(keyId); + SubkeyChange change = mSaveKeyringParcel.getSubkeyChange(keyId); + if (change == null) { + mSaveKeyringParcel.mChangeSubKeys.add(new SubkeyChange(keyId, true, false)); + break; } + // toggle + change.mDummyStrip = !change.mDummyStrip; break; } getLoaderManager().getLoader(LOADER_ID_SUBKEYS).forceLoad(); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java index a032e96fc..a8fa3f1aa 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/adapter/SubkeysAdapter.java @@ -35,6 +35,7 @@ import android.widget.ImageView; import android.widget.TextView; import org.sufficientlysecure.keychain.R; +import org.sufficientlysecure.keychain.service.SaveKeyringParcel.SubkeyChange; import org.sufficientlysecure.keychain.ui.util.FormattingUtils; import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey.SecretKeyType; import org.sufficientlysecure.keychain.ui.util.KeyFormattingUtils; @@ -160,7 +161,11 @@ public class SubkeysAdapter extends CursorAdapter { cursor.getString(INDEX_KEY_CURVE_OID) )); - if (mSaveKeyringParcel != null && mSaveKeyringParcel.mStripSubKeys.contains(keyId)) { + SubkeyChange change = mSaveKeyringParcel != null + ? mSaveKeyringParcel.getSubkeyChange(keyId) + : null; + + if (change.mDummyStrip) { algorithmStr.append(", "); final SpannableString boldStripped = new SpannableString( context.getString(R.string.key_stripped) diff --git a/extern/spongycastle b/extern/spongycastle index f13d9c202..c2a91166c 160000 --- a/extern/spongycastle +++ b/extern/spongycastle @@ -1 +1 @@ -Subproject commit f13d9c202f7470ede75f2c02cc8c578acd3acee9 +Subproject commit c2a91166cef1a08c97ad2e988e368fe36babfc05