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

Merged
merged 17 commits into from
Oct 2, 2024

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?

Update (2 Oct): adds interop tests to google.com with x25519_kyber768 and X25519MLKEM768

Fixes #503

@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.

@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!

@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.

Signed-off-by: Basil Hess <[email protected]>
@bhess
Copy link
Member Author

bhess commented Sep 23, 2024

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....

I don't think I changed the code point for x25519_kyber768. It was 0x6399 before and still is.
There was a codepoint as env variable defined for x25519_kyber512 which I removed, because Cloudflare discontinued support.

@bhess bhess marked this pull request as ready for review September 23, 2024 09:57
@bhess bhess requested a review from feventura as a code owner September 23, 2024 09:57
@bhess
Copy link
Member Author

bhess commented Sep 23, 2024

The encoders are updated to be able to reverse the key shares. Thanks @praveksharma for adding the "standard names" X25519MLKEM768 and SecP256r1MLKEM768.

@baentsch
Copy link
Member

Thanks for all these updates, @bhess & @praveksharma !

Question to @feventura : Would you have time to check this statement for composites?

Tried to leave the code for hybrid/composite signatures untouched as good as possible

As far as the hybrid sigs are concerned, I'd argue that (continued) proper operation in the IETF PQCerts hackathon would be a good indication everything's (still) OK. @praveksharma : Are we still in contact with that team or even better, have scripting to validate all is fine?

Given how convoluted all this "keyslot logic" is (Yes, I know you said so @thb-sb...), I'd also feel much better if we had a test server to test this against (in CI). For now, the manual checks done by @ghen2 & @pi-314159 are arguably good enough. As discussed yesterday, we should be getting back to where we've been in this regard (worthwhile creating an issue if this can't be brought back in this PR, e.g., by way of testing with boringssl as discussed?).

@bhess
Copy link
Member Author

bhess commented Sep 25, 2024

Given how convoluted all this "keyslot logic" is (Yes, I know you said so @thb-sb...), I'd also feel much better if we had a test server to test this against (in CI). For now, the manual checks done by @ghen2 & @pi-314159 are arguably good enough. As discussed yesterday, we should be getting back to where we've been in this regard (worthwhile creating an issue if this can't be brought back in this PR, e.g., by way of testing with boringssl as discussed?).

Fully agree. I found the code in oqsprov_keys.c especially difficult to follow and spent quite some time for a seemingly simple task like reversing the key share. It's mixing logic between KEMs, signatures, hybrids and composites, sometimes encoding key sizes in the key structure, sometimes using keyslots and sometimes not. Better structuring this code would be helpful, but a major task.

@baentsch
Copy link
Member

Better structuring this code would be helpful, but a major task.

Completely agree; see e.g. #375 #483

SWilson4 added a commit that referenced this pull request Sep 26, 2024
commit 77214a4
Author: Basil Hess <[email protected]>
Date:   Wed Sep 25 09:40:31 2024 +0200

    More explicit logic for share reversal in oqsnames.fragment

    Signed-off-by: Basil Hess <[email protected]>

commit 1572ebb
Author: Basil Hess <[email protected]>
Date:   Mon Sep 23 11:31:03 2024 +0200

    fixing encoders

    Signed-off-by: Basil Hess <[email protected]>

commit 965f235
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 20:37:13 2024 +0200

    Cast void* to char* to satisy MSVC

    Signed-off-by: Pravek Sharma <[email protected]>

commit 72d1e37
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 19:36:43 2024 +0200

    Fixed formatting

    Signed-off-by: Pravek Sharma <[email protected]>

commit 0e8346b
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 19:19:50 2024 +0200

    Ran ./scripts/format_code.sh

    Signed-off-by: Pravek Sharma <[email protected]>

commit 83db8a6
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 19:15:31 2024 +0200

    Hacked hybrid logic to work with new name SecP256r1MLKEM768

    Signed-off-by: Pravek Sharma <[email protected]>

commit ad03863
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 18:40:06 2024 +0200

    Ran ./oqs-template/generate.sh

    Signed-off-by: Pravek Sharma <[email protected]>

commit 46e1aad
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 18:38:29 2024 +0200

    Updated templates to change p256_mlkem768 to SecP256r1MLKEM768

    Signed-off-by: Pravek Sharma <[email protected]>

commit e255f59
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 03:18:28 2024 +0200

    Hacked hybrid logic to work with new name X25519MLKEM768

    Signed-off-by: Pravek Sharma <[email protected]>

commit 806eb59
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 03:12:11 2024 +0200

    Changed leftover instances of x25519_mlkem768 manually

    Signed-off-by: Pravek Sharma <[email protected]>

commit ae5251b
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 03:11:32 2024 +0200

    Ran ./oqs-template/generate.sh

    Signed-off-by: Pravek Sharma <[email protected]>

commit 3f8eadc
Author: Pravek Sharma <[email protected]>
Date:   Fri Sep 20 03:09:30 2024 +0200

    Updated templates to change x25519_mlkem768 to X25519MLKEM768

    Signed-off-by: Pravek Sharma <[email protected]>

commit 4bf9c35
Author: Basil Hess <[email protected]>
Date:   Wed Sep 18 23:41:56 2024 -0700

    interop onlu with x25519_kyber768

    Signed-off-by: Basil Hess <[email protected]>

commit 78e34ff
Author: Basil Hess <[email protected]>
Date:   Wed Sep 18 23:27:47 2024 -0700

    Update x25519_mlkem768 code point

    Signed-off-by: Basil Hess <[email protected]>

commit 95e3ab2
Author: Basil Hess <[email protected]>
Date:   Wed Sep 18 22:02:11 2024 -0700

    code-format

    Signed-off-by: Basil Hess <[email protected]>

commit a6f2adc
Author: Basil Hess <[email protected]>
Date:   Wed Sep 18 21:59:03 2024 -0700

    Share reversal for x25519_mlkem*

    Signed-off-by: Basil Hess <[email protected]>
@ghen2
Copy link

ghen2 commented Oct 1, 2024

You can also interop test against www.google.com (or any other google website), which supports both x25519_mlkem768 and x25519_kyber768 at this point (and this branch interoperates with both).

@Avamander
Copy link

Just mentioning that I've tested this PR out of interest, X25519MLKEM768 seems to work nicely with Chrome 131.0.6738.0 (chrome://flags/#use-ml-kem enabled).

@bhess
Copy link
Member Author

bhess commented Oct 2, 2024

You can also interop test against www.google.com (or any other google website), which supports both x25519_mlkem768 and x25519_kyber768 at this point (and this branch interoperates with both).

That's great, thanks! Added interop tests to google.com with x25519_kyber768 and X25519MLKEM768 in c4dcd1c.

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.

Thanks for bringing back interop tests, @bhess ! More work required to make this maintainable but that is for another day. Main thing is it works.

@baentsch baentsch merged commit 98a355f into main Oct 2, 2024
54 checks passed
@praveksharma praveksharma mentioned this pull request Oct 2, 2024
@bhess bhess deleted the bhe-hybridreverse branch October 3, 2024 10:59
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
7 participants