Skip to content

Update wolfSSL_X509_verify_cert to retry all certs until a valid chain is found.#8692

Merged
douzzer merged 22 commits intowolfSSL:masterfrom
kareem-wolfssl:zd19563_verify
Sep 30, 2025
Merged

Update wolfSSL_X509_verify_cert to retry all certs until a valid chain is found.#8692
douzzer merged 22 commits intowolfSSL:masterfrom
kareem-wolfssl:zd19563_verify

Conversation

@kareem-wolfssl
Copy link
Contributor

Description

Previously wolfSSL_X509_verify_cert was failing out after finding an invalid chain, this commit updates it to remove the invalid cert and retry until a valid chain is found or all certs are exhausted. This matches OpenSSL's behavior according to the documentation and testing.
This allows wolfSSL_X509_verify_cert to work with multiple CA certs with the same subject key, ie. alt cert chains.

Fixes zd#19563

Testing

Tested with customer provided test.
Unit tests to be added. Confirming it passes CI/CD tests first.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

Copy link
Contributor

Copilot AI left a 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 updates the wolfSSL certificate verification function to retry alternative certificate chains until a valid one is found, addressing issues with multiple CA certs having the same subject key.

  • Added a RemoveCA function prototype in wolfssl/internal.h and its implementation in src/ssl.c.
  • Updated wolfSSL_X509_verify_cert in src/x509_str.c to include retry logic that removes invalid certs and manages a failedCerts stack.
  • Introduced X509StorePopCert to extract a failing cert from the candidate stack.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
wolfssl/internal.h Added prototype for RemoveCA to support cert removal
src/x509_str.c Updated verify_cert to retry with alternative cert chains
src/ssl.c Implemented RemoveCA to remove a CA cert by subject hash
Comments suppressed due to low confidence (4)

wolfssl/internal.h:4262

  • [nitpick] Consider using 'const byte* hash' if the hash parameter is not modified within RemoveCA to enhance const-correctness and clarify intent.
WOLFSSL_LOCAL int RemoveCA(WOLFSSL_CERT_MANAGER* cm, byte* hash, byte type);

src/x509_str.c:409

  • Ensure that the updated return value documentation for wolfSSL_X509_verify_cert aligns with all call sites and the handling of WOLFSSL_SUCCESS/WOLFSSL_FAILURE macros.
 * returns 1 on success or 0 on failure.

src/x509_str.c:511

  • Review the retry logic condition and the manipulation of 'added' and 'depth' to ensure that it does not inadvertently lead to infinite loops or skipped retries when processing alternative cert chains.
if ((origDepth - depth) <= 1)

src/x509_str.c:1099

  • Verify that using pointer equality to identify the certificate in X509StorePopCert is the intended behavior and that it correctly handles cases with duplicate certificate pointers.
if (wolfSSL_sk_X509_value(certs_stack, i) == cert) {

@kareem-wolfssl kareem-wolfssl added For This Release Release version 5.9.0 and removed Not For This Release labels Apr 18, 2025
@JacobBarthelmeh
Copy link
Contributor

Looks like it's currently failing unit tests with this configure --disable-sha --enable-opensslextra.

@kareem-wolfssl kareem-wolfssl added Not For This Release and removed For This Release Release version 5.9.0 labels Apr 18, 2025
@kareem-wolfssl kareem-wolfssl force-pushed the zd19563_verify branch 2 times, most recently from c0be320 to e02811f Compare May 9, 2025 19:49
@dgarske dgarske requested review from dgarske and removed request for ColtonWilley May 30, 2025 20:17
@dgarske dgarske assigned dgarske and unassigned dgarske May 30, 2025
@dgarske dgarske requested review from JacobBarthelmeh and removed request for dgarske June 2, 2025 16:12
@JacobBarthelmeh
Copy link
Contributor

Needs a test case added to avoid regression and to excersice the code with the CI make check's.

may be skipped, modifying the return code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants