Add CRL generation capability #315

padelsbach · 2026-01-20T17:50:00Z

Add wrappers for CRL generation logic. Requires wolfSSL/wolfssl#9631 to be merged.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 15 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T21:29:23Z

.github/workflows/linux-common.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          repository: padelsbach/wolfssl
+          ref: crl-generation


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

repository: padelsbach/wolfssl

ref: crl-generation

repository: wolfSSL/wolfssl

Copilot · 2026-01-21T21:29:24Z

.github/workflows/infer.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          ref: crl-generation
+          repository: padelsbach/wolfssl


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository. Note that the ref and repository parameters are in reverse order compared to other workflow files.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

ref: crl-generation

repository: padelsbach/wolfssl

# Use official wolfSSL repository

repository: wolfSSL/wolfssl

native/com_wolfssl_WolfSSLCRL.c

src/java/com/wolfssl/WolfSSLCRL.java

Copilot · 2026-01-21T21:29:26Z

.github/workflows/windows-vs.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          repository: padelsbach/wolfssl
+          ref: crl-generation


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

repository: padelsbach/wolfssl

ref: crl-generation

repository: wolfSSL/wolfssl

Copilot · 2026-01-21T21:29:26Z

.github/workflows/address-sanitizer.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          repository: padelsbach/wolfssl
+          ref: crl-generation


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

repository: padelsbach/wolfssl

ref: crl-generation

repository: wolfSSL/wolfssl

Copilot · 2026-01-21T21:29:27Z

.github/workflows/stable-wolfssl-releases.yml

      fail-fast: false
      matrix:
-        wolfssl_version: ${{ fromJson(needs.get-stable-releases.outputs.versions) }}
+        wolfssl_version: crl-generation


The matrix strategy has been changed to use a hardcoded crl-generation branch instead of the dynamic versions from the previous step. This appears to be temporary for testing but breaks the purpose of this workflow which is to test against stable releases. This should be reverted or the workflow should be updated to handle the new dependency properly.

Suggested change

wolfssl_version: crl-generation

wolfssl_version: ${{ fromJson(needs.get-stable-releases.outputs.versions) }}

Copilot · 2026-01-21T21:29:27Z

.github/workflows/scan-build.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          repository: padelsbach/wolfssl
+          ref: crl-generation


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

repository: padelsbach/wolfssl

ref: crl-generation

# Use official wolfSSL repository for CI builds

repository: wolfSSL/wolfssl

Copilot · 2026-01-21T21:29:27Z

.github/workflows/maven.yml

+          # Point to PR branch until merge
+          # TODO: Switch back to wolfssl/wolfssl after merge
+          repository: padelsbach/wolfssl
+          ref: crl-generation


The workflow is pointing to a temporary repository (padelsbach/wolfssl) and branch (crl-generation) for testing. The TODO comment indicates this should be changed back to wolfSSL/wolfssl after merge. This is acceptable for development but must be reverted before merging this PR to ensure CI uses the official repository.

Suggested change

# Point to PR branch until merge

# TODO: Switch back to wolfssl/wolfssl after merge

repository: padelsbach/wolfssl

ref: crl-generation

repository: wolfSSL/wolfssl

ref: master

padelsbach · 2026-01-22T19:48:33Z

The FIPS failures are due to the use of the main wolfSSL repo and are expected to be fixed once the CRL PR is merged on the wolfSSL repo.

I will also revert the yamls at that point.

cconlon · 2026-01-22T23:54:16Z

native/com_wolfssl_WolfSSLCRL.c

+    timeBuf = (byte*)(*jenv)->GetByteArrayElements(jenv, time, NULL);
+    timeSz = (*jenv)->GetArrayLength(jenv, time);
+    if (timeBuf == NULL || 
+            timeSz < (com_wolfssl_WolfSSLCRL_CTC_DATE_SIZE + 8)) {


Could we add a small comment saying what 8 represents here?

cconlon · 2026-01-22T23:56:38Z

native/com_wolfssl_WolfSSLCRL.c

+    }
+    else {
+        /* Extract length from bytes 32-35 (assuming native byte order) */
+        timeLen = *((int*)(timeBuf + com_wolfssl_WolfSSLCRL_CTC_DATE_SIZE));


I was pondering if we should use the native CTC_DATE_SIZE here rather than the JNI-defined equivalent (com_wolfssl_WolfSSLCRL_CTC_DATE_SIZE)? If it were me I probably would use CTC_DATE_SIZE since we are calling down to native wolfSSL from this layer, and it's shorter to read.

Comment applies across all usages of that define.

cconlon · 2026-01-22T23:58:18Z

native/com_wolfssl_WolfSSLCRL.c

+    timeBuf = (byte*)(*jenv)->GetByteArrayElements(jenv, time, NULL);
+    timeSz = (*jenv)->GetArrayLength(jenv, time);
+    if (timeBuf == NULL || 
+            timeSz < (com_wolfssl_WolfSSLCRL_CTC_DATE_SIZE + 8)) {


Same comment as above, a small comment about what 8 represents would be nice.

cconlon · 2026-01-23T00:00:06Z

native/com_wolfssl_WolfSSLCRL.c

+        ret = WOLFSSL_FAILURE;
+    }
+    else {
+        ret = WOLFSSL_SUCCESS;


Would it be better to initialize ret = WOLFSSL_SUCCESS at variable declaration, then just worry about setting ret = WOLFSSL_FAILURE when needed?

cconlon · 2026-01-23T00:02:33Z

native/com_wolfssl_WolfSSLCRL.c

+        }
+    }
+
+    (*jenv)->ReleaseByteArrayElements(jenv, time, (jbyte*)timeBuf, JNI_ABORT);


Should this be wrapped in a if (timeBuf != NULL), in case GetByteArrayElements() failed up above? Same comment applies to different functions where ReleaseByteArrayElements() is called as well.

That pattern exists in places in this repo, but the large majority of calls to ReleaseByteArrayElements() don't perform the null check

cconlon · 2026-01-23T00:07:57Z

native/com_wolfssl_WolfSSLCRL.c

+
+    /* Set correct WOLFSSL_EVP_MD, does not need to be freed */
+    if (ret == WOLFSSL_FAILURE) {
+        /* skip if already failed */


This seems a little odd, at least different than how most of our native wolfSSL code looks:

if (ret == WOLFSSL_FAILURE) { /* skip if already failed */ }

In wolfSSL I think you would typically see a pattern like the following instead of the if(skip)/else:

if (ret == WOLFSSL_SUCCESS) { if (digestAlg != NULL) { ... } }

Or:

if ((ret == WOLFSSL_SUCCESS) && (digestAlg != NULL)) { ... }

cconlon · 2026-01-23T00:11:05Z

native/com_wolfssl_WolfSSLCRL.c

+        /* already in DER */
+        derBuf = keyBuf;
+        derSz = keySz;
+        ret = WOLFSSL_SUCCESS;


Hasn't ret already been initialized to WOLFSSL_SUCCESS at this point, unless it has been overridden by WOLFSSL_FAILURE (but then we shouldn't get to this point).

cconlon · 2026-01-23T00:13:42Z

native/com_wolfssl_WolfSSLCRL.c

+    defined(WOLFSSL_CERT_GEN) && !defined(NO_ASN_TIME)
+    WOLFSSL_X509_CRL* crl = (WOLFSSL_X509_CRL*)(uintptr_t)crlPtr;
+    WOLFSSL_ASN1_TIME* date = NULL;
+    char ret[32];


A comment about why 32 is used here would be good, or maybe use of MAX_TIME_STRING_SZ instead?

Ok. I'll update a couple similar methods in com_wolfssl_WolfSSLCertificate.c which follow this pattern

cconlon · 2026-01-23T00:15:16Z

native/com_wolfssl_WolfSSLCRL.c

+    defined(WOLFSSL_CERT_GEN) && !defined(NO_ASN_TIME)
+    WOLFSSL_X509_CRL* crl = (WOLFSSL_X509_CRL*)(uintptr_t)crlPtr;
+    WOLFSSL_ASN1_TIME* date = NULL;
+    char ret[32];


Same as above, for possible use of MAX_TIME_STRING_SZ

cconlon · 2026-01-23T00:22:11Z

src/java/com/wolfssl/WolfSSLCRL.java

+                    WolfSSLDebug.INFO, this.crlPtr,
+                    () -> "entered getEncoded(format: " + format + ")");
+
+                ret = write_X509_CRL(this.crlPtr, tempPath.toString(), format);


Would it be easier just to add a native wolfSSL API that returns back a byte array of the CRL? That seems easier and less error prone to me than making a temporary file, especially if this is running in some sort of restricted Java environment where file creation isn't allowed (maybe Android or such).

padelsbach force-pushed the crl-generation branch from d84fe0d to 8e8480d Compare January 20, 2026 21:28

cconlon requested a review from Copilot January 20, 2026 21:31

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Copilot started reviewing on behalf of cconlon January 20, 2026 21:57 View session

padelsbach force-pushed the crl-generation branch 5 times, most recently from a016758 to 3676f25 Compare January 21, 2026 20:32

cconlon requested a review from Copilot January 21, 2026 21:24

Copilot started reviewing on behalf of cconlon January 21, 2026 21:24 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

padelsbach force-pushed the crl-generation branch 7 times, most recently from 8b9532c to 8af03a8 Compare January 22, 2026 19:07

padelsbach marked this pull request as ready for review January 22, 2026 19:48

padelsbach requested a review from cconlon January 22, 2026 20:51

padelsbach assigned cconlon Jan 22, 2026

cconlon requested changes Jan 23, 2026

View reviewed changes

cconlon assigned padelsbach and unassigned cconlon Jan 23, 2026

Add CRL generation capability

dfa7848

padelsbach force-pushed the crl-generation branch from 8af03a8 to dfa7848 Compare January 23, 2026 22:07

	wolfssl_version: crl-generation
	wolfssl_version: ${{ fromJson(needs.get-stable-releases.outputs.versions) }}

Add CRL generation capability #315

Are you sure you want to change the base?

Add CRL generation capability #315

Conversation

padelsbach commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

padelsbach commented Jan 22, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

padelsbach commented Jan 20, 2026 •

edited

Loading