tls: new tls.TLSSocket() supports sec ctx options #11005

sam-github · 2017-01-25T20:55:54Z

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,test

sam-github · 2017-01-25T20:57:30Z

This is the non-controversial parts of #10846 PRed seperately.

At least, I assume its not controversial, the changes are straight forward, consistent, and have been requested in #10538

@nodejs/crypto PTAL

bnoordhuis · 2017-01-26T00:06:04Z

lib/_tls_wrap.js

@@ -348,7 +348,7 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
  // Wrap socket's handle
  var context = options.secureContext ||
                options.credentials ||
-                tls.createSecureContext();
+                tls.createSecureContext(options);


semver-major?

Do we always treat new properties in options objects as semver-major? If someone was passing unsupported option values to an API, and then the API started supporting those option values, it would feel major to them, but I don't know if that is our standard. What our API is is under-defined ATM.

bnoordhuis · 2017-01-26T00:07:50Z

test/parallel/test-tls-socket-default-options.js

+const join = require('path').join;
+const {
+  assert, connect, keys, tls
+} = require(join(common.fixturesDir, 'tls-connect'))();


That's a rather oblique way of doing require('assert')... Can you do it the regular way and leave in the common.hasCrypto check as well? It's much more Obviously Correct.

bnoordhuis · 2017-01-26T00:10:29Z

test/parallel/test-tls-socket-default-options.js

