Skip to content
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

Better error messaging when no keyring is found. #152

Closed
seebees opened this issue Jul 17, 2019 · 11 comments · Fixed by #212
Closed

Better error messaging when no keyring is found. #152

seebees opened this issue Jul 17, 2019 · 11 comments · Fixed by #212

Comments

@seebees
Copy link
Contributor

seebees commented Jul 17, 2019

Currently if no configured keyring is found to unwrap any of the encrypted data keys,
the CMM will throw an unencryptedDataKey has not been set error.
While this error is accurate, it is unhelpful.

seebees pushed a commit to seebees/aws-encryption-sdk-javascript that referenced this issue Sep 16, 2019
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.
seebees added a commit that referenced this issue Sep 19, 2019
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.
@davidyell
Copy link

I was Googling for this error and ended up here. The message still doesn't tell me how to fix the error or provide a link for me to learn more.

@seebees
Copy link
Contributor Author

seebees commented Apr 15, 2020

Thanks, that’s a good call out.
The most common case in which we saw this error was where the principle that is trying to decrypt does not have access to decrypt on any of the configured CMKs. The fix ( #212 ) is to bubble up errors directly from KMS to provide the caller with more context.

If you are getting this “Unable to decrypt data key and one or more KMS CMKs had an error.” message in the latest version when trying to decrypt, let's reopen this and figure out what is going on.

If you are not using KMS or are trying to build your own keyring can we open a new issue? Please include a description of what you’re calling and the error you’re seeing.

@davidyell
Copy link

Unfortunately I don't have the code to hand, but as I recall I was using the Node.js AWS SDK. Following along with the decrypt example given here.

Although I am a beginner, so might have just been a code error on my part.

@seebees
Copy link
Contributor Author

seebees commented Apr 21, 2020

@davidyell I opened #305 as a tracker for this generally.
If you have the code handy and can reproduce the specific case you are talking about please put it there.

@RalphBragg
Copy link

Likewise an example end to end with associated necessary KMS policies would be really handy. I've tried following the example and i'm stuck at the same location as @davidyell.

@bardock
Copy link

bardock commented Apr 24, 2020

I was using the same example that followed @davidyell.
To avoid this error you must use the key ARN, not the alias, as was noted here:
https://docs.aws.amazon.com/encryption-sdk/latest/developer-guide/js-examples.html

To specify a CMK for a decryption keyring in the AWS Encryption SDK for JavaScript, you must use the key ARN. Otherwise, the CMK is not recognized

@RalphBragg
Copy link

RalphBragg commented Apr 24, 2020

Hi @bardock - yeah i'm using the ARN's not the alias and this error is still being thrown.
-- Update: Key Properties was the cause of my issue. +1 for better error messaging would be grand!

@seebees
Copy link
Contributor Author

seebees commented Apr 24, 2020

@RalphBragg What specific key properties was the issue?

@bardock so you were getting this error when using something like alias/myAlias?

I also see that the raw keyrings can leak this error as well
https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/raw-keyring/src/raw_keyring_decorators.ts#L61

@bardock
Copy link

bardock commented Apr 24, 2020

@RalphBragg What specific key properties was the issue?

@bardock so you were getting this error when using something like alias/myAlias?

I also see that the raw keyrings can leak this error as well
https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/raw-keyring/src/raw_keyring_decorators.ts#L61

Yes. This example uses an alias for the generator key, but it works just because it also adds the same CMK as an alternate key.

@seebees seebees reopened this Apr 24, 2020
@seebees
Copy link
Contributor Author

seebees commented Apr 24, 2020

I think I understand the issue more now.
The check here https://github.com/aws/aws-encryption-sdk-javascript/blob/master/modules/material-management-node/src/node_cryptographic_materials_manager.ts#L108

Getting an unencrypted data key, when one is not set is an error,
so this check throws an error before 'Unencrypted data key is invalid.'
could even be throw.

This also is sub-optimal.
I'm going to start a PR with

No keyring generated an unencrypted data key.
You may not have access to any wrapping keys.

seebees added a commit to seebees/aws-encryption-sdk-javascript that referenced this issue Aug 7, 2020
resolves: aws#152, aws#31
linked: awslabs/aws-encryption-sdk-specification#105

If no keyrings attempt to decrypt any encrypted data keys,
then the message can not be decrypted.
The code attempted to enforce this,
by retrieving the unencrypted data key in node.

There were two issues here

1. The check ensure the validity of the materials,
itself threw an error.
1. Had this check succeeded, the error message
`'Unencrypted data key is invalid.’` is not incredibly more helpful than
'unencryptedDataKey has not been set'

The error message has been updated,
and the tests have been updated
to verify _this_ error message.

On a related note
awslabs/aws-encryption-sdk-specification#97
starts to explore some additional possibilities.
The fullness of this issue is not only in failure,
but success can also have similar issues.
@seebees seebees closed this as completed Dec 9, 2021
@MrsBookik
Copy link

I have seen this error when only one GeneratorKeyId has been used and not additional keys, that I needed for unknown reasons in order to make decrypt work:

Given official example code there are two different key concepts given that I am confused about:

const keyring = new KmsKeyringNode({ generatorKeyId, keyIds });

const { encrypt, decrypt } = buildClient(CommitmentPolicy.REQUIRE_ENCRYPT_REQUIRE_DECRYPT);
const { result } = await encrypt(keyring, cleartext, { encryptionContext: context });
const { plaintext, messageHeader } = await decrypt(keyring, result);

In order to make encrypt and even decrypt work using KMS, I had to provide a generatorKeyId and second key that I have put into keyIds.
Without the second Key, no decryption will happen and it will raise exception stating

unencryptedDataKey has not been set

Can someone tell me what is the purpose of each on both keys and why do we need both together?

(I was already asking at #951, maybe we can shed some light into this quite undocumented concept)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants