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

Send "supported_groups" in encrypted extensions #375

Closed
wants to merge 8 commits into from
Closed

Send "supported_groups" in encrypted extensions #375

wants to merge 8 commits into from

Conversation

ocheron
Copy link
Contributor

@ocheron ocheron commented Jul 7, 2019

When the group selected is not the one prefered by the server it is possible to send this extension.

On client side, we can also control not only that the group received is supported by the client, but that this is the same group for which the key share was sent.

Adds the requirement from RFC 8446 section 4.2.7:

   If the server has a group it prefers to the ones in the "key_share"
   extension but is still willing to accept the ClientHello, it SHOULD
   send "supported_groups" to update the client's view of its
   preferences; this extension SHOULD contain all groups the server
   supports, regardless of whether they are currently supported by the
   client.
Replaces current verification with a more specific one to comply with
RFC 8446 section 4.2.8:

   If using (EC)DHE key establishment and a HelloRetryRequest
   containing a "key_share" extension was received by the client, the
   client MUST verify that the selected NamedGroup in the ServerHello
   is the same as that in the HelloRetryRequest.  If this check fails,
   the client MUST abort the handshake with an "illegal_parameter"
   alert.
@kazu-yamamoto kazu-yamamoto self-requested a review July 8, 2019 01:06
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

I would like to have test cases for this feature.

@ocheron
Copy link
Contributor Author

ocheron commented Jul 9, 2019

There is no user feature. What aspect do you think should be tested?

@kazu-yamamoto
Copy link
Collaborator

I was thinking of a normal scenario based on parameters selected by hand, not generated by QuickCheck. In this case, we should check that the server surely sends supported_groups so that the client view can be changed.

@kazu-yamamoto
Copy link
Collaborator

@ocheron Are you planning to add a test case? Or would you like to merge this PR as is?

@ocheron
Copy link
Contributor Author

ocheron commented Jul 17, 2019

Yes I will add a test case with hookRecvHandshake13.

@ocheron
Copy link
Contributor Author

ocheron commented Jul 18, 2019

So I added hookRecvHandshake13, a test case verifying that the EE supported groups are sent, and marshalling tests for Handshake13.

@kazu-yamamoto kazu-yamamoto self-requested a review July 19, 2019 00:15
Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

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

Now LGTM

kazu-yamamoto added a commit to kazu-yamamoto/hs-tls that referenced this pull request Jul 19, 2019
@ocheron
Copy link
Contributor Author

ocheron commented Jul 20, 2019

Thank you

@ocheron ocheron closed this Jul 20, 2019
@ocheron ocheron deleted the ee-groups branch July 20, 2019 07:13
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.

None yet

2 participants