From 466060888714638215e0b02bc25e66172c73c490 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 14 Jan 2015 11:07:09 +0100 Subject: fix log entry for addition of user attributes --- .../java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 18a5410bf..56f7b3309 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -531,12 +531,12 @@ public class PgpKeyOperation { WrappedUserAttribute attribute = saveParcel.mAddUserAttribute.get(i); switch (attribute.getType()) { - case WrappedUserAttribute.UAT_NONE: - log.add(LogType.MSG_MF_UAT_ADD_UNKNOWN, indent); - break; case WrappedUserAttribute.UAT_IMAGE: log.add(LogType.MSG_MF_UAT_ADD_IMAGE, indent); break; + default: + log.add(LogType.MSG_MF_UAT_ADD_UNKNOWN, indent); + break; } PGPUserAttributeSubpacketVector vector = attribute.getVector(); -- cgit v1.2.3 From bbc9d90c3efc2665a58f3ec69c44981b63c984b5 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 14 Jan 2015 13:04:09 +0100 Subject: add basic tests for addition of user attributes --- .../keychain/pgp/PgpKeyOperationTest.java | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) 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 52115a76d..3ea88aac6 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 @@ -34,6 +34,8 @@ import org.spongycastle.bcpg.S2K; import org.spongycastle.bcpg.SecretKeyPacket; import org.spongycastle.bcpg.SecretSubkeyPacket; import org.spongycastle.bcpg.SignaturePacket; +import org.spongycastle.bcpg.UserAttributePacket; +import org.spongycastle.bcpg.UserAttributeSubpacket; import org.spongycastle.bcpg.UserIDPacket; import org.spongycastle.bcpg.sig.KeyFlags; import org.spongycastle.jce.provider.BouncyCastleProvider; @@ -864,6 +866,70 @@ public class PgpKeyOperationTest { } + @Test + public void testUserAttributeAdd() throws Exception { + + { + parcel.mAddUserAttribute.add(WrappedUserAttribute.fromData(new byte[0])); + assertModifyFailure("adding an empty user attribute should fail", ring, parcel, + LogType.MSG_MF_UAT_ERROR_EMPTY); + } + + parcel.reset(); + + Random r = new Random(); + int type = r.nextInt(110)+1; + byte[] data = new byte[r.nextInt(2000)]; + new Random().nextBytes(data); + + WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data); + parcel.mAddUserAttribute.add(uat); + + UncachedKeyRing modified = applyModificationWithChecks(parcel, ring, onlyA, onlyB); + + Assert.assertEquals("no extra packets in original", 0, onlyA.size()); + Assert.assertEquals("exactly two extra packets in modified", 2, onlyB.size()); + + Assert.assertTrue("keyring must contain added user attribute", + modified.getPublicKey().getUnorderedUserAttributes().contains(uat)); + + Packet p; + + p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); + Assert.assertTrue("first new packet must be user attribute", p instanceof UserAttributePacket); + { + UserAttributeSubpacket[] subpackets = ((UserAttributePacket) p).getSubpackets(); + Assert.assertEquals("user attribute packet must contain one subpacket", + 1, subpackets.length); + Assert.assertEquals("user attribute subpacket type must be as specified above", + type, subpackets[0].getType()); + Assert.assertArrayEquals("user attribute subpacket data must be as specified above", + data, subpackets[0].getData()); + } + + p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(1).buf)).readPacket(); + Assert.assertTrue("second new packet must be signature", p instanceof SignaturePacket); + Assert.assertEquals("signature type must be positive certification", + PGPSignature.POSITIVE_CERTIFICATION, ((SignaturePacket) p).getSignatureType()); + + // make sure packets can be distinguished by timestamp + Thread.sleep(1000); + + // applying the same modification AGAIN should not add more certifications but drop those + // as duplicates + modified = applyModificationWithChecks(parcel, modified, onlyA, onlyB, passphrase, true, false); + + Assert.assertEquals("duplicate modification: one extra packet in original", 1, onlyA.size()); + Assert.assertEquals("duplicate modification: one extra packet in modified", 1, onlyB.size()); + + p = new BCPGInputStream(new ByteArrayInputStream(onlyA.get(0).buf)).readPacket(); + Assert.assertTrue("lost packet must be signature", p instanceof SignaturePacket); + p = new BCPGInputStream(new ByteArrayInputStream(onlyB.get(0).buf)).readPacket(); + Assert.assertTrue("new packet must be signature", p instanceof SignaturePacket); + + } + + @Test public void testUserIdPrimary() throws Exception { -- cgit v1.2.3 From 73feaa974cc90c86ca63ba09b86b8a10eb1a7e18 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 14 Jan 2015 13:05:13 +0100 Subject: small fixes to user attribute handling --- .../operations/results/OperationResult.java | 1 + .../keychain/pgp/PgpKeyOperation.java | 4 ++++ .../keychain/pgp/WrappedUserAttribute.java | 25 +++++++++++++++++++--- OpenKeychain/src/main/res/values/strings.xml | 1 + 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java index 556d70cb2..8c76ebb8a 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/operations/results/OperationResult.java @@ -507,6 +507,7 @@ public abstract class OperationResult implements Parcelable { MSG_MF_UID_PRIMARY (LogLevel.INFO, R.string.msg_mf_uid_primary), MSG_MF_UID_REVOKE (LogLevel.INFO, R.string.msg_mf_uid_revoke), MSG_MF_UID_ERROR_EMPTY (LogLevel.ERROR, R.string.msg_mf_uid_error_empty), + MSG_MF_UAT_ERROR_EMPTY (LogLevel.ERROR, R.string.msg_mf_uat_error_empty), MSG_MF_UAT_ADD_IMAGE (LogLevel.INFO, R.string.msg_mf_uat_add_image), MSG_MF_UAT_ADD_UNKNOWN (LogLevel.INFO, R.string.msg_mf_uat_add_unknown), MSG_MF_UNLOCK_ERROR (LogLevel.ERROR, R.string.msg_mf_unlock_error), 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 56f7b3309..4bab7f2b9 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/PgpKeyOperation.java @@ -531,6 +531,10 @@ public class PgpKeyOperation { WrappedUserAttribute attribute = saveParcel.mAddUserAttribute.get(i); switch (attribute.getType()) { + // the 'none' type must not succeed + case WrappedUserAttribute.UAT_NONE: + log.add(LogType.MSG_MF_UAT_ERROR_EMPTY, indent); + return new PgpEditKeyResult(PgpEditKeyResult.RESULT_ERROR, log, null); case WrappedUserAttribute.UAT_IMAGE: log.add(LogType.MSG_MF_UAT_ADD_IMAGE, indent); break; diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java index 248ef11aa..da6d8b287 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/WrappedUserAttribute.java @@ -22,6 +22,7 @@ import org.spongycastle.bcpg.BCPGOutputStream; import org.spongycastle.bcpg.Packet; import org.spongycastle.bcpg.UserAttributePacket; import org.spongycastle.bcpg.UserAttributeSubpacket; +import org.spongycastle.bcpg.UserAttributeSubpacketInputStream; import org.spongycastle.bcpg.UserAttributeSubpacketTags; import org.spongycastle.openpgp.PGPUserAttributeSubpacketVector; @@ -30,6 +31,8 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.ObjectStreamException; import java.io.Serializable; +import java.util.ArrayList; +import java.util.Arrays; public class WrappedUserAttribute implements Serializable { @@ -72,9 +75,17 @@ public class WrappedUserAttribute implements Serializable { return out.toByteArray(); } - public static WrappedUserAttribute fromData (byte[] data) { - // TODO - return null; + public static WrappedUserAttribute fromData (byte[] data) throws IOException { + UserAttributeSubpacketInputStream in = + new UserAttributeSubpacketInputStream(new ByteArrayInputStream(data)); + ArrayList list = new ArrayList(); + while (in.available() > 0) { + list.add(in.readPacket()); + } + UserAttributeSubpacket[] result = new UserAttributeSubpacket[list.size()]; + list.toArray(result); + return new WrappedUserAttribute( + new PGPUserAttributeSubpacketVector(result)); } /** Writes this object to an ObjectOutputStream. */ @@ -102,4 +113,12 @@ public class WrappedUserAttribute implements Serializable { private void readObjectNoData() throws ObjectStreamException { } + @Override + public boolean equals(Object o) { + if (!WrappedUserAttribute.class.isInstance(o)) { + return false; + } + return mVector.equals(((WrappedUserAttribute) o).mVector); + } + } diff --git a/OpenKeychain/src/main/res/values/strings.xml b/OpenKeychain/src/main/res/values/strings.xml index 87de16f6a..561275b83 100644 --- a/OpenKeychain/src/main/res/values/strings.xml +++ b/OpenKeychain/src/main/res/values/strings.xml @@ -857,6 +857,7 @@ "Changing primary user ID to %s" "Revoking user ID %s" "User ID must not be empty!" + "User attribute must not be empty!" "Adding user attribute of type image" "Adding user attribute of unknown type" "Error unlocking keyring!" -- cgit v1.2.3 From 2e04888d36ada6248e311835fce92492cb839239 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Wed, 14 Jan 2015 13:23:03 +0100 Subject: involve user attributes in unit tests for merge and canonicalize! --- .../pgp/UncachedKeyringCanonicalizeTest.java | 41 ++++++++++++++-------- .../keychain/pgp/UncachedKeyringMergeTest.java | 37 +++++++++++++++++++ .../keychain/pgp/UncachedKeyringTest.java | 10 ++++++ 3 files changed, 73 insertions(+), 15 deletions(-) diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java index 721d1a51d..f9e0d52c3 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringCanonicalizeTest.java @@ -104,6 +104,12 @@ public class UncachedKeyringCanonicalizeTest { parcel.mAddUserIds.add("twi"); parcel.mAddUserIds.add("pink"); + { + WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(100, + "sunshine, sunshine, ladybugs awake~".getBytes()); + parcel.mAddUserAttribute.add(uat); + } + // passphrase is tested in PgpKeyOperationTest, just use empty here parcel.mNewUnlock = new ChangeUnlockParcel(""); PgpKeyOperation op = new PgpKeyOperation(null); @@ -116,7 +122,7 @@ public class UncachedKeyringCanonicalizeTest { staticRing = staticRing.canonicalize(new OperationLog(), 0).getUncachedKeyRing(); // just for later reference - totalPackets = 9; + totalPackets = 11; // we sleep here for a second, to make sure all new certificates have different timestamps Thread.sleep(1000); @@ -150,8 +156,8 @@ public class UncachedKeyringCanonicalizeTest { Assert.assertEquals("packet #4 should be signature", PacketTags.SIGNATURE, it.next().tag); - Assert.assertEquals("packet #5 should be secret subkey", - PacketTags.SECRET_SUBKEY, it.next().tag); + Assert.assertEquals("packet #5 should be user id", + PacketTags.USER_ATTRIBUTE, it.next().tag); Assert.assertEquals("packet #6 should be signature", PacketTags.SIGNATURE, it.next().tag); @@ -160,7 +166,12 @@ public class UncachedKeyringCanonicalizeTest { Assert.assertEquals("packet #8 should be signature", PacketTags.SIGNATURE, it.next().tag); - Assert.assertFalse("exactly 9 packets total", it.hasNext()); + Assert.assertEquals("packet #9 should be secret subkey", + PacketTags.SECRET_SUBKEY, it.next().tag); + Assert.assertEquals("packet #10 should be signature", + PacketTags.SIGNATURE, it.next().tag); + + Assert.assertFalse("exactly 11 packets total", it.hasNext()); Assert.assertArrayEquals("created keyring should be constant through canonicalization", ring.getEncoded(), ring.canonicalize(log, 0).getEncoded()); @@ -380,7 +391,7 @@ public class UncachedKeyringCanonicalizeTest { @Test public void testSubkeyDestroy() throws Exception { // signature for second key (first subkey) - UncachedKeyRing modified = KeyringTestingHelper.removePacket(ring, 6); + UncachedKeyRing modified = KeyringTestingHelper.removePacket(ring, 8); // canonicalization should fail, because there are no valid uids left CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0); @@ -424,7 +435,7 @@ public class UncachedKeyringCanonicalizeTest { secretKey.getPublicKey(), pKey.getPublicKey()); // inject in the right position - UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 6); + UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 8); // canonicalize, and check if we lose the bad signature CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0); @@ -449,7 +460,7 @@ public class UncachedKeyringCanonicalizeTest { secretKey.getPublicKey(), pKey.getPublicKey()); // inject in the right position - UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 6); + UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig.getEncoded(), 8); // canonicalize, and check if we lose the bad signature CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0); @@ -481,10 +492,10 @@ public class UncachedKeyringCanonicalizeTest { secretKey, PGPSignature.SUBKEY_BINDING, subHashedPacketsGen, secretKey.getPublicKey(), pKey.getPublicKey()); - UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig1.getEncoded(), 8); - modified = KeyringTestingHelper.injectPacket(modified, sig2.getEncoded(), 9); - modified = KeyringTestingHelper.injectPacket(modified, sig1.getEncoded(), 10); - modified = KeyringTestingHelper.injectPacket(modified, sig3.getEncoded(), 11); + UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sig1.getEncoded(), 10); + modified = KeyringTestingHelper.injectPacket(modified, sig2.getEncoded(), 11); + modified = KeyringTestingHelper.injectPacket(modified, sig1.getEncoded(), 12); + modified = KeyringTestingHelper.injectPacket(modified, sig3.getEncoded(), 13); // canonicalize, and check if we lose the bad signature CanonicalizedKeyRing canonicalized = modified.canonicalize(log, 0); @@ -512,13 +523,13 @@ public class UncachedKeyringCanonicalizeTest { // get subkey packets Iterator it = KeyringTestingHelper.parseKeyring(ring.getEncoded()); - RawPacket subKey = KeyringTestingHelper.getNth(it, 5); + RawPacket subKey = KeyringTestingHelper.getNth(it, 7); RawPacket subSig = it.next(); // inject at a second position UncachedKeyRing modified = ring; - modified = KeyringTestingHelper.injectPacket(modified, subKey.buf, 7); - modified = KeyringTestingHelper.injectPacket(modified, subSig.buf, 8); + modified = KeyringTestingHelper.injectPacket(modified, subKey.buf, 9); + modified = KeyringTestingHelper.injectPacket(modified, subSig.buf, 10); // canonicalize, and check if we lose the bad signature OperationLog log = new OperationLog(); @@ -557,7 +568,7 @@ public class UncachedKeyringCanonicalizeTest { sKey = new PGPSecretKey(masterSecretKey.getPrivateKey(), subPubKey, sha1Calc, false, keyEncryptor); } - UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sKey.getEncoded(), 5); + UncachedKeyRing modified = KeyringTestingHelper.injectPacket(ring, sKey.getEncoded(), 7); // canonicalize, and check if we lose the bad signature OperationLog log = new OperationLog(); diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java index 7f6f480d4..ccd47d0ee 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringMergeTest.java @@ -46,6 +46,7 @@ import java.io.ByteArrayInputStream; import java.security.Security; import java.util.ArrayList; import java.util.Iterator; +import java.util.Random; /** Tests for the UncachedKeyring.merge method. * @@ -97,6 +98,12 @@ public class UncachedKeyringMergeTest { parcel.mAddUserIds.add("twi"); parcel.mAddUserIds.add("pink"); + { + WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(100, + "sunshine, sunshine, ladybugs awake~".getBytes()); + parcel.mAddUserAttribute.add(uat); + } + // passphrase is tested in PgpKeyOperationTest, just use empty here parcel.mNewUnlock = new ChangeUnlockParcel(""); PgpKeyOperation op = new PgpKeyOperation(null); @@ -339,6 +346,36 @@ public class UncachedKeyringMergeTest { } } + @Test + public void testAddedUserAttributeSignature() throws Exception { + + final UncachedKeyRing modified; { + parcel.reset(); + + Random r = new Random(); + int type = r.nextInt(110)+1; + byte[] data = new byte[r.nextInt(2000)]; + new Random().nextBytes(data); + + WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data); + parcel.mAddUserAttribute.add(uat); + + CanonicalizedSecretKeyRing secretRing = new CanonicalizedSecretKeyRing( + ringA.getEncoded(), false, 0); + modified = op.modifySecretKeyRing(secretRing, parcel, "").getRing(); + } + + { + UncachedKeyRing merged = ringA.merge(modified, log, 0); + Assert.assertNotNull("merge must succeed", merged); + Assert.assertFalse( + "merging keyring with extra user attribute into its base should yield that same keyring", + KeyringTestingHelper.diffKeyrings(merged.getEncoded(), modified.getEncoded(), onlyA, onlyB) + ); + } + + } + private UncachedKeyRing mergeWithChecks(UncachedKeyRing a, UncachedKeyRing b) throws Exception { return mergeWithChecks(a, b, a); diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java index a3c58a5c8..65395f1ab 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java @@ -37,6 +37,7 @@ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.util.Iterator; +import java.util.Random; @RunWith(RobolectricTestRunner.class) @org.robolectric.annotation.Config(emulateSdk = 18) // Robolectric doesn't yet support 19 @@ -59,6 +60,15 @@ public class UncachedKeyringTest { parcel.mAddUserIds.add("twi"); parcel.mAddUserIds.add("pink"); + { + Random r = new Random(); + int type = r.nextInt(110)+1; + byte[] data = new byte[r.nextInt(2000)]; + new Random().nextBytes(data); + + WrappedUserAttribute uat = WrappedUserAttribute.fromSubpacket(type, data); + parcel.mAddUserAttribute.add(uat); + } // passphrase is tested in PgpKeyOperationTest, just use empty here parcel.mNewUnlock = new ChangeUnlockParcel(""); PgpKeyOperation op = new PgpKeyOperation(null); -- cgit v1.2.3