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

KMS Keyring should fail initialization if any keyId is malformed #55

Closed
WesleyRosenblum opened this issue Nov 20, 2019 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@WesleyRosenblum
Copy link
Contributor

WesleyRosenblum commented Nov 20, 2019

Upon KMS initialization, if any key ID (either the generator or any of the input Key Ids,) is malformed, initialization should fail, so that the user is immediately aware they are handling malformed data.

@mattsb42-aws mattsb42-aws added the enhancement New feature or request label Nov 20, 2019
@WesleyRosenblum WesleyRosenblum changed the title KMS Keyring should fail if any keyId or providerId is malformed KMS Keyring should fail initialization if any keyId is malformed Apr 28, 2020
@WesleyRosenblum
Copy link
Contributor Author

The Java ESDK does this currently by using the AwsKmsCmkId type as input, which only allows well formed ARNs (if an ARN is being used, and not a raw Key ID or Alias)

@lavaleri
Copy link
Contributor

Resolving this issue means clarifying exactly what our verification for key IDs should look like, for each type of key that the ESDK MUST or MAY support.

@mattsb42-aws mattsb42-aws added this to the keyrings milestone May 15, 2020
@acioc acioc self-assigned this May 26, 2020
@acioc
Copy link
Contributor

acioc commented Jun 1, 2020

Upon discussion, we have decided that we MUST accept all key identifiers supported by AWS KMS. We will not perform validation on these, and will pass the strings directly to AWS KMS. The reason for this is because we need to support Key IDs. Key IDs are currently in UUID format, but (based on public documentation) there is no guarantee this will always be the case - public documentation states the only restriction is that a Key ID must be a string between length 1 and 2048 (I believe).

Therefore, it makes sense to treat key identifiers as strings as pass them as-is.

@WesleyRosenblum
Copy link
Contributor Author

I think it's still useful to wrap the string in something to identify it as a AWS KMS CMK Identifier, in case we want to add some validation in the future.

Are we OK with the current validation the Java ESDK does? Basically the input is only validated if it starts with the prefix "arn:". The reason for validation is that the Java ESDK uses the AWS SDK's Arn.fromString method to convert a string to an Arn to extract the Region ID. This method requires a valid Arn. See https://github.com/aws/aws-encryption-sdk-java/blob/keyring/src/main/java/com/amazonaws/encryptionsdk/kms/AwsKmsCmkId.java#L35

@acioc
Copy link
Contributor

acioc commented Jun 1, 2020

We need to additionally support Key Ids and unqualified aliases, so we can't guarantee we exclusively have ARNs.

I created a PR replacing most of our current description of what we support with a link to AWS KMS docs: #123

We should support whatever AWS KMS supports and just pass through the string to AWS KMS, so they do the validation they need. This also means our ESDK is viable long-term (if their format ever changes, we don't need to specifically update the ESDK to support it, since it's just a pass through).

Quick edit: You can totally use Arn.fromString to determine region, for example. But we can't guarantee all strings we get are ARNs. Rather "if it's an ARN, we can extract the region, so we can do this" but if it's not an ARN, just pass it along since any string is valid (between a length of 1 and 2048).

@acioc
Copy link
Contributor

acioc commented Jun 4, 2020

PR merged. Tracking implementation in #129

@acioc acioc closed this as completed Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants