feat(Keystore): Introduce additional KMSConfiguration options #270
Conversation
The Hierarchical Keyring's Keystore now supports four (4) `KMSConfigurations`: - kmsKeyArn - kmsMRKeyArn - discovery - MRDiscovery
| A "static Keystore" is a (Branch) Keystore with | ||
| a KMS Configuration of KMS Key ARN. | ||
|
|
||
| A "strict Keystore" is a (Branch) Keystore with | ||
| a KMS Configuration of KMS Multi-Region Key ARN or KMS Key ARN. |
There was a problem hiding this comment.
Why do we have both? What is the difference?
There was a problem hiding this comment.
- The difference is clearly described. Please, compare line 43 to line 46.
- I want a short hand for "a Keystore with a KMS Configuration of KMS Key ARN". That is 10 words! "Static Keystore" is 2!
- I need to describe the "change". We started with only the "static Keystore". Today, we have "strict Keystore", tomorrow, we will have "Discovery Keystore".
There was a problem hiding this comment.
Andy's recent change, 82911ea, is an example of a "static" behavior.
While VersionKey and CreateKey are supported with Strict KMS Configurations,
the KMS-ARN is always static,
even if a MKR Configuration is used when invoking VerisonKey.
I believe it is valuable to introduce this language.
I grant you, there is functionally no difference between a "static Keystore" and a "strict Keystore",
but we did NOT start there.
At one time, we were going to only allow VersionKey & CreateKey from "static Keystores".
Unless anyone strongly objects,
I am inclined to leave this here.
| (and therefore, backing persistance medium, which is currently always a DynamoDB table) | ||
| and Branch Key ID. | ||
|
|
||
| Usage may not need the KMS ARN. |
There was a problem hiding this comment.
What does this mean? Can you expand on why this is here?
There was a problem hiding this comment.
Can you define "this"?
The Sentence:
Usage may not need the KMS ARN.
Completes the "Put another way..." sentence above.
Do you believe that Branch Key Usage requires giving the KMS ARN?
If so, we should cancel this feature.
| Rather than correct the exception, | ||
| Keystore construction MUST fail if the KMS Configuration | ||
| is "strict" | ||
| (KMS Key ARN or KMS MRKey ARN) | ||
| and the provided ARN is an Alias. |
There was a problem hiding this comment.
Why not make the Alias work? We should add some words about why we chose the error path
There was a problem hiding this comment.
What wording is missing from lines 102 to 107 that you would like to add?
| The changes proposed (and comitted to) here | ||
| do not include such a Cache refactoring, | ||
| as that would not address the stated goal | ||
| of allowing one Keystore to work across multiple |
There was a problem hiding this comment.
Why do customers want to use one keystore?
There was a problem hiding this comment.
I believe this was explained in lines 73 to 98.
Do you disagree?
What needs to be added in the Motivation section?
framework/branch-key-store.md
Outdated
| - `KeyId` MUST be [compatible with](#aws-key-arn-compatibility) the configured `AWS KMS Key ARN` in the [AWS KMS Configuration](#aws-kms-configuration) for this keystore. | ||
| - `KeyId`, if the KMS Configuration is Discovery, MUST be the `kms-arn` attribute value of the AWS DDB response item. | ||
| If the KMS Configuration is MRDiscovery, `KeyId` MUST be the `kms-arn` attribute value of the AWS DDB response item, with the region replaced by the configured region. | ||
| Otherwise, it MUST BE the Keystore's `KMS ARN`. |
There was a problem hiding this comment.
This is a mouthful. Can we simplify this?
There was a problem hiding this comment.
I am open to suggestions, but, at this time, looking to ship software, and do not have the capacity to refactor this.
Co-authored-by: seebees <ryanemer@amazon.com>
| and Branch Key ID. | ||
|
|
||
| Usage may not need the KMS ARN. |
There was a problem hiding this comment.
| and Branch Key ID. | |
| Usage may not need the KMS ARN. | |
| and Branch Key ID; | |
| Usage may not need the KMS ARN. |
|
@seebees We launched this feature, and this spec was effectively merged. There are lots of fish to fry. If we do not have the capacity to fully address these spec issues by then, |
Issue #, if available:
Description of changes:
The Hierarchical Keyring's Keystore now supports four (4)
KMSConfigurations:Also see the related PR:
aws/aws-cryptographic-material-providers-library#316
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Check any applicable: