-
Notifications
You must be signed in to change notification settings - Fork 26
feat(branch-keys): add hv-2 spec #297
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, need to do a proper review when I get home.
framework/branch-key-store.md
Outdated
If the `hierarchy-version` is v1, AWS KMS encryption context MUST be same as [branch key context](#branch-key-context). | ||
If the `hierarchy-version` is v2, AWS KMS encryption context MUST be the [encryption context](../structures.md#encryption-context) send by users without any transformation. | ||
|
||
AWS KMS encryption context MUST be always the same encryption context send by user regardless of any variation of encryption context (i.e.: ACTIVE Encryption Context, DECRYPT_ONLY Encryption Context, Beacon Key Encryption Context and Custom Encryption Context). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line reads funny. I think I understand what we are trying to say.
But I think we need to clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrase it to
"""
If the hierarchy-version
is v2, the encryption context send to AWS KMS MUST always match the encryption context originally sent by the user, without any variations. This requirement applies regardless of the type of encryption context being used, whether it is the ACTIVE encryption context, the DECRYPT_ONLY encryption context, the Beacon Key encryption context, or a custom encryption context.
"""
lmk if you have any suggestion
Co-authored-by: Tony Knapp <[email protected]>
|
||
# 1 Plain-text Commitment instead of KMS Encryption Context | ||
|
||
The KMS RSA Keyring has already solved the need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the ecdh keyrings as well
changes/2025-06-30_branch_keys_version_2/protect_branch_key_without_kms_ec.md
Outdated
Show resolved
Hide resolved
A root key used to then derive different beacon keys per beacon. | ||
- [UUID](https://www.ietf.org/rfc/rfc4122.txt): a universally unique identifier that can be represented as a byte sequence or a string. | ||
|
||
#### kms-arn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another nit:
i think kms-arn and hierachy version are meant to be bullet points under definitions and not their own sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having them as sections so we can link to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, it's just hard to read
These are any members of the Branch Key's Context whose key is not prefixed by `aws-crypto-ec:` OR another reserved Branch Key Context Key Name, such as `kms-arn` or `hierarchy-version`. | ||
Such Key-Value pairs were NOT added to a Branch Key Item by a Crypto Tools product. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are revered keys not added by CryptoTools?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOT (prefixed with aws-crypto-ec:
or another reserved word).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the NOT applies to both sides of the OR.
changes/2025-06-30_branch_keys_version_2/protect_branch_key_without_kms_ec.md
Outdated
Show resolved
Hide resolved
- [KMS ARN](#kms-arn) | ||
- [Create Time](#create-time) | ||
- [Hierarchy Version](#hierarchy-version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking. We may consider this lightly breaking because who else is constructing these. Just bringing it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. I think it is worth it.
I have a ticket right now where this would solve an issue.
Co-authored-by: Tony Knapp <[email protected]>
changes/2025-06-30_branch_keys_version_2/un-modeled_branch_key_context_key_values.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Lucas McDonald <[email protected]>
…_context_key_values.md Co-authored-by: Tony Knapp <[email protected]>
Co-authored-by: Tony Knapp <[email protected]>
…thout_kms_ec.md Co-authored-by: José Corella <[email protected]>
Review |
Issue #, if available:
Description of changes:
Crypto tools is adding a new feature (Hierarchical Keyring V2) in MPL main branch. This PR adds the specification for it.
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: