fix: Better error messageing

modules/kms-keyring/src/kms_keyring.ts

@@ -171,6 +171,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 {
@@ -181,10 +183,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. */
@@ -203,6 +206,20 @@ export function KmsKeyringClass<S extends SupportedAlgorithmSuites, Client exten
         return material
       }
 
+      /* Postcondition: A CMK must provide a valid data key.
+       * 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,37 @@ 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.', 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 clientProvider: any = () => {
+        return { decrypt }
+        function decrypt () {
+          throw new Error('failed to decrypt')
+        }
+      }
+      class TestKmsKeyring extends KmsKeyringClass(Keyring as KeyRingConstructible<NodeAlgorithmSuite>) {}
+
+      const testKeyring = new TestKmsKeyring({
+        clientProvider,
+        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.')
+    })
   })

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.
+     * 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,23 @@ 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.', async () => {
+    const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16)
+    const [edk0] = makeEDKandTrace(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.')
+  })
 })
 
 function makeEDKandTrace (num: number): [EncryptedDataKey, KeyringTrace] {