feat(mldsa): add mldsa#113
Open
james-d-elliott wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a complete implementation of the FIPS 204 ML-DSA signature scheme in Go, supporting parameter sets 44, 65, and 87. It includes internal field arithmetic and polynomial operations, key generation and signing/verification algorithms, comprehensive test infrastructure with known-answer tests, FIPS 140 support stubs, and a public cryptographic API compatible with the standard Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant PK as PrivateKey
participant Signer as Signing<br/>(rejection loop)
participant Field as Field Arithmetic<br/>(NTT, etc.)
participant Hash as SHAKE256
App->>PK: Sign(message, context)
PK->>Hash: μ = H(tr ∥ ctx-len ∥ context ∥ msg)
Hash-->>PK: μ
PK->>Signer: Sign with μ
loop Rejection sampling
Signer->>Hash: y (sample via SHAKE128)
Signer->>Field: w = A·NTT(y)
Field-->>Signer: w₁ coefficients
Signer->>Hash: challenge ch (SHAKE256)
Signer->>Field: z, r₀, ct₀ (poly ops)
Field-->>Signer: computed values
Signer->>Signer: Check bounds & hint count
alt Bounds/hint valid
Signer-->>PK: signature
else Rejection
Signer->>Signer: Continue loop
end
end
PK-->>App: sig bytes
sequenceDiagram
actor App as Application
participant PubKey as PublicKey
participant Verifier as Verification
participant Field as Field Arithmetic
participant Hash as SHAKE256
App->>PubKey: Verify(message, signature, context)
PubKey->>Hash: μ = H(tr ∥ ctx-len ∥ context ∥ msg)
Hash-->>PubKey: μ
PubKey->>Verifier: Decode sig (ch, z, hints)
Verifier->>Field: w = A·NTT(z) - 2^d·ch
Field-->>Verifier: w coefficients
Verifier->>Field: w₁ = apply_hints(w, hints)
Field-->>Verifier: w₁ adjusted
Verifier->>Hash: ch' = H(w₁)
Verifier->>Verifier: Check: ch == ch'?
alt Challenge matches & bounds valid
Verifier-->>App: ✓ Valid
else Challenge mismatch or invalid
Verifier-->>App: ✗ Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
mldsa/mldsacrypto/mldsamu.go (1)
8-13: Minor documentation clarification.The comment "MLDSAMu is a function that produces..." is slightly misleading since
MLDSAMuis a sentinel constant, not a function. Consider rewording to clarify it's a sentinel value that indicates pre-hashed μ input.📝 Suggested documentation improvement
-// MLDSAMu is a function that produces a [pre-hashed μ message representative]. -// It has no implementation, but is used a [crypto.SignerOpts.HashFunc] return -// value for [mldsa.PrivateKey.Sign]. +// MLDSAMu is a sentinel [crypto.Hash] value indicating a [pre-hashed μ message representative]. +// It has no implementation, but is used as a [crypto.SignerOpts.HashFunc] return +// value for [mldsa.PrivateKey.Sign] to signal external pre-hashing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/mldsacrypto/mldsamu.go` around lines 8 - 13, Update the doc comment for the MLDSAMu constant to clarify that MLDSAMu is a sentinel constant (not a function) used to indicate that the input to mldsa.PrivateKey.Sign is a pre-hashed μ message representative; change the wording around "is a function that produces" to explicitly state it is a sentinel value returned as a crypto.SignerOpts.HashFunc to signal pre-hashed μ input and keep the existing link to the RFC.mldsa/internal/fips140/mldsa/semiexpanded.go (1)
152-157: Bit length calculation could differ from encoding.The decode uses
bits.Len(uint(η)*2)to calculate coefficient bit length, whilesemiExpandedPrivKeySizeusesbits.Len(uint(p.η)) + 1. These should be equivalent (sincebits.Len(x*2) == bits.Len(x) + 1for x > 0), but the inconsistent formulation could cause confusion during maintenance.♻️ Suggested consistency improvement
s1 = make([]ringElement, l) for i := range l { - length := n * bits.Len(uint(η)*2) / 8 + ηBitlen := bits.Len(uint(η)) + 1 + length := n * ηBitlen / 8 s1[i], err = bitUnpackSlow(sk[:length], η, η)Apply the same pattern to the s2 decoding loop for consistency with
semiExpandedPrivKeySize.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/internal/fips140/mldsa/semiexpanded.go` around lines 152 - 157, The bit-length calculation in the s1 decoding loop uses bits.Len(uint(η)*2) while semiExpandedPrivKeySize uses bits.Len(uint(p.η)) + 1, causing inconsistency; update the s1 (and mirror the same change in the s2 decoding loop) to compute length using bits.Len(uint(η)) + 1 (matching semiExpandedPrivKeySize) so length := n * (bits.Len(uint(η)) + 1) / 8, then proceed to call bitUnpackSlow(sk[:length], η, η) and adjust sk = sk[length:] as before.mldsa/internal/fips140/mldsa/mldsa_test.go (1)
299-304: Consider clarifying the purpose of the two equality checks.Line 299 uses
Equal()which performs semantic equality (parameter set + constant-time seed comparison per the context snippet), while line 302 uses struct comparison*pub != *priv.PublicKey()for byte-level field equality. A brief comment explaining why both checks are valuable would improve maintainability.📝 Suggested clarification
+ // Equal() checks semantic equality (parameters + seed) if !pub.Equal(priv.PublicKey()) { t.Errorf("Parsed public key not equal to original") } + // Struct comparison ensures all internal fields (raw, tr, t1, etc.) match if *pub != *priv.PublicKey() { t.Errorf("Parsed public key not identical to original") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/internal/fips140/mldsa/mldsa_test.go` around lines 299 - 304, Add a short comment above the two assertions explaining the difference and intent: state that pub.Equal(priv.PublicKey()) checks semantic equality (same parameter set and constant-time seed comparison) while the struct comparison (*pub != *priv.PublicKey()) verifies byte-level/field-for-field identity of the parsed public key, and that both checks are performed to catch subtle differences in representation versus complete structural equality; place this comment near the Equal() and *pub != *priv.PublicKey() assertions in mldsa_test.go to clarify their purposes.mldsa/internal/fips140/mldsa/field.go (1)
377-382: Consider reusing the byte slice to avoid allocations in hot loop.The
make([]byte, 1)inside the loop allocates on each iteration of the outer loop. Moving the allocation outside would reduce GC pressure.♻️ Suggested optimization
+ j := make([]byte, 1) for i := 256 - p.τ; i < 256; i++ { - j := make([]byte, 1) H.Read(j) for j[0] > byte(i) { H.Read(j) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/internal/fips140/mldsa/field.go` around lines 377 - 382, The loop repeatedly allocates a 1-byte slice with make([]byte, 1) causing GC churn; move the allocation out of the for i := 256 - p.τ; i < 256; i++ loop and reuse a single j []byte (e.g. declare j := make([]byte, 1) before the loop) and continue to call H.Read(j) and check j[0] inside the loop; update the code in the same function containing that loop (the block referencing p.τ and H.Read) so the byte slice is reused rather than allocated each iteration.mldsa/internal/fips140/mldsa/field_test.go (1)
359-369: Variable shadows package-level constantq.Line 361 declares
q := big.NewInt(q)which shadows the package-level constantq. While this works due to Go's scoping rules (the constantqis evaluated before the localqis declared), it reduces readability and could cause confusion.♻️ Suggested rename
func TestZetas(t *testing.T) { ζ := big.NewInt(1753) - q := big.NewInt(q) + bigQ := big.NewInt(q) for k, zeta := range zetas { // ζ^BitRev₈(k) mod q - exp := new(big.Int).Exp(ζ, big.NewInt(int64(BitRev8(uint8(k)))), q) + exp := new(big.Int).Exp(ζ, big.NewInt(int64(BitRev8(uint8(k)))), bigQ) got := fieldFromMontgomery(zeta) if big.NewInt(int64(got)).Cmp(exp) != 0 { t.Errorf("zetas[%d] = %v, expected %v", k, got, exp) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/internal/fips140/mldsa/field_test.go` around lines 359 - 369, In TestZetas rename the local variable that shadows the package-level constant q (the line creating q := big.NewInt(q)) to a distinct identifier (e.g., qBig or qInt) and update subsequent uses in that test (the Exp call and comparisons) to use the new name; ensure you reference the package-level q only by name when needed and avoid reusing it for the big.Int variable so TestZetas, fieldFromMontgomery, BitRev8 and the zetas loop remain correct and clear.mldsa/internal/fips140test/mldsa_test.go (1)
98-105: Use the re-parsed public key in the accumulated loop.Line 102 does a full
PublicKeystruct comparison on every iteration, but Line 105 still verifies withdk.PublicKey(). That adds a lot of extra work to the 10k/60M variants and never exercises verification against the parsed key. Compare semantically and passpubtoVerify.Possible cleanup
- if *pub != *dk.PublicKey() { + if !pub.Equal(dk.PublicKey()) { t.Fatalf("public key mismatch") } - if err := Verify(dk.PublicKey(), msg, sig, ""); err != nil { + if err := Verify(pub, msg, sig, ""); err != nil { t.Fatalf("Verify: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mldsa/internal/fips140test/mldsa_test.go` around lines 98 - 105, The test re-parses the public key into pub via newPublicKey(pk) but still calls Verify with dk.PublicKey(), which wastes work and doesn't exercise verification against the parsed key; change the Verify call to use the re-parsed pub (and update the semantic comparison accordingly) so the loop both compares pub to dk.PublicKey() semantically and passes pub into Verify (use newPublicKey, pub, dk.PublicKey(), Verify, msg, sig to locate the spots).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mldsa/internal/fips140/mldsa/mldsa.go`:
- Around line 341-350: The Sign and SignExternalMu functions currently ignore
errors from drbg.Read causing a zeroed "random" to be used; update both Sign and
SignExternalMu to capture the error returned by drbg.Read(random[:]) and return
nil, err when non-nil instead of proceeding, so no signature is produced on RNG
failure; locate calls to drbg.Read in signInternal invocation sites (Sign and
SignExternalMu) and add the error check/early return, and consider applying the
same pattern to GenerateKey44, GenerateKey65, GenerateKey87 and the seed
initialization in semiexpanded.go where drbg.Read is used.
In `@mldsa/mldsa.go`:
- Around line 81-91: The functions GenerateKey, NewPrivateKey, and NewPublicKey
currently match on pointer identity (e.g., comparing params to
mldsa44/mldsa65/mldsa87), which rejects callers who pass a copied Parameters
value; change the canonicalization to match by value or by an explicit internal
identifier: either add an unexported ID field to Parameters (e.g., params.id)
and switch on params.id in GenerateKey/NewPrivateKey/NewPublicKey, or
dereference and switch on the struct value (e.g., switch *params) comparing
against the canonical parameter values instead of pointer addresses; update all
referenced switches (including the ones noted around lines 97-109 and 198-210)
to use the chosen approach so copied Parameters are accepted.
- Around line 127-133: PrivateKey.Equal currently compares only seed bytes which
can make keys from different ML-DSA parameter sets equal; update
PrivateKey.Equal to nil-guard both wrappers and delegate comparison to the
underlying/internal key comparison (e.g., the internal key value or method on
the wrapped key rather than Bytes()), and ensure the comparison includes the
parameter set (so keys with same seed but different parameter sets are not
equal). Locate the PrivateKey.Equal method and replace the Bytes() based compare
with a nil-check on sk and other and a call to the internal key's equality
method (or compare both the parameter-set identifier and internal key material)
to perform the equality test.
- Around line 155-160: In Sign and SignDeterministic, avoid dereferencing a
potentially typed-nil *Options returned by the type assertion; after doing
`opts, ok := opts.(*Options)` check both ok and that opts != nil before
accessing opts.Context (and similarly for any other fields), and apply the same
nil-check pattern to the other occurrence so a typed nil options value doesn't
cause a panic when reading Context or calling HashFunc.
---
Nitpick comments:
In `@mldsa/internal/fips140/mldsa/field_test.go`:
- Around line 359-369: In TestZetas rename the local variable that shadows the
package-level constant q (the line creating q := big.NewInt(q)) to a distinct
identifier (e.g., qBig or qInt) and update subsequent uses in that test (the Exp
call and comparisons) to use the new name; ensure you reference the
package-level q only by name when needed and avoid reusing it for the big.Int
variable so TestZetas, fieldFromMontgomery, BitRev8 and the zetas loop remain
correct and clear.
In `@mldsa/internal/fips140/mldsa/field.go`:
- Around line 377-382: The loop repeatedly allocates a 1-byte slice with
make([]byte, 1) causing GC churn; move the allocation out of the for i := 256 -
p.τ; i < 256; i++ loop and reuse a single j []byte (e.g. declare j :=
make([]byte, 1) before the loop) and continue to call H.Read(j) and check j[0]
inside the loop; update the code in the same function containing that loop (the
block referencing p.τ and H.Read) so the byte slice is reused rather than
allocated each iteration.
In `@mldsa/internal/fips140/mldsa/mldsa_test.go`:
- Around line 299-304: Add a short comment above the two assertions explaining
the difference and intent: state that pub.Equal(priv.PublicKey()) checks
semantic equality (same parameter set and constant-time seed comparison) while
the struct comparison (*pub != *priv.PublicKey()) verifies
byte-level/field-for-field identity of the parsed public key, and that both
checks are performed to catch subtle differences in representation versus
complete structural equality; place this comment near the Equal() and *pub !=
*priv.PublicKey() assertions in mldsa_test.go to clarify their purposes.
In `@mldsa/internal/fips140/mldsa/semiexpanded.go`:
- Around line 152-157: The bit-length calculation in the s1 decoding loop uses
bits.Len(uint(η)*2) while semiExpandedPrivKeySize uses bits.Len(uint(p.η)) + 1,
causing inconsistency; update the s1 (and mirror the same change in the s2
decoding loop) to compute length using bits.Len(uint(η)) + 1 (matching
semiExpandedPrivKeySize) so length := n * (bits.Len(uint(η)) + 1) / 8, then
proceed to call bitUnpackSlow(sk[:length], η, η) and adjust sk = sk[length:] as
before.
In `@mldsa/internal/fips140test/mldsa_test.go`:
- Around line 98-105: The test re-parses the public key into pub via
newPublicKey(pk) but still calls Verify with dk.PublicKey(), which wastes work
and doesn't exercise verification against the parsed key; change the Verify call
to use the re-parsed pub (and update the semantic comparison accordingly) so the
loop both compares pub to dk.PublicKey() semantically and passes pub into Verify
(use newPublicKey, pub, dk.PublicKey(), Verify, msg, sig to locate the spots).
In `@mldsa/mldsacrypto/mldsamu.go`:
- Around line 8-13: Update the doc comment for the MLDSAMu constant to clarify
that MLDSAMu is a sentinel constant (not a function) used to indicate that the
input to mldsa.PrivateKey.Sign is a pre-hashed μ message representative; change
the wording around "is a function that produces" to explicitly state it is a
sentinel value returned as a crypto.SignerOpts.HashFunc to signal pre-hashed μ
input and keep the existing link to the RFC.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb50ee9f-a8f0-43a6-b03b-93712cd9a7e8
📒 Files selected for processing (14)
mldsa/internal/byteorder/byteorder.gomldsa/internal/constanttime/constant_time.gomldsa/internal/cryptotest/stubs.gomldsa/internal/fips140/mldsa/field.gomldsa/internal/fips140/mldsa/field_test.gomldsa/internal/fips140/mldsa/mldsa.gomldsa/internal/fips140/mldsa/mldsa_test.gomldsa/internal/fips140/mldsa/semiexpanded.gomldsa/internal/fips140/mldsa/stubs.gomldsa/internal/fips140/stubs.gomldsa/internal/fips140test/mldsa_test.gomldsa/mldsa.gomldsa/mldsa_test.gomldsa/mldsacrypto/mldsamu.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
crypto.Signerinterface compatibility.