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

proposal: Key Store Admin #282

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Conversation

texastony
Copy link
Contributor

Issue #, if available:

Description of changes:

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:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

changes/2024-TBD_key-store-admin/background.md Outdated Show resolved Hide resolved
changes/2024-TBD_key-store-admin/background.md Outdated Show resolved Hide resolved
changes/2024-TBD_key-store-admin/background.md Outdated Show resolved Hide resolved
changes/2024-TBD_key-store-admin/background.md Outdated Show resolved Hide resolved
framework/branch-key-store-admin.md Show resolved Hide resolved
Comment on lines 143 to 144
The Operation behaves identically to the [Key Store Client's CreateKey](../branch-key-store.md#createkey),
with the following caveats:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to move the operation here, and then point the key store here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not against that, but not willing to do it today.

Co-authored-by: Lucas McDonald <[email protected]>
Co-authored-by: seebees <[email protected]>
@texastony texastony force-pushed the tony/change-key-store-admin branch from e8c9f98 to af1bccf Compare December 17, 2024 16:46
@texastony texastony marked this pull request as ready for review January 17, 2025 23:04
@texastony texastony requested a review from a team as a code owner January 17, 2025 23:04
changes/2025-01-17_key-store-admin/background.md Outdated Show resolved Hide resolved
changes/2025-01-17_key-store-admin/background.md Outdated Show resolved Hide resolved
changes/2025-01-17_key-store-admin/background.md Outdated Show resolved Hide resolved
### AWS KMS ReEncrypt (default)

`AwsKmsReEncrypt` dictates the Key Store Operation use
AWS KMS' ReEncrypt Operation to
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a link to the API docs?

Comment on lines +190 to +192
`AwsKmsDecryptEncrypt` is a structure that holds two `AwsKms`,
one designated for Decrypt,
one designated for Encrypt.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`AwsKmsDecryptEncrypt` is a structure that holds two `AwsKms`,
one designated for Decrypt,
one designated for Encrypt.
`AwsKmsDecryptEncrypt` is a structure that holds two `AwsKms` clients,
one for Decrypt, and one for Encrypt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I like the term designated,
as later we will use the Encrypt client for GenerateDataKey and Decrypt.

Maybe I will change the wording to "Primarily but not exclusively for"

framework/branch-key-store-admin.md Outdated Show resolved Hide resolved

#### [Branch Key and Beacon Key Creation](./branch-key-store.md#branch-key-and-beacon-key-creation)

The `AwsKmsReEncrypt` configuration MUST be treated as if it were the Key Store's `AwsKms`.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can re-word this.

- An optional [Key Management Strategy](#key-management-strategy)

At this time,
the [Key Management Strategy](#key-management-strategy) MUST be `AwsKmsReEncrypt`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the [Key Management Strategy](#key-management-strategy) MUST be `AwsKmsReEncrypt`.
the default [Key Management Strategy](#key-management-strategy) MUST be `AwsKmsReEncrypt`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No... only ReEncrypt is supported for Version and Create.

I do not have the capacity to implement Decrypt/Encrypt for Version and Create;
When we have a customer asking for it,
then we should take it on.

And default is tricky;
Smithy-Dafny does not support the Default trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not blocking, I would like to leave this as is.

framework/branch-key-store-admin.md Outdated Show resolved Hide resolved
Comment on lines +182 to +185
<!-- LocalWords: MRK AwsKms grantTokenList kmsClient ReEncrypt -->
<!-- LocalWords: AwsKmsReEncrypt keystore AwsKmsDecryptEncrypt -->
<!-- LocalWords: Admin ReEncrypt Changelog aws arn createkey -->
<!-- LocalWords: AwsCryptographyKeyStoreOperations versionkey GenerateDataKeyWithoutPlaintext -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<!-- LocalWords: MRK AwsKms grantTokenList kmsClient ReEncrypt -->
<!-- LocalWords: AwsKmsReEncrypt keystore AwsKmsDecryptEncrypt -->
<!-- LocalWords: Admin ReEncrypt Changelog aws arn createkey -->
<!-- LocalWords: AwsCryptographyKeyStoreOperations versionkey GenerateDataKeyWithoutPlaintext -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not blocking, I would like to keep these here;
I will need to write the Mutation Spec,
and these spelling hints will help me.

Copy link
Contributor Author

@texastony texastony left a comment

Choose a reason for hiding this comment

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

I am not sure what is blocking vs what are nits,
but I have responded to some of @josecorella comments.

Comment on lines +182 to +185
<!-- LocalWords: MRK AwsKms grantTokenList kmsClient ReEncrypt -->
<!-- LocalWords: AwsKmsReEncrypt keystore AwsKmsDecryptEncrypt -->
<!-- LocalWords: Admin ReEncrypt Changelog aws arn createkey -->
<!-- LocalWords: AwsCryptographyKeyStoreOperations versionkey GenerateDataKeyWithoutPlaintext -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not blocking, I would like to keep these here;
I will need to write the Mutation Spec,
and these spelling hints will help me.

- An optional [Key Management Strategy](#key-management-strategy)

At this time,
the [Key Management Strategy](#key-management-strategy) MUST be `AwsKmsReEncrypt`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is not blocking, I would like to leave this as is.


#### [Branch Key and Beacon Key Creation](./branch-key-store.md#branch-key-and-beacon-key-creation)

The `AwsKmsReEncrypt` configuration MUST be treated as if it were the Key Store's `AwsKms`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can re-word this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants