tls: set ecdhCurve default to 'auto' #16853

Hativ · 2017-11-07T00:01:38Z

For best out-of-the-box compatibility there should not be one default ecdhCurve for the tls client, OpenSSL should choose them automatically.

I've had a lot of struggle connecting to a server that did not support the default curve. Many third party modules have no support for setting ecdhCurve, therefore I think the tls client should support as much curves as possible by default. Using 'auto' would achieve this.

Refs: #16196
Refs: #1495
Refs: https://wiki.openssl.org/index.php/Manual:SSL_CTX_set1_curves(3)
Refs: #15206

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)

tls

mscdex · 2017-11-07T01:17:22Z

/cc @nodejs/crypto

bnoordhuis

Removing tls.DEFAULT_ECDH_CURVE outright is not an option: it's documented and in use in the ecosystem. (edit: Whether it actually worked as intended before v8.6.0 is something of an open question, though.)

Changing it to 'auto' however should be acceptable for node 10.

tniessen · 2017-11-07T11:52:45Z

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

Hativ · 2017-11-07T11:52:51Z

@bnoordhuis I've updated my commit.

indutny

LGTM

bnoordhuis

Thanks, LGTM.

Trott · 2017-11-07T16:51:27Z

CI failure on AIX is unrelated. CI is good.

sam-github · 2017-11-08T10:31:15Z

doc/api/tls.md

@@ -1154,7 +1154,7 @@ added: v0.11.13
 -->

 The default curve name to use for ECDH key agreement in a tls server. The
-default value is `'prime256v1'` (NIST P-256). Consult [RFC 4492] and
+default value is `'auto'`. Consult [RFC 4492] and


I searched RFC 4492 and FIPS 186-4 for the word auto, and it does not appear. Could you provide a little more text that says something about what this new value actually does? IIRC, this curve is used for creation of ephemeral ECDH keys, is it that auto will cause the curve used to be the same as the curve in the server's EC certificate?

tniessen

I would prefer to update the included references. auto is not part of the RFC AFAIK, but rather an OpenSSL definition. Otherwise LGTM.

Hativ · 2017-11-08T15:47:55Z

@sam-github @tniessen I've updated my commit.

tniessen · 2017-11-08T16:01:09Z

doc/api/tls.md

@@ -1154,8 +1154,7 @@ added: v0.11.13
 -->

 The default curve name to use for ECDH key agreement in a tls server. The
-default value is `'prime256v1'` (NIST P-256). Consult [RFC 4492] and
-[FIPS.186-4] for more details.
+default value is `'auto'`. OpenSSL will choose the curve automatically, see the [OpenSSL set curve documentation].


This line should not exceed 80 characters if possible. Also, the description of the ecdhCurve option in tls.createSecureContext already covers what auto means, I would prefer to link to that instead of the OpenSSL documentation. Maybe something like

default value is `'auto'`. See [`tls.createSecureContext()`] for further information.

I guess most users will assume this behavior, auto is a pretty self-explanatory name.

For best out-of-the-box compatibility there should not be one default `ecdhCurve` for the tls client, OpenSSL should choose them automatically. I've had a lot of struggle connecting to a server that did not support the default curve. Many third party modules have no support for setting `ecdhCurve`, therefore I think the tls client should support as much curves as possible by default. Using `'auto'` would achieve this. Refs: #16196 Refs: #1495 Refs: https://wiki.openssl.org/index.php/Manual:SSL_CTX_set1_curves(3) Refs: #15206

Hativ · 2017-11-08T16:06:46Z

@tniessen I've updated my commit again, using your suggestion.

tniessen · 2017-11-16T11:32:18Z

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

mhdawson · 2017-11-28T22:29:09Z

CI is not longer visible so starting a new one: https://ci.nodejs.org/job/node-test-pull-request/11785/

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

tniessen · 2017-11-28T23:45:41Z

CI failures are unrelated (centos6-64 is failing on all PRs and AIX is async-hooks/test-graph.signal once again).

Landed in af78840, thank you! 🎉 We are looking forward to future contributions!

ghost · 2017-11-29T02:55:11Z

@tniessen What release version(s) will that commit be part of?

tniessen · 2017-11-29T09:04:08Z

@jacobcabantomski-ct This PR was marked as semver-major, so these changes will not be shipped until node 10 is released.

sam-github · 2017-11-29T17:25:53Z

