src/x509.c: refactor wolfSSL_PEM_read_bio_X509_CRL onto the per-block reader#10336
Open
julek-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
Open
src/x509.c: refactor wolfSSL_PEM_read_bio_X509_CRL onto the per-block reader#10336julek-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
julek-wolfssl wants to merge 2 commits intowolfSSL:masterfrom
Conversation
… reader
ReadPemFromBioToBuffer slurps the entire BIO in one shot, so iterative
callers like wolfSSL_PEM_read_bio_X509_CRL (and by extension
wolfSSL_X509_load_crl_file's BIO branch) saw EOF after the first block
and silently dropped every CRL after the first in a multi-CRL bundle.
Refactor wolfSSL_PEM_read_bio_X509_CRL to delegate to
wolfSSL_PEM_X509_X509_CRL_X509_PKEY_read_bio, which already reads one
PEM BEGIN/END pair per call and leaves the BIO positioned just past the
END line. Loop over it so we skip past intervening cert/key blocks and
return the next CRL in the stream — matching OpenSSL's
PEM_read_bio_X509_CRL, verified against OpenSSL 3.0.13 with cases
{cert,CRL}, {CRL,cert}, {CRL,cert,CRL}, {key,CRL}, {CRL,key,CRL}: in
each case OpenSSL skips non-CRL blocks until EOF.
When the caller passes a non-NULL `x` whose `*x` is already populated,
free the previous CRL before overwriting the slot — matching the
d2i_X509_CRL reuse contract the old body relied on.
To keep both helpers visible at the new call site, drop their `static`
qualifier (wolfSSL_PEM_X509_X509_CRL_X509_PKEY_read_bio for the per-block
read, wolfSSL_X509_PKEY_free to free defensively-allocated keys parsed
from intervening non-CRL blocks). Their definitions in src/x509.c and
declarations in wolfssl/internal.h are widened from OPENSSL_ALL to
OPENSSL_EXTRA || OPENSSL_ALL so the OPENSSL_EXTRA-only build (which
compiles wolfSSL_PEM_read_bio_X509_CRL) links cleanly. The unrelated
INFO_read_bio / INFO_read_bio_X509_INFO group below them keeps its
OPENSSL_ALL gate because it depends on wolfSSL_X509_INFO_new/free that
are still OPENSSL_ALL-only.
Also register the previously-orphaned test_wolfSSL_X509_load_crl_file
(its slot in TEST_OSSL_X509_LOOKUP_DECLS was a duplicated
test_wolfSSL_X509_LOOKUP_ctrl_hash_dir entry), update its assertion for
crl2.pem (which already contains two CRLs) to expect 2 instead of 1, and
add a multi-CRL bundle case that builds a memory BIO from
crl.pem + server-cert.pem + crl2.pem and asserts that the reader walks
past the cert and returns all 3 CRLs before NULL.
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors wolfSSL_PEM_read_bio_X509_CRL to use a per-PEM-block reader so multi-CRL PEM bundles (and mixed PEM streams) return all CRLs instead of only the first.
Changes:
- Reworked
wolfSSL_PEM_read_bio_X509_CRLto iterate over a per-block PEM reader and skip non-CRL blocks. - Exposed internal helper/free routines via
internal.hfor reuse at the new call site. - Fixed and expanded API tests to register
test_wolfSSL_X509_load_crl_file, expect multi-CRL counts, and add a mixed-stream memory BIO test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/x509.c |
Implements iterative CRL reading by delegating to the per-block reader and skipping non-CRL PEM blocks. |
wolfssl/internal.h |
Adds internal prototypes to allow the refactor to call the per-block reader and free helper. |
tests/api/test_ossl_x509_lu.h |
Registers the previously orphaned CRL-load test in the test declaration list. |
tests/api/test_ossl_x509_lu.c |
Updates expected CRL counts and adds a mixed PEM-stream test to validate multi-CRL iteration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
julek-wolfssl
force-pushed
the
wolfSSL_PEM_read_bio_X509_CRL-multi-crl-fix
branch
from
9b780e8 to
90584f2
Compare
The CRL refactor broke nginx's ssl_cache.t (and the wolfSSL/wolfssl nginx_check matrix on 1.24.0/1.25.0/1.28.1) because nginx loads the test CRL through a FIFO. wolfSSL_PEM_X509_X509_CRL_X509_PKEY_read_bio() asks wolfSSL_BIO_get_len() for the BIO size up front; for a FIFO the underlying ftell() returns ESPIPE, wolfssl_file_len() reports WOLFSSL_BAD_FILETYPE, and BIO_get_len() returns 0. The function then hit the l <= pem_struct_min_sz guard and bailed with ASN_NO_PEM_HEADER before reading a byte, so the caller's loop saw "no CRL" and nginx emitted "PEM_read_bio_X509_CRL() failed". Treat l == 0 as "streaming source, size unknown" and allocate up to MAX_BIO_READ_BUFFER (the same cap ReadPemFromBioToBuffer used for this case before the refactor). The existing byte-by-byte reader already stops at the END marker or at EOF, so this is enough; if the upstream short-reads we still surface ASN_NO_PEM_HEADER from the pem_struct_min_sz read below. Keep rejecting tiny non-zero lengths since those are real "buffer too small" cases.
julek-wolfssl
force-pushed
the
wolfSSL_PEM_read_bio_X509_CRL-multi-crl-fix
branch
from
90584f2 to
b261ee6
Compare
Member
Author
|
Retest this please flaky test.. |
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.
ReadPemFromBioToBuffer slurps the entire BIO in one shot, so iterative
callers like wolfSSL_PEM_read_bio_X509_CRL (and by extension
wolfSSL_X509_load_crl_file's BIO branch) saw EOF after the first block
and silently dropped every CRL after the first in a multi-CRL bundle.
Refactor wolfSSL_PEM_read_bio_X509_CRL to delegate to
wolfSSL_PEM_X509_X509_CRL_X509_PKEY_read_bio, which already reads one
PEM BEGIN/END pair per call and leaves the BIO positioned just past the
END line. Loop over it so we skip past intervening cert/key blocks and
return the next CRL in the stream — matching OpenSSL's
PEM_read_bio_X509_CRL, verified against OpenSSL 3.0.13 with cases
{cert,CRL}, {CRL,cert}, {CRL,cert,CRL}, {key,CRL}, {CRL,key,CRL}: in
each case OpenSSL skips non-CRL blocks until EOF.
To make those helpers callable from the new site, drop their
staticqualifier (wolfSSL_PEM_X509_X509_CRL_X509_PKEY_read_bio for the per-block
read, wolfSSL_X509_PKEY_free to free defensively-allocated keys parsed
from intervening non-CRL blocks) and declare both WOLFSSL_LOCAL in
internal.h under their existing OPENSSL_ALL[/NO_BIO] gates.
Also register the previously-orphaned test_wolfSSL_X509_load_crl_file
(its slot in TEST_OSSL_X509_LOOKUP_DECLS was a duplicated
test_wolfSSL_X509_LOOKUP_ctrl_hash_dir entry), update its assertion for
crl2.pem (which already contains two CRLs) to expect 2 instead of 1, and
add a multi-CRL bundle case that builds a memory BIO from
crl.pem + server-cert.pem + crl2.pem and asserts that the reader walks
past the cert and returns all 3 CRLs before NULL.