FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix missing extra ca in tls.rootCertificates by ebickle · Pull Request #32075 · nodejs/node · GitHub

/ node Public

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

Closed
ebickle wants to merge 4 commits into
nodejs:masterfrom
ebickle:fix/tls-missing-rootcertificates
ConversationCommits (4)ChecksFiles changed
Closed

src: fix missing extra ca in tls.rootCertificates#32075
ebickle wants to merge 4 commits into
nodejs:masterfrom
ebickle:fix/tls-missing-rootcertificates

Conversation

ebickle commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

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

src: fix missing extra ca in tls.rootCertificates
7717169 
Fixes tls.rootCertificates missing certificates loaded from NODE_EXTRA_CA_CERTS.

Fixes: nodejs#32074
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 commented Mar 3, 2020

Copy link
Copy Markdown
Member

@nodejs/crypto

addaleax added the crypto Issues and PRs related to the crypto subsystem. label Mar 3, 2020

sam-github commented Mar 3, 2020

Copy link
Copy Markdown
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 commented Mar 4, 2020

Copy link
Copy Markdown
Contributor Author

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.

bnoordhuis reviewed Mar 4, 2020
View reviewed changes
Comment thread src/node_crypto.cc Outdated
Comment thread src/node_crypto.cc Outdated
Comment thread src/node_crypto_common.cc Outdated
src: add exceptions to X509ToPEM
177a36c 
- 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
jasnell reviewed Mar 5, 2020
View reviewed changes
Comment thread src/node_crypto.cc Outdated
jasnell reviewed Mar 5, 2020
View reviewed changes
Comment thread src/node_crypto.h Outdated
BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 9, 2020

nodejs-github-bot commented Mar 9, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax commented Mar 11, 2020

Copy link
Copy Markdown
Member

Landed in 091444a

addaleax closed this Mar 11, 2020
addaleax pushed a commit that referenced this pull request Mar 11, 2020
src: fix missing extra ca in tls.rootCertificates
091444a 
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
src: fix missing extra ca in tls.rootCertificates
1fc7c04 
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 11, 2020
src: fix missing extra ca in tls.rootCertificates
2248ba7 
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Mar 12, 2020
Merged
ebickle added a commit to ebickle/node that referenced this pull request Mar 17, 2020
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 mentioned this pull request Mar 17, 2020
Closed
3 tasks
MylesBorins pushed a commit to ebickle/node that referenced this pull request Mar 26, 2020
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 pull request Apr 8, 2020
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 pushed a commit that referenced this pull request Apr 22, 2020
src: fix missing extra ca in tls.rootCertificates
5b2f698 
Fixes tls.rootCertificates missing certificates loaded from
NODE_EXTRA_CA_CERTS.

Fixes: #32074

PR-URL: #32075
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Apr 22, 2020
Merged
ebickle deleted the fix/tls-missing-rootcertificates branch September 5, 2022 16:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extra CA certificates missing from tls.rootCertificates

7 participants

Back | FazBrowse Home | New Git URL