src: decouple KeyObject and CryptoKey and move CryptoKey to src - #12
Closed
panva wants to merge 11 commits into
Closed
src: decouple KeyObject and CryptoKey and move CryptoKey to src#12panva wants to merge 11 commits into
panva wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Node.js’ WebCrypto CryptoKey implementation to decouple it from KeyObject by moving CryptoKey’s internal-slot storage into a new native NativeCryptoKey base and updating internal crypto code to use slot accessors instead of JS-visible properties.
Changes:
- Introduces
NativeCryptoKey(C++) plus JS-side slot helpers (getCryptoKey{Type,Extractable,Algorithm,Usages,Handle}) and updates WebCrypto/internal consumers accordingly. - Migrates WebCrypto key generation/import/export paths from
KeyObjectwrappers toKeyObjectHandleusage, including structured-clone/worker transfer support. - Adds targeted regression tests for slot hiding/brand checks/clone-transfer and adds WebCrypto sign/verify benchmarks.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/parallel/test-webcrypto-cryptokey-no-own-symbols.js | New coverage ensuring CryptoKey instances don’t expose own symbol/name keys. |
| test/parallel/test-webcrypto-cryptokey-hidden-slots.js | New coverage ensuring internal consumers/inspect bypass user-mutable prototype getters. |
| test/parallel/test-webcrypto-cryptokey-clone-transfer.js | New coverage for structuredClone and worker/message cloning of CryptoKey. |
| test/parallel/test-webcrypto-cryptokey-brand-check.js | New coverage for robust receiver brand checks on CryptoKey getters. |
| src/node_crypto.cc | Registers NativeCryptoKey with crypto binding init machinery. |
| src/env_properties.h | Adds env property for internal CryptoKey constructor used during transfer deserialize. |
| src/crypto/crypto_rsa.cc | Relaxes RSA keygen argument-count checks to support handle-returning job usage. |
| src/crypto/crypto_keys.h | Declares new NativeCryptoKey BaseObject with slot access + transfer support. |
| src/crypto/crypto_keys.cc | Implements NativeCryptoKey initialization, slot retrieval, and clone/transfer behavior. |
| lib/internal/util/comparisons.js | Updates deep-equal semantics for CryptoKey to use slot accessors and handle equality. |
| lib/internal/crypto/webidl.js | Switches WebIDL validators to use internal slot helpers instead of public getters. |
| lib/internal/crypto/webcrypto_util.js | Changes key import helpers to return KeyObjectHandle (plus secret-key import helpers). |
| lib/internal/crypto/webcrypto.js | Replaces direct symbol-slot reads with slot helper calls across WebCrypto ops. |
| lib/internal/crypto/util.js | Removes exported kKeyObject symbol (part of decoupling). |
| lib/internal/crypto/rsa.js | Switches RSA keygen/import/export/sign-verify to handle-based plumbing and jobs. |
| lib/internal/crypto/pbkdf2.js | Uses getCryptoKeyHandle() for PBKDF2 deriveBits inputs. |
| lib/internal/crypto/ml_kem.js | Uses Nid keygen job + handle-based import/export/cipher operations. |
| lib/internal/crypto/ml_dsa.js | Uses Nid keygen job + handle-based import/export/sign-verify operations. |
| lib/internal/crypto/mac.js | Migrates HMAC/KMAC keygen/import/sign-verify to handle-based plumbing/jobs. |
| lib/internal/crypto/keys.js | Replaces symbol-based CryptoKey storage with NativeCryptoKey + private-field caching and helper APIs. |
| lib/internal/crypto/hkdf.js | Updates HKDF deriveBits to use CryptoKey handle path. |
| lib/internal/crypto/ec.js | Migrates EC keygen/import/export/sign-verify to handle-based plumbing/jobs. |
| lib/internal/crypto/diffiehellman.js | Uses slot helpers and handles for ECDH deriveBits. |
| lib/internal/crypto/chacha20_poly1305.js | Migrates keygen/import/cipher to handle-based plumbing/jobs. |
| lib/internal/crypto/cfrg.js | Migrates CFRG algos to Nid keygen + handle-based import/export/sign-verify. |
| lib/internal/crypto/argon2.js | Uses getCryptoKeyHandle() for Argon2 deriveBits key material. |
| lib/internal/crypto/aes.js | Migrates AES keygen/import/cipher to handle-based plumbing/jobs. |
| lib/eslint.config_partial.mjs | Adds lint restriction guiding getCryptoKeyHandle import source. |
| benchmark/crypto/webcrypto-verify.js | Adds benchmark for WebCrypto verify throughput across algorithms. |
| benchmark/crypto/webcrypto-sign.js | Adds benchmark for WebCrypto sign throughput across algorithms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
panva
force-pushed
the
native-cryptokey
branch
2 times, most recently
from
April 24, 2026 10:09
fc03610 to
e3cb124
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Introduces a C++ `NativeCryptoKey` class that holds the real CryptoKey slots (`handle_data_`, `algorithm_`, `usages_`, `extractable_`) and provides structured-clone and worker-transfer support through a dedicated `CryptoKeyTransferData`. New bindings `createCryptoKeyClass`, `getCryptoKeyHandle`, and `getCryptoKeySlots` expose the class and accessors to JS; a brand-tag class-ID pointer rejects spoofed receivers on the accessors. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
JS-side `CryptoKey` now extends the native `NativeCryptoKey` created via
`createCryptoKeyClass`. `InternalCryptoKey` caches the
`[type, extractable, algorithm, usages, handle]` slot tuple in a
`#slots` private field on first access; the public getters and
`isCryptoKey` route through dedicated `getCryptoKey{Type,Extractable,
Algorithm,Usages,Handle}` helpers re-exported from this module. Symbol
property storage (`kKeyObject`, `kAlgorithm`, `kExtractable`,
`kKeyUsages`, `kCachedAlgorithm`, `kCachedKeyUsages`) is gone;
`kKeyObject` is dropped from `internal/crypto/util` exports.
`deepStrictEqual` on CryptoKey switches to the new accessors plus
`handle.equals()` (instead of structural compare on wrapper objects).
An ESLint rule forbids destructuring `getCryptoKeyHandle` from
`internalBinding('crypto')`; it must come from `internal/crypto/keys`
so the brand-check path is used.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Every Web Crypto algorithm module drops `key[kKeyObject][kHandle]`
symbol access in favor of `getCryptoKey*` accessors and stores the raw
`KeyObjectHandle` directly (no more `SecretKeyObject`/`PublicKeyObject`/
`PrivateKeyObject` wrappers on the CryptoKey slot). Key generation
moves off the promisified `generateKey`/`generateKeyPair` paths onto
ones that return `KeyObjectHandle` (i.e. not wrapped in KeyObject):
- `SecretKeyGenJob` for AES, ChaCha20-Poly1305, HMAC/CMAC (aes.js,
chacha20_poly1305.js, mac.js).
- `EcKeyPairGenJob` for ECDSA, ECDH (ec.js).
- `NidKeyPairGenJob` for Ed25519/Ed448/X25519/X448, ML-DSA-{44,65,87},
ML-KEM-{512,768,1024} (cfrg.js, ml_dsa.js, ml_kem.js).
- `RsaKeyPairGenJob` for RSASSA-PKCS1-v1_5, RSA-PSS, RSA-OAEP (rsa.js).
`webcrypto_util.js` switches its import helpers (`importDerKey`,
`importJwkKey`, `importRawKey`) to return raw `KeyObjectHandle`s and
adds `importSecretKey` / `importJwkSecretKey` helpers for symmetric
imports. `webcrypto.js` follows the same pattern; its `getPublicKey`
builds a temporary wrapper and carries a TODO for future refactor.
`hkdf.js` switches from `promisify(hkdf)` to
`jobPromise(() => new HKDFJob(...))` directly. `diffiehellman.js`,
`argon2.js`, and `pbkdf2.js` are accessor-only migrations (the
`TODO(panva)` notes on argon2/pbkdf2 are orthogonal and left for a later
cleanup).
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Loosens two `AdditionalConfig` precondition checks so the new Web Crypto
keygen jobs added earlier (RsaKeyPairGenJob, EcKeyPairGenJob) can reuse
the shared traits without threading unused encoding args through the job
constructor.
- `RsaKeyGenTraits::AdditionalConfig` now CHECKs RSA key
type-dependant argument count accounting for being able to skip
unused parameters.
- `EcKeyGenTraits::AdditionalConfig` now defaults `param_encoding` to
`OPENSSL_EC_NAMED_CURVE`, this is not observable in existing
crypto.generateKeyPair(Sync) as its dispatch already applies the
same default. This is just so that a stray OPENSSL_EC_NAMED_CURVE
isn't needed in ec.js
Pure precondition relaxation — no new code paths. Existing
`generateKeyPair` callers still pass the same args and hit the same
branches.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Enforces a 1024-byte maximum on `HkdfParams.info` at the WebIDL layer using refactored `validateMaxBufferLength`. Its error message is also fixed. Oversized `info` was already rejected via a different code path. It just relocates the rejection earlier into the WebIDL conversion step so the failure reproduces the OpenSSL's limitation early. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Tightens `validateJwk` to require the per-`kty` string members up front
(before switching to C++), short-circuiting with the same
`Invalid keyData` message and `DataError` when the JWK is missing
required fields or passes a non-string value for one. In theory non-strings
are already rejected by WebIDL's JWK converter but this doesn't hurt.
- RSA: requires `n`, `e`; if `d` is present, also requires `p`, `q`,
`dp`, `dq`, and `qi`.
- EC: requires `crv`, `x`, `y`; optional `d`.
- OKP: requires `crv`, `x`; optional `d`.
- oct: requires `k`.
- AKP: requires `alg`, `pub`; optional `priv`.
Four export/import negative tests update their expected error text from
the later "Invalid JWK … Parameter and algorithm name mismatch" to the
new short-circuit "Invalid keyData" (for the case where `crv`/`alg` is
missing entirely). A new `{ kty: 'oct' }` missing-`k` negative is added
for ChaCha20-Poly1305. The tests check error messages but the error
class (DataError/DOMException) is the same everywhere.
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Adds four focused tests that check guarantees either introduced by the
native `CryptoKey` refactor or carried over from the existing state.
- `test-webcrypto-cryptokey-brand-check.js` - each of the four
prototype getters (`type`, `extractable`, `algorithm`, `usages`)
throws `ERR_INVALID_THIS` for foreign receivers (plain objects,
null-proto, primitives, null/undefined, functions, a subverted
`Symbol.hasInstance`, and a real `BaseObject` of a different kind).
`util.types.isCryptoKey()` remains accurate after a prototype
swap, confirming it cannot be spoofed.
- `test-webcrypto-cryptokey-clone-transfer.js` - exhaustive
structured-clone, `MessagePort.postMessage`, and
`Worker.postMessage` round-trips.
Verifies slot preservation, inspect-output equivalence, and that
crypto operations interoperate across clones including repeated
round-trips.
- `test-webcrypto-cryptokey-hidden-slots.js` - replaces all four
prototype getters with forged versions and confirms internal
consumers (export, inspect) still read the real native slots.
- `test-webcrypto-cryptokey-no-own-symbols.js`, asserts CryptoKey
instances expose no own symbol-keyed properties even after every
public getter has been touched (proof the `#slots` private field
plus native storage leaves the instance shape pristine).
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Adds two benchmarks exercising `subtle.sign` / `subtle.verify` across the main algorithm families: - ECDSA P-256 - RSASSA-PKCS1-v1_5 - RSA-PSS - Ed25519 - ML-DSA-44 (gated on OpenSSL >= 3.5) Each benchmark runs two modes (serial / parallel) × two key-reuse patterns (shared / unique per call) so regressions in slot-access hot paths (getCryptoKey* accessors) and in per-call key wrapping surface on both dimensions. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Signed-off-by: Filip Skokan <panva.ip@gmail.com>
panva
pushed a commit
that referenced
this pull request
Jun 6, 2026
This function call can fail with `Z_VERSION_ERROR` if the compiled
library vs loaded library mismatched in version number or in
stream structure size.
In those cases, zlib doesn't initialize the `strm_.msg` field to
null. Therefore, when a `CompressionError` object is created via
`ErrorForMessage()`, it can read a stale or uninitialized `strm_.msg`
pointer that will cause a crash.
Example ASAN report:
```
AddressSanitizer: SEGV on unknown address
#0 __strlen_avx2
string/../sysdeps/x86_64/multiarch/strlen-avx2.S:76
#1 strlen (/work/node/out/Debug/node+0x1a42ab7)
#2 v8::(anonymous namespace)::StringLength(char const*)
/work/node/out/../deps/v8/src/api/api.cc:7581:16
#3 v8::(anonymous namespace)::StringLength(unsigned char const*)
/work/node/out/../deps/v8/src/api/api.cc:7587:10
#4 v8::String::NewFromOneByte(v8::Isolate*,
unsigned char const*, v8::NewStringType, int)
/work/node/out/../deps/v8/src/api/api.cc:7677:3
#5 node::OneByteString(v8::Isolate*,
char const*, int, v8::NewStringType)
/work/node/out/../src/util-inl.h:166:10
nodejs#6 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::EmitError(node::(anonymous namespace)
::CompressionError const&)
/work/node/out/../src/node_zlib.cc:565:7
nodejs#7 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::CheckError()
/work/node/out/../src/node_zlib.cc:519:5
#8 node::(anonymous namespace)::CompressionStream<
node::(anonymous namespace)::ZlibContext>
::AfterThreadPoolWork(int)
/work/node/out/../src/node_zlib.cc:543:10
#9 node::ThreadPoolWork::ScheduleWork()
::'lambda'(uv_work_s*, int)
::operator()(uv_work_s*, int) const
/work/node/out/../src/threadpoolwork-inl.h:57:15
#10 node::ThreadPoolWork::ScheduleWork()
::'lambda'(uv_work_s*, int)
::__invoke(uv_work_s*, int)
/work/node/out/../src/threadpoolwork-inl.h:48:7
#11 uv__work_done /work/libuv-1.51.0/src/threadpool.c:330:5
#12 uv__async_io.part.0
/work/libuv-1.51.0/src/unix/async.c:208:5
```
Signed-off-by: ndossche <nora.dossche@ugent.be>
PR-URL: nodejs#63476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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.
CI: https://ci.nodejs.org/job/node-test-commit/87270/CI: https://ci.nodejs.org/job/node-test-commit/87272/