From ebf0687a9317cd882d0b8f5b00ef96c228552237 Mon Sep 17 00:00:00 2001
From: seebees <ryanemer@amazon.com>
Date: Fri, 24 Apr 2020 16:35:30 -0700
Subject: [PATCH] fix: Missing errors in raw keyrings

On decrypt, error messages can be passed over.
However, if the keyring attempts to decrypt,
and fails how does anyone know what happend?

This is a similar solution to #212
---
 .../raw-keyring/src/raw_keyring_decorators.ts | 29 ++++++++++++++++---
 .../test/raw_keyring_decorators.test.ts       |  8 +++--
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/modules/raw-keyring/src/raw_keyring_decorators.ts b/modules/raw-keyring/src/raw_keyring_decorators.ts
index c3ecf50af..008e35d20 100644
--- a/modules/raw-keyring/src/raw_keyring_decorators.ts
+++ b/modules/raw-keyring/src/raw_keyring_decorators.ts
@@ -8,6 +8,7 @@ import {
   KeyringTrace,
   KeyringTraceFlag,
   EncryptedDataKey,
+  needs,
 } from '@aws-crypto/material-management'
 
 export interface RawKeyRing<S extends SupportedAlgorithmSuites> {
@@ -54,17 +55,37 @@ export function _onDecrypt<
     /* Check for early return (Postcondition): If there are not EncryptedDataKeys for this keyring, do nothing. */
     if (!edks.length) return material
 
+    const cmkErrors: Error[] = []
+
     for (const edk of edks) {
       try {
         return await this._unwrapKey(material, edk)
       } 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)
       }
     }
 
+    /* Postcondition: An EDK must provide a valid data key or _unwrapKey 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 ${this.keyName} ${this.keyNamespace}.\n `
+      )
+    )
+
     return material
   }
 }
diff --git a/modules/raw-keyring/test/raw_keyring_decorators.test.ts b/modules/raw-keyring/test/raw_keyring_decorators.test.ts
index 0f9a80528..67efb7e84 100644
--- a/modules/raw-keyring/test/raw_keyring_decorators.test.ts
+++ b/modules/raw-keyring/test/raw_keyring_decorators.test.ts
@@ -225,7 +225,7 @@ describe('_onDecrypt', () => {
     expect(filterCalled).to.equal(1)
   })
 
-  it('errors in _unwrapKey should not cause _onDecrypt to throw.', async () => {
+  it('Postcondition: An EDK must provide a valid data key or _unwrapKey must not have raised any errors.', async () => {
     const suite = new NodeAlgorithmSuite(
       AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16
     )
@@ -258,8 +258,10 @@ describe('_onDecrypt', () => {
       _filter,
     } as any
 
-    const test = await testKeyring._onDecrypt(material, [edk])
-    expect(test.hasValidKey()).to.equal(false)
+    await expect(testKeyring._onDecrypt(material, [edk])).to.rejectedWith(
+      Error,
+      'Unable to decrypt data key'
+    )
     expect(unwrapCalled).to.equal(1)
     expect(filterCalled).to.equal(1)
   })