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!: Support AWS Cryptographic Material Providers Library (MPL) #685

Merged
merged 40 commits into from
Nov 12, 2024

Conversation

lucasmcdonald3
Copy link
Contributor

Issue #, if available:

Description of changes:

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

Check any applicable:

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

lucasmcdonald3 and others added 25 commits April 23, 2024 09:12
@lucasmcdonald3 lucasmcdonald3 changed the title feat: Support AWS Cryptographic Material Providers Library (MPL) feat!: Support AWS Cryptographic Material Providers Library (MPL) Sep 9, 2024
@lucasmcdonald3 lucasmcdonald3 marked this pull request as ready for review October 2, 2024 19:46
@lucasmcdonald3 lucasmcdonald3 requested a review from a team as a code owner October 2, 2024 19:46
@lucasmcdonald3 lucasmcdonald3 marked this pull request as draft October 2, 2024 19:47
README.rst Outdated Show resolved Hide resolved
@lucasmcdonald3 lucasmcdonald3 marked this pull request as ready for review October 28, 2024 22:52
seebees
seebees previously approved these changes Oct 29, 2024
Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

This is a very large PR.
Giving it a once-over it looks good.
You should get a few more eyes on it than me, but from what I see it looks good.

I would like to see a more clear link to the test vectors so I can wrap my mind around the... scope of testing :)

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we checking in bin files?

Copy link
Contributor Author

@lucasmcdonald3 lucasmcdonald3 Oct 29, 2024

Choose a reason for hiding this comment

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

It looks like this is just how Ritvik wrote the performance tests -- the tests take in a file and decrypt/encrypt it many times. We should probably rewrite this to generate files on the fly for our own testing at some point

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.

small nit in the error message

src/aws_encryption_sdk/internal/formatting/deserialize.py Outdated Show resolved Hide resolved
seebees
seebees previously approved these changes Oct 31, 2024
Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

Thanks for adding the extra tests!

Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

LGTM

@lucasmcdonald3 lucasmcdonald3 merged commit a7ebffe into master Nov 12, 2024
257 checks passed
@lucasmcdonald3 lucasmcdonald3 deleted the mpl-reviewed branch November 12, 2024 23:38
@alex
Copy link
Contributor

alex commented Nov 13, 2024

Following this PR, we (pyca/cryptography) are seeing that our CI where we run aws-encryption-sdk-python's tests is failing: https://github.com/pyca/cryptography/actions/runs/11807967227/job/32895677501?pr=11941

Is this expected? Do we need to be doing something more, or is this an issue to be resolved on teh aws-encryption-sdk-python side?

@lucasmcdonald3
Copy link
Contributor Author

Hi @alex,

This is expected. I opened a PR to cryptography to update the invocation: https://github.com/pyca/cryptography/pull/11942/files

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.

5 participants