Skip to content

Conversation

@bigbrett
Copy link
Contributor

@bigbrett bigbrett commented Oct 22, 2025

Adds support for global keys, enabling cryptographic keys to be shared across multiple wolfHSM clients. When a key is marked as global, it becomes accessible to all clients rather than being isolated to the client that cached it. The global keycache is currently located in the NVM context, as this is the only global state accessible from every server struct instance, and also serves to explicitly indicate the NVM (if there were to be multiple instances) that should be used as the backing store.

Feature Additions

  • Refactors key cache into its own structure that can be instantiated either locally (in a server context) or globally (in the NVM context)
  • Adds new global keycache to NVM struct and new keyId translation macros that handle managing client's global indicator flag
  • Refactors crypto and keystore layers to properly handle global keys
  • Adds new multi-client test harness, currently supporting only sequential operations, and adds global key tests to it
  • Adds default keyId for tests that don't care about it

Details

  • Global keys stored in globalCache in NVM context (shared across all servers)
  • Local keys remain in per-client localCache in server context
  • Global keys are indicated to the server by the client setting the WH_KEYID_GLOBAL bit in the keyId (bit 8) when caching/using global keys (e.g. a client specifying keyId of 0x1005 indicates global key 5, whereas 0x0005 indicates client-local key 5)
  • Server strips the flag and associates key with USER=WH_KEYUSER_GLOBAL==0 encoding via WH_TRANSLATE_CLIENT_KEYID macro
  • Cache routing via _GetCacheContext() helper allows the server to automatically select the appropriate cache based on the updated user field
  Client API:                                                                                                                                                                                                                                                                                                                 
  // Mark key as global                                                                                                                                                                                                                                                                                                       
  whKeyId globalKey = WH_MAKE_KEYID_GLOBAL(5);                                                                                                                                                                                                                                                                                
  wh_Client_KeyCache(client, globalKey, data, len, ...);                                                                                                                                                                                                                                                                      
                                                                                                                                                                                                                                                                                                                              
  // Regular local key (not shared)                                                                                                                                                                                                                                                                                           
  whKeyId localKey = 10;                                                                                                                                                                                                                                                                                                      
  wh_Client_KeyCache(client, localKey, data, len, ...);

@bigbrett bigbrett requested a review from Copilot October 22, 2025 16:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements global keystore functionality, enabling cryptographic keys to be shared across multiple wolfHSM clients. The feature refactors the key cache architecture to support both client-local and globally accessible keys, with the global cache residing in the NVM context to ensure visibility across all server instances.

Key changes:

  • Refactors cache structures into a unified whKeyCacheContext that can be instantiated globally or locally
  • Adds client-facing macros (WH_MAKE_KEYID_GLOBAL) and server-side translation logic (WH_TRANSLATE_CLIENT_KEYID) to manage global key routing
  • Implements comprehensive multi-client test suite validating global key operations, isolation, and persistence

Reviewed Changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
wolfhsm/wh_server_cache.h New header defining unified cache structures to avoid circular dependencies
wolfhsm/wh_common.h Adds global key flag definitions and client-to-server keyId translation macros
wolfhsm/wh_nvm.h Adds global cache field to NVM context when feature enabled
wolfhsm/wh_server.h Refactors server to use unified cache structure for local keys
wolfhsm/wh_client.h Adds client-facing helper macro for marking keys as global
wolfhsm/wh_settings.h Documents new configuration option
src/wh_server_keystore.c Core refactor implementing cache routing and global key support
src/wh_server_crypto.c Updates all crypto operations to use translation macro
src/wh_server_cert.c Updates certificate operations for global key support
src/wh_server.c Adds validation preventing client_id=0 (reserved for global keys)
src/wh_nvm.c Initializes global cache during NVM setup
test/wh_test_multiclient.{h,c} New multi-client test framework and comprehensive test suite
test/wh_test_*.c Updates tests to use default client ID constant
test/config/wolfhsm_cfg.h Enables global keys for testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

* into it */
rc = wh_Server_KeystoreGetCacheSlot(server, cacheBufSize,
&cacheBuf, &cacheMeta);
rc = wh_Server_KeystoreGetCacheSlot(server, *inout_keyId,
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to make wh_Server_KeystoreGetCacheSlot_ex if possible since this will cause build failures downstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good note, I'm on the fence though.... mostly since we haven't made any API compatibility guarantees yet, and if we were to, it would likely be limited to the client API. I'd like to avoid a rats nest of _ex() functions this early in the codebase's lifetime :)

* global" via the wrapped metadata. */
metadata.id =
WH_MAKE_KEYID(WH_KEYTYPE_CRYPTO, server->comm->client_id, 0);
ret = wh_Server_KeystoreGetUniqueId(server, &metadata.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since wrapped keys are most likely to be stored in external flash and not managed by the server the key ids produced from this could collide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why would it matter whether or not it is stored in NVM accessible to the server?

Also, the line you highlighted is explictly not the global case, so the server does actually manage the keyIds in this case.

But for the global case, key collision is a very real concern, and I'm not sure what the best way to address it for wrapped keys is other than placing the burden of properly coordinating global keyIds across multiple clients entirely on the user. Since the global keyIds would be set by the client either offline or at "wrap" time, it is up to them to ensure they have a registry of global keys in the system. I imagine 99% of real-world utilization of wrapped keys deals with offline provisioning anyway so it should be easy enough to prevent against. I'm not sure what a server should do other than error if it unwraps two global keys, both with the same keyId.

Copy link
Contributor

@AlexLanzano AlexLanzano left a comment

Choose a reason for hiding this comment

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

Initial high level review. Just some minor questions and concerns. Great work otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants