Add WOLFSSL_CHAIN_VERIFY_CB to replace peer chain verification - #11367
Open
julek-wolfssl wants to merge 7 commits into
Open
Add WOLFSSL_CHAIN_VERIFY_CB to replace peer chain verification#11367julek-wolfssl wants to merge 7 commits into
julek-wolfssl wants to merge 7 commits into
Conversation
Adds an opt-in callback that takes over the peer certificate trust decision. wolfSSL builds no chain, verifies no signature and checks no date, revocation status, key usage or host name; the verify callback is not called either. The callback gets only the DER certificates from the Certificate message as WOLFSSL_BUFFER_INFO entries, certs[0] first. It is consulted even under WOLFSSL_VERIFY_NONE, and its rejection fails the handshake either way. wolfSSL still decodes every certificate with ParseCert(NO_VERIFY) before the callback runs, so malformed DER fails the handshake and the callback is never handed it. Content wolfSSL does not understand - an unknown critical extension, an unsupported key or signature algorithm, a signature algorithm mismatch - is not a decoding failure and is left for the callback to judge. The check runs once, not again on re-entry after a deferred verdict. The callback may return CHAIN_VERIFY_WANT_E to suspend the handshake and be asked again with the same certificates when the application re-enters wolfSSL_connect(), wolfSSL_accept(), wolfSSL_read() or wolfSSL_write(). A certificate received after the handshake (TLS 1.3 post-handshake authentication) arrives inside wolfSSL_read() and is resumed by reading again: ReceiveData() re-enters on any handshake-suspend error, and the ticket-sent accept state leaves a suspended message alone. Rejection fails with CHAIN_VERIFY_CB_E and one bad_certificate alert, and sets peerVerifyRet so wolfSSL_get_verify_result() does not report X509_V_OK. Still applied: an empty Certificate message keeps the existing mutual-auth policy (the callback is not called for it), and the minimum peer key sizes still gate the peer's own key, which the handshake uses directly. DTLS, raw public keys and OCSP stapling are not supported with the callback. wolfSSL_CTX_SetChainVerifyCb() and wolfSSL_SetChainVerifyCb() refuse with CHAIN_VERIFY_UNSUPPORTED_E when the context or object is already configured for one of them, and a connection that uses one of them anyway fails with the same error and an internal_error alert before the callback is called. The DTLS paths are otherwise untouched. The callback and its user context can be set on the context or on the SSL object, the object's taking precedence. The feature is part of --enable-all, has a CMake option and Doxygen entries. The suspend machinery is shared with WOLFSSL_NONBLOCK_OCSP and WOLFSSL_ASYNC_CRYPT. The open-coded pairs of comparisons against WC_PENDING_E and OCSP_WANT_READ are replaced by IsHsSuspendErr(), and the matching build guards by WOLFSSL_HAVE_HS_SUSPEND. That also closes three holes which affected WOLFSSL_NONBLOCK_OCSP builds: - DoHandShakeMsg() freed ssl->pendingMsg on a non-WC_PENDING_E suspend while ProcPeerCertArgs still pointed into it. - wolfSSL_connect()/wolfSSL_accept() and their TLS 1.3 counterparts called FreeAsyncCtx() when buffered output flushed, discarding the saved certificate state. - ProcessPeerCerts() XFREE'd args in WOLFSSL_SMALL_STACK builds without WOLFSSL_ASYNC_CRYPT, where args points inside ssl->async.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a security-critical verification bypass mechanism and modifies handshake suspend/resume behavior across multiple core handshake paths, which warrants final human review.
Pull request overview
Adds an opt-in chain verification replacement callback (WOLFSSL_CHAIN_VERIFY_CB) that fully takes over peer trust decisions (including the ability to suspend/resume the handshake), and refactors handshake-suspend handling into a shared IsHsSuspendErr() helper used across TLS 1.2/1.3 code paths.
Changes:
- Introduces
wolfSSL_{CTX_,}SetChainVerifyCb()+ context plumbing, new error codes (CHAIN_VERIFY_*), and internal enforcement of unsupported feature combinations (DTLS, RPK, OCSP stapling). - Refactors handshake-suspend checks to use
IsHsSuspendErr()and extends suspend-aware state handling to cover the new callback deferral behavior. - Adds targeted API tests and CI configs to exercise accept/deferral/reject/unsupported combinations (including TLS 1.3 post-handshake auth).
File summaries
| File | Description |
|---|---|
| wolfssl/wolfcrypt/settings.h | Ensures async I/O support is enabled when chain-verify callback is compiled in. |
| wolfssl/ssl.h | Adds public API + callback contract documentation for WOLFSSL_CHAIN_VERIFY_CB. |
| wolfssl/internal.h | Extends internal structs/context with callback fields; introduces WOLFSSL_HAVE_HS_SUSPEND + IsHsSuspendErr() decl. |
| wolfssl/error-ssl.h | Adds CHAIN_VERIFY_WANT_E, CHAIN_VERIFY_CB_E, CHAIN_VERIFY_UNSUPPORTED_E error codes. |
| src/ssl_api_cert.c | Implements the new public setters/getter for callback and user context. |
| src/internal.c | Implements suspend-error helper, unsupported-feature checks, decode-only parsing, and callback invocation in cert processing. |
| src/tls13.c | Updates TLS 1.3 handshake processing to treat any suspend error uniformly via IsHsSuspendErr(). |
| src/ssl_api_hs.c | Makes connect/accept flush logic suspend-aware via IsHsSuspendErr(). |
| tests/utils.c | Updates memio handshake helper to treat CHAIN_VERIFY_WANT_E as retryable. |
| tests/api.c | Updates API memio handshake helper to treat CHAIN_VERIFY_WANT_E as retryable. |
| tests/api/test_tls.h | Registers new chain-verify-callback tests. |
| tests/api/test_tls.c | Adds comprehensive TLS 1.2/1.3 callback tests (accept/deferral/reject/bad DER/unsupported DTLS+stapling+postauth). |
| tests/api/test_tls13.h | Registers TLS 1.3 RPK/chain-verify incompatibility test. |
| tests/api/test_tls13.c | Adds TLS 1.3 test ensuring RPK and chain-verify callback are mutually unsupported. |
| examples/configs/user_settings_all.h | Enables WOLFSSL_CHAIN_VERIFY_CB in the “all” user settings template. |
| doc/dox_comments/header_files/ssl.h | Adds Doxygen documentation for the new callback API. |
| configure.ac | Adds --enable-chain-verify-cb configure option and wires it into --enable-all. |
| CMakeLists.txt | Adds CMake option WOLFSSL_CHAIN_VERIFY_CB and definition wiring. |
| cmake/options.h.in | Adds generated-option define for WOLFSSL_CHAIN_VERIFY_CB. |
| .github/configs/os-check-linux.json | Adds CI jobs/configurations to exercise the new feature and interactions. |
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Callback-owned checks are still applied internally, and minimum key sizes can be bypassed under WOLFSSL_VERIFY_NONE.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 4
- Review effort level: Balanced
…ent auth ChainVerifyCbStaplingRequested() was only called under the stapling macros, so a build with the callback and no stapling failed on the unused function. Guard its definition the same way. With WOLFSSL_NO_CLIENT_AUTH a TLS 1.2 server neither requests nor gets a client certificate, so the server-side TLS 1.2 test's negative control cannot fail. Skip that test there; the TLS 1.3 tests are unaffected.
The setter-side checks lived inside the peer-certificate region, which is compiled out with NO_WOLFSSL_CLIENT and WOLFSSL_NO_CLIENT_AUTH, so the setters failed to link there. Move them out. Review findings: - The minimum peer key sizes were skipped under WOLFSSL_VERIFY_NONE even with the callback set, contrary to the documentation. They now apply whenever the callback is in use. - ParseCert(NO_VERIFY) still rejects keyCertSign on a non-CA with KEYUSAGE_E. That is content for the callback to judge, so the decode check passes it through like the other content errors. - The verify depth no longer limits the chain handed to the callback; only MAX_CHAIN_DEPTH does. - The leaf's second parse could still fail on the content errors the decode check passes through; it now treats them the same way. Add a test that an undersized peer key fails under WOLFSSL_VERIFY_NONE with the callback accepting.
The helper is defined without the NO_WOLFSSL_CLIENT condition its caller has, so a build with the client compiled out fails on the unused function. Give the definition the same guard.
Member
Author
|
retest this please |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical certificate-parsing semantics and moderate compatibility, allocation, and test-guard issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 20/20 changed files
- Comments generated: 4
- Review effort level: Balanced
args->certs is already an array of WOLFSSL_BUFFER_INFO, so the copy made for every call, including every re-entry after CHAIN_VERIFY_WANT_E, was an allocation and a MEMORY_E path for nothing. Check the unsupported-feature configuration before any certificate entry or its extensions is parsed. Checking after left a malformed status_request in a TLS 1.3 Certificate entry failing with the parser's error instead of the documented CHAIN_VERIFY_UNSUPPORTED_E. Document that a certificate the parser refuses regardless of the verify mode, such as one with a zero serial number, fails before the callback like malformed DER does.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical suspension defects and moderate parsing and test-guard issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
tests/api/test_tls.c:3756
- Post-handshake client authentication is disabled by
WOLFSSL_NO_CLIENT_AUTH, but this test remains enabled when both macros are defined. Guard this case as well; otherwise the test expects a certificate callback and completed PHA flow that the build explicitly omits.
src/internal.c:18068
- Recognized certificate algorithms that are disabled at build time can make
ParseCert(NO_VERIFY)returnNOT_COMPILED_IN(for example,wolfcrypt/src/asn.c:18052-18063). Because this classifier omits that result,CheckPeerCertsDecode()rejects the certificate before invoking the callback, contrary to the documented contract that unsupported key/signature algorithms are left to the callback. Treat this parse result as certificate content as well; the later leaf-key extraction can still fail when the handshake actually needs that unsupported key.
tests/api/test_tls.c:3618 - This TLS 1.3 server-authentication test also needs the
WOLFSSL_NO_CLIENT_AUTHexclusion used by the TLS 1.2 counterpart above. In that supported build configuration the server does not request/process a client certificate, sotest_chain_verify_cb_accept()reaches its callback-count assertions without the callback having run and fails the API suite.
#if defined(HAVE_CHAIN_VERIFY_CB_TESTS) && defined(WOLFSSL_TLS13)
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Balanced
Sending frees the saved state of a Certificate the chain verify callback suspended: under async crypto SendData() frees ssl->async after building the record. Now wolfSSL_write() resumes the message through wolfSSL_negotiate() while the handshake is in progress, and fails with CHAIN_VERIFY_WANT_E after it, where only wolfSSL_read() resumes a post-handshake Certificate. A write duplicate treated the deferral as a fatal read error and stayed failed for good. Use IsHsSuspendErr() in that gate so no suspend result is handed over.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces an opt-in callback, set via
wolfSSL_CTX_SetChainVerifyCb()orwolfSSL_SetChainVerifyCb(), that takes over the peer certificate trust decision entirely. When enabled, wolfSSL builds no chain and performs no signature, date, revocation, key-usage or hostname checks, and the regular verify callback is not invoked. The callback receives only the DER certificates from the Certificate message (asWOLFSSL_BUFFER_INFOentries, certs[0] first), and is consulted even underWOLFSSL_VERIFY_NONE; rejection always fails the handshake.Key details:
ParseCert(NO_VERIFY)before the callback runs, so malformed DER still fails the handshake without reaching the callback. Content wolfSSL doesn't understand (unknown critical extensions, unsupported key/signature algorithms, algorithm mismatches) is left for the callback to judge, and the check runs only once per certificate.CHAIN_VERIFY_WANT_Eto suspend the handshake; it is re-invoked with the same certificates on re-entry intowolfSSL_connect(),wolfSSL_accept(),wolfSSL_read()orwolfSSL_write(), including for post-handshake authentication in TLS 1.3.CHAIN_VERIFY_CB_E, sends one bad_certificate alert, and setspeerVerifyRetsowolfSSL_get_verify_result()no longer reportsX509_V_OK.CHAIN_VERIFY_UNSUPPORTED_Eif those are already configured, and a connection attempting to combine them fails with the same error plus an internal_error alert before the callback runs. DTLS code paths are otherwise unchanged.--enable-all, with a CMake option and Doxygen documentation.Also refactors the handshake-suspend machinery shared by
WOLFSSL_NONBLOCK_OCSPandWOLFSSL_ASYNC_CRYPT: replaces open-coded checks againstWC_PENDING_E/OCSP_WANT_READwith a newIsHsSuspendErr()helper guarded byWOLFSSL_HAVE_HS_SUSPEND. This fixes three related bugs affectingWOLFSSL_NONBLOCK_OCSPbuilds:DoHandShakeMsg()freeingssl->pendingMsgon a non-WC_PENDING_Esuspend whileProcPeerCertArgsstill referenced it.wolfSSL_connect()/wolfSSL_accept()(and their TLS 1.3 counterparts) discarding saved certificate state viaFreeAsyncCtx()when buffered output flushed.ProcessPeerCerts()freeingargsinWOLFSSL_SMALL_STACKbuilds withoutWOLFSSL_ASYNC_CRYPT, whereargspoints insidessl->async.