Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: tls.rootCertificates empty #32229

Closed
AdamMajer opened this issue Mar 12, 2020 · 10 comments
Closed

crypto: tls.rootCertificates empty #32229

AdamMajer opened this issue Mar 12, 2020 · 10 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@AdamMajer
Copy link
Contributor

AdamMajer commented Mar 12, 2020

  • Version: 13.10.1
  • Platform: openSUSE Tumbleweed
  • Subsystem: crypto

For environments that use system CA store by configuring with --openssl-use-def-ca-store --shared-openssl

What steps will reproduce the bug?

> tls.rootCertificates.length
0

After reverting commit 091444a

> tls.rootCertificates.length
137

So this is a regression after fixing #32074

@AdamMajer
Copy link
Contributor Author

AdamMajer commented Mar 12, 2020

Furthermore, the build is failing with the tests that were added in the commit.

https://build.opensuse.org/package/show/devel:languages:nodejs/nodejs13

[  116s] not ok 2204 parallel/test-tls-root-certificates
[  116s]   ---
[  116s]   duration_ms: 1.27
[  116s]   severity: fail
[  116s]   exitcode: 1
[  116s]   stack: |-
[  116s]     assert.js:101
[  116s]       throw new AssertionError(obj);
[  116s]       ^
[  116s]     
[  116s]     AssertionError [ERR_ASSERTION]: Missing expected exception.
[  116s]         at Object.<anonymous> (/home/abuild/rpmbuild/BUILD/node-v13.11.0/test/parallel/test-tls-root-certificates.js:31:10)
[  116s]         at Module._compile (internal/modules/cjs/loader.js:1147:30)
[  116s]         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1167:10)
[  116s]         at Module.load (internal/modules/cjs/loader.js:996:32)
[  116s]         at Function.Module._load (internal/modules/cjs/loader.js:896:14)
[  116s]         at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
[  116s]         at internal/main/run_main_module.js:17:47 {
[  116s]       generatedMessage: false,
[  116s]       code: 'ERR_ASSERTION',
[  116s]       actual: undefined,
[  116s]       expected: /TypeError/,
[  116s]       operator: 'throws'
[  116s]     }
[  116s]     assert.js:101
[  116s]       throw new AssertionError(obj);
[  116s]       ^
[  116s]     
[  116s]     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
[  116s]     
[  116s]     1 !== 0
[  116s]     
[  116s]         at ChildProcess.<anonymous> (/home/abuild/rpmbuild/BUILD/node-v13.11.0/test/parallel/test-tls-root-certificates.js:19:12)
[  116s]         at ChildProcess.<anonymous> (/home/abuild/rpmbuild/BUILD/node-v13.11.0/test/common/index.js:362:15)
[  116s]         at ChildProcess.emit (events.js:315:20)
[  116s]         at Process.ChildProcess._handle.onexit (internal/child_process.js:275:12) {
[  116s]       generatedMessage: true,
[  116s]       code: 'ERR_ASSERTION',
[  116s]       actual: 1,
[  116s]       expected: 0,
[  116s]       operator: 'strictEqual'
[  116s]     }
[  116s]   ...

@jasnell jasnell added the crypto Issues and PRs related to the crypto subsystem. label Mar 12, 2020
@jasnell
Copy link
Member

jasnell commented Mar 12, 2020

/cc @nodejs/crypto @addaleax @bnoordhuis @ebickle

@ebickle
Copy link
Contributor

ebickle commented Mar 12, 2020

Will take a look. Has the original commit been reverted or are you looking for an additional commit/fix over top?

The issue likely relates to the behavior of X509_STORE_set_default_paths called by NewRootCertStore() in node_crypto.cc. OpenSSL documentation states it loads certificates into the X509_STORE but that might not be the case, or they might not be accessible through the other X509_STORE functions.

@AdamMajer
Copy link
Contributor Author

I've reverted it for testing purposes. Of course what we need is fix the regression and also fix the original issue.

@ebickle
Copy link
Contributor

ebickle commented Mar 12, 2020

I've narrowed the issue down to the following:

  1. When per_process::cli_options->ssl_openssl_cert_store is true, X509_STORE_set_default_paths(store) is called instead of adding the hardcoded node.js certificates.
  2. OpenSSL documentation for X509_STORE_set_default_paths is not partially incorrect. It states that it "loads certificates into the X509_STORE", which it doesn't entirely do.
  3. X509_STORE_set_default_paths calls three other OpenSSL functions: X509_LOOKUP_load_file, X509_LOOKUP_add_dir, and X509_LOOKUP_add_store. load_file loads the certificates into directly into the X509_STORE while add_dir and add_store create on-demand lookups.
  4. The behavior of tls.rootCertificates is broken in both the old and new code. In the old code it returns the hardcoded Node.js root certificates instead of the default OpenSSL ones that are actually used. In the new code it will only return default OpenSSL certificates that have been loaded into the X509_STORE.

A proper fix for this will need to load all certificates into the X509_STORE instead of on-demand. I need to dig a bit deeper to see if there's an existing OpenSSL function for doing so.

Can anyone think of any potential issues if the "on-demand lookup" certificates were completely loaded into the X509_STORE cache?

@bnoordhuis
Copy link
Member

Can anyone think of any potential issues if the "on-demand lookup" certificates were completely loaded into the X509_STORE cache?

The biggest one is probably performance?

@ebickle
Copy link
Contributor

ebickle commented Mar 13, 2020

The biggest one is probably performance?

I agree.

All solutions, regardless of technology, will require an unknown number of certificates to be loaded from the filesystem whenever --use-openssl-ca or NODE_OPENSSL_SYSTEM_CERT_PATH are specified.

In terms of technology, OpenSSL appears to lack a function to repopulate the cache. Loading all of the certificates would likely require listing all files in the certificate directory, filtering out any filename that doesn't match the hashing pattern, then loading each file as an X509 certificate into memory.

There are only two sensible locations to put any code

  1. During the first call to tls.rootCertificates. The load would have to be synchronous.
  2. During Node.js startup. This would be a location less likely to cause unexpected latency in during an application's normal execution but would always require the certificates to be loaded. The usage of tls.rootCertificates is likely low, but all users (who use the OpenSSL filesystem options) would pay the performance penalty.

There aren't any good alternate options either:

  1. The behavior of the PR was non-deterministic, with certificates included in the array dependant on whether or not they had been cached at the time of first access.
  2. The previous behavior of returning the built-in certificates when --use-openssl-ca or NODE_OPENSSL_SYSTEM_CERT_PATH would have the original issue of returning the wrong certificates. The root certificate store in use would be completely different than the node.js hardcoded certificates. Even if this is documented, it seems arbitrary.
  3. Returning an empty array when --use-openssl-ca or NODE_OPENSSL_SYSTEM_CERT_PATH are used seems equally as broken.

The original request that lead to the creation of tls.rootCertificates was a user wanting to add a custom root certificate to node.js at runtime without overriding the default set of certificates. In other words, tlsOptions = { ca: [...tls.rootCertificates, customRootCertificate] }.

We have a similar issue at the company I work for, and I had been planning on submitting a subsequent PR to add setter support to tls.rootCertificates so that the root certificates could be modified without the performance penalty of specifying them in the secure context options.

At this point, there are a few design approaches:

  1. Load the certificates from disk during the first call to get tls.rootCertificates, accept the performance penalty, add set tls.rootCertificates to allow the cert store to be modified.
  2. Return a blank array from get tls.rootCertificates when system certificates are used, document the behavior, add set tls.rootCertificates to allow the cert store to be modified.
  3. Return the hardcoded set of node.js root certificates from get tls.rootCertificates when system certificates are used, add set tls.rootCertificates to allow the cert store to be modified.
  4. Change the specification and documentation of get tls.rootCertificates so that it always returns the built-in set of hardcoded node.js root certificates. It would no longer represent the "current" root certificates used by node.js.
    a. Do nothing else; no capability to modify root certificate store at runtime. Doesn't solve underlying issue.
    b. Add additional functions to the tls library allowing extra CAs to be loaded. This is ultimately what most users are requesting - e.g. tls.addRootCertificates([...]). Simple and effective, but starts to increase the surface area of tls with root certificate management functions.
    c. Add a new CertificateStore or X509Store class to node.js, wrapping the OpenSSL data structure. Users could use the functions on the class to add root certificates or otherwise manipulate node's the root certificate store. As a bonus, a new certificate store object could be passed as an option to tls.createSecureContext (e.g. { store: some_new_cert_store }), removing the current performance penalty of having to construct new OpenSSL certificates in memory from ca: [""] on each and every call.

4a is the simplest option - do nothing.
4b is the simplest option that implements the ability to add extra root CAs at runtime.
4c is the most robust solution.
1, 2, or 3 don't change the surface area of the tls library except for making the existing tls.rootCertificates settable.

I'm equally torn between 2, 3, and 4b.

@AdamMajer
Copy link
Contributor Author

AdamMajer commented Mar 13, 2020 via email

ebickle added a commit to ebickle/node that referenced this issue Mar 17, 2020
Adds CAs from NODE_EXTRA_CA_CERTS to root_certs_vector in node_crypto.cc so that the extra certificates are always added to SecureContext instances.

tls.rootCertificates restored to previous behavior of returning built-in Node.js certificates when --openssl-use-def-ca-store CLI option is  set.

Fixes: nodejs#32229
Fixes: nodejs#32010
Refs: nodejs#32075
MylesBorins pushed a commit to ebickle/node that referenced this issue Mar 26, 2020
Adds CAs from NODE_EXTRA_CA_CERTS to root_certs_vector in node_crypto.cc so that the extra certificates are always added to SecureContext instances.

tls.rootCertificates restored to previous behavior of returning built-in Node.js certificates when --openssl-use-def-ca-store CLI option is  set.

Fixes: nodejs#32229
Fixes: nodejs#32010
Refs: nodejs#32075
ebickle added a commit to ebickle/node that referenced this issue Apr 8, 2020
Adds CAs from NODE_EXTRA_CA_CERTS to root_certs_vector in node_crypto.cc so that the extra certificates are always added to SecureContext instances.

tls.rootCertificates restored to previous behavior of returning built-in Node.js certificates when --openssl-use-def-ca-store CLI option is  set.

Fixes: nodejs#32229
Fixes: nodejs#32010
Refs: nodejs#32075
@ebickle
Copy link
Contributor

ebickle commented May 7, 2020

Crypto team, could I get some feedback on what you'd recommend as the best option to resolve this issue?

Options:

  1. Revert commit 091444a and change tls.rootCertificates specification/documentation to state that it will only ever return the hardcoded default node.js certificates.
  2. When --openssl-use-def-ca-store or --use-openssl-ca are set, have tls.rootCertificates return a blank array.
  3. When --openssl-use-def-ca-store or --use-openssl-ca are set, have tls.rootCertificates return the default hardcoded set of Node.js (Mozilla) root certificates.
  4. When --openssl-use-def-ca-store or --use-openssl-ca are set, have tls.rootCertificates return the certificates currently cached in the X509_STORE. (current behavior, but without caching the rootCertificates array in JavaScript). This is status-quo, but without freezing to whatever state the X509_STORE was in on the first call to tls.rootCertificates.

#4 is non-deterministic. The others have similar pros and cons.

@bnoordhuis
Copy link
Member

I prefer (1). #25824 asked for a way to obtain the built-in certificates and that's why I added tls.rootCertificates, no more, no less.

ebickle added a commit to ebickle/node that referenced this issue May 8, 2020
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: nodejs#32229
Refs: nodejs#32074
ebickle added a commit to ebickle/node that referenced this issue May 8, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates used by tls.createSecureContext.

Fixes: nodejs#32074
Refs: nodejs#32229
BridgeAR pushed a commit to BridgeAR/node that referenced this issue May 23, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates
used by tls.createSecureContext.

Fixes: nodejs#32074
Refs: nodejs#32229

PR-URL: nodejs#33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: #32229
Refs: #32074

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates
used by tls.createSecureContext.

Fixes: #32074
Refs: #32229

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: #32229
Refs: #32074

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates
used by tls.createSecureContext.

Fixes: #32074
Refs: #32229

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: #32229
Refs: #32074

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates
used by tls.createSecureContext.

Fixes: #32074
Refs: #32229

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jul 8, 2020
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: #32229
Refs: #32074

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jul 8, 2020
Update tls.rootCertificates documentation to clarify that it returns
the bundled Node.js root certificates rather than the root certificates
used by tls.createSecureContext.

Fixes: #32074
Refs: #32229

PR-URL: #33313
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants