cache-only shared secrets#372
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds cache-mode shared-secret support so ECDH and X25519 outputs can remain server-resident instead of always being returned to the client.
Changes:
- Extends ECDH and Curve25519 request/response messages with cache flags, output key IDs, and labels.
- Adds client/server handling for cached shared-secret outputs using raw key-cache import.
- Adds crypto tests covering cached ECDH/X25519 secrets, caller-supplied IDs, NONEXPORTABLE behavior, and guard cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
wolfhsm/wh_server_crypto.h |
Declares raw key-cache import helper for shared-secret/KDF outputs. |
wolfhsm/wh_message_crypto.h |
Extends ECDH and Curve25519 message structs for cache-mode semantics. |
wolfhsm/wh_client_crypto.h |
Adds public cache-mode shared-secret APIs and async Curve25519 shared-secret APIs. |
src/wh_server_crypto.c |
Implements cache-mode ECDH/X25519 server handling and raw cache import helper. |
src/wh_message_crypto.c |
Translates new message fields. |
src/wh_client_crypto.c |
Refactors shared-secret client paths and adds cache-key request/response wrappers. |
test/wh_test_crypto.c |
Adds ECDH and X25519 cache-mode shared-secret coverage. |
test/wh_test_check_struct_padding.c |
Includes Curve25519 message structs in padding checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #372
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
No new issues found in the changed files. ✅
| } | ||
| /* Scrub the secret from the response buffer regardless of import | ||
| * success/failure. */ | ||
| memset(res_out, 0, res_len); |
There was a problem hiding this comment.
would you consider getting a cache slot with wh_Server_KeystoreGetCacheSlotChecked before the wc_ecc_shared_secret operation to avoid a copy and a memset?
There was a problem hiding this comment.
OK I thought about this but it means that the crypto would have to occur inside the critical section, otherwise there could be a race on getting a cache slot for global keys. For now I think its best to follow the pattern of the other handlers that work similarly. Would rather pay for a small copy inside the critical section than pay for a public key crypto operation
| out_key_id = wh_KeyId_TranslateFromClient( | ||
| WH_KEYTYPE_CRYPTO, ctx->comm->client_id, req.keyId); | ||
| if (WH_KEYID_ISERASED(out_key_id)) { | ||
| ret = wh_Server_KeystoreGetUniqueId(ctx, &out_key_id); |
There was a problem hiding this comment.
this is probably racey under THREAD_SAFE
Extend EccSharedSecret and Curve25519SharedSecret APIs with CacheKey variants (sync + async request/response) that store the derived secret in a server key cache slot instead of returning it to the client. Adds flags/keyId/label fields to the ECDH and Curve25519 messages, a shared wh_Server_KeyCacheImportRaw helper, and crypto tests covering both paths.
When the blocking CacheKey API auto-imports local input keys, the server allocates their keyIds from WH_KEYID_IDMAX downward (0xFF, 0xFE, ...). If the caller picks an output keyId that collides with one of those auto-imported inputs, the EVICTPUB/EVICTPRV cleanup runs after the import and deletes the cache slot we just wrote the secret to, while still returning success and the keyId. Clear the matching evict flag after a successful import so cleanup leaves the cached secret alone
bigbrett
force-pushed
the
cached-shared-secret
branch
from
241948c to
362f093
Compare
This PR adds cache-mode shared-secret support so ECDH and X25519 outputs can remain server-resident instead of always being returned to the client.
Changes: