Require verify result to be captured in client cert response handlers#426
Conversation
e885fe8 to
2e3352e
Compare
There was a problem hiding this comment.
Pull request overview
This PR enforces that certificate verification APIs always capture the server’s verification verdict by making out_rc mandatory (non-NULL) across client-side cert verify response handlers and synchronous helpers, addressing F-6199.
Changes:
- Updated public API documentation in
wh_client.hto stateout_rcis required and to clarify thatrc == 0does not imply a successful verification verdict. - Added argument validation in client cert verification code paths to reject
out_rc == NULLwithWH_ERROR_BADARGS. - Extended certificate client tests to ensure passing a NULL verdict pointer is rejected.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
wolfhsm/wh_client.h |
Updates API docs to require out_rc and clarify verification semantics. |
src/wh_client_cert.c |
Enforces out_rc non-NULL in cert verify response/dispatch helpers. |
test/wh_test_cert.c |
Adds regression coverage for rejecting out_rc == NULL in cert verify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| whMessageCert_VerifyResponse resp; | ||
|
|
||
| if (c == NULL) { | ||
| /* out_rc is mandatory; it carries the verification verdict */ |
There was a problem hiding this comment.
@padelsbach can we get rid of these comments in whatever PR you are working on next? Kinda just clutters things and unneccessary. But dont want to block this merge
There was a problem hiding this comment.
For future reference, the removal of these are in #430
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #426
Scan targets checked: wolfhsm-core-bugs, wolfhsm-crypto-bugs, wolfhsm-src
Findings: 1
Info (1)
Inner out_rc != NULL guards are now always-true dead conditions
File: src/wh_client_cert.c:431
Function: _certVerifyResponse
Category: Dead/unreachable code
The new top-level out_rc == NULL early return makes the pre-existing inner if (out_rc != NULL) guards unreachable when false in the response helpers (_certVerifyResponse, _certVerifyMultiRootResponse, _certVerifyDmaResponse, _certVerifyMultiRootDmaResponse, and the Acert responders). Harmless but now-redundant.
Recommendation: Optionally drop the now-redundant inner NULL guard around the *out_rc = resp.rc assignment.
Referenced code: src/wh_client_cert.c:431-433 (3 lines)
This review was generated automatically by Fenrir. Findings are non-blocking.
Addresses F-6199. Caller is no longer allowed to pass
out_rc == NULLsince this value contains the actual verification result. This breaks convention within wolfHSM, admittedly.