Conversation
There was a problem hiding this comment.
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.
AlexLanzano
left a comment
There was a problem hiding this comment.
Initial high level review. Just some minor questions and concerns. Great work otherwise
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eywrap compatibility. Key changes: - Add WOLFHSM_CFG_GLOBAL_KEYS feature flag and configuration - Implement global key cache in NVM context separate from local caches - Add keyId translation layer between client flags and server encoding - Create unified cache routing infrastructure for local/global keys - Add comprehensive multi-client test suite with 15+ test cases - Update all crypto/keystore operations to support global keys - Major refactor to keywrap feature to add clientId-based access control and addiional test coverage for wrap/unwrap scenarios with global and local keys - Standardize client_id usage across benchmarks and tests - Add new wh_keyid module for keyId manipulation helpers
billphipps
left a comment
There was a problem hiding this comment.
Excellent addition and awesome testing! I had a few questions and nits, but nothing that would stop this from being merged.
- Renamed whServerCacheXXX to whKeyCacheXXX - Relocated client global+wrapped flags to wh_keyid.h from wh_client.h - Fixed copyright year - Fixed wh_settings.h include order
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.
Also refactors wrapped keys to use a similar signaling mechanism to the server, such that they can be stored as a different keytype, enabling them to have the same client-facing IDs as regular keys, meaning the feature is able to coexist with dynamic ID generation for non-wrapped keys.
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