Conversation
bigbrett
force-pushed
the
wrapped-cached-certs
branch
from
7f3ddc0 to
5ee1a25
Compare
There was a problem hiding this comment.
Pull request overview
Adds “wrapped cached certificates” support so clients can store trusted roots as AES-GCM wrapped blobs and have the server unwrap/cache them in RAM for cert verification, rather than requiring NVM-resident roots.
Changes:
- Added client-side cert wrap/unwrap/export/cache convenience APIs (guarded by
WOLFHSM_CFG_KEYWRAP). - Updated server cert manager + request handlers to route trusted-root reads/verifies to keystore cache when IDs indicate wrapped/cached certs.
- Added documentation and new tests for cached-root verify/readtrusted (including DMA variants).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
wolfhsm/wh_server_cert.h |
Updates cert APIs to accept whKeyId and documents cache vs NVM behavior. |
src/wh_server_cert.c |
Implements cache-aware trusted-root reads and request-handler routing for wrapped IDs. |
wolfhsm/wh_client.h |
Adds public client API declarations for cert wrap/unwrap/export/cache helpers. |
src/wh_client_cert.c |
Implements the new client cert wrap/unwrap helper functions via keywrap APIs. |
test/wh_test_cert.c |
Adds tests for cached-root verify/readtrusted, including DMA coverage. |
docs/draft/wrapped-certs.md |
Adds draft documentation for workflow, ID encoding, and handler routing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @brief Get a trusted certificate from NVM storage | ||
| * @brief Get a trusted certificate from NVM storage or keystore cache | ||
| * @param server The server context | ||
| * @param id The NVM ID of the certificate to read | ||
| * @param id The key ID of the certificate to read. If the key type is | ||
| * WH_KEYTYPE_NVM, the certificate is read from NVM. Otherwise, the certificate | ||
| * is read from the keystore cache (e.g. for wrapped certs cached via | ||
| * wh_Client_KeyUnwrapAndCache). | ||
| * @param cert Buffer to store the certificate data | ||
| * @param inout_cert_len On input, size of cert buffer. On output, actual cert | ||
| * size | ||
| * @return WH_ERROR_OK on success, error code on failure. If certificate is too | ||
| * large for the buffer, WH_ERROR_BUFFER_SIZE will be returned and | ||
| * inout_cert_len will be updated to the actual certificate size. |
There was a problem hiding this comment.
The doc says that on buffer-too-small, WH_ERROR_BUFFER_SIZE is returned and inout_cert_len is updated to the actual cert size. wh_Server_CertReadTrusted currently doesn't update *inout_cert_len on error (and the cache path may return WH_ERROR_NOSPACE). Please align either the documentation or the implementation so callers can rely on consistent sizing/error behavior.
|
|
||
| 1. **Translate the client ID**: If the incoming `req.id` (or | ||
| `req.trustedRootNvmId`) has `WH_KEYID_CLIENT_WRAPPED_FLAG` set, call | ||
| `wh_KeyId_TranslateFromClient(WH_KEYTYPE_NVM, server->comm->client_id, req.id)` |
There was a problem hiding this comment.
This doc example calls wh_KeyId_TranslateFromClient(WH_KEYTYPE_NVM, ...) but the text says it should produce an internal key ID with TYPE=WH_KEYTYPE_WRAPPED. The first argument should reflect the wrapped key type (or match what the server code actually passes) to avoid confusing readers.
| `wh_KeyId_TranslateFromClient(WH_KEYTYPE_NVM, server->comm->client_id, req.id)` | |
| `wh_KeyId_TranslateFromClient(WH_KEYTYPE_WRAPPED, server->comm->client_id, req.id)` |
| /* Wrap the root CA cert */ | ||
| whNvmMetadata certMeta = {0}; | ||
| certMeta.id = WH_CLIENT_KEYID_MAKE_WRAPPED_META(client->comm->client_id, 7); | ||
| certMeta.flags = WH_NVM_FLAGS_USAGE_ANY; | ||
| certMeta.access = WH_NVM_ACCESS_ANY; | ||
| WH_TEST_RETURN_ON_FAIL(wh_Client_CertWrap( |
There was a problem hiding this comment.
This function declares whNvmMetadata certMeta after executable statements, which breaks C90 (-std=c90) compilation (declarations must come before statements in a block). Move certMeta (and any other declarations) to the start of the function body.
| /* Wrap the root CA cert for client-side storage. | ||
| * certMeta.id must have WH_KEYTYPE_WRAPPED set in metadata. */ | ||
| whNvmMetadata certMeta = {0}; | ||
| certMeta.id = WH_CLIENT_KEYID_MAKE_WRAPPED_META(client->comm->client_id, 5); | ||
| certMeta.flags = WH_NVM_FLAGS_USAGE_ANY; | ||
| certMeta.access = WH_NVM_ACCESS_ANY; | ||
| WH_TEST_PRINT("Wrapping root certificate...\n"); |
There was a problem hiding this comment.
This function declares whNvmMetadata certMeta mid-block (after statements). The test suite is built with C90, so declarations must appear before any statements in the block. Please move certMeta to the top with the other locals.
| if (rc == WH_ERROR_OK) { | ||
| *inout_cert_len = sz; | ||
| } |
There was a problem hiding this comment.
The cache path delegates to wh_Server_KeystoreReadKey, which returns different error codes/semantics than the NVM path (e.g., WH_ERROR_NOSPACE instead of WH_ERROR_BUFFER_SIZE) and does not update *inout_cert_len on undersized buffers. To keep wh_Server_CertReadTrusted's contract consistent for callers, map cache errors to the cert API's documented errors and ensure *inout_cert_len is set to the required cert length on buffer-too-small returns.
| if (rc == WH_ERROR_OK) { | |
| *inout_cert_len = sz; | |
| } | |
| if (rc == WH_ERROR_OK) { | |
| /* On success, report the actual size read back to the caller. */ | |
| *inout_cert_len = sz; | |
| return WH_ERROR_OK; | |
| } | |
| /* Map keystore buffer-too-small error to the cert API's documented | |
| * error and report the required size via *inout_cert_len, matching | |
| * the behavior of the NVM path. */ | |
| if (rc == WH_ERROR_NOSPACE) { | |
| /* meta.len is expected to contain the full certificate size. */ | |
| if (meta.len <= WOLFHSM_CFG_MAX_CERT_SIZE) { | |
| *inout_cert_len = meta.len; | |
| } | |
| return WH_ERROR_BUFFER_SIZE; | |
| } | |
| /* For all other errors, propagate the keystore error code. */ |
| if WH_KEYID_TYPE(id) == WH_KEYTYPE_WRAPPED | ||
| → wh_Server_KeystoreReadKey(server, id, &meta, cert, &sz) // cache path | ||
| else | ||
| → wh_Nvm_GetMetadata / wh_Nvm_Read // NVM path (unchanged) |
There was a problem hiding this comment.
The pseudo-code here says the cache path is taken for any WH_KEYID_TYPE(id) != WH_KEYTYPE_NVM, but the current implementation in wh_Server_CertReadTrusted only routes to the cache for WH_KEYTYPE_WRAPPED. Update this section to match the actual branching logic (or update the implementation if broader routing is intended).
| if WH_KEYID_TYPE(id) == WH_KEYTYPE_WRAPPED | |
| → wh_Server_KeystoreReadKey(server, id, &meta, cert, &sz) // cache path | |
| else | |
| → wh_Nvm_GetMetadata / wh_Nvm_Read // NVM path (unchanged) | |
| switch (WH_KEYID_TYPE(id)) | |
| case WH_KEYTYPE_WRAPPED: | |
| → wh_Server_KeystoreReadKey(server, id, &meta, cert, &sz) // cache path | |
| break | |
| case WH_KEYTYPE_NVM: | |
| → wh_Nvm_GetMetadata / wh_Nvm_Read // NVM path (unchanged) | |
| break | |
| default: | |
| → reject id / return error for unsupported key type |
| ### What changes for cached certs | ||
|
|
||
| Cached certs do not touch NVM at all — they are read from the in-memory key | ||
| cache via `wh_Server_KeystoreReadKey`. However, the NVM lock is still | ||
| unconditionally acquired around both cache and NVM paths. This is conservative | ||
| but correct: the key cache (`localCache` / `globalCache`) does not have its own | ||
| lock, so the NVM lock serves as the coarse serialization mechanism for all | ||
| server-side storage operations (both NVM and cache) when | ||
| `WOLFHSM_CFG_THREADSAFE` is enabled. | ||
|
|
There was a problem hiding this comment.
This section claims the handlers now use conditional NVM locking (and may skip locking when server->nvm == NULL), but the updated cert handlers in src/wh_server_cert.c still call WH_SERVER_NVM_LOCK(server) unconditionally even for the cache path. Please adjust the documentation or the code so the described locking behavior matches reality.
| /* Wrap the root CA cert */ | ||
| whNvmMetadata certMeta = {0}; | ||
| certMeta.id = WH_CLIENT_KEYID_MAKE_WRAPPED_META(client->comm->client_id, 6); | ||
| certMeta.flags = WH_NVM_FLAGS_USAGE_ANY; | ||
| certMeta.access = WH_NVM_ACCESS_ANY; | ||
| WH_TEST_RETURN_ON_FAIL(wh_Client_CertWrap( |
There was a problem hiding this comment.
whNvmMetadata certMeta is declared after statements in this block, which violates the project's C90 requirement (declarations before statements). Move this declaration to the beginning of whTest_CertCacheReadTrusted.
| int wh_Client_CertUnwrapAndExportResponse(whClientContext* ctx, | ||
| enum wc_CipherType cipherType, | ||
| whNvmMetadata* metadataOut, | ||
| uint8_t* certOut, | ||
| uint16_t* inout_certSz) | ||
| { | ||
| if ((ctx == NULL) || (certOut == NULL) || (inout_certSz == NULL)) { | ||
| return WH_ERROR_BADARGS; | ||
| } | ||
|
|
||
| return wh_Client_KeyUnwrapAndExportResponse(ctx, cipherType, metadataOut, | ||
| certOut, inout_certSz); |
There was a problem hiding this comment.
wh_Client_CertUnwrapAndExportResponse doesn't validate metadataOut, but the underlying wh_Client_KeyUnwrapAndExportResponse requires it to be non-NULL and will return WH_ERROR_BADARGS. Either enforce metadataOut != NULL here (and in the blocking wrapper) or clearly document/handle the NULL case.
| int rc; | ||
|
|
||
| if ((ctx == NULL) || (wrappedCert == NULL) || (wrappedCertSz == 0) || | ||
| (certOut == NULL) || (inout_certSz == NULL)) { | ||
| return WH_ERROR_BADARGS; | ||
| } |
There was a problem hiding this comment.
wh_Client_CertUnwrapAndExport allows metadataOut == NULL, but the delegated keywrap API requires non-NULL metadata output. Add a NULL check for metadataOut in the argument validation so this wrapper doesn't advertise a call pattern that can never succeed.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #318
Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
Findings: 1
Medium (1)
DMA READTRUSTED cache path reads cert data before non-exportable check
File: src/wh_server_cert.c:652-670
Function: wh_Server_HandleCertRequest
Category: Logic errors
In the WH_MESSAGE_CERT_ACTION_READTRUSTED_DMA handler, the new cache path calls wh_Server_KeystoreReadKey which writes the full certificate plaintext into cert_data (a DMA-mapped client-writable buffer obtained via wh_Server_DmaProcessClientAddress) before checking WH_NVM_FLAGS_NONEXPORTABLE on the metadata. In contrast, the NVM path in the same handler correctly calls wh_Nvm_GetMetadata first and only reads the certificate data (via wh_Server_CertReadTrusted) if the non-exportable flag is not set. This means for cached wrapped certs marked non-exportable, the plaintext is written to the DMA buffer before resp.rc is set to WH_ERROR_ACCESS. The non-DMA READTRUSTED handler has the same read-before-check ordering in its cache path, but cert_data is a server-local buffer there and resp.cert_len is not set on error, so the data is not included in the response.
if (req.id & WH_KEYID_CLIENT_WRAPPED_FLAG) {
/* Cache path: translate and read cert + metadata */
whKeyId certId = wh_KeyId_TranslateFromClient(
WH_KEYTYPE_WRAPPED, server->comm->client_id,
req.id);
cert_len = req.cert_len;
resp.rc = wh_Server_KeystoreReadKey(
server, certId, &meta, cert_data, &cert_len);
if (resp.rc == WH_ERROR_OK) {
if ((meta.flags & WH_NVM_FLAGS_NONEXPORTABLE) !=
0) {
resp.rc = WH_ERROR_ACCESS;
}
}
}Recommendation: Read the cached cert into a temporary server-side buffer first, check WH_NVM_FLAGS_NONEXPORTABLE on the returned metadata, and only copy to the DMA cert_data buffer if the cert is exportable. This matches the check-before-read pattern used in the NVM path. Alternatively, if wh_Server_KeystoreReadKey (or a variant) supports returning metadata without data (e.g., by passing NULL for the data buffer), use that to check exportability before the full read.
This review was generated automatically by Fenrir. Findings are non-blocking.
Allows clients to use trusted root certificates that are AES-GCM wrapped and cached in server RAM rather than committed to NVM. The client stores the wrapped blob cheaply and unwraps it on demand for verification.
Changes