Skip to content

Add SLH-DSA Support - #455

Open
aidangarske wants to merge 24 commits into
wolfSSL:masterfrom
aidangarske:add-slhdsa
Open

Add SLH-DSA Support#455
aidangarske wants to merge 24 commits into
wolfSSL:masterfrom
aidangarske:add-slhdsa

Conversation

@aidangarske

@aidangarskeaidangarske commented Jul 30, 2026

Copy link
Copy Markdown
Member

Description

CopilotAI review requested due to automatic review settings July 30, 2026 18:04

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@aidangarske
aidangarskeforce-pushed the add-slhdsa branch 2 times, most recently from 0487400 to 02cbe8bCompareJuly 30, 2026 18:27
@aidangarskeaidangarske added the ci:nginx-pqc PR OSP toggle: run nginx-pqc label Jul 30, 2026
@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:nginx-pqc PR OSP toggle: run nginx-pqc ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Jul 30, 2026
@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Jul 30, 2026
@aidangarske
aidangarske marked this pull request as ready for review July 30, 2026 23:26
@aidangarske
aidangarskeforce-pushed the add-slhdsa branch 2 times, most recently from 8bf2ba2 to ba88c76CompareJuly 31, 2026 01:05
@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Jul 31, 2026
Frauschi

This comment was marked as resolved.

@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Jul 31, 2026
@aidangarskeaidangarske removed the ci:libacvp-pqc PR OSP toggle: run libacvp-pqc label Aug 1, 2026
@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Aug 5, 2026

@FrauschiFrauschi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐺 Skoll Code Review

Overall recommendation:REQUEST_CHANGES
Findings: 12 total — 5 posted, 7 skipped

Posted findings

  • [High] openssl_example.c rewrite hard-fails on the IDE/Android and IDE/XCODE flowsexamples/openssl_example.c:47-75 (duplicated in examples/pqc_openssl_example.c:225-238)
  • [Medium] KAT vector accounting: the verify-only SLH-DSA KAT can pass with zero vectors, the exact 3099 count is pinned against a floating OpenSSL tag, and the old 2602 comment was left behind.github/workflows/wolfssl-versions-pqc.yml:204-214
  • [Medium] WOLFSSL_SLHDSA_CONFIG is missing from the dependency cache key, undocumented in --help, and contradicted by INTEGRATION_GUIDEscripts/utils-wolfssl.sh:64-70
  • [Medium] Example build gating and portability: executableDir may trip -Wmaybe-uninitialized, and BUILD_PQC_EXAMPLE is true for verify-only SLH-DSA builds where the example always failsexamples/openssl_example.c:62-75; examples/pqc_openssl_example.c:225-238
  • [Low] wp_mlkem_kem.c doc comment no longer matches the code it documentssrc/wp_mlkem_kem.c:186
Skipped findings
  • [High] libacvp PQC job likely cannot build runtest, so the whole workflow may no-op
  • [Medium] SLH-DSA test coverage gaps: four guard paths and the deterministic-keygen seed length validation are unexercised
  • [Low] CI workflow hygiene: example steps run before the dependency push, nginx-pqc adds per-workflow apt installs, and test.sh defaults WOLFPROV_ROOT to a path nothing produces
  • [Low] wp_slhdsa_key_types uses opaque index arithmetic into a flat parameter array
  • [Low] test_slhdsa.c hygiene: randomised input described as deterministic, a verify context initialised with sign-only params, mixed XMEM*/libc string calls, and redundant double cleanup
  • [Low] SLH-DSA locking contract is looser than the mutex implies: the key mutex is held across the full multi-second sign and across thread-local CheckKey work, wp_slhdsa_get_key() hands out a raw pointer to guarded state, and construction-time hasPub/hasPriv writes are unlocked
  • [Info] settings.h pulls a wolfSSL algorithm header into every translation unit

Review generated by Skoll via Claude/Codex

Comment threadexamples/openssl_example.c
Comment thread.github/workflows/wolfssl-versions-pqc.yml
Comment threadscripts/utils-wolfssl.sh
Comment threadsrc/wp_mlkem_kem.c Outdated

@FrauschiFrauschi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐺 Skoll Code Review

Overall recommendation:APPROVE
Findings: 16 total — 7 posted, 9 skipped

Posted findings

  • [Medium] SLH-DSA verify-only CI row is indistinguishable from the plain slhdsa row and cannot detect a lost WOLFSSL_SLHDSA_CONFIG.github/workflows/wolfssl-versions-pqc.yml:183-191
  • [Medium] wc_SlhDsaKey_CheckKey runs twice on every SLH-DSA keypair importsrc/wp_slhdsa_kmgmt.c:854,895
  • [Low] wp_slhdsa_validate holds keyMutex across a full key copy and wc_SlhDsaKey_CheckKeysrc/wp_slhdsa_kmgmt.c:745-795
  • [Low] nginx-pqc installs packages that are already baked into the test-deps image, against that image's stated policy.github/workflows/nginx-pqc.yml:132-136
  • [Low] Missing cast on OPENSSL_malloc returnsrc/wp_slhdsa_kmgmt.c:766
  • [Low] if (dupPub) is not guarded by ok while the adjacent if (ok && dupPriv) issrc/wp_slhdsa_kmgmt.c:455
  • [Low] word32 sigSz; declared uninitialized in wp_slhdsa_signsrc/wp_slhdsa_sig.c:344
