-
Notifications
You must be signed in to change notification settings - Fork 113
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
Conversation
@bifurcation sure. Here's an example PR to add more sizes: RustCrypto/hybrid-array#104 Note that the |
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 |
Thanksgiving update 🦃: Now passing ACVP tests, working on cleanup. |
@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? |
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.
@bifurcation new sizes released in |
OK, this is ready for review. @tarcieri looks like the SLH-DSA build is broken at the moment. |
@bifurcation |
@tarcieri - CI is green. What do you think about review? |
@bifurcation sorry, have a huge backlog of PRs and haven't been reviewing as much lately. Will try to look this weekend |
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]>
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> { |
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.
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
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.
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.
@bifurcation regarding extracting a shared |
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:
@tarcieri - This is currently blocked on
typenum
not having enough integers. Specifically, there is noArraySize
implementation forU4896
, which the compiler says is necessary forMlDsa87
. 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:
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.