Skip to content

crypto: prevent stale OpenSSL errors - #67225

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-crypto-openssl-error-queue
Open

mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-crypto-openssl-error-queue

Conversation

@mcollina

@mcollina mcollina commented Nov 3, 2026

Copy link
Copy Markdown
Member

Several crypto operations can leave errors in OpenSSL's thread-local queue after returning a normal result or a Node.js error. A later operation can then consume those stale errors.

Clear errors at the affected API boundaries and before TLS I/O and private-key parsing so each operation only observes its own errors. Add regression coverage for key comparison, SPKAC methods, and EC JWK import.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/security-wg

@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. dependencies PRs that add, update, or configure Node.js dependencies. needs-ci PRs that need a full CI run. labels Nov 3, 2026

@panva panva 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.

Can you avoid clearing an already empty queue before every TLS read/write? The change costs 5–9% throughput in local tests with 1 KiB records. This avoids the regression:

if (ERR_set_mark() != 0) ERR_clear_error();

Can you also move the ClearErrorOnReturn in KeyObjectHandle::Equals() into the public/private-key branch? Secret-key comparisons don't generate these errors.

The test also needs independent queue checks: createPrivateKey() now clears the queue itself, so it hides missing cleanup in the preceding operation. Can you add cctests that check the queue directly after each source, plus a TLS test that injects stale errors between reads?

@codecov

codecov Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.67%. Comparing base (45df6b7) to head (845c880).
⚠️ Report is 37 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_tls.cc 82.35% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67225      +/-   ##
==========================================
- Coverage   90.69%   90.67%   -0.02%     
==========================================
  Files         797      797              
  Lines      278883   279387     +504     
  Branches    53630    53748     +118     
==========================================
+ Hits       252934   253340     +406     
- Misses      16314    16340      +26     
- Partials     9635     9707      +72     
Files with missing lines Coverage Δ
src/crypto/crypto_keys.cc 72.58% <100.00%> (+0.05%) ⬆️
src/crypto/crypto_tls.cc 78.44% <82.35%> (+0.04%) ⬆️

... and 35 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.

Clear errors raised by key comparison, SPKAC parsing, and JWK
import. Ensure TLS and private-key parsing only inspect errors
from their own OpenSSL calls.

Assisted-by: Codex
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
mcollina force-pushed the fix-crypto-openssl-error-queue branch from 072d7e4 to 845c880 Compare November 4, 2026 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. dependencies PRs that add, update, or configure Node.js dependencies. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants