crypto: allow adding extra certs to well-known CAs

[sam-github]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

crypto,tls,https

Description of change

In closed environments, self-signed or privately signed certificates are
commonly used, and rejected by Node.js since their root CAs are not
well-known. Allow extending the set of well-known compiled-in CAs via
environment, so they can be set as a matter of policy.

Note that #8334 addresses a similar but not identical use-case, and works better for those (like linux distributions) that are willing to recompile Node.js to use OpenSSL's default certificate store. #8334
doesn't address those who cannot recompile, do not have access to the system certificate store, or
who are are on a system where the default certificate store is not exposed as an OpenSSL compatible
store (for example, OS X and Windows).

See #3159, #8334

Partially fixes #4175 (particularly #4175 (comment)), and may also address microsoft/tfs-cli#118 and apigee/microgateway-core#9 without forcing them to recompile node.

[mscdex]

This function can be passed as a second argument to listen() to make things shorter.

[mscdex]

Also, you might wrap it in a common.mustCall() to be extra safe.

[mscdex]

It would be better to instead set these variables in the config object passed to fork():

const env = {
  CHILD: 'yes',
  PORT: this.address().port,
  NODE_EXTRA_CA_CERTS: common.fixturesDir + '/keys/ca1-cert.pem'
};
fork(__filename, { env: env }).on('exit', common.mustCall(function(status) {

[mscdex]

This should be indented more to align with the other function arguments.

[mscdex]

&& should be at end of the previous line.

[mscdex]

This should be indented more to align with the other function arguments.

[mscdex]

There's an extra space between 'the' and 'well'.

[mscdex]

There's an extra space before 'A message'.

[mscdex]

I think the fprintf()s should be replaced with a call to process.emitWarning(). I'm not sure if there is already something in node internals that only emits a warning once that could be reused.

[bnoordhuis]

Typo: extra

[sam-github]

I'll look around to see if I can call process.emitWarning() from C++.

Note that there is a pre-existing fprintf(stderr, ... statement at

node/src/node_crypto.cc

Line 5788 in 7014566

fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(err, NULL));

, should it be emitWarning, too?

[mscdex]

@sam-github No, because that's a more fatal error and the process will abort after it prints that message.

[sam-github]

@mscdex @bnoordhuis Now calling process.emitWarning(), PTAL

Also, a design detail: currently, the value of the NODE_EXTRA_CA_CERTS is retrieved from env during node startup. So, this doesn't work as maybe someone would hope:

process.env.NODE_EXTRA_CA_CERTS = "cacerts.pem";
require('tls').createServer({}) // ... no extra are loaded

It would be possible to call secure_getenv() during the root store initialization to enable the above usage.

But, I think how it is now is consistent with general node practice, and that anyone who can edit the program source could also just provide the extra CA certs themselves via the options.

Agreed?

[sam-github]

If the call to emitWarning() from c++ is OK, I will update the docs and add a unit test.

[mscdex]

Is this necessary? What about just calling server.close() inside the server's connection handler?

[sam-github]

I don't really mind, though I don't see how that is preferable. unref allows node to close gracefully when it has nothing else to do, closing would also have the same effect, why is it better?

[mscdex]

It might be better to use MakeCallback() here instead of calling it directly.

[mhdawson]

@mscdex, for my benefit when would you suggest MakeCallback instead of Call or vice versa. I see that existing code has a mix of both.

[mscdex]

I assume MakeCallback() should almost always be used because of domains/async_hooks and such?

[mhdawson]

I did find this more detailed discussion on the topic from a while back: nodejs/nan#284. I think the net is that unless you have a specific reason that you don't want process._tickCallback() to be called, then MakeCallback() is preferred over the direct Call.

[sam-github]

@mhdawson 's link above says

The latter (MakeCallback) case calls the JS callback function, but then also ends the current tick, calling process._tickCallback() and any other callbacks registered via nextTick(). This form is intended only for use by C++ code that was dispatched from the libuv event loop, and not for C++ code that was invoked from JS code.

It sounds like MakeCallback() has lots of side effects, are you sure its right? My usage seems to fit the "not C++ invoked from the event loop" case.

It occured to me that process.emitWarning() could actually emit... which would call user code, but I checked, and it emits in the next tick, so isn't going to hit any user-supplied js.

I don't know enough about the distinction to choose, I'll need some guidance here.

[mhdawson]

@bnoordhuis do you have an opinion on Call/MakeCallback in this instance ?

[sam-github]

@trevnorris I think that MakeCallback can cause running microtasks as a side-effect (

node/src/node.cc

Line 1268 in 09d41e2

env->isolate()->RunMicrotasks();

), which would not be desireable, but I'm not sure how to demonstrate this. Trevor, you know this code, should usage like above use direct Call(), or node::MakeCallback()?

My failed attempt at making a "microtask" :-( at https://gist.github.com/927a451526de55847d53694b53b01a20

[mhdawson]

LGTM subject to fixing up @mscdex remaining comments.

[sam-github]

conversation in #9139 (comment) looks outdated because I added MakeCallback(), but it is not outdated. /to @bnoordhuis @trevnorris

[bnoordhuis]

Should be static.

[sam-github]

done, note that root_certs a few lines above is not static, but appears that it could be (I was mirroring its definition). root_cert_store below is explicitly declared extern, it shouldn't be static.

[bnoordhuis]

EmitWarning

[bnoordhuis]

Don't leave dead code in.

[sam-github]

Its not dead code. I am asking for confirmation that MakeCallbacks() is the right thing to do, I believe the #if 0 branch is the correct one, and that MakeCallbacks() is designed for callbacks from uv, and other async usage, not for this kind of direct js-to-c++-to-js usage. That it is leaking handles and running microtasks make it seem the wrong tool. @trevnorris @bnoordhuis Shouldn't this be a simple Call()?

[bnoordhuis]

Call() interacts poorly with domains and other mechanisms that hang off async_hooks so it's really only safe for internal JS code. W.r.t. handle leaks, Call does that as well (the return value.)

[sam-github]

@bnoordhuis what is "internal JS code"? In this case I am calling

node/lib/internal/process/warning.js

Lines 28 to 49 in 6845d6e

	process.emitWarning = function(warning, name, ctor) {
	if (typeof name === 'function') {
	ctor = name;
	name = 'Warning';
	}
	if (warning === undefined || typeof warning === 'string') {
	warning = new Error(warning);
	warning.name = name || 'Warning';
	Error.captureStackTrace(warning, ctor || process.emitWarning);
	}
	if (!(warning instanceof Error)) {
	throw new TypeError('\'warning\' must be an Error object or string.');
	}
	if (warning.name === 'DeprecationWarning') {
	if (process.noDeprecation)
	return;
	if (process.throwDeprecation)
	throw warning;
	}
	process.nextTick(() => process.emit('warning', warning));
	};
	}

, which is internal. It does nextTick a process.emit(), which can call user-code. Is that the difference here? That user code (process.on('warning', user....) was triggered from within createServer(), so we need to use MakeCallback() to keep the accounting correct for domains, etc.?

Otherwise, this doesn't seem so different from url.js, which calls into c++ to do some work, and synchronously calls back with ->Call() to internal js code. Or node_http_parser, which uses Call() to call pushValueToArray(), a js function from lib/internal/bootstrap_node.js

I (and @mhdawson , perhaps others), would like to understand the basic principle here, so I correctly distinguish between the two in the future.

Also, with MakeCallback():

require("tls").createServer()
  // -> MakeCallback
  //   -> process.emitWarning()
  //   -> env->isolate()->RunMicrotasks()
  //   -> env->tick_callback_function()->Call()
  // return from createServer()

How would I demonstrate that createServer(), when it uses MakeCallback(), is causing micro tasks and ticks to get called? Or do they not? I'm trying to see what the side-effects are, but couldn't prove there were any with https://gist.github.com/sam-github/927a451526de55847d53694b53b01a20 (I was looking to cause output between the BEGIN and END, which should not happen).

I addressed your other comments, thanks, and I will fix the handle leak, looking into that now.

[bnoordhuis]

Oh, that's a good point; I didn't realize emitWarning() defers to the next tick. In that case Call() should be fine.

[bnoordhuis]

You can leave out the strlen().

[bnoordhuis]

Oh, and you could write this as:

Local<Value> arg = node::OneByteString(env->isolate(), warning);
MakeCallback(env, process, "emitWarning", 1, &arg);

It's a little more succinct but whatever you prefer.

[sam-github]

I did used the array form because I had the impression it was more idiomatic, but I simplified the code as you suggest.

[bnoordhuis]

This function needs a HandleScope, it's leaking handles into the scope of the caller.

[bnoordhuis]

Arguments should line up here and below.

[bnoordhuis]

Style: BIO* bio = ...

[bnoordhuis]

This function could share code with SSL_CTX_use_certificate_chain. Also, you could add a MarkPopErrorOnReturn mark_pop_error_on_return at the top so you don't have to manually juggle the error stack.

[bnoordhuis]

You could use sc->env() here instead.

[bnoordhuis]

It's kind of odd that this is a (static) method but LoadExtraCaCerts() is not. Neither manipulate SecureContext state so they probably shouldn't be methods, static or not.

[bnoordhuis]

I have a feeling we can move much of the logic to JS land if we simply exposed the root CA list somewhere.