Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: expose built-in root certificates #26415

Closed
wants to merge 1 commit into from

Conversation

@bnoordhuis
Copy link
Member

commented Mar 3, 2019

Fixes: #25824

@cjihrig

cjihrig approved these changes Mar 3, 2019

Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
@BridgeAR
Copy link
Member

left a comment

LGTM with the doc comments being addressed.

@lpinca

lpinca approved these changes Mar 4, 2019

@sam-github
Copy link
Member

left a comment

A suggestion about the naming and docs, but otherwise LGTM, thanks.

Show resolved Hide resolved doc/api/tls.md Outdated
Show resolved Hide resolved doc/api/tls.md Outdated

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix25824 branch from 765bd55 to 270f323 Mar 5, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

Thanks for the reviews, feedback incorporated. I also added a regression test (forgot to check it in.)

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@bnoordhuis no comment on calling it tls.DEFAULT_CA like the other defaults?

Show resolved Hide resolved doc/api/tls.md Outdated
@jasnell

jasnell approved these changes Mar 5, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

no comment on calling it tls.DEFAULT_CA like the other defaults?

No strong opinion anyway. :-) I was waiting to see if anyone else commented on that.

FWIW, if I had to pick something in all caps, I'd probably opt for tls.CA_ROOTS or something like that.

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

To be clear, I'm not opionated about the capsiness of the name, they both look like fine names, it's this story that seem strange to me:

TLS exposes default values of some of the options as module properties:

  • tls .DEFAULT_CIPHERS, default value of tls.createSecureContext({ciphers: ...
  • tls.DEFAULT_ECDH_CURVE, default value ... {ecdhCurve:
  • tls.DEFAULT_MAX_VERSION, default value of ... {maxVersion:
  • tls.DEFAULT_MIN_VERSION, default value of ... {minVersion:

and now:

  • tls.CA_ROOTS / tls.roots the default value of .... {ca:

Perhaps I'm unreasonably perturbed by pattern breaks?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2019

The tls.DEFAULT_FOO exports are reassignable (and the new value gets picked up the next time a connection is created) but this one isn't, that's why I picked a different style for the name.

I suppose making the default CA roots fully overridable by assigning to tls.DEFAULT_CA or whatever isn't completely out of the question but I don't know.

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

I'd really prefer if we could move away from that DEFAULT_FOO setter pattern on the module as it's just not going to work for the ESM side of things. That's not blocking for this PR but we need to consider an alternative approach at some point here.

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@jasnell That is a bit of a side-track, but what would it have to look like to be esm-ok?

@bnoordhuis I see your point about not being able to be used to change the default, so being a bit different from the other DEFAULT_ definitions. I suspect reassignability might come as a feature request, and eventually we'll make it possible. It looks like you set the descriptor so that it will throw if anyone tries to set it, so that's good, nobody can complain that they set it and it didn't do anything. That was good enough for me, YMMV.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

I added one more sanity check to the test: https://ci.nodejs.org/job/node-test-pull-request/21294/

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

@sam-github @jasnell is this ready to land out of your perspective? (I did not check the comments fully)

@sam-github

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Would be good to get some feedback by @jasnell on esm-safety, but given this pattern is used for other top-level vars in tls, I can't see why it would be blocking.

Would be good to have some @nodejs/collaborators weigh in on the naming. Its bike shedding, but we'll have to live with the shed for a long time, best be happy with it now.

@Trott

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Would be good to have some @nodejs/collaborators weigh in on the naming. Its bike shedding, but we'll have to live with the shed for a long time, best be happy with it now.

Since you're inviting the bike shed discussion: I'd prefer rootCertificates to roots.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

Is there consensus on the name?

@mcollina
Copy link
Member

left a comment

LGTM I don't care about the names.

@ronkorving

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

If bike shedding, I would also prefer rootCertificates (+0) to roots, but regardless the name am very happy to see this land. LGTM.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

I agree with @ronkorving and @Trott that rootCertificates is a nicer name but in the end I am fine with either.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

I renamed it to rootCertificates because I don't really care either way. ^^

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

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member

left a comment

Still LGTM

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

The test failures are #23089 and #24593.

@nodejs-github-bot

This comment has been minimized.

@bnoordhuis bnoordhuis added author ready and removed author ready labels May 2, 2019

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix25824 branch from cfa3d8a to b4f2ac2 May 16, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

@nodejs-github-bot

This comment has been minimized.

@Trott

Trott approved these changes May 16, 2019

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix25824 branch from b4f2ac2 to c07cb00 May 17, 2019

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

Again because 11:23:07 collect2: fatal error: ld terminated with signal 9 [Killed]:

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@bnoordhuis bnoordhuis closed this May 20, 2019

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:fix25824 branch from c07cb00 to f1a3968 May 20, 2019

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request May 20, 2019

tls: expose built-in root certificates
Fixes: nodejs#25824
PR-URL: nodejs#26415
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Landed in f1a3968, cheers.

targos added a commit that referenced this pull request May 20, 2019

tls: expose built-in root certificates
Fixes: #25824
PR-URL: #26415
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

@BridgeAR BridgeAR referenced this pull request May 21, 2019

Merged

v12.3.0 proposal #27799

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799

BridgeAR added a commit that referenced this pull request May 21, 2019

2019-05-21, Version 12.3.0 (Current)
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.