-
Notifications
You must be signed in to change notification settings - Fork 146
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-DSA (FIPS204) #480
Add ML-DSA (FIPS204) #480
Conversation
9537079
to
5bd6e0d
Compare
e60db3b
to
47bb92a
Compare
1fbec7a
to
7970d07
Compare
7970d07
to
16fa7b7
Compare
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.
Just minor changes. Overall is ok.
Q: is this compliant with ACVP test files?
SignTo(sk, msg, sig[:]) | ||
return sig[:], nil | ||
{{- if .NIST }} | ||
if err = SignTo(sk, msg, nil, false, ret[:]); err != nil { |
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.
here we are defaulting Sign
to be a deterministic algorithm,
From Sec 3.4:
Only implementing the hedged variant (i.e., without the deterministic variant) is sufficient to guarantee interoperability.
It seems to me that the default should be randomized signatures.
Note that Sign
receives a 'rand io.Reader' variable for providing randomness to the function.
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.
From the Dilithium specification:
Randomised signing is easier to make side channel secure against power/EM leakage in hardware. Thus it makes sense to use it as default in hardware. In software deterministic signing as default seems to make more sense. I don't think it makes a big difference. A non security argument is that the deterministic variant is easier to test.
@@ -16,13 +16,27 @@ import ( | |||
"github.com/cloudflare/circl/sign/ed448" | |||
"github.com/cloudflare/circl/sign/eddilithium2" | |||
"github.com/cloudflare/circl/sign/eddilithium3" | |||
|
|||
dilithium2 "github.com/cloudflare/circl/sign/dilithium/mode2" |
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.
In the past, we've recommend not to use dilithium alone, but used as a hybrid.
Has something changed?
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.
For the web, PQ certificates only add value once a CRQC is close. Once that happens, hybrids don't make sense anymore. Also, the standardisation of hybrids for signatures is quite messy.
Forgot to add tests. Done now. Should be good to merge (and squash.) |
The final version of FIPS204 has not been released, thus this implementation is not yet final.
Remove old mode API and hook Dilithium and ML-DSA into generic signature API. Expose support for randomized ML-DSA mode. (It is not exposed in the generic signature API.) Remove old AES variants of Dilithium. Does not add the HashML-DSA variants (yet).
Fix one bug in key derivation.
Rebased. |
Note There are no complete test vectors for FIPS 204 yet. I propose we'll wait for that before we merge this. (The ACVP test vectors only test ML-DSA.Sign_internal.)Adds ML-DSA aka FIPS 204.
Tests against reference implementation and ACVP test vectors.
Keeps old Dilithium around in case it's used, but removes the AES modes.
Hooks Dilithium and ML-DSA into the generic signatures API. Removes the old Dilithium mode API.
Adds support for the ML-DSA randomised variant, but only via the package API — not generic signature API.
Does not add support for the HashML-DSA variants.