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 support for ML-DSA #877

Merged
merged 39 commits into from
Jan 19, 2025
Merged

Add support for ML-DSA #877

merged 39 commits into from
Jan 19, 2025

Conversation

bifurcation
Copy link
Contributor

@bifurcation bifurcation commented Nov 14, 2024

This PR is a first step toward ML-DSA support, as discussed in #8. It puts in place enough of the basic algebraic and encoding primitives to get key generation going.

Still TODO:

  • More complete unit test coverage (e.g., on encoding routines)
  • Signing
  • Verification
  • Integration tests against NIST ACVP test vectors
  • Performance benchmarks
  • Pull out things that are common with ML-KEM into a shared module (?)

@tarcieri - This is currently blocked on typenum not having enough integers. Specifically, there is no ArraySize implementation for U4896, which the compiler says is necessary for MlDsa87. Can we get that added?

@cothan, @marija-mijailovic - If you guys are still interested in collaborating, making PRs to this branch would be a good way to help.

On the latter point about overlap with ML-KEM: Thinking about this while I was writing this PR, it seems like the main shared stuff would be:

  • Structs for polynomials, polynomial vectors, NTT vectors, and NTT matrices
  • Fixed-width encoding of unsigned integers

Getting those factored out would be a non-trivial savings, but would require some careful introduction of generics. For example, the algebraic stuff would have to be generic over the field over which the polynomials exist (since there are different q moduli in the two algorithms), as well as the details of multiplication of polynomials and NTT.

@tarcieri
Copy link
Member

@bifurcation sure. Here's an example PR to add more sizes: RustCrypto/hybrid-array#104

Note that the uint! macro uses a little endian bit encoding for the size

@bifurcation
Copy link
Contributor Author

Latest push adds signing/verification logic. It can verify its own signatures, but fails the ACVP test vectors.

Note that it will not build against the public version of hybrid-array, since that is missing some ArraySize implementations. I'm planning to submit a PR to that repo once things are in basic working order here, so that we have confidence that we've covered all the necessary integers.

@bifurcation
Copy link
Contributor Author

Thanksgiving update 🦃: Now passing ACVP tests, working on cleanup.

@tarcieri tarcieri mentioned this pull request Dec 2, 2024
10 tasks
@bifurcation
Copy link
Contributor Author

@tarcieri - I think this is about ready for review and merging, though the CI won't succeed until RustCrypto/hybrid-array#108 is merged and there's a new release. What are the next steps here?

tarcieri pushed a commit to RustCrypto/hybrid-array that referenced this pull request Dec 7, 2024
This PR was developed in parallel with RustCrypto/signatures#877,
to provide the array sizes necessary for serializing / deserializing
ML-DSA keys and signatures.

In addition to the end key/signature sizes, some intermediate value sizes
are required.
@tarcieri
Copy link
Member

tarcieri commented Dec 7, 2024

@bifurcation new sizes released in hybrid-array v0.2.3

@bifurcation bifurcation marked this pull request as ready for review December 9, 2024 15:30
@bifurcation
Copy link
Contributor Author

OK, this is ready for review.

@tarcieri looks like the SLH-DSA build is broken at the moment.

@tarcieri
Copy link
Member

@bifurcation slh-dsa build is fixed if you can rebase

@bifurcation
Copy link
Contributor Author

@tarcieri - CI is green. What do you think about review?

@tarcieri
Copy link
Member

@bifurcation sorry, have a huge backlog of PRs and haven't been reviewing as much lately. Will try to look this weekend

ArthurHeymans added a commit to chipsalliance/caliptra-sw that referenced this pull request Dec 19, 2024
The goal of updating the rust library is to be able to use Mldsa from
rustCrypto repo: RustCrypto/signatures#877

The goal of using this code is to replace openssl to generate TBS and
CSR tempaltes. Openssl does not yet support Mldsa

The minimum required rust version is 1.81.

It looks like this toolchain needs a lot more stack than 1.70.

Rust 1.83 warns about unused stuff a lot more often which is addressed
in this commit.

Signed-off-by: Arthur Heymans <[email protected]>
ArthurHeymans added a commit to chipsalliance/caliptra-sw that referenced this pull request Dec 19, 2024
The goal of updating the rust library is to be able to use Mldsa from
rustCrypto repo: RustCrypto/signatures#877

The goal of using this code is to replace openssl to generate TBS and
CSR tempaltes. Openssl does not yet support Mldsa

The minimum required rust version is 1.81.

It looks like this toolchain needs a lot more stack than 1.70.

Rust 1.83 warns about unused stuff a lot more often which is addressed
in this commit.

Signed-off-by: Arthur Heymans <[email protected]>
/// Generate a signing key pair from the specified RNG
// Algorithm 1 ML-DSA.KeyGen()
#[cfg(feature = "rand_core")]
fn key_gen(rng: &mut impl CryptoRngCore) -> KeyPair<P> {
Copy link
Member

Choose a reason for hiding this comment

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

We typically use the names generate* or random for these sorts of methods. See also:

https://docs.rs/crypto-common/latest/crypto_common/trait.KeyInit.html

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Sorry for the belated review, especially because I didn't really have much to say except this implements things the way I would've expected (e.g. using the Signer and RandomizedSigner traits).

I had some notes/bikeshed on keygen API names, but I don't see that as a reason to block merging this and getting an initial implementation landed. We can follow up on that.

@tarcieri tarcieri merged commit 1d3a1d1 into RustCrypto:master Jan 19, 2025
47 checks passed
@tarcieri
Copy link
Member

@bifurcation regarding extracting a shared module_lattice which can be reused between ml-kem and ml-dsa, I don't see that as important enough to block initial releases but a nice-to-have we can follow up on. First we'd have to figure out where to put it (possibly https://github.com/RustCrypto/utils for lack of a better option)

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.

2 participants