-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Better error messageing #212
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are missing a test for the case when |
||
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) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -358,6 +358,37 @@ describe('MultiKeyring: onDecrypt', () => { | |
expect(unwrapDataKey(test.getUnencryptedDataKey())).to.deep.equal(unencryptedDataKey) | ||
expect(called).to.equal(true) | ||
}) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, I think we want to test the case where |
||
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] { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unsure if this is the right behavior. Because the spec is silent about error handling, we should go with what makes sense for this implementation while being cognizant of what the C implementation is doing.
In C, OnDecrypt will always return success except possibly on line https://github.com/aws/aws-encryption-sdk-c/blob/master/aws-encryption-sdk-cpp/source/kms_keyring.cpp#L133
However, it will still log an error in these cases:
As such, the C implementation has the following behavior: "If in discovery mode, do not log errors from KMS Decrypt". Should the JS implementation also have this behavior? I.e. Should we bubble up CMK errors from KMS discovery keyrings to the user? Or should it be silent about those errors like in the C implementation?