Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

OYTIS
Copy link
Contributor

@OYTIS OYTIS commented Aug 5, 2019

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})

@nodejs-github-bot 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 OYTIS force-pushed the hsm_keys branch 2 times, most recently from b020c1b to 33cf16f Compare August 5, 2019 12:35
@jasnell
Copy link
Member

jasnell commented Aug 7, 2019

Needs documentation and a unit test

@jasnell 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
@Trott
Copy link
Member

Trott commented Aug 8, 2019

@nodejs/crypto

lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
lib/_tls_common.js Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@@ -153,6 +153,38 @@ exports.createSecureContext = function createSecureContext(options) {
}
}

const engineKey = options.engineKey;
const privateKeyEngine = options.privateKeyEngine;
if (engineKey && privateKeyEngine) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@OYTIS OYTIS Aug 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

tniessen commented Aug 8, 2019

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
Copy link
Contributor Author

OYTIS commented Aug 10, 2019

@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
Copy link
Contributor Author

OYTIS commented Aug 11, 2019

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

doc/api/tls.md Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

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

doc/api/tls.md Outdated
* `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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Trott commented Aug 25, 2019

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

@rvagg
Copy link
Member

rvagg commented Aug 25, 2019

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?

@mhdawson
Copy link
Member

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

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

OYTIS commented Aug 29, 2019

@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
Copy link
Member

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
Copy link
Member

rvagg commented Aug 30, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

OYTIS commented Sep 4, 2019

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>
Signed-off-by: Anton Gerasimov <agerasimov@twilio.com>
@OYTIS
Copy link
Contributor Author

OYTIS commented Sep 26, 2019

Rebased, probably Jenkins tests also need to be run.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 27, 2019

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

@OYTIS
Copy link
Contributor Author

OYTIS commented Sep 27, 2019

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

@sam-github
Copy link
Contributor

Yes, landable now that CI is green.

Trott pushed a commit that referenced this pull request Sep 27, 2019
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
Copy link
Member

Trott commented Sep 27, 2019

Landed in c2ce8d0

@Trott Trott closed this Sep 27, 2019
@OYTIS OYTIS mentioned this pull request Sep 28, 2019
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
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>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
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
BridgeAR added a commit that referenced this pull request Oct 11, 2019
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
BridgeAR added a commit that referenced this pull request Oct 11, 2019
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
Copy link

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?

mildsunrise added a commit to mildsunrise/node that referenced this pull request Dec 7, 2020
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
mildsunrise added a commit that referenced this pull request Dec 12, 2020
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>
targos pushed a commit that referenced this pull request Dec 21, 2020
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>
targos pushed a commit that referenced this pull request May 1, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.