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

Extra CA certificates missing when certain TLS options specified #32010

Closed
ebickle opened this issue Feb 28, 2020 · 0 comments
Closed

Extra CA certificates missing when certain TLS options specified #32010

ebickle opened this issue Feb 28, 2020 · 0 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@ebickle
Copy link
Contributor

ebickle commented Feb 28, 2020

  • Version: v14.0.0-pre
  • Platform: Windows 10 Version 1903 64-bit (OS Build 18362.657)
  • Subsystem: crypto / tls

What steps will reproduce the bug?

Test Case 1: CRL

  1. Set NODE_EXTRA_CA_CERTS environment variable to a root certificate file.
  2. Call tls.connect() and target a host that has a certificate signed off of the root certificate. Supply the crl option.

Test Case 2: PFX

  1. Set NODE_EXTRA_CA_CERTS environment variable to a root certificate file.
  2. Call tls.connect() and target a host that has a certificate signed off of the root certificate. Supply pfx option.

Test Case 3: CA

  1. Set NODE_EXTRA_CA_CERTS environment variable to a root certificate file.
  2. Call tls.createSecureContext()
  3. Call context.addCACert on the new secure context, passing it a different root certificate than the one specified in NODE_EXTRA_CA_CERTS.
  4. Call tls.connect() and target a host that has a certificate signed off of the root certificate specified in NODE_EXTRA_CA_CERTS. Supply the secureContext option.

How often does it reproduce? Is there a required condition?

Reproduces 100% of the time.

What is the expected behavior?

Root certificates specified in NODE_EXTRA_CA_CERTS should always exist in a SecureContext whenever the Node.js hardcoded root certificates exist.

What do you see instead?

In all three test cases, the TLS connection will incorrectly fail due the root certificate being untrusted. This occurs even though the necessary root certificate was specified in NODE_EXTRA_CA_CERTS.

Additional information

node_crypto.cc has multiple bugs where calls to SecureContext::AddCACert, SecureContext::AddCRL and SecureContext::LoadPKCS12 will create a new OpenSSL X509_STORE and fail to include the extra certificates.

These functions call NewRootCertStore(), which only loads the static root certificates and path-based system certificates.

I'll submit a PR with reproduction unit tests and a bug fix.

ebickle added a commit to ebickle/node that referenced this issue Feb 28, 2020
Fixes SecureContext missing NODE_EXTRA_CA_CERTS when
SecureContext::AddCACert, SecureContext::AddCRL,
or SecureContext::LoadPKCS12 are called.

Fixes: nodejs#32010
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
@targos targos added the tls Issues and PRs related to the tls subsystem. label Dec 27, 2020
ebickle added a commit to ebickle/node that referenced this issue Aug 29, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
ebickle added a commit to ebickle/node that referenced this issue Aug 30, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
ebickle added a commit to ebickle/node that referenced this issue Aug 30, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
ebickle added a commit to ebickle/node that referenced this issue Aug 30, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
ebickle added a commit to ebickle/node that referenced this issue Sep 5, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the root certificates
(both bundled and extra) will no longer be preloaded at startup.
This improves Node.js startup time and makes the behavior of
NODE_EXTRA_CA_CERTS consistent with the default behavior when
NODE_EXTRA_CA_CERTS is omitted.

The original reason NODE_EXTRA_CA_CERTS were loaded at startup
(issues nodejs#20432, nodejs#20434) was to prevent the environment variable from
being changed at runtime. This change preserves the runtime consistency
without actually having to load the certs at startup.

Fixes: nodejs#32010
Refs: nodejs#40524
Refs: nodejs#23354
ebickle added a commit to ebickle/node that referenced this issue Sep 6, 2022
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the root certificates
(both bundled and extra) will no longer be preloaded at startup.
This improves Node.js startup time and makes the behavior of
NODE_EXTRA_CA_CERTS consistent with the default behavior when
NODE_EXTRA_CA_CERTS is omitted.

The original reason NODE_EXTRA_CA_CERTS were loaded at startup
(issues nodejs#20432, nodejs#20434) was to prevent the environment variable from
being changed at runtime. This change preserves the runtime consistency
without actually having to load the certs at startup.

Fixes: nodejs#32010
Refs: nodejs#40524
Refs: nodejs#23354
ebickle added a commit to ebickle/node that referenced this issue Jul 26, 2024
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the root certificates
(both bundled and extra) will no longer be preloaded at startup.
This improves Node.js startup time and makes the behavior of
NODE_EXTRA_CA_CERTS consistent with the default behavior when
NODE_EXTRA_CA_CERTS is omitted.

The original reason NODE_EXTRA_CA_CERTS were loaded at startup
(issues nodejs#20432, nodejs#20434) was to prevent the environment variable from
being changed at runtime. This change preserves the runtime consistency
without actually having to load the certs at startup.

Fixes: nodejs#32010
Refs: nodejs#40524
Refs: nodejs#23354
targos pushed a commit that referenced this issue Aug 14, 2024
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called, rather than losing them when unrelated options are provided.

When NODE_EXTRA_CA_CERTS is specified, the root certificates
(both bundled and extra) will no longer be preloaded at startup.
This improves Node.js startup time and makes the behavior of
NODE_EXTRA_CA_CERTS consistent with the default behavior when
NODE_EXTRA_CA_CERTS is omitted.

The original reason NODE_EXTRA_CA_CERTS were loaded at startup
(issues #20432, #20434) was to prevent the environment variable from
being changed at runtime. This change preserves the runtime consistency
without actually having to load the certs at startup.

Fixes: #32010
Refs: #40524
Refs: #23354
PR-URL: #44529
Reviewed-By: Tim Perry <pimterry@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
2 participants