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

Add ML-KEM / FIPS203 final #1899

Merged
merged 4 commits into from
Aug 27, 2024
Merged

Add ML-KEM / FIPS203 final #1899

merged 4 commits into from
Aug 27, 2024

Conversation

bhess
Copy link
Member

@bhess bhess commented Aug 20, 2024

Adds ML-KEM from FIPS203

  • Pulls ML-KEM from pq-crystals/kyber upstream: https://github.com/pq-crystals/kyber/tree/standard
  • Pulls ACVP test vectors from NIST repository. A script (fetch_values.sh) that downloads the internalProjection.json with all test values is included in the PR.
  • ACVP for ML-KEM is defined here. This PR supports testing "ML-KEM / keyGen / * AFT", "ML-KEM / encapDecap / * AFT" and "ML-KEM /encapDecap / * VAL".
  • ACVP vectors for ML-KEM pass

The PR removes the previous ML-KEM-ipd implementation, as initially suggested in #1891.
Technically it would be possible to support both ML-KEM and ML-KEM-ipd (e.g. to give potential users of -ipd the possibility to phase it out over the next release).

TODOs:

  • review / update constant-time tests
  • review documentation
  • decide if ML-KEM-ipd should be retained or not.

Partially-fixes #1891.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@baentsch
Copy link
Member

Even though just a draft, allow me to comment that this LGTM:

  • Documentation seems OK/updated; what additional review do you deem necessary?
  • I'd vote to retain this PR as-is, i.e., remove -ipd; if someone wants it back, they could/should state this; it's been present for but one release and at least oqsprovider didn't expose it and we provided the alias to discourage people to use the -ipd moniker to boot: Are we aware of anyone having disregarded that?
  • regarding CT -- why not simply run it and see whether anything needs changing at all? Is the code base so different from -ipd (apart from the re-name changing many file names)?

--> This leads me to suggest including this PR also already in the next release. Any reason to wait?

Add ACVP vectors for ML-KEM

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]>
@bhess
Copy link
Member Author

bhess commented Aug 27, 2024

Thanks for the feedback @baentsch . The PR is conceptually ready - docs are updated and ct-tests pass, so moving it to "ready for review".

In the meanwhile, TLS hybrid code points for ML-KEM are available (open-quantum-safe/oqs-provider#503) as well as NIST OIDs (https://csrc.nist.gov/projects/computer-security-objects-register/algorithm-registration), so it should be ok to include this in a release.

As far as I've seen the ML-KEM changes from -ipd to final are minor: Encaps/Decaps didn't change, keygen had one change in the initial seed (one byte is added to the seed). I wouldn't see a big issue to just removing the -ipd variant given this minor change.

@bhess bhess marked this pull request as ready for review August 27, 2024 11:34
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 the integration. Looking over it, it indeed mostly seems like a removal of the "-ipd" moniker -- making downstream integration (at least in oqsprovider) seamless. Thus OK to merge even without tracker PR there (basically, it'll just be an OID update if I'm not mistaken, right, @bhess ?).

@dstebila
Copy link
Member

Thanks @bhess! This is really important work, great to have it completed.

I haven't heard any use of -ipd, so I'm personally in favour of dropping it immediately. If we wanted to be thorough, we could make a discussion forum post asking for feedback on that, send an email to our mailing lists pointing people to that, and give it say one week, then make a decision based on that.

@bhess bhess merged commit dc4deaa into main Aug 27, 2024
46 checks passed
@bhess bhess deleted the bhe-fips203-final branch August 27, 2024 17:25
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.

ML-DSA: integrate final standard
3 participants