Skip to content

tls: load all CRLs from a PEM bundle - #65577

Open
lazerg wants to merge 2 commits into
nodejs:mainfrom
lazerg:fix/issue-65576-crl-bundle
Open

tls: load all CRLs from a PEM bundle#65577
lazerg wants to merge 2 commits into
nodejs:mainfrom
lazerg:fix/issue-65576-crl-bundle

Conversation

@lazerg

@lazerg lazerg commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

AddCRL() read a single PEM block and returned, so a crl option holding several concatenated CRLs only ever got its first entry into the store. Any certificate whose issuer's CRL came later in the bundle then failed with UNABLE_TO_GET_CRL.

Read in a loop until the BIO is exhausted, the way AddCACertificates() right above it already does for ca bundles.

Fixes: #65576

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 27, 2026
@lazerg
lazerg force-pushed the fix/issue-65576-crl-bundle branch from 06781d7 to d20618d Compare August 27, 2026 08:31
AddCRL() read a single PEM block and returned, so a crl option holding
several concatenated CRLs only ever got its first entry into the store.
Any certificate whose issuer's CRL came later in the bundle then failed
with UNABLE_TO_GET_CRL.

Read in a loop until the BIO is exhausted, the way AddCACertificates()
right above it already does for ca bundles.

Signed-off-by: Lazizbek Ergashev <lazerg2@gmail.com>
@lazerg
lazerg force-pushed the fix/issue-65576-crl-bundle branch from d20618d to 9db73ed Compare August 27, 2026 09:36
Comment thread src/crypto/crypto_tls_certificates.cc
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (abb365a) to head (02967d3).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_tls_certificates.cc 71.42% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65577   +/-   ##
=======================================
  Coverage   90.07%   90.08%           
=======================================
  Files         751      751           
  Lines      254875   254882    +7     
  Branches    48115    48113    -2     
=======================================
+ Hits       229586   229600   +14     
+ Misses      16467    16463    -4     
+ Partials     8822     8819    -3     
Files with missing lines Coverage Δ
src/crypto/crypto_tls_certificates.cc 71.18% <71.42%> (+0.03%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Return false unless the read loop ran to the end of the BIO, so a bundle
holding an unparseable CRL throws instead of quietly applying only the
entries that were read.

Signed-off-by: Lazizbek Ergashev <lazerg2@gmail.com>

@pimterry pimterry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

There is a question on whether this should be semver major that others might want to chime in on: before, loading a CRL bundle where the first CRL was valid but others were broken was successful - now it fails if any CRLs in the bundle can't be loaded.

Personally I think it's reasonable as a bug fix - you clearly want to load the CRLs and not doing so was a silent error before - but I could be persuaded if anybody else feels strongly.

@pimterry pimterry added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 27, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lazerg

lazerg commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

The red on node-test-commit comes from the first node-test-commit-windows-fanned attempt, build 79686, which went from pending to failed in eight seconds. The rerun, build 79689, passed, and every other platform in that run is green. A fresh CI run should clear it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tls: crl option only loads first CRL from concatenated PEM bundle

4 participants