-
Notifications
You must be signed in to change notification settings - Fork 706
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: Changes ticket encryption scheme to be nonce-reuse resistant #4663
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lrstewart
reviewed
Jul 26, 2024
lrstewart
reviewed
Jul 26, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do a minor version bump for this, since it's going to cause a temporary drop in resumption rates during deployments.
lrstewart
reviewed
Jul 30, 2024
goatgoose
reviewed
Jul 30, 2024
lrstewart
reviewed
Aug 1, 2024
goatgoose
approved these changes
Aug 1, 2024
lrstewart
approved these changes
Aug 2, 2024
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
N/A
Description of changes:
This PR changes the pre-encryption schema for session tickets to prepend a version number. This allows us to update tickets in the future without as much disruption to existing resumption deployments. It also includes a change to how the ticket key is calculated, now we generate a unique key per ticket using an hkdf. It also includes a rust bindings function that I forgot to add in my last stek PR.
Call-outs:
Note that the integration batch job is failing. This is because this change will cause the cross-compatibility test to fail because the test asserts successful resumption between old and new servers. However this change breaks that test. I think we should just override and merge this PR rather than changing the test because that property should be true for most of our PRs. Integ passing without cross-compatibility: https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/S2nIntegrationV2SmallBatch/batch/S2nIntegrationV2SmallBatch%3A34e42b29-cd9f-44b7-9764-04d23c746379?region=us-west-2
The specific line that fails is "assert S2N_RESUMPTION_MARKER in results.stdout"
Testing:
Adds extra tests to ensure we error if we don't recognize the pre-encrypted ticket schema version number or if the bytes needed to generate the ticket key changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.