crypto: expose ECDH class #8188

bengl · 2016-08-19T21:04:28Z

Checklist

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

Affected core subsystem(s)

crypto

Description of change

ECDH class was not previously exposed, which was inconsistent with the rest of the classes in that module.

Docs already document the class, and no tests current exist (that I could fine) to test the presence of any of the other classes, so didn't change tests. Shall I add another set of tests that checks the presence of both the classes and the create functions for them? Separate PR?

jbergstroem · 2016-08-19T21:07:43Z

Adding a test to this PR at least sounds good to me.

jasnell · 2016-08-19T21:23:38Z

@nodejs/crypto

Trott · 2016-08-19T21:29:20Z

Shall I add another set of tests that checks the presence of both the classes and the create functions for them?

Yes, please! No opinion on whether it belongs in this PR or a separate PR.

bengl · 2016-08-19T21:46:28Z

@Trott @jbergstroem Cool. Test added.

Trott · 2016-08-19T21:49:40Z

Test LGTM. As for the crypto changes, I prefer to leave even the simple stuff to the experts: @nodejs/crypto

bengl · 2016-08-19T21:52:35Z

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

jbergstroem · 2016-08-19T22:05:01Z

test/parallel/test-crypto-classes.js

+const common = require('../common');
+const assert = require('assert');
+
+if (!common.hasCrypto) {


Fips fail: # Error: crypto.createCipher() is not supported in FIPS mode.

You probably want to add a check against common.hasFipsCrypto

bengl · 2016-08-19T22:24:57Z

CI again, w/ FIPS mode checking: https://ci.nodejs.org/job/node-test-pull-request/3761/

bengl · 2016-08-19T22:59:41Z

Using a larger keysize for FIPS... CI: https://ci.nodejs.org/job/node-test-pull-request/3762/

bnoordhuis · 2016-08-20T08:36:59Z

test/parallel/test-crypto-classes.js

+Object.keys(TEST_CASES).forEach((Class) =>
+  assert(
+    crypto[`create${Class}`](...TEST_CASES[Class]) instanceof crypto[Class]
+  )


Missing semicolon and no blank line at end of file. Surprising the linter didn't complain.

Can you s/Class/clazz/?

The semicolon is unnecessary because there's no wrapping {..} around the function body:

someArray.forEach((clazz) => foo);

If it required a semicolon for function bodies whether or not they were wrapped in {...}, it would be this:

someArray.forEach((clazz) => foo;);

~~That seems odd (to me at least).~~ That will result in a SyntaxError.

If the function body is contained in {...}, then the linter will complain about a missing semicolon.

So this is OK:

Object.keys(TEST_CASES).forEach((Class) => // <- no wrapping { assert( crypto[`create${Class}`](...TEST_CASES[Class]) instanceof crypto[Class] ) );

But this would require a semicolon:

Object.keys(TEST_CASES).forEach((Class) => { // <- wrapping { assert( crypto[`create${Class}`](...TEST_CASES[Class]) instanceof crypto[Class] ) // <- linter now complains about missing semicolon here });

-1 on using multiline arrow function without brackets which makes lint rule more complex :-(

Ah, I overlooked that it's a single expression arrow function. I'd use braces here.

Cool. Will s/Class/clazz/ and add braces for clarity.

bnoordhuis · 2016-08-20T08:42:49Z

The test is timing out on the rpi2 and rpi3 buildbots but not, strangely, the rpi1 buildbots.

Aside, for bonus points make it test both function and constructor invocation.

jasnell · 2017-01-06T20:16:33Z

Ping @bengl ... next steps on this?

sam-github

Basically LGTM. One request: reorder the commits, so the first adds the instanceof test, and the second commit changes ECDH and adds one line to the tests so that ECDH becomes like all the others.

sam-github · 2017-01-17T21:31:00Z

@bengl If you don't have time, I can complete and land for you.

fhinkel · 2017-03-26T11:44:00Z

Ping @bengl @sam-github

bengl · 2017-06-02T07:01:17Z

Apologies folks! This one fell off my radar somehow.

@sam-github I've re-organized the commits as requested, and rebased it on top of master.

jasnell · 2017-08-24T23:28:53Z

ping @bengl and @sam-github ... what's the status on this?

BridgeAR · 2017-09-12T18:47:15Z

I removed the stalled as this seems to be fine in general. No conflicts and all comments were addressed, it does not some LGs though. PTAL @nodejs/crypto

Notable Changes: * crypto: - expose ECDH class nodejs/node#8188 * http2: - http2 is now exposed by defualt without the need for a flag nodejs/node#15685 - a new environment varible NODE\_NO\_HTTP2 has been added to allow userland http2 to be required nodejs/node#15685 - support has been added for generic `Duplex` streams nodejs/node#16269 * module: - resolve and instantiate loader pipeline hooks have been added to the ESM lifecycle nodejs/node#15445 * zlib: - CVE-2017-14919 - In zlib v1.2.9, a change was made that causes an error to be raised when a raw deflate stream is initialized with windowBits set to 8. On some versions this crashes Node and you cannot recover from it, while on some versions it throws an exception. Node.js will now gracefully set windowBits to 9 replicating the legacy behavior to avoid a DOS vector. nodejs-private/node-private#95 PR-URL: nodejs-private/node-private#98

gibfahn · 2018-01-15T22:24:47Z

Release team were +1 on backporting to v6.x.

The crypto classes are also exposed as createClass for each class. This tests that each of them returns an instance of the class in question. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

For consistency with the rest of the crypto classes, exposes the ECDH class. Originally, only the createECDH function was exposed, and there was no real reason to hide the class. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

The crypto classes are also exposed as createClass for each class. This tests that each of them returns an instance of the class in question. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

For consistency with the rest of the crypto classes, exposes the ECDH class. Originally, only the createECDH function was exposed, and there was no real reason to hide the class. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

The crypto classes are also exposed as createClass for each class. This tests that each of them returns an instance of the class in question. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

For consistency with the rest of the crypto classes, exposes the ECDH class. Originally, only the createECDH function was exposed, and there was no real reason to hide the class. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

The crypto classes are also exposed as createClass for each class. This tests that each of them returns an instance of the class in question. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

For consistency with the rest of the crypto classes, exposes the ECDH class. Originally, only the createECDH function was exposed, and there was no real reason to hide the class. Backport-PR-URL: #16245 PR-URL: #8188 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 19, 2016

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 19, 2016

jbergstroem reviewed Aug 19, 2016
View reviewed changes

bengl force-pushed the exposeecdh branch from 6e38a76 to 7560ebe Compare August 19, 2016 22:20

bengl force-pushed the exposeecdh branch from 7560ebe to bd82047 Compare August 19, 2016 22:58

bnoordhuis reviewed Aug 20, 2016
View reviewed changes

bengl force-pushed the exposeecdh branch from bd82047 to d52a4e5 Compare August 24, 2016 20:56

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017

sam-github previously requested changes Jan 17, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

bengl force-pushed the exposeecdh branch from d52a4e5 to 003c7fb Compare June 2, 2017 06:58

BridgeAR removed the stalled Issues and PRs that are stalled. label Sep 12, 2017

gibfahn added the lts-watch-v6.x label Jan 15, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

BethGriggs added backported-to-v8.x and removed backport-requested-v8.x labels Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: expose ECDH class #8188

crypto: expose ECDH class #8188

bengl commented Aug 19, 2016 •

edited

Loading

jbergstroem commented Aug 19, 2016

jasnell commented Aug 19, 2016

Trott commented Aug 19, 2016

bengl commented Aug 19, 2016

Trott commented Aug 19, 2016

bengl commented Aug 19, 2016

jbergstroem Aug 19, 2016

bengl commented Aug 19, 2016

bengl commented Aug 19, 2016

bnoordhuis Aug 20, 2016

Trott Aug 20, 2016 •

edited

Loading

yorkie Aug 20, 2016 •

edited

Loading

bnoordhuis Aug 20, 2016

bengl Aug 20, 2016

bnoordhuis commented Aug 20, 2016

jasnell commented Jan 6, 2017

sam-github left a comment

sam-github commented Jan 17, 2017

fhinkel commented Mar 26, 2017

bengl commented Jun 2, 2017

jasnell commented Aug 24, 2017

BridgeAR commented Sep 12, 2017

gibfahn commented Jan 15, 2018

crypto: expose ECDH class #8188

crypto: expose ECDH class #8188

Conversation

bengl commented Aug 19, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jbergstroem commented Aug 19, 2016

jasnell commented Aug 19, 2016

Trott commented Aug 19, 2016

bengl commented Aug 19, 2016

Trott commented Aug 19, 2016

bengl commented Aug 19, 2016

jbergstroem Aug 19, 2016

Choose a reason for hiding this comment

bengl commented Aug 19, 2016

bengl commented Aug 19, 2016

bnoordhuis Aug 20, 2016

Choose a reason for hiding this comment

Trott Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

yorkie Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Aug 20, 2016

Choose a reason for hiding this comment

bengl Aug 20, 2016

Choose a reason for hiding this comment

bnoordhuis commented Aug 20, 2016

jasnell commented Jan 6, 2017

sam-github left a comment

Choose a reason for hiding this comment

sam-github commented Jan 17, 2017

fhinkel commented Mar 26, 2017

bengl commented Jun 2, 2017

jasnell commented Aug 24, 2017

BridgeAR commented Sep 12, 2017

gibfahn commented Jan 15, 2018

bengl commented Aug 19, 2016 •

edited

Loading

Trott Aug 20, 2016 •

edited

Loading

yorkie Aug 20, 2016 •

edited

Loading