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

PQC: Add hybrid groups x25519/ML-KEM-768 and secp256r1/ML-KEM-768 #4375

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Oct 15, 2024

Pull Request Dependencies

Description

Those groups are described in draft-kwiatkowski-tls-ecdhe-mlkem and the code points are registered by IANA.

@reneme reneme added this to the Botan 3.6.0 milestone Oct 15, 2024
@reneme reneme requested a review from randombit October 15, 2024 15:55
@reneme reneme self-assigned this Oct 15, 2024
@coveralls
Copy link

coveralls commented Oct 15, 2024

Coverage Status

coverage: 91.133% (-0.007%) from 91.14%
when pulling 59abd7a on Rohde-Schwarz:tls/ml_kem_hybrids
into 3a62c9a on randombit:master.

@reneme reneme mentioned this pull request Oct 15, 2024
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

🚀

@reneme
Copy link
Collaborator Author

reneme commented Oct 16, 2024

Thanks for the reviews. I'm holding back on merging that because I'd like to update bogo (which I believe has test cases for those groups already).

@reneme
Copy link
Collaborator Author

reneme commented Oct 17, 2024

Enabling relevant BoGo tests revealed something that we might want to have a look at here (or independently):

  FAILED (CurveTest-Invalid-MLKEMEncapKeyNotReduced-Server-MLKEM-TLS13)
  bad error (wanted ":BAD_ECPOINT:" / "remote error: illegal parameter"): local error "EOF", child error "signal: aborted (core dumped)", stdout:
  
  stderr:
  False assertion mapped_coeff <= range in unpack @/home/runner/work/botan/botan/build/build/include/internal/botan/internal/pqcrystals_encoding.h:206

Those groups are described in draft-kwiatkowski-tls-ecdhe-mlkem and
the code points are officially provided by IANA. Therefore they
can be seen as 'fit for production use'.
Comment on lines 1008 to 1014
// TODO: once `TLS::Policy::key_exchange_groups()` contains it by
// default, remove this explicit check.
if(group == Botan::TLS::Group_Params::HYBRID_X25519_KYBER_768_R3_OQS) {
//
// See: https://github.com/randombit/botan/pull/4305
if(group == Botan::TLS::Group_Params::HYBRID_X25519_ML_KEM_768) {
groups.push_back(group);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See also: #4305 (comment)

@reneme
Copy link
Collaborator Author

reneme commented Oct 17, 2024

Should be ready for merge now.

@reneme reneme requested a review from randombit October 18, 2024 14:37
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.

4 participants