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.

Sign up for GitHub

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

Jump to bottom

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
@ebickle
src: fix missing extra ca in secure contexts
d47a7ab 
Fixes SecureContext missing NODE_EXTRA_CA_CERTS when
SecureContext::AddCACert, SecureContext::AddCRL,
or SecureContext::LoadPKCS12 are called.

Fixes: nodejs#32010
@ebickle ebickle mentioned this issue Feb 28, 2020
Closed
3 tasks
ebickle added a commit to ebickle/node that referenced this issue Mar 17, 2020
@ebickle
src: fix CAs missing from secure contexts
c351a0e 
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 ebickle mentioned this issue Mar 17, 2020
Closed
3 tasks
MylesBorins pushed a commit to ebickle/node that referenced this issue Mar 26, 2020
@ebickle @MylesBorins
src: fix CAs missing from secure contexts
1b0f50b 
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
@ebickle
src: fix CAs missing from secure contexts
d6b075d 
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 ebickle mentioned this issue Aug 24, 2022
Open
ebickle added a commit to ebickle/node that referenced this issue Aug 29, 2022
@ebickle
src: Add NODE_EXTRA_CA_CERTS to modified cert stores
ba09b5c 
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
@ebickle
src: Add NODE_EXTRA_CA_CERTS to modified cert stores
31a6a83 
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
@ebickle
src: add NODE_EXTRA_CA_CERTS to modified cert stores
2ab0b3c 
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
@ebickle
src: add NODE_EXTRA_CA_CERTS to modified cert stores
ae2939f 
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 ebickle mentioned this issue Aug 30, 2022
Closed
ebickle added a commit to ebickle/node that referenced this issue Sep 5, 2022
@ebickle
crypto: add extra CA certs to all secure contexts
314bada 
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 ebickle mentioned this issue Sep 6, 2022
Closed
ebickle added a commit to ebickle/node that referenced this issue Sep 6, 2022
@ebickle
crypto: add extra CA certs to all secure contexts
3857c32 
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
@ebickle
crypto: add extra CA certs to all secure contexts
61b1f13 
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
@ebickle @targos
crypto: include NODE_EXTRA_CA_CERTS in all secure contexts by default
d52f515 
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
Assignees
No one assigned
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
2 participants
@ebickle @targos