fix(ext/node): support PKCS#8 encrypted private key PEM export via PBES2#33758
Conversation
Enables tests/node_compat/runner/suite/test/parallel/test-crypto-keygen-async-named-elliptic-curve-encrypted-p256.js Co-authored-by: Divy Srivastava <[email protected]>
divybot
changed the title
The test-crypto-keygen-async-named-elliptic-curve-encrypted-p256 node compatibility test asserts the exact OpenSSL 3 error string when attempting to use an encrypted PKCS#8 key without a passphrase. Update the AsymmetricPrivateKeyError variant message to match. Also wrap an over-length .map_err line in encrypt_pkcs8_der so the formatter is satisfied. Co-authored-by: Divy Srivastava <[email protected]>
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Verified: (1) the new error string error:07880109:common libcrypto routines::interrupted or cancelled at keys.rs:632 matches Node's own assertion in test/parallel/test-crypto-keygen-async-named-elliptic-curve-encrypted-p256.js for the hasOpenSSL3 branch — faithful Node-parity, not a random OpenSSL string. (2) The crypto_key_test.ts assertion flip from BEGIN PRIVATE KEY to BEGIN ENCRYPTED PRIVATE KEY is correct — that test already passes cipher: "aes-256-cbc" + passphrase, so the prior assertion was hiding a bug where the cipher was silently dropped. CI 133/136 success, 2 skip, 1 cla pending.
One non-blocking smell inline on the parse-error mapping in encrypt_pkcs8_der. Worth a follow-up; doesn't gate this.
| fn encrypt_pkcs8_der( | ||
| data: &[u8], | ||
| cipher: &str, | ||
| passphrase: &[u8], |
There was a problem hiding this comment.
Minor: PrivateKeyInfo::try_from(data).map_err(|err| ExportPrivateKeyPemError::UnsupportedCipher(err.to_string())) mislabels a DER parse failure as an unsupported-cipher error. If data is malformed PKCS#8 DER (corrupted upstream), the user sees "Unsupported cipher: ..." which sends them debugging the wrong thing.
Not a blocker — the upstream call sites pass DER bytes that just came out of op_node_export_private_key_pem's own encoder so they're well-formed in practice. But worth a follow-up to add an InvalidPkcs8Der variant (or reuse an existing parse-error variant) so the diagnostic matches the actual failure.
Address review feedback: when encrypt_pkcs8_der is called with malformed DER bytes, surface "invalid PKCS#8 DER: ..." via a dedicated InvalidPkcs8Der variant instead of mislabeling it as UnsupportedCipher, which would send users debugging the wrong thing. Co-authored-by: Divy Srivastava <[email protected]>
…ariant The previous variant fired from both the PEM-import call site and the new encrypted-PKCS#8 DER call site, but Node emits different errors at those sites: a JS-side ERR_MISSING_PASSPHRASE for the encrypted-DER-no-passphrase case, and an OpenSSL3 surface message for the encrypted-PEM path. Add a distinct EncryptedPkcs8DerRequiresPassphrase variant for the DER site so each path can carry its own message and code without conflicting with work in denoland#33758. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
3 participants
Summary
Enables
test-crypto-keygen-async-named-elliptic-curve-encrypted-p256in node_compat suite.Test plan
cargo test --test node_compat -- test-crypto-keygen-async-named-elliptic-curve-encrypted-p256