-
Notifications
You must be signed in to change notification settings - Fork 95
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
Conversation
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Signed-off-by: Basil Hess <[email protected]>
Tested with Google-BoringSSL's X25519MLKEM768 and passed :) |
Thanks for the PR, @bhess! Please let me know when you consider it Ready for Review.
I don't understand: What did you test? That the key share reversal works using Kyber768, not MLKEM768? Using which code point?
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.
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? |
There was a problem hiding this 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?)?
@baentsch I asked @bhess earlier about changing the order of all X-hybrids. |
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
It surely may not be necessary. Taking the perspective of a user, wouldn't that be more consistent (across implementions and with spec), though? |
Tested this x25519_mlkem768 with Firefox 132 nightly and they are interoperable. |
I agree that changing all x-hybrids would be a bit simpler. Changing the codepoints would also be ok IMO.
No, I just added the commented-out interop test to Cloudflare again with x25519_kyber768.
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. |
Thanks for testing with Firefox and BoringSSL @ghen2 & @pi-314159! |
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). |
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.
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 <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
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....
That'd also be my favourite to keep key information in one place.
Thanks also from me @praveksharma ! Any and all additional help with this project welcome! |
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
Signed-off-by: Pravek Sharma <[email protected]>
The next revision of draft-kwiatkowski-tls-ecdhe-mlkem will also standardise p384_mlkem1024 as |
Signed-off-by: Basil Hess <[email protected]>
I don't think I changed the code point for x25519_kyber768. It was 0x6399 before and still is. |
The encoders are updated to be able to reverse the key shares. Thanks @praveksharma for adding the "standard names" X25519MLKEM768 and SecP256r1MLKEM768. |
Signed-off-by: Basil Hess <[email protected]>
Thanks for all these updates, @bhess & @praveksharma ! Question to @feventura : Would you have time to check this statement for composites?
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?). |
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. |
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]>
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). |
Just mentioning that I've tested this PR out of interest, |
Signed-off-by: Basil Hess <[email protected]>
That's great, thanks! Added interop tests to google.com with x25519_kyber768 and X25519MLKEM768 in c4dcd1c. |
There was a problem hiding this 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.
fips_standard
to kems in generate.yml. If this is set, the key shares in x25519/x448 hybrids are reversed.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