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

MatthewBennington · 2019-10-29T19:19:56Z

Issue #, if available: #49

Description of changes: Changes to OnEncrypt/OnDecrypt to relax the dependence on generator ARN for AWS region.

What this PR does NOT cover: This PR does NOT redefine the Client Supplier to take a CMK ARN as input.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

seebees · 2019-10-29T19:54:05Z

framework/kms-keyring.md

-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


This is also true of the KMS Encrypt call below.

seebees · 2019-10-29T19:55:00Z

framework/kms-keyring.md

-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


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

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?

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

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.

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.

mattsb42-aws · 2019-10-29T20:54:17Z

framework/kms-keyring.md

-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


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.

mattsb42-aws · 2019-10-29T21:08:10Z

framework/kms-keyring.md

+If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
+[KMS GenerateDataKey](#kms-generatedatakey) MUST be the client returned by the [client supplier](#client-supplier)
+when given that region as input.


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.

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).

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

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.

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

framework/kms-keyring.md

* Add language to specify behavior when a region cannot be extracted from a CMK ARN * Better define Client Supplier * Fix Keyring Trace `KeyName` field specifications * Misc spelling and grammar

framework/kms-keyring.md

lavaleri · 2019-11-04T20:04:27Z

framework/kms-keyring.md

  - The [flags](#structures.md$flags) field of this record includes exactly the following flags:
    - [GENERATED DATA KEY](#structures.md#supported-flags)
    - [ENCRYPTED DATA KEY](#structures.md#supported-flags)
    - [SIGNED ENCRYPTION CONTEXT](#structures.md#supported-flags)

-Given a plaintext data key in the [encryption materials](#structures.md#encryption-materials),


Without this qualifier is it clear what plaintext data key is referring to on the next line? Maybe there is a way to restate this which is less confusing, or do you think it's not necessary?

Good point about clarifying plaintext data key. I removed this line because I think it creates the appearance of an else to the if on line 154. I will try to find wording that addresses this.

lavaleri · 2019-11-04T20:11:46Z

framework/kms-keyring.md

@@ -64,7 +64,7 @@ On keyring initialization, a keyring MAY define the following:

 ### Client Supplier

-A function that returns a KMS client that can make the following API calls in a particular AWS region:
+A function that maps a region string to a KMS client that can make the following API calls in the given AWS region:


I think we want to be clear what the inputs/outputs are. The input is a string, the output is a KMS client.

Can you help me understand in what way is this sentence unclear about that?

The latest wording addresses my concern. I was just pointing out that using the verb maps isn't as clear as explicitly stating what the input is and what the output is.

lavaleri · 2019-11-04T20:16:01Z

framework/kms-keyring.md

+If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
+[KMS GenerateDataKey](#kms-generatedatakey) MUST be the client returned by the [client supplier](#client-supplier)
+when given that region as input. If an AWS region cannot be extraced from the [generator](#generator),
+then the KMS Keyring MAY choose an arbitrary region.


We might want to clarify what we mean by May choose an arbitrary region. What we really mean is that if you can't get the region from the key, then the KMS keyring is allowed to use a KMS client from any arbitrary region to make the call to KMS. Maybe just say "If the AWS region cannot be extracted...the KMS Keyring MAY use a KMS client from any region"

This is where I hit a bit of a sticking point: Who is the actor here?

Region discernible from key ID

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring passes the region name to the client supplier.

The client supplier provides the KMS Keyring with a client.

The KMS Keyring uses that client.

Region not discernible from key ID

Keyring is the actor

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring fails to determine region from key ID.

The KMS Keyring through some undefined mechanism obtains a client.

Client Supplier is the actor

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring fails to determine region from key ID.

The KMS Keyring tells the Client Supplier that it doesn't know what the region is.

The Client Supplier MAY provide the KMS Keyring with a client.

The KMS Keyring uses that client or fails if the Client Supplier fails to provide one.

Pedantically, the Client Supplier always may not provide a client, even if a region is obtained.
This is important because if the Client Supplier is an API to infer something about regions,
and the Keyring obtains a client by bypassing the client supplier our API contract is violated.

To add to this confusion, the AWS-SDK itself has a "defined" mechanism for determining a default region.
So I would like to add

AWS SDK Client is the actor

I offer this case because given this, the customer configuration store for regions is already defined by the AWS_REGION environment variable.
As a customer using both the AWS-SDK and the AWS Encryption SDK I expect them to agree on this default region.
I assert that this at least defines the mechanism for determining the region :)

So working backward from the customer interface say I have an alias in every region alias/my-key.
I have a Lambda running in every region that will use the AWS Encryption SDK with a KMS Keyring configured like so

const keyring = new KmsKeyringNode({ generatorKeyId: 'alias/my-key' clientProvider: excludeRegions(['us-west-1']), })

What should happen in us-west-1?

There are two points being brought up here:

We should specify that the client supplier is responsible for determining default/"I can't get a region" behavior instead of the keyring. As said above, this makes sense because otherwise we violate the API contract.

In the view of a client supplier, it can be a bit tricky what the default behavior SHOULD be. Generally the supplier should make use of the AWS SDK defualt region, however the "limit regions" supplier example above shows how this might not always be the desired behavior. I think more precisely defining the restrictions on client supplier behavior is out of scope for this PR.

As such, I think these are the next steps for this PR:

Update client supplier interface to say that it MUST support some input (i.e. empty string) to indicate that the region in unknown

Update L166 to say that if the region can't be determined, then the Keyring MUST input the "I don't know" input to the client supplier, and if the client supplier returns a client the Keyring MUST use that client.

@mattsb42-aws @seebees Thoughts?

Good to me.

Sounds good!

As I read the spec, the Client Supplier is an interface which we let the customer implement. As such, I don't think it makes sense to define default behavior.

How about:

Client Supplier

A function which MUST take as input either a region string (e.g. us-east-1), or some value denoting an unknown region, and MAY return a KMS Client which can make the following API calls in the given AWS region:

and

If an AWS region cannot be extracted from the generator then the KMS Keyring MUST input a value denoting an unknown region. If the client supplier does not provide any client for the given region for this Encrypt call, OnEncrypt MUST skip that particular CMK.

I've opted not to define the value indicating an unknown region, as that seems fairly language specific.

s/which/that/ but otherwise LGTM @MatthewBennington

new wording LGTM

lavaleri · 2019-11-04T20:17:45Z

framework/kms-keyring.md

-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 [encrypted data key](structures.md#encrypted-data-key)'s


An AWS region MUST be extractable from the EDK. This is because the EDK key provider info serializes the keyARN, and never the alias. If the KMS Keyring cannot extract the region in this case, it should result in an error.

mattsb42-aws · 2019-11-04T22:58:45Z

framework/kms-keyring.md

+If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
+[KMS GenerateDataKey](#kms-generatedatakey) MUST be the client returned by the [client supplier](#client-supplier)
+when given that region as input. If an AWS region cannot be extraced from the [generator](#generator),
+then the KMS Keyring MAY choose an arbitrary region.


This is where I hit a bit of a sticking point: Who is the actor here?

Region discernible from key ID

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring passes the region name to the client supplier.

The client supplier provides the KMS Keyring with a client.

The KMS Keyring uses that client.

Region not discernible from key ID

Keyring is the actor

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring fails to determine region from key ID.

The KMS Keyring through some undefined mechanism obtains a client.

Client Supplier is the actor

The KMS Keyring through some undefined mechanism attempts to determine region from key ID.

The KMS Keyring fails to determine region from key ID.

The KMS Keyring tells the Client Supplier that it doesn't know what the region is.

The Client Supplier MAY provide the KMS Keyring with a client.

The KMS Keyring uses that client or fails if the Client Supplier fails to provide one.

Changed Client Supplier specification to accept some value denoting an unknown region.

lavaleri · 2019-11-21T19:27:13Z

framework/kms-keyring.md

+If an AWS region cannot be extracted from the [encrypted data key](structures.md#encrypted-data-key)'s [key provider info](structures.md#key-provider-information) then the KMS Keyring MUST input to the [client supplier](#client-supplier) a value denoting an unknown region.
+If the client supplier does not provide any client for the given region for this Decrypt call, OnDecrypt MUST skip that particular [encrypted data key](#encrypted-data-key).


We shouldn't specify this for OnDecrypt, because the previously specified behavior (only attempt to decrypt if key is in ARN format) will always give us a key that MUST have a region. Instead, the spec at this point should state that you MUST get the region from the key. If for whatever reason the implementation is unable to get a region from the key at this point, something is wrong, and the implementation should fail due to being unable to follow the specification.

lavaleri · 2019-11-21T19:27:38Z

framework/kms-keyring.md

+If an AWS region can be extracted from the [generator](#generator), then the [KMS client](#kms-client) that calls
+[KMS GenerateDataKey](#kms-generatedatakey) MUST be the client returned by the [client supplier](#client-supplier)
+when given that region as input. If an AWS region cannot be extraced from the [generator](#generator),
+then the KMS Keyring MAY choose an arbitrary region.


new wording LGTM

lavaleri

One issues that should be addressed. Once that is fixed this LGTM.

lavaleri · 2019-11-21T19:47:00Z

framework/kms-keyring.md

+For each [KMS Decrypt](#kms-decrypt) call, if an AWS region can be extracted from the [encrypted data key](structures.md#encrypted-data-key)'s [key provider info](structures.md#key-provider-information).
+The KMS Keyring MUST call [KMS Decrypt](#kms-decrypt) using the client supplied by the [client supplier](#client-supplier), given the region as input.


Whoops, this wording needs to also be updated to remove the "if an AWS region can be extracted".

framework/kms-keyring.md

seebees · 2019-12-11T22:07:58Z

framework/kms-keyring.md

@@ -235,7 +237,7 @@ If the call succeeds, OnEncrypt MUST do the following with the response from [KM
 - append a new [record](#structures.md#record) to the [keyring trace](#structures.md#keyring-trace)
  in the input [encryption materials](#structures.md#encryption-materials), constructed as follows:
  - The string field KeyNamespace contains "aws-kms".
-  - The string field KeyName contains [generator](#generator).
+  - The string field KeyName contains the CMK ARN used for [KMS Encrypt](#kms-encrypt).


Both the C and JS ESDKs do not do this. They both use the value returned from KMS.

seebees · 2019-12-11T22:11:42Z

I also note that there is a conflict between the things we take as CMKs and what we send to KMS.
We state that we support alias, ARN, and alias ARN. But the clientSupplier will be passed a form of unknown if a region can not be extracted.
While I feel strongly that configuring the KMS Keyring with a raw key id is a bad operational idea, it would work.

Co-Authored-By: seebees <[email protected]>

seebees

LGTM, need to coordinate documentation updates.

framework/kms-keyring.md

Capitalize "NOT" Co-Authored-By: Matt Bullock <[email protected]>

Co-Authored-By: Matt Bullock <[email protected]>

Remove lines mandating a region be specified by the generator

db5a6a5

MatthewBennington requested a review from lavaleri October 29, 2019 19:19

seebees requested changes Oct 29, 2019

View reviewed changes

MatthewBennington requested a review from mattsb42-aws October 29, 2019 20:26

mattsb42-aws requested changes Oct 29, 2019

View reviewed changes

Changing previously missed paragraph, and fixing typos

cc6c879

mattsb42-aws mentioned this pull request Oct 31, 2019

feat(kms-keyring): KMS CMK alias is a valid identifier aws/aws-encryption-sdk-javascript#231

Merged

1 task

seebees reviewed Oct 31, 2019

View reviewed changes

framework/kms-keyring.md Show resolved Hide resolved

Clarifications and fixes to KMS Keyring

8d6cec2

* Add language to specify behavior when a region cannot be extracted from a CMK ARN * Better define Client Supplier * Fix Keyring Trace `KeyName` field specifications * Misc spelling and grammar

lavaleri reviewed Nov 4, 2019

View reviewed changes

mattsb42-aws requested changes Nov 4, 2019

View reviewed changes

Pull request iteration

8977030

Changed Client Supplier specification to accept some value denoting an unknown region.

MatthewBennington requested review from seebees and mattsb42-aws November 20, 2019 19:06

lavaleri reviewed Nov 21, 2019

View reviewed changes

lavaleri suggested changes Nov 21, 2019

View reviewed changes

Remove unreachable Decrypt behavior

a2455bd

MatthewBennington requested a review from lavaleri November 21, 2019 19:40

lavaleri reviewed Nov 21, 2019

View reviewed changes

Remove unreachable lines

0c5c19a

lavaleri approved these changes Nov 21, 2019

View reviewed changes

MatthewBennington mentioned this pull request Nov 25, 2019

KMS Keyring aws/aws-encryption-sdk#86

Merged

lavaleri mentioned this pull request Nov 25, 2019

Defining the KMS Keyring aws/aws-encryption-sdk-java#147

Merged

1 task

seebees requested changes Dec 11, 2019

View reviewed changes

seebees added the doc change required label Feb 11, 2020

MatthewBennington and others added 2 commits February 17, 2020 10:25

Update framework/kms-keyring.md

3dc5751

Co-Authored-By: seebees <[email protected]>

"Input CMK -> "Response KeyID"

7fffe53

seebees approved these changes Feb 17, 2020

View reviewed changes

mattsb42-aws requested changes Feb 18, 2020

View reviewed changes

MatthewBennington and others added 7 commits February 18, 2020 08:50

Update framework/kms-keyring.md

e0748d5

Capitalize "NOT" Co-Authored-By: Matt Bullock <[email protected]>

Update framework/kms-keyring.md

128c87b

Co-Authored-By: Matt Bullock <[email protected]>

Update framework/kms-keyring.md

c4da98a

Co-Authored-By: Matt Bullock <[email protected]>

"contains" ->

6cd77d5

Update framework/kms-keyring.md

fe8e61d

Co-Authored-By: Matt Bullock <[email protected]>

Update framework/kms-keyring.md

2143ee4

Co-Authored-By: Matt Bullock <[email protected]>

Update framework/kms-keyring.md

ecdabbf

Co-Authored-By: Matt Bullock <[email protected]>

MatthewBennington requested a review from mattsb42-aws February 18, 2020 19:57

mattsb42-aws approved these changes Feb 18, 2020

View reviewed changes

MatthewBennington merged commit cba8cbf into awslabs:master Feb 18, 2020

WesleyRosenblum mentioned this pull request Apr 28, 2020

OnEncrypt and OnDecrypt in KMS Keyring contains wrong key id for the keyring trace #60

Closed

		If an AWS region cannot be extracted from the [encrypted data key](structures.md#encrypted-data-key)'s [key provider info](structures.md#key-provider-information) then the KMS Keyring MUST input to the [client supplier](#client-supplier) a value denoting an unknown region.
		If the client supplier does not provide any client for the given region for this Decrypt call, OnDecrypt MUST skip that particular [encrypted data key](#encrypted-data-key).

		For each [KMS Decrypt](#kms-decrypt) call, if an AWS region can be extracted from the [encrypted data key](structures.md#encrypted-data-key)'s [key provider info](structures.md#key-provider-information).
		The KMS Keyring MUST call [KMS Decrypt](#kms-decrypt) using the client supplied by the [client supplier](#client-supplier), given the region as input.

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

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

Uh oh!

Conversation

MatthewBennington commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lavaleri Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Region discernible from key ID

Region not discernible from key ID

Keyring is the actor

Client Supplier is the actor

Uh oh!

Choose a reason for hiding this comment

AWS SDK Client is the actor

Uh oh!

lavaleri Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Client Supplier

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Region discernible from key ID

Region not discernible from key ID

Keyring is the actor

Client Supplier is the actor

MatthewBennington commented Oct 29, 2019 •

edited

Loading

lavaleri Nov 21, 2019 •

edited

Loading

lavaleri Nov 11, 2019 •

edited

Loading

lavaleri Nov 21, 2019 •

edited

Loading