crypto: support multiple ECDH curves and auto #15206

rogaps · 2017-09-05T17:38:18Z

Using SSL_CTX_set1_curves_list() (OpenSSL 1.0.2+), this allows to set
colon separated ECDH curve names in SecureContext's ecdhCurve option.
The option can also be set to "auto" to select the curve automatically
from list built in OpenSSL by enabling SSL_CTX_set_ecdh_auto()
(OpenSSL 1.0.2+).
Refs: #15054

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto

mscdex · 2017-09-05T19:29:06Z

src/node_crypto.cc

+  if (!SSL_CTX_set1_curves_list(sc->ctx_, *curve))
+    return env->ThrowError("Failed to set ECDH curve");
+#else
+  int nid = OBJ_sn2nid(strcmp(*curve, "auto") == 0 ? "prime256v1" : *curve);


I'm not sure we should be implicitly selecting our own hardcoded curve in this case for "auto." If the intent is to use the value of tls.DEFAULT_ECDH_CURVE, we would probably need to dynamically read that value since it could be changed by end users. However, we should probably throw an error instead when we see "auto" and the appropriate OpenSSL API is not available.

My intention was to use tls.DEFAULT_ECDH_CURVE's value but displaying error is perhaps better if auto is actually not supported the version of OpenSSL.

mscdex · 2017-09-05T19:29:41Z

src/node_crypto.cc

-  int nid = OBJ_sn2nid(*curve);
+  SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE);
+
+#if (defined SSL_CTX_set1_curves_list || defined SSL_CTRL_SET_CURVES_LIST)


Minor nit: we use defined() below but not here. I think we should be consistent.

Sure. I will update it.

mscdex · 2017-09-05T19:35:06Z

/cc @nodejs/crypto

BridgeAR · 2017-09-13T18:21:38Z

PTAL @nodejs/crypto

jasnell · 2017-09-14T23:01:47Z

test/parallel/test-tls-ecdh-auto.js

+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.


The copyright header block should not be added to new files

jasnell · 2017-09-14T23:02:23Z

test/parallel/test-tls-ecdh-auto.js

