crypto: use system CAs instead of bundled ones #8334

Closed
wants to merge 3 commits into
from

Projects

None yet
@AdamMajer
Contributor
AdamMajer commented Aug 30, 2016 edited
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)

@addaleax addaleax added the openssl label Aug 30, 2016
@addaleax
Member
@addaleax
Member

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

@AdamMajer
Contributor

@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 :)) ?

@addaleax
Member

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

@jasnell
Member
jasnell commented Aug 30, 2016

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.

@jasnell jasnell changed the title from Distro crypto to crypto: use system CAs instead of bundled ones Aug 30, 2016
@AdamMajer
Contributor

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),

  1. look for system CAs as primary (including the environmental variables)
  2. 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.

@dougwilson

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

@Fishrock123
Member

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?

@indutny indutny commented on an outdated diff Aug 30, 2016
configure
@@ -905,6 +910,8 @@ def configure_openssl(o):
o['variables']['node_use_openssl'] = b(not options.without_ssl)
o['variables']['node_shared_openssl'] = b(options.shared_openssl)
o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0
+ if options.use_system_ca_store:
+ o['defines'] += ['USE_SYSTEM_CERTIFICATE_STORE']
@indutny
indutny Aug 30, 2016 Member

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

@indutny indutny and 1 other commented on an outdated diff Aug 30, 2016
src/node_crypto.h
@@ -142,13 +142,6 @@ class SecureContext : public BaseObject {
void FreeCTXMem() {
if (ctx_) {
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
- if (ctx_->cert_store == root_cert_store) {
- // SSL_CTX_free() will attempt to free the cert_store as well.
- // Since we want our root_cert_store to stay around forever
- // we just clear the field. Hopefully OpenSSL will not modify this
- // struct in future versions.
- ctx_->cert_store = nullptr;
@indutny
indutny Aug 30, 2016 Member

Why isn't this necessary anymore?

@AdamMajer
AdamMajer Aug 30, 2016 Contributor

On 08/30/2016 06:44 PM, Fedor Indutny wrote:

In src/node_crypto.h
#8334 (comment):

@@ -142,13 +142,6 @@ class SecureContext : public BaseObject {
void FreeCTXMem() {
if (ctx_) {
env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);

  •  if (ctx_->cert_store == root_cert_store) {
    
  •    // SSL_CTX_free() will attempt to free the cert_store as well.
    
  •    // Since we want our root_cert_store to stay around forever
    
  •    // we just clear the field. Hopefully OpenSSL will not modify this
    
  •    // struct in future versions.
    
  •    ctx_->cert_store = nullptr;
    

Why isn't this necessary anymore?

See the commit message that touches this. Basically, using references
count makes this no longer necessary. The OpenSSL free function checks
that references > 1 and does not free the cert store. The free function
does decrement reference hence the reason why it is needed to increment
it every time it is assigned to a SSL_CTX

  • Adam
@indutny
indutny Aug 30, 2016 Member

Ah, I didn't see reference count increment above. Thanks!

@AdamMajer
Contributor

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.

@indutny
Member
indutny commented Aug 30, 2016

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

@jasnell
Member
jasnell commented Aug 30, 2016

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

@dougwilson

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(?)

@AdamMajer
Contributor

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,

  1. 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.
  2. all Linux distros would prefer to manage CAs in one place. Here they need to change current default behavior.

One step at a time .

@jasnell
Member
jasnell commented Aug 31, 2016

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.

@bnoordhuis bnoordhuis and 2 others commented on an outdated diff Sep 5, 2016
src/node_crypto.cc
@@ -751,6 +751,23 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
CHECK_EQ(sc->ca_store_, nullptr);
if (!root_cert_store) {
+#if defined(NODE_SYSTEM_CERT_STORE)
+ // *Assume* OpenSSL is setup correctly, which is the case
+ // for distribution supplied versions.
@bnoordhuis
bnoordhuis Sep 5, 2016 Member

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/

@AdamMajer
AdamMajer Sep 6, 2016 Contributor

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.

@sam-github
sam-github Oct 27, 2016 Member

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

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Sep 5, 2016
src/node_crypto.cc
}
sc->ca_store_ = root_cert_store;
+ // increment reference count so global store is not deleted along with CTX
+ CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
@bnoordhuis
bnoordhuis Sep 5, 2016 Member

Can you capitalize and punctuate the comments you've added?

@AdamMajer
AdamMajer Sep 6, 2016 Contributor

Will do.

@AdamMajer
Contributor

so, LGTM? Would be nice to have this in Node 6.6

@jasnell
Member
jasnell commented Sep 9, 2016

@bnoordhuis @nodejs/crypto ... does this LGTY?

@sxa555
Contributor
sxa555 commented Sep 15, 2016

I think this is a great addition, but has the name NODE_SYSTEM_CERT_STORE been chosen arbitrarily? I've got the following variables that I already define on my system:

CURL_CA_BUNDLE
GIT_SSL_CAINFO

So while those two examples aren't quite consistent in the naming they do both have "CA" in the name, especially given that we're not talking about a client certificate store here. I don't know which other products use such a variable to know if either of those names are better, but to me the second one with SSL in it makes a bit more sense, so how about NODE_SSL_CAINFO?

@indutny
Member
indutny commented Sep 15, 2016

LGTM

@AdamMajer
Contributor

On 09/15/2016 02:48 PM, Stewart Addison wrote:

I think this is a great addition, but has the name
NODE_SYSTEM_CERT_STORE been chosen arbitrarily? I've got the following
variables that I already define on my system:

CURL_CA_BUNDLE
GIT_SSL_CAINFO

This is not an environmental variable. This is just a compile-time
define, for now.

  • Adam
@chino
chino commented Sep 26, 2016

Sorry if this isn't the right ticket but any reason not to detect and use the semi standard $SSL_CERT_FILE environment variable? It's used by curl and various other tools.

@AdamMajer
Contributor

On 09/26/2016 11:50 PM, Daniel Aquino wrote:

Sorry if this isn't the right ticket but any reason not to detect and
use the semi standard |$SSL_CERT_FILE| environment variable? It's used
by curl and various other tools.

The variable name here is a compile time flag that simply switches
default store to not bundled stuff, but distribution supplied CA store.
The runtime stuff is defined elsewhere, for example openssl.gypi has,

  # Set to ubuntu default path for convenience. If necessary,
  # override this at runtime with the SSL_CERT_DIR environment
  # variable.
  'OPENSSLDIR="/etc/ssl"',

so you can already do that. And I just tested this,

SSL_CERT_DIR=/cert_dir node node_app.js

it works.

SSL_CERT_FILE=/cert_dir/cert.pem node node_app.js

it works too.

Try it. But maybe you need to have this patch enabled to actually enable
these modified default paths to be used by bunbled or distro OpenSSL.
This I did not test since my version of NodeJS is not using bundled CAs
for few weeks now.

Cheers,
Adam

Anyway, LGTM ? It's been sitting in queue without much objection for
sometime now.

@chino
chino commented Sep 28, 2016

I actually did try it but it only worked after I set the "cafile" config
value. Hopefully the new version wouldn't require that anymore.

On Tue, Sep 27, 2016, 6:15 AM Adam Majer notifications@github.com wrote:

On 09/26/2016 11:50 PM, Daniel Aquino wrote:

Sorry if this isn't the right ticket but any reason not to detect and
use the semi standard |$SSL_CERT_FILE| environment variable? It's used
by curl and various other tools.

The variable name here is a compile time flag that simply switches
default store to not bundled stuff, but distribution supplied CA store.
The runtime stuff is defined elsewhere, for example openssl.gypi has,

Set to ubuntu default path for convenience. If necessary,

override this at runtime with the SSL_CERT_DIR environment

variable.

'OPENSSLDIR="/etc/ssl"',

so you can already do that. And I just tested this,

SSL_CERT_DIR=/cert_dir node node_app.js

it works.

SSL_CERT_FILE=/cert_dir/cert.pem node node_app.js

it works too.

Try it. But maybe you need to have this patch enabled to actually enable
these modified default paths to be used by bunbled or distro OpenSSL.
This I did not test since my version of NodeJS is not using bundled CAs
for few weeks now.

Cheers,
Adam

Anyway, LGTM ? It's been sitting in queue without much objection for
sometime now.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#8334 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AABSO8f7bRzE_d-iGBeTkh3PnEg_VTKUks5quOzGgaJpZM4JweeU
.

@AdamMajer
Contributor

On 09/28/2016 03:53 AM, Daniel Aquino wrote:

I actually did try it but it only worked after I set the "cafile" config
value. Hopefully the new version wouldn't require that anymore.

You need to build with the patch enabled for
SSL_CERT_DIR and SSL_CERT_FILE to actually work. These are OpenSSL env
variables used by SSL_CTX_set_default_verify_paths AFAIK, so if you
don't configure with

./configure --use-system-ca-store

then you need to use cafile because OpenSSL stuff is not used.

@AdamMajer
Contributor

ping? is this patch now in the "lost" pile?

@bnoordhuis
Member

I can't speak for other collaborators but for me personally it's in my "revisit when I have more time and inclination" pile. :-)

@fujifish

Wouldn't it be simpler to change the createSecureContext code in _tls_common.js to this:

// always add bundled root CA's
c.context.addRootCerts();
// also add custom CA if needed
if (options.ca) {
  if (Array.isArray(options.ca)) {
    for (i = 0; i < options.ca.length; i++) {
      c.context.addCACert(options.ca[i]);
    }
  } else {
    c.context.addCACert(options.ca);
  }
} 
@sam-github
Member

I'm looking at it, it hasn't fallen into the dust bin.

src/node_crypto.cc
+ // Increase references to 2 to avoid this scenario.
+ CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
+ } else {
+ // Failed to load, default to nothing.
@sam-github
sam-github Oct 14, 2016 Member

This creates an undebuggable (by the user of node) situation. Is it not possible to print some kind of error here? What are the possible reasons for failure? non-existence of cert store? invalid format of cert store? ...?

@sam-github
sam-github Oct 27, 2016 Member

@AdamMajer I suggest emitting a warning. I added code to do this in #9139, not too sure what to do here, maybe you want to wait? Maybe pull in the code, and whichever PR merges last deals with the conflicts?

@sam-github
Member

@AdamMajer This doesn't literally use the system's cert store, it uses the OpenSSL cert store, which should be setup to link to the system's certs. On my laptop (Ubuntu), this is arranged by keeping symlinks up to date from /etc/ssl/certs/... into /user/share/ca-certificates/mozilla/.... For those wondering, the location from which CA certs will be loaded with this PR is described here: https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_load_verify_locations.html, note some overrides are possible using env vars.

Maybe the configure option could be more accurately named as --use-openssl-default-cert-stores?

This seems a useful feature, but also quite narrow. I assume it will work for SUSE, because you are part of that org, and it looks like it will work with Ubuntu. Your in-source comments imply that you think this will be widely applicable to the linux distros, because they will all arrange for /etc/ssl/certs to be linked?

It doesn't help people who aren't using a linux distro compiled node, which is a fair number of us, everyone using nvm for example.

It doesn't look like it will work on Windows or OS X, either, at least I don't see any openssl X509_LOOKUP_METHODs defined for them.

And it won't help anyone who is being forced to use a self-signed HTTPS proxy, but doesn't have access to change their system cert store.

Not that this one PR it has to solve everyone's problems! If its helpful for the linux distros, that's good.

@sam-github sam-github added the crypto label Oct 17, 2016
@AdamMajer
Contributor

On 14/10/16 12:45 PM, Sam Roberts wrote:

@sam-github commented on this pull request.


In src/node_crypto.cc
#8334 (review):

@@ -751,6 +751,23 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) {
CHECK_EQ(sc->ca_store_, nullptr);

if (!root_cert_store) {

+#if defined(NODE_SYSTEM_CERT_STORE)

  • // Assume OpenSSL is setup correctly, which is the case
  • // for distribution supplied versions.
  • //
  • // If this does not work, define SSL_CERT_DIR environment variable.
  • if (SSL_CTX_set_default_verify_paths(sc->ctx_)) {
  •  root_cert_store = SSL_CTX_get_cert_store(sc->ctx_);
    
  •  // root_cert_store created here is already assigned to the SSL_CTX
    
  •  // so when it is assigned again below, the reference is dropped by 1
    
  •  // and then we will delete root store along with the SSL_CTX deletion.
    
  •  // Increase references to 2 to avoid this scenario.
    
  •  CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
    
  • } else {
  •  // Failed to load, default to nothing.
    

This creates an undebuggable (by the user of node) situation. Is it
not possible to print some kind of error here? What are the possible
reasons for failure? non-existence of cert store? invalid format of
cert store? ...?

https://www.openssl.org/docs/man1.1.0/crypto/ERR_print_errors.html

Probably these functions could be used to print all errors in queue, but
I haven't tested this and how it would work with cert store failures.

  • Adam
@sam-github sam-github referenced this pull request Oct 27, 2016
Open

Make it build using openssl 1.1.0 #8491

0 of 2 tasks complete
@AdamMajer
Contributor

On 14/10/16 01:58 PM, Sam Roberts wrote:

@AdamMajer https://github.com/AdamMajer This doesn't literally use
the system's cert store, it uses the OpenSSL cert store, which should
be setup to link to the system's certs. On my laptop (Ubuntu), this is
arranged by keeping symlinks up to date from |/etc/ssl/certs/...| into
|/user/share/ca-certificates/mozilla/...|. For those wondering, the
location from which CA certs will be loaded with this PR is described
here:
https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_load_verify_locations.html,
note some overrides are possible using env vars.

Maybe the configure option could be more accurately named as
|--use-openssl-default-cert-stores|?

This seems a useful feature, but also quite narrow. I assume it will
work for SUSE, because you are part of that org, and it looks like it
will work with Ubuntu. Your in-source comments imply that you think
this will be widely applicable to the linux distros, because they will
all arrange for |/etc/ssl/certs| to be linked?

No quite.

The goal of this is to actually use default (or system, or however you
want to call it) certificate store. This default certificate store is
used by virtually ALL applications that use OpenSSL, except NodeJS
because NodeJS didn't even allow for system OpenSSL until recently.

Linux or BSD distributions that use OpenSSL or whatever distribution
that uses OpenSSL and maintains it, should setup these paths in the
OpenSSL already. There is no need to assume any hardcoded path when
using system OpenSSL.

The assumption is only for in-tree OpenSSL supplied by NodeJS, which
just happens to be the same as Debian which has the certificates
symlinked from /etc/ssl/certs. And this just happens to work for most
distributions including Debian, Ubuntu and OpenSUSE and probably others.

The purpose of the patch is just for NodeJS to use system default
certificates. As is, users cannot even add their own certificate stores
without this patch, so not very good situation at all.

It doesn't help people who aren't using a linux distro compiled node,
which is a fair number of us, everyone using |nvm| for example.

I wanted to have a simple, least disruptive change and then change that
later to maybe runtime option. But it seems to be stalled for whatever
reason.

It doesn't look like it will work on Windows or OS X, either, at least
I don't see any openssl |X509_LOOKUP_METHOD|s defined for them.

That problem is system specific and I think you could probably fix that
with OpenSSL patches to OpenSSL project.

And it won't help anyone who is being forced to use a self-signed
HTTPS proxy, but doesn't have access to change their system cert store

Why not? Copy the cert store links, add your own, and set OpenSSL cert
path environmental variable. Then Node will resolve your own certs in
your own cert path.

  • Adam
@sam-github
Member

OK, I'm +1 on this change in terms of function, but I'm not able to review or comment on the reference count changes, they look subtle, I've no idea if they are correct.

@indutny you still around? your thoughts?

@sam-github
Member

But it seems to be stalled for whatever reason.

Its not stalled, its just a bit slow. It will go faster if you respond to all comments.

@AdamMajer
Contributor

On 27/10/16 03:55 PM, Sam Roberts wrote:

OK, I'm +1 on this change in terms of function, but I'm not able to
review or comment on the reference count changes, they look subtle,
I've no idea if they are correct.

They are correct. You can look in OpenSSL's source code and triple
check. They only have that ref. count in only a few places so it is not
difficult to see what is happening.

@AdamMajer
Contributor

On 27/10/16 03:59 PM, Sam Roberts wrote:

@sam-github commented on this pull request.


In src/node_crypto.cc #8334:

@@ -751,6 +751,23 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) {
CHECK_EQ(sc->ca_store_, nullptr);

if (!root_cert_store) {

+#if defined(NODE_SYSTEM_CERT_STORE)

  • // Assume OpenSSL is setup correctly, which is the case
  • // for distribution supplied versions.
  • //
  • // If this does not work, define SSL_CERT_DIR environment variable.
  • if (SSL_CTX_set_default_verify_paths(sc->ctx_)) {
  •  root_cert_store = SSL_CTX_get_cert_store(sc->ctx_);
    
  •  // root_cert_store created here is already assigned to the SSL_CTX
    
  •  // so when it is assigned again below, the reference is dropped by 1
    
  •  // and then we will delete root store along with the SSL_CTX deletion.
    
  •  // Increase references to 2 to avoid this scenario.
    
  •  CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
    
  • } else {
  •  // Failed to load, default to nothing.
    

@AdamMajer https://github.com/AdamMajer I suggest emitting a
warning. I added code to do this in #9139
#9139, not too sure what to do
here, maybe you want to wait? Maybe pull in the code, and whichever PR
merges last deals with the conflicts?

I'm on vacation now so I don't want to be compiling Node and debugging
error conditions in SSL cert loading at the moment :) It would be nice
to have common error dump function for OpenSSL.

Per manual page for this function, error(s) will be in error stack for
OpenSSL. You can only dump them all. (I've commented on that in some
other comment already) But this can be done later. If certs are not
working, users can already test exact same store with openssl s_client. Also, for default stores with external OpenSSL, it means
OpenSSL will not work for almost any application, not just Node.

Once this patch is included, I would like to rip out the cert store from
the NodeJS binary and ship it externally. Then the decision on loading
paths can be done at runtime too. But that's for later and another patch.

I think this is very low risk as is, but in the future code can be added
to dump the OpenSSL error code stack.

  • Adam

PS. As for conflicts, there shouldn't really be any. Your change should
have little to do with my change as you want to append certificates to
the cert store in addition to the ones that are already there. So your
changes are outside the #if...#endif and you don't have to worry about
anything.

I'm assuming that OpenSSL handles this correctly irrespective of the
cert store type.

@AdamMajer
Contributor

On 27/10/16 04:02 PM, Sam Roberts wrote:

But it seems to be stalled for whatever reason.

Its not stalled, its just a bit slow. It will go faster if you respond
to all comments.

I think I did :) Sometimes twice.

@sam-github
Member

You have not addressed or even commented on https://github.com/nodejs/node/pull/8334/files#r85426392, once you look at it more closely (by all means, after your vacation), you will understand what I mean by conflicts. You also still didn't address my initial comment. If there are errors loading the cert store, node will silently fail to have any root certs, and fail to validate any certs, leading to mysterious and aggravating failures.

My comment on not being able to review the ref counting was not meant to imply your code is wrong, but that you'll need to find a crypto subsystem maintainer to LGTM them.

@AdamMajer
Contributor

On 27/10/16 09:26 PM, Sam Roberts wrote:

You have not addressed or even commented on
https://github.com/nodejs/node/pull/8334/files#r85426392, once you
look at it more closely (by all means, after your vacation), you will
understand what I mean by conflicts. You also still didn't address my
initial comment. If there are errors loading the cert store, node will
silently fail to have any root certs, and fail to validate any certs,
leading to mysterious and aggravating failures.

Actually I did. Just not under that comment because I use email reply
and combined replies,

https://github.com/nodejs/node/pull/8334#issuecomment-256792868

I wrote that it is very unlikely for that thing to fail in the first
place. I've also commented that you can debug it by running openssl s_client and that dumping OpenSSL's error stack can be done later.

That code is geared toward distributions and external OpenSSL libraries.
That it works with included OpenSSL library too (for most distros) is
great too.

Also, it is unlikely that this code will fail, because then things like
wget and curl would also fail to load their certificates ie. a
completely broken OpenSSL install.

I want to add error handling here at a later point, once the #if #endif
block is not needed because certificates are external to the NodeJS
binary. But like I wrote before, I wanted a simple patch that is already
used in Debian/Ubuntu (they have a slightly different one but
essentially same thing) and in SUSE and probably other distros and have
it upstreamed first. I don't want to make this into 5 or 10 commits with
more expansive changes in one pull request.

  • Adam
@sam-github
Member

OK, please add an in-source comment explaining that any failures indicate a damaged system CA store, which would effect the entire system, and doesn't need specific handling by node. As is, code that silently ignores errors with no comment is a problem.

I want to add error handling here at a later point, once the #if #endif block is not needed because certificates are external to the NodeJS binary.

Keep in mind that an important feature of Node.js is that once compiled, it can distributed to/installed on production machines by copying a single binary. That isn't the use-case of the distros which are integrating Node.js into a larger system, but breaking that capability, as removing CA certs sounds like it would, is going to be a very hard sell.

@ChALkeR ChALkeR added the security label Nov 3, 2016
@agl agl added a commit to agl/node that referenced this pull request Nov 8, 2016
@AdamMajer @agl AdamMajer + agl crypto: Use reference count to manage cert_store
Setting reference count at the time of setting cert_store instead of
trying to manage it by modifying internal states in destructor.

PR-URL: nodejs#8334
8dbe1aa
@AdamMajer
Contributor

Rewritten and updated. Now it is a runtime option with a build time option to pick the default runtime option.

@AdamMajer
Contributor

Ping? This has been in queue since August, in one form or another. In effect this is a 1-line patch - load bundled CA's or load default path.

configure
@@ -196,6 +196,11 @@ shared_optgroup.add_option('--shared-openssl-libpath',
dest='shared_openssl_libpath',
help='a directory to search for the shared OpenSSL DLLs')
+shared_optgroup.add_option('--use-def-ca-store',
@sam-github
sam-github Dec 12, 2016 Member

Why is this in the shared libraries section of the configuration? It should be up with the other openssl options.

src/node.cc
@@ -3585,6 +3593,8 @@ static void PrintHelp() {
" --v8-pool-size=num set v8's thread pool size\n"
#if HAVE_OPENSSL
" --tls-cipher-list=val use an alternative default TLS cipher list\n"
+ " --ssl-[no-]def-store use only default certificate store\n"
@sam-github
sam-github Dec 12, 2016 Member

The default certificate store is the bundled one. Something like this would make more sense:

--use-ssl-ca-store    use the system's OpenSSL CA certificate store instead of the compiled in Mozilla roots
@AdamMajer
AdamMajer Dec 19, 2016 Contributor

This probably now needs 2 options to be clear.

--use-openssl-ca         use OpenSSL's default CA store instead of bundled one
--use-bundled-ca        use Node's bundled Mozilla CA store

I could add (default) string to one or the other based on the #define, but then the string could be a little messy. Acceptable? What do you think?

@sam-github
Member

commit message typo:

a compile-time option to this CA

"to use this CA"

@sam-github

There are no doc changes, even though the docs describe node's bundled CA roots. Perhaps this doesn't matter, because those docs aren't packaged by distros, and distros don't care that the nodejs.org docs don't quite apply to them?

The CLI option should be in doc/node.1.

configure
+shared_optgroup.add_option('--use-def-ca-store',
+ action='store_true',
+ dest='use_def_ca_store',
+ help='use system supplied Root CA store instead of bundled copy')
@sam-github
sam-github Dec 12, 2016 Member

This only uses a "system" CA store on Linux, one Windows, the system CA store isn't used. The name of the option should more clearly identify it as using the OpenSSL Root CA store, so people can find the docs on the location and configuration of the OpenSSL CA store.

@nikitakit nikitakit referenced this pull request in nteract/hydrogen Dec 12, 2016
Closed

Remote kernel + token #473

@AdamMajer
Contributor
@AdamMajer
Contributor

Hopefully not too many typos. I've changed the manual page and the markdown file. Not sure if documentation should be separate commit or not, but for now in one commit.

doc/api/cli.md
+
+Use OpenSSL's default CA store or use bundled Mozilla CA store as supplied by
+current NodeJS version. Using OpenSSL store allows external modifications
+while bundled store is fixed at compile time. See `SSL_CERT_DIR` and
@sam-github
sam-github Dec 19, 2016 Member

"while bundled store" -> ", the bundled store" -- as-is, the text can be misunderstood to mean that the ossl store can be modified only if the bundled store is fixed. I suggest adding some text to the effect that the ossl store exists on most Linux distributions but may not exist elsewhere, and the certificates in it are maintained and decided by the distribution packagers unless explicitly reconfigured by the system admin, and also that the bundled store is identical for all Node.js binaries across all platforms.

configure
+parser.add_option('--openssl-use-def-ca-store',
+ action='store_true',
+ dest='use_openssl_ca_store',
+ help='use OpenSSL supplied Root CA store instead of bundled copy')
@sam-github
sam-github Dec 19, 2016 Member

There is lots of inconsistency in the capitalization and formatting of the configure messages, but it looks like the style that is most readable would be:

Use OpenSSL supplied Root CA store instead of compiled-in Mozilla CA store.

^--- Makes it a capitalized sentence, and makes it clear that the "bundled copy" is not a copy of the "OpenSSL supplied Root CA store".

@sam-github
Member

Docs in code in the same commit is house style, what you did works perfectly.

This looks really close to landable to me, only a couple comments on the doc text.

@AdamMajer
Contributor
@sam-github
Member

@indutny @jasnell some more commits landed since you LGTMed, specifically the run-time CLI option to use the openssl CA store. Can you two please give another thumbs up before I land this?

AdamMajer added some commits Dec 21, 2016
@AdamMajer AdamMajer crypto: do not use pointers to std::vector
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.
0aa8ac7
@AdamMajer AdamMajer crypto: Use system CAs instead of using bundled ones
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
d1e0228
@AdamMajer
Contributor
AdamMajer commented Dec 21, 2016 edited

Resolved merge conflict and changed comment in doc/api/cli.md since 7.3.0 was tagged already and fixed formatting since alignment of PrintHelp() changed by few spaces.

@AdamMajer
Contributor

@indutny @jasnell @sam-github also, someone probably needs to remove the dont-land-on-v7.x label that got automatically added by the bot.

@gibfahn
Member
gibfahn commented Dec 28, 2016

Label removed. While we're at it we should discuss backporting, should this be backported to v6/v4?

@AdamMajer
Contributor

It probably should depend if related changes to crypto are backported. In my opinion, if #9409 gets backported, then so should this change.

@AdamMajer
Contributor

So, any reason why this is still pending?

doc/api/cli.md
@@ -257,6 +257,24 @@ Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with
`./configure --openssl-fips`.
+### `--use-openssl-ca`, `--use-bundled-ca`
+<!-- YAML
+added: v7.3.1
@addaleax
addaleax Jan 19, 2017 Member

Can you use added: REPLACEME for features that are not released yet?

@AdamMajer
AdamMajer Jan 19, 2017 Contributor

done. now it needs the bot dont-land label removed.

@addaleax
Member

Should this be labelled semver-minor?

@AdamMajer
Contributor

About semver-minor, there is no functionality changes. The change is a no-op for default compilation and usage. It only changes CA store source when enabled explicitly at compile time and/or run time.

+If `--use-openssl-ca` is enabled, this overrides and sets OpenSSL's directory
+containing trusted certificates.
+
+### `SSL_CERT_FILE=file`
@jasnell
jasnell Jan 19, 2017 Member

One concern that I have with the use of the environment variables for this is that it is easily possible for a rogue module to overwrite the value for child processes. Obviously rogue modules running untrusted code is a completely different problem in and of itself but I'm not convinced that allowing the trust root to be modified in this way is something we should allow. I'm good with the command-line arguments because those are much more explicit.

@sam-github
sam-github Jan 19, 2017 Member

@jasnell this is out of our control, the env vars come as part of the OpenSSl trust store feature. This env var is implemented by OpenSSL in https://github.com/nodejs/node/blob/master/deps/openssl/openssl/crypto/x509/by_file.c#L100, which also implies its an env var supported by pretty much every program that uses OpenSSL, unless they have their own built-in trust store (Node is unusual in this respect).

@AdamMajer
AdamMajer Jan 20, 2017 Contributor

@jasnell basically what @sam-github says, this is just documenting OpenSSL's environment usage.

@jasnell
jasnell Jan 23, 2017 Member

Yes, but this env var currently has no effect in Node.js, correct? This PR makes it so that this env var can be used? At the very least, we should document the potential risk of using it and point out that changing the value in process.env would have an impact on child processes.

@sam-github
sam-github Jan 23, 2017 edited Member

(EDIT: sorry, I mistyped that). What is the potential risk of allowing the user to decide for themselves what CAs they trust? Should we document the potential risk of having the certs compiled in... where its impossible for them to be changed if your sec policy requires it, such as on CA compromise?

To emphasize that the behaviour may be inherited, perhaps a note such as below would be OK?

Note: Be aware that unless the child environment is explicitly set, this evironment variable will be inherited by any child processes, and if they use OpenSSL, it may cause them to trust the same CAs as node.

@AdamMajer
AdamMajer Jan 23, 2017 Contributor

Do we have the same warnings about PATH and LD_LIBRARY_PATH and everything else?

@sam-github
sam-github Jan 23, 2017 Member

I guess the specific concern here from @jasnell is that people may not realize that these env vars are meaningful to non-node programs.

@jasnell
jasnell Jan 24, 2017 Member

The risk is more that a user may not know that process.env changes may propagate to child processes and that potentially untrusted code could make such changes.

@AdamMajer
AdamMajer Jan 24, 2017 Contributor

I don't understand these concerns at all. These env. variables already apply for non-node programs and will continue to apply. This change does nothing for that. And warning "unauthorized code can affect child processes"??

@sam-github
sam-github Jan 24, 2017 Member

I know these vars are from OpenSSL, not just node, so do you, but our average user won't, so lets warn. Its easier to modify the PR to make everyone happy than to stall it longer. My suggested text doesn't include the phrase you quote.

@jasnell
jasnell Jan 24, 2017 Member

This is better and it's good enough for me to land.

@AdamMajer ... as @sam-github mentions, the issue is about making sure the average user knows what the impact of the options are, even if they are not already familiar with OpenSSL.

@sam-github
sam-github Jan 24, 2017 Member

@AdamMajer Do you want to add the text, so I can land right away? I'm willing to add the text while landing, too, but since the commit will have your name on it I'd like an OK from you before adding anything.

@sam-github
Member

Yes, its semver-minor, because it adds a feature.

@AdamMajer
Contributor
@sam-github
Member

Thanks, and I know they are rarely used.

@sam-github sam-github added a commit that referenced this pull request Jan 24, 2017
@AdamMajer @sam-github AdamMajer + sam-github crypto: do not use pointers to std::vector
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: #8334
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
3ada5ae
@sam-github sam-github added a commit that referenced this pull request Jan 24, 2017
@AdamMajer @sam-github AdamMajer + sam-github crypto: Use system CAs instead of using bundled ones
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: #3159
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>
3aa17e4
@sam-github sam-github added a commit that referenced this pull request Jan 24, 2017
@AdamMajer @sam-github AdamMajer + sam-github crypto: ability to select cert store at runtime
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>
33012e9
@sam-github
Member

Landed in 3ada5ae, 3aa17e4, 33012e9

@sam-github sam-github closed this Jan 24, 2017
@sam-github
Member

Thanks, Adam.

@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: do not use pointers to std::vector
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>
41dd5a6
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: Use system CAs instead of using bundled ones
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>
9956612
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: do not use pointers to std::vector
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>
5f51208
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: Use system CAs instead of using bundled ones
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>
94fce57
@targos targos added a commit that referenced this pull request Jan 28, 2017
@AdamMajer @targos AdamMajer + targos crypto: ability to select cert store at runtime
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>
43f4951
@italoacasas italoacasas referenced this pull request Jan 29, 2017
Merged

v7.5.0 proposal #11062

@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: do not use pointers to std::vector
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>
ac2b059
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: Use system CAs instead of using bundled ones
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>
aeea13b
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: ability to select cert store at runtime
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>
a1897c1
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: do not use pointers to std::vector
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>
8d6a087
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: Use system CAs instead of using bundled ones
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>
388b6d3
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@AdamMajer @italoacasas AdamMajer + italoacasas crypto: ability to select cert store at runtime
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>
3dc93e9
@evanlucas evanlucas added a commit that referenced this pull request Jan 31, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
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
42b4cee
@evanlucas evanlucas added a commit that referenced this pull request Jan 31, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
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
a34f1d6
@evanlucas evanlucas added a commit that referenced this pull request Feb 1, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
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
a1c91ec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment