Test signature subpackets and fix bug for missing user-id sig

pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditor.java

@@ -16,6 +16,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.Set;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
@@ -36,10 +37,14 @@ import org.bouncycastle.openpgp.operator.PBESecretKeyDecryptor;
 import org.bouncycastle.openpgp.operator.PBESecretKeyEncryptor;
 import org.bouncycastle.openpgp.operator.PGPContentSignerBuilder;
 import org.pgpainless.PGPainless;
+import org.pgpainless.algorithm.AlgorithmSuite;
+import org.pgpainless.algorithm.CompressionAlgorithm;
+import org.pgpainless.algorithm.Feature;
 import org.pgpainless.algorithm.HashAlgorithm;
 import org.pgpainless.algorithm.KeyFlag;
 import org.pgpainless.algorithm.PublicKeyAlgorithm;
 import org.pgpainless.algorithm.SignatureType;
+import org.pgpainless.algorithm.SymmetricKeyAlgorithm;
 import org.pgpainless.algorithm.negotiation.HashAlgorithmNegotiator;
 import org.pgpainless.implementation.ImplementationFactory;
 import org.pgpainless.key.OpenPgpFingerprint;
@@ -101,9 +106,31 @@ public class SecretKeyRingEditor implements SecretKeyRingEditorInterface {
         KeyRingInfo info = PGPainless.inspectKeyRing(secretKeyRing);
         List<KeyFlag> keyFlags = info.getKeyFlagsOf(info.getKeyId());
 
+        Set<HashAlgorithm> hashAlgorithmPreferences;
+        Set<SymmetricKeyAlgorithm> symmetricKeyAlgorithmPreferences;
+        Set<CompressionAlgorithm> compressionAlgorithmPreferences;
+        try {
+            hashAlgorithmPreferences = info.getPreferredHashAlgorithms();
+            symmetricKeyAlgorithmPreferences = info.getPreferredSymmetricKeyAlgorithms();
+            compressionAlgorithmPreferences = info.getPreferredCompressionAlgorithms();
+        } catch (IllegalStateException e) {
+            // missing user-id sig
+            AlgorithmSuite algorithmSuite = AlgorithmSuite.getDefaultAlgorithmSuite();
+            hashAlgorithmPreferences = algorithmSuite.getHashAlgorithms();
+            symmetricKeyAlgorithmPreferences = algorithmSuite.getSymmetricKeyAlgorithms();
+            compressionAlgorithmPreferences = algorithmSuite.getCompressionAlgorithms();
+        }
+
         SelfSignatureBuilder builder = new SelfSignatureBuilder(primaryKey, protector);
         builder.setSignatureType(SignatureType.POSITIVE_CERTIFICATION);
+
+        // Retain signature subpackets of previous signatures
         builder.getHashedSubpackets().setKeyFlags(keyFlags);
+        builder.getHashedSubpackets().setPreferredHashAlgorithms(hashAlgorithmPreferences);
+        builder.getHashedSubpackets().setPreferredSymmetricKeyAlgorithms(symmetricKeyAlgorithmPreferences);
+        builder.getHashedSubpackets().setPreferredCompressionAlgorithms(compressionAlgorithmPreferences);
+        builder.getHashedSubpackets().setFeatures(Feature.MODIFICATION_DETECTION);
+
         builder.applyCallback(signatureSubpacketCallback);
 
         PGPSignature signature = builder.build(primaryKey.getPublicKey(), userId);
@@ -121,7 +148,8 @@ public class SecretKeyRingEditor implements SecretKeyRingEditorInterface {
     }
 
     @Override
-    public SecretKeyRingEditorInterface addSubKey(@Nonnull KeySpec keySpec,
+    public SecretKeyRingEditorInterface addSubKey(
+            @Nonnull KeySpec keySpec,
             @Nonnull Passphrase subKeyPassphrase,
             SecretKeyRingProtector secretKeyRingProtector)
             throws InvalidAlgorithmParameterException, NoSuchAlgorithmException, PGPException, IOException {
@@ -146,7 +174,27 @@ public class SecretKeyRingEditor implements SecretKeyRingEditorInterface {
     }
 
     @Override
-    public SecretKeyRingEditorInterface addSubKey(PGPKeyPair subkey,
+    public SecretKeyRingEditorInterface addSubKey(
+            @Nonnull KeySpec keySpec,
+            @Nullable Passphrase subkeyPassphrase,
+            @Nullable SelfSignatureSubpackets.Callback subpacketsCallback,
+            SecretKeyRingProtector secretKeyRingProtector)
+            throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException, IOException {
+        PGPKeyPair keyPair = KeyRingBuilder.generateKeyPair(keySpec);
+
+        SecretKeyRingProtector subKeyProtector = PasswordBasedSecretKeyRingProtector
+                .forKeyId(keyPair.getKeyID(), subkeyPassphrase);
+
+        List<KeyFlag> keyFlags = KeyFlag.fromBitmask(keySpec.getSubpackets().getKeyFlags());
+        KeyFlag firstFlag = keyFlags.remove(0);
+        KeyFlag[] otherFlags = keyFlags.toArray(new KeyFlag[0]);
+
+        return addSubKey(keyPair, subpacketsCallback, subKeyProtector, secretKeyRingProtector, firstFlag, otherFlags);
+    }
+
+    @Override
+    public SecretKeyRingEditorInterface addSubKey(
+            PGPKeyPair subkey,
             @Nullable SelfSignatureSubpackets.Callback bindingSignatureCallback,
             SecretKeyRingProtector subkeyProtector,
             SecretKeyRingProtector primaryKeyProtector,

pgpainless-core/src/main/java/org/pgpainless/key/modification/secretkeyring/SecretKeyRingEditorInterface.java

@@ -67,6 +67,11 @@ public interface SecretKeyRingEditorInterface {
                                            SecretKeyRingProtector secretKeyRingProtector)
             throws InvalidAlgorithmParameterException, NoSuchAlgorithmException, PGPException, IOException;
 
+    SecretKeyRingEditorInterface addSubKey(@Nonnull KeySpec keySpec,
+                                           @Nullable Passphrase subkeyPassphrase,
+                                           @Nullable SelfSignatureSubpackets.Callback subpacketsCallback,
+                                           SecretKeyRingProtector secretKeyRingProtector) throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException, IOException;
+
     SecretKeyRingEditorInterface addSubKey(PGPKeyPair subkey,
                                            @Nullable SelfSignatureSubpackets.Callback bindingSignatureCallback,
                                            SecretKeyRingProtector subkeyProtector,

pgpainless-core/src/test/java/org/pgpainless/key/generation/KeyGenerationSubpacketsTest.java

@@ -4,29 +4,154 @@
 
 package org.pgpainless.key.generation;
 
-import org.bouncycastle.openpgp.PGPException;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
-import org.bouncycastle.openpgp.PGPSecretKeyRing;
+import static org.junit.jupiter.api.Assertions.assertEquals;
-import org.bouncycastle.openpgp.PGPSignature;
+import static org.junit.jupiter.api.Assertions.assertFalse;
-import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
-import org.pgpainless.PGPainless;
+import static org.junit.jupiter.api.Assertions.assertNull;
-import org.pgpainless.key.info.KeyRingInfo;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.io.IOException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.NoSuchAlgorithmException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 
-import static org.junit.jupiter.api.Assertions.assertNotNull;
+import org.bouncycastle.bcpg.sig.IssuerFingerprint;
+import org.bouncycastle.openpgp.PGPException;
+import org.bouncycastle.openpgp.PGPPublicKey;
+import org.bouncycastle.openpgp.PGPSecretKeyRing;
+import org.bouncycastle.openpgp.PGPSignature;
+import org.bouncycastle.openpgp.PGPSignatureSubpacketVector;
+import org.junit.jupiter.api.Test;
+import org.pgpainless.PGPainless;
+import org.pgpainless.algorithm.HashAlgorithm;
+import org.pgpainless.algorithm.KeyFlag;
+import org.pgpainless.key.generation.type.KeyType;
+import org.pgpainless.key.generation.type.eddsa.EdDSACurve;
+import org.pgpainless.key.generation.type.xdh.XDHSpec;
+import org.pgpainless.key.info.KeyRingInfo;
+import org.pgpainless.key.protection.SecretKeyRingProtector;
+import org.pgpainless.signature.subpackets.SelfSignatureSubpackets;
 
 public class KeyGenerationSubpacketsTest {
 
     @Test
-    public void verifyDefaultSubpackets()
+    public void verifyDefaultSubpacketsForUserIdSignatures()
             throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException {
         PGPSecretKeyRing secretKeys = PGPainless.generateKeyRing().modernKeyRing("Alice", null);
-        KeyRingInfo info = PGPainless.inspectKeyRing(secretKeys);
 
+        KeyRingInfo info = PGPainless.inspectKeyRing(secretKeys);
         PGPSignature userIdSig = info.getLatestUserIdCertification("Alice");
         assertNotNull(userIdSig);
-        assertNotNull(userIdSig.getHashedSubPackets().getIssuerFingerprint());
+        int keyFlags = userIdSig.getHashedSubPackets().getKeyFlags();
+        int[] preferredHashAlgorithms = userIdSig.getHashedSubPackets().getPreferredHashAlgorithms();
+        int[] preferredSymmetricAlgorithms = userIdSig.getHashedSubPackets().getPreferredSymmetricAlgorithms();
+        int[] preferredCompressionAlgorithms = userIdSig.getHashedSubPackets().getPreferredCompressionAlgorithms();
+        assureSignatureHasDefaultSubpackets(userIdSig, secretKeys, KeyFlag.CERTIFY_OTHER);
+        assertTrue(userIdSig.getHashedSubPackets().isPrimaryUserID());
 
+        assertEquals("Alice", info.getPrimaryUserId());
+
+        secretKeys = PGPainless.modifyKeyRing(secretKeys)
+                .addUserId("Bob",
+                        new SelfSignatureSubpackets.Callback() {
+                            @Override
+                            public void modifyHashedSubpackets(SelfSignatureSubpackets hashedSubpackets) {
+                                hashedSubpackets.setPrimaryUserId();
+                            }
+                        },
+                        SecretKeyRingProtector.unprotectedKeys())
+                .addUserId("Alice", SecretKeyRingProtector.unprotectedKeys())
+                .done();
+
+        info = PGPainless.inspectKeyRing(secretKeys);
+
+        userIdSig = info.getLatestUserIdCertification("Alice");
+        assertNotNull(userIdSig);
+        assureSignatureHasDefaultSubpackets(userIdSig, secretKeys, KeyFlag.CERTIFY_OTHER);
+        assertFalse(userIdSig.getHashedSubPackets().isPrimaryUserID());
+        assertEquals(keyFlags, userIdSig.getHashedSubPackets().getKeyFlags());
+        assertArrayEquals(preferredHashAlgorithms, userIdSig.getHashedSubPackets().getPreferredHashAlgorithms());
+        assertArrayEquals(preferredSymmetricAlgorithms, userIdSig.getHashedSubPackets().getPreferredSymmetricAlgorithms());
+        assertArrayEquals(preferredCompressionAlgorithms, userIdSig.getHashedSubPackets().getPreferredCompressionAlgorithms());
+
+        userIdSig = info.getLatestUserIdCertification("Bob");
+        assertNotNull(userIdSig);
+        assureSignatureHasDefaultSubpackets(userIdSig, secretKeys, KeyFlag.CERTIFY_OTHER);
+        assertTrue(userIdSig.getHashedSubPackets().isPrimaryUserID());
+        assertArrayEquals(preferredHashAlgorithms, userIdSig.getHashedSubPackets().getPreferredHashAlgorithms());
+        assertArrayEquals(preferredSymmetricAlgorithms, userIdSig.getHashedSubPackets().getPreferredSymmetricAlgorithms());
+        assertArrayEquals(preferredCompressionAlgorithms, userIdSig.getHashedSubPackets().getPreferredCompressionAlgorithms());
+
+        assertEquals("Bob", info.getPrimaryUserId());
+
+        secretKeys = PGPainless.modifyKeyRing(secretKeys)
+                .addUserId("Alice", new SelfSignatureSubpackets.Callback() {
+                    @Override
+                    public void modifyHashedSubpackets(SelfSignatureSubpackets hashedSubpackets) {
+                        hashedSubpackets.setPrimaryUserId();
+                        hashedSubpackets.setPreferredHashAlgorithms(HashAlgorithm.SHA1);
+                    }
+                }, SecretKeyRingProtector.unprotectedKeys())
+                .done();
+        info = PGPainless.inspectKeyRing(secretKeys);
+        assertEquals("Alice", info.getPrimaryUserId());
+        assertEquals(Collections.singleton(HashAlgorithm.SHA1), info.getPreferredHashAlgorithms("Alice"));
+    }
+
+    @Test
+    public void verifyDefaultSubpacketsForSubkeyBindingSignatures() throws PGPException, InvalidAlgorithmParameterException, NoSuchAlgorithmException, IOException {
+        PGPSecretKeyRing secretKeys = PGPainless.generateKeyRing().modernKeyRing("Alice", null);
+        KeyRingInfo info = PGPainless.inspectKeyRing(secretKeys);
+        List<PGPPublicKey> keysBefore = info.getPublicKeys();
+
+        secretKeys = PGPainless.modifyKeyRing(secretKeys)
+                .addSubKey(KeySpec.getBuilder(KeyType.EDDSA(EdDSACurve._Ed25519), KeyFlag.SIGN_DATA).build(), null, SecretKeyRingProtector.unprotectedKeys())
+                .done();
+
+
+        info = PGPainless.inspectKeyRing(secretKeys);
+        List<PGPPublicKey> keysAfter = new ArrayList<>(info.getPublicKeys());
+        keysAfter.removeAll(keysBefore);
+        assertEquals(1, keysAfter.size());
+        PGPPublicKey newSigningKey = keysAfter.get(0);
+
+        PGPSignature bindingSig = info.getCurrentSubkeyBindingSignature(newSigningKey.getKeyID());
+        assertNotNull(bindingSig);
+        assureSignatureHasDefaultSubpackets(bindingSig, secretKeys, KeyFlag.SIGN_DATA);
+        assertNotNull(bindingSig.getHashedSubPackets().getEmbeddedSignatures().get(0));
+
+        secretKeys = PGPainless.modifyKeyRing(secretKeys)
+                .addSubKey(KeySpec.getBuilder(KeyType.XDH(XDHSpec._X25519), KeyFlag.ENCRYPT_COMMS).build(), null,
+                        new SelfSignatureSubpackets.Callback() {
+                            @Override
+                            public void modifyHashedSubpackets(SelfSignatureSubpackets hashedSubpackets) {
+                                hashedSubpackets.setIssuerFingerprint((IssuerFingerprint) null);
+                            }
+                        }, SecretKeyRingProtector.unprotectedKeys())
+                .done();
+
+        info = PGPainless.inspectKeyRing(secretKeys);
+        keysAfter = new ArrayList<>(info.getPublicKeys());
+        keysAfter.removeAll(keysBefore);
+        keysAfter.remove(newSigningKey);
+        assertEquals(1, keysAfter.size());
+        PGPPublicKey newEncryptionKey = keysAfter.get(0);
+        bindingSig = info.getCurrentSubkeyBindingSignature(newEncryptionKey.getKeyID());
+        assertNotNull(bindingSig);
+        assertNull(bindingSig.getHashedSubPackets().getIssuerFingerprint());
+        assertEquals(KeyFlag.toBitmask(KeyFlag.ENCRYPT_COMMS), bindingSig.getHashedSubPackets().getKeyFlags());
+    }
+
+    private void assureSignatureHasDefaultSubpackets(PGPSignature signature, PGPSecretKeyRing secretKeys, KeyFlag... keyFlags) {
+        PGPSignatureSubpacketVector hashedSubpackets = signature.getHashedSubPackets();
+        assertNotNull(hashedSubpackets.getIssuerFingerprint());
+        assertEquals(secretKeys.getPublicKey().getKeyID(), hashedSubpackets.getIssuerKeyID());
+        assertArrayEquals(
+                secretKeys.getPublicKey().getFingerprint(),
+                hashedSubpackets.getIssuerFingerprint().getFingerprint());
+        assertEquals(hashedSubpackets.getKeyFlags(), KeyFlag.toBitmask(keyFlags));
     }
 }