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

Clarify how to negotiate groups if you want to respect the client order. #1331

Merged
merged 4 commits into from
Nov 29, 2023

Conversation

ekr
Copy link
Contributor

@ekr ekr commented Nov 11, 2023

This tries to make the situation clearer. It does add a normative SHOULD but I think it's obvious.

@ekr ekr requested a review from chris-wood November 11, 2023 07:06
@ekr
Copy link
Contributor Author

ekr commented Nov 11, 2023

@davidben

Copy link
Contributor

@kaduk kaduk left a comment

Choose a reason for hiding this comment

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

The SHOULD is scoped to "servers that wish to respect the client's group preferences" anyway; it could equally well be "need to" in my opinion.

though saving the round trip of HelloRetryRequest. Servers
that wish to respect the client's group preferences SHOULD first
select a group based "supported_groups" and then either complete the
handshake or send a HelloRetryRequest depending on the contents of
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "complete the handshake" what we want here? I would say that both branches of the decision tree are completing the handshake, it's just that one is not doing so immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. How about "send either a ServerHello or a HelloRetryRequest depending..."

Copy link
Contributor

Choose a reason for hiding this comment

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

Chris has put in the suggestion; thanks (looks good to me)

@ekr
Copy link
Contributor Author

ekr commented Nov 12, 2023

The SHOULD is scoped to "servers that wish to respect the client's group preferences" anyway; it could equally well be "need to" in my opinion.

Yes, we could if we were trying to avoid 2119 language, but it seems like a circumlocution

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,
Copy link
Contributor

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:

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 "key_share" alone may result a less preferred group than the client and server mutually support. Although this saves the round trip of HelloRetryRequest, the selected group may be less secure than another common option. Servers MAY preferentially select a group based on "key_share" to reduce round trips, but MUST consider all options in "supported_groups" when making this decision.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my hope.

@seanturner
Copy link
Contributor

@kaduk Is this good-to-go from your perspective?

@kaduk
Copy link
Contributor

kaduk commented Nov 29, 2023

@kaduk Is this good-to-go from your perspective?

Once Chris's suggestion is taken, it is okay from my perspective.

…ntents of KeyshareClienthello."

Co-authored-by: Christopher Wood <caw@heapingbits.net>
@dconnolly
Copy link
Contributor

@kaduk Is this good-to-go from your perspective?

Once Chris's suggestion is taken, it is okay from my perspective.

Suggestion committed

@dconnolly dconnolly merged commit 55a718b into tlswg:main Nov 29, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants