In wolfSSL_CTX_set_cert_store, send certificates into the CertMgr #8708

anhu · 2025-04-23T22:52:20Z

Fixes ZD 19760

Before this fix, the certificates were not getting into the certificate manager. This makes sure they are going in.

anhu · 2025-04-24T15:05:21Z

Will need to add test so that will kick off another round of PRB

Found unhandled org.jenkinsci.plugins.workflow.support.steps.AgentOfflineException exception:

anhu · 2025-05-12T22:40:04Z

Jenkins retest this please

(lost the build data)

anhu · 2025-05-13T14:07:29Z

Jenkins retest this please

java.io.StreamCorruptedException: invalid stream header: 636F7272

anhu · 2025-05-15T15:24:39Z

Jenkins retest this please

(aborted after 1.5 hours)

anhu · 2025-06-04T16:09:04Z

jenkins retest this please

anhu · 2025-06-06T15:38:42Z

Jenkins retest this please

Copilot

Pull Request Overview

Adds support for moving existing certificates into the CertMgr when setting a certificate store on an SSL context.

Introduces X509StoreAddCa and updates wolfSSL_sk_X509_num and wolfSSL_sk_X509_pop declarations
Modifies wolfSSL_CTX_set_cert_store to transfer all stored certs into the CertMgr and clear the original stack
Adds a unit test to verify the store’s cert stack is cleared after calling wolfSSL_CTX_set_cert_store

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
wolfssl/ssl.h	Moved declaration of `wolfSSL_sk_X509_num` out of the legacy block
wolfssl/internal.h	Declared `X509StoreAddCa` for internal CertMgr additions
tests/api.c	Added `test_wolfSSL_CTX_set_cert_store_null_certs`
src/x509_str.c	Implemented `X509StoreAddCa` and removed duplicate static copy
src/x509.c	Added `wolfSSL_sk_X509_pop` wrapper and extended `sk_X509_num` scope
src/ssl.c	Updated `wolfSSL_CTX_set_cert_store` to move certs into CertMgr

Comments suppressed due to low confidence (2)

tests/api.c:28775

The new test only verifies that store->certs is set to NULL; consider adding a complementary test case to confirm that certificates are actually added into the internal CertMgr (e.g., by inspecting ctx->cm).

static int test_wolfSSL_CTX_set_cert_store_null_certs(void)

src/x509.c:4180

The function signature for wolfSSL_sk_X509_pop uses WOLFSSL_X509_NAME instead of WOLFSSL_X509. This mismatch may cause invalid casts or runtime errors. Update the parameter to WOLF_STACK_OF(WOLFSSL_X509)* sk.

WOLFSSL_X509* wolfSSL_sk_X509_pop(WOLF_STACK_OF(WOLFSSL_X509_NAME)* sk)

src/ssl.c

Don't ignore return code. Co-authored-by: Copilot <[email protected]>

JacobBarthelmeh · 2025-06-06T20:32:43Z

tests/api.c

+                                                            SSL_FILETYPE_PEM));
+
+    /* Add the certificate to the store */
+    ExpectIntEQ(X509_STORE_add_cert(store, cert), SSL_SUCCESS);


I was looking over the support case referenced and I don't think this test reflects the edge case reported. It looks like we should be testing the order of :

wolfSSL_CTX_set_cert_store(ctx, store);

then

X509_STORE_add_cert(store, cert)

Where a cert store is assigned to the WOLFSSL_CTX struct and after it is assigned to it there is certificates added.

This is going to be tricky or even impossible to fully support with the current design. X509_STORE owned by SSL_CTX expects everything to be in underlying CM, while X509_STORE not-owned expects them in str->certs. I should say this flow will work for TLS, but setting a X509_STORE inside a SSL_CTX, then expecting direct X509_STORE APIs to work right is likely not going to work with current model.

JacobBarthelmeh · 2025-06-06T20:38:04Z

src/ssl.c

+        if (str->certs != NULL) {
+            while (wolfSSL_sk_X509_num(str->certs) > 0) {
+                int ret;
+                x = wolfSSL_sk_X509_pop(str->certs);


The cert store has 3 X509 stacks, one for certs, one for trusted, and one named owned. Are we sure that it is the certs stack that should get added as CA's?

In my opinion, certs should get added but trusted should also.

JacobBarthelmeh · 2025-06-06T20:39:08Z

src/ssl.c

+                    break; /* Exit the loop on failure */
+                }
+            }
+            wolfSSL_sk_X509_pop_free(str->certs, NULL);


Why does the certs stack need free'd and emptied here? Is it being converted over to a WOLFSSL_CTX cert store, and that future calls to wolfSSL_X509_STORE_add_cert would then load certs as a CA?

Also it would be worth checking if this behavior conforms to the expected OpenSSL compat behavior. Attempting untrusted vs trusted self signed certificate loads after the store is set to the WOLFSSL_CTX struct.

There is a concept of SSL_CTX owned X509_STORE vs non-owned. While confusing, it was done this way to leave all existing code for TLS and CM usage unmodified. To determine if X509_STORE is owned by SSL_CTX or not, we do NULL checks on str->certs. So that is why it needs to be pop_freed here, otherwise logic in X509_STORE wont work right.

JacobBarthelmeh · 2025-06-06T20:44:45Z

tests/api.c

+    wolfSSL_CTX_set_cert_store(ctx, store);
+
+    /* Verify that the certs member of the store is null */
+    ExpectNull(store->certs);


We should be testing a successful verification making use of the loaded CA's.

anhu · 2025-06-23T21:30:36Z

Assigned to @ColtonWilley as agreed in offline conversation.

anhu added the Not For This Release label Apr 23, 2025

anhu requested review from ColtonWilley and wolfSSL-Bot April 23, 2025 22:52

anhu self-assigned this Apr 23, 2025

anhu force-pushed the wolfSSL_CTX_set_cert_store branch from 6eda342 to f83ebb2 Compare April 24, 2025 19:02

anhu removed the Not For This Release label May 14, 2025

anhu force-pushed the wolfSSL_CTX_set_cert_store branch 2 times, most recently from adc18d7 to c90fb03 Compare May 15, 2025 13:26

anhu added 2 commits June 6, 2025 10:59

In wolfSSL_CTX_set_cert_store, send certificates into the CertMgr

66dc2b2

WOLFSSL_WPAS_SMALL needs this as well.

5e78122

anhu force-pushed the wolfSSL_CTX_set_cert_store branch from 8166be6 to 5e78122 Compare June 6, 2025 14:59

anhu assigned wolfSSL-Bot and unassigned anhu Jun 6, 2025

JacobBarthelmeh requested a review from Copilot June 6, 2025 20:10

Copilot AI reviewed Jun 6, 2025

View reviewed changes

src/ssl.c Outdated Show resolved Hide resolved

anhu and others added 2 commits June 6, 2025 16:23

Update src/ssl.c

a9dd008

Don't ignore return code. Co-authored-by: Copilot <[email protected]>

C89

8468fe3

JacobBarthelmeh requested changes Jun 6, 2025

View reviewed changes

JacobBarthelmeh assigned anhu and unassigned wolfSSL-Bot Jun 6, 2025

anhu assigned ColtonWilley and unassigned anhu Jun 23, 2025

In wolfSSL_CTX_set_cert_store, send certificates into the CertMgr #8708

Are you sure you want to change the base?

In wolfSSL_CTX_set_cert_store, send certificates into the CertMgr #8708

Uh oh!

Conversation

anhu commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anhu commented Apr 24, 2025

Uh oh!

anhu commented May 12, 2025

Uh oh!

anhu commented May 13, 2025

Uh oh!

anhu commented May 15, 2025

Uh oh!

anhu commented Jun 4, 2025

Uh oh!

anhu commented Jun 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anhu commented Jun 23, 2025

Uh oh!

Uh oh!

anhu commented Apr 23, 2025 •

edited

Loading