Generalized provisioning of crypto keys from OpenSSL engines and/or providers

[nwf-msr]

What is the problem this feature will solve?

The fix for #15113, #24234, added support for crypto key objects, but the issue notes that "Loading keys from engines is not supported yet.". Later, c2ce8d0 (see #28973) added support for loading engine keys specifically for TLS, which means that the requisite calls to OpenSSL had to be added but only for SecureContext objects. In the interim, this code has migrated over to crypto_context.cc, but it is still exclusively associated with SecureContext objects, meaning, I think, that it is still not possible to use the lower-level cryptographic primitives of the crypto API with engine keys.

This impacts node.js downstream libraries, by requiring that applications handle exposed private key material to, for example, construct JWTs. See Azure/azure-sdk-for-js#22011 and octokit/auth-app.js#336 .

What is the feature you are proposing to solve the problem?

I would like the API of #24234 to be extended to support loading key objects from engines. This should be, I think, a fairly straightforward refactoring of existing code.

What alternatives have you considered?

There appear to be no general-purpose, viable alternatives that do not require exposing keying material to the running node.js application. It could be possible to specifically modify https://github.com/auth0/node-jwa to reach around node.js to, say, contact an openssh agent for PKCS#11 services, but this is much less preferable to just allowing the built-in crypto layer, which jwa already uses, able to load engine keys.

[bnoordhuis]

Can you post an example of what you think the API should look like?

[nwf-msr]

Riffing off the existing APIs, specifically...

• crypto.createPrivateKey,
• tls.createSecureContext, and
• OpenSSL req's -keyform parameter and its implication for -inkey,

I think I want to extend crypto.createPrivateKey to accept an object key with

• its .format field being the string "engine",
• an optional .engine string field behaving like tls.createSecureContext's privateKeyEngine
• its .key then being a string identifier suitable for the engine's use (that is, acting like tls.createSecureContext's privateKeyIdentifier).

So, for example, using the OpenSC PKCS#11 engine, I could write something like

p11key = crypto.createPrivateKey(
  { format: "engine"
  , engine: "pkcs11"
  , key: "pkcs11:token=sometokenname;object=somekeyname;type=private"
  })

