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: add X25519MLKEM768 and use by default; remove x25519Kyber768Draft00 #69985

Closed
FiloSottile opened this issue Oct 22, 2024 · 14 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Oct 22, 2024

Important

Nov 20, 2024: The latest version of the proposal is here.

The experiment with X25519Kyber768Draft00 #67061 went well, and NIST has now published the final version of the ML-KEM standard. https://datatracker.ietf.org/doc/draft-kwiatkowski-tls-ecdhe-mlkem/ specifies ML-KEM hybrids, including permanent codepoints, and Chrome and Cloudflare already deployed one of them.

I propose in Go 1.24 we remove X25519Kyber768Draft00 and add SecP256r1MLKEM768 and X25519MLKEM768, enabling both by default, and sending X25519MLKEM768 + X25519 key shares by default. (See #69393 for the precise preference ordering.)

We'll also add the following two CurveID constants, for use in Config.CurvePreferences and possibly for #67516.

const SecP256r1MLKEM768 CurveID = 4587
const X25519MLKEM768 CurveID = 4588

GODEBUG=tlsmlkem=0 reverts the default Config.CurvePreferences (but doesn't disable anything else).

It would be tempting to retain support for X25519Kyber768Draft00, because it's unfortunate that Go 1.23 to Go 1.24 connections would be weaker than Go 1.23 to Go 1.23, but that is already unavoidable in one direction: Go 1.23 will prefer a key share over other supported key exchange methods, so a Go 1.24 client that sends X25519MLKEM768 + X25519 will see X25519 selected by a Go 1.24 server even if it advertises support for X25519Kyber768Draft00.

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

Related Issues and Documentation

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

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Oct 22, 2024
@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 aclements moved this from Incoming to Active in Proposals Oct 23, 2024
@aclements
Copy link
Member

it's unfortunate that Go 1.23 to Go 1.24 connections would be weaker than Go 1.23 to Go 1.23

Given that post-quantum support in 1.23 was an experiment anyway, I'm not too bothered by the weaker crypto in this case.

@aclements
Copy link
Member

Have all remaining concerns about this proposal been addressed?

The proposal detalis are in #69985 (comment)

@FiloSottile
Copy link
Contributor Author

SecP256r1MLKEM768 has not yet stabilized in the IETF, so let's leave it out for Go 1.24.

X25519MLKEM768 has been widely deployed already, so it's not changing at this point.

@rsc
Copy link
Contributor

rsc commented Nov 12, 2024

I believe this proposal is now:

  1. Add X25519MLKEM768 CurveID to crypto/tls.
  2. Enable X25519MLKEM768 for use by default, by adding it to the default Config.CurvePreferences (semantics more like "AllowedCurves" after crypto/tls: automatic CurvePreferences ordering #69393).
  3. Add new GODEBUG tlsmlkem to control (2). tlsmlkem=1 is the default for Go 1.24; setting tlsmlkem=0 undoes (2).

@aclements
Copy link
Member

Just to be clear, we're still proposing to remove X25519Kyber768Draft00, right?

@FiloSottile
Copy link
Contributor Author

Just to be clear, we're still proposing to remove X25519Kyber768Draft00, right?

Correct!

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

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

The proposal is to

  1. Add const X25519MLKEM768 CurveID = 4588 to crypto/tls
  2. Enable X25519MLKEM768 for use by default, by adding it to the default Config.CurvePreferences
  3. Add new GODEBUG tlsmlkem to control (2). tlsmlkem=1 is the default for Go 1.24; setting tlsmlkem=0 undoes (2).
  4. Remove X25519Kyber768Draft00

@aclements
Copy link
Member

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

The proposal is to

  1. Add const X25519MLKEM768 CurveID = 4588 to crypto/tls
  2. Enable X25519MLKEM768 for use by default, by adding it to the default Config.CurvePreferences
  3. Add new GODEBUG tlsmlkem to control (2). tlsmlkem=1 is the default for Go 1.24; setting tlsmlkem=0 undoes (2).
  4. Remove X25519Kyber768Draft00

@aclements aclements moved this from Likely Accept to Accepted in Proposals Nov 21, 2024
@aclements aclements changed the title proposal: crypto/tls: SecP256r1MLKEM768 and X25519MLKEM768 crypto/tls: SecP256r1MLKEM768 and X25519MLKEM768 Nov 21, 2024
@aclements aclements modified the milestones: Proposal, Backlog Nov 21, 2024
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Nov 22, 2024
This makes three related changes that work particularly well together
and would require significant extra work to do separately: it replaces
X25519Kyber768Draft00 with X25519MLKEM768, it makes CurvePreferences
ordering crypto/tls-selected, and applies a preference to PQ key
exchange methods over key shares (to mitigate downgrades).

TestHandshakeServerUnsupportedKeyShare was removed because we are not
rejecting unsupported key shares anymore (nor do we select them, and
rejecting them actively is a MAY). It would have been nice to keep the
test to check we still continue successfully, but testClientHelloFailure
is broken in the face of any server-side behavior which requires writing
any other messages back to the client, or reading them.

Updates #69985
Fixes #69393

Change-Id: I58de76f5b8742a9bd4543fd7907c48e038507b19
Reviewed-on: https://go-review.googlesource.com/c/go/+/630775
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 26, 2024
@dmitshur
Copy link
Contributor

It looks like CL 630775 implemented most of this proposal if not all, though it didn't close it. What's left for Go 1.24? Is it just SecP256r1MLKEM768, which according to #69985 (comment) is being left out of Go 1.24? (If so, perhaps that can be tracked in a separate issue, or this one should be moved to the next milestone.)

@aclements
Copy link
Member

Since we decided to leave SecP256r1MLKEM768 out of this proposal in the end, I think that should be a new proposal (perhaps once it's stabilized by the IETF). Given that, it look like CL 630775 did indeed implement the whole proposal, but I'm not 100% sure so I'll let @FiloSottile do the closing.

@FiloSottile
Copy link
Contributor Author

Sounds good, we can leave SecP256r1MLKEM768 to a future proposal.

@dmitshur dmitshur changed the title crypto/tls: SecP256r1MLKEM768 and X25519MLKEM768 crypto/tls: add X25519MLKEM768 and use by default; remove x25519Kyber768Draft00 Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Accepted
Development

No branches or pull requests

6 participants