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

X25519MLKEM768 to MLKEM768X25519 #26

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kriskwiatkowski
Copy link
Member

This is done to align with the requirement described in section 3.2 of draft-ietf-tls-hybrid-design-11.

This is done to align with the requirement described in section 3.2 of draft-ietf-tls-hybrid-design-11.
@bwesterb
Copy link
Contributor

bwesterb commented Dec 11, 2024

I don't believe there is working group consensus to make this change, and X25519MLKEM768 under that name is widely deployed already.

@kriskwiatkowski
Copy link
Member Author

kriskwiatkowski commented Dec 11, 2024

Ha! I calculated (zulip + mailing list), 5 people prefer to swap, 4 people prefer not to, 2 people undecided/don't care.
The other thing we could is just to mention that the name X25519MLKEM768 is kind of exception and it is not aligned with draft-tls-hybrid. And that's it.

Frankly, I would like to find consensus (whatever it is) and make the draft ready for adoption.

@csosto-pk
Copy link
Contributor

X25519MLKEM768 is kind of exception and it is not aligned with draft-tls-hybrid.

I think that is painless enough. Did it come up in the list a people objected? I don't remember.

@dconnolly
Copy link

X25519MLKEM768 is kind of exception and it is not aligned with draft-tls-hybrid.

I think that is painless enough. Did it come up in the list a people objected? I don't remember.

I did 🫣

geraldcombs pushed a commit to wireshark/wireshark that referenced this pull request Dec 11, 2024
@kriskwiatkowski
Copy link
Member Author

kriskwiatkowski commented Dec 11, 2024

How about that, then? #27?

@bwesterb
Copy link
Contributor

cc @davidben

@kriskwiatkowski
Copy link
Member Author

@dconnolly what do you think about #27 ?

@davidben
Copy link
Contributor

No strong preference on the name, as long as the wire format stays compatible. Leaving a footnote is fine with me.

@dconnolly
Copy link

Since the wire format is the same, the codepoint is the same, it should align with -hybrid-design and have the name match the wire format, as intended

@bwesterb
Copy link
Contributor

bwesterb commented Dec 11, 2024

If you are a developer and see these two choices:

  • P256MLKEM768
  • MLKEM768X25519

The user might think: why is the MLKEM768 in a different place? Does it matter?

Of course the order is immaterial. It's less confusing if we just use the names.

  • P256MLKEM768
  • X25519MLKEM768

Of course this is more confusing to the crypto engineer that need to implement this in TLS libraries. But there are many more developers picking KEMs than there are crypto engineers implementing them. We shouldn't move undue complexity to the enduser.

@davidben
Copy link
Contributor

I do agree it would be a poor outcome if the names were different like that. It is... unfortunate that we ended up making the wire order inconsistent but the wire order impacts fewer people than the names.

@dconnolly
Copy link

To be clear, this is a MUST in -hybrid-design:

image

@FiloSottile
Copy link

Strong preference for keeping X25519MLKEM768.

We already deployed it under that name and it's forever seared into our backwards-compatibility promise. golang/go#69985

I also find the argument in #26 (comment) persuasive. There are dozens (maybe less?) of implementers and tens of thousands (maybe more?) system administrators that will be exposed to these names.

I'd even argue to change the overall rule to [non-PQ][PQ] in -hybrid-design.

@dconnolly
Copy link

Strong preference for keeping X25519MLKEM768.

We already deployed it under that name and it's forever seared into our backwards-compatibility promise. golang/go#69985

I also find the argument in #26 (comment) persuasive. There are dozens (maybe less?) of implementers and tens of thousands (maybe more?) system administrators that will be exposed to these names.

I'd even argue to change the overall rule to [non-PQ][PQ] in -hybrid-design.

You made backwards-compatibility commitments on an unstable document?

@dconnolly
Copy link

Strong preference for keeping X25519MLKEM768.

We already deployed it under that name and it's forever seared into our backwards-compatibility promise. golang/go#69985

