Skip to content

Remove lines mandating region be specified by KMS keyring generator #50

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Feb 18, 2020
Merged

Remove lines mandating region be specified by KMS keyring generator #50

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
db5a6a5
Remove lines mandating a region be specified by the generator
MatthewBennington Oct 29, 2019
cc6c879
Changing previously missed paragraph, and fixing typos
MatthewBennington Oct 30, 2019
8d6cec2
Clarifications and fixes to KMS Keyring
MatthewBennington Nov 4, 2019
8977030
Pull request iteration
MatthewBennington Nov 20, 2019
a2455bd
Remove unreachable Decrypt behavior
MatthewBennington Nov 21, 2019
0c5c19a
Remove unreachable lines
MatthewBennington Nov 21, 2019
3dc5751
Update framework/kms-keyring.md
MatthewBennington Feb 17, 2020
7fffe53
"Input CMK -> "Response KeyID"
MatthewBennington Feb 17, 2020
e0748d5
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
128c87b
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
c4da98a
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
6cd77d5
"contains" ->
MatthewBennington Feb 18, 2020
fe8e61d
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
2143ee4
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
ecdabbf
Update framework/kms-keyring.md
MatthewBennington Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions framework/kms-keyring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ If the input [encryption materials](#structures.md#encryption-materials) do not
and this keyring has a [generator](#generator) defined, and onEncrypt MUST attempt to generate a new plaintext data key
and encrypt that data key by calling [KMS GenerateDataKey](#kms-generatedatakey).

The [KMS client](#kms-client) that calls [KMS GenerateDataKey](#kms-generatedatakey) MUST be the
client returned by the [client supplier](#client-supplier).
The client MUST be a client that calls the AWS region specified in the [generator](#generator) ARN.
If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also true of the KMS Encrypt call below.

[KMS GenerateDataKey](#kms-generatedatakey) MUST be the client returned by the [client supplier](#client-supplier)
when given that region as input.
Comment on lines +164 to +166
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it is drawing an odd relationship that I don't think exists. The way this is worded make it sound to me like we are saying that there is something special about the region name extracted from this ARN that makes it different from the same region name extracted from some other ARN. This is not the case (though, side note, this is why I wanted to the client provider to accept a key name rather than a region name, but that's a different conversation).

I think we can simplify this to something along the lines of the following.

Client Provider

  • The client provider MUST require a region name.
  • The client provider MUST either provide a client for the requested region or fail.

Keyring

  • If the key ID is an ARN then the keyring MUST call the client provider with the region in the ARN.
  • If the key ID is not an ARN then the keyring MAY select a region with which to call the client provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is worded make it sound to me like we are saying that there is something special about the region name extracted from this ARN that makes it different from the same region name extracted from some other ARN.

As I understand the spec, there is something special about this region. No other region derived from an ARN determines the region used by the client (or after this PR, the region given to the client supplier).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something special about the generator key. That generator key happens to exist in a single region. The region is a side effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this?

If the generator is formatted as either a well-formed key ARN, or a well-formed alias ARN, then an AWS region MUST be derived from the generator and used to call the client supplier. The client produced as output of the client supplier MUST be used to call KMS Encrypt/Decrypt/GenerateDataKey.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client provider MUST require a region name.

I disagree.
The AWS SDK can pick up a default region in the same way it gets credentials.
The client provider is the interface to the AWS SDK.
Let the SDK handle it.
If there is any additional complication regarding what a default region should be,
putting this logic in a function is much easier for customers.

I have an example of what I'm thinking about here: aws/aws-encryption-sdk-javascript#231

If the [client supplier](#client-supplier) does not provide any client for the given region for this GenerateDataKey call,
OnEncrypt MUST fail.

Expand Down Expand Up @@ -266,9 +266,10 @@ in the input encrypted data key list with the following conditions, until it suc
To attempt to decrypt a particular [encrypted data key](#structures.md#encrypted-data-key),
OnDecrypt MUST call [KMS Decrypt](#kms-decrypt).

The [KMS client](#kms-client) that calls [KMS Decrypt](#kms-decrypt) MUST be the
client returned by the [client supplier](#client-supplier).
The client MUST be a client that calls the AWS region specified in the [generator](#generator) ARN.

If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand. For decrypt the CMK must be the ARN.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CMK used for decryption must be an ARN, but that CMK is not necessarily the generator. I don't understand how this comment relates to this line, can you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For decryption, only keys with a well formed key ARN will produce a successful decryption. This is because the keyring will only attempt to call KMS Decrypt for EDKs where the keyProvider matches a keyID. Because the EDK is serialized to always use the key ARN as the keyProvider, the only way to get the KMS Keyring to call KMS Decrypt for an EDK is if you include the key ARN in the keyIDs/generator.

Thus, if we ever actually get to attempting to call KMS, we know that the key in question has a region. HOWEVER, the generator could still just be an alias... I think it still makes sense to specify the "everything is a well formed ARN" case, and leave specifying other behavior as part of #49

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the EDK is serialized to always use the key ARN as the keyProvider, the only way to get the KMS Keyring to call KMS Decrypt for an EDK is if you include the key ARN in the keyIDs/generator.

If I understand correctly, then if a given EDK's KeyProvider matches one of the keyring's KeyIDs, then it will attempt to decrypt. In this scenario the keyring would need to create a client by inputting a region into the client supplier. To determine that region it would need to parse the generator string, which may be an alias.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it still makes sense to specify the "everything is a well formed ARN" case, and leave specifying other behavior as part of #49

I support this endeavor.

[KMS Decrypt](#kms-decrypt) MUST be the client returned by the [client supplier](#client-supplier)
when given that region as input.
If the [client supplier](#client-supplier) does not provide any client for the given region for the Decrypt call,
OnDecrypt MUST skip that particular [encrypted data key](#encrypted-data-key).

Expand Down