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

crypto/tls: automatic CurvePreferences ordering #69393

Closed
FiloSottile opened this issue Sep 11, 2024 · 12 comments
Closed

crypto/tls: automatic CurvePreferences ordering #69393

FiloSottile opened this issue Sep 11, 2024 · 12 comments
Labels
Documentation Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues release-blocker
Milestone

Comments

@FiloSottile
Copy link
Contributor

CurvePreferences (a legacy misnomer for key exchange mechanisms) are about to become a lot more complex with the post-quantum transition. For example on the client side we will want to send both a MLKEM768X25519 and a X25519 key share, while on the server side we might want to trigger a Hello Retry Request if it allows upgrading to PQ. All this complexity is both difficult to express in the current API, and an excessive burden to delegate to applications.

I propose we do the same thing we did for CipherSuites in #45430 and remove the preference order semantics and the implicit key share selection. The CurvePreferences list, if not nil, will be a list of enabled key exchanges, and the choice of which ones to prioritize and what key shares to send will be left with crypto/tls.

Changing the key share logic is allowed by the current docs. Removing the preference ordering is equivalent to #45430.

// CurvePreferences contains the elliptic curves that will be used in
// an ECDHE handshake, in preference order. If empty, the default will
// be used. The client will use the first preference as the type for
// its key share in TLS 1.3. This may change in the future.
CurvePreferences []CurveID

One day we might need a way for the application to pass on draft-ietf-tls-key-share-prediction hints from DNS, like ECH parameters, but the current CurvePreferences was anyway poorly suited for the nuanced semantics required to avoid downgrade attacks. (If an application simply passed the draft-ietf-tls-key-share-prediction hints as CurvePreferences, a DNS attacker could downgrade key exchanges from PQ to non-PQ. They would have to sort the list based on the hints, but also applying logic to allow key share reuse. That's unrealistic and I'd rather have the application pass on the raw DNS payload like with ECH.)

/cc @golang/security @golang/proposal-review

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Sep 11, 2024
@FiloSottile FiloSottile added this to the Proposal milestone Sep 11, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@mateusz834
Copy link
Member

@FiloSottile Do you see a need for a new GODEBUG for this change? #45430 did not introduce one.

@aclements
Copy link
Member

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.

@aclements
Copy link
Member

Am I understanding correctly that the proposal is to switch tls.Config.CurvePreferences from an ordered preference list to, effectively, a set, where std always provides the preference order?

Given that we haven't had any complaints about the equivalent change to cipher suites, I'm leaning toward not providing a GODEBUG. If we do end up adding a GODEBUG, it would have to either do nothing in FIPS mode or panic.

@FiloSottile
Copy link
Contributor Author

Am I understanding correctly that the proposal is to switch tls.Config.CurvePreferences from an ordered preference list to, effectively, a set, where std always provides the preference order?

Yep!

Given that we haven't had any complaints about the equivalent change to cipher suites, I'm leaning toward not providing a GODEBUG. If we do end up adding a GODEBUG, it would have to either do nothing in FIPS mode or panic.

I'm not sure this interacts that much with FIPS mode. In FIPS mode the set of supported CurvePreferences is thinned to only the NIST-approved ones, but ordering is mostly unaffected.

I'm ok with not adding a GODEBUG.

@aclements aclements moved this from Active to Likely Accept in Proposals Nov 6, 2024
@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.

The proposal is to change tls.Config.CurvePreferences from a preference-ordered list to a set of acceptable curves.

        // CurvePreferences contains the elliptic curves that will be used in
        // an ECDHE handshake. The order of the list is ignored, and a curve
        // is chosen from this list using an internal preference order.
	// If empty, the default will be used.
        CurvePreferences []CurveID

@aclements
Copy link
Member

@FiloSottile , I wrote a new doc comment for CurvePreferences. Please check if what I wrote is reasonable.

@aclements aclements moved this from Likely Accept to Accepted in Proposals Nov 13, 2024
@aclements
Copy link
Member

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

The proposal is to change tls.Config.CurvePreferences from a preference-ordered list to a set of acceptable curves.

        // CurvePreferences contains the elliptic curves that will be used in
        // an ECDHE handshake. The order of the list is ignored, and a curve
        // is chosen from this list using an internal preference order.
	// If empty, the default will be used.
        CurvePreferences []CurveID

@aclements aclements changed the title proposal: crypto/tls: automatic CurvePreferences ordering crypto/tls: automatic CurvePreferences ordering Nov 13, 2024
@aclements aclements modified the milestones: Proposal, Backlog Nov 13, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/630775 mentions this issue: crypto/tls: implement X25519MLKEM768

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 26, 2024
@dmitshur
Copy link
Contributor

This looks like a change that needs to be mentioned in Go 1.24 release notes, is that right? Reopening as a release blocker to track that.

@mknyszek
Copy link
Contributor

mknyszek commented Dec 4, 2024

The RC is planned for next week, and we need a full draft of the release notes before then. Please prioritize writing the release notes for this. Thanks!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/634075 mentions this issue: _content/doc: add crypto/tls key exchange note

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues release-blocker
Projects
Status: Accepted
Development

No branches or pull requests

8 participants