Skipped findings
  • [Low] run_vector_family silently mutates its caller's local counters via bash dynamic scoping
  • [Low] 7 of 12 SLH-DSA AlgorithmIdentifier OID arcs are never exercised by any test
  • [Low] extra_key gains a stray trailing dash on 7 of 8 matrix rows
  • [Low] wp_slhdsa_validate nests refMutex inside keyMutex and runs the full CheckKey inside the shared key's critical section
  • [Low] wp_slhdsa_sign and wp_slhdsa_verify hold the per-key mutex across the entire multi-second wolfSSL SLH-DSA operation
  • [Info] Do not 'fix' SLH-DSA's export_object / verify_message_final to match ML-DSA and ECX -- SLH-DSA is the correct one
  • [Info] Examples gate on the configure macro WOLFPROV_HAVE_* rather than the resolved WP_HAVE_*
  • [Info] encData cleansed with post-encryption length instead of allocation size
  • [Info] Three sites write hasPub/hasPriv without keyMutex, contradicting the invariant documented at wp_slhdsa_kmgmt.c:271

Review generated by Skoll via Claude/Codex

Comment thread.github/workflows/wolfssl-versions-pqc.yml
Comment threadsrc/wp_slhdsa_kmgmt.c Outdated
Comment threadsrc/wp_slhdsa_kmgmt.c
Comment thread.github/workflows/nginx-pqc.yml Outdated
Comment threadsrc/wp_slhdsa_kmgmt.c Outdated
Comment threadsrc/wp_slhdsa_kmgmt.c Outdated
Comment threadsrc/wp_slhdsa_sig.c Outdated

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Suppressed comments (1)

docs/INTEGRATION_GUIDE.md:171

  • The updated PQC requirements still say that any OpenSSL 3.x is supported on the next line, but settings.h gates ML-KEM, ML-DSA, and SLH-DSA on OpenSSL 3.6+. Following this guide with OpenSSL 3.0–3.5 therefore ends in a compile-time error. Update the OpenSSL requirement to 3.6+.
- **wolfSSL**: v5.9.2-stable or later. v5.9.1-stable defines `HAVE_DILITHIUM` and exposes `wc_dilithium_sign_ctx_msg` (the older name for the FIPS 204 pure-mode signer) but does not yet ship the canonical `WOLFSSL_HAVE_MLDSA` macro, `<wolfssl/wolfcrypt/wc_mldsa.h>` header, or `wc_MlDsaKey_SignCtx` alias that wolfProvider gates on.

@wolfSSL-Fenrir-botwolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #455

Scan targets checked:wolfprovider-bugs, wolfprovider-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment threadtest/test_slhdsa.c
Comment threadsrc/wp_slhdsa_sig.c
@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Aug 8, 2026
Comment threadtest/test_slhdsa.c

#include "unit.h"

#include <openssl/core_names.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some A/B comparison scenarios to this file between WP and OSSL? Iiuc, the outputs should be byte identical

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added WP/OpenSSL A/B checks for identical seeded keys and deterministic signatures.


#include <wolfssl/wolfcrypt/wc_mlkem.h>
#include <wolfssl/wolfcrypt/wc_mldsa.h>
#if defined(WP_HAVE_SLHDSA) && defined(WP_HAVE_SLHDSA_PRIVATE) && \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For interop testing, can you add PEM and DER file interop between WP and OSSL?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added DER and PEM key-file interop in both directions.


#include <wolfprovider/settings.h>

#if defined(WP_HAVE_MLKEM) && defined(WP_HAVE_MLDSA)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WP_HAVE_SLHDSA check is within the WP_HAVE_MLKEM check. Shouldn't these be orthogonal?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the SLH-DSA tests independent of the ML-KEM build guards.

Comment threadsrc/wp_slhdsa_kmgmt.c

/* Parameter set tables. Security category follows FIPS 205 Table 2. */
#ifdef WP_HAVE_SLH_DSA_SHAKE_128S
static const wp_SlhDsaData slhdsaShake128sData = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI tells me only 5 of the 12 of this table are covered by tests

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added keygen and metadata coverage for all 12 parameter sets.

@aidangarske

Copy link
Copy Markdown
MemberAuthor

Jenkins retest this please

@aidangarskeaidangarske added ci:libacvp-pqc PR OSP toggle: run libacvp-pqc and removed ci:libacvp-pqc PR OSP toggle: run libacvp-pqc labels Aug 14, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:libacvp-pqcPR OSP toggle: run libacvp-pqc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@aidangarske@Frauschi@padelsbach@wolfSSL-Fenrir-bot