Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit

[yosuke-wolfssl]

Problem

wh_Server_KeystoreCommitKey writes the cached key to NVM through the
unchecked wh_Nvm_AddObjectWithReclaim, so nothing consulted the stored
object's flags. A client could overwrite an existing NONMODIFIABLE or
TRUSTED NVM object — including a trusted KEK — by caching a key under its id
and committing. Closes f-4225.

Fix (src/wh_server_keystore.c)

Added a WH_KS_OP_COMMIT branch to _KeystoreCheckPolicy that reads the
stored object's metadata and denies the overwrite:

• Stored flags, not cached flags — an unchecked cache path
  (unwrap-and-cache, SHE) cannot launder the policy by populating a slot whose
  flags do not reflect the stored object.
• NONMODIFIABLE | TRUSTED — the same pair as wh_Nvm_AddObjectChecked,
  the correct model for an add-shaped operation. NONDESTROYABLE gates
  destroy, not overwrite.
• Independent of cache residency — the verdict comes from nvmMeta whether
  or not the cache slot survived, so a denied commit always reports
  WH_ERROR_ACCESS instead of the WH_ERROR_NOTFOUND raised by a missing slot.
• Unreadable flags fail closed — a backend that cannot report metadata
  cannot be policed, so the commit is denied rather than blind.

Client-visible behavior change: re-committing a cached key whose stored
object is NONMODIFIABLE now returns WH_ERROR_ACCESS where a byte-identical
rewrite previously returned WH_ERROR_OK. This makes commit consistent with
every other immutable-write path — _NvmCheckPolicy (WH_NVM_OP_ADD) already
refuses a no-op rewrite, and commit was the only exception. The contract is now
documented on wh_Server_KeystoreCommitKeyChecked
(wolfhsm/wh_server_keystore.h): a client retrying after a lost response must
treat WH_ERROR_ACCESS as "already committed". Revoke is unaffected; it keeps
its own "already revoked and committed" short-circuit.

Tests

Added to test-refactor/client-server/wh_test_crypto_keystore.c, driven only
by wh_Client_*:

• _whTest_NonModifiableCommit — first commit succeeds, repeat commit returns
  WH_ERROR_ACCESS, the denial still holds once the slot is evicted, and the
  stored bytes and label survive the denial. Gated behind
  WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS, since a committed
  NONMODIFIABLE object cannot be erased.
• _whTest_ModifiableRecommit — ungated; a key without the flag still commits
  repeatedly.

Making that gate runnable. test-refactor could not build with the macro
defined at all: wh_test_crypto_keypolicy.c calls WH_CLIENT_DEVID(client)
inside a helper that takes no client context (pre-existing on main, so both
gated suites were dead code). Threading the context into
whTest_RevocationTryAESEncrypt fixes it, and a new trailing step in
.github/workflows/build-and-test-refactor.yml builds and runs with the macro
defined. That revives this PR's deny-path test and the keypolicy AES-CBC
revocation test, dormant since 606866e.

Verification

Config	Gate off	Gate on
default	45 passed, 26 skipped, 0 failed of 71	45 / 26 / 0
DMA=1 ASAN=1	49 passed, 22 skipped, 0 failed of 71	49 / 22 / 0

• Identical counts either way; clean under -std=c90 -Werror -Wall -Wextra,
  ASan clean. Legacy test/ suite exits 0.
• Negative control: with only the WH_KS_OP_COMMIT branch reverted, the gated
  run fails — Non-modifiable key was re-committed unexpectedly: 0.

[Copilot]

Pull request overview

This pull request closes a defense-in-depth gap in the server keystore “gate” by ensuring WH_KEY_COMMIT cannot overwrite existing NVM objects that are marked NONMODIFIABLE or TRUSTED. The policy enforcement is implemented centrally in the keystore policy checker (rather than in the lower-level commit function), and is validated via a new server-side test suite.

Changes:

• Add a WH_KS_OP_COMMIT policy branch that consults the stored NVM object’s flags and denies overwrite when NONMODIFIABLE or TRUSTED.
• Register and add a new server test (whTest_KeystoreCommitPolicy) covering overwrite-denial, first-commit allowance, normal commit round-trip, and the “uncached commit returns NOTFOUND” guard.
• Extend the server test registry to include the new test.

Reviewed changes

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

File	Description
src/wh_server_keystore.c	Enforces NONMODIFIABLE/TRUSTED overwrite denial for commit by checking stored NVM metadata when a cache slot is being committed.
test-refactor/server/wh_test_keystore_policy.c	Adds targeted server-side tests validating commit policy behavior and ensuring stored bytes/flags remain unchanged on denied overwrite.
test-refactor/wh_test_list.c	Registers the new keystore commit policy test in the server test group.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #492

Scan targets checked: wolfhsm-core-bugs, wolfhsm-src

