Uh oh!
There was an error while loading. Please reload this page.
Add LMS Support - #459
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds verification-only LMS (RFC 8554 / NIST SP 800-208) support to wolfProvider for OpenSSL 3.6+, including raw XDR public key import/export, an XDR decoder, and CI/test coverage integration alongside existing PQC work (ML-KEM/ML-DSA).
Changes:
- Register LMS keymgmt, signature (verify-only), and XDR decoder implementations in the provider.
- Add LMS unit tests for import/export, XDR decoding, and unsupported operations.
- Extend build/test scripts and CI workflows to support LMS opt-in builds and LMS KAT vector runs.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit.h | Adds LMS unit test prototypes behind WP_HAVE_LMS. |
| test/unit.c | Registers LMS unit tests behind WP_HAVE_LMS. |
| test/test_lms.c | New LMS-focused unit tests (import/export, decode, unsupported ops). |
| test/include.am | Adds test/test_lms.c to unit test sources. |
| src/wp_wolfprov.c | Registers LMS in provider keymgmt/signature/decoder algorithm tables. |
| src/wp_lms.c | New LMS implementation: keymgmt import/export/match, verify-only signature, XDR decoder. |
| src/include.am | Adds src/wp_lms.c to library sources. |
| scripts/utils-wolfssl.sh | Adds LMS configure flags and mismatch checks for existing wolfSSL installs. |
| scripts/utils-wolfprovider.sh | Adds --enable-lms forwarding and avoids system wolfSSL when PQC/LMS requested. |
| scripts/utils-openssl.sh | Adds LMS mismatch check and enable-lms for OpenSSL builds when requested. |
| scripts/test-pqc-kat.sh | Extends KAT runner to optionally include LMS vectors and enforce expected sub-test counts. |
| scripts/build-wolfprovider.sh | Adds --enable-lms, version gating, and blocks LMS/PQC on FIPS builds and Debian packaging path. |
| README.md | Documents LMS opt-in usage and its verification-only contract/format expectations. |
| include/wolfprovider/settings.h | Adds compile-time gating for LMS availability and explicit error messages. |
| include/wolfprovider/alg_funcs.h | Declares LMS algorithm name and new dispatch tables. |
| docs/INTEGRATION_GUIDE.md | Documents --enable-lms and expands PQC section to include LMS. |
| configure.ac | Adds --enable-lms configure option and defines WOLFPROV_HAVE_LMS. |
| .github/workflows/wolfssl-versions-pqc.yml | Adds an LMS-only matrix row, adjusts eligibility logic, and separates cache variants. |
| .github/workflows/wolfssl-pqc-kat.yml | Adds LMS to the KAT matrix, caching, build flags, and runtime selection. |
| .github/workflows/nginx-pqc.yml | Updates PQC floor logic to v5.9.2-stable and adjusts eligibility checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation:REQUEST_CHANGES
Findings: 18 total — 15 posted, 3 skipped
Posted findings
- [High] No unit test ever reaches wc_LmsKey_Verify - the entire verification path is untested outside the optional KAT job —
test/test_lms.c:321-378 - [Medium] wp_lms_dec_selection has the wrong first-parameter type for OSSL_FUNC_DECODER_DOES_SELECTION, and wp_lms_dec_decode still carries a dead (void)selection cast —
src/wp_lms.c:398-403,524 - [Medium] src/wp_lms.c has no WOLFPROV_ENTER/LEAVE tracing and no Doxygen comments, unlike every other provider source —
src/wp_lms.c:58-528 - [Medium] C99 compound literal in wp_lms_dec_decode - the only one in the whole src/ tree —
src/wp_lms.c:497-500 - [Medium] LMS keymgmt and signature dispatch tables omit GET_PARAMS/GETTABLE_PARAMS/DUP/DUPCTX, so EVP_PKEY_get_bits/size/security_bits return 0 and EVP_PKEY_dup fails —
src/wp_lms.c:267-281,366-373 - [Medium] wp_lms_verify_message_init silently discards the caller's OSSL_PARAM array —
src/wp_lms.c:308-327 - [Medium] wp_lms_import accepts any public-key length in [48,56] instead of deriving the exact length from the LMS type —
src/wp_lms.c:193-209 - [Medium] wp_lms_has() reports a public-only LMS key as a complete keypair, and the new test locks that in —
src/wp_lms.c:128-141 - [Low] Redundant pubLen checks in wp_lms_match —
src/wp_lms.c:152-163 - [Low] configure.ac summary line for LMS is misaligned —
configure.ac:272 - [Low] test-pqc-kat.sh replaced the named EXPECTED_TESTS constant with inline magic numbers —
scripts/test-pqc-kat.sh:72-81 - [Low] wp_lms_free() returns early on wc_LockMutex failure, leaking the key with no comment explaining the tradeoff —
src/wp_lms.c:92-100 - [Low] check_openssl_lms_mismatch reports a misleading cause when the install directory exists but headers do not —
scripts/utils-openssl.sh:403-415 - [Low] New system-wolfSSL guard omits WOLFPROV_PQC, so PQC/LMS builds can still fall back to the distro libwolfssl —
scripts/utils-wolfssl.sh:158-163 - [Info] No ChangeLog.md entry for the new LMS feature —
ChangeLog.md:10
Skipped findings
- [Medium] settings.h gates WP_HAVE_LMS on WP_WOLFSSL_PQC_CAPABLE, which is documented as the ML-DSA header marker
- [Low] Raw LMS type codes in wp_lms_xdr_pub_len have no named constants or spec reference
- [Low] wp_lms_new frees with OPENSSL_free on error paths while wp_lms_free uses OPENSSL_clear_free
Review generated by Skoll via Claude/Codex
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #459
Scan targets checked:wolfprovider-bugs, wolfprovider-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation:REQUEST_CHANGES
Findings: 25 total — 7 posted, 18 skipped
Posted findings
- [High] test_lms.c lmsPub2 is a SHAKE256/192 key, but this commit narrows the build to --enable-lms=sha256-192, so its import assertion cannot pass —
test/test_lms.c:38-45,133-139 - [Medium] test_lms_unsupported_operations reuses one already-modified EVP_PKEY_CTX, pins return codes OpenSSL does not guarantee, and hosts a badSig check that belongs in the verify test —
test/test_lms.c:350-364 - [Medium] src/wp_lms.c still has no WOLFPROV_ENTER/LEAVE tracing and no doxygen comments, unlike every other provider source —
src/wp_lms.c:1-628 - [Medium] OSSL_PKEY_PARAM_PUB_KEY is not gettable, so raw-key support is import-only: EVP_PKEY_new_raw_public_key_ex works but EVP_PKEY_get_raw_public_key fails —
src/wp_lms.c:234-275 - [Medium] wp_lms_import accepts a KEYPAIR selection that the decoder rejects, so the two entry points disagree on what an LMS key can be —
src/wp_lms.c:185-187 - [High] wp_lms_get_params reports both BITS and SECURITY_BITS at values OpenSSL's own LMS keymgmt does not use - SECURITY_BITS comes out at half the real strength —
src/wp_lms.c:255-262 - [Low] LMS externs split the ML-KEM declaration group in alg_funcs.h and place a decoder in the keymgmt block —
include/wolfprovider/alg_funcs.h:435-437
Skipped findings
- [High] wp_lms_match() hard-fails any selection carrying the PRIVATE_KEY bit, diverging from wp_rsa_match/wp_mldsa_match
- [Medium] The dispatch functions added by this commit have no direct test coverage: dup/dupctx, get_params values, dec_export, verify NULL guards, match selection handling, and the SHA-256/256 key path
- [Medium] WP_HAVE_LMS is gated on WP_WOLFSSL_PQC_CAPABLE, a marker defined entirely in terms of ML-DSA
- [Medium] wolfSSL is built with full LMS rather than verify-only, compiling in signing and key-generation code wolfProvider can never reach
- [Medium] run_pqc_kat's new infrastructure-error paths are inverted into a green build on force-fail rows, and WOLFPROV_FORCE_FAIL is expanded bare next to default-safe expansions
- [Medium] Vector handling in test-pqc-kat.sh is inconsistent: EXPECTED_LMS_TESTS is a hardcoded strict-equality count against a dynamically resolved OpenSSL, and only LMS gets an existence check
- [Medium] The FIPS rejection message names only "PQC" while silently broadening the rejection to --enable-fips --enable-pqc
- [Low] --enable-lms help text omits the wolfSSL v5.9.2+/master floor the version gate enforces
- [Low] utils-wolfssl.sh nits: check_wolfssl_feature blames the feature when options.h is missing entirely, and WOLFSSL_PQC_CONFIG_OPTS is a no-op indirection
- [Low] OpenSSL cache separation for LMS rides on the variant string, but the adjacent extra_key reads as though it did the job
- [Low] test_lms.c style nits: braceless single-statement if bodies and two key-import failures with no diagnostic message
- [Low] Two brittle negative-path cases: badPubLen's size is coupled to sizeof(lmsPub3), and the decoder's under-4-byte header path is uncovered
- [Low] XDR decoder readability: WP_LMS_LEVELS_SZ is reused to size the lms_type field, and wp_lms_xdr_pub_len encodes bare LMS registry ranges with no explanatory comment
- [Low] wp_lms_verify_message_init() still silently discards its params array
- [Info] Unrelated trailing-newline deletions add churn to six files
- [Info] wp_lms_free() keeps the key object when the mutex lock fails (scan re-raise: this is the agreed wp_mldsa_free behaviour, not a defect)
- [Info] A failed re-import leaves wp_Lms holding a stale public key that no longer matches the wolfCrypt key
- [Info] The XDR decoder reports 'not my format' when wolfCrypt import genuinely fails, hiding real errors from OpenSSL's decoder chain
Review generated by Skoll via Claude/Codex
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #459
Scan targets checked:wolfprovider-bugs, wolfprovider-src
Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @@ -0,0 +1,818 @@ | |||
| /* test_lms.c | |||
There was a problem hiding this comment.
This file does not use osslLibCtx. Can you add some A/B equiv to the existing test cases? I believe it's just running both contexts through the existing tests and diffing the output
There was a problem hiding this comment.
Added WP/OpenSSL A/B coverage using both library contexts.
| return 1 | ||
| fi | ||
| if [ ${total} -ne ${EXPECTED_TESTS} ]; then | ||
| if [ ${total} -ne ${expected} ]; then |
There was a problem hiding this comment.
The -ne comparison is a rigid check and may fail if/when OSSL updates. The SLH PR does a more robust check here, I believe: if [ ${familyFiles} -lt ${minFiles} ]; then
There was a problem hiding this comment.
Changed the KAT checks to per-family minimums so new vectors are allowed.
| 0x45, 0x1f, 0x2c, 0x28, 0xd4, 0x8a, 0x29, 0x67 | ||
| }; | ||
| static const unsigned char lmsPub3[] = { |
There was a problem hiding this comment.
lmsPub3 seems to be identical to lmsPub2
There was a problem hiding this comment.
Removed the duplicate key and added distinct SHAKE/192 and SHAKE/256 keys.
| (expectedRawSigLen < WP_LMS_LEVELS_SZ) || | ||
| (sigLen != (size_t)expectedRawSigLen - WP_LMS_LEVELS_SZ) || | ||
| (msgLen > INT_MAX))) { | ||
| ok = 0; |
There was a problem hiding this comment.
Can you add a test that covers this failure path, maybe +/- 1 byte since the sig size must match exactly
There was a problem hiding this comment.
Added rejection tests for signatures one byte short and one byte long.
| (EVP_PKEY_get_security_bits(key1) == 0) || | ||
| (EVP_PKEY_get_bits(key1) != 384) || | ||
| (EVP_PKEY_get_security_bits(key1) != 192) || | ||
| (EVP_PKEY_get_size(key1) == 0); |
There was a problem hiding this comment.
Can we check for the exact key size here?
There was a problem hiding this comment.
Changed this to require the exact 780-byte signature size.
| } | ||
| if (ok) { | ||
| p = OSSL_PARAM_locate_const(params, OSSL_PKEY_PARAM_PUB_KEY); | ||
| if ((p == NULL) || |
There was a problem hiding this comment.
Can we add a test case for OSSL_PKEY_PARAM_ENCODED_PUBLIC_KEY?
There was a problem hiding this comment.
Added encoded-public-key export, import, and round-trip checks.
| 0x19, 0x2b, 0x92, 0x3c, 0x5b, 0x6a, 0xf9, 0xa2, | ||
| 0x70, 0xc8, 0x46, 0xd9, 0xfa, 0xfb, 0x22, 0xb0, | ||
| 0x45, 0x1f, 0x2c, 0x28, 0xd4, 0x8a, 0x29, 0x67 | ||
| }; |
There was a problem hiding this comment.
AI tells me these are all 48 bytes. Can we test 192 and 256?
Edit: also, these seem to be all SHA-256/M24. Can we test SHAKE?
There was a problem hiding this comment.
Added SHA-256 and SHAKE coverage at both 192 and 256 bits.
| LmsProviderResult osslResult = { 0 }; | ||
| (void)data; | ||
| err = lms_collect_provider_result(wpLibCtx, &wpResult); |
There was a problem hiding this comment.
Can we add more test cases here, or at least one failure case?
Description