Fix isForwardingKey to support keys without valid encryption subkeys

The function used to mistakenly check only for the absence of valid encryption keys.
This would work in practice as forwarding subkeys are not valid for encryption, and
standard private address keys are expected to have valid encryption subkeys.
However, it caused issues in a specific account setup where the user imported
an expiring key, which was then wrongly considered to be a forwarding key,
which prevented the user from generating a valid SKL as a result (the FE filtered out
the key on SKL generation, while the BE expected the key to be included).

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,33 @@ 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) => {
+        const flags = maybeForwardingSubkey.bindingSignatures[0].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;