crypto: support multiple ECDH curves and auto by rogaps · Pull Request #15206 · nodejs/node · GitHub

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

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.

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

Sure. I will update it.

/cc @nodejs/crypto

PTAL @nodejs/crypto

The copyright header block should not be added to new files

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

The 127.0.0.1 host can be omitted

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

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

Getting closer! Left a few comments.

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

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

Likewise.

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.

Why was this removed?

fwiw it was moved up

Thanks for the reviews. I will address them.

LGTM if @bnoordhuis is happy also.

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

Thanks, LGTM.

JS and doc LGTM

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.

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"?

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.

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

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

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

@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 <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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 <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

* **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 <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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

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

* **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 <info@bnoordhuis.nl> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@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

crypto: support multiple ECDH curves and auto by rogaps · Pull Request #15206 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: support multiple ECDH curves and auto - #15206

crypto: support multiple ECDH curves and auto#15206rogaps wants to merge 1 commit intonodejs:masternodejs/node:masterfrom rogaps:multiple-curves-supportrogaps/node:multiple-curves-supportCopy head branch name to clipboard

Conversation

rogaps commented Sep 5, 2017

Checklist

Affected core subsystem(s)

Uh oh!

mscdex Sep 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!

crypto: support multiple ECDH curves and auto#15206
rogaps wants to merge 1 commit into
nodejs:masterfrom
rogaps:multiple-curves-support

mscdex Sep 5, 2017 •
edited
Loading

jasnell commented Sep 15, 2017 •
edited
Loading

mhdawson commented Sep 18, 2017 •
edited
Loading