crypto: add crypto.parsePKCS12() - #65627
Conversation
Return the private key, end-entity certificate, and CA certificates from a PKCS#12 (.p12/.pfx) bundle as a KeyObject and X509Certificate instances. Node.js already parses PKCS#12 in SecureContext::LoadPKCS12, which backs tls's `pfx` option, but the results are consumed directly into an SSL_CTX and never reach JavaScript. Callers who need the key or the certificates for anything other than an immediate TLS connection have to shell out to `openssl pkcs12` or take a userland dependency. The binding wraps d2i_PKCS12_bio() and PKCS12_parse() and follows their semantics, matching the existing TLS path: the first private key is returned, the end-entity certificate is the one associated with that key, and any remaining certificates are returned through `ca`. A bundle containing no private key reports `cert` as null and returns its certificates through `ca`. Absent and empty passphrases are kept distinct, since OpenSSL treats them differently. Bundles that require OpenSSL's legacy provider throw ERR_CRYPTO_UNSUPPORTED_OPERATION, reusing the error added for the TLS path. Signed-off-by: bmuenzenmeyer <brian.muenzenmeyer@gmail.com>
|
Review requested:
|
panva
left a comment
There was a problem hiding this comment.
The direction looks alright. I noted a few issues to work through.
| return { | ||
| key: keyHandle === null ? null : new PrivateKeyObject(keyHandle), | ||
| cert: certHandle === null ? null : new InternalX509Certificate(certHandle), | ||
| ca: ArrayPrototypeMap(caHandles, (h) => new InternalX509Certificate(h)), |
There was a problem hiding this comment.
ca does not necessarily contain CA certificates or trust anchors; it contains every certificate other than the one matching the first private key. The certificate-only fixture even returns an end-entity certificate through ca. Could the result instead be { privateKey, certificate, additionalCertificates }?
| if (len > 0) abv->CopyContents(pass_storage.data(), len); | ||
| // An explicitly-supplied empty passphrase is NOT the same as no | ||
| // passphrase; both reach OpenSSL, and they behave differently. | ||
| pass = pass_storage.c_str(); |
There was a problem hiding this comment.
PKCS12_parse() has no password-length parameter, so c_str() silently truncates at an embedded NUL. I verified that both "sample\0junk" and Buffer.from("sample\0junk") open a PFX protected by "sample". Could we reject embedded NUL bytes and add regression tests?
| protecting the bundle. Omit for bundles with no passphrase. Omitting this | ||
| option is **not** equivalent to passing an empty string; the two are | ||
| handled differently, and a bundle created with one will not open with the | ||
| other. |
There was a problem hiding this comment.
This is not the behavior exposed by PKCS12_parse(). When given either nullptr or "", OpenSSL tries both PKCS#12 password encodings and uses whichever verifies the MAC. The tests already open the same ec.pfx once with "" and once with omission.
| { | ||
| // Round-trip: a bundle containing one key and one leaf cert, no CA certs. | ||
| const bundle = fixtures.readKey('rsa_cert.pfx'); | ||
| const { key, cert, ca } = crypto.parsePKCS12(bundle, { passphrase: 'sample' }); |
There was a problem hiding this comment.
Please use a fixture that passes with FIPS enabled.
| // OpenSSL treats NULL and "" differently for both MAC verification and bag | ||
| // decryption, so callers must not collapse them. PKCS12_parse() verifies the | ||
| // MAC itself when one is present, and tolerates the NULL / "" ambiguity. | ||
| PKCS12ParseResult ParseBundle(const BIOPointer& bio, const char* pass) { |
There was a problem hiding this comment.
Could this direct parser be shared with SecureContext::LoadPKCS12() instead of maintaining two implementations of the same decoding? TLS can consume the shared parse result and then apply its stricter requirement that both a key and certificate are present.
There was a problem hiding this comment.
would you be open to a follow up PR to do this? I'd considered the same pathway you suggest here for DRYness sake, but opted to keep the scope smaller and reviewable especially as I am learning the codebase
There was a problem hiding this comment.
if you are open to it, i'll look :) just trying to guard everyone's time
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65627 +/- ##
==========================================
- Coverage 90.05% 90.04% -0.01%
==========================================
Files 754 755 +1
Lines 255727 255877 +150
分支 48312 48348 +36
==========================================
+ Hits 230286 230408 +122
- Misses 16564 16569 +5
- Partials 8877 8900 +23
🚀 新建 features to boost your workflow:
|
This reverts commit 42bc331.
Return the private key, end-entity certificate, and CA certificates from a PKCS#12 (.p12/.pfx) bundle as a KeyObject and X509Certificate instances.
Reading a
.p12/.pfxbundle from JavaScript today means shelling out to theopenssl pkcs12CLI or taking a userland dependency such as node-forge. In talking to a colleague about this unfortunate missing method in core, I (with Claude) noticed Node.js already parses this internally.SecureContext::LoadPKCS12has backed tls'spfxoption for years, but its results are loaded straight into anSSL_CTXand never reach JavaScript. The capability is in the runtime already; it just isn't reachable. This exposes it.Returns
{ key: KeyObject|null, cert: X509Certificate|null, ca: X509Certificate[] }.This PR exposes those internals to end users. Our use case is loading identity files supplied by our environment, to be forwarded during MCP tool calls. This allows us to use real identity instead of service account.
Note
This is my first significant contribution to core that touches the internals. I am still getting my bearings with regard to the module mechanics, bindings, and c++. I'm committed to shaping this, but learning.