diff --git a/modules/kms-keyring/src/kms_keyring.ts b/modules/kms-keyring/src/kms_keyring.ts index f8d9073fa..b7dba0b5e 100644 --- a/modules/kms-keyring/src/kms_keyring.ts +++ b/modules/kms-keyring/src/kms_keyring.ts @@ -172,6 +172,8 @@ export function KmsKeyringClass `${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 } } diff --git a/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts b/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts index b4fd74fef..c15ab0f8a 100644 --- a/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts +++ b/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(CiphertextBlob).toString('utf8') + } } } class TestKmsKeyring extends KmsKeyringClass(Keyring as KeyRingConstructible) {} @@ -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) {} + + 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) + }) }) diff --git a/modules/material-management/src/multi_keyring.ts b/modules/material-management/src/multi_keyring.ts index 581995f99..8b054c407 100644 --- a/modules/material-management/src/multi_keyring.ts +++ b/modules/material-management/src/multi_keyring.ts @@ -113,6 +113,8 @@ function buildPrivateOnDecrypt () { 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 @@ -120,12 +122,28 @@ function buildPrivateOnDecrypt () { 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 } } diff --git a/modules/material-management/test/multi_keyring.test.ts b/modules/material-management/test/multi_keyring.test.ts index 64c8873ca..c7d7dca31 100644 --- a/modules/material-management/test/multi_keyring.test.ts +++ b/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] {