Conversation
Secret key bytes are immutable after KeyObjectData construction. AES, ChaCha20-Poly1305, MAC, and KDF jobs read those bytes without acquiring KeyObjectData::mutex(), and secret-key export and equality do the same. Do not allocate a shared mutex that none of these operations uses. Asymmetric key mutex initialization remains eager so copies continue to share the same lock while asymmetric acquisitions remain. Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
RSA_Cipher delegates to ncrypto::Rsa with a new EVP_PKEY_CTX per operation. Padding, digests, labels, and output buffers belong to that context. OpenSSL and BoringSSL synchronize RSA blinding and Montgomery caches internally. The lock was introduced for early OpenSSL 3 key downgrading that cleared shared provider fields. Current OpenSSL publishes a separate legacy cache under its own lock. Cover cold shared CryptoKeys and worker clones with concurrent encrypt/decrypt, synchronous KeyObject aliases, and checked plaintexts. Refs: nodejs#36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
ncrypto::KEM creates a new EVP_PKEY_CTX per operation. Parameters, entropy, scratch buffers, and outputs are private to each call. ML-KEM expansion finishes during import, and operations read an immutable key. EC and X25519 retain read-only recipient keys. RSA synchronizes mutable caches internally. Remove the outer locks serializing jobs that share a KeyObject or CryptoKey. Cover cold keys and worker clones across RSA, EC, X25519, and ML-KEM, checking every resulting shared secret. Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
panva
added
crypto
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Contributor
|
Benchmark GHA (crypto / kem.js): https://github.com/nodejs/node/actions/runs/36720318950 Results
Benchmark results:
|
Contributor
|
Benchmark GHA (crypto / verify.js): https://github.com/nodejs/node/actions/runs/36720331582 Results
Benchmark results:
|
Verify configuration only copies signature bytes and reads DSA/EC order width for P1363 conversion. Conversion constructs independent DER and BIGNUM values. Signing and verification already run without the mutex using separate operation contexts. Cover cold shared public keys and worker clones with checked P1363 verification while signatures, derivation, exports, metadata queries, and equality checks overlap. Compare source PKCS8 encoding after workers finish. OpenSSL's ordinary EC PKCS8 encoder temporarily changes source flags and races even on the unmodified base. Keep WebCrypto PKCS8 exports concurrent through their independent clones. Refs: nodejs#36825 Refs: nodejs#37816 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
Raw and JWK getters return independently owned bytes or BIGNUMs. RSA metadata snapshots public parameters and PSS restrictions. EC fallback paths reconstruct or duplicate the source key. EC PKCS8 export changes encoding flags only on its own clone. Early OpenSSL 3 key downgrading mutated shared provider fields. Current OpenSSL publishes separate legacy and provider conversion caches under internal read/write locks. Ordinary DER/PEM export, equality, signing, and key derivation already access the same keys without these locks. Remove export and metadata acquisitions, including their coverage of V8 allocation and encoding, and remove the now-unused shared mutex storage. Retain shared immutable key ownership. The concurrent reuse test checks that EC PKCS8 export leaves the original flags intact. Refs: nodejs#36825 Refs: https://docs.openssl.org/3.5/man7/openssl-threads/ Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex
Contributor
|
Benchmark GHA (crypto / export.js): https://github.com/nodejs/node/actions/runs/36720345253 Results
Benchmark results:
|
Contributor
|
Benchmark GHA (crypto / key-details.js): https://github.com/nodejs/node/actions/runs/36720356022 Results
Benchmark results:
|
Contributor
|
Benchmark GHA (crypto / keyobject-serialization.js): https://github.com/nodejs/node/actions/runs/36720367101 Results
Benchmark results:
|
Contributor
|
Benchmark GHA (crypto / class-construction.js): https://github.com/nodejs/node/actions/runs/36720379055 Results
Benchmark results:
|
Contributor
|
Benchmark GHA (crypto / sign.js): https://github.com/nodejs/node/actions/runs/36720391744 Results
Benchmark results:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66413 +/- ##
==========================================
- Coverage 90.39% 90.37% -0.03%
==========================================
Files 792 792
Lines 275580 275658 +78
Branches 52840 52862 +22
==========================================
+ Hits 249104 249119 +15
- Misses 16897 16942 +45
- Partials 9579 9597 +18
🚀 New features to boost your workflow:
|
panva
force-pushed
the
crypto-key-mutexes
branch
from
September 30, 2026 16:30
15b1e98 to
286570b
Compare
addaleax
reviewed
Sep 30, 2026
addaleax
left a comment
addaleax
left a comment
Member
There was a problem hiding this comment.
Might be helpful to reference documentation that says these methods are threadsafe, including whether that's a guarantee across BoringSSL/OpenSSL. (If it's about allowing concurrency, an RwLock would allow that, but I get that if it's truly unnecessary then there's no point in adding that)
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
3 participants
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.
Remove the shared KeyObjectData mutex and its 18 acquisitions, allowing RSA-OAEP and KEM jobs to run concurrently on the same backing key. This also removes unnecessary locking around verify setup, key exports, and metadata queries, plus unused mutex allocations for secret keys.
Assisted-by: Codex