Nvm ids per client by bigbrett · Pull Request #460 · wolfSSL/wolfHSM

bigbrett · 2026-07-20T20:09:14Z

Refactors NVM layer to add per-client NVM id translation for all client-facing requests. Client NVM objecs are no longer global identifiers, but now are scoped similar to existing keyIds: every client gets 1-255 unique objects dedicated to them, with 1-255 global objects shared across clients.

It also adds enforcement for an additional server-side rule: any NVM object used for something by the server, either internally or in response to a client API (e.g. certificate API) now MUST be classified by their on NVM object type. This prevents maliciously misusing the cert API to name non-cert objects.

It also introduces some test-only SHE functionality such that the SHE layer remains compatible.

The bulk of this PR is testing code.

Copilot

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.

wolfSSL-Fenrir-bot

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.

wolfSSL-Fenrir-bot

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.

wolfSSL-Fenrir-bot

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.

wolfSSL-Fenrir-bot · 2026-09-03T19:45:43Z

+         * so refuse the request with a properly shaped error response. */
+        if ((group != WH_MESSAGE_GROUP_COMM) &&
+            (server->comm->client_id == WH_KEYUSER_GLOBAL)) {
+            uint16_t resp_size = _FormatErrorResponse(


Pre-COMM-INIT refusal returns a malformed response for the crypto message groups · Message parsing flaws

The new gate formats every refused request with _FormatErrorResponse, whose default: branch writes the error at offset 0 and returns 4 bytes. For WH_MESSAGE_GROUP_CRYPTO/CRYPTO_DMA the response header is {algoType, rc, reserved}, so WH_ERROR_ACCESS lands in algoType and rc is never written; _getCryptoResponse() rejects the packet and the client reports WH_ERROR_ABORTED instead of WH_ERROR_ACCESS.

Related known finding #4033 (similar but distinct): Both involve pre-COMM-INIT handling in wh_Server_HandleRequestMessage, but 4033 faults by allowing requests through due to a missing INIT-state gate, while this candidate faults when the new refusal path formats CRYPTO/CRYPTO_DMA error payloads with the wrong header layout. The root causes and operations differ, and fixing 4033's gate would not populate the crypto response rc field; this requires a separate _FormatErrorResponse patch.

Fix: Add explicit WH_MESSAGE_GROUP_CRYPTO/WH_MESSAGE_GROUP_CRYPTO_DMA cases to _FormatErrorResponse that emit a full whMessageCrypto_GenericResponseHeader with rc set.

global

billphipps

Wow!! Looks great! I only checked headers and src. Lemme know when you want another review. Minor comments

billphipps · 2026-09-09T23:56:52Z

+     * legacy-style 16-bit id must fail loudly instead */
+    if ((reqId & (whKeyId) ~(WH_KEYID_MASK | WH_CLIENT_KEYID_FLAGS_MASK)) !=
+        0) {
+        return WH_ERROR_BADARGS;


Since this is a new behavior for a legacy interface, consider a new error message like DEPRECATED?

billphipps · 2026-09-10T00:00:52Z

+        if ((req.client_id == WH_KEYUSER_GLOBAL) ||
+            (req.client_id > WH_CLIENT_ID_MAX)) {
            *out_resp_size = 0;
            return WH_ERROR_BADARGS;


Consider ABORTED here. To me, BADARGS means a coding error, like assert. In this case, the client somehow passed an invalid client id.

billphipps · 2026-09-10T00:06:46Z

+    int rc = create ? wh_KeyId_CheckClientObjectIdForCreate(reqId)
+                    : wh_KeyId_CheckClientObjectId(reqId);


Nit: consider more verbose if/else instead of the shorthand. not sure we do this anywhere else in the code.

billphipps · 2026-09-10T00:12:02Z

+             * unchecked NVM read, so this is the only gate. */
+            if (rc == WH_ERROR_OK) {
+                rc = WH_SERVER_NVM_LOCK(server);
+            }


Unexpected close } here. Consider moving this down and spacing the next few sunblocks to improve readability.

billphipps · 2026-09-10T00:14:59Z

-            ret = WH_SERVER_NVM_LOCK(server);
+            /* Refuse an id that translation would silently alias before
+             * creating anything */
+            ret = wh_KeyId_CheckClientObjectIdForCreate(req.counterId);


Is there a concurrency issue with checking the object id without holding the nvm_lock?

billphipps · 2026-09-10T00:17:05Z

+static whNvmId _NvmTranslateFromClient(whServerContext* server,
+                                       whNvmId          clientId)
+{
+#ifdef WOLFHSM_CFG_LEGACY_CLIENT_NVM


Is this CFG going to be deprecated? Could this test be put within translateobjectfromclient?

billphipps · 2026-09-10T00:55:34Z

+ *  hardware flags ignored)
+ * @return Server-internal keyId with the fixed TYPE, resolved USER, and ID.
+ */
+whKeyId wh_KeyId_TranslateObjectFromClient(uint16_t type, uint16_t clientId,


Consider naming ...TranslateObjectIdFrom... ?

Copilot AI review requested due to automatic review settings July 20, 2026 20:09

Copilot started reviewing on behalf of bigbrett July 20, 2026 20:09 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread src/wh_server_nvm.c

Comment thread src/wh_server_nvm.c

Comment thread src/wh_server_nvm.c

bigbrett force-pushed the nvm-ids-per-client branch from 8f07038 to 693e3e9 Compare July 20, 2026 21:58

bigbrett force-pushed the nvm-ids-per-client branch from e600900 to 80e57ce Compare August 5, 2026 20:25

bigbrett force-pushed the nvm-ids-per-client branch from 4c9b0d4 to ea87b29 Compare August 27, 2026 15:12

bigbrett requested a review from wolfSSL-Fenrir-bot August 27, 2026 19:39

wolfSSL-Fenrir-bot requested changes Aug 27, 2026

View reviewed changes

bigbrett self-assigned this Sep 2, 2026

bigbrett marked this pull request as ready for review September 2, 2026 21:05

bigbrett force-pushed the nvm-ids-per-client branch from ea87b29 to f1db476 Compare September 2, 2026 21:05

bigbrett requested a review from wolfSSL-Fenrir-bot September 2, 2026 21:10

wolfSSL-Fenrir-bot requested changes Sep 2, 2026

View reviewed changes

bigbrett force-pushed the nvm-ids-per-client branch from f1db476 to 09c67c0 Compare September 3, 2026 18:52

bigbrett requested a review from wolfSSL-Fenrir-bot September 3, 2026 19:10

wolfSSL-Fenrir-bot requested changes Sep 3, 2026

View reviewed changes

bigbrett added 3 commits September 8, 2026 14:05

Major refactor of NVM object IDs making them per-client instead of

4d0c43e

global

feedback and additional hardening

17cef51

fenrir fix

8804e56

bigbrett force-pushed the nvm-ids-per-client branch from 09c67c0 to 8804e56 Compare September 9, 2026 20:28

billphipps reviewed Sep 10, 2026

View reviewed changes

		int rc = create ? wh_KeyId_CheckClientObjectIdForCreate(reqId)
		: wh_KeyId_CheckClientObjectId(reqId);

Uh oh!

Conversation

bigbrett commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #460

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #460

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #460

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billphipps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bigbrett commented Jul 20, 2026 •

edited

Loading