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

feat: Adding Caching CMM #80

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: Adding Caching CMM #80

wants to merge 15 commits into from

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Oct 3, 2023

Adding a Caching CMM implementation

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

Resolves #480

Adding a Caching CMM implementation
@seebees seebees requested a review from a team as a code owner October 3, 2023 04:05
texastony
texastony previously approved these changes Oct 3, 2023
Copy link
Contributor

@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 know it's not finished, but I don't have any blocking complaints.

@javadoc("The Cryptographic Materials Cache the Caching Cryptographic Materials Manager will use to cache requests.")
underlyingCMC: CryptographicMaterialsCacheReference,
@required
@javadoc("Sets the maximum lifetime for entries in the cache, for both encrypt and decrypt operations. When the specified amount of time passes after initial creation of the entry, the entry will be considered unusable, and the next operation will incur a cache miss.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/Suggestion: we cache materials, not operations

Suggested change
@javadoc("Sets the maximum lifetime for entries in the cache, for both encrypt and decrypt operations. When the specified amount of time passes after initial creation of the entry, the entry will be considered unusable, and the next operation will incur a cache miss.")
@javadoc("Sets the maximum lifetime for entries in the cache, for both encrypt and decrypt materials. When the specified amount of time passes after initial creation of the entry, the entry will be considered unusable, and the next operation will incur a cache miss.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, I think I want operations but I'm not sure.
The point here is that there is 1 cache.
But that items put in from encrypt are not hits on decrypt.

//# - [Partition ID](#partition-id)
//# - [Limit Bytes](#limit-bytes)
//# - [Limit Messages](#limit-messages)
@javadoc("Sets the partition ID for this CMM. By default, two CMMs will never use each other's cache entries. This helps ensure that CMMs with different delegates won't incorrectly use each other's encrypt and decrypt results. However, in certain special circumstances it can be useful to share entries between different CMMs - for example, if the backing CMM is constructed based on some parameters that depend on the operation, you may wish for delegates constructed with the same parameters to share the same partition. To accomplish this, set the same partition ID and backing cache on both CMMs; entries cached from one of these CMMs can then be used by the other. This should only be done with careful consideration and verification that the CMM delegates are equivalent for your application's purposes. By default, the partition ID is set to a random UUID to avoid any collisions.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: javadoc is not handled by our Polymorph-.NET.
Is this content recorded in the public hosted docs?
Otherwise, .NET customers will never receive it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This content comes from the ESDK-Java.
So it is somewhat public.
I'd like to get javadoc to work in every language so we don't have to worry about this :)

Comment on lines +384 to +387
// For Dafny keyrings this is a trivial statement
// because they implement a trait that ensures this.
// However not all keyrings are Dafny keyrings.
// Customers can create custom keyrings.
Copy link
Contributor

Choose a reason for hiding this comment

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

Praise: These comments are very helpful

0,0,0,0,0,0,0,0
]

lemma PaddingIsCorrect()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda of fun.

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 was very pleased myself.
Especially since the first time I wrote it I have 8x9!

Comment on lines +890 to +892
:= cryptoPrimitives.History.Digest[
|old(cryptoPrimitives.History.Digest)| .. |old(cryptoPrimitives.History.Digest)| + |input.encryptedDataKeys|
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is this selection by index of History of Digest calls?
I'm not familiar with .. in Dafny,
but I presume it is slicing.

I am confused because |old(cryptoPrimitives.History.Digest)| is used twice...
OH, select from oldIndex to oldIndex + EDK count.
Got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is slice.
Since there are some number of EDKs, I need to get all of them.
http://dafny.org/dafny/DafnyRef/DafnyRef#sec-other-sequence-expressions

Copy link
Contributor

Choose a reason for hiding this comment

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

I had not realized the Decrypt cache's identifier included a digest of all EDKs for a message.

Updates to CachingCMM

Small update to the create operation

small update
…alProviders/Model/cmms.smithy

Co-authored-by: Tony Knapp <[email protected]>
…alProviders/Model/cmms.smithy

Co-authored-by: Tony Knapp <[email protected]>
…alProviders/Model/cmms.smithy

Co-authored-by: Tony Knapp <[email protected]>
…alProviders/Model/cmms.smithy

Co-authored-by: Tony Knapp <[email protected]>
See test vector PR for updates
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.

Release Caching Cryptographic Materials Manager for Keyrings
2 participants