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.

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

Jump to bottom

feat: Adding a storage option to the KeyStore #594

Merged
merged 1 commit into from
Sep 18, 2024
Merged

feat: Adding a storage option to the KeyStore #594

merged 1 commit into from
Sep 18, 2024

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Aug 13, 2024

The key store now allows for both a default DynamoDB table, or any custom storage system.

The important aspect about the key store
is the fact that branch keys can be versioned easily, and are cryptographically safe to use.
The actual storage medium is not important.

See: https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/changes/2024-6-17_key-store-persistance/background.md#background

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@seebees seebees requested a review from a team as a code owner August 13, 2024 23:08
@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

Detected changes to the release files or to the check-files action

@github-actions GitHub Actions
Copy link

Changes to the release files or the check-files action requires 2 approvals from CODEOWNERS

@seebees seebees force-pushed the seebees/add-storage-interface branch from ad43b7e to ca83676 Compare August 13, 2024 23:45
@github-actions GitHub Actions
Copy link

Detected changes to the release files or to the check-files action

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

Changes to the release files or the check-files action requires 2 approvals from CODEOWNERS

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

ajewellamz
ajewellamz previously approved these changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@ajewellamz ajewellamz left a comment

Choose a reason for hiding this comment

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

LGTM

@texastony texastony mentioned this pull request Aug 26, 2024
Open
@github-actions GitHub Actions
Copy link

@texastony, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 4, 2024

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@texastony texastony mentioned this pull request Sep 11, 2024
Draft
1 task
@seebees seebees force-pushed the seebees/add-storage-interface branch from 7fa97d5 to 1c0f43a Compare September 11, 2024 20:49
@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

josecorella
josecorella reviewed Sep 12, 2024
View reviewed changes
Copy link
Contributor

@josecorella josecorella left a comment

Choose a reason for hiding this comment

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

looking good!

...hicMaterialProviders/dafny/AwsCryptographyKeyStore/src/AwsCryptographyKeyStoreOperations.dfy Show resolved Hide resolved
...hicMaterialProviders/dafny/AwsCryptographyKeyStore/src/AwsCryptographyKeyStoreOperations.dfy
Comment on lines +338 to +344
// config.ddbTableName,
config.logicalKeyStoreName,
config.kmsConfiguration,
config.grantTokens,
config.kmsClient,
config.ddbClient
config.storage
// config.ddbClient
Copy link
Contributor

Choose a reason for hiding this comment

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

intentional comments?

AwsCryptographicMaterialProviders/dafny/AwsCryptographyKeyStore/src/CreateKeys.dfy

&& 3 == |writeNewKey.TransactItems|
&& Seq.Last(storage.History.WriteNewEncryptedBranchKey).input.Active
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I liked how above you separated the Seq.Last(..) on line 112 into its own variable that made it easy to follow along. Down below it gets a little difficult to tell what the Seq.Last is referring to.

AwsCryptographicMaterialProviders/dafny/AwsCryptographyKeyStore/src/CreateKeys.dfy Show resolved Hide resolved
AwsCryptographicMaterialProviders/dafny/AwsCryptographyKeyStore/src/Structure.dfy Show resolved Hide resolved
@seebees seebees mentioned this pull request Sep 17, 2024
Closed
@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@seebees
feat: Adding a storage option to the KeyStore
53c9c25 
The key store now allows for both a default DynamoDB table,
or any custom storage system.

The important aspect about the key store
is the fact that branch keys can be versioned easily,
and are cryptographically safe to use.
The actual storage medium is not important.

See: https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/changes/2024-6-17_key-store-persistance/background.md#background
@seebees seebees force-pushed the seebees/add-storage-interface branch from 56e5262 to 53c9c25 Compare September 17, 2024 19:29
@github-actions GitHub Actions
Copy link

@seebees, I noticed you are updating the smithy model files.
Does this update need new or updated javadoc trait documentation?
Are you adding constraints inside list, map or union? Do you know about this issue: smithy-lang/smithy-dafny#491?

@seebees seebees changed the base branch from main to rc-1.7.0 September 17, 2024 19:29
@seebees seebees merged commit 0f79e74 into rc-1.7.0 Sep 18, 2024
71 checks passed
@texastony texastony mentioned this pull request Sep 28, 2024
Draft
texastony
texastony reviewed Oct 7, 2024
View reviewed changes
...ptographicMaterialProviders/dafny/AwsCryptographyKeyStore/src/DefaultKeyStorageInterface.dfy

ensures
&& old(ddbClient.History.GetItem) < ddbClient.History.GetItem
&& old(ddbClient.History.TransactWriteItems) == ddbClient.History.TransactWriteItems
Copy link
Contributor

Choose a reason for hiding this comment

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

@seebees What is going on here?

I just stumbled on this while working on Mutations,
but why assert the history of TransactWriteItems?

Is this a copy-paste mistake?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@texastony texastony texastony left review comments

@josecorella josecorella josecorella left review comments

@ajewellamz ajewellamz ajewellamz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@seebees @texastony @josecorella @ajewellamz