Fix isForwardingKey to support keys without valid encryption subkeys

lib/key/forwarding.ts

@@ -1,4 +1,4 @@
-import { type PrivateKey, type UserID, type SecretSubkeyPacket, type MaybeArray, type SubkeyOptions, type Subkey, KDFParams, SecretKeyPacket, enums } from '../openpgp';
+import { type PrivateKey, type UserID, type MaybeArray, type SubkeyOptions, type Subkey, KDFParams, enums, SecretSubkeyPacket, type Key, config as defaultConfig } from '../openpgp';
 import { generateKey, reformatKey } from './utils';
 import { serverTime } from '../serverTime';
 import { bigIntToUint8Array, mod, modInv, uint8ArrayToBigInt } from '../bigInteger';
@@ -16,20 +16,22 @@ export async function computeProxyParameter(
     return proxyParameter;
 }
 
-async function getEncryptionKeysForForwarding(forwarderKey: PrivateKey, date: Date) {
+const doesKeyPacketSupportForwarding = (maybeForwardeeKey: Key | Subkey) => {
     const curveName = 'curve25519Legacy';
+
+    return (maybeForwardeeKey.keyPacket instanceof SecretSubkeyPacket) && // only ECDH can forward, and they are always subkeys
+        !maybeForwardeeKey.keyPacket.isDummy() &&
+        maybeForwardeeKey.keyPacket.version === 4 && // TODO add support for v6
+        maybeForwardeeKey.getAlgorithmInfo().algorithm === 'ecdh' &&
+        maybeForwardeeKey.getAlgorithmInfo().curve === curveName;
+};
+
+async function getEncryptionKeysForForwarding(forwarderKey: PrivateKey, date: Date) {
     const forwarderEncryptionKeys = (await Promise.all(forwarderKey.getKeyIDs().map(
         (maybeEncryptionKeyID) => forwarderKey.getEncryptionKey(maybeEncryptionKeyID, date).catch(() => null)
     ))).filter(((maybeKey): maybeKey is (PrivateKey | Subkey) => !!maybeKey));
 
-    if (forwarderEncryptionKeys.some((maybeForwarderSubkey) => (
-        maybeForwarderSubkey === null ||
-        !(maybeForwarderSubkey.keyPacket instanceof SecretKeyPacket) || // SecretSubkeyPacket is a subclass
-        maybeForwarderSubkey.keyPacket.isDummy() ||
-        maybeForwarderSubkey.keyPacket.version !== 4 || // TODO add support for v6
-        maybeForwarderSubkey.getAlgorithmInfo().algorithm !== 'ecdh' ||
-        maybeForwarderSubkey.getAlgorithmInfo().curve !== curveName
-    ))) {
+    if (!forwarderEncryptionKeys.every(doesKeyPacketSupportForwarding)) {
         throw new Error('One or more encryption key packets are unsuitable for forwarding');
     }
 
@@ -53,15 +55,37 @@ export async function doesKeySupportForwarding(forwarderKey: PrivateKey, date: D
 }
 
 /**
- * Whether all the encryption-capable (sub)keys are setup as forwarding keys.
+ * Whether all the decryption-capable (sub)keys are setup as forwardee keys.
  * This function also supports encrypted private keys.
  */
-export const isForwardingKey = (keyToCheck: PrivateKey, date: Date = serverTime()) => (
-    getEncryptionKeysForForwarding(keyToCheck, date)
-        // @ts-ignore missing `bindingSignatures` definition
-        .then((keys) => keys.every((key) => key.bindingSignatures[0].keyFlags & enums.keyFlags.forwardedCommunication))
-        .catch(() => false)
-);
+export const isForwardingKey = async (keyToCheck: PrivateKey, date: Date = serverTime()) => {
+    // NB: we need this function to be strict since it's used by the client to determine whether a key
+    // should be included in the SKL (forwarding keys are not included).
+    // For this reason, we need to e.g. check binding signatures.
+
+    const allDecryptionKeys = await keyToCheck
+        .getDecryptionKeys(undefined, date, undefined, {
+            ...defaultConfig,
+            allowForwardedMessages: true
+        })
+        .catch(() => []); // throws if no valid decryption keys are found
+
+    const hasForwardingKeyFlag = (maybeForwardingSubkey: Subkey) => (
+        maybeForwardingSubkey.bindingSignatures.length > 0 &&
+            maybeForwardingSubkey.bindingSignatures.every(({ keyFlags }) => {
+                const flags = keyFlags?.[0];
+                if (!flags) {
+                    return false;
+                }
+                return (flags & enums.keyFlags.forwardedCommunication) !== 0;
+            })
+    );
+
+    const allValidKeys = allDecryptionKeys.every(
+        (key) => doesKeyPacketSupportForwarding(key) && hasForwardingKeyFlag(key as Subkey)
+    );
+    return allDecryptionKeys.length > 0 && allValidKeys;
+};
 
 /**
  * Generate a forwarding key for the final recipient ('forwardee'), as well as the corresponding proxy parameter,

test/key/forwarding.spec.ts

@@ -370,6 +370,20 @@ z5FbOJXSHsoez1SZ7GKgoxC+X0w=
     });
 
     it('isForwardingKey', async () => {
+        const signOnlyKey = await readPrivateKey({
+            armoredKey: `-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+xVgEZ4evPxYJKwYBBAHaRw8BAQdAz/OfKP1cqnXkjwiYbhvkPzPV4SBpc+IK
+zc9j/limEXIAAQD7k7p8GpP5W9iMDFfNQZ/q8xFIiAQcbPXG/bcPVgYRvRAs
+zQg8YUBhLml0PsLAEQQTFgoAgwWCZ4evPwMLCQcJkIHN0wt4lUcZRRQAAAAA
+ABwAIHNhbHRAbm90YXRpb25zLm9wZW5wZ3Bqcy5vcmd4nycM2KL0cTS8Ttv0
+mQFbx8Q+4bovdfed2qSvArkmPgMVCggEFgACAQIZAQKbAwIeARYhBNF4Mj8k
+jFoxVyK1FYHN0wt4lUcZAACbsQEA+O5gxkeu+KDS1fdyNhPasqhPMbj5nEyl
+fbFd4a5yy3kBAMSHD8k0/DSw7NPfO5XzHJ5hP0nhLjSFHOc8YjITQGcM
+=0mtr
+-----END PGP PRIVATE KEY BLOCK-----`
+        });
+
         const charlieKeyEncrypted = await readPrivateKey({ armoredKey: `-----BEGIN PGP PRIVATE KEY BLOCK-----
 
 xYYEZAdtGBYJKwYBBAHaRw8BAQdAcNgHyRGEaqGmzEqEwCobfUkyrJnY8faB
@@ -406,6 +420,7 @@ siLL+xMJ+Hy4AhsMAAAKagEA4Knj6S6nG24nuXfqkkytPlFTHwzurjv3+qqXwWL6
 =un5O
 -----END PGP PRIVATE KEY BLOCK-----` });
 
+        await expect(isForwardingKey(signOnlyKey)).to.eventually.be.false;
         await expect(isForwardingKey(charlieKeyEncrypted)).to.eventually.be.true;
         await expect(isForwardingKey(charlieKey)).to.eventually.be.true;
         await expect(isForwardingKey(bobKey)).to.eventually.be.false;