Nvm ids per client - #460
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens and clarifies client-facing NVM object identifiers by translating client-supplied NVM ids into the server’s internal TYPE/USER/ID encoding (per-client namespaces + optional shared global namespace), while adding test/provisioning-only SHE key preprogram/destroy messages that remain compatible with the new NVM id translation.
Changes:
- Add per-client NVM id translation in the server NVM message handlers, with an opt-out legacy mode (
WOLFHSM_CFG_LEGACY_CLIENT_NVM). - Introduce gated, test/provisioning-only SHE key management messages/APIs (
PreProgramKey,DestroyKey) and update tests/CI to use them. - Expand multiclient/stress tests and documentation to validate and explain the new NVM namespace semantics (including global/local/mixed variants).
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfhsm/wh_settings.h | Documents the new WOLFHSM_CFG_LEGACY_CLIENT_NVM compatibility switch. |
| wolfhsm/wh_message.h | Adds new SHE actions for test key management. |
| wolfhsm/wh_message_she.h | Defines new SHE request/response structs and translate APIs (gated). |
| wolfhsm/wh_client.h | Documents client-facing NVM id encoding/namespace semantics. |
| wolfhsm/wh_client_she.h | Declares gated test/provisioning-only SHE key management APIs. |
| test/wh_test_she.c | Switches SHE key destruction helper to the new SHE destroy API. |
| test/wh_test_posix_threadsafe_stress.c | Extends stress suite to run global/local/mixed namespace variants and validates data isolation. |
| test/wh_test_multiclient.c | Adds a new NVM id-translation/isolation test suite (gated off in legacy mode). |
| test/wh_test_clientserver.c | Updates test NVM id to remain valid under client-facing id constraints (1–255). |
| test/config/wolfhsm_cfg.h | Increases NVM object count for tests; enables gated SHE test key management. |
| test-refactor/misc/wh_test_multiclient.c | Mirrors the new NVM id-translation test suite in refactor tests. |
| test-refactor/config/wolfhsm_cfg.h | Increases NVM object count; enables gated SHE test key management in refactor config. |
| test-refactor/client-server/wh_test_she.c | Switches SHE key destruction helper to the new SHE destroy API. |
| src/wh_server_she.c | Implements gated server handlers for SHE preprogram/destroy key messages; updates state validation. |
| src/wh_server_nvm.c | Implements client NVM id translation + legacy passthrough and list namespace filtering logic. |
| src/wh_message_she.c | Implements translation helpers for the new SHE test key management messages (gated). |
| src/wh_client_she.c | Implements gated client APIs for SHE preprogram/destroy via SHE message group (with response validation). |
| examples/posix/wh_posix_server/wolfhsm_cfg.h | Increases example server’s NVM object count to align with heavier tests. |
| docs/src/9-Configuration.md | Documents WOLFHSM_CFG_SHE_ENABLE_TEST_KEY_MGMT and WOLFHSM_CFG_LEGACY_CLIENT_NVM. |
| docs/src/6-Utilities.md | Clarifies that whnvmtool ids are server-internal and how that interacts with client NVM namespaces. |
| docs/src/5-Features.md | Adds a new section documenting client NVM namespaces + updates SHE helper documentation. |
| .github/workflows/build-and-test-stress.yml | Increases stress CI timeout to account for more variants. |
| .github/workflows/build-and-test-clientonly.yml | Builds CI server with gated SHE test key management so client SHE tests work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8f07038 to
693e3e9
Compare
e600900 to
80e57ce
Compare
4c9b0d4 to
ea87b29
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #460
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
ea87b29 to
f1db476
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #460
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| size = sizeof(int32_t); | ||
| break; | ||
| } | ||
| resp_size = |
There was a problem hiding this comment.
Denied DMA keystore requests are reported to the client as success · Client/server authentication bypass
_FormatRcOnlyResponse writes the error code at offset 0 and zeroes the rest, but whMessageKeystore_CacheDmaResponse, ExportDmaResponse and ExportPublicDmaResponse start with a 16-byte dmaAddrStatus and carry rc at offset 16. The client's size check now passes and it reads rc == 0, so a pre-COMM-INIT or authorization-denied DMA key cache/export is treated as successful (keyId 0, len 0).
Related known finding #9395 (similar but distinct): Both concern dispatcher error paths that leave clients without a usable error result, but this finding uses _FormatRcOnlyResponse on DMA response structs whose rc is at offset 16, while #9395 sends an empty payload for unknown message groups. The faulting operations, root causes, and required patches are different.
Fix: Populate the DMA keystore error responses through their real struct layouts so rc lands at its actual offset instead of using _FormatRcOnlyResponse.
No description provided.