tls: expose built-in root certificates by bnoordhuis · Pull Request #26415 · nodejs/node · GitHub

Fixes: #25824

@bnoordhuis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2751/pipeline

LGTM with the doc comments being addressed.

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

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

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

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.

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?

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.

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.

@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.

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

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

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.

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.

Is there consensus on the name?

LGTM I don't care about the names.

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

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

The test failures are #23089 and #24593.

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

Rebased one more time. CI: https://ci.nodejs.org/job/node-test-pull-request/23139/

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

Fixes: nodejs#25824

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

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

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

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

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>

Landed in f1a3968, cheers.

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>

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

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

This is a partial backport of commit f1a3968 ("tls: expose built-in root certificates") from the master branch. The original commit adds a new API, this commit just backports the non-visible changes to ease backporting follow-up commits. Refs: nodejs#26415

This is a partial backport of commit f1a3968 ("tls: expose built-in root certificates") from the master branch. The original commit adds a new API, this commit just backports the non-visible changes to ease backporting follow-up commits. PR-URL: #26415 Backport-PR-URL: #29137 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>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 3, 2019

cjihrig approved these changes Mar 3, 2019

View reviewed changes

vsemozhetbyt reviewed Mar 3, 2019

View reviewed changes

Comment thread doc/api/tls.md Outdated

Comment thread doc/api/tls.md Outdated

BridgeAR approved these changes Mar 4, 2019

View reviewed changes

lpinca approved these changes Mar 4, 2019

View reviewed changes

sam-github approved these changes Mar 4, 2019

View reviewed changes

Comment thread doc/api/tls.md Outdated

Comment thread doc/api/tls.md Outdated

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 4, 2019

bnoordhuis force-pushed the fix25824 branch from 765bd55 to 270f323 Compare March 5, 2019 19:48

vsemozhetbyt reviewed Mar 5, 2019

View reviewed changes

Comment thread doc/api/tls.md Outdated

jasnell approved these changes Mar 5, 2019

View reviewed changes

mcollina approved these changes Mar 22, 2019

View reviewed changes

ronkorving approved these changes Mar 25, 2019

View reviewed changes

sam-github mentioned this pull request Apr 4, 2019

Documented way to add certificates to existing SecureContext #27079

Open

bnoordhuis force-pushed the fix25824 branch from f6cfaea to cfa3d8a Compare April 9, 2019 10:28

bnoordhuis added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels May 2, 2019

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

Trott approved these changes May 16, 2019

View reviewed changes

tls: expose built-in root certificates

c07cb00

Fixes: nodejs#25824

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

bnoordhuis closed this May 20, 2019

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

BridgeAR mentioned this pull request May 21, 2019

v12.3.0 proposal #27799

Merged

4 tasks

bnoordhuis mentioned this pull request Aug 15, 2019

[v8.x] crypto: update root certificates #29137

Closed

BethGriggs added the lts-watch-v10.x label Aug 20, 2019

BethGriggs mentioned this pull request Sep 19, 2019

v8.16.2 proposal #29617

Merged

BethGriggs added land-on-v10.x and removed lts-watch-v10.x labels Oct 7, 2019

tls: expose built-in root certificates by bnoordhuis · Pull Request #26415 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: expose built-in root certificates - #26415

tls: expose built-in root certificates#26415bnoordhuis wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix25824bnoordhuis/io.js:fix25824Copy head branch name to clipboard

Conversation

bnoordhuis commented Mar 3, 2019

Uh oh!

nodejs-github-bot commented Mar 3, 2019

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnoordhuis commented Mar 5, 2019

Uh oh!

sam-github commented Mar 5, 2019

Uh oh!

Uh oh!

bnoordhuis commented Mar 5, 2019

Uh oh!

sam-github commented Mar 6, 2019

Uh oh!

bnoordhuis commented Mar 6, 2019

Uh oh!

jasnell commented Mar 6, 2019

Uh oh!

sam-github commented Mar 6, 2019

Uh oh!

bnoordhuis commented Mar 7, 2019

Uh oh!

BridgeAR commented Mar 10, 2019

Uh oh!

sam-github commented Mar 11, 2019

Uh oh!

Trott commented Mar 11, 2019

Uh oh!

bnoordhuis commented Mar 22, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronkorving commented Mar 25, 2019

Uh oh!

BridgeAR commented Mar 25, 2019

Uh oh!

bnoordhuis commented Apr 9, 2019

Uh oh!

nodejs-github-bot commented Apr 10, 2019

Uh oh!

bnoordhuis commented May 16, 2019

Uh oh!

nodejs-github-bot commented May 16, 2019

Uh oh!

bnoordhuis commented May 17, 2019

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

nodejs-github-bot commented May 17, 2019

Uh oh!

bnoordhuis commented May 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

tls: expose built-in root certificates#26415
bnoordhuis wants to merge 1 commit into
nodejs:masterfrom
bnoordhuis:fix25824