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

src: fix missing extra ca in tls.rootCertificates #32075

Closed

Conversation

ebickle
Copy link
Contributor

@ebickle ebickle commented Mar 3, 2020

Fixes tls.rootCertificates missing certificates loaded from NODE_EXTRA_CA_CERTS.

Fixes: #32074

Potentially contains a semver-major breaking change. Change adheres to all existing specifications, but there is a non-zero risk that callers may have written code that does not expect changes to the contents of the tls.rootCertificates array.

  • tls.rootCertificates now includes certificate loaded from NODE_EXTRA_CA_CERTS environment variable.
  • each PEM string in tls.rootCertificates is now terminated with a \n line break. This adheres to the PEM file format specification and is generally preferred - many non node.js systems fail if the line-break is not specified.

Notes for code reviewers:

  • OpenSSL writes PEM strings with a trailing newline. Keeping the trailing newline reduces code complexity and allows multiple certificates to be easily concatenated into a single PEM file.
  • Certificates are read directly from the root X509_STORE* to ensure that tls.rootCertificates reflects the true certificates used by SecureContext and to avoid synchronizing/maintaining a separate variable in node_crypto.cc.
  • The new X509ToPEM function was placed in node_crypto_common.cc since it pairs with the existing X509ToObject function.
  • The GetRootCertificates function returns without setting a JavaScript return value when an unexpected error occurs (e.g. error calling PEM_write_bio_X509). This was done to remain consistent with the previous implementation of GetRootCertificates.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Fixes tls.rootCertificates missing certificates loaded from NODE_EXTRA_CA_CERTS.

Fixes: nodejs#32074
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 3, 2020
@addaleax
Copy link
Member

addaleax commented Mar 3, 2020

@nodejs/crypto

@addaleax addaleax added the crypto Issues and PRs related to the crypto subsystem. label Mar 3, 2020
@sam-github
Copy link
Contributor

Seems reasonable to me.

I don't think that adding a NL to the PEM is semver-major, particularly since PEM tooling is robust to extra NLs. In other words if people used to be adding a NL while concatenation certs and were getting "END X -----\n-----BEGIN ..." between certs and now get "END X -----\n\n-----BEGIN ...", IMO, the output is still within spec.

All bug fixes have a non-zero chance of breaking code which depends on implementation details.

@ebickle
Copy link
Contributor Author

ebickle commented Mar 4, 2020

Looks as though all of the node ASAN builds were failing across the node.js ecosystem when this PR was submitted. Doesn't appear to be related to this PR.

src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto_common.cc Outdated Show resolved Hide resolved
- Moved X509ToPEM function to node_crypto.cc
- Removed braces for single line statements
- Modified X509ToPEM so that it throws crypto errors for unexpected OpenSSL errors
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.h Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 9, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in 091444a

@addaleax addaleax closed this Mar 11, 2020
addaleax pushed a commit that referenced this pull request Mar 11, 2020
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 12, 2020
ebickle added a commit to ebickle/node that referenced this pull request 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 pull request 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 pull request 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 pushed a commit that referenced this pull request Apr 22, 2020
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@ebickle ebickle deleted the fix/tls-missing-rootcertificates branch September 5, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra CA certificates missing from tls.rootCertificates
7 participants