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

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

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

The copyright header block should not be added to new files

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

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

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

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

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

The 127.0.0.1 host can be omitted

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

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

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

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

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

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

src/node_crypto.cc

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

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

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 9636b1cCompare 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

test/parallel/test-tls-ecdh-auto.js Outdated
Copy link
Member
jasnellSep 14, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright header block should not be added to new files

jasnell reviewed Sep 14, 2017
View reviewed changes

test/parallel/test-tls-ecdh-auto.js Outdated
Copy link
Member
jasnellSep 14, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make use of the new require('common/fixtures') utility here.

jasnell reviewed Sep 14, 2017
View reviewed changes

test/parallel/test-tls-ecdh-auto.js Outdated
Copy link
Member
jasnellSep 14, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 127.0.0.1 host can be omitted

jasnell reviewed Sep 14, 2017
View reviewed changes

test/parallel/test-tls-ecdh-auto.js Outdated
Copy link
Member
jasnellSep 14, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a fan of multiline template literals and we've tried to avoid their use in the past.

jasnell reviewed Sep 14, 2017
View reviewed changes

test/parallel/test-tls-ecdh-multiple.js Outdated
Copy link
Member
jasnellSep 14, 2017
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here... the copyright header should not be included.

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 a266edcCompare 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 affa214Compare 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

Uh oh!

crypto: support multiple ECDH curves and auto#15206

crypto: support multiple ECDH curves and auto #15206

Uh oh!

Conversation

rogaps commented Sep 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdexSep 5, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 5, 2017

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogaps commented Sep 15, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Sep 15, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 15, 2017

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

jasnell commented Sep 15, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rogaps commented Sep 15, 2017

Uh oh!

mhdawson commented Sep 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rogaps commented Sep 18, 2017

mscdexSep 5, 2017•
edited
Loading

jasnell commented Sep 15, 2017•
edited
Loading

mhdawson commented Sep 18, 2017•
edited
Loading