No new issues found in the changed files. ✅

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: CONDITIONAL
Findings: 6 total — 3 posted, 3 skipped

Posted findings

• [Low] Repeat/idempotent commit of a NONMODIFIABLE key now fails with WH_ERROR_ACCESS (undeclared client-visible behavior change) — src/wh_server_keystore.c:247-263
• [Info] The TRUSTED half of the new check is only reachable via unchecked cache paths; comment does not say so — src/wh_server_keystore.c:248-257
• [Info] Commit-overwrite does not consider NONDESTROYABLE, unlike the evict gate — src/wh_server_keystore.c:247-263

Skipped findings

• [Low] New access-control branch has no test coverage at any level, and the stated rationale for omitting tests does not hold
• [Info] Re-committing an already-committed NONMODIFIABLE key now returns WH_ERROR_ACCESS, breaking idempotent commit retries
• [Info] Commit path now hard-depends on the optional NVM GetMetadata callback

Review generated by Skoll via Claude/Codex

[yosuke-wolfssl]

Hello @Frauschi ,
I fixed this based on your feedbacks.
Could you review it again ?

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: CONDITIONAL
Findings: 6 total — 2 posted, 4 skipped

Posted findings

• [Low] New test leaves an undeletable NONMODIFIABLE NVM object, bypassing the repo's WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS convention — test-refactor/client-server/wh_test_crypto_keystore.c:909-911
• [Low] Commit denial returns WH_ERROR_NOTFOUND instead of WH_ERROR_ACCESS when the cache slot is gone — src/wh_server_keystore.c:247-263

Skipped findings

• [Medium] New keystore test permanently leaks an unerasable NVM object into the shared test fixture
• [Low] wh_Client_KeyCommit is no longer idempotent for NONMODIFIABLE keys, making commit retries a hard failure
• [Info] Fail-closed unreadable-metadata branch in the new commit policy has no test coverage and returns a non-ACCESS error code
• [Info] Revoke path still writes cached bytes to NVM unchecked, contradicting the new comment on the shared switch case

Review generated by Skoll via Claude/Codex

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 10 total — 3 posted, 7 skipped

Posted findings

• [Medium] Fix guards only one of the two client-reachable commit paths; LMS/XMSS DMA keygen still commits unchecked — src/wh_server_crypto.c:7816
• [Medium] Commit is no longer idempotent for NONMODIFIABLE keys, and the contract is undocumented — src/wh_server_keystore.c:247
• [Medium] New NONMODIFIABLE commit regression test never compiles or runs in any buildable configuration — test-refactor/client-server/wh_test_crypto_keystore.c:842

Skipped findings

• [Medium] New commit-denial test is compiled out in every supported configuration
• [Low] Discarded evict return lets the uncached-denial test pass for the wrong reason
• [Low] Fail-closed comment does not cover the server->nvm == NULL path
• [Low] COMMIT branch duplicates the NVM metadata lookup already performed above the switch
• [Low] Discarded evict return leaves Test 4's uncached-commit premise unverified
• [Info] Identical 32-byte key literal duplicated across the two new tests
• [Info] _KeystoreCheckPolicy now carries two divergent notions of "the key's flags"

Review generated by Skoll via Claude/Codex

[Frauschi]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 10 total — 5 posted, 5 skipped

Posted findings

• [Medium] New CI step's CFLAGS_EXTRA override silently disables -Werror -Wall -Wextra for the only job that compiles the gated tests — .github/workflows/build-and-test-refactor.yml:133-136
• [Medium] No test covers the TRUSTED half of the new commit mask - the actual KEK-overwrite scenario — src/wh_server_keystore.c:261-265
• [Medium] Deny-path test could live in test-refactor/server/ and avoid the persistence gate and the new CI job entirely — test-refactor/client-server/wh_test_crypto_keystore.c:842-931
• [Medium] Client-facing header not updated with the new non-idempotent commit contract — wolfhsm/wh_server_keystore.h:162-179
• [Low] New WH_ERROR_NOTFOUND doc on wh_Server_KeystoreCommitKeyChecked is inaccurate, and the retry contract it documents is incomplete — wolfhsm/wh_server_keystore.h:171-178

Skipped findings

• [Low] Gated CI job permanently consumes NVM object slots out of a budget of 30
• [Low] WH_KS_OP_REVOKE keeps the unconditional allow the COMMIT branch was added to close, under a new comment asserting it is safe
• [Info] Comment says "deny" but the code propagates the backend error code
• [Info] Header doc overstates that the verdict never comes from cache flags
• [Info] New gated test permanently consumes an NVM slot mid-run, and the TRUSTED half of the new check is untested

Review generated by Skoll via Claude/Codex

[Frauschi]

🟡 [Medium] New CI step's CFLAGS_EXTRA override silently disables -Werror -Wall -Wextra for the only job that compiles the gated tests
💡 SUGGEST bug

