Skip to content

fix(ext/node): improve crypto.generateKeyPair validation#32620

Merged
bartlomieju merged 3 commits intomainfrom
fix/crypto-keygen-validation
Mar 12, 2026
Merged

fix(ext/node): improve crypto.generateKeyPair validation#32620
bartlomieju merged 3 commits intomainfrom
fix/crypto-keygen-validation

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju commented Mar 10, 2026

Summary

  • Restructures generateKeyPair to call createJob synchronously so validation errors throw synchronously (matching Node.js behavior)
  • Adds key encoding validation (format, type, cipher, passphrase) with proper Node.js error codes (ERR_INVALID_ARG_VALUE, ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, ERR_CRYPTO_UNKNOWN_CIPHER)
  • Adds DH group name validation (ERR_CRYPTO_UNKNOWN_DH_GROUP)
  • Adds RSA-PSS hash algorithm validation (ERR_CRYPTO_INVALID_DIGEST)
  • Fixes RSA key generation to return Result instead of panicking on invalid parameters, and validates public exponents to prevent infinite loops with even/small exponents
  • Fixes EC curve error message to match Node.js
  • Supports parsing private key PEM/DER in publicEncrypt to extract the public key (matching Node.js behavior)
  • Rejects encrypt/decrypt operations on non-RSA key types (rsa-pss, dsa, ec, etc.) with "operation not supported for this keytype"
  • Rejects PKCS1 padding for RSA-PSS keys in sign/verify with "illegal or unsupported padding mode"
  • Enables 6 new Node compat tests: test-crypto-keygen, test-crypto-keygen-sync, test-crypto-keygen-rsa-pss, test-crypto-keygen-eddsa, test-crypto-keygen-key-object-without-encoding, test-crypto-keygen-promisify

bartlomieju and others added 2 commits March 10, 2026 18:05
Adds proper synchronous validation to generateKeyPair/generateKeyPairSync
matching Node.js behavior:

- Restructure generateKeyPair to call createJob synchronously so
  validation errors throw synchronously instead of rejecting a promise
- Add key encoding validation (format, type, cipher, passphrase) with
  proper error codes (ERR_INVALID_ARG_VALUE, ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS)
- Add DH group name validation (ERR_CRYPTO_UNKNOWN_DH_GROUP)
- Add RSA-PSS hash algorithm validation (ERR_CRYPTO_INVALID_DIGEST)
- Fix RSA key generation to return Result instead of panicking on
  invalid parameters
- Add RSA public exponent validation to prevent infinite loops with
  even/small exponents
- Fix EC curve error message to match Node.js
- Enable test-crypto-keygen.js Node compat test

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… padding

- Support parsing private key PEM/DER in publicEncrypt to extract the
  public key (matching Node.js behavior where publicEncrypt accepts
  private keys)
- Reject encrypt/decrypt operations on non-RSA key types (rsa-pss, dsa,
  ec, ed25519, etc.) with "operation not supported for this keytype"
- Reject PKCS1 padding for RSA-PSS keys in sign/verify operations with
  "illegal or unsupported padding mode"
- Enable 5 new Node compat tests: test-crypto-keygen-sync,
  test-crypto-keygen-rsa-pss, test-crypto-keygen-eddsa,
  test-crypto-keygen-key-object-without-encoding,
  test-crypto-keygen-promisify

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@kajukitli kajukitli left a comment

Choose a reason for hiding this comment

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

lgtm, solid set of validation improvements

highlights:

  • sync validation errors now throw sync (not in promise rejection) — matches Node.js
  • RSA exponent validation prevents infinite loops with even/small exponents
  • good coverage of error codes matching Node.js (ERR_CRYPTO_INCOMPATIBLE_KEY_OPTIONS, ERR_CRYPTO_UNKNOWN_DH_GROUP, etc.)
  • rejecting PKCS1 padding for RSA-PSS keys is correct

one minor note on the DH group check:

if (
  group !== "modp5" && group !== "modp14" && group !== "modp15" &&
  group !== "modp16" && group !== "modp17" && group !== "modp18"
) {

could use a Set for cleaner lookup, but it's only 6 values so not a big deal.

enabling 6 new node compat tests is a nice win 👍

…alidation

# Conflicts:
#	ext/node/polyfills/internal/crypto/keygen.ts
#	tests/node_compat/config.jsonc
@bartlomieju bartlomieju requested a review from littledivy March 11, 2026 23:10
Copy link
Copy Markdown
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 5b9986d into main Mar 12, 2026
112 checks passed
@bartlomieju bartlomieju deleted the fix/crypto-keygen-validation branch March 12, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants