Prepare for v5.9.1 release#126
Conversation
In the newer wolfSSL signing and verifying without context is not available unless it is explicitly enabled. This change modifies the Python binding and test suite to accommodate this.
Smaller authentication tags may not be supported by the library. This fix makes the test work for the default case that tags should be minimum 12 bytes in size.
Read minimum supported tag size from settings and make the test less ambiguous.
Also detect WOLFSSL_DILITHIUM_FIPS204_DRAFT as implying no-ctx support, mirroring the logic in dilithium.h. Add ML_DSA_NO_CTX to the default features dict for consistency with peer flags.
mjdemilliano
left a comment
mjdemilliano
left a comment
There was a problem hiding this comment.
Processed all comments
|
Would like to include #129 , so marking as draft for now |
| # Valid NIST sizes: verify the resulting tag has the requested length. | ||
| for good in (4, 8, 12, 13, 14, 15, 16): | ||
| if good < _lib.MIN_AUTH_TAG_SZ: | ||
| continue |
There was a problem hiding this comment.
Just skipping the size like this loses coverage. In the previous review all these sizes were tested, with an error expected for some. I'd like to preserve that - but expect a general error for sizes below the threshold rather than a literal numeric code.
| @@ -540,6 +547,7 @@ def build_ffi(local_wolfssl, features): | |||
| extern int ML_DSA_ENABLED; | |||
| extern int ML_DSA_NO_CTX; | |||
There was a problem hiding this comment.
INFO-2: Boolean flag ML_DSA_NO_CTX omits the _ENABLED suffix used by sibling flags
- File:
scripts/build_ffi.py:506,548 - Function:
build_ffi - Action: NIT
- Tag: convention
- Confidence: Medium
Description: ML_DSA_NO_CTX is a 0/1 boolean capability flag exposed via _lib, but every other boolean capability flag in this list uses the _ENABLED suffix (ML_DSA_ENABLED, ML_KEM_ENABLED, AESGCM_STREAM_ENABLED, WC_RNG_SEED_CB_ENABLED, etc.). The new flag breaks that pattern. (MIN_AUTH_TAG_SZ is a numeric value, so it correctly does not take the suffix.)
Code:
int ML_DSA_NO_CTX = {features["ML_DSA_NO_CTX"]};
...
extern int ML_DSA_NO_CTX;
Recommendation: Optional rename for convention consistency; not blocking.
| assert not mldsa_pub.verify(signature, message, ctx=wrong_ctx) | ||
|
|
||
| @pytest.mark.skipif(not _lib.ML_DSA_NO_CTX, reason="Requires support for signing without context") | ||
| def test_sign_with_seed(mldsa_type, rng): |
There was a problem hiding this comment.
MEDIUM-1: sign_with_seed determinism coverage lost on default (non-NO_CTX) builds [SUGGEST] (test)
File: tests/test_mldsa.py:171-199
Function: test_sign_with_seed
Confidence: Medium
The diff adds @pytest.mark.skipif(not _lib.ML_DSA_NO_CTX, ...) to test_sign_with_seed. Confirmed from lib/wolfssl/configure.ac:1822-1823, WOLFSSL_DILITHIUM_NO_CTX is only defined for --enable-dilithium=no-ctx or the FIPS204 draft option; the plain --enable-dilithium used by scripts/build_ffi.py:239 does NOT define it. So on the default wolfcrypt-py build _lib.ML_DSA_NO_CTX == 0 and this entire test is skipped. The skipped test is the ONLY place that asserts deterministic signing (sign_with_seed(message, seed) twice → identical signatures, lines 190-191) and the only place testing the seed-length/seed-type validation (lines 194-199). The sibling test_sign_with_seed_and_context (not skipped) signs once with ctx= but never re-signs to assert determinism, so determinism of sign_with_seed — a core property of the newly-added deterministic-signing feature — has zero coverage on the most common build configuration. Determinism does not actually require no-context support; it could be re-asserted by passing a ctx= value.
Code:
@pytest.mark.skipif(not _lib.ML_DSA_NO_CTX, reason="Requires support for signing without context")
def test_sign_with_seed(mldsa_type, rng):
...
signature = mldsa_priv.sign_with_seed(message, signature_seed)
...
# re-generate from the same seed:
signature_from_same_seed = mldsa_priv.sign_with_seed(message, signature_seed)
assert signature == signature_from_same_seed
Recommendation: Add a determinism assertion (re-sign with the same seed and compare) to test_sign_with_seed_and_context, which already runs on default builds, so the deterministic-signing guarantee is exercised regardless of NO_CTX support.
| pub_key3 = mldsa_pub.encode_key() | ||
| assert pub_key == pub_key3 | ||
|
|
||
| def test_sign_verify(mldsa_type, rng): |
There was a problem hiding this comment.
MEDIUM-1: No test coverage for the newly-documented empty-context (ctx=b"") ML-DSA path [SUGGEST] (test)
File: tests/test_mldsa.py:146-169
Function: test_sign_verify
Confidence: Medium
This diff simultaneously (a) disables the ctx=None no-context path on default builds (the new elif not _lib.ML_DSA_NO_CTX: raise WolfCryptError(...) guards in sign/sign_with_seed/verify) and (b) updates the docstrings to instruct users to pass empty string "" for FIPS-204 empty-context signing/verification. On the standard --enable-dilithium build (where _lib.ML_DSA_NO_CTX == 0), the empty-context call form is now the documented replacement for the disabled None default. However, no test exercises it. The existing tests only cover non-empty ctx=ctx, ctx=wrong_ctx, and (for the raise path) ctx omitted. The empty-context boundary (ctx=b"" → t2b("") → b"" → wc_dilithium_sign_ctx_msg(..., ctxLen=0, ...)) is precisely the FIPS-204 path the new docs steer default-build users toward, yet it is never executed or round-trip verified. A regression in the len==0 ctx handling (or a wolfSSL change to it) would pass CI silently.
Code:
# Sign a message with context
signature = mldsa_priv.sign(message, rng, ctx=ctx)
...
if not _lib.ML_DSA_NO_CTX:
with pytest.raises(WolfCryptError):
mldsa_priv.sign(message)
...
# Verify with wrong context
assert not mldsa_pub.verify(signature, message, ctx=wrong_ctx)
Recommendation: Add an explicit empty-context (ctx=b"") sign+verify round-trip assertion to test_sign_verify so the documented FIPS-204 empty-context behavior — the de-facto default usage on standard builds after this change — is actually exercised.
3 participants
No description provided.