Skip to content

crypto: evict getCiphers/getHashes cache on setFips/setEngine#62985

Open
srikanth-karthi wants to merge 4 commits intonodejs:mainfrom
srikanth-karthi:fix-cipher-hash-cache-eviction
Open

crypto: evict getCiphers/getHashes cache on setFips/setEngine#62985
srikanth-karthi wants to merge 4 commits intonodejs:mainfrom
srikanth-karthi:fix-cipher-hash-cache-eviction

Conversation

@srikanth-karthi
Copy link
Copy Markdown

@srikanth-karthi srikanth-karthi commented Apr 27, 2026

Fixes: #62982

Summary

getCiphers() and getHashes() used cachedResult() which memoizes on first call and never clears. Two operations mutate the OpenSSL algorithm set but did not invalidate the cache:

  • setFips(true/false) — restricts/restores the visible cipher/hash list to FIPS-approved algorithms only
  • setEngine(id, flags) — loads an OpenSSL engine that may register additional ciphers/hashes

This meant that after calling either function, getCiphers() and getHashes() would continue returning the stale pre-call snapshot.

Changes

  • Replace the two cachedResult() calls for getCiphers/getHashes in lib/internal/crypto/util.js with manual let _ciphersCache / let _hashesCache variables — mirroring the existing _hashCache / getHashCache() pattern already in the same file.
  • Add evictCipherHashCache() that resets both variables to undefined.
  • Call evictCipherHashCache() from setEngine() (after a successful engine load) and from setFips() (after setFipsCrypto(val)).
  • getCurves intentionally keeps using cachedResult() — curves are not affected by FIPS mode or engine loading.

getCiphers() and getHashes() used cachedResult() which memoizes once
and never clears. setFips() changes which algorithms OpenSSL exposes
(FIPS-approved only vs. all), and setEngine() can register additional
ciphers/hashes from a loaded engine, but neither invalidated the cache.

Replace the two cachedResult() calls with manual cache variables
(_ciphersCache, _hashesCache) that mirror the existing _hashCache
pattern. Add evictCipherHashCache() and call it from both setFips()
and setEngine() after they mutate OpenSSL state.

getCurves is intentionally left using cachedResult — curves are not
affected by FIPS mode or engine loading.

Fixes: nodejs#62982
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Apr 27, 2026
@panva
Copy link
Copy Markdown
Member

panva commented Apr 27, 2026

@srikanth-karthi thank you, this looks good on first a cursory look but can you add a test? we do have fips enabled runners in CI that could use one.

@ChALkeR
Copy link
Copy Markdown
Member

ChALkeR commented Apr 27, 2026

As #62982 details mention: under BoringSSL, internal code in conditionalAlgorithms calls getHashes and getCiphers at load.

'ChaCha20-Poly1305': !process.features.openssl_is_boringssl ||
ArrayPrototypeIncludes(getCiphers(), 'chacha20-poly1305'),
'cSHAKE128': !process.features.openssl_is_boringssl ||
ArrayPrototypeIncludes(getHashes(), 'shake128'),
'cSHAKE256': !process.features.openssl_is_boringssl ||
ArrayPrototypeIncludes(getHashes(), 'shake256'),

Those results are also stale

@panva
Copy link
Copy Markdown
Member

panva commented Apr 27, 2026

BoringSSL has no fips mode. And when it's not boringssl and the ciphers "become unavailable" due to a toggle switch they'll start failing with OperationError and the only inconsistency will be SubtleCrypto.supports reporting true when it should be false.

The "fips to non-fips" case 🤷

Imho not worth the churn.

@srikanth-karthi
Copy link
Copy Markdown
Author

Thanks for the context. Agreed — under BoringSSL there's no FIPS mode so conditionalAlgorithms is stable for the process lifetime, and for OpenSSL the SubtleCrypto.supports() inconsistency after a FIPS toggle is an accepted limitation given the churn required to fix it. I'll leave conditionalAlgorithms out of scope for this PR.

Comment thread test/parallel/test-crypto-ciphers-hashes-fips-cache.js Outdated
@panva panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 27, 2026
Comment thread test/parallel/test-crypto-ciphers-hashes-fips-cache.js Outdated
@panva
Copy link
Copy Markdown
Member

panva commented Apr 27, 2026

@ChALkeR what do you think about keeping the identity of the cached result the same? that way consumer can actually rely on their returned value and not get tripped up by someone elses' concern flipping fips mode.

Add a FIPS-only regression test for nodejs#62982 that confirms getCiphers()
and getHashes() reflect the restricted FIPS algorithm set after
setFips(true) and restore the full list after setFips(false).

Signed-off-by: srikanth-karthi <[email protected]>
Capture initialFips state and explicitly disable FIPS before recording
the baseline algorithm lists, so the test works correctly on systems
where FIPS is enabled by default. Restore the original state on exit.

Signed-off-by: srikanth-karthi <[email protected]>
Each test runs in its own process so restoring the initial FIPS state
is unnecessary.

Signed-off-by: srikanth-karthi <[email protected]>
@srikanth-karthi srikanth-karthi force-pushed the fix-cipher-hash-cache-eviction branch from 967e31f to 76e61cb Compare April 27, 2026 10:57
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.66%. Comparing base (85ae4cd) to head (76e61cb).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #62985   +/-   ##
=======================================
  Coverage   89.66%   89.66%           
=======================================
  Files         707      707           
  Lines      219501   219523   +22     
  Branches    42087    42092    +5     
=======================================
+ Hits       196807   196832   +25     
  Misses      14588    14588           
+ Partials     8106     8103    -3     
Files with missing lines Coverage Δ
lib/crypto.js 93.04% <100.00%> (+0.81%) ⬆️
lib/internal/crypto/util.js 95.89% <100.00%> (+0.09%) ⬆️

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

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

Labels

commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

setFips() / setEngine do not evict ciphers cache

4 participants