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:
|
nodejs-github-bot
added
lib / src
panva
left a comment
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.
| kKeyEncodingPKCS8, | ||
| kKeyEncodingSPKI, | ||
| kKeyEncodingSEC1, | ||
| parsePKCS12: _parsePKCS12, |
There was a problem hiding this comment.
Please update typings/internalBinding/crypto.d.ts
| // 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.
|
|
||
| ```mjs | ||
| import { parsePKCS12 } from 'node:crypto'; | ||
| import { readFileSync } from 'node:fs'; |
There was a problem hiding this comment.
Please use the const { ... } = await import('...'); syntax.
|
|
||
| ```cjs | ||
| const { parsePKCS12 } = require('node:crypto'); | ||
| const { readFileSync } = require('node:fs'); | ||
|
|
||
| const { key, cert, ca } = parsePKCS12( | ||
| readFileSync('bundle.p12'), | ||
| { passphrase: 'secret' }, | ||
| ); | ||
|
|
||
| console.log(cert.subject); | ||
| console.log(key.export({ type: 'pkcs8', format: 'pem' })); | ||
| ``` |
There was a problem hiding this comment.
| ```cjs | |
| const { parsePKCS12 } = require('node:crypto'); | |
| const { readFileSync } = require('node:fs'); | |
| const { key, cert, ca } = parsePKCS12( | |
| readFileSync('bundle.p12'), | |
| { passphrase: 'secret' }, | |
| ); | |
| console.log(cert.subject); | |
| console.log(key.export({ type: 'pkcs8', format: 'pem' })); | |
| ``` |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65627 +/- ##
==========================================
- Coverage 90.07% 90.05% -0.03%
==========================================
Files 751 752 +1
Lines 254778 255136 +358
Branches 48089 48195 +106
==========================================
+ Hits 229494 229760 +266
- Misses 16464 16531 +67
- Partials 8820 8845 +25
🚀 New features to boost your workflow:
|
3 participants
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.