Unawaited promises when signature verification fails

[kdeberk]

Hello,

I am trying to test the behavior of my program if an incoming PGP message is not signed with the correct key. The tests, (I'm testing with jest --detectOpenHandles) complain about unhandled errors.

Error: Unhandled error. (Error: Could not find signing key with key ID 3bba80420dcab22b
    at Error (<project>/node_modules/openpgp/src/message.js:778:19)
    at <project>/node_modules/openpgp/src/message.js:776:15
    at createVerificationObject (<project>/node_modules/openpgp/src/message.js:842:12)
    at vu.map (<anonymous>)
    at map <project>/node_modules/openpgp/src/message.js:841:6)
    at Eh.createVerificationObjects [as verify] (<project>/node_modules/openpgp/src/message.js:631:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at config (<project>/node_modules/openpgp/src/openpgp.js:348:103)
    at decrypt (<project>/src/Encryption.ts:30:34)
    at handleIncomingFile (<project>/src/Main.ts:41:46)
    at Object.<anonymous> (<project>/test-intg/Integration.test.ts:63:9))

I am trying to await all signature promises

    const { data, signatures } = await decrypt({
        message,
        decryptionKeys: decKey,
        verificationKeys: verKey,
        expectSigned: true,
        config: { allowUnauthenticatedStream: true },
    });
    const validSignatures = new Promise<void>((resolve, reject) => {
        const promises = []
        for(const result of signatures) {
            promises.push(result.signature)
            promises.push(result.verified)
        }
        Promise.all(promises)
            .then((x) => {
                resolve()
            }).catch(reject)
    })

   ....
   await validSignatures

but it seems that even in that case, promises are not being handled, and I don't know which ones I am missing.

[twiss]

A promise is only marked as "handled" once you await it, or call .then or .catch on it. So, if it's rejected before that point (depending on how long the code in the .... takes), it's possible that the error is considered unhandled. To fix that, you can add something like

validSignatures.catch(() => {});

which marks the promise as being "handled", even if it's only actually handled later when you await it.

But, note that all signature-related promises should already be marked as handled when they're returned by OpenPGP.js. So, it shouldn't be necessary to await all of them, just the ones you care about.

[kdeberk]

Thanks for the quick response! I'll try it out and inform you of the outcome

[kdeberk]

I doesn't seem to have worked. In the openpgpjs codebase, I see that the two promises are already treated with this approach, and that doesn't seem to have had the desired effect.

    verifiedSig.signature.catch(() => {});
    verifiedSig.verified.catch(() => {});

I modified some of my code so that the jest `openHandler check could add some more info and I get the following slightly different stacktrace:

Error: Could not find signing key with key ID 5bce63f60b6ca427
    at Error (<project>/node_modules/openpgp/src/message.js:778:19)
    at <project>/node_modules/openpgp/src/message.js:776:15
    at createVerificationObject (<project>/node_modules/openpgp/src/message.js:842:12)
    at vu.map (<anonymous>)
    at map (/<project>/node_modules/openpgp/src/message.js:841:6)
    at Eh.createVerificationObjects [as verify] (<project>/node_modules/openpgp/src/message.js:631:14)
    at processTicksAndRejections (node:internal/process/task_queues:95:5) <<<-----
    at config (<project>/node_modules/openpgp/src/openpgp.js:348:103)
    at decrypt (<project>/src/Encryption.ts:30:34)
    at handleIncomingFile (<project>/src/Main.ts:41:46)
    at Object.<anonymous> (<project>/test-intg/Integration.test.ts:63:9)

Would the fact that the projectTicksAndRejections is called within the openpgpjs part of the stacktrace show that the openpgpjs itself is somehow not catching the exception? As far as I can see, I'm immediately handling any promise that I receive from the decrypt call so I don't see how I could be missing one.

[twiss]

The stack trace represents the point where the error was created, not the point where an unhandled promise was rejected. Unfortunately, the latter can be a bit difficult to track. Could you post your complete amended code (i.e. the version where you tried to handle the promise)?

[twiss]

But - given that the error no longer mentions an unhandled rejection - are you sure this error is not just your test failing? You might need to catch the error and check that it matches the error you're expecting, or something like that.

[kdeberk]

Hey Twiss, thanks for your patience. I think I've figured it out.

My application uploads the stream that the decrypt call returns, to S3 using the AWS S3 SDK. All the following things seem to be necessary to get it to work:

• pipe the decrypt stream to a new PassThrough and upload the PassThrough stream. If we instead upload the decrypt stream then the S3 upload will block under normal (= non-error) circumstances,
• Add an on("error", () => {}) handler to the decrypt stream, otherwise any streaming decrypt errors will not be picked up correctly
• Await all the signature promises (both signature & verified) + the S3 upload promise using Promise.all.

Basically, the code looks like this:

const { data, signatures } = await decrypt({
 message,
 decryptionKeys: decKey,
 verificationKeys: verKey,
 expectSigned: true,
 config: { allowUnauthenticatedStream: true },
});

const promises = []
for(const sig of signatures) {
 promises.push(sig.verified, sig.signature)
}
const validSignature = Promise.all(promises)

const pt = new PassThrough()
data.pipe(pt)
data.on("error", (err) => {
 Logs.logError(err, "decryption failure")
})

await Promise.all([validSignature, s3.upload(pt, dstObj)])

[twiss]

OK 👍 Great that you got it to work. Just so you know, the expectSigned: true already causes the data stream to error in case of signature failures, with one difference: it succeeds if at least one signature verifies using the given verification keys, instead of checking all of them. (So, if that's sufficient for you, you should be able to get rid of all the other signature handling code, I think.) And, note that that should also cause the S3 upload to fail, which may or may not be what you want.

But if you want to verify all signatures then it is needed indeed 😊 However, note that with your current code, it wouldn't cancel the upload, I think - you'd need to cancel it manually or insert an error on the stream. Or, just delete the file afterwards, I suppose (it shouldn't make a big difference in bandwidth since the signatures are at the end anyway).

[kdeberk]

Hey Twiss,

it wouldn't cancel the upload, I think - you'd need to cancel it manually or insert an error on the stream

I was trying to test this by forcing an incorrect signature on an existing pgp message, but I've been unable to do this so far. I guess that the the signature is nested within the symEncryptedIntegrityProtectedData packet, would that be correct?

[larabr]

Correct, I think the easiest way to test signature verification failure is to add an "bad" signature when encrypting:

const dummySignature = await openpgp.sign({
  // sign any message different from the one you'll encrypt, or use a mismatching private key
  message: await openpgp.createMessage({ text: 'whatever' }), 
  signingKeys: ...
});

const encrypted = await openpgp.encrypt({
  message: ...,
  signature: dummySignature, // important, to include the signature in the encrypted message
  encryptionKeys: ..., 
  // do not pass `signingKeys`
})

// decrypt and expect verification error
const { data, signatures } = await decrypt({ ... })

[kdeberk]

Thanks! That worked! Thanks so much. I can now test that the s3 upload is aborted when the digest is not correct.