Repository navigation
Support parsing pem key only once for crypto api #15113
Description
Activity
I don't like passing an opaque object to the
signmethod, and introducing a "parsed key" class would increase complexity. I think it would be a cleaner API to allow passing the PEM key tocreateSignand allow reusing the resultingSignobject.@seishun This get a bit problematic to use when you are streaming data as what it seems the current API is build for. I'm guessing the hash is being incrementally calculated when doing the 'write' calls and the 'sign' call then encrypts that to provide the signature.
The example below shows a sample where using a shared sign object would give an unexpected result:
const crypto = require('crypto'); // Create shared sign object const sign = crypto.createSign('RSA-SHA256', getPrivateKeySomehow()); // Start taking requests. const server = http.createServer((req, res) => { req.on('data', (chunk) => { // Here we are mixing data between the requests sign.write(chunk); }).on('end', () => { sign.end(); response.end(sign.sign(privateKey, 'hex')) }); ...
Most other languages have a "parsed key" class as there are many different ways signing can be done, fx. both C# and Java can use different underlying signing implementations fx. when using HSM devices or running on other platforms where the key material is not a available to application. Fx. C# uses windows certificate store on windows, keychain on mac and openssl on Linux. Java use a hardware backed keystore on Android, etc.
Having someone kind of abstraction would make the whole key handling a lot more flexible, fx. imagine using a hardware backed keystore for storage of the private key.
For inspiration here is how the API looks in Java, it's similar to Node with the exception that you pass a key object so it's only constructed once.
PrivateKey privatekey = customPemToPrivateKey(getPrivateKeySomehow()) for(let i = 0; i < 10000; i++) { Signature signature = Signature.getInstance("SHA256withRSA"); signature.initSign(privatekey); signature.update(String.getBytes("some data to sign" + i, StandardCharsets.UTF_8)); byte[] signatureBytes = signature.sign(); }
And here in C# where the "key" in the focal point of the interface, so the "key" object has a sign function.
SHA256 sha256 = SHA256.Create(); RSACryptoServiceProvider privatekey = customPemToCryptoProvider(getPrivateKeySomehow()) for(let i = 0; i < 10000; i++) { byte[] signatureBytes = privatekey.SignData(Encoding.UTF8.GetBytes("some data to sign" + i), sha256); }
cc @nodejs/crypto
I'd like to revive the idea. After having worked on the crypto module a lot recently, I think having key objects could actually be advantageous for us:
- Once a key has been created, it is guaranteed to be valid and not malformed.
- Currently, users need to keep the passphrase for encrypted PKCS#8 keys or the unencrypted key in JS-managed memory which isn't optimal from a security point of view. Having key objects would allow us to protect the memory, see crypto.alloc() for encryption key memory management #18896.
- We would finally be able to work with keys stored in engines! (see Support parsing pem key only once for crypto api #15113 (comment))
- A crypto key API would allow users to seamlessly convert between PKCS#1 / SPKI and PKCS#1 / PKCS#8 / SEC1 and even between PEM / DER.
- All existing APIs can be adapted to support key objects.
- The way we currently handle keys is a mess, to be honest, and having key objects could greatly simplify that.
- Key objects would play nicely with the API being designed in crypto: add key pair generation #22660.
- A carefully designed key API would allow users to implement the JWK key format rather easily compared to the current situation.
I am currently trying to assess some API drafts, I'll let you know when I have a proposal.
Reacted by Troels Liebe Bentsen and Juan Campa@tniessen this would also enable us to work with keys stored outside the nodejs process, fx. in keychain on mac, certificate store on Windows, GNOME Keyring on linux, TPM (Trusted Platform Module) or HSM's (Hardware Security Module) like Yubikey, TouchID, smartcards, etc.
Fx. if we stick with openssl it can be extended with an engine config: https://developers.yubico.com/YubiHSM2/Usage_Guides/OpenSSL_with_pkcs11_engine.html
Reacted by Tobias Nießenthis would also enable us to work with keys stored outside the nodejs process, fx. in keychain on mac, certificate store on Windows, GNOME Keyring on linux, TPM (Trusted Platform Module) or HSM's (Hardware Security Module) like Yubikey, TouchID, smartcards, etc.
@tlbdk Will that require support for custom key objects (user-defined classes)? That would make a secure implementation more difficult.
@tniessen No, not if the implementation detail is left to OpenSSL fx using its engine API to add the different HSM backends, it already has the needed abstraction.
There is already API for setting the engine:
https://nodejs.org/api/crypto.html#crypto_crypto_setengine_engine_flags
But we need an API to load keys from engines:
- https://www.openssl.org/docs/man1.1.1/man3/ENGINE_load_private_key.html
- https://github.com/OpenSC/libp11/blob/09b4b9c6253ca5d2868f0685675d91ad98a512c1/tests/rsa-pss-sign.c#L137
fx.
const privateKey = crypto.readEnginePrivateKey("keyid"); // This would just be a wrapper around the openssl key refThe key object class could have the same the suggested conversion methods but would just return null when the engine does give access to the underlying key data.
Key objects have landed. Please let us know whether there is a feature you would like to see that is still missing. (Loading keys from engines is not supported yet.)
Would be nice when it's available for the key object and should be fairly simple with the EVP_PKEY_get* methods:
https://www.openssl.org/docs/man1.0.2/crypto/EVP_PKEY_get1_RSA.html
https://www.openssl.org/docs/man1.0.2/crypto/rsa.html#DESCRIPTIONI guess an api to work directly with public and private exponents, etc. by fx creating the RSA object directly with EVP_PKEY_set would also make some things simpler:
EVP_PKEY* pRsaKey = EVP_PKEY_new(); RSA* rsa = RSA_new(); rsa->e = e; rsa->n = n; EVP_PKEY_assign_RSA(pRsaKey, rsa);At the moment I have done the code to convert a JWK to PEM in zero dependency javascript, but parsing PEM without an ASN.1 library would be a lot more work:
https://github.com/connectedcars/node-jwtutils/blob/master/src/jwkutils.js6 remaining items
@panva Right now Node.js is using OpenSSL to do the PEM parsing and that internally is doing the ASN.1 decoding to a struct that has the key components, the struct can also manually be populated or read into for any other format/engines OpenSSL supports. The API's I linked gives access to the struct so it should be fairly simple to expose that in the key object. But I have not seen an API's exposing the ASN.1 parts without having to decode the same key twice, I did not look that hard so it might be there. My point was just that ASN.1 is an encoding detail better to not expose in an API.
Generating PEM without dependencies is fairly easy if you have the key components. Adding support for converting JWK to PEM in crypto.createPrivateKey or crypto.createPublicKey would be super easy. Would be happy to do a pull request that does exactly that if it would be accepted, @tniessen ? The pretty option would be populating the OpenSSL key struct directly in C world.
jwkutils.js should support all the key formats in JWK, am I missing any?
That would sort:
- crypto.createPrivateKey accepting a JWK
- crypto.createPublicKey accepting a JWK
For exporting to JWK I guess with a bit of cleanup we could get it down to 2-3 dependencies with an ASN.1 library or zero with only doing enough of the ASN.1 decoding to support PEM, but it would be quite a bit of work. Key components from the OpenSSL struct would make this so much easier.
It's a very small portion of JOSE I have actually seen used in the wild so that was my definition of "needed" for now :).
Deriving a PublicKeyObject from PrivateKeyObject would be useful and also calculating the public key fingerprint would also be nice.
Would the OpenSSL key API not help with this, it seems to be fairly little code?
It certainly does, the implementation itself is quite simple. The API design is what we should consider carefully.
And why would it be linked to TLS certificate handing(note I have not looked at the code so I might just be missing something obvious)?
TLS already provides ways to access certain properties of the key, but I think @sam-github and I agreed that the property names being used there are not necessarily elegant choices, so I'd be okay with breaking compatibility there.
get a
PublicKeyObjectinstance out ofPrivateKeyObjectoneThis is on my list of upcoming features, I am trying to split them into small incremental changes to make our internal processes easier. Eventually,
createPublicKeywill accept a key object with typeprivate.I think if we attach the key properties to a
.fieldssub-object as @tniessen suggests, that we can apply that to TLS certificates as well as to crypto key objects. The top-level field names in TLS certificate objects would become effectively legacy.Its worth bike-shedding the name since it will live a long time. I mildly lean towards
.infoinstead of.fields(as in SubjectKeyInfo from the ASN.1). Maybe the property names can also come from the ASN.1, I think they are already camelCase.So I agree with @panva that in an ideal world it would be great to get core JWK support. I think node.js and JWK go well together.
For the majority of use-cases having the features mentioned above:
crypto.createPrivateKey accepting a JWK
crypto.createPublicKey accepting a JWK
keyObject.export supporting JWK formatWould remove the need to even access a
.fieldssub-object.Having the key properties in a
.fieldssub-object would make exporting to a JWK fairly easy to do in userland, but I still think that it should belong in core.I mildly lean towards .info instead of .fields
.infogets my vote as well.Having a fields property fx with generic names inspired by JWK would be a nicer solution.
I have to say I agree with @tlbdk . At the keyObject level I shouldn't care whether the key was encoded with
pkcs1orspki. The different encodings result in different ASN.1 sequences.@sam-github @panva @tlbdk Would you expect BIGNUM fields to be exposed as hexadecimal strings or as JS bigints?
Using JWK names is insteresting, if possible. They are described in rfc7518 if anyone is looking (took me a while to find them).
If we go that way, wouldn't it make sense to have the property values be JSON compatible as they are in JWK, so they would be hex strings on output, and bigint/buffer/hexstrings on input?
@sam-github if the properties follow jwk names then the same format would indeed make more sense.
@tniessen IIRC you thought there was some difficulty with JWK support, was it around the key usage fields? They all appear optional to output, and node would have to ignore them on input.
Key usage fields (key_ops and use) are optional use by a given application. Node just needs to ignore all fields that aren’t the actual key components.
IIRC you thought there was some difficulty with JWK support, was it around the key usage fields?
That and the problem of Bigint support in JWK.
@tniessen what issue with bigint support? AFAIK all key material is encoded as
base64url with no paddingBase64urlUInt
Current API parses the PEM files on every crypto operation, fx:
node/src/node_crypto.cc
Line 4168 in 0d22858
I would be nice if this could be done once and a ref to the openssl key could be retained: