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

Handshake changes necessary to negotiate NPN #3558

Merged
merged 12 commits into from
Oct 24, 2022

Conversation

maddeleine
Copy link
Contributor

@maddeleine maddeleine commented Oct 17, 2022

Resolved issues:

Related to #3516

Description of changes:

This pulls some code out of the monster PR: #3545. Essentially I am pulling out the important handshake message changes that need to occur to negotiate NPN. These are:

  1. Calculate the verify data when receiving the Finished message to incorporate the Encrypted Extensions message in an NPN handshake.
  2. Switch to the secure crypto parameters when sending an Encrypted Extensions message as this is now the first encrypted message.

None of these changes should affect the regular non-NPN handshake.

Call-outs:

I am trying to keep the changes in this PR in sync with #3545 so you can see that these changes let us pass the s2n_self_talk_npn_test. Otherwise I can't really prove that these changes are necessary to successfully negotiate NPN.

Testing:

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

@maddeleine maddeleine requested a review from a team as a code owner October 17, 2022 22:36
@github-actions github-actions bot added the s2n-core team label Oct 17, 2022
@maddeleine maddeleine requested a review from lrstewart October 18, 2022 04:11
@maddeleine maddeleine changed the title Removes unnecessary sequence number zeroing Handshake changes necessary to negotiate NPN Oct 19, 2022
@maddeleine maddeleine requested a review from lrstewart October 19, 2022 21:57
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

From offline discussion: it's pretty hard to reason about these changes, because the actions of these messages are now very dependent on message ordering. Which isn't great.

I think we might be able to make this easier to follow with some abstraction. Like, a "start encryption" method that, if the crypto parameters aren't set to conn->secure yet, zeroes the sequence number and sets the crypto parameters to conn->secure. Both npn and the ccs message would call the new method, but wouldn't have to be aware of each other.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Just a few nits now. It's sad that pre-renegotiation you'd be saving us a few bytes, because we wouldn't need to store the verify_data anymore :(

@maddeleine maddeleine requested a review from lrstewart October 21, 2022 23:57
@maddeleine maddeleine merged commit 99b4351 into aws:main Oct 24, 2022
@maddeleine maddeleine deleted the npn_sequence_num branch October 24, 2022 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants