fix: Better error messageing (#212)

resolves #152

When attempting to decrypt a set of encrypted data keys,
if any attempt is successful,
then the entire operation should be considered successful.
However if no data key can be obtained,
and there were errors,
these errors should be visible to the caller.

An excellent example is attempting to decrypt
with a KMS CMK alias arn.
The KMS Keyring will be unable to decrypt,
but was returning no error.
This resulted the Error 'Unencrypted data key is invalid.'
This is because the default CMM sees that the material
does not have any unencrypted data key.

A better error message would be the one from KMS in this case.

Updating with tests both the KMS Keyrings,
as well as the MultiKeyring.

Author: seebees

modules/kms-keyring/src/kms_keyring.ts

@@ -172,6 +172,8 @@ export function KmsKeyringClass<S extends SupportedAlgorithmSuites, Client exten
           return this.isDiscovery || keyIds.includes(providerInfo)
         })
 
+      let cmkErrors: Error[] = []
+
       for (const edk of decryptableEDKs) {
         let dataKey: RequiredDecryptResponse|false = false
         try {
@@ -182,10 +184,11 @@ export function KmsKeyringClass<S extends SupportedAlgorithmSuites, Client exten
             grantTokens
           )
         } catch (e) {
-          // there should be some debug here?  or wrap?
-          // Failures decrypt should not short-circuit the process
-          // If the caller does not have access they may have access
-          // through another Keyring.
+          /* Failures onDecrypt should not short-circuit the process
+           * If the caller does not have access they may have access
+           * through another Keyring.
+           */
+          cmkErrors.push(e)
         }
 
         /* Check for early return (Postcondition): clientProvider may not return a client. */
@@ -204,6 +207,20 @@ export function KmsKeyringClass<S extends SupportedAlgorithmSuites, Client exten
         return material
       }
 
+      /* Postcondition: A CMK must provide a valid data key or KMS must not have raised any errors.
+       * If I have a data key,
+       * decrypt errors can be ignored.
+       * However, if I was unable to decrypt a data key AND I have errors,
+       * these errors should bubble up.
+       * Otherwise, the only error customers will see is that
+       * the material does not have an unencrypted data key.
+       * So I return a concatenated Error message
+       */
+      needs(material.hasValidKey() || (!material.hasValidKey() && !cmkErrors.length)
+        , cmkErrors
+          .reduce((m, e, i) => `${m} Error #${i + 1} \n ${e.stack} \n`,
+            'Unable to decrypt data key and one or more KMS CMKs had an error. \n '))
+
       return material
     }
   }

modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts

@@ -138,10 +138,20 @@ describe('KmsKeyring: _onDecrypt',
       const discovery = true
       const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
 
+      let edkCount = 0
       const clientProvider: any = () => {
         return { decrypt }
-        function decrypt () {
-          throw new Error('failed to decrypt')
+        function decrypt ({ CiphertextBlob, EncryptionContext, GrantTokens }: any) {
+          if (edkCount === 0) {
+            edkCount += 1
+            throw new Error('failed to decrypt')
+          }
+          expect(EncryptionContext).to.deep.equal(context)
+          expect(GrantTokens).to.equal(grantTokens)
+          return {
+            Plaintext: new Uint8Array(suite.keyLengthBytes),
+            KeyId: Buffer.from(<Uint8Array>CiphertextBlob).toString('utf8')
+          }
         }
       }
       class TestKmsKeyring extends KmsKeyringClass(Keyring as KeyRingConstructible<NodeAlgorithmSuite>) {}
@@ -160,11 +170,11 @@ describe('KmsKeyring: _onDecrypt',
 
       const material = await testKeyring.onDecrypt(
         new NodeDecryptionMaterial(suite, context),
-        [edk]
+        [edk, edk]
       )
 
-      expect(material.hasUnencryptedDataKey).to.equal(false)
-      expect(material.keyringTrace).to.have.lengthOf(0)
+      expect(material.hasUnencryptedDataKey).to.equal(true)
+      expect(material.keyringTrace).to.have.lengthOf(1)
     })
 
     it('Check for early return (Postcondition): clientProvider may not return a client.', async () => {
@@ -279,4 +289,49 @@ describe('KmsKeyring: _onDecrypt',
         [edk]
       )).to.rejectedWith(Error, 'Key length does not agree with the algorithm specification.')
     })