I also find the argument in #26 (comment) persuasive. There are dozens (maybe less?) of implementers and tens of thousands (maybe more?) system administrators that will be exposed to these names.

I'd even argue to change the overall rule to [non-PQ][PQ] in -hybrid-design.

Also -hybrid-design has already gone through last call

@FiloSottile
Copy link

I deployed a security-relevant improvement to my users, the same that's deployed by browsers and CDNs, and referred to it by its current name. I stand by that choice.

-hybrid-design is a sign, not a cop, and it would be a deeply sad outcome if we deliberately chose to sign up thousands of people for confusion because we feel we can't amend a last call or add a note saying it doesn't match a document that anyway AFAICT is not necessary to implement X25519MLKEM768.

Looking at this thread, X25519MLKEM768 has rough consensus and running code.

@FiloSottile
Copy link

Also, the IANA registry already has an entry for 4588 with name X25519MLKEM768.

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8

It feels completely legitimate to rely on names in the IANA registry not to change. Can IANA registries be changed in a non-backwards compatible way? What would even be the point of IANA then?

@kriskwiatkowski
Copy link
Member Author

@FiloSottile, if we proceed with this change, then the plan is to update this name in IANA without modifying the code point value.

@tomato42
Copy link
Contributor

@FiloSottile

Also, the IANA registry already has an entry for 4588 with name X25519MLKEM768.

https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-parameters-8

It feels completely legitimate to rely on names in the IANA registry not to change. Can IANA registries be changed in a non-backwards compatible way? What would even be the point of IANA then?

the IDs in IANA referenced by drafts can definitely change, they're not immutable

while I agree that changing the name is painful, providing an alias, where both X25519MLKEM768 and MLKEM768X25519 will work to enable the same group isn't insurmountable

and just looking at gnutls, openssl and NSS I see different names for this fundamentally the same group already; hell, we've had OpenSSL refer to secp256r1 as prime256v1 as basically the only implementation and the internet still managed to widely deploy it, so it's not like Go needs to change the name to align it with IANA

at the end of the day, the users don't care (or even know) what key exchange they use, as long as the connection works, they're happy

@rolandshoemaker
Copy link

rolandshoemaker commented Dec 12, 2024

Chiming in as a maintainer of the Go crypto libraries. While I understand the intent behind this change, I would like to add that this seems likely to cause more pain and confusion than it is likely to solve. The mismatch between the name and wire format is unfortunate, but that property is likely of importance to 1% of the people who will be exposed to the name.

Switching now, after many libraries have already deployed using the current name, and much documentation, and many blog posts have been written using the current name, feels like it'd cause significant confusion.

As a library maintainer, the idea that users don't need to worry about these naming choices strikes me as incorrect. The confusing naming choices of ciphersuites and crypto primitives, and a lack of consistency around them, turns out to be a consistent pain point for users, and for maintainers who have to answer their questions. The NIST P-256/secp256r1/prime256v1 confusion is still something we get questions about, and the RFC 8422 Appendix A "clarifications" have done nothing to prevent this.

In this case we are perhaps lucky that the proposed change is just reversing the order, but I would put a not insignificant amount of money on the fact that if the name changes now, I'll be fielding questions about whether X25519MLKEM768 is the same thing as MLKEM768X25519, or why we don't implement one or the other, for the foreseeable future.

For better or worse, (quite a bit of) deployed software now uses the name X25519MLKEM768, and search engines are going to perpetuate this mistake for eternity, whether or not the name is changed at this late point.

@alexzas
Copy link

alexzas commented Dec 13, 2024

An implementer here adding hybrid MLKEM to our CryptoComply library. I must admit I initially assumed that the order of key shares is dictated by scheme name (which is correct for most other schemes) and spent several days debugging failed connections to Chrome client.
I wonder if "NIST intends to allow the 800-56C key derivation methods to apply to shared secrets of the form Z = T || Z’, where T and Z’ are ...in reverse order." should help here?

My assumption is that PQC migration will affect much wider audience, with some not even familiar with FIPS, but they might open wireshark and see inconsistency on the wire.

