crypto: use system CAs instead of bundled ones by AdamMajer · Pull Request #8334 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

crypto

Description of change

Use system supplied CAs instead of using Node's bundled version.
This is a compile time option with default reverting back to
using bundled certificates.

Also simplify cert_store lifetime by using reference count instead
of pulling it from under the SSL_CTX right before deletion. (merged)

/cc @nodejs/crypto

@AdamMajer Here typically Fixes: and full URIs are used when referencing issues to be closed :)

Here’s a CI run: https://ci.nodejs.org/job/node-test-commit/4835/

@addaleax ah ,yes of couse. I'm completely new to NodeJS code review, but it seems that all commit messages have PR-URL: and Reviewed-By:. so is it correct to assume that once pull requests are reviewed, I amend the commit message with these extra headers (And fix my Fixes: header :)) ?

@AdamMajer Usually, the person who lands the PR adds these review headers (the ones you named), but it’s not like there’s anything stopping you from adding PR-URL: https://github.com/nodejs/node/pull/8334 yourself. :)

I know that @rvagg had suggested that this is best done as a configure option but I can definitely see value in this as a runtime command-line flag also -- even if left undocumented for the time being.

Having it as runtime option is OK in rare situation, maybe if you need to specify multiple stores? But then can't that be done already with the environmental variables SSL_CERT_DIR and SSL_CERT_FILE? Enable this at compile time, then select various stores with environmental variables overriding OpenSSL defaults. That's how I understand it from quick grep through OpenSSL.

An alternative would be to have compile time option "no-bundled-ca-certs" with runtime override option (no-)use-bundled-ca-certs (or something like that),

look for system CAs as primary (including the environmental variables)
fall back bundled CAs unless runtime/compiletime flag set to not use them.

The idea behind compile time selection is to make like easier for Linux distributions (and their users) where bundled CAs are very bad idea. On Linux we definitely would not want to fallback to bundled CAs. But there are quite a per different permutations on how this can be done.

Would this make it possible to use the Windows system cert store?

Fixes #3159.

Being able to specify a path to root CAs at may also be useful. I think that would be more suited as a runtime flag?

Let's prefix it NODE_, may be something like NODE_SYSTEM_CERT_STORE?

On 08/30/2016 05:15 PM, Douglas Wilson wrote:

Would this make it possible to use the Windows system cert store?

For this better question would be, does OpenSSL support using Windows
cert store? I have no idea.

LGTM with a define naming nit, unless @bnoordhuis has some comments.

Having it as runtime option is OK in rare situation, maybe if you need to specify multiple stores?

For one, it would make it easier/possible to test this in CI :-)

For this better question would be, does OpenSSL support using Windows cert store? I have no idea.

Yea, I have no idea, that's why I was asking :) Your config parameter along with it's description would suggest it does, and that it is not restricted to whatever OpenSSL happens to do. Some Googling suggests that no, OpenSSL cannot use the Windows cert store (but I have no idea how to verify those claims), which seems to make the configure switch misleading(?)

Updated to use suggested define name. Minor change to comment, but otherwise same patch.

The runtime option can be done later. The compile time option still needs to be there for 2 reasons,

to not change current default behavior . maybe in node 7 we can delegate root certs to OS only as it should be possible on windows too, see http://stackoverflow.com/questions/9507184/can-openssl-on-windows-use-the-system-certificate-store but I'm not sure whether this belongs in Node or OpenSSL.
all Linux distros would prefer to manage CAs in one place. Here they need to change current default behavior.

One step at a time .

LGTM. It's a shame that we don't have a direct way of testing this in CI since we can't set compile time flags for CI. I would say that this should likely be considered to be unsupported/experimental until it's gone through at least one release and we can see if any issues come up on it.

That's only going to be the case when node is linked against the distro openssl. It defaults to /usr/local/ssl with the bundled copy and that's unlikely to be the right path.

Aside: s/setup/set up/

Actually, this does not appear to be the case. Look in openssl.gypi. I've tested this with OpenSUSE Leap 42.1 which needs bundled OpenSSL and it certainly uses the /etc/ssl path as defined in the openssl.gypi file.

@bnoordhuis I agree with @AdamMajer here, node's copy of OpenSSL uses a cert path in /etc, not /usr/local

PR-URL: #8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

The pointer to std::vector is unnecessary, so replace it with standard instance. Also, make the for() loop more readable by using actual type instead of inferred - there is no readability benefit here from obfuscating the type. PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

NodeJS can already use an external, shared OpenSSL library. This library knows where to look for OS managed certificates. Allow a compile-time option to use this CA store by default instead of using bundled certificates. In case when using bundled OpenSSL, the paths are also valid for majority of Linux systems without additional intervention. If this is not set, we can use SSL_CERT_DIR to point it to correct location. Fixes: nodejs#3159 PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

The pointer to std::vector is unnecessary, so replace it with standard instance. Also, make the for() loop more readable by using actual type instead of inferred - there is no readability benefit here from obfuscating the type. PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

NodeJS can already use an external, shared OpenSSL library. This library knows where to look for OS managed certificates. Allow a compile-time option to use this CA store by default instead of using bundled certificates. In case when using bundled OpenSSL, the paths are also valid for majority of Linux systems without additional intervention. If this is not set, we can use SSL_CERT_DIR to point it to correct location. Fixes: nodejs#3159 PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

PR-URL: nodejs#8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

Notable changes: * crypto: * ability to select cert store at runtime (Adam Majer) #8334 * Use system CAs instead of using bundled ones (Adam Majer) #8334 * deps: * upgrade npm to 4.1.2 (Kat Marchán) #11020 * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021 * doc: add basic documentation for WHATWG URL API (James M Snell) #10620 * process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * url: allow use of URL with http.request and https.request (James M Snell) #10638 PR-URL: #11062

@MylesBorins Is LTS interested in having this on 6.x?

It would require backporting (along with #11007 which fixes up the docs)

@sgallagher were you using node LTS when you thought the feature didn't exist, and PRed #11774 ?

cc: @jasnell

@sam-github Yes, we are on 6.10.0 in Fedora. I backported the patch manually and it's currently carried in our downstream packages. It would be convenient if it was maintained by upstream in 6.x, but not strictly required.

@sgallagher I suspect (EDIT: "ed") it would be more useful to distros if backported.

Can you PR your backport to node so Adam or I don't have to redo it?

@sam-github Sure. It was nearly trivial, but see #11794

In Alpine Linux we are also on 6.10.0 now, I've used the patch from @sgallagher (thanks for it!).

This was a while ago, but I'm curious if the runtime option ever got worked on? I have customers who install my app and I'd like for my app to use system-installed CAs on macOS. I think I'd have to build Node native module to access the certificates in Keychain Access, though, similar to how node-keytar does.

Any tips for telling Node to use system certs on macOS at runtime?

No work was done on Node.js side. You might want to ask on the openssl-users mailing list if OpenSSL has a mechanism for doing this.

@sam-github Isn’t node --use-openssl-ca the “runtime option”? Did I miss anything?

Sorry didn’t saw that @aguynamedben was asking for macOS and Keychain.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. labels Aug 30, 2016

addaleax added the openssl Issues and PRs related to the OpenSSL dependency. label Aug 30, 2016

AdamMajer mentioned this pull request Aug 30, 2016

please provide a way to use system-installed root certificates instead of bundled ones #3159

Closed

jasnell changed the title Distro crypto crypto: use system CAs instead of bundled ones Aug 30, 2016

indutny reviewed Aug 30, 2016
View reviewed changes

AdamMajer force-pushed the distro_crypto branch from b95d276 to 257910e Compare August 31, 2016 09:36

AdamMajer mentioned this pull request Aug 31, 2016

Node uses an hardcoded list of certificate authorities #4175

Closed

bnoordhuis reviewed Sep 5, 2016
View reviewed changes

AdamMajer force-pushed the distro_crypto branch from 257910e to 0463c07 Compare September 6, 2016 13:52

targos pushed a commit that referenced this pull request Jan 28, 2017

crypto: ability to select cert store at runtime

43f4951

PR-URL: #8334 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

sam-github mentioned this pull request Mar 10, 2017

doc: add version meta for SSL_CERT_DIR/FILE #11007

Merged

2 tasks

sgallagher mentioned this pull request Mar 10, 2017

crypto: Use system CAs instead of using bundled ones #11794

Closed

4 tasks

gibfahn mentioned this pull request Mar 17, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

grahamaj mentioned this pull request May 25, 2017

Configure the nodejs build to use os CA store smanders/externpro#140

Closed

djKianoosh mentioned this pull request Jun 13, 2017

Error: unable to get local issuer certificate lwsjs/local-web-server#65

Closed

timbru31 mentioned this pull request Jun 29, 2017

main/nodejs: upgrade to 6.11.0 alpinelinux/aports#1789

Closed

JPvRiel mentioned this pull request May 22, 2018

CA File atom/apm#340

Open

FranklinYu mentioned this pull request Jun 9, 2020

Trust the system root CA certificates yarnpkg/yarn#6578

Open

crypto: use system CAs instead of bundled ones by AdamMajer · Pull Request #8334 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: use system CAs instead of bundled ones - #8334

crypto: use system CAs instead of bundled ones#8334AdamMajer wants to merge 3 commits intonodejs:masternodejs/node:masterfrom AdamMajer:distro_cryptoAdamMajer/node:distro_cryptoCopy head branch name to clipboard

Conversation

AdamMajer commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented Aug 30, 2016

Uh oh!

addaleax commented Aug 30, 2016

Uh oh!

AdamMajer commented Aug 30, 2016

Uh oh!

addaleax commented Aug 30, 2016

Uh oh!

jasnell commented Aug 30, 2016

Uh oh!

AdamMajer commented Aug 30, 2016

Uh oh!

dougwilson commented Aug 30, 2016

Uh oh!

Fishrock123 commented Aug 30, 2016

Uh oh!

indutny Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

AdamMajer commented Aug 30, 2016

Uh oh!

indutny commented Aug 30, 2016

Uh oh!

jasnell commented Aug 30, 2016

Uh oh!

dougwilson commented Aug 30, 2016

Uh oh!

AdamMajer commented Aug 31, 2016

Uh oh!

jasnell commented Aug 31, 2016

Uh oh!

bnoordhuis Sep 5, 2016

Choose a reason for hiding this comment

Uh oh!

AdamMajer Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Oct 27, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github commented Mar 10, 2017

Uh oh!

sgallagher commented Mar 10, 2017

Uh oh!

sam-github commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgallagher commented Mar 10, 2017

Uh oh!

jirutka commented Mar 11, 2017

Uh oh!

aguynamedben commented Feb 22, 2019

Uh oh!

sam-github commented Feb 22, 2019

Uh oh!

FranklinYu commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

19 participants

crypto: use system CAs instead of bundled ones#8334
AdamMajer wants to merge 3 commits into
nodejs:masterfrom
AdamMajer:distro_crypto

AdamMajer commented Aug 30, 2016 •
edited
Loading

sam-github commented Mar 10, 2017 •
edited
Loading

FranklinYu commented Mar 27, 2019 •
edited
Loading