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: simplify structured encryption #866

Merged
merged 46 commits into from
May 13, 2024
Merged

feat: simplify structured encryption #866

merged 46 commits into from
May 13, 2024

Conversation

ajewellamz
Copy link
Contributor

@ajewellamz ajewellamz commented Mar 27, 2024

Description of changes:

Change complex recursive data structure to seq<Path, Data, Action>

Keep backward compatible interface with old map<String, Data> and map<String, Action>

If you're new to this, you should probably start with
specification/changes/2024-05-19-simplify-structured-encryption/
and then
DynamoDbEncryption/dafny/StructuredEncryption/Model/StructuredEncryption.smithy

The only source file where anything "real" happened is
DynamoDbEncryption/dafny/StructuredEncryption/src/AwsCryptographyDbEncryptionSdkStructuredEncryptionOperations.dfy

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ajewellamz ajewellamz marked this pull request as ready for review March 29, 2024 13:23
@ajewellamz ajewellamz requested a review from a team as a code owner March 29, 2024 13:23
@ajewellamz ajewellamz marked this pull request as draft March 30, 2024 15:28
Comment on lines -529 to +519
: (ret : Result<CSE.CryptoSchema, string>)
: (ret : Result<CSE.CryptoAction, string>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be a breaking change for customers? (i.e. this symbol change)

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 this is all internal to a single library, nothing outside of the Gazelle library should ever see this, so replacing the Gazelle library should not break anything.
The smithy model hasn't changed.

Comment on lines +127 to +128
// Does NOT guarantee a unique output for every unique input
// e.g. ['a.b'] and ['a','b'] both return 'a.b'.
Copy link
Contributor

Choose a reason for hiding this comment

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

For this kind of thing, I wonder if a lemma would be helpful?
As it, this would encode this invariant in a way that can't be mindlessly broken

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 don't understand. A lemma that ensures what?

Copy link
Contributor

Choose a reason for hiding this comment

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

a Lemma that ensures:
Does NOT guarantee a unique output for every unique input

Comment on lines +227 to +231
// must be true for any correct UTF8 implementation
lemma {:axiom} Utf8EncodeUnique(x : string, y : string)
requires UTF8.Encode(x).Success?
requires UTF8.Encode(y).Success?
ensures !(x <= y) ==> !(UTF8.Encode(x).value <= UTF8.Encode(y).value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we already have this somewhere?

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 don't think so.

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.

my main concern regarding releasing new ungulates and backwards compatibility with the existing dbesdk is already captured as a blocking task for launching a new ungulate.

@ajewellamz ajewellamz merged commit a70a569 into main May 13, 2024
30 checks passed
@ajewellamz ajewellamz deleted the ajewell/simplify branch May 13, 2024 15:35
- Commitment Policy: This MUST be
[REQUIRE_ENCRYPT_REQUIRE_DECRYPT](../../submodules/MaterialProviders/aws-encryption-sdk-specification/framework/commitment-policy.md#esdkrequire_encrypt_require_decrypt).
- Algorithm Suite: If provided, this is the [input algorithm suite](#algorithm-suite);
otherwise, this field MUST be the algorithm suite corresponding to the enum
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this value be part of the CMM? this may be too late, but how would a CMM pick the default?

Comment on lines +155 to +156
An error MUST be returned if any of the entries added to the encryption context in this step
have the same key as any entry already in the encryption context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as I'm reading, it would be nice to have a link from here (and the decrypt relevant section) to a discussion of the complex path problem. e.g. an attribute named a.b and an attribute a with a sub attribute of b.

e.g. the flattened EC for these 2 things would both be a.b

Comment on lines +179 to +180
This mapping does not produce a unique output for every unique input.
For example ['a.b'] and ['a', 'b'] both produce "a.b".
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 the section! nice, I think that Create New Encryption Context and CMM should link to this somehow yes?

rishav-karanjit pushed a commit that referenced this pull request May 30, 2024
## [3.5.0](v3.4.0...v3.5.0) (2024-05-30)

### Features

* **DynamoDbEncryption:** Add GetEncryptedDataKeyDescription operation ([#856](#856)) ([8f8471a](8f8471a))
* improve verification ([#1020](#1020)) ([cbde4ef](cbde4ef))
* simplify structured encryption ([#866](#866)) ([a70a569](a70a569))

### Maintenance

* allow Legacy to use subclass of DynamoDBEncryptor ([#1073](#1073)) ([135acd9](135acd9))
* bump MPL to 1.4 ([#1067](#1067)) ([51bbab5](51bbab5))
* **Java-Release:** update release commands and use SNAPSHOT builds ([#995](#995)) ([ac9b79e](ac9b79e))
* reformat and enforce formatting ([#1035](#1035)) ([8a76a9d](8a76a9d))
* verify with Dafny 4.6 ([#1072](#1072)) ([9db6e78](9db6e78))
ajewellamz pushed a commit that referenced this pull request May 31, 2024
* chore(release): 3.5.0

## [3.5.0](v3.4.0...v3.5.0) (2024-05-30)

### Features

* **DynamoDbEncryption:** Add GetEncryptedDataKeyDescription operation ([#856](#856)) ([8f8471a](8f8471a))
* improve verification ([#1020](#1020)) ([cbde4ef](cbde4ef))
* simplify structured encryption ([#866](#866)) ([a70a569](a70a569))

### Maintenance

* allow Legacy to use subclass of DynamoDBEncryptor ([#1073](#1073)) ([135acd9](135acd9))
* bump MPL to 1.4 ([#1067](#1067)) ([51bbab5](51bbab5))
* **Java-Release:** update release commands and use SNAPSHOT builds ([#995](#995)) ([ac9b79e](ac9b79e))
* reformat and enforce formatting ([#1035](#1035)) ([8a76a9d](8a76a9d))
* verify with Dafny 4.6 ([#1072](#1072)) ([9db6e78](9db6e78))
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.

3 participants