Skip to content

Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit #492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enforce NONMODIFIABLE and TRUSTED NVM policy on keystore key commit #492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/build-and-test-refactor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,3 +129,8 @@ jobs:
# Build and test with AUTH=1 and NOCRYPTO=1 (auth on, crypto off)
- name: Build and test refactor with AUTH NOCRYPTO
run: cd test-refactor/posix && make clean && make -j AUTH=1 NOCRYPTO=1 WOLFSSL_DIR=../../wolfssl && make run

# Build and test the suites that leave undeletable NVM objects behind.

@Frauschi Frauschi Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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

# Kept last: they occupy NVM slots for the remainder of the run.
- name: Build and test refactor with persistent NVM artifacts
run: cd test-refactor/posix && make clean && make -j ASAN=1 WOLFSSL_DIR=../../wolfssl CFLAGS_EXTRA=-DWOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS && make run
22 changes: 21 additions & 1 deletion src/wh_server_keystore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,28 @@ static int _KeystoreCheckPolicy(whServerContext* server, whKsOp op,
break;

case WH_KS_OP_COMMIT:
Comment thread
Frauschi marked this conversation as resolved.
Comment thread
Frauschi marked this conversation as resolved.
Comment thread
Frauschi marked this conversation as resolved.
Comment thread
Frauschi marked this conversation as resolved.
/* Stored flags decide, not cached, so an unchecked cache path
* cannot launder them; same pair as wh_Nvm_AddObjectChecked.
* Fetched here too, so cache residency cannot change the verdict. */
if (!foundInNvm && (server->nvm != NULL)) {
ret = wh_Nvm_GetMetadata(server->nvm, keyId, &nvmMeta);
if (ret == WH_ERROR_OK) {
foundInNvm = 1;
}
else if (ret != WH_ERROR_NOTFOUND) {
/* Unreadable flags cannot be enforced: deny. */
return ret;
}
}
if (foundInNvm &&

@Frauschi Frauschi Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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 */

(nvmMeta.flags &
(WH_NVM_FLAGS_NONMODIFIABLE | WH_NVM_FLAGS_TRUSTED))) {
return WH_ERROR_ACCESS;
}
break;

case WH_KS_OP_REVOKE:
/* Always allowed */
/* Revocation only tightens policy */
break;
default:
/* unknown operation */
Expand Down
13 changes: 7 additions & 6 deletions test-refactor/client-server/wh_test_crypto_keypolicy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,8 @@ static int _whTest_CryptoKeyUsagePolicies(whClientContext* client)

#if !defined(NO_AES) && defined(HAVE_AES_CBC) && \
defined(WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS)
static int whTest_RevocationTryAESEncrypt(whKeyId keyId, WC_RNG* rng,
static int whTest_RevocationTryAESEncrypt(whClientContext* client,
whKeyId keyId, WC_RNG* rng,
int* encryptRes)
{
int ret;
Expand Down Expand Up @@ -584,7 +585,7 @@ static int _whTest_CryptoKeyRevocationAesCbc(whClientContext* client)
return ret;
}

ret = whTest_RevocationTryAESEncrypt(keyId, rng, &encryptRes);
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);
if (ret != 0) {
WH_ERROR_PRINT("Failed to encrypt with unrevoked AES key: %d\n", ret);
(void)wh_Client_KeyEvict(client, keyId);
Expand All @@ -605,7 +606,7 @@ static int _whTest_CryptoKeyRevocationAesCbc(whClientContext* client)
return ret;
}

ret = whTest_RevocationTryAESEncrypt(keyId, rng, &encryptRes);
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);
if (ret != 0 || encryptRes != WH_ERROR_USAGE) {
WH_ERROR_PRINT(
"Encrypt with revoked AES key should fail (%d), got %d\n",
Expand All @@ -621,7 +622,7 @@ static int _whTest_CryptoKeyRevocationAesCbc(whClientContext* client)
return ret;
}

ret = whTest_RevocationTryAESEncrypt(keyId, rng, &encryptRes);
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);
if (ret != 0 || encryptRes != WH_ERROR_USAGE) {
WH_ERROR_PRINT(
"Encrypt with revoked AES key should fail (%d), got %d\n",
Expand Down Expand Up @@ -653,7 +654,7 @@ static int _whTest_CryptoKeyRevocationAesCbc(whClientContext* client)
(void)wc_FreeRng(rng);
return ret;
}
ret = whTest_RevocationTryAESEncrypt(keyId, rng, &encryptRes);
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);
if (ret != 0 || encryptRes != 0) {
WH_ERROR_PRINT(
"Failed to encrypt with unrevoked AES key (2nd time): %d\n", ret);
Expand All @@ -673,7 +674,7 @@ static int _whTest_CryptoKeyRevocationAesCbc(whClientContext* client)
(void)wc_FreeRng(rng);
return ret;
}
ret = whTest_RevocationTryAESEncrypt(keyId, rng, &encryptRes);
ret = whTest_RevocationTryAESEncrypt(client, keyId, rng, &encryptRes);
if (ret != 0 || encryptRes != WH_ERROR_USAGE) {
WH_ERROR_PRINT(
"Encrypt with revoked AES key should fail (%d), got %d\n",
Expand Down
136 changes: 136 additions & 0 deletions test-refactor/client-server/wh_test_crypto_keystore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,13 +839,149 @@ static int _whTest_NonExportableKeystore(whClientContext* ctx)
return 0;
}

#if defined(WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS)
Comment thread
Frauschi marked this conversation as resolved.

@Frauschi Frauschi Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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.

/* Committing a NONMODIFIABLE key leaves an object that
* wh_Nvm_DestroyObjectsChecked refuses to erase, so it occupies one NVM
* slot for the rest of the run. Gated like the keypolicy revocation test. */
static int _whTest_NonModifiableCommit(whClientContext* ctx)
{
int ret = 0;
whKeyId keyId = WH_KEYID_ERASED;
uint8_t key[WH_TEST_KEYSTORE_TEST_SZ] = {
0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45,
0x67, 0x89, 0xAB, 0xCD, 0xEF, 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54,
0x32, 0x10, 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10};
uint8_t exportedKey[WH_TEST_KEYSTORE_TEST_SZ] = {0};
uint8_t label[WH_NVM_LABEL_LEN] = "NonModifiableCommitKey";
uint8_t exportedLabel[WH_NVM_LABEL_LEN] = {0};
uint16_t exportedKeySize;

WH_TEST_PRINT("Testing non-modifiable commit enforcement...\n");

/* Test 1: first commit of a NONMODIFIABLE key stores it, and the commit
* leaves the slot cached, so a repeat commit is an overwrite attempt. */
ret = wh_Client_KeyCache(ctx, WH_NVM_FLAGS_NONMODIFIABLE, label,
sizeof(label), key, sizeof(key), &keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to cache non-modifiable key: %d\n", ret);
return ret;
}

ret = wh_Client_KeyCommit(ctx, keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed first commit of non-modifiable key: %d\n", ret);
return ret;
}

/* Test 2: re-committing over the stored non-modifiable object is denied */
ret = wh_Client_KeyCommit(ctx, keyId);
if (ret != WH_ERROR_ACCESS) {
WH_ERROR_PRINT("Non-modifiable key was re-committed unexpectedly: %d\n",
ret);
return -1;
}

WH_TEST_DEBUG_PRINT("Non-modifiable key re-commit correctly denied\n");

/* Test 3: the denial left the stored object intact. Evicting is allowed
* because the key is committed, so the export below must freshen it back
* out of NVM rather than read the surviving cache slot. */
ret = wh_Client_KeyEvict(ctx, keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to evict committed non-modifiable key: %d\n",
ret);
return ret;
}

exportedKeySize = sizeof(exportedKey);
ret = wh_Client_KeyExport(ctx, keyId, exportedLabel, sizeof(exportedLabel),
exportedKey, &exportedKeySize);
if (ret != 0) {
WH_ERROR_PRINT("Failed to export stored non-modifiable key: %d\n", ret);
return ret;
}

if (exportedKeySize != sizeof(key) ||
memcmp(key, exportedKey, exportedKeySize) != 0 ||
memcmp(label, exportedLabel, sizeof(label)) != 0) {
WH_ERROR_PRINT("Denied commit altered the stored key\n");
return -1;
}

WH_TEST_DEBUG_PRINT("Stored non-modifiable key unchanged after denial\n");

/* The key cannot be erased: wh_Nvm_DestroyObjectsChecked refuses a
Comment thread
Frauschi marked this conversation as resolved.
* NONMODIFIABLE object, so only the cache slot is reclaimed here. */
(void)wh_Client_KeyEvict(ctx, keyId);

/* Test 4: the denial does not depend on cache residency. With no slot
* left, the stored flags still decide, so commit reports ACCESS rather
* than the NOTFOUND raised by the missing slot. */
ret = wh_Client_KeyCommit(ctx, keyId);
if (ret != WH_ERROR_ACCESS) {
WH_ERROR_PRINT("Uncached non-modifiable commit not denied: %d\n", ret);
return -1;
}

WH_TEST_DEBUG_PRINT("Uncached non-modifiable commit correctly denied\n");

WH_TEST_PRINT("NON-MODIFIABLE COMMIT TEST SUCCESS\n");
return 0;
}
#endif /* WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS */

