Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the wolfHSM SHA2 client/server “wire format” and APIs to support multi-block variable-length SHA requests and introduces async Request/Response split operations (including DMA variants), with accompanying test expansion and POSIX example buffer sizing updates.
Changes:
- Redesign SHA256/SHA224 and SHA512/SHA384 request messages to carry variable-length trailing input (
inSz) instead of fixedinBlock/lastBlockLen. - Add async SHA{256,224,384,512} Request/Response client APIs (plus DMA async variants) and update server handlers + message translation accordingly.
- Expand crypto tests to cover large inputs, capacity boundaries, and new async flows; increase POSIX example comm/transport buffer sizing.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
wolfhsm/wh_message_crypto.h |
Updates SHA2 request/DMA message structs for variable-length trailing input; adds inline-capacity macros and static asserts. |
wolfhsm/wh_client_crypto.h |
Adds public async SHA2 Request/Response API declarations and DMA async prototypes. |
wolfhsm/wh_client.h |
Adds per-operation DMA async context storage to survive Request/Response for POST cleanup. |
src/wh_message_crypto.c |
Updates request/response translation to match new SHA2 message layouts and new DMA request types. |
src/wh_server_crypto.c |
Updates SHA2 handlers to validate inSz, process variable-length input, and rework DMA SHA2 handling with inline state. |
src/wh_client_crypto.c |
Implements async SHA2 APIs (non-DMA + DMA), refactors blocking wrappers to use them. |
test/wh_test_crypto.c |
Adds reference hashes and extensive large-input + async (DMA and non-DMA) SHA2 tests. |
test/wh_test_check_struct_padding.c |
Updates padding checks for renamed DMA request structs. |
examples/posix/wh_posix_server/wolfhsm_cfg.h |
Sets comm data length to 8 KiB to match client. |
examples/posix/wh_posix_client/wolfhsm_cfg.h |
Sets comm data length to 8 KiB. |
examples/posix/wh_posix_cfg.h |
Adjusts SHM request/response buffer sizes to carry the larger comm packets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bigbrett
force-pushed
the
async-crypto-v2
branch
from
f12c904 to
b19155f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Contract: at most one outstanding async SHA256 request per wc_Sha256 | ||
| * instance. Caller MUST call wh_Client_Sha256UpdateResponse if and only if | ||
| * *requestSent is true before issuing another Request on the same sha. |
There was a problem hiding this comment.
The async SHA256 contract here says only one outstanding request per wc_Sha256 instance, but wh_Client_SendRequest stores last_req_kind/last_req_id on the whClientContext, so issuing any other async request on the same ctx before calling the matching Response will break response validation. The contract should be stated in terms of whClientContext (or explicitly note both constraints).
| * Contract: at most one outstanding async SHA256 request per wc_Sha256 | |
| * instance. Caller MUST call wh_Client_Sha256UpdateResponse if and only if | |
| * *requestSent is true before issuing another Request on the same sha. | |
| * Contract: at most one outstanding async request may exist per | |
| * whClientContext. If *requestSent is true, caller MUST call | |
| * wh_Client_Sha256UpdateResponse before issuing another async Request on the | |
| * same ctx. The caller must also not issue another Request on the same sha | |
| * until that matching Response has completed. |
| ret = wh_MessageCrypto_TranslateSha256DmaRequest( | ||
| magic, (const whMessageCrypto_Sha256DmaRequest*)cryptoDataIn, &req); |
There was a problem hiding this comment.
Potential unaligned 64-bit access: cryptoDataIn points to the algorithm-specific struct immediately after whMessageCrypto_GenericRequestHeader (12 bytes) which starts at whCommHeader+8. This makes cryptoDataIn typically 4-bytes mod 8, but whMessageCrypto_Sha256DmaRequest contains uint64_t DMA fields that wh_MessageCrypto_TranslateSha256DmaRequest/TranslateDmaBuffer read via WH_T64. On strict-alignment targets this can fault. Consider memcpy'ing the fixed-size request header into an aligned local before translation, or changing DMA translation to use memcpy-based loads for uint64 fields instead of direct struct access.
| ret = wh_MessageCrypto_TranslateSha512DmaRequest( | ||
| magic, (const whMessageCrypto_Sha512DmaRequest*)cryptoDataIn, &req); |
There was a problem hiding this comment.
Potential unaligned 64-bit access: same issue as the SHA256 DMA handler. cryptoDataIn is offset by 12 bytes (GenericRequestHeader), so casting it to whMessageCrypto_Sha512DmaRequest and translating uint64 DMA fields via WH_T64 can fault on strict-alignment architectures. Prefer memcpy into an aligned local struct first, or update DMA translation helpers to use memcpy-based loads for uint64 fields.
| ret = wh_MessageCrypto_TranslateDmaBuffer(magic, &src->input, &dest->input); | ||
| if (ret != 0) { | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
These DMA request translations ultimately read uint64 DMA buffer fields from src via WH_T64 (in wh_MessageCrypto_TranslateDmaBuffer). Because the on-wire layout places DMA request structs immediately after a 12-byte GenericRequestHeader, src is typically not 8-byte aligned. On strict-alignment targets, direct uint64 loads from an unaligned src are undefined and may fault. Consider changing DMA buffer translation to use memcpy-based loads/stores for uint64 fields (and/or require/pad the generic header to 16 bytes).
| - **No message format changes**: the same wire protocol is used; only the | ||
| client-side calling pattern changes. |
There was a problem hiding this comment.
The design principle "No message format changes" conflicts with the rest of this document, which specifies a new SHA wire layout (variable-length trailing input with inSz, and new DMA request structs). Consider rewording this bullet to reflect that the async API may introduce new message formats, or scope the claim to algorithms where the wire protocol is unchanged.
| - **No message format changes**: the same wire protocol is used; only the | |
| client-side calling pattern changes. | |
| - **Preserve existing wire formats where possible**: for operations whose | |
| request/response layout is already suitable, the async API only changes the | |
| client-side calling pattern. Some algorithms may still require new message | |
| layouts to carry async-specific inputs or DMA metadata. |
| * Contract: at most one outstanding async SHA224 request per wc_Sha224 | ||
| * instance. Caller MUST call wh_Client_Sha224UpdateResponse if and only if | ||
| * *requestSent is true before issuing another Request on the same sha. |
There was a problem hiding this comment.
Same as SHA256: the contract should reflect that only one async request can be in flight per whClientContext because response matching uses ctx->last_req_kind/last_req_id. As written, this suggests multiple hashes could overlap on the same ctx as long as they are different wc_Sha224 instances, which would be incorrect.
| * Contract: at most one outstanding async SHA224 request per wc_Sha224 | |
| * instance. Caller MUST call wh_Client_Sha224UpdateResponse if and only if | |
| * *requestSent is true before issuing another Request on the same sha. | |
| * Contract: at most one outstanding async request per whClientContext. | |
| * Caller MUST call wh_Client_Sha224UpdateResponse if and only if | |
| * *requestSent is true before issuing another async Request on the same ctx, | |
| * regardless of which wc_Sha224 instance is used. |
| * Contract: at most one outstanding async SHA384 request per wc_Sha384 | ||
| * instance. Caller MUST call wh_Client_Sha384UpdateResponse if and only if | ||
| * *requestSent is true before issuing another Request on the same sha. |
There was a problem hiding this comment.
Same as SHA256: the contract should reflect that only one async request can be in flight per whClientContext because response matching uses ctx->last_req_kind/last_req_id. As written, this suggests multiple hashes could overlap on the same ctx as long as they are different wc_Sha384 instances, which would be incorrect.
| * Contract: at most one outstanding async SHA384 request per wc_Sha384 | |
| * instance. Caller MUST call wh_Client_Sha384UpdateResponse if and only if | |
| * *requestSent is true before issuing another Request on the same sha. | |
| * Contract: at most one outstanding async request may be in flight per | |
| * whClientContext. Caller MUST call wh_Client_Sha384UpdateResponse if and | |
| * only if *requestSent is true before issuing any other async Request on | |
| * the same ctx, even when using a different wc_Sha384 instance. |
| * Contract: at most one outstanding async SHA512 request per wc_Sha512 | ||
| * instance. Caller MUST call wh_Client_Sha512UpdateResponse if and only if | ||
| * *requestSent is true before issuing another Request on the same sha. |
There was a problem hiding this comment.
Same as SHA256: the contract should reflect that only one async request can be in flight per whClientContext because response matching uses ctx->last_req_kind/last_req_id. As written, this suggests multiple hashes could overlap on the same ctx as long as they are different wc_Sha512 instances, which would be incorrect.
| * Contract: at most one outstanding async SHA512 request per wc_Sha512 | |
| * instance. Caller MUST call wh_Client_Sha512UpdateResponse if and only if | |
| * *requestSent is true before issuing another Request on the same sha. | |
| * Contract: at most one outstanding async request may be in flight per | |
| * whClientContext. If *requestSent is true, the caller MUST call | |
| * wh_Client_Sha512UpdateResponse before issuing any other async request on | |
| * the same ctx, including a request using a different wc_Sha512 instance. |
bigbrett
force-pushed
the
async-crypto-v2
branch
from
67f2927 to
6c7e032
Compare
bigbrett
force-pushed
the
async-crypto-v2
branch
from
6c7e032 to
7ef52c5
Compare
set credentials messaging login messaging and demo login variable length for set credentials message and logout/login touch up demo using certificates for login credentials add delete user and get user implementations add demo for set permissions and update delete function to have current user check in test cases planned so far updates for dynmaic size of credentials and auth data check in wh_auth_base.h file add auth login and logout tests add sanity checks on username size add client only tcp auth tests adding bad function argument tests better server response to authorization error cases remove debug printf's and make note for future logging location run git-clang-format and checking format changes adding in more function comments move base example auth users to port/posix directory spelling fixes, cast on sizeof return, macro guard for certificate use, less verbose auth demos check in auth demo client files update action permissions and method in message layer fix for bitmask of permissions and remove permissions return from login add auth login as admin during SHE tests update posix client auth demo for new login function signature touch up of comments and demo Fix typo and remove redundent return value check account for no WOLFHSM_CFG_ENABLE_SERVER build with test case addressing some feedback about sanity checks and null string terminators update login comments and add defensive memset's add hashing of example pin, use of WH_ERROR_OK, update comment make the authentication feature off by default and enabled by defining WOLFHSM_CFG_ENABLE_AUTHENTICATION add server simple response back of auth not enabled add WH_TEST_SKIP and authentication skipping when server does not support auth move most of authentication logic into wolfHSM rather than in port, touch up for test cases fix for scan-build warning spelling fix, additional sanity checks add flag to avoid gcc coverage bug 68080 use boolean array for group permissions, fix permissions bitmask, remove unused enum add locks around user login / modify sections of code for thread safety move base implementation from port/posix to src/wh_auth_base.c and add test that authorization callback override is being called when set git-clang-format, add additional sanity checks, add lock for auth check add admin boolean to permissions, aditional delete user test case, unique max credentials, add force zero to utils add a WH_MESSAGE_GROUP_MAX and use it to get the number of groups add constant compare utils function, permissions helper macros, fixes to auth translation layer, admin user add restriction, duplicate user name restriction add authentication documentation sanity checks on input and adjust return value add more macro guards around authentication feature, zero out sensitive data before function return add more force zero calls and remove dead code path results from git-clang-format remove const on input buffer that gets zeroized adjust test case to account of SECEVNT log when auth enabled but not set fix for duplicate user check and update documentation to clarify auth is a experimental feature in development
authentication manager feature addition
add wolfBoot image verification to image manager
No description provided.