The new step passes the macro as CFLAGS_EXTRA=-DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS on the make command line. test-refactor/posix/Makefile:51-53 defines that variable with a plain = followed by two += lines:

CFLAGS_EXTRA = -Werror -Wall -Wextra
CFLAGS_EXTRA += -ffunction-sections -fdata-sections
CFLAGS_EXTRA += -MMD -MP
...
CFLAGS ?= $(ARCHFLAGS) $(CSTD) $(CFLAGS_EXTRA)

A command-line variable definition overrides every makefile assignment to that variable, and GNU make ignores subsequent += appends to a command-line-defined variable (GNU Make manual §6.6/§6.7). So this job compiles with CFLAGS = -std=c90 -DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS -fsanitize=address and loses -Werror -Wall -Wextra -ffunction-sections -fdata-sections -MMD -MP.

The macro does reach the compiler (so the gated tests run), but three concrete consequences follow:

1. This is the only CI configuration that compiles _whTest_NonModifiableCommit and the revived _whTest_CryptoKeyRevocationAesCbc — code that could not compile at all before this PR — and it is the one config where warnings are not errors. The PR's stated verification ("clean under -std=c90 -Werror -Wall -Wextra") is not what CI enforces.
2. test-refactor/posix/Makefile:248 (wh_test_check_struct_padding.o: CFLAGS += -Wpadded -DWH_PADDING_CHECK) relies on -Werror to turn padding violations into build failures. Without it, the struct-padding check degrades to a printed warning in this job.
3. Losing -MMD -MP means no .d files, so the following make run (invoked without the override, per the same shell line) has no header dependencies; any rebuild triggered there would silently produce objects compiled without the macro.

Every other build knob in this Makefile is a named variable that appends to DEF (SHE=1, AUTH=1, DMA=1, NOCRYPTO=1, …). The macro should follow that convention rather than hijacking the flag variable.

Suggestion: Add a knob to test-refactor/posix/Makefile alongside the other options:

# Allow tests to leave undeletable NVM objects behind (NONMODIFIABLE keys,
# revoked keys). One NVM is shared by every test in a run, so these suites
# occupy object slots for the remainder of the run.
ifeq ($(PERSISTENT_NVM_ARTIFACTS),1)
    DEF += -DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS
endif

and change the workflow step to:

    - name: Build and test refactor with persistent NVM artifacts
      run: cd test-refactor/posix && make clean && make -j ASAN=1 PERSISTENT_NVM_ARTIFACTS=1 WOLFSSL_DIR=../../wolfssl && make run

[Frauschi]

🟡 [Medium] No test covers the TRUSTED half of the new commit mask - the actual KEK-overwrite scenario
💡 SUGGEST test

The new branch denies commit when the stored object carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED, and the PR description frames the threat as "a client could overwrite ... a trusted KEK". Only the NONMODIFIABLE half is tested.

The TRUSTED bit is also the only half that the new code path uniquely protects. Reaching the commit branch means the shared pre-check at line 214 (if (flags & WH_NVM_FLAGS_TRUSTED) return WH_ERROR_ACCESS;) already passed, so either the key is cached with non-trusted cache flags, or it is NVM-only with non-trusted NVM flags. The TRUSTED bit in the new mask therefore only fires in exactly the laundering case the PR is about: a cache slot whose flags do not reflect a TRUSTED stored object. That case has no regression test.

It cannot be tested through wh_Client_* alone - _SanitizeClientFlags and wh_Nvm_AddObjectChecked both strip WH_NVM_FLAGS_SERVER_ONLY, so a client can never provision a TRUSTED object. A server-side test can: provision with the unchecked wh_Nvm_AddObject, populate the cache slot with non-trusted flags via the unchecked wh_Server_KeystoreCacheKey, then assert wh_Server_KeystoreCommitKeyChecked returns WH_ERROR_ACCESS.

Suggestion: Add to test-refactor/server/ (sketch):

/* Stored object is TRUSTED; cache slot claims it is not. */
meta.id = kekId; meta.flags = WH_NVM_FLAGS_TRUSTED; meta.len = sizeof(kek);
WH_TEST_RETURN_ON_FAIL(wh_Nvm_AddObject(nvm, &meta, sizeof(kek), kek));
meta.flags = WH_NVM_FLAGS_NONE;
WH_TEST_RETURN_ON_FAIL(wh_Server_KeystoreCacheKey(server, &meta, attacker));
WH_TEST_ASSERT_RETURN(
    wh_Server_KeystoreCommitKeyChecked(server, kekId) == WH_ERROR_ACCESS);
/* stored bytes must still be the KEK */

[Frauschi]

🟡 [Medium] Deny-path test could live in test-refactor/server/ and avoid the persistence gate and the new CI job entirely
💡 SUGGEST test