static int _whTest_ModifiableRecommit(whClientContext* ctx)
{
int ret = 0;
whKeyId keyId = WH_KEYID_ERASED;
uint8_t key[WH_TEST_KEYSTORE_TEST_SZ] = {
0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45,
0x67, 0x89, 0xAB, 0xCD, 0xEF, 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54,
0x32, 0x10, 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10};
uint8_t label[WH_NVM_LABEL_LEN] = "ModifiableCommitKey";

WH_TEST_PRINT("Testing modifiable commit is unaffected...\n");

/* A key without the flag still commits repeatedly */
ret = wh_Client_KeyCache(ctx, WH_NVM_FLAGS_NONE, label, sizeof(label), key,
sizeof(key), &keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to cache modifiable key: %d\n", ret);
return ret;
}

ret = wh_Client_KeyCommit(ctx, keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed first commit of modifiable key: %d\n", ret);
return ret;
}

ret = wh_Client_KeyCommit(ctx, keyId);
if (ret != 0) {
WH_ERROR_PRINT("Failed repeat commit of modifiable key: %d\n", ret);
return ret;
}

WH_TEST_DEBUG_PRINT("Modifiable key repeat commit allowed\n");

/* Clean up */
(void)wh_Client_KeyErase(ctx, keyId);

WH_TEST_PRINT("MODIFIABLE COMMIT TEST SUCCESS\n");
return 0;
}

