Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1259
Error: RuntimeError
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1259
Error: RuntimeError
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1259
Error: RuntimeError
e490ef4 to
e50b848
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1259
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
| Set for ML-DSA-65. | ||
| WOLFSSH_NO_MLDSA87 | ||
| Set for ML-DSA-87. | ||
| WOLFSSH_NO_MLDSA44_ES256, WOLFSSH_NO_MLDSA65_ES256, |
There was a problem hiding this comment.
Missing WOLFSSH_HAVE_COMPOSITE_ED448.
| # traditional halves stay on; the six gates must follow. | ||
| - name: composites-off | ||
| defines: -DWOLFSSH_NO_MLDSA_COMPOSITES | ||
| expect: '' |
There was a problem hiding this comment.
The four rows cover the pairing gates well, but every one of them builds wolfSSL --enable-all, so all the wolfCrypt sub-features are on in all of them. That means no row ever reaches the state where HAVE_ED448 is set but WOLFSSH_HAVE_COMPOSITE_ED448 is not, which is precisely the state the new macro was added for, and the one where tests/unit.c currently fails to compile. A row here would have caught that.
This needs no second wolfSSL build: settings.h derives HAVE_ED448_SIGN from the absence of NO_ED448_SIGN in every wolfSSH translation unit, so a -DNO_ED448_SIGN in CPPFLAGS gives wolfSSH's compiles the sub-feature-off view while the installed library still exports the symbol. I confirmed the expect string below is what client -E actually offers in that build.
| expect: '' | |
| expect: '' | |
| # A wolfCrypt sub-feature off rather than a pairing gate: HAVE_ED448 | |
| # stays set but WOLFSSH_HAVE_COMPOSITE_ED448 does not, which is the | |
| # state that macro exists for. No second wolfSSL build is needed -- | |
| # settings.h is re-processed in every wolfSSH translation unit, so | |
| # this suppresses HAVE_ED448_SIGN for wolfSSH's compiles while the | |
| # installed library still exports the symbol. | |
| - name: no-ed448-sign | |
| defines: -DNO_ED448_SIGN | |
| expect: ssh-mldsa44-ed25519@openssh.com ssh-mldsa44-es256@wolfssl.com ssh-mldsa65-ed25519@wolfssl.com ssh-mldsa65-es256@wolfssl.com ssh-mldsa87-es384@wolfssl.com |
There was a problem hiding this comment.
Good point, I applied suggested change
| if (ret != 0) return ret; | ||
| #endif | ||
| #if !defined(WOLFSSH_NO_MLDSA87) && defined(HAVE_ED448) | ||
| #ifndef WOLFSSH_NO_MLDSA87_ED448 |
There was a problem hiding this comment.
This Ed448 call site is converted, but the helper it calls isn't: test_DoUserAuthRequestMlDsaComposite_Params still gates its Ed448 code on bare HAVE_ED448, so the gate you just added here is the only thing standing between the helper and a compile error.
With ./configure --enable-all --with-wolfssl=/opt/wolfssl CPPFLAGS="-DNO_ED448_SIGN", make succeeds -- library, apps and examples all build, which is exactly what this PR set out to achieve -- but make tests/unit.test fails:
tests/unit.c:14871:13: error: use of undeclared identifier 'wc_ed448_sign_msg'
options.h defines HAVE_ED448 but none of the sub-features; settings.h derives HAVE_ED448_SIGN and friends per translation unit from the absence of the matching NO_ED448_*. ed448.h:114 declares wc_ed448_sign_msg() inside #ifdef HAVE_ED448_SIGN and :190 declares wc_ed448_export_public() inside #ifdef HAVE_ED448_KEY_EXPORT. So with NO_ED448_SIGN set, WOLFSSH_HAVE_COMPOSITE_ED448 is off, WOLFSSH_NO_MLDSA87_ED448 gets derived and this call site correctly vanishes -- but the guards inside the helper body are still true and still call functions wolfSSL no longer declares.
After this PR, tests/unit.c is the only file left in the tree with functional composite Ed448 code on bare HAVE_ED448. The remaining bare uses in src/keygen.c and wolfssh/internal.h are #include guards for ed448.h, which are right as they are.
There are five guards in the helper needing the same swap to WOLFSSH_HAVE_COMPOSITE_ED448. They're outside the diff, so I can't mark them inline -- at the current head they're tests/unit.c lines 14667 (the ed448_key declaration), 14736 (init/make-key), 14774 (export-public), 14869 (sign) and 15005 (free). Worth doing all five rather than just the two that error: wc_ed448_init(), wc_ed448_make_key() and wc_ed448_free() are declared unconditionally in ed448.h, so converting only the sign and export-public branches would leave ed448Key and ed448Init unused, which is a -Werror failure of its own.
I applied the five-line change locally: make tests/unit.test builds clean and ./tests/unit.test runs green in that config, including MlDsaCompositeKeyGen: SUCCESS. Default --enable-all is unaffected -- it builds and both unit.test and kex.test pass before and after.
There was a problem hiding this comment.
Applied suggested fixes
4 participants
Added per-composite gate macros to group in dependencies, and fixes to the ED448 composite and ED25519 macros. Updated macro call sites to use the new, single macros. Added CI workflow to exercise composite configurations.