Support parsing pem key only once for crypto api

Current API parses the PEM files on every crypto operation, fx:

https://nodejs.org/api/crypto.html#crypto_class_sign
node/src/node_crypto.cc

Line 4168 in 0d22858

pkey = PEM_read_bio_PrivateKey(bp,

I would be nice if this could be done once and a ref to the openssl key could be retained:

const crypto = require('crypto');

// Read and parse key once
const privateKey = crypto.readPrivateKey(getPrivateKeySomehow());
// privateKey would be a ref to the PEM_read_bio_PrivateKey() pointer

for(let i = 0; i < 10000; i++) {
  const sign = crypto.createSign('RSA-SHA256');
  sign.write('some data to sign' + i);
  sign.end();
  console.log(sign.sign(privateKey, 'hex'));
}

I don't like passing an opaque object to the sign method, and introducing a "parsed key" class would increase complexity. I think it would be a cleaner API to allow passing the PEM key to createSign and allow reusing the resulting Sign object.

@seishun

@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.

@tniessen

@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

@tlbdk

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.

@tlbdk Will that require support for custom key objects (user-defined classes)? That would make a secure implementation more difficult.

@tniessen

@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:

fx.

const privateKey = crypto.readEnginePrivateKey("keyid"); // This would just be a wrapper around the openssl key ref

The 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.

@tlbdk

@tlbdk Awesome, thanks for the clarification.

@tlbdk

@tlbdk I proposed an API in #24234, for now, it does not support engine key objects, but we can definitely add support later once the API has been established.

@tniessen

@tniessen Looks really good, thanks for working on this :)

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#DESCRIPTION

I 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.js

@panva

@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.

@sam-github

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 PublicKeyObject instance out of PrivateKeyObject one

This is on my list of upcoming features, I am trying to split them into small incremental changes to make our internal processes easier. Eventually, createPublicKey will accept a key object with type private.

@tniessen

I think if we attach the key properties to a .fields sub-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 .info instead 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.

@panva

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 format

Would remove the need to even access a .fields sub-object.

Having the key properties in a .fields sub-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

.info gets 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 pkcs1 or spki. The different encodings result in different ASN.1 sequences.

@tlbdk

I have to say I agree with @tlbdk . At the keyObject level I shouldn't care whether the key was encoded with pkcs1 or spki. The different encodings result in different ASN.1 sequences.

Agreed, generic field names make more sense.

@sam-github

@sam-github @panva @tlbdk Would you expect BIGNUM fields to be exposed as hexadecimal strings or as JS bigints?

@tniessen

@tniessen I would say Bigint for output and Bigint and Buffer for input

@tlbdk

@tlbdk @tniessen

I would say Bigint for output and Bigint and Buffer for input

Agreed

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

@sam-github if the properties follow jwk names then the same format would indeed make more sense.

@tniessen

@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

@tniessen what issue with bigint support? AFAIK all key material is encoded as ~~base64url with no padding~~ Base64urlUInt

@panva

@panva I mean that Node.js should optimally output bigints, but JWK does not seem to support that (since JSON does not).

Navigation Menu

Repository navigation

Support parsing pem key only once for crypto api #15113

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Support parsing pem key only once for crypto api #15113

Description

Activity

seishun commented on Sep 1, 2017

tlbdk commented on Sep 1, 2017

tniessen commented on Sep 17, 2018

tlbdk commented on Sep 17, 2018

tniessen commented on Sep 18, 2018

tlbdk commented on Sep 18, 2018

tniessen commented on Sep 18, 2018

tniessen commented on Nov 7, 2018

tlbdk commented on Nov 11, 2018

tniessen commented on Dec 25, 2018

tlbdk commented on Jan 1, 2019

6 remaining items

tlbdk commented on Jan 2, 2019

tniessen commented on Jan 2, 2019

sam-github commented on Jan 4, 2019

davidgtonge commented on Jan 4, 2019

panva commented on Jan 4, 2019

tniessen commented on Jan 4, 2019

tlbdk commented on Jan 4, 2019

panva commented on Jan 4, 2019

sam-github commented on Jan 4, 2019

panva commented on Jan 4, 2019

sam-github commented on Jan 4, 2019

panva commented on Jan 4, 2019

tniessen commented on Jan 4, 2019

panva commented on Jan 4, 2019

tniessen commented on Jan 4, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions