crypto: allow adding extra certs to well-known CAs #9139

sam-github · 2016-10-17T19:35:25Z

mscdex · 2016-10-17T21:17:44Z

test/parallel/test-tls-env-extra-ca.js

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

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

mscdex · 2016-10-17T21:20:45Z

test/parallel/test-tls-env-extra-ca.js

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 · 2016-10-17T21:23:03Z

src/node_crypto.cc

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

mscdex · 2016-10-17T21:23:28Z

src/node_crypto.cc

&& should be at end of the previous line.

mscdex · 2016-10-17T21:23:53Z

src/node_crypto.cc

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

mscdex · 2016-10-17T21:24:37Z

doc/api/cli.md

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

mscdex · 2016-10-17T21:24:58Z

doc/api/cli.md

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

mscdex · 2016-10-17T21:29:47Z

bnoordhuis · 2016-10-18T14:09:09Z

doc/api/cli.md

Typo: extra

sam-github · 2016-10-18T15:33:00Z

mscdex · 2016-10-18T15:36:26Z

sam-github · 2016-10-18T20:31:14Z

sam-github · 2016-10-18T20:32:31Z

mscdex · 2016-10-18T20:38:42Z

test/parallel/test-tls-env-extra-ca.js

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

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 · 2016-10-18T20:43:47Z

src/node_crypto.cc

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

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

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

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.

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

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

@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 · 2016-10-18T21:56:02Z

sam-github · 2016-10-19T18:22:15Z

bnoordhuis · 2016-10-20T11:10:34Z

src/node_crypto.cc

Should be static.

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 · 2016-10-20T11:11:05Z

src/node_crypto.cc

EmitWarning

bnoordhuis · 2016-10-20T11:11:21Z

src/node_crypto.cc

Don't leave dead code in.

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

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

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

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

bnoordhuis · 2016-10-20T11:12:24Z

src/node_crypto.cc

You can leave out the strlen().

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.

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

bnoordhuis · 2016-10-20T11:17:46Z

src/node_crypto.cc

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

bnoordhuis · 2016-10-20T11:18:01Z

src/node_crypto.cc

Arguments should line up here and below.

bnoordhuis · 2016-10-20T11:18:58Z

src/node_crypto.cc

Style: BIO* bio = ...

bnoordhuis · 2016-10-20T11:23:20Z

src/node_crypto.cc

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 · 2016-10-20T11:26:44Z

src/node_crypto.cc

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

bnoordhuis · 2016-10-20T11:31:06Z

src/n 8000 ode_crypto.h

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 · 2016-10-20T11:38:27Z

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 17, 2016

sam-github added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. https Issues or PRs related to the https subsystem. labels Oct 17, 2016