GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — src: fix missing extra ca in tls.rootCertificates by ebickle · Pull Request #32075 · nodejs/node

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction


          src: fix missing extra ca in tls.rootCertificates

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 involving general changes in the lib/ or src/ directories. labels

Mar 3, 2020

addaleax commented Mar 3, 2020

Copy link

Copy Markdown

Member

@nodejs/crypto

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          src: fix unsigned data type warning in node_crypto

ce40db3

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis reviewed

Mar 4, 2020

View reviewed changes

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

src/node_crypto_common.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.


          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 Resolved

Uh oh!

There was an error while loading. Please reload this page.

jasnell reviewed

Mar 5, 2020

View reviewed changes

Comment thread

src/node_crypto.h Resolved

Uh oh!

There was an error while loading. Please reload this page.


          src: remove header decl of X509ToPEM

2ff1257

addaleax approved these changes

Mar 5, 2020

View reviewed changes

bnoordhuis approved these changes

Mar 5, 2020

View reviewed changes

jasnell approved these changes

Mar 5, 2020

View reviewed changes

BridgeAR added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Mar 11, 2020

Copy link

Copy Markdown

Member

Landed in 091444a

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 <[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


          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 <[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


          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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request

Mar 12, 2020

v13.11.0 proposal #32185

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

src: fix CAs missing from secure contexts #32315

Closed

3 tasks done

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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>

targos mentioned this pull request

Apr 22, 2020

v12.16.3 release proposal #33009

Merged

ebickle deleted the fix/tls-missing-rootcertificates branch

September 5, 2022 16:18

Reviewers

addaleax

addaleax approved these changes

bnoordhuis

bnoordhuis approved these changes

jasnell

jasnell approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

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 involving general changes in the lib/ or src/ directories.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Extra CA certificates missing from tls.rootCertificates

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.