feat: send psk_ke_modes ext in first flight #4177
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
rustls expects the psk_key_exchange_modes extension to be present in the first flight of messages if the client supports session resumption, otherwise it will not return a session ticket. This commit alters s2n-tls client behavior to match rustls expectations.
lrstewart
reviewed
Aug 30, 2023
* add compliance comment to relevant RFC section * add unit test for "should send" functionality
|
#4210 adds the CI workflow to assert that the interop tests contained in the rust bench crate are passing. |
maddeleine
approved these changes
Sep 20, 2023
lrstewart
reviewed
Sep 20, 2023
* move test above functional test * change test name to fit voted majority * use helper function to construct PSK * add semi-colons to unit tests scopes
lrstewart
reviewed
Sep 20, 2023
* sentence case the test names * remove the newline in the duvet comment
lrstewart
approved these changes
Sep 20, 2023
* assert on relevant qualities
lrstewart
reviewed
Sep 21, 2023
Comment on lines
+230
to
+231
| EXPECT_SUCCESS(s2n_config_set_session_tickets_onoff(no_resumption_config, false)); | ||
|
|
There was a problem hiding this comment.
Oh I like this as a solution :)
lrstewart
approved these changes
Sep 21, 2023
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:
#4124
Description of changes:
More context is in the linked issue, but essentially rustls expects the
psk_key_exchange_modesextension to be sent in the first flight of messages if the client wants a session ticket. s2n-tls does not sent this extension in the first flight of messages, so rustls servers won't send a session ticket, so s2n-tls clients can't use session resumption with rustls servers.This PR changes s2n-tls behavior to send the
psk_key_exchanges_modesextension whenever the clients supports either stateful or stateless session resumption. We still include the final check for PSKs to support out of bands PSKs.This change should be a strict increase in compatibility with other implementations.
Testing:
We have interop testing for session resumption in the rust bindings. Additionally all of the current unit tests pass.
I'm a little concerned that there isn't any direct testing of stateful session resumption. Testing that is going to be a little tricky, and the easiest way to do so would probably be to just add it to our benchmarking harness implementation? I'm interested in reviewers thoughts about whether this is a good investment of our time.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.