int whTest_Crypto_Keystore(whClientContext* ctx)
{
/* A preceding suite may leave the DMA-preferred dispatch mode set; reset
* to the std path so this suite runs the same way in every config. */
(void)wh_Client_SetDmaMode(ctx, 0);
WH_TEST_RETURN_ON_FAIL(_whTest_KeyCache(ctx));
WH_TEST_RETURN_ON_FAIL(_whTest_NonExportableKeystore(ctx));
#if defined(WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS)
WH_TEST_RETURN_ON_FAIL(_whTest_NonModifiableCommit(ctx));
#endif
WH_TEST_RETURN_ON_FAIL(_whTest_ModifiableRecommit(ctx));
return 0;
}

Expand Down
3 changes: 2 additions & 1 deletion test-refactor/config/wolfhsm_cfg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@
#define WOLFHSM_CFG_SERVER_NVM_FLASH_LOG

/* WOLFHSM_CFG_TEST_ALLOW_PERSISTENT_NVM_ARTIFACTS is intentionally NOT
* defined here. Not implemented yet. */
* defined here: one NVM is shared by every test in a run. The persistent
* NVM artifacts CI job defines it on the command line instead. */

#define WOLFHSM_CFG_ENABLE_TIMEOUT

Expand Down
14 changes: 13 additions & 1 deletion wolfhsm/wh_server_keystore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,19 @@ int wh_Server_KeystoreCommitKey(whServerContext* server, whNvmId keyId);
/**

@Frauschi Frauschi Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [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".
 */

* @brief Commit a cached key to NVM with policy enforcement
*
* Runs keystore policy checks before committing.
* Runs keystore policy checks before committing. The verdict comes from the
* flags of the stored NVM object, not the cache slot, so an unchecked cache
* path cannot launder them.
*
* @param[in] server Server context
* @param[in] keyId Key ID to commit
* @return WH_ERROR_OK on success

@Frauschi Frauschi Aug 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [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.

* @return WH_ERROR_ACCESS if an NVM object already exists under keyId and
* carries WH_NVM_FLAGS_NONMODIFIABLE or WH_NVM_FLAGS_TRUSTED. Commit
* is therefore not idempotent for such keys: a repeat commit of
* unchanged bytes is refused, so a client retrying after a lost
* response must treat WH_ERROR_ACCESS as "already committed".
* @return WH_ERROR_NOTFOUND if the key is in neither cache nor NVM
*/
int wh_Server_KeystoreCommitKeyChecked(whServerContext* server, whNvmId keyId);

Expand Down
Loading