Skip to content

https: support rejectUnauthorized for unix sockets#13505

Merged
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:13470
Jun 8, 2017
Merged

https: support rejectUnauthorized for unix sockets#13505
cjihrig merged 1 commit intonodejs:masterfrom
cjihrig:13470

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 6, 2017

This commit allows self signed certificates to work with unix sockets by forwarding the rejectUnauthorized option.

Fixes: #13470

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

https

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 6, 2017
Trott
Trott previously requested changes Jun 6, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test needs a common.refreshTmpDir().

test/parallel/test-https-unix-socket-self-signed.js Outdated
Copy link
Member

@Trott Trott Jun 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using common.PIPE requires common.refreshTmpDir() (unless the test only runs on Windows).

lpinca
lpinca reviewed Jun 7, 2017
View reviewed changes
test/parallel/test-https-unix-socket-self-signed.js Outdated
Copy link
Member

@lpinca lpinca Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super tiny nit: what do you think about using https.get()? I like it because it calls request.end() implicitly.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 7, 2017

Added common.refreshTmpDir() and switched from https.request() to https.get().

@Trott Trott dismissed their stale review June 7, 2017 18:21

test looks good to me now, thanks

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 8, 2017 •
edited
Loading

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

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

@cjihrig
https: support rejectUnauthorized for unix sockets
d0571a9 
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Fixes: nodejs#13470
PR-URL: nodejs#13505
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@cjihrig cjihrig merged commit d0571a9 into nodejs:master Jun 8, 2017
@cjihrig cjihrig deleted the 13470 branch June 8, 2017 17:46
addaleax pushed a commit that referenced this pull request Jun 10, 2017
@cjihrig @addaleax
https: support rejectUnauthorized for unix sockets
c4cbd99 
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Fixes: #13470
PR-URL: #13505
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
addaleax added a commit that referenced this pull request Jun 10, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
e1b2a17 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)
@addaleax addaleax mentioned this pull request Jun 10, 2017
Merged
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
2b0a8bd 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
e90ad78 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-13, Version 8.1.1 (Current)
c7f35e2 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
MylesBorins pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @MylesBorins
2017-06-13, Version 8.1.1 (Current)
3e1492a 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
jasnell pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @jasnell
2017-06-13, Version 8.1.1 (Current)
9dc3f93 
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 17, 2017

should this land on v6.x? Was this a new feature or a bugfix?

@sam-github
Copy link
Contributor

sam-github commented Jul 21, 2017

Its a bug fix, there was a problem with self-signed certs on unix domain sockets (TCP and Unix domain sockets should not have differences in their TLS behaviour).

core/node (pr/13505 $% u=) % node -v
v6.11.1
core/node (pr/13505 $% u=) % node test/parallel/test-https-unix-socket-self-signed.js
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: self signed certificate
    at Error (native)
    at TLSSocket.<anonymous> (_tls_wrap.js:1092:38)
    at emitNone (events.js:86:13)
    at TLSSocket.emit (events.js:185:7)
    at TLSSocket._finishInit (_tls_wrap.js:610:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:440:38)
core/node (pr/13505 $% u=) % nvm i 8
Downloading https://nodejs.org/dist/v8.2.1/node-v8.2.1-linux-x64.tar.xz...
######################################################################## 100.0%
Now using node v8.2.1 (npm v5.3.0)
core/node (pr/13505 $% u=) % node test/parallel/test-https-unix-socket-self-signed.js

@sam-github
Copy link
Contributor

sam-github commented Jul 21, 2017

Doesn't cherry pick, I am backporting.

@sam-github sam-github mentioned this pull request Jul 21, 2017
Closed
4 tasks
sam-github pushed a commit to sam-github/node that referenced this pull request Jul 21, 2017
@cjihrig @sam-github
https: support rejectUnauthorized for unix sockets
1658cb3 
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Fixes: nodejs#13470
PR-URL: nodejs#13505
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 21, 2017
@cjihrig @MylesBorins
https: support rejectUnauthorized for unix sockets
65f8bfb 
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Backport-PR-URL: #14415
Fixes: #13470
PR-URL: #13505
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 21, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jul 31, 2017
@cjihrig @MylesBorins
https: support rejectUnauthorized for unix sockets
0130298 
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Backport-PR-URL: #14415
Fixes: #13470
PR-URL: #13505
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott left review comments

+3 more reviewers

@sam-github sam-github sam-github approved these changes

@refack refack refack approved these changes

@danbev danbev danbev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@cjihrig @MylesBorins @sam-github @refack @danbev @Trott @lpinca @nodejs-github-bot