+
+const options = {
+  key: fs.readFileSync(`${common.fixturesDir}/keys/agent2-key.pem`),
+  cert: fs.readFileSync(`${common.fixturesDir}/keys/agent2-cert.pem`),


Please make use of the new require('common/fixtures') utility here.

jasnell · 2017-09-14T23:02:45Z

test/parallel/test-tls-ecdh-auto.js

+  conn.end(reply);
+}));
+
+server.listen(0, '127.0.0.1', common.mustCall(function() {


The 127.0.0.1 host can be omitted

jasnell · 2017-09-14T23:03:22Z

test/parallel/test-tls-ecdh-auto.js

+
+server.listen(0, '127.0.0.1', common.mustCall(function() {
+  let cmd = `"${common.opensslCli}" s_client -cipher ${
+    options.ciphers} -connect 127.0.0.1:${this.address().port}`;


Not really a fan of multiline template literals and we've tried to avoid their use in the past.

jasnell · 2017-09-14T23:03:56Z

test/parallel/test-tls-ecdh-multiple.js

+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.


Ditto here... the copyright header should not be included.

jasnell

Getting closer! Left a few comments.

tniessen · 2017-09-15T08:30:05Z

Mostly LGTM, will do a review when @jasnell's comments were addressed.

bnoordhuis · 2017-09-15T08:20:01Z

src/node_crypto.cc

@@ -923,6 +923,22 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {

  node::Utf8Value curve(env->isolate(), args[0]);

+  SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE);
+
+#if defined(SSL_CTX_set1_curves_list) || defined(SSL_CTRL_SET_CURVES_LIST)


I don't think you have to do this #define dance; the versions of openssl we support, support this API.

bnoordhuis · 2017-09-15T08:20:25Z

src/node_crypto.cc

+  SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE);
+
+#if defined(SSL_CTX_set1_curves_list) || defined(SSL_CTRL_SET_CURVES_LIST)
+# if defined(SSL_CTX_set_ecdh_auto)


bnoordhuis · 2017-09-15T08:31:30Z

src/node_crypto.cc

+  if (strcmp(*curve, "auto") == 0)
+    return;
+
+  if (!SSL_CTX_set1_curves_list(sc->ctx_, *curve))


One (possibly academic) drawback to SSL_CTX_set1_curves_list() is that it won't let you set more than ~30 curves.

I.e., options = { ecdhCurve: crypto.getCurves().join(':') } won't work because there are over 80 different curves.

Supported curves are listed here. Does the documentation need to mention the lists?

Yes, I think that's a good idea. I was initially going to suggest to just refer readers to the relevant RFCs but on second thought, that would work okay for Brainpool (RFC 7027) but RFC 4492 is too much of a grab bag of different algorithms.

bnoordhuis · 2017-09-15T08:32:51Z

src/node_crypto.cc

@@ -933,10 +949,10 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {
  if (ecdh == nullptr)
    return env->ThrowTypeError("First argument should be a valid curve name");

-  SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE);


Why was this removed?

fwiw it was moved up

rogaps · 2017-09-15T13:40:02Z

Thanks for the reviews. I will address them.

jasnell

LGTM if @bnoordhuis is happy also.

tniessen · 2017-09-15T17:24:21Z

LGTM if CI passes: https://ci.nodejs.org/job/node-test-pull-request/10098/

bnoordhuis

Thanks, LGTM.

BridgeAR

JS and doc LGTM

BridgeAR · 2017-09-15T20:45:53Z

test/parallel/test-tls-ecdh-auto.js

+  });
+
+  client.on('error', function(error) {
+    assert.ifError(error);


Nit: (non blocking) assert.ifError is actually meant as callback replacement. Therefore you do not have to wrap it in a function and can use it as client.on("error", assert.ifError) instead. The same in the other test.

BridgeAR · 2017-09-15T20:51:20Z

I am OK with this but in general - would it not be much nicer to use an Array instead of a colon separated string? And we could also allow a Boolean where false is used instead of the string "false" for deactivation and true for "auto"?

jasnell · 2017-09-15T22:25:16Z

would it not be much nicer to use an Array

Perhaps, but using the :-delimited string is consistent with openssl in general. Perhaps as a separate PR support for an Array input can be added.

jasnell · 2017-09-15T22:26:21Z

This is failing significantly on FIPS...

not ok 1445 parallel/test-tls-ecdh-multiple
  ---
  duration_ms: 0.412
  severity: fail
  stack: |-
    _tls_common.js:142
        c.context.setECDHCurve(options.ecdhCurve);
                  ^
    
    Error: Failed to set ECDH curve
        at Object.createSecureContext (_tls_common.js:142:15)
        at new Server (_tls_wrap.js:805:25)
        at Object.exports.createServer (_tls_wrap.js:898:10)
        at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/parallel/test-tls-ecdh-multiple.js:30:20)
        at Module._compile (module.js:600:30)
        at Object.Module._extensions..js (module.js:611:10)
        at Module.load (module.js:521:32)
        at tryModuleLoad (module.js:484:12)
        at Function.Module._load (module.js:476:3)
        at Function.Module.runMain (module.js:641:10)
  ...

ping @mhdawson @gibfahn

rogaps · 2017-09-15T22:46:41Z

@jasnell I overlooked FIPS mode doesn't support brainpoolP256r1.

mhdawson · 2017-09-18T13:33:51Z

@rogaps I assume you are just going to update the tests so that they don't try to use brainpoolP256r1 when in FIPs mode. Although maybe you have already updated.

rogaps · 2017-09-18T14:11:38Z

@mhdawson Sure. I will update the tests for some unsupported curves.

Using SSL_CTX_set1_curves_list() (OpenSSL 1.0.2+), this allows to set colon separated ECDH curve names in SecureContext's ecdhCurve option. The option can also be set to "auto" to select the curve automatically from list built in OpenSSL by enabling SSL_CTX_set_ecdh_auto() (OpenSSL 1.0.2+). PR-URL: #15206 Ref: #15054 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

jasnell · 2017-09-20T08:20:37Z

Landed in 873e5bd

Using SSL_CTX_set1_curves_list() (OpenSSL 1.0.2+), this allows to set colon separated ECDH curve names in SecureContext's ecdhCurve option. The option can also be set to "auto" to select the curve automatically from list built in OpenSSL by enabling SSL_CTX_set_ecdh_auto() (OpenSSL 1.0.2+). PR-URL: #15206 Ref: #15054 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

Using SSL_CTX_set1_curves_list() (OpenSSL 1.0.2+), this allows to set colon separated ECDH curve names in SecureContext's ecdhCurve option. The option can also be set to "auto" to select the curve automatically from list built in OpenSSL by enabling SSL_CTX_set_ecdh_auto() (OpenSSL 1.0.2+). PR-URL: nodejs/node#15206 Ref: nodejs/node#15054 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](#7855) * Custom lookup functions are now supported. [#14560](#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

@sebdeckers

* **crypto** * Support for multiple ECDH curves. [#15206](nodejs/node#15206) * **dgram** * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855) * Custom lookup functions are now supported. [#14560](nodejs/node#14560) * **n-api** * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902) * **tls** * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245) * **New Contributors** * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)

For best out-of-the-box compatibility there should not be one default `ecdhCurve` for the tls client, OpenSSL should choose them automatically. See https://wiki.openssl.org/index.php/Manual:SSL_CTX_set1_curves(3) PR-URL: #16853 Refs: #16196 Refs: #1495 Refs: #15206 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

gibfahn · 2018-01-19T11:47:41Z

@nodejs/lts

Post-mortem on this, it contained an accidental breaking change (which brought the code in line with the docs), changing the default curve setting from prime256v1 to auto. I think reverting at this point would do more harm than good, but it is causing problems for users. The change in default will be reverted in 10.x (as a semver-major).

One thing that would help would be @sam-github 's suggestion in #16853 (comment), which is adding aNODE_OPTIONS option that allows users to change the default back to auto without needing to change dependency code.

Quite a few people have hit this, so IMO we should prioritize getting this fixed and backported to 8.x and 6.x.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Sep 5, 2017

mscdex reviewed Sep 5, 2017

View reviewed changes

rogaps force-pushed the multiple-curves-support branch from 1e960c6 to 9636b1c Compare September 6, 2017 13:55

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 14, 2017

jasnell reviewed Sep 14, 2017

View reviewed changes

jasnell requested changes Sep 14, 2017

View reviewed changes

bnoordhuis requested changes Sep 15, 2017

View reviewed changes

rogaps force-pushed the multiple-curves-support branch from 9636b1c to a266edc Compare September 15, 2017 16:30

jasnell approved these changes Sep 15, 2017

View reviewed changes

bnoordhuis approved these changes Sep 15, 2017

View reviewed changes

BridgeAR approved these changes Sep 15, 2017

View reviewed changes

tniessen approved these changes Sep 16, 2017

View reviewed changes

rogaps force-pushed the multiple-curves-support branch from a266edc to affa214 Compare September 16, 2017 17:08

jasnell closed this Sep 20, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

bnoordhuis mentioned this pull request Sep 21, 2017

TLS module: Support multiple ecdhCurve's #15054

Closed

princjef mentioned this pull request Oct 14, 2017

SSL Handshake Error using node >= 8.6.0 #16196

Closed

Hativ mentioned this pull request Nov 7, 2017

tls: set ecdhCurve default to 'auto' #16853

Closed

4 tasks

rogaps deleted the multiple-curves-support branch December 23, 2017 08:46

MylesBorins mentioned this pull request Jan 15, 2018

Semver Minor Tracking nodejs/Release#298

Closed

mscdex mentioned this pull request Mar 14, 2018

Write EPROTO error on Node 8 and above #19359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: support multiple ECDH curves and auto #15206

crypto: support multiple ECDH curves and auto #15206

rogaps commented Sep 5, 2017

mscdex Sep 5, 2017 •

edited

Loading

rogaps Sep 6, 2017

mscdex Sep 5, 2017

rogaps Sep 6, 2017

mscdex commented Sep 5, 2017

BridgeAR commented Sep 13, 2017

jasnell Sep 14, 2017

jasnell Sep 14, 2017

jasnell Sep 14, 2017

jasnell Sep 14, 2017

jasnell Sep 14, 2017

jasnell left a comment

tniessen commented Sep 15, 2017

bnoordhuis Sep 15, 2017

bnoordhuis Sep 15, 2017

bnoordhuis Sep 15, 2017

rogaps Sep 15, 2017

bnoordhuis Sep 15, 2017

bnoordhuis Sep 15, 2017

tniessen Sep 15, 2017

rogaps commented Sep 15, 2017

jasnell left a comment

tniessen commented Sep 15, 2017

bnoordhuis left a comment

BridgeAR left a comment

BridgeAR Sep 15, 2017

BridgeAR commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017 •

edited

Loading

rogaps commented Sep 15, 2017

mhdawson commented Sep 18, 2017 •

edited

Loading

rogaps commented Sep 18, 2017

jasnell commented Sep 20, 2017

gibfahn commented Jan 19, 2018

crypto: support multiple ECDH curves and auto #15206

crypto: support multiple ECDH curves and auto #15206

Conversation

rogaps commented Sep 5, 2017

Checklist

Affected core subsystem(s)

mscdex Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Sep 5, 2017

BridgeAR commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

tniessen commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogaps commented Sep 15, 2017

jasnell left a comment

Choose a reason for hiding this comment

tniessen commented Sep 15, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 15, 2017

jasnell commented Sep 15, 2017

jasnell commented Sep 15, 2017 • edited Loading

rogaps commented Sep 15, 2017

mhdawson commented Sep 18, 2017 • edited Loading

rogaps commented Sep 18, 2017

jasnell commented Sep 20, 2017

gibfahn commented Jan 19, 2018

mscdex Sep 5, 2017 •

edited

Loading

jasnell commented Sep 15, 2017 •

edited

Loading

mhdawson commented Sep 18, 2017 •

edited

Loading