From 3bf691684bddd10ae5415a234958dcad67b19adf Mon Sep 17 00:00:00 2001 From: Emery Date: Tue, 6 Aug 2019 15:38:24 -0700 Subject: [PATCH 1/4] fix: Better error messageing 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. --- modules/kms-keyring/src/kms_keyring.ts | 25 +++++++-- .../test/kms_keyring.ondecrypt.test.ts | 53 +++++++++++++++++-- .../material-management/src/multi_keyring.ts | 26 +++++++-- .../test/multi_keyring.test.ts | 17 ++++++ 4 files changed, 108 insertions(+), 13 deletions(-) diff --git a/modules/kms-keyring/src/kms_keyring.ts b/modules/kms-keyring/src/kms_keyring.ts index 06a759b17..b7177fff2 100644 --- a/modules/kms-keyring/src/kms_keyring.ts +++ b/modules/kms-keyring/src/kms_keyring.ts @@ -171,6 +171,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..1fc204d95 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,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) {} + + 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.') + }) }) diff --git a/modules/material-management/src/multi_keyring.ts b/modules/material-management/src/multi_keyring.ts index 581995f99..f6d5e7032 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. + * 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 ce0841c16..bc6bbafbd 100644 --- a/modules/material-management/test/multi_keyring.test.ts +++ b/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] { From f281c8186414b6937b231c34631b05a0f6b52ad4 Mon Sep 17 00:00:00 2001 From: Ryan Emery Date: Wed, 18 Sep 2019 17:42:27 -0700 Subject: [PATCH 2/4] updates More tests and update condition names --- modules/kms-keyring/src/kms_keyring.ts | 2 +- .../test/kms_keyring.ondecrypt.test.ts | 18 +++++++++++++++--- .../material-management/src/multi_keyring.ts | 2 +- .../test/multi_keyring.test.ts | 16 +++++++++++++++- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/modules/kms-keyring/src/kms_keyring.ts b/modules/kms-keyring/src/kms_keyring.ts index b7177fff2..6d03a2275 100644 --- a/modules/kms-keyring/src/kms_keyring.ts +++ b/modules/kms-keyring/src/kms_keyring.ts @@ -206,7 +206,7 @@ export function KmsKeyringClass { + 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 clientProvider: any = () => { + const clientProviderError: any = () => { return { decrypt } function decrypt () { throw new Error('failed to decrypt') @@ -306,7 +306,7 @@ describe('KmsKeyring: _onDecrypt', class TestKmsKeyring extends KmsKeyringClass(Keyring as KeyRingConstructible) {} const testKeyring = new TestKmsKeyring({ - clientProvider, + clientProvider: clientProviderError, grantTokens, discovery }) @@ -321,5 +321,17 @@ describe('KmsKeyring: _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 f6d5e7032..313ba8e2f 100644 --- a/modules/material-management/src/multi_keyring.ts +++ b/modules/material-management/src/multi_keyring.ts @@ -130,7 +130,7 @@ function buildPrivateOnDecrypt () { } } - /* Postcondition: A child keyring must provide a valid data key. + /* Postcondition: A child keyring must provide a valid data key or not 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, diff --git a/modules/material-management/test/multi_keyring.test.ts b/modules/material-management/test/multi_keyring.test.ts index bc6bbafbd..df1dd40eb 100644 --- a/modules/material-management/test/multi_keyring.test.ts +++ b/modules/material-management/test/multi_keyring.test.ts @@ -359,7 +359,7 @@ describe('MultiKeyring: onDecrypt', () => { expect(called).to.equal(true) }) - it('Postcondition: A child keyring must provide a valid data key.', async () => { + it('Postcondition: A child keyring must provide a valid data key or not child keyring must have raised an error.', async () => { const suite = new NodeAlgorithmSuite(AlgorithmSuiteIdentifier.ALG_AES128_GCM_IV12_TAG16) const [edk0] = makeEDKandTrace(0) const material = new NodeDecryptionMaterial(suite, {}) @@ -374,6 +374,20 @@ describe('MultiKeyring: onDecrypt', () => { 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) }) }) From d0b8850185fd79c1e03dd6689c69a9816db7c9b8 Mon Sep 17 00:00:00 2001 From: Ryan Emery Date: Thu, 19 Sep 2019 10:28:13 -0700 Subject: [PATCH 3/4] lint --- modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts b/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts index 344b29d34..c15ab0f8a 100644 --- a/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts +++ b/modules/kms-keyring/test/kms_keyring.ondecrypt.test.ts @@ -331,7 +331,7 @@ describe('KmsKeyring: _onDecrypt', grantTokens, discovery }).onDecrypt(new NodeDecryptionMaterial(suite, context), - [edk, edk] + [edk, edk] )).to.not.rejectedWith(Error) }) }) From 88763e4584145feec83ee1a6291a749888b3e7c3 Mon Sep 17 00:00:00 2001 From: Ryan Emery Date: Thu, 19 Sep 2019 12:40:22 -0700 Subject: [PATCH 4/4] Better wording and integration fix --- modules/material-management/src/multi_keyring.ts | 2 +- modules/material-management/test/multi_keyring.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/material-management/src/multi_keyring.ts b/modules/material-management/src/multi_keyring.ts index 313ba8e2f..8b054c407 100644 --- a/modules/material-management/src/multi_keyring.ts +++ b/modules/material-management/src/multi_keyring.ts @@ -130,7 +130,7 @@ function buildPrivateOnDecrypt () { } } - /* Postcondition: A child keyring must provide a valid data key or not child keyring must have raised an error. + /* 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, diff --git a/modules/material-management/test/multi_keyring.test.ts b/modules/material-management/test/multi_keyring.test.ts index c6f3f841e..c7d7dca31 100644 --- a/modules/material-management/test/multi_keyring.test.ts +++ b/modules/material-management/test/multi_keyring.test.ts @@ -359,9 +359,9 @@ describe('MultiKeyring: onDecrypt', () => { expect(called).to.equal(true) }) - it('Postcondition: A child keyring must provide a valid data key or not child keyring must have raised an error.', async () => { + 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] = makeEDKandTrace(0) + const [edk0] = makeEDKandTraceForDecrypt(0) const material = new NodeDecryptionMaterial(suite, {}) const childNotSucceeded = keyRingFactory({ async onDecrypt () {