RFC: support for HSM private keys in TLS handshake #28973

OYTIS · 2019-08-05T10:55:47Z

The use-case is briefly described here: #28921. I want to use a private key managed by an OpenSSL engine to establish a TLS connection.

Another option, sigalgs, is useful, among the other things, to adapt to the limitations of the hardware (e.g. if it doesn't support RSAPSS). I can make it a separate PR if it makes more sense.

Usage example:

tls = require('tls')
tls_ctx = tls.createSecureContext( {'privateKeyEngine':'mycoolengine',
                                    'engineKey':'myenginekey',
                                    'ca':ca_cert,
                                    'cert':client_cert,
                                    'sigalgs' : 'RSA+SHA384:RSA+SHA256'})
socket = tls.connect({'host':'host.domain.tld','port':8080,'secureContext':tls_ctx})

jasnell · 2019-08-07T01:09:34Z

Needs documentation and a unit test

Trott · 2019-08-08T02:44:23Z

@nodejs/crypto

lib/_tls_common.js

src/node_crypto.cc

tniessen · 2019-08-08T09:34:41Z

lib/_tls_common.js

@@ -153,6 +153,38 @@ exports.createSecureContext = function createSecureContext(options) {
    }
  }

+  const engineKey = options.engineKey;
+  const privateKeyEngine = options.privateKeyEngine;
+  if (engineKey && privateKeyEngine) {


Node.js has traditionally been lazy when it comes to input validation, but new APIs should be stricter: Use !== undefined or something similar to not ignore inputs such as engineKey: 0.

Also, I don't know if it would make sense to validate types even if only one option is given? Or to even throw if only one is given? Is there any use case for only specifying one of these options?

Is there any use case for only specifying one of these options?

@tniessen From the OpenSSL point of view, you can extract multiple keys from one engine, but since we only have one key in the SecureContext, they should probably go together.

I agree, but I would prefer to throw when only one of the options is given. (Currently, you are ignoring the options if only one is used.) If there is no legitimate use case for only specifying one of the two options, then it is most likely an error or a mistake, and we shouldn't silently ignore that.

I'm not sure this comment was addressed, though the code was outdated, since if only one of engine or keyId is provided, they are both ignored.

tniessen · 2019-08-08T09:39:23Z

I can't comment much on the matter, maybe @bnoordhuis or @sam-github can, I never used OpenSSL engines. These options would be for server keys then, not for client keys?

The other addition, sigalgs, is completely unrelated, isn't it? Consider splitting these changes into two PRs if you would like. Maybe we should also consider other names for this option.

OYTIS · 2019-08-10T18:34:30Z

@tniessen Thank you for your review, your comments are extremely valuable for a newcomer like myself. I am addressing all of them in the coming version.

These options would be for server keys then, not for client keys?

These options are for the communication party under control of Node.js. If it's a server (which is a more common scenario with Node.js, as far as I know), then it will be server keys, the example in my first comment is rather a client.

OYTIS · 2019-08-11T16:52:25Z

Fixed hopefully all the issues, added documentation and tests. sigalgs part was removed, another PR is coming

vsemozhetbyt · 2019-08-11T17:49:51Z

doc/api/tls.md

@@ -1425,6 +1425,11 @@ changes:
    passphrase: <string>]}`. The object form can only occur in an array.
    `object.passphrase` is optional. Encrypted keys will be decrypted with
    `object.passphrase` if provided, or `options.passphrase` if it is not.
+  * `privateKeyEngine` {string} Name of an OpenSSL engine to get private key
+    from. Should be use together with `engineKey`, otherwise ignored.


Nit:

Suggested change

from. Should be use together with `engineKey`, otherwise ignored.

from. Should be used together with `engineKey`, otherwise ignored.

vsemozhetbyt · 2019-08-11T17:50:58Z

doc/api/tls.md

+  * `privateKeyEngine` {string} Name of an OpenSSL engine to get private key
+    from. Should be use together with `engineKey`, otherwise ignored.
+  * `engineKey` {string} Identifier of a private key managed by OpenSSL engine.
+    Should be use together with `privateKeyEngine`, otherwise ignored. Overrides


Nit (it seems this needs rewrapping to conform with 80 chars limit):

Suggested change

Should be use together with `privateKeyEngine`, otherwise ignored. Overrides

Should be used together with `privateKeyEngine`, otherwise ignored. Overrides

Trott · 2019-08-25T23:36:55Z

I can't comment much on the matter, maybe @bnoordhuis or @sam-github can, I never used OpenSSL engines.

/ping @nodejs/crypto @rvagg Any opinions on this? If we're definitely not going to merge this, I'd like to close it. But if we might merge it, it would be great to nudge it forward a bit....

test/parallel/test-tls-keyengine-unsupported.js

rvagg · 2019-08-25T23:53:52Z

If we're definitely not going to merge this

I don't see a reason to reject this out of hand, seems like a reasonable ask if the code can get up to scratch. Solid tests are tricky to do for it though, without building and loading in a custom engine so I'm not sure how to view that problem—maybe in the same way that we view the problem of testing commits that deal with LibreSSL? Caveat emptor, it's expected the committer and users of the code in question do the testing IRL? Is that an OK standard for crypto?

lib/_tls_common.js

mhdawson · 2019-08-27T15:16:49Z

I'll add my 2 cents that it is the kind of thing we want one the PR is in good shape.

rvagg

I'm fine with this. BUT, it's important to note here that the tests are only asserting (a) the existence of the API, (b) the API argument validation and (c) what happens when used with an OpenSSL compiled with -no-engines which is not the normal mode we ship, so is perhaps more of an edge-case. The tests don't test anything about the keys coming in via the API. In coverage terms, this is adding a lot more uncovered code and not improving existing coverage. Such a test is possible (I think) but would involve creating a dummy engine and having it loaded during test time. The awkwardness of creating a test environment for this to happen might be significant, I'm not sure, but I'm fairly sure it's at least possible if someone felt the need to hold this up on needing proper tests.
Would like to hear additional thoughts. @sam-github @bnoordhuis @tniessen @addaleax?

OYTIS · 2019-08-29T11:05:27Z

@vsemozhetbyt @rvagg @mhdawson @jasnell Thank you for your reviews, everything should be fixed now (CI job should be restarted probably, it seems the failure is unrelated to the PR).

I agree that a dummy engine is the way to test this functionality meaningfully. If you decide it's the way to go, I can try to implement that.

mhdawson · 2019-08-29T21:19:54Z

I'd agree with @rvagg that it would be better to avoid adding code that we can't test. If you can try to create a test with a dummy engine that would be great.

rvagg · 2019-08-30T00:54:04Z

if you can give it a go that would be great @OYTIS, I don't imagine it'll be simple but if you come up with a mechanism to compile and load a custom engine during test time then we could go back and apply it to the custom engine use for certs.

sam-github · 2019-09-04T16:18:00Z

We kindof support engines, in that there is code and some docs, but we don't test the support, so its hard to know it works, what it works for, if the docs are correct, etc.

All of which is to say, if someone has the time to add a dummy engine that would be great. Or possibly, if there is a software only engine available on Linux, perhaps the tests could search for it, and use it if available? That would allow testing locally for developers, and allow us to add a container test that uses engines.

OYTIS · 2019-09-04T16:48:43Z

Sorry, I'm a bit short on time now, I'll try to finish that shortly.

From what I can see the test in test/addons/openssl-client-cert-engine is almost what we need here, just for cert engines.

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

OYTIS · 2019-09-26T11:52:50Z

Rebased, probably Jenkins tests also need to be run.

nodejs-github-bot · 2019-09-26T21:51:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25721/

Trott · 2019-09-27T05:03:43Z

Is this ready to land? @jasnell @rvagg @tniessen @sam-github @OYTIS

OYTIS · 2019-09-27T07:50:54Z

@Trott I've got no amendments from my side.

sam-github · 2019-09-27T14:44:35Z

Yes, landable now that CI is green.

Add `privateKeyIdentifier` and `privateKeyEngine` options to get private key from an OpenSSL engine in tls.createSecureContext(). PR-URL: #28973 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Trott · 2019-09-27T22:54:16Z

Landed in c2ce8d0

Add `privateKeyIdentifier` and `privateKeyEngine` options to get private key from an OpenSSL engine in tls.createSecureContext(). PR-URL: #28973 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Notable changes: * build: * Add `--force-context-aware` flag to prevent usage of native node addons that aren't context aware #29631 * deprecations: * Add documentation-only deprecation for `process._tickCallback()` #29781 * esm: * Using JSON modules is experimental again #29754 * fs: * Introduce `opendir()` and `fs.Dir` to iterate through directories #29349 * process: * Add source-map support to stack traces by using `--source-map-support` #29564 * tls: * Honor `pauseOnConnect` option #29635 * Add option for private keys for OpenSSL engines #28973 PR-URL: #29919

heol-files · 2020-08-10T08:21:03Z

Hi! I have a yubikey, and i guess this is the way to go for me when connecting with TLS?
Do you have any example on how to use/write a "privateKeyEngine"?
Or any documentation on how to do/use this?

We have a few places where we individually forward each parameter to tls.createSecureContext(). In nodejs#28973 and others, we added new SecureContext options but forgot to keep these places up to date. As per https.Agent#getName, I understand that at least `privateKeyIdentifier` and `privateKeyEngine` should be added too, since they're a substitute for `key`. I've also added sigalgs. Fixes: nodejs#36322 Refs: nodejs#28973

We have a few places where we individually forward each parameter to tls.createSecureContext(). In #28973 and others, we added new SecureContext options but forgot to keep these places up to date. As per https.Agent#getName, I understand that at least `privateKeyIdentifier` and `privateKeyEngine` should be added too, since they're a substitute for `key`. I've also added sigalgs. Fixes: #36322 Refs: #28973 PR-URL: #36416 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 5, 2019

OYTIS force-pushed the hsm_keys branch 2 times, most recently from b020c1b to 33cf16f Compare August 5, 2019 12:35

jasnell added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem. labels Aug 7, 2019

tniessen reviewed Aug 8, 2019

View reviewed changes

OYTIS force-pushed the hsm_keys branch from 33cf16f to 769a786 Compare August 11, 2019 16:47

vsemozhetbyt reviewed Aug 11, 2019

View reviewed changes

rvagg reviewed Aug 25, 2019

View reviewed changes

test/parallel/test-tls-keyengine-unsupported.js Show resolved Hide resolved

rvagg reviewed Aug 25, 2019

View reviewed changes

lib/_tls_common.js Show resolved Hide resolved

OYTIS force-pushed the hsm_keys branch from 769a786 to b1a9625 Compare August 27, 2019 20:41

rvagg approved these changes Aug 28, 2019

View reviewed changes

jasnell approved these changes Aug 28, 2019

View reviewed changes

OYTIS force-pushed the hsm_keys branch from b1a9625 to 9ea421c Compare September 12, 2019 11:57

OYTIS added 2 commits September 26, 2019 13:52

tls: test for 'privateKeyEngine' and 'privateKeyIdentifier'

3dae10a

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

tls: add cleanup for private key engine

ea21407

Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>

OYTIS force-pushed the hsm_keys branch from 88bd15a to ea21407 Compare September 26, 2019 11:52

Trott closed this Sep 27, 2019

OYTIS mentioned this pull request Sep 28, 2019

TLS: keys from engines? #28921

Closed

BridgeAR mentioned this pull request Oct 10, 2019

v12.12.0 proposal #29919

Merged

dnoliver mentioned this pull request Sep 29, 2020

Custom Application integration with TPM2 PKCS#11 tpm2-software/tpm2-pkcs11#605

Closed

mildsunrise mentioned this pull request Dec 6, 2020

tls_wrap does not forward privateKeyEngine and privateKeyIdentifier to tls.createSecureContext() on Server opening #36322

Closed

mildsunrise mentioned this pull request Dec 6, 2020

tls: forward new SecureContext options #36416

Closed

nwf-msr mentioned this pull request Jul 2, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: support for HSM private keys in TLS handshake #28973

RFC: support for HSM private keys in TLS handshake #28973

OYTIS commented Aug 5, 2019 •

edited

Loading

jasnell commented Aug 7, 2019

Trott commented Aug 8, 2019

tniessen Aug 8, 2019

tniessen Aug 8, 2019

OYTIS Aug 10, 2019 •

edited

Loading

tniessen Aug 11, 2019

sam-github Sep 20, 2019

tniessen commented Aug 8, 2019

OYTIS commented Aug 10, 2019 •

edited

Loading

OYTIS commented Aug 11, 2019

vsemozhetbyt Aug 11, 2019

vsemozhetbyt Aug 11, 2019 •

edited

Loading

Trott commented Aug 25, 2019

rvagg commented Aug 25, 2019

mhdawson commented Aug 27, 2019

rvagg left a comment

OYTIS commented Aug 29, 2019

mhdawson commented Aug 29, 2019

rvagg commented Aug 30, 2019

sam-github commented Sep 4, 2019

OYTIS commented Sep 4, 2019

OYTIS commented Sep 26, 2019

nodejs-github-bot commented Sep 26, 2019

Trott commented Sep 27, 2019

OYTIS commented Sep 27, 2019

sam-github commented Sep 27, 2019

Trott commented Sep 27, 2019

heol-files commented Aug 10, 2020

	from. Should be use together with `engineKey`, otherwise ignored.
	from. Should be used together with `engineKey`, otherwise ignored.

	Should be use together with `privateKeyEngine`, otherwise ignored. Overrides
	Should be used together with `privateKeyEngine`, otherwise ignored. Overrides

RFC: support for HSM private keys in TLS handshake #28973

RFC: support for HSM private keys in TLS handshake #28973

Conversation

OYTIS commented Aug 5, 2019 • edited Loading

jasnell commented Aug 7, 2019

Trott commented Aug 8, 2019

tniessen Aug 8, 2019

Choose a reason for hiding this comment

tniessen Aug 8, 2019

Choose a reason for hiding this comment

OYTIS Aug 10, 2019 • edited Loading

Choose a reason for hiding this comment

tniessen Aug 11, 2019

Choose a reason for hiding this comment

sam-github Sep 20, 2019

Choose a reason for hiding this comment

tniessen commented Aug 8, 2019

OYTIS commented Aug 10, 2019 • edited Loading

OYTIS commented Aug 11, 2019

vsemozhetbyt Aug 11, 2019

Choose a reason for hiding this comment

vsemozhetbyt Aug 11, 2019 • edited Loading

Choose a reason for hiding this comment

Trott commented Aug 25, 2019

rvagg commented Aug 25, 2019

mhdawson commented Aug 27, 2019

rvagg left a comment

Choose a reason for hiding this comment

OYTIS commented Aug 29, 2019

mhdawson commented Aug 29, 2019

rvagg commented Aug 30, 2019

sam-github commented Sep 4, 2019

OYTIS commented Sep 4, 2019

OYTIS commented Sep 26, 2019

nodejs-github-bot commented Sep 26, 2019

Trott commented Sep 27, 2019

OYTIS commented Sep 27, 2019

sam-github commented Sep 27, 2019

Trott commented Sep 27, 2019

heol-files commented Aug 10, 2020

OYTIS commented Aug 5, 2019 •

edited

Loading

OYTIS Aug 10, 2019 •

edited

Loading

OYTIS commented Aug 10, 2019 •

edited

Loading

vsemozhetbyt Aug 11, 2019 •

edited

Loading