-
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
Client key share ordering does not mesh with HelloRetryRequest #644
Comments
Just put an "except for the last |
Sounds reasonable. Another option is we say |
Although keeping them sorted would be a huge pain, even if it is easier to enforce. That's probably not a good idea. |
Well, if we wanted the HRR case to jive nicely with sorting, we could mandate reverse priority order; the appended would be top priority. This would work nicely, but could be considered unnecessary if sorting is any burden. I don't remember if this was specifically why I put a SHOULD here, but mandating an absolute requirement here could just be more trouble than it's worth. The worry with not having any requirement is that a pair of bad implementations could use the key_share priority instead of supported_groups and mess things up, however I'm not so sure it's that big of a risk. I think a SHOULD with @martinthomson's exception is fine. |
@ekr's proposal on-list to replace the key_share after an HRR, rather than append to it, would fix this rather easily. Much better than fiddling with the language or ordering. |
This is what I get for uploading PR #643 immediately before trying to enforce it instead of immediately afterwards... :-) On the plus side, yay for tests!
The
client_shares
list in thekey_share
extension is supposed to be "in descending order of client preference". However, this isn't true for the second ClientHello because the new KeyShareEntry is always appended.It's only a SHOULD-level requirement with #643 reverted, but the spec text is still off, and it is a little bit odd for stateless HRR implementations. (Though if the stateless implementation remembers in the cookie that it already committed to a group, it's probably still fine?)
The text was updated successfully, but these errors were encountered: