Un-nest Ed25519 cryptocb cases from HAVE_CURVE25519#493
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a conditional-compilation bug in the client crypto callback dispatch so Ed25519 operations are available even when Curve25519 is disabled (they are independent wolfSSL build options). It also adds a targeted regression test and CI coverage to prevent future guard/dispatch regressions in Ed25519 offload.
Changes:
- Un-nests the Ed25519
WC_PK_TYPE_ED25519_{KEYGEN,SIGN,VERIFY}cases from#ifdef HAVE_CURVE25519inwh_Client_CryptoCbStd. - Adds a new refactor test that signs/verifies via wolfCrypt APIs using a server-only Ed25519 keyId (ensuring the cryptocb path is required).
- Introduces an
NOCURVE25519=1build knob for both test harness Makefiles and adds a CI job to exercise Ed25519-without-Curve25519.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/wh_client_cryptocb.c |
Moves the #endif /* HAVE_CURVE25519 */ to make Ed25519 dispatch cases compile independently of Curve25519. |
test-refactor/client-server/wh_test_crypto_ed25519.c |
Adds a regression test that forces Ed25519 sign/verify through the crypto callback using server-only keyIds. |
test/config/user_settings.h |
Allows disabling HAVE_CURVE25519 for tests via WOLFHSM_CFG_TEST_NO_CURVE25519. |
test/Makefile |
Adds NOCURVE25519=1 knob to define WOLFHSM_CFG_TEST_NO_CURVE25519. |
test-refactor/posix/Makefile |
Adds the same NOCURVE25519=1 knob for the refactor harness. |
.github/workflows/build-and-test-refactor.yml |
Adds a CI build/test row that runs with NOCURVE25519=1 to cover the fixed dispatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #493
Scan targets checked: wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
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
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.
Problem
In
wh_Client_CryptoCbStd, the#ifdef HAVE_ED25519block holding theWC_PK_TYPE_ED25519_KEYGEN,_SIGNand_VERIFYcases was nested inside the#ifdef HAVE_CURVE25519block. These are independent wolfSSL build options, sothe three dispatch cases compiled only when both were defined.
In a build with Ed25519 enabled and Curve25519 disabled, every Ed25519 operation
on a wolfHSM devId fell through to
default: ret = CRYPTOCB_UNAVAILABLE;, whichwh_Client_CryptoCbdeliberately propagates, so wolfCrypt silently used itssoftware implementation. For a key whose private scalar lives only in the HSM
(
devCtxholds a keyId, no local material) that path cannot sign at all — HSMoffload broke with no diagnostic. The DMA sibling
wh_Client_CryptoCbDmaalready had the correct sibling layout, confirming the nesting was accidental.
Closes F-7149.
Fix (
src/wh_client_cryptocb.c)Moved
#endif /* HAVE_CURVE25519 */up to just after theWC_PK_TYPE_CURVE25519case so the Ed25519 group is a sibling rather than a child:
One line moved, net zero. Every other Curve25519/Ed25519 guard pair in the repo
was audited and already correct.
Test harness
The existing Ed25519 tests did not catch this: they call the
wh_Client_Ed25519*APIs directly, bypassing the crypto callback, and the inlinetest signs with a key that still holds local private material. The whole suite
passed with the dispatch cases compiled out.
_whTest_CryptoEd25519CryptoCbHsmKey(test-refactor/client-server/wh_test_crypto_ed25519.c)signs via
wc_ed25519_sign_msg()on a key reduced to a bare keyId, so thecallback must handle it. A software fallback returns
BAD_FUNC_ARG.NOCURVE25519=1build knob added to both harness Makefiles, gatingHAVE_CURVE25519in the sharedtest/config/user_settings.h.build-and-test-refactor.yml.Verification
Negative control on unfixed source:
NOCURVE25519=1failed withrc=-173(
BAD_FUNC_ARG); the default build passed — isolating the defect to the guard.NOCURVE25519=1DMA=1DMA=1 ASAN=1 NOCURVE25519=1NOCURVE25519=1