-
Notifications
You must be signed in to change notification settings - Fork 22
Global keystore #224
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.
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
base: main
Are you sure you want to change the base?
Global keystore #224
Conversation
- Fix server bug where upper keyId bits were not masked off in response
There was a problem hiding this 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
whKeyCacheContextthat 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
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
Details
WH_KEYID_GLOBALbit in the keyId (bit 8) when caching/using global keys (e.g. a client specifying keyId of0x1005indicates global key 5, whereas0x0005indicates client-local key 5)WH_KEYUSER_GLOBAL==0encoding via WH_TRANSLATE_CLIENT_KEYID macro