nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.41k stars 28.99k forks source link

`node:crypto` PEM key import method unexpected/undocumented behaviours #50304

Open panva opened 10 months ago

panva commented 10 months ago

All methods that cover importing PEM of any kind (crypto.createPublicKey(pem), crypto.createPrivateKey(pem), crypto.createPrivateKey(x509), new crypto.X509Certificate(x509)) have the following undocumented, possibly unexpected, behaviour.

const pkcs8 = `
This isn't part of PKCS#8

-----BEGIN PRIVATE KEY-----
MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgiyvo0X+VQ0yIrOaN
nlrnUclopnvuuMfoc8HHly3505OhRANCAAQWUcdZ8uTSAsFuwtNy4KtsKqgeqYxg
l6kwL5D4N3pEGYGIDjV69Sw0zAt43480WqJv7HCL0mQnyqFmSrxj8jMa
-----END PRIVATE KEY-----

NEITHER IS THIS

-----BEGIN PRIVATE KEY-----
THis is clearly invalid
-----END PRIVATE KEY-----
`

crypto.createPrivateKey(pkcs8) // <- this successfully imports the first detected key

All these methods ignore everything outside of the first encountered PEM. Is this expected behaviour ?

panva commented 10 months ago

cc @nodejs/crypto

bnoordhuis commented 10 months ago

Is this expected behaviour ?

Yes. Google doesn't turn up good references but openssl works that way since time immemorial, as far as I'm aware.

panva commented 10 months ago

OpenSSL's behaviours don't necessarily have to translate to Node.js API behaviours.

bnoordhuis commented 10 months ago

You're right they don't have to, but they have, and now here we are. :-)

bnoordhuis commented 10 months ago

@panva unless you have anything to add, can I go and close this?

panva commented 10 months ago

I don't think of this as resolved, the behaviour seems undesirable or at the very least undocumented.

panva commented 10 months ago

cc @tniessen

tniessen commented 10 months ago

Ben is right, this has always been a quirk of OpenSSL. We might at least document this in order to resolve this issue. We use various APIs to interact with PEM, so we'd have to carefully look up what APIs have what quirks around PEM.

bnoordhuis commented 10 months ago

As quirks go this one is pretty inconsequential, arguably not noteworthy enough to encumber the documentation with yet additional trivia.

panva commented 10 months ago

As quirks go this one is pretty inconsequential, arguably not noteworthy enough to encumber the documentation with yet additional trivia.

I agree that for public/private keys this is inconsequential, but how about passing a cert chain to createPublicKey or new X509Certificate()?

bnoordhuis commented 10 months ago

Doesn't seem like a problem to me.