@jacobcabantomski-ct and @nodejs/crypto and @nodejs/lts - would there be interest in my PRing a --tls-default-ecdh-curve= CLI option? That would be semver-minor, so it could be easily backported, and would allow using NODE_OPTIONS=--tls-default-ecdh-curve=auto across all the release lines to set the curve to "auto".

ghost · 2017-11-29T22:04:42Z

@sam-github That would be very helpful in the short/medium term until 10 is out next fall.

rodrigok · 2018-01-05T16:00:31Z

NODE_OPTIONS=--tls-default-ecdh-curve=auto seems to be a good option meanwhile

mhdawson · 2018-01-29T17:55:16Z

@sam-github any chance you are going to have time to look at implementing your suggestion in the near future ?

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Nov 7, 2017

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 7, 2017

bnoordhuis requested changes Nov 7, 2017

View reviewed changes

indutny approved these changes Nov 7, 2017

View reviewed changes

bnoordhuis approved these changes Nov 7, 2017

View reviewed changes

Trott approved these changes Nov 7, 2017

View reviewed changes

jasnell approved these changes Nov 8, 2017

View reviewed changes

sam-github suggested changes Nov 8, 2017

View reviewed changes

tniessen approved these changes Nov 8, 2017

View reviewed changes

lpinca approved these changes Nov 8, 2017

View reviewed changes

tniessen reviewed Nov 8, 2017

View reviewed changes

ghost mentioned this pull request Nov 13, 2017

SSL Handshake Error using node >= 8.6.0 #16196

Closed

sam-github approved these changes Nov 15, 2017

View reviewed changes

tniessen closed this Nov 28, 2017

rodrigok mentioned this pull request Jan 5, 2018

[0.60.x] Secure LDAP connection issues - Error: socket hang up RocketChat/Rocket.Chat#9316

Closed

gibfahn mentioned this pull request Jan 19, 2018

crypto: support multiple ECDH curves and auto #15206

Closed

4 tasks

xPaw mentioned this pull request Mar 6, 2018

Fix default ecdh curve for better compatibility thelounge/thelounge#2159

Merged

addaleax mentioned this pull request Mar 14, 2018

Write EPROTO error on Node 8 and above #19359

Closed

sampaiodiego mentioned this pull request Sep 12, 2019

Support multiple ECDH curves on older versions of Node RocketChat/Rocket.Chat#15245

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: set ecdhCurve default to 'auto' #16853

tls: set ecdhCurve default to 'auto' #16853

Hativ commented Nov 7, 2017

mscdex commented Nov 7, 2017

bnoordhuis left a comment •

edited

Loading

tniessen commented Nov 7, 2017

Hativ commented Nov 7, 2017 •

edited

Loading

indutny left a comment

bnoordhuis left a comment

Trott commented Nov 7, 2017

sam-github Nov 8, 2017

tniessen left a comment

Hativ commented Nov 8, 2017

tniessen Nov 8, 2017

Hativ commented Nov 8, 2017

tniessen commented Nov 16, 2017

mhdawson commented Nov 28, 2017

tniessen commented Nov 28, 2017

ghost commented Nov 29, 2017

tniessen commented Nov 29, 2017 •

edited

Loading

sam-github commented Nov 29, 2017 •

edited

Loading

ghost commented Nov 29, 2017

rodrigok commented Jan 5, 2018

mhdawson commented Jan 29, 2018

tls: set ecdhCurve default to 'auto' #16853

tls: set ecdhCurve default to 'auto' #16853

Conversation

Hativ commented Nov 7, 2017

Checklist

Affected core subsystem(s)

mscdex commented Nov 7, 2017

bnoordhuis left a comment • edited Loading

Choose a reason for hiding this comment

tniessen commented Nov 7, 2017

Hativ commented Nov 7, 2017 • edited Loading

indutny left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Trott commented Nov 7, 2017

sam-github Nov 8, 2017

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

Hativ commented Nov 8, 2017

tniessen Nov 8, 2017

Choose a reason for hiding this comment

Hativ commented Nov 8, 2017

tniessen commented Nov 16, 2017

mhdawson commented Nov 28, 2017

tniessen commented Nov 28, 2017

ghost commented Nov 29, 2017

tniessen commented Nov 29, 2017 • edited Loading

sam-github commented Nov 29, 2017 • edited Loading

ghost commented Nov 29, 2017

rodrigok commented Jan 5, 2018

mhdawson commented Jan 29, 2018

bnoordhuis left a comment •

edited

Loading

Hativ commented Nov 7, 2017 •

edited

Loading

tniessen commented Nov 29, 2017 •

edited

Loading

sam-github commented Nov 29, 2017 •

edited

Loading