-
Notifications
You must be signed in to change notification settings - Fork 159
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
Clarify how to negotiate groups if you want to respect the client order. #1331
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2304,6 +2304,16 @@ MAY omit the most preferred groups. Such a situation could arise if the most | |
preferred groups are new and unlikely to be supported in enough places to | ||
make pregenerating key shares for them efficient. | ||
|
||
For this reason, the omission of a share for group A and inclusion of | ||
one for group B does not mean that the client prefers B to A. | ||
Selecting a group based on KeyShareEntry may result in the use of | ||
a less preferred group than the client and server mutually support, | ||
though saving the round trip of HelloRetryRequest. Servers | ||
that wish to respect the client's group preferences SHOULD first | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This may not just be client prefs, but also server prefs. I think really this is about the server not believing in equipreference groups, either by way of the client's prefs (since the client can't express them) or its own. But talking about equipreference groups in a spec that intentionally doesn't talk about selection criteria is kinda weird. Not sure what to do here. Maybe we don't need that example if we've spelled it out in the preceding text clearly enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's my hope. |
||
select a group based "supported_groups" and then either send a | ||
ServerHello or a HelloRetryRequest depending on the contents of | ||
KeyshareClienthello. | ||
|
||
Clients can offer as many KeyShareEntry values as the number of supported | ||
groups it is offering, each | ||
representing a single set of key exchange parameters. For instance, a | ||
|
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.
Optional: Is it worth calling out explicitly out the security issue explicitly?
Perhaps something like:
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.
I think the explanatory text is good, but the MUST in the last sentence is too strong. Like, suppose my policy is: I am happy with groups A, B, or C, though I prefer A. In that case, I don't need to look at supported_groups, I just need to pick the one I like best out of KeyShare if present, right?
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.
Ah fair. Yeah if you know that you only have one equipreference group then it doesn't matter. Maybe a SHOULD? Or we could just drop the sentence. Not attached to it.
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.
I think we do want a MUST-level requirement on the server, but for something like "MUST account for the possibility that supported_groups contains a more-preferred group than is present in key_share". Knowing that you only have a single equipreference grouping accounts for that possibility by rendering it impossible.
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.
I don't see how "MUST account for" is operationalizable.
As @davidben says, we've never made any requirements at all about how the server negotiates, so I don't think doing so now unconditionally is really appropriate.