I hope we are still in the early days of PQC migration and can make things right..

my 2c,
Alex

@bwesterb
Copy link
Contributor

I must admit I initially assumed that the order of key shares is dictated by scheme name (which is correct for most other schemes) and spent several days debugging failed connections to Chrome client.

Sorry about that :(.

My assumption is that PQC migration will affect much wider audience, with some not even familiar with FIPS, but they might open wireshark and see inconsistency on the wire.

On the wire these are almost uniform random blobs. Technically I can tell the order with good probability (because coefficients mod 3329 are stored in 12 bits), but I doubt a casual user would see that.

@vdukhovni
Copy link

I fail to see how the requirement on the wire order of the components matching the order of algorithms defining the hybrid translates to a requirement on the group name. The group name X25519MLKEM768 can easily refer the ordered algorithm pair (MLKEM768, X25519), which then produce their outputs in the widely implemented order expected of the code point. The group name (bikeshed colour) could be REDBLUE for all it matters.

Let's not change the name now unless (rather unexpectedly) the wire order is changing and a new code point is being assigned.

@@ -64,7 +64,7 @@ informative:
ANSI: ANS X9.62-2005
--- abstract

This draft defines three hybrid key agreements for TLS 1.3: X25519MLKEM768,
This draft defines three hybrid key agreements for TLS 1.3: MLKEM768X25519,

Choose a reason for hiding this comment

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

This confuses identities with identifiers. The name is just an identifier, and so long as the group by that name is defined to be an ordered combination of ML-KEM-768 (NIST name) and X25519, there is no conflict with the hybrid design draft, that draft isn't about how things are named, it is about how they're defined.

So this change feels gratuitous.

@@ -106,14 +106,14 @@ special publication 800-56Cr2 {{SP56C}} approves the usage of HKDF
a FIPS-approved key-establishment scheme. FIPS also requires a certified implementation
of the scheme, which will remain more ubiqutous for secp256r1 in the coming years.

For this reason we put the ML-KEM shared secret first in X25519MLKEM768,
For this reason we put the ML-KEM shared secret first in MLKEM768X25519,

Choose a reason for hiding this comment

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

Whatever the reason, the definition should first state the order of the two component algorithms (state the definition of the group), and only then perhaps give a rationale, or note that in that particular order the result may be better aligned with FIPS (which may or may not be true).

and the ECDH shared secret first in SecP256r1MLKEM768 and SecP384r1MLKEM1024.

## Construction

### Client share

When the X25519MLKEM768 group is negotiated, the client's key_exchange value
When the MLKEM768X25519 group is negotiated, the client's key_exchange value
is the concatenation of the client's ML-KEM-768 encapsulation key
and the client's X25519 ephemeral share.

Choose a reason for hiding this comment

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

Suggested change
and the client's X25519 ephemeral share.
and the client's X25519 ephemeral share, in that order.

This finally is the definition of the group, though one could also state that the hybrid algorithm is an ordered combination of ML-KEM-768 and X25519 and the wire order is then, in accordance with hybrid-design, ML-KEM first, X25519 second.

@@ -241,7 +241,7 @@ ratified by NIST, version of ML-KEM which is specified in {{FIPS203}}.
: This document

Comment:
: Combining X25519 ECDH with ML-KEM-768
: Combining ML-KEM-768 with X25519 ECDH

Choose a reason for hiding this comment

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

This part is justified, since it is reaffirming the order of the components that the group represents as identified by the group name.

@@ -275,6 +275,7 @@ This document obsoletes 25497 and 25498 in the TLS Supported Groups registry.
* draft-kwiatkowski-tls-ecdhe-mlkem-03:
* Adds P-384 combined with ML-KEM-1024
* Adds text that describes error-handling and outlines how the client and server must ensure the integrity of the key exchange process.
* Change the name of X25519MLKEM768 to MLKEM768X25519. This is done to align with the requirement described in section 3.2 of draft-ietf-tls-hybrid-design-11.

Choose a reason for hiding this comment

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

Let's not.

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.

10 participants