tls: rejectUnauthorized is treated to true by default #5923

ghaiklor · 2016-03-27T13:12:31Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

tls

Description of change

Improves usability as described here - #5917

tls.connect treats rejectUnauthorized as a false value, when we need to treat it only when rejectUnauthorized is really set to false.

ghaiklor · 2016-03-27T14:00:41Z

In my last commit I should explicitly set to rejectUnauthorized: false because of new behaviour of default params for rejectUnauthorized.

I think, all test-cases are passed until now because rejectUnauthorized was false by default in all test-cases.

When I've fixed that problem, I'm start getting this error:

New, TLSv1/SSLv3, Cipher is ECDHE-RSA-AES256-SHA
Server public key is 2048 bit
Secure Renegotiation IS supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
    Protocol  : TLSv1
    Cipher    : ECDHE-RSA-AES256-SHA
    Session-ID: 84BA6EEF82ED3FEC65728E801DBAE5E4FF0898A61C9CDD5A39EC6B9DB1883DB6
    Session-ID-ctx:
    Master-Key: 0B44AE6722D0039BFEF033417DA8F6F624E9E0DE98BB844254E93D3F641A3B937550EB4DE46FB83F16394B80A85F57AB
    Key-Arg   : None
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    Start Time: 1459086986
    Timeout   : 7200 (sec)
    Verify return code: 10 (certificate has expired)
---
drop connection and then reconnect
CONNECTED(00000003)

Certificate has expired and tests are failing. So, for now, I explicitly set rejectUnauthorized: false in test-cases.

jasnell · 2016-04-01T15:42:46Z

@nodejs/crypto

jasnell · 2017-02-28T23:20:22Z

ping @nodejs/crypto

bnoordhuis

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

sam-github

change is good, but it requires changing the documentation as well

ghaiklor · 2017-03-20T16:56:35Z

@sam-github I've found only one wrong occurrence in docs. Updated.

sam-github

f true, the server certificate is verified against the list of supplied CAs.

tls.connect docs need to be changed, this should now read "If not false, the server..."

sam-github

tls.createServer docs still say "If true, ..", but also need to say "If not false, ....".

sam-github · 2017-03-22T22:55:19Z

doc/api/tls.md

@@ -1014,7 +1014,7 @@ changes:
    `false`.
  * `rejectUnauthorized` {boolean} If `true` the server will reject any


If not false

sam-github · 2017-03-22T22:59:45Z

https://nodejs.org/api/tls.html#tls_tlssocket_renegotiate_options_callback, tls.renegotiate() says nothing at all about the default values... but did they change? I think they did, but its a bit hard to see from just the PR diff.

sam-github · 2017-03-22T23:03:30Z

And btw, @ghaiklor , thanks for picking this up again, the behaviour this changes should be fixed.

Heads up, it will need to land pretty quickly to get into 8.x... if it misses the -rc, it won't be released until 9.x, which would be a pity. I've been travelling lately, but I'll keep a sharp eye on this to land as soon as possible.

tls.connect treats rejectUnauthorized as a false value, when we need to treat it only when rejectUnauthorized is really set to false

requestCert is false by default, rejectUnauthorized is true by default.

The problem here is old behaviour of rejectUnauthorized. With previous implementation you could treat undefined as false. After this bevahiour was fixed, we need to explicitly set to false.

Updates docs in all rejectUnauthorized flags. With this PR it defaults to true instead false.

ghaiklor · 2017-03-23T08:24:51Z

@sam-github added your change to all rejectUnauthorized properties.

sam-github · 2017-03-23T16:46:21Z

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

sam-github · 2017-03-23T17:52:06Z

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

sam-github · 2017-03-23T17:53:04Z

Thanks @jasnell, I'll merge once ci is green, which I expect to see. But still waiting.

sam-github · 2017-03-23T20:31:29Z

Landed in 348cc80, thank you, @ghaiklor

rejectUnauthorized used to be false when the property was undefined or null, quietly allowing client connections for which certificates have been requested (requestCert is true) even when the client certificate was not authorized (signed by a trusted CA). Change this so rejectUnauthorized is always true unless it is explicitly set to false. PR-URL: #5923 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

Fishrock123 added the tls Issues and PRs related to the tls subsystem. label Mar 27, 2016

vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 27, 2016

ghaiklor mentioned this pull request Mar 27, 2016

Fix rejectUnauthorized: undefined in tls.connect, requestCert in tls.createServer #5917

Closed

2 tasks

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23

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:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

bnoordhuis approved these changes Mar 1, 2017

View reviewed changes

cjihrig approved these changes Mar 1, 2017

View reviewed changes

sam-github suggested changes Mar 1, 2017

View reviewed changes

sam-github approved these changes Mar 22, 2017

View reviewed changes

sam-github suggested changes Mar 22, 2017

View reviewed changes

sam-github reviewed Mar 22, 2017

View reviewed changes

sam-github removed the stalled Issues and PRs that are stalled. label Mar 22, 2017

ghaiklor added 5 commits March 23, 2017 10:09

tls: rejectUnauthorized is treated to true by default

22ffa31

tls.connect treats rejectUnauthorized as a false value, when we need to treat it only when rejectUnauthorized is really set to false

tls: change default values in createServer()

1575739

requestCert is false by default, rejectUnauthorized is true by default.

tls: fix tests

f975376

The problem here is old behaviour of rejectUnauthorized. With previous implementation you could treat undefined as false. After this bevahiour was fixed, we need to explicitly set to false.

docs: update tls section

be35f5a

Updates docs in all rejectUnauthorized flags. With this PR it defaults to true instead false.

docs: update description to rejectUnauthorized options

567bf44

sam-github approved these changes Mar 23, 2017

View reviewed changes

jasnell approved these changes Mar 23, 2017

View reviewed changes

jasnell added this to the 8.0.0 milestone Mar 23, 2017

sam-github mentioned this pull request Mar 23, 2017

claims that PR is not accepted yet nodejs/node-review#8

Closed

sam-github closed this Mar 23, 2017

ghaiklor deleted the fix/5917 branch March 23, 2017 20:34

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

ChALkeR added the security Issues and PRs related to security. label May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: rejectUnauthorized is treated to true by default #5923

tls: rejectUnauthorized is treated to true by default #5923

ghaiklor commented Mar 27, 2016

ghaiklor commented Mar 27, 2016

jasnell commented Apr 1, 2016

jasnell commented Feb 28, 2017

bnoordhuis left a comment

sam-github left a comment

ghaiklor commented Mar 20, 2017

sam-github left a comment

sam-github left a comment

sam-github Mar 22, 2017

sam-github commented Mar 22, 2017

sam-github commented Mar 22, 2017

ghaiklor commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017

		@@ -1014,7 +1014,7 @@ changes:
		`false`.
		* `rejectUnauthorized` {boolean} If `true` the server will reject any

tls: rejectUnauthorized is treated to true by default #5923

tls: rejectUnauthorized is treated to true by default #5923

Conversation

ghaiklor commented Mar 27, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

ghaiklor commented Mar 27, 2016

jasnell commented Apr 1, 2016

jasnell commented Feb 28, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

ghaiklor commented Mar 20, 2017

sam-github left a comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

sam-github Mar 22, 2017

Choose a reason for hiding this comment

sam-github commented Mar 22, 2017

sam-github commented Mar 22, 2017

ghaiklor commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017

sam-github commented Mar 23, 2017