+
+    it('Postcondition: A CMK must provide a valid data key or KMS must not have raised any errors.', async () => {
+      const generatorKeyId = 'arn:aws:kms:us-east-1:123456789012:alias/example-alias'
+      const context = { some: 'context' }
+      const grantTokens = ['grant']
+      const discovery = true
+      const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
+
+      const clientProviderError: any = () => {
+        return { decrypt }
+        function decrypt () {
+          throw new Error('failed to decrypt')
+        }
+      }
+      class TestKmsKeyring extends KmsKeyringClass(Keyring as KeyRingConstructible<NodeAlgorithmSuite>) {}
+
+      const testKeyring = new TestKmsKeyring({
+        clientProvider: clientProviderError,
+        grantTokens,
+        discovery
+      })
+
+      const edk = new EncryptedDataKey({
+        providerId: 'aws-kms',
+        providerInfo: generatorKeyId,
+        encryptedDataKey: Buffer.from(generatorKeyId)
+      })
+
+      await expect(testKeyring.onDecrypt(
+        new NodeDecryptionMaterial(suite, context),
+        [edk, edk]
+      )).to.rejectedWith(Error, 'Unable to decrypt data key and one or more KMS CMKs had an error.')
+
+      /* This will make the decrypt loop not have an error.
+       * This will exercise the `(!material.hasValidKey() && !cmkErrors.length)` `needs` condition.
+       */
+      const clientProviderNoError: any = () => false
+      await expect(new TestKmsKeyring({
+        clientProvider: clientProviderNoError,
+        grantTokens,
+        discovery
+      }).onDecrypt(new NodeDecryptionMaterial(suite, context),
+        [edk, edk]
+      )).to.not.rejectedWith(Error)
+    })
   })

modules/material-management/src/multi_keyring.ts

@@ -113,19 +113,37 @@ function buildPrivateOnDecrypt<S extends SupportedAlgorithmSuites> () {
     const children = this.children.slice()
     if (this.generator) children.unshift(this.generator)
 
+    let childKeyringErrors: Error[] = []
+
     for (const keyring of children) {
     /* Check for early return (Postcondition): Do not attempt to decrypt once I have a valid key. */
       if (material.hasValidKey()) return material
 
       try {
         await keyring.onDecrypt(material, encryptedDataKeys)
       } catch (e) {
-      // there should be some debug here?  or wrap?
-      // Failures onDecrypt should not short-circuit the process
-      // If the caller does not have access they may have access
-      // through another Keyring.
+      /* Failures onDecrypt should not short-circuit the process
+       * If the caller does not have access they may have access
+       * through another Keyring.
+       */
+        childKeyringErrors.push(e)
       }
     }
+
+    /* Postcondition: A child keyring must provide a valid data key or no child keyring must have raised an error.
+     * If I have a data key,
+     * decrypt errors can be ignored.
+     * However, if I was unable to decrypt a data key AND I have errors,
+     * these errors should bubble up.
+     * Otherwise, the only error customers will see is that
+     * the material does not have an unencrypted data key.
+     * So I return a concatenated Error message
+     */
+    needs(material.hasValidKey() || (!material.hasValidKey() && !childKeyringErrors.length)
+      , childKeyringErrors
+        .reduce((m, e, i) => `${m} Error #${i + 1} \n ${e.stack} \n`,
+          'Unable to decrypt data key and one or more child keyrings had an error. \n '))
+
     return material
   }
 }

modules/material-management/test/multi_keyring.test.ts

@@ -358,6 +358,37 @@ describe('MultiKeyring: onDecrypt', () => {
     expect(unwrapDataKey(test.getUnencryptedDataKey())).to.deep.equal(unencryptedDataKey)
     expect(called).to.equal(true)
   })
+
+  it('Postcondition: A child keyring must provide a valid data key or no child keyring must have raised an error.', async () => {
+    const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
+    const [edk0] = makeEDKandTraceForDecrypt(0)
+    const material = new NodeDecryptionMaterial(suite, {})
+    const childNotSucceeded = keyRingFactory({
+      async onDecrypt () {
+        // Because this keyring does not return a value, it will result in an error
+      },
+      onEncrypt: never
+    })
+    const children = [childNotSucceeded]
+
+    const mkeyring = new MultiKeyringNode({ children })
+
+    await expect(mkeyring.onDecrypt(material, [edk0])).to.rejectedWith(Error, 'Unable to decrypt data key and one or more child keyrings had an error.')
+
+    /* This will make the decrypt loop not have an error.
+     * This will exercise the `(!material.hasValidKey() && !childKeyringErrors.length)` `needs` condition.
+     */
+    const childNoDataKey = keyRingFactory({
+      async onDecrypt (material: NodeDecryptionMaterial /*, encryptedDataKeys: EncryptedDataKey[] */) {
+        return material
+      },
+      onEncrypt: never
+    })
+
+    const mkeyringNoErrors = new MultiKeyringNode({ children: [ childNoDataKey ] })
+
+    await expect(mkeyringNoErrors.onDecrypt(material, [edk0])).to.not.rejectedWith(Error)
+  })
 })
 
 function makeEDKandTraceForEncrypt (num: number): [EncryptedDataKey, KeyringTrace] {