tls: load NODE_EXTRA_CA_CERTS at startup

[oyyd]

NODE_EXTRA_CA_CERTS is not intended to be used to set the paths of extra certificates and this approach to setting is not reliable. This commit makes node load extra certificates at startup rather than on first use.

Fixes: #20434
Refs: #20432

Checklist

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

[sam-github]

This makes the env var behave as intended and described. One suggested change.

[sam-github]

I find the text confusing here, I'm not sure what is meant by "setting paths". Rather than rewriting though, I suggest removing the entire doc change, both sentences. Its unnecessary, env variables are intended to be set outside node, not by node to configure itself at runtime.

[oyyd]

Right. My explanation here makes the doc even confusing.

[oyyd]

@sam-github @bnoordhuis PTAL

[sam-github]

One suggestion: I think this can be called unconditionally. certLoadExtraRootCertsFile is a null op if extra_root_certs_file hasn't been set, and it is only set if this env var is present, so this is a non-obvious optimization. I think its a bit fragile, if a CLI flag was introduced to set the extra_root_certs_file, for example, then this code would not execute, and the bug you are fixing would resurface for that use case.

[bnoordhuis]

Another potential footgun: the C++ code does suid root checks (calls SafeGetenv("NODE_EXTRA_CA_CERTS")); process.env does not.

[oyyd]

I have removed the conditional statement as @sam-github suggested. After the removing, we don't use process.env.NODE_EXTRA_CA_CERTS in js land which avoids the potential footgun @bnoordhuis described.

[bnoordhuis]

This breaks ./configure --without-ssl builds. Not the require statement itself but the internalBinding('crypto') statement in the included file, it'll throw an exception.

[sam-github]

Would making this conditional on Boolean(process.versions.openssl) as test/common/util.js does for hasCrypto be OK, or is there a better way?

[oyyd]

I have placed this line in if (process.versions.openssl) {.

... or is there a better way?

I think it's ok as lib/internal/util also uses process.versions.openssl to assert crypto:

node/lib/internal/util.js

Line 18 in 972d0be

const noCrypto = !process.versions.openssl;

[oyyd]

And a new test has been added to ensure that it won't throw without ssl.

[bnoordhuis]

Another potential footgun: the C++ code does suid root checks (calls SafeGetenv("NODE_EXTRA_CA_CERTS")); process.env does not.

[addaleax]

Is this a semver-major change? It seems like it could be?

[addaleax]

You could just access the binding directly here if you like

[oyyd]

I assumed there are codes other than just calling functions from binding before.

Accessing the binding directly is better as it could indicate why if (process.versions.openssl) is necessary to other contributors.

[addaleax]

Ditto about accessing the binding directly (again, only if you like)

[oyyd]

@addaleax Yes. This might break something.

[thefourtheye]

Why the comma is introduced?

[oyyd]

It's unintentionally introduced(my personal code style :-)). I'm going to restore this line.

[thefourtheye]

Shouldn't this condition be split into two, like in the current code? If root_cert_store is already created and extra_root_certs_file is not empty, we should still add them, right?

[oyyd]

If root_cert_store is already created and extra_root_certs_file is not empty, we should still add them, right?

No, it seems the original codes won't set root_cert_store again if it's available. Please remind me if I mistake something.

[thefourtheye]

You are correct. I misread the code. But I still have one question. As per old code if the root_cert_store is not there yet, a new instance of NewRootCertStore is created and assigned to it. Shouldn't the new code also do the same?

[oyyd]

As per old code if the root_cert_store is not there yet, a new instance of NewRootCertStore is created and assigned to it.

Yes. One of the reasons is that I use root_cert_store to indicate whether the certs have been loaded in the flowing test.

But I think you are right. I'm going to modify the code and make the new code do the same.

[oyyd]

@thefourtheye PTAL

[thefourtheye]

Nit: The newly exposed functions' names sound a little wordy.

[thefourtheye]

Nit: Maybe we can just run this test only when crypto is not there, just to emphasize?

[oyyd]

It seems that we don't run tests with --without-ssl build. I would like and change the comment a bit and keep the test.

[bnoordhuis]

Left some comments but mostly LGTM.

[bnoordhuis]

Style nit: 4 space indent (line continuation.)

[bnoordhuis]

Style: root_cert_store == nullptr

(I know older code says !root_cert_store but new code shouldn't.)

[bnoordhuis]

Style: can you line up the arguments?

[bnoordhuis]

The certIsExtra... name is kind of misleading; the other cert... methods are for SPKAC.

[bnoordhuis]

--expose-internals isn't necessary, is it?

[bnoordhuis]

Maybe clarify that this test is written for ./configure --without-ssl builds. (That's right, isn't it?

[oyyd]

@bnoordhuis Thanks for your comments. PTAL.

[jasnell]

Could this possibly be injected via bootstrapper.cc instead? Especially since it's only used here.

[oyyd]

Now, it's removed to UseExtraCaCerts.

[refack]

Left some questions, and requests.

[refack]

Avoid globals. If this is per environment, it should be a member property of node::Environment. If it's actually a per-process global, IMHO it should be grouped into a struct with root_cert_store and extra_root_certs_file

[oyyd]

Grouping root_cert_store will make the code a bit messy. And as the extra_root_certs_file has been removed, I guess it's okay now.

[refack]

Since there is no test IMHO the description should be:

... won't throw even when there is no... Hence we don't test `common.hasCrypto`
                ^^^^

[oyyd]

Thanks. I guess I made this test too confusing.

[refack]

IMHO this will be more readable is it was:

if {
} else if {
} else if {
} else {
  assert.fail("Bad child process invocation")
}

Having a if (process.argv[2] !== 'child') guard remove the need to do return in each clause

[refack]

This should be outside of the scope that start in L9

[refack]

What is the difference between this and the one before?
IF they can be merged, please do, else please add a comment.

[refack]

Is there a requirement to call this from JS? If not this should be called during Environment::Start, or even in

node/src/node.cc

Lines 2972 to 2975 in cf3f8dd

	std::string extra_ca_certs;
	if (SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
	crypto::UseExtraCaCerts(extra_ca_certs);
	}

https://github.com/nodejs/node/blob/bcef787dce0a380e3ce66936e72c49176de67838/src/node_crypto.cc#L835-L837

[oyyd]

Loading extra ca certs will emit a warning when the certs files are not valid:

node/src/node_crypto.cc

Line 881 in 05394d2

ProcessEmitWarning(sc->env(),

And as the Environment and the js runtime(this name maybe not correct) is not setup yet while calling UseExtraCaCerts, we can't move the calling here.

I would like to move LoadExtraRootCertsFile() to bootstrapper.cc as @jasnell suggested, how do you think about it?

[refack]

Since IIUC this is sort of a command line error, we do have precident of printing the error directly:

node/src/node.cc

Lines 2434 to 2444 in cf3f8dd

	if (!errors.empty()) {
	for (auto const& error : errors) {
	fprintf(stderr, "%s: %s\n", args->at(0).c_str(), error.c_str());
	}
	exit(9);
	}
	if (per_process_opts->print_version) {
	printf("%s\n", NODE_VERSION);
	exit(0);
	}

node/src/node.cc

Lines 354 to 359 in cf3f8dd

	void StartTracingAgent() {
	if (!trace_enabled_categories.empty()) {
	fprintf(stderr, "Node compiled with NODE_USE_V8_PLATFORM=0, "
	"so event tracing is not available.\n");
	}
	}

node/src/node.cc

Lines 382 to 415 in cf3f8dd

	void PrintErrorString(const char* format, ...) {
	va_list ap;
	va_start(ap, format);
	#ifdef _WIN32
	HANDLE stderr_handle = GetStdHandle(STD_ERROR_HANDLE);
	// Check if stderr is something other than a tty/console
	if (stderr_handle == INVALID_HANDLE_VALUE ||
	stderr_handle == nullptr ||
	uv_guess_handle(_fileno(stderr)) != UV_TTY) {
	vfprintf(stderr, format, ap);
	va_end(ap);
	return;
	}
	// Fill in any placeholders
	int n = _vscprintf(format, ap);
	std::vector<char> out(n + 1);
	vsprintf(out.data(), format, ap);
	// Get required wide buffer size
	n = MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, nullptr, 0);
	std::vector<wchar_t> wbuf(n);
	MultiByteToWideChar(CP_UTF8, 0, out.data(), -1, wbuf.data(), n);
	// Don't include the null character in the output
	CHECK_GT(n, 0);
	WriteConsoleW(stderr_handle, wbuf.data(), n - 1, nullptr, nullptr);
	#else
	vfprintf(stderr, format, ap);
	#endif
	va_end(ap);
	}

---

Alternatively if you add this to Environment::Start, you could use this to setup the warning:
for example after:

node/src/env.cc

Line 278 in cf3f8dd

SetupProcessObject(this, args, exec_args);

[oyyd]

we do have precident of printing the error directly

Then I think moving the calling into the function body of UseExtraCaCerts is better. Let me take a try on my mac(as I remember there is a test to check the warning message.).

[refack]

Hello @oyyd, thank you for this fix.
I left some questions, and points that might need changes. I don't have the full context of the review up till now, so if I'm suggesting something that negates a previous comment, sorry.

[oyyd]

Hi @refack, feel free to leave more comments :-). I'm going to check them later.

[refack]

Can we replace this new global boolean with following logic:

  if (!extra_root_certs_file.empty()) {
    ...
    if (err) {
      ...
      extra_root_certs_file = nullptr;
    }
  }
...
  static void IsExtraRootCertsFileLoaded(const FunctionCallbackInfo<Value>& args) {
    return args.GetReturnValue().Set(extra_root_certs_file != nullptr);
  }

[oyyd]

The compiler complained about the extra_root_certs_file != nullptr. (Otherwise, I think it should be a better choice.)

[oyyd]

@refack Thanks. Please take another look.

[richardlau]

The new test needs to extend env rather than replace it for the child processes otherwise it is clearing LD_LIBRARY_PATH which is why the shared libraries are failing to load.

[refack]

The new test needs to extend env rather than replace it for the child processes otherwise it is clearing LD_LIBRARY_PATH which is why the shared libraries are failing to load.

Ohh, that's a common pitfall.

[refack]

Suggested change

	{ env: { NODE_EXTRA_CA_CERTS } },
	{env: Object.assign({}, process.env, { NODE_EXTRA_CA_CERTS })},

[refack]

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

[oyyd]

The failed travis ci test is unrelated. Now it's ready.