(For this engine, the suitable key identifiers are PKCS#11 URIs or its custom naming scheme.)

Using https://github.com/auth0/node-jwa as an example consumer of such an API, we see that it simply passes its privateKey arguments through to crypto.createSign().sign(), which is documented to behave as though non-KeyObject privateKey values are passed through crypto.createPrivateKey first.

This kind of pass-through extends quite high in application stacks and would address at least one of my two examples (octokit/auth-app.js#336) quite nicely: because private keys are simply passed down

• to jwa
• through jws and
• through jsonwebtoken and
• through universal-github-app-jwt and
• through octokit/auth-app
• from octokit/app,

the proposed API change would mean that changing only the very top of an OctoKit.js GitHub App (or even its json configuration file), from specifying a string privateKey to instead specifying a suitable object, would likely be enough to use keys in a PKCS#11 HSM.

(The other example, Azure/azure-sdk-for-js#22011, starts off well from jsonwebtoken and ascending through msal, but it runs into ad-hoc validation logic in https://github.com/Azure/azure-sdk-for-js/blob/191e4ce330acd60e014c13412204b9598870cfa1/sdk/identity/identity/src/msal/nodeFlows/msalClientCertificate.ts#L83-L90, and so the Azure SDK will probably need a new, fairly simple, kind of nodeFlow for this engine-based workflow.)

In any case, all of that argues, I think, for the suitability of this kind of extension to createPrivateKey and, similarly, createPublicKey. I'm not going to insist that create*Key pronounce the identifier as key as OpenSSL does, and the use of keyIdentifier a la createSecureContext also seems fine; the real key piece is format: "engine" and its semantics.

[bnoordhuis]

Seems reasonable. For feature parity there should probably be a similar "bridge" in crypto.createPublicKey() to ENGINE_load_public_key().

cc @nodejs/crypto - needs your input.

[bnoordhuis]

I'll take the lack of comments as quiet approval. :-)

@nwf-msr Want to open a pull request?

[github-actions]

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

[nwf-msr]

Please don't close this as "stale". It's still an outstanding issue.

[bnoordhuis]

@nwf-msr if no one is going to work on it, then the right course of action is to eventually close it. Keeping all feature requests open indefinitely isn't really workable for a project the size of node.js, you end up with 10,000+ open issues.

[GauriSpears]

It looks like awesome work has been done but Node.JS doesn't have common view of private keys yet.

1. tls.createSecureContext doesn't support key object as a private key, only strings and buffers.
2. Dynamically linked OpenSSL: crypto.setEngine adds ciphers from engine to tls.getCiphers(), but not to crypto.getCiphers() and crypto.getHashes(). Despite that signing by crypto.createSign works.
3. Statically linked OpenSSL: crypto.setEngine doesn't add ciphers anywhere (I disabled caching by cachedResult(), but it didn't help). Despite that signing by crypto.createSign works... even without crypto.setEngine O_O

[GauriSpears]

I've digged into the second issue. Node.js in crypto.getCiphers and crypto.getHashes filters ciphers and hashes that are not fetchable to prevent some internal OpenSSL algorithms from appearing in the list. But some useful ciphers are not fetchable as well. I'll try to understand how OpenSSL does the same filtration.

[GauriSpears]

Ok, I understood the reason of the 3rd issue mentioned by me earlier.
The problem: Node.JS with statically compiled OpenSSL does not add gost-engine ciphers to tls.getCiphers() result. Even crypto.setEngine doesn't help.
Short story: OPENSSL_NO_GOST is defined in deps/openssl/config/*asm.h files.
Long story. OpenSSL doesn't limit the list of encoding ciphers and hashes. That's why when an additional engine is loaded by Node.JS you can use it for signing and hashing. But TLS ciphers list is hard-coded in OpenSSL. Usually (when I compile OpenSSL library separately with default settings): OpenSSL takes comprehensive list of all known TLS ciphers list (contains gost ciphers as well) and checks which of them are supported. For example, if gost-engine is not loaded, gost ciphers are filtered away. Meanwhile static OpenSSL in deps/openssl defines OPENSSL_NO_GOST so deps/openssl/openssl/ssl/ssl_lib.c, s3_lib.c, t1_lib.c and so on don't add gost ciphers to that initial comprehensive list. That's why loading engine in this case doesn't help.
Can we remove all "define OPENSSL_NO_GOST"? I tested and it helps with the issue.

[bnoordhuis]

Can we remove all "define OPENSSL_NO_GOST"?

I'm leaning towards 'no'. It's irrelevant for normal users (and won't work until you install the third-party GOST engine) but it increases the attack surface in openssl's TLS parser.

For background, GOST was removed from openssl in 2016 (and moved to gost-engine) because it had fallen in disrepair. That's not very encouraging.

[GauriSpears]

1). Gost TLS in gost-engine doesn't work with OPENSSL_NO_GOST. Users have to use stunnel as a TLS proxy.
2). If gost-engine is not installed nothing changes and no attack appears. I don't ask to build gost-engine with Node.JS and statically link it by default. I ask to allow statically linked OpenSSL work with the gost-engine if it is installed separately.

[bnoordhuis]

I'm aware, but I still lean towards 'no'.

If gost-engine is not installed nothing changes

That's not quite true and it's the reason I'm -1. A non-trivial amount of code in openssl's TLS stack is involved. Grep deps/openssl/openssl/ssl for the define and you'll see.

[GauriSpears]

Please let me explain. Indeed, without OPENSSL_NO_GOST cipher descriptions are added to deps/openssl/openssl/ssl/*.c and src/crypto/crypto_tls.cc and even a couple of functions for gost TLS processing are added to deps/openssl/openssl/ssl/statem/statem*, but without gost-engine built and installed all this code remains unused.
For example, let's see how node tls.getCiphers() works. The function leads to CipherBase::GetSSLCiphers in src/crypto/crypto_cipher.cc where new SSL context is created by openssl's SSL_CTX_new(TLS_method()). Let's dive into openssl source. ssl/ssl_lib.c: SSL_CTX_new calls SSL_CTX_new_ex and there:
1). ssl_ciph.c: ssl_load_ciphers -> get_optional_pkey_id -> EVP_PKEY_asn1_find_str is used to find which engine supports each authentication cipher. If gost-engine is not loaded, EVP_PKEY_asn1_find_str returns NULL for corresponding cippher and ctx->disabled_auth_mask is set.
2). ssl_ciph.c: ssl_create_cipher_list is used to filter ciphers with ctx->disabled_auth_mask and three other masks so SSL context will not contain gost ciphers if gost-engine is not available.
Let's return to CipherBase::GetSSLCiphers. SSL context is created and then SSL_get_ciphers is used to extract TLS cipher list created before (as we see, it contains only checked supported ciphers). So tls.getCiphers() will never include gost ciphers if gost-engine is not installed on a certain computer. As a result, they will not be used for connection negotiation and so on. No impact on 'normal' users.
Moreover, all this 'non-trivial amount of code' is supported by openssl developers (it is included in 'normal' builds of openssl by default) and I don't see a reason why I should trust to all other library source but should't trust to this piece.

[bnoordhuis]

The cost/benefit calculation here is "what are the risks?" vs. "what does the average user gain?"

The answer to the first question is "non-zero" whereas the answer to the second one is "nothing."

I get that some people have a legitimate interest in GOST but they can build from source.

[GauriSpears]

Could you tell me what risks do you mean? If there are any, I'd like to fix them.

[github-actions]

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

[github-actions]

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.