Driving the test only through wh_Client_* forces the WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS gate (the committed NONMODIFIABLE object can never be destroyed through the checked client path), which in turn forces the new single-purpose CI job and the whTest_RevocationTryAESEncrypt signature fix just to make that job compile. The result is that the regression test for this fix runs in exactly one of the ~22 CI configurations.

A test in test-refactor/server/ would not need the gate at all: it can provision with wh_Nvm_AddObject, exercise wh_Server_KeystoreCommitKeyChecked directly, and clean up with the unchecked wh_Nvm_DestroyObjects, which ignores policy flags (this is exactly how wh_Server_KeystoreEraseKey erases). No persistent artifact, no gate, no new CI job - and it runs in every config, including the DMA and THREADSAFE ones. It would also make the TRUSTED coverage gap above trivially closable.

The client-driven test is still valuable as an end-to-end check of the client-visible error code; the point is that it should not be the only coverage.

Suggestion: Move the core deny assertions into a test-refactor/server/wh_test_nvm_policy.c-style test that calls wh_Server_KeystoreCommitKeyChecked directly and tears down with wh_Nvm_DestroyObjects (unchecked), leaving the gated client-driven test as an optional end-to-end companion.

[Frauschi]

🟡 [Medium] Client-facing header not updated with the new non-idempotent commit contract
💡 SUGGEST api

The PR correctly documents the behavior change - a repeat commit of a NONMODIFIABLE key now returns WH_ERROR_ACCESS where a byte-identical rewrite previously returned WH_ERROR_OK - and correctly calls out that a client retrying after a lost response must treat WH_ERROR_ACCESS as "already committed".

But that contract is written only on the server header. The client-visible entry point is wh_Client_KeyCommit in wolfhsm/wh_client.h:947-960, whose doc still reads only "Returns 0 on success, or a negative error code on failure". Application authors writing retry logic read the client header, not wh_server_keystore.h. The retry-after-lost-response guidance is exactly the kind of thing that needs to be where the caller will see it.

Suggestion: Mirror the contract on wh_Client_KeyCommit (and wh_Client_KeyCommitResponse) in wolfhsm/wh_client.h:

 * @return int Returns 0 on success, or a negative error code on failure.
 *         WH_ERROR_ACCESS if an NVM object already exists under keyId and is
 *         NONMODIFIABLE (or server-trusted). Commit is not idempotent for such
 *         keys: on retry after a lost response, treat WH_ERROR_ACCESS as
 *         "already committed".
 */

[Frauschi]

🔵 [Low] New WH_ERROR_NOTFOUND doc on wh_Server_KeystoreCommitKeyChecked is inaccurate, and the retry contract it documents is incomplete
🔧 NIT

The newly added doc block states @return WH_ERROR_NOTFOUND if the key is in neither cache nor NVM. That is not the actual condition. After _KeystoreCheckPolicy passes, wh_Server_KeystoreCommitKey looks the key up with _FindInKeyCache only (src/wh_server_keystore.c:1244) and returns its WH_ERROR_NOTFOUND verbatim if there is no cache slot. So commit returns WH_ERROR_NOTFOUND whenever the key is absent from the cache, whether or not it exists in NVM.

This matters because the same doc block introduces a client retry contract: "a client retrying after a lost response must treat WH_ERROR_ACCESS as 'already committed'." For a modifiable key that contract is under-specified in the opposite direction. _GetKeyCacheSlot (src/wh_server_keystore.c:376-387) reclaims any slot whose committed == 1 under cache pressure, so a successfully-committed modifiable key can lose its slot at any time. A client whose commit response is lost and which retries then receives WH_ERROR_NOTFOUND for a key that is in fact committed — the documented "treat ACCESS as already committed" rule gives no guidance for that case, and the NOTFOUND line actively suggests the key does not exist in NVM either.

Triggering sequence: cache key K with WH_NVM_FLAGS_NONE, commit K (succeeds), cache enough other keys to reclaim K's slot, then commit K again -> WH_ERROR_NOTFOUND, even though K is present in NVM.

Recommendation: Correct the return doc and complete the retry guidance:

 * @return WH_ERROR_NOTFOUND if the key is not resident in the cache. Commit
 *         writes the cached copy, so this is returned even when an object
 *         already exists in NVM under keyId (for example after the cache slot
 *         of an already-committed key was reclaimed).
 * @return WH_ERROR_ACCESS if an NVM object already exists under keyId and
 *         carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED. ...
 *         A client retrying after a lost response must therefore treat both
 *         WH_ERROR_ACCESS (immutable object already stored) and
 *         WH_ERROR_NOTFOUND (cache slot reclaimed after a successful commit)
 *         as possible outcomes of an already-completed commit, and confirm
 *         with an export or metadata read rather than re-caching.