fix: Better error messageing #212
Conversation
resolves aws#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.
| * the material does not have an unencrypted data key. | ||
| * So I return a concatenated Error message | ||
| */ | ||
| needs(material.hasValidKey() || (!material.hasValidKey() && !cmkErrors.length) |
There was a problem hiding this comment.
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:
- if no key was found by the end of the function, these errors:
- errors from KMS Decrypt if in "non-discovery" mode
- a parsed invalid EDK ARN error (https://github.com/aws/aws-encryption-sdk-c/blob/master/aws-encryption-sdk-cpp/source/kms_keyring.cpp#L93)
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?
| throw new Error('failed to decrypt') | ||
| } | ||
| expect(EncryptionContext).to.deep.equal(context) | ||
| expect(GrantTokens).to.equal(grantTokens) |
There was a problem hiding this comment.
I think we are missing a test for the case when !material.hasValidKey() && !cmkErrors.length
| @@ -203,6 +206,20 @@ export function KmsKeyringClass<S extends SupportedAlgorithmSuites, Client exten | |||
| return material | |||
| } | |||
|
|
|||
| /* Postcondition: A CMK must provide a valid data key. | |||
There was a problem hiding this comment.
Is this the right wording if it is valid for no CMKs to produce a valid data key as long as there are no errors?
| } | ||
| } | ||
|
|
||
| /* Postcondition: A child keyring must provide a valid data key. |
| @@ -358,6 +358,23 @@ describe('MultiKeyring: onDecrypt', () => { | |||
| expect(unwrapDataKey(test.getUnencryptedDataKey())).to.deep.equal(unencryptedDataKey) | |||
| expect(called).to.equal(true) | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
Same here, I think we want to test the case where !material.hasValidKey() && !childKeyringErrors.length
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 aws#212
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 aws#212
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: