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

Reverse TLS hybrid keyshares for x25519/x448-mlkem hybrids #524

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

bhess
Copy link
Member

@bhess bhess commented Sep 19, 2024

Open point: working encoder for the "reversed" hybrid kems. @baentsch is this still be needed after #522 lands?

Fixes #503

Signed-off-by: Basil Hess <bhe@zurich.ibm.com>
Signed-off-by: Basil Hess <bhe@zurich.ibm.com>
Signed-off-by: Basil Hess <bhe@zurich.ibm.com>
Signed-off-by: Basil Hess <bhe@zurich.ibm.com>
@pi-314159
Copy link
Member

Tested with Google-BoringSSL's X25519MLKEM768 and passed :)

@baentsch
Copy link
Member

Thanks for the PR, @bhess! Please let me know when you consider it Ready for Review.

Interop with x25519_kyber768

I don't understand: What did you test? That the key share reversal works using Kyber768, not MLKEM768? Using which code point?

Open point: working encoder for the "reversed" hybrid kems. @baentsch is this still be needed after #522 lands?

What reading do you think #522 has on this, @bhess? The changes this PR does apply (with most externally visible relevance) to KEMs with OIDs (well, really primarily x25519mlkem768), right? #522 in turn changes the handling of KEMs without OIDs.

Tested with Google-BoringSSL's X25519MLKEM768 and passed :)

Nice, indeed. Thanks for the confirmation @pi-314159 . Just the names don't seem to align yet: Do you plan on aligning this, too @bhess?

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Finally, a general comment: On the positive side the approach taken in the PR avoids encoding specific algorithms' names into the provider, so good; I'm not convinced this added configuration option doesn't complicate maintainability of the code, though. Looking at the changes, this looks "brittle": Are you comfortable maintaining/supporting this going forward, @bhess? If so shouldn't there be some documentation explaining how this works? Might it not be better to simply change the order for all X-hybrids (as per suggestion by @pi-314159 I think?)?

@pi-314159
Copy link
Member

@baentsch I asked @bhess earlier about changing the order of all X-hybrids.
In this case, we need to change about 20 codepoints in generate.yml and modify oqs_kmgmt.c/keymgmt_functions.fragment.
Changing names may not be necessary since current names are consistent with other hybrids supported by the oqs-provider.

@baentsch
Copy link
Member

@baentsch I asked @bhess earlier about changing the order of all X-hybrids. In this case, we need to change about 20 codepoints in generate.yml and modify oqs_kmgmt.c/keymgmt_functions.fragment.

Changing 20 ids one-time doesn't sound that hard compared to having to explain and understand time and again what's going on in which cases and which code paths in hybrid vs composite vs plain PQ.

If oqsprovider remains a research vehicle, no problem -- that type of user can read the code and help themselves. But there's quite a few people that try to re-label this software as "production ready": Problem there: most of those folks have never contributed meaningfully or at all to OQS so don't know what their decisions practically imply. I'd like to ensure that the code in oqsprovider also caters to such case, i.e., that the (then growing) support obligations on the community should not be getting unmanageable (btw, there is precedent to the TAC disregarding community concerns -- and it is exactly in the area of declaring software of high quality).

Changing names may not be necessary since current names are consistent with other hybrids supported by the oqs-provider.

It surely may not be necessary. Taking the perspective of a user, wouldn't that be more consistent (across implementions and with spec), though?

@ghen2
Copy link

ghen2 commented Sep 19, 2024

Tested this x25519_mlkem768 with Firefox 132 nightly and they are interoperable.
(while Firefox 130 continues to work with x25519_kyber768 on the same oqs-provider)

@bhess
Copy link
Member Author

bhess commented Sep 19, 2024

Might it not be better to simply change the order for all X-hybrids (as per suggestion by @pi-314159 I think?)?

@baentsch I asked @bhess earlier about changing the order of all X-hybrids.
In this case, we need to change about 20 codepoints in generate.yml and modify

I agree that changing all x-hybrids would be a bit simpler. Changing the codepoints would also be ok IMO.
However, we would also lose interop with Chrome&Cloudflare's (edit: and Firefox) x25519_kyber768 which will still be around for a while, as the order there is not reversed. Don't we want to keep that at least until Cloudflare and Chrome supports x25519_mlkem768? (Chrome plans to do the switch to mlkem in version 131 November).
The current implementation basically implements the logic employed in draft-kwiatkowski-tls-ecdhe-mlkem.

I don't understand: What did you test? That the key share reversal works using Kyber768, not MLKEM768? Using which code point?

No, I just added the commented-out interop test to Cloudflare again with x25519_kyber768.

Just the names don't seem to align yet: Do you plan on aligning this, too @bhess?

If we want to do this, yes. It may take me 1-2 days since I'm traveling this week. @praveksharma kindly offered help with the templating changes needed. Thanks Pravek! Feel free to give it a go. My idea was to add a property like "standard_name" to the .yml file, and to use this as name if available.
The other open point is getting the encoder for the reversed orders working again.

@bhess
Copy link
Member Author

bhess commented Sep 19, 2024

Thanks for testing with Firefox and BoringSSL @ghen2 & @pi-314159!

@ghen2
Copy link

ghen2 commented Sep 19, 2024

However, we would also lose interop with Chrome&Cloudflare's (edit: and Firefox) x25519_kyber768 which will still be around for a while, as the order there is not reversed. Don't we want to keep that at least until Cloudflare and Chrome supports x25519_mlkem768? (Chrome plans to do the switch to mlkem in version 131 November).

FYI, Firefox 132 with x25519_mlkem768 will be released on October 29, and has it enabled by default for HTTP/2 (but they seem to have broken the knob for HTTP/3).

@pi-314159
Copy link
Member

pi-314159 commented Sep 19, 2024

@bhess @baentsch I don't think it's a big problem

  1. this is currenly for research only.
  2. Chrome and Firefox will support x25519mlkem768 within two months, so we can switch early.
  3. Since x25519_kyber768 will be dropped soon, I don't see the necessity to interop with Cloudflare's x25519_kyber768.

@bhess
Copy link
Member Author

bhess commented Sep 19, 2024

@bhess @baentsch I don't think it's a big problem

  1. this is currenly for research only.
  2. Chrome and Firefox will support x25519mlkem768 within two months, so we can switch early.
  3. Since x25519_kyber768 will be dropped soon, I don't see the necessity to interop with Cloudflare's x25519_kyber768.

So why don't we do the switch (reversing all x-hybrids) in two months, i.e. in the oqs-provider release after this one? I think it doesn't hurt and is a plus to have interop of what is used now in practice. @pi-314159 I think it's not a problem if you do this differently in oqs-BoringSSL and drop interop now.

Are you comfortable maintaining/supporting this going forward, @bhess?

Sure and I'll also be happy to drop this logic when it is not used anymore. The change required is minimal.

Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
@baentsch
Copy link
Member

just added the commented-out interop test to Cloudflare again with x25519_kyber768.

At a different code point, though. So CF simply switched to "our" code point for x22519_kyber768 it seems but then works towards implementing MLKEM....

to add a property like "standard_name" to the .yml file

That'd also be my favourite to keep key information in one place.

@praveksharma kindly offered help with the templating changes needed

Thanks also from me @praveksharma ! Any and all additional help with this project welcome!

Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
Signed-off-by: Pravek Sharma <sharmapravek@gmail.com>
@ghen2
Copy link

ghen2 commented Sep 20, 2024

The next revision of draft-kwiatkowski-tls-ecdhe-mlkem will also standardise p384_mlkem1024 as SecP384r1MLKEM1024 with id 0x11ED.

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.

Implement new ML-KEM hybrid key exchange in TLS
5 participants