+      conn.on('end', common.mustCall(() => {
+        // Server sees nothing wrong with connection, even though the client's
+        // authentication of the server cert failed.
+        assert.strictEqual(recv, 'helo');


Just curious, is the spelling intentional?

Habit from SMTP. I can add another "l".

sam-github · 2017-01-27T20:27:18Z

@bnoordhuis I pushed 570a99a, though I think starting all the tls tests with a half-screen of copy-n-pasted boilerplate distracts from the test itself.

sam-github · 2017-02-13T21:43:40Z

@bnoordhuis PTAL, I made both changes you requested, and @jasnell confirms additional properties are not semver-major.

bnoordhuis

LGTM with a nit and a suggestion.

bnoordhuis · 2017-02-13T22:04:07Z

doc/api/tls.md

+    will be created by passing the entire `options` object to
+    `tls.createSecureContext()`. *Note*: In effect, all
+    [`tls.createSecureContext()`][] options can be provided, but they will be
+    _completely ignored_ unless the `secureContext` option is missing.


"when the secureContext option is set"? Maybe it's because it's 11 PM but I found it hard to parse.

bnoordhuis · 2017-02-13T22:05:54Z

test/parallel/test-tls-socket-default-options.js


 if (!common.hasCrypto) {
  common.skip('missing crypto');
-  return;
+  process.exit(0);


Unnecessary change.

sam-github · 2017-02-20T15:28:42Z

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

sam-github · 2017-02-20T15:36:02Z

git checkout fail: https://ci.nodejs.org/job/node-test-commit-linux-fips/6595/nodes=ubuntu1404-64/console, trying again

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

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: nodejs#10538 PR-URL: nodejs#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236)

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [nodejs#11489](nodejs#11489) PR-URL: nodejs#11553

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [nodejs#11288](nodejs#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [nodejs#11562](nodejs#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [nodejs#11275](nodejs#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [nodejs#11005](nodejs#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [nodejs#11236](nodejs#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [nodejs#11489](nodejs#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [nodejs#11006](nodejs#11006) * src: adding support for trace-event tracing (misterpoe) [nodejs#11106](nodejs#11106) PR-URL: nodejs#11553

Notables changes: * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig) [#11288](nodejs/node#11288) * http: new functions to access the headers for an outgoing HTTP message (Brian White) [#11562](nodejs/node#11562) * lib: deprecate node --debug at runtime (Josh Gavant) [#11275](nodejs/node#11275) * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts) [#11005](nodejs/node#11005) * url: adding URL.prototype.toJSON support (Michaël Zasso) [#11236](nodejs/node#11236) * doc: items in the API documentation may now have changelogs (Anna Henningsen) [#11489](nodejs/node#11489) * crypto: adding support for OPENSSL_CONF again (Sam Roberts) [#11006](nodejs/node#11006) * src: adding support for trace-event tracing (misterpoe) [#11106](nodejs/node#11106) PR-URL: nodejs/node#11553 Signed-off-by: Ilkka Myller <[email protected]>

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: #10538 PR-URL: #11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: #10538 PR-URL: #11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: #10538 PR-URL: #11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: #10538 PR-URL: #11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) #10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - ability to select cert store at runtime (Adam Majer) #8334 - Use system CAs instead of using bundled ones (Adam Majer) #8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 - adding support for OPENSSL_CONF again (Sam Roberts) #11006 - make LazyTransform compabile with Streams1 (Matteo Collina) #12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) #11094 - upgrade libuv to 1.10.2 (cjihrig) #10717 - upgrade libuv to 1.10.1 (cjihrig) #9647 - upgrade libuv to 1.10.0 (cjihrig) #9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) #9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * readline: - add option to stop duplicates in history (Danny Nemer) #2982 * src: - support "--" after "-e" as end-of-options (John Barboza) #10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) #11005 - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 PR-URL: #13059

Because of a poorly constructed test, only one of the two test vectors ran. The test also failed to cover the authentication error that occurs when the server's certificate is not trusted. Both issues are fixed. Fix: nodejs/node#10538 PR-URL: nodejs/node#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add support to new tls.TLSSocket() to create a SecureContext object with all its supported options, in the same way they are supported for all the other APIs that need SecureContext objects. Fix: nodejs/node#10538 PR-URL: nodejs/node#11005 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

This LTS release comes with 126 commits. This includes 40 which are test related, 32 which are doc related, 12 which are build / tool related and 4 commits which are updates to dependencies. Notable Changes: * build: - support for building mips64el (nanxiongchao) nodejs/node#10991 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - ability to select cert store at runtime (Adam Majer) nodejs/node#8334 - Use system CAs instead of using bundled ones (Adam Majer) nodejs/node#8334 - The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 - adding support for OPENSSL_CONF again (Sam Roberts) nodejs/node#11006 - make LazyTransform compabile with Streams1 (Matteo Collina) nodejs/node#12380 * deps: - upgrade libuv to 1.11.0 (cjihrig) nodejs/node#11094 - upgrade libuv to 1.10.2 (cjihrig) nodejs/node#10717 - upgrade libuv to 1.10.1 (cjihrig) nodejs/node#9647 - upgrade libuv to 1.10.0 (cjihrig) nodejs/node#9267 * dns: - Implemented `{ttl: true}` for `resolve4()` and `resolve6()` (Ben Noordhuis) nodejs/node#9296 * process: - add NODE_NO_WARNINGS environment variable (cjihrig) nodejs/node#10842 * readline: - add option to stop duplicates in history (Danny Nemer) nodejs/node#2982 * src: - support "--" after "-e" as end-of-options (John Barboza) nodejs/node#10651 * tls: - new tls.TLSSocket() supports sec ctx options (Sam Roberts) nodejs/node#11005 - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 PR-URL: nodejs/node#13059

nodejs-github-bot added tls Issues and PRs related to the tls subsystem. dont-land-on-v7.x labels Jan 25, 2017

sam-github mentioned this pull request Jan 25, 2017

doc: tls API for direct TLS socket use #10846

Closed

4 tasks

bnoordhuis requested changes Jan 26, 2017

View reviewed changes

sam-github added semver-minor PRs that contain new features and should be released in the next minor version. and removed dont-land-on-v7.x labels Jan 27, 2017

jasnell approved these changes Jan 27, 2017

View reviewed changes

bnoordhuis approved these changes Feb 13, 2017

View reviewed changes

sam-github force-pushed the tls-ctor-sec-ctx-options branch from 570a99a to 208612e Compare February 20, 2017 15:27

sam-github force-pushed the tls-ctor-sec-ctx-options branch from 208612e to 15e6d16 Compare February 20, 2017 15:29

sam-github added 2 commits February 20, 2017 07:38

sam-github force-pushed the tls-ctor-sec-ctx-options branch from 15e6d16 to ea56799 Compare February 20, 2017 15:40

sam-github merged commit ea56799 into nodejs:master Feb 20, 2017

sam-github deleted the tls-ctor-sec-ctx-options branch February 20, 2017 15:58

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins added the land-on-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: new tls.TLSSocket() supports sec ctx options #11005

tls: new tls.TLSSocket() supports sec ctx options #11005

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

bnoordhuis Jan 26, 2017

sam-github Jan 27, 2017

bnoordhuis Jan 26, 2017

bnoordhuis Jan 26, 2017

sam-github Jan 27, 2017

sam-github commented Jan 27, 2017

sam-github commented Feb 13, 2017

bnoordhuis left a comment

bnoordhuis Feb 13, 2017

bnoordhuis Feb 13, 2017

sam-github commented Feb 20, 2017

sam-github commented Feb 20, 2017

tls: new tls.TLSSocket() supports sec ctx options #11005

tls: new tls.TLSSocket() supports sec ctx options #11005

Conversation

sam-github commented Jan 25, 2017

Checklist

Affected core subsystem(s)

sam-github commented Jan 25, 2017

bnoordhuis Jan 26, 2017

Choose a reason for hiding this comment

sam-github Jan 27, 2017

Choose a reason for hiding this comment

bnoordhuis Jan 26, 2017

Choose a reason for hiding this comment

bnoordhuis Jan 26, 2017

Choose a reason for hiding this comment

sam-github Jan 27, 2017

Choose a reason for hiding this comment

sam-github commented Jan 27, 2017

sam-github commented Feb 13, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Feb 13, 2017

Choose a reason for hiding this comment

bnoordhuis Feb 13, 2017

Choose a reason for hiding this comment

sam-github commented Feb 20, 2017

sam-github commented Feb 20, 2017