豆豆友情提示:这是一个非官方 GitHub 代理镜像,主要用于网络测试或访问加速。请勿在此进行登录、注册或处理任何敏感信息。进行这些操作请务必访问官方网站 github.com。 Raw 内容也通过此代理提供。
Skip to content

fix(node): support ECDSA with secp256k1 in node:crypto#32390

Merged
bartlomieju merged 3 commits intodenoland:mainfrom
bartlomieju:fix/node-crypto-ecdsa-secp256k1
Mar 2, 2026
Merged

fix(node): support ECDSA with secp256k1 in node:crypto#32390
bartlomieju merged 3 commits intodenoland:mainfrom
bartlomieju:fix/node-crypto-ecdsa-secp256k1

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

Summary

  • Adds secp256k1 ECDSA support to node:crypto key generation, signing, verification, and key import/export (DER, PEM, JWK)
  • Adds Secp256k1 variant to EcPrivateKey/EcPublicKey enums using the existing k256 crate
  • Enables ecdh, jwk, pem features on k256 workspace dependency

Closes #27523

Details

The k256 crate was already used for ECDH with secp256k1, but the ECDSA key generation/signing/verification paths only supported P-224, P-256, and P-384. This PR adds secp256k1 support across all EC code paths:

  • Key generation: generateKeyPair('ec', { namedCurve: 'secp256k1' }) now works
  • Signing/verification: sign(), verify(), createSign(), createVerify() with secp256k1 keys
  • Key import/export: PKCS#8, SEC1, SPKI (DER/PEM), and JWK formats
  • Key details: Reports namedCurve: "secp256k1" in asymmetricKeyDetails
  • X.509: Parsing secp256k1 public keys from certificates
  • DH: diffieHellman() with secp256k1 KeyObject pairs

Test plan

  • Added unit tests for key generation, sign/verify (one-shot and streaming), PEM export/import roundtrip, and async key generation
  • cargo test unit_node::crypto::crypto_key_test passes

🤖 Generated with Claude Code

Adds secp256k1 support for EC key generation, signing, verification,
and key import/export (DER, PEM, JWK) in the node:crypto polyfill.
Previously only ECDH was supported for secp256k1 via the k256 crate.

Closes denoland#27523

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.

Good start but incomplete implementation. This PR adds secp256k1 key support but misses critical signing/verification operations.

✅ What works

  • Key generation: generateKeyPair('ec', { namedCurve: 'secp256k1' })
  • Key import/export: PEM, DER, JWK formats
  • ECDH operations (existing support via k256 crate)
  • Proper OID handling (1.3.132.0.10)

❌ What's missing

  • Signing/verification operations - no secp256k1 case in ext/node_crypto/sign.rs
  • sign(), verify(), createSign(), createVerify() will fail at runtime
  • Pattern established for P224/P256/P384 but secp256k1 not added

The gap

In ext/node_crypto/sign.rs, the EcPrivateKey match around line 175 is incomplete:

EcPrivateKey::P224(key) => { /* signing logic */ }
EcPrivateKey::P256(key) => { /* signing logic */ } 
EcPrivateKey::P384(key) => { /* signing logic */ }
// EcPrivateKey::Secp256k1(key) => { /* MISSING */ }

Fix needed

Add secp256k1 signing support in sign.rs:

EcPrivateKey::Secp256k1(key) => {
  let signing_key = k256::ecdsa::SigningKey::from(key);
  let signature: k256::ecdsa::Signature = signing_key
    .sign_prehash(digest)
    .map_err(|_| KeyObjectHandlePrehashedSignAndVerifyError::FailedToSignDigest)?;
  dsa_signature(dsa_signature_encoding, signature)
}

Same pattern needed in the verification path for EcPublicKey::Secp256k1.

The k256 crate has the ecdsa feature enabled so the signing types should be available.

Testing

The current tests likely only cover key generation. Need to verify actual sign/verify operations work end-to-end, not just key creation and export.

Copy link
Copy Markdown
Member Author

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Thanks for the review! However, the signing/verification support is already implemented in this PR.

sign.rs already has both:

  • Signing (line 200): EcPrivateKey::Secp256k1 match arm with k256::ecdsa::SigningKey
  • Verification (line 326): EcPublicKey::Secp256k1 match arm with k256::ecdsa::VerifyingKey

The tests also cover actual sign/verify operations end-to-end — not just key generation:

  • ec secp256k1 sign and verify — tests sign()/verify() one-shot API
  • ec secp256k1 createSign and createVerify — tests streaming API
  • ec secp256k1 export and import PEM — tests PEM roundtrip + signing with imported keys

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@bartlomieju bartlomieju requested a review from littledivy March 2, 2026 09:25
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.

Found Node.js compatibility issue with asymmetricKeyDetails

Tested against Node.js and found that Deno returns incorrect curve names in asymmetricKeyDetails.

Current behavior (Deno):

  • P-256 → { named_curve: "p256" }
  • P-384 → { named_curve: "p384" }
  • secp256k1 → { named_curve: "secp256k1" }

Expected behavior (Node.js):

  • P-256 → { namedCurve: 'prime256v1' }
  • P-384 → { namedCurve: 'secp384r1' }
  • secp256k1 → { namedCurve: 'secp256k1' }

Node.js uses OpenSSL standard curve names via OBJ_nid2sn(nid).

Required fix: Update curve name mappings in ext/node_crypto/keys.rs lines 1852-1855 and 1906-1909:

// Lines 1852-1855
let named_curve = match key {
  EcPrivateKey::P224(_) => "secp224r1",    // was "p224"
  EcPrivateKey::P256(_) => "prime256v1",   // was "p256"
  EcPrivateKey::P384(_) => "secp384r1",    // was "p384"
  EcPrivateKey::Secp256k1(_) => "secp256k1", // correct
};

Otherwise this breaks Node.js compatibility for P-224/P-256/P-384 curves.

Node.js returns OpenSSL short names (prime256v1, secp384r1, secp224r1)
via OBJ_nid2sn() rather than NIST names (p256, p384, p224).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@bartlomieju bartlomieju merged commit fd9def3 into denoland:main Mar 2, 2026
112 checks passed
@bartlomieju bartlomieju deleted the fix/node-crypto-ecdsa-secp256k1 branch March 2, 2026 13:21
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.

node:crypto implementation doesn't support ECDSA secp256k1

3 participants