GeezerVison

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

GeezerVison is showing github.com — src: check node_extra_ca_certs after openssl config by ckcr4lyf · Pull Request #48159 · nodejs/node

Conversation

ckcr4lyf commented May 24, 2023

Copy link

Copy Markdown

Contributor

I recently discovered that the custom NodeJS specific OpenSSL config section in openssl.cnf would not be respected, if the environment variable NODE_EXTRA_CA_CERTS was set.

This happens even if it contains an invalid value, i.e no actual certs are read.

Someone suggested moving the checking of extra ca certs to after the OpenSSL config is read, and this seems to work.

I wasn't sure how to add a test for this directly into the repo, but as part of my bug report, I had constructed a test which attempts to make a TLS connection to the offending server with a custom OpenSSL config, once with NODE_EXTRA_CA_CERTS set to "" , and once with a dummy value (e.g /dev/null)

To test this change, I build node with the patch and made published it as a container (you can see dockerfile & patch here: https://github.com/ckcr4lyf/no-rfc5746/tree/master/patched_node)

I then updated my tests to load the custom SSL config into this patched node and try the same test, and it passes both cases now!

An example run can be found here: https://github.com/ckcr4lyf/no-rfc5746/actions/runs/5071123450

Note: I am testing the node's usage of the custom SSL config by providing a non-default legacy renegotiation option, and then connecting to a mocked server where I handcraft this insecure ServerHello.

Fixes #48143

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: check node_extra_ca_certs after openssl config

fd6235f

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

nodejs-github-bot commented May 24, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/startup

All reactions

Sorry, something went wrong.

Uh oh!

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

nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci PRs that need a full CI run. labels

May 24, 2023

richardlau approved these changes

May 24, 2023

View reviewed changes

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 24, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 24, 2023

nodejs-github-bot commented May 24, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot mentioned this pull request

May 25, 2023

CI Reliability 2023-05-25 nodejs/reliability#576

Open

21 tasks

ckcr4lyf mentioned this pull request

May 25, 2023

Race conditions between setTimeout() & setImmediate() #48163

Closed

nodejs-github-bot commented May 25, 2023

Copy link

Copy Markdown

Collaborator

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

ckcr4lyf reacted with eyes emoji

All reactions

👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

tniessen approved these changes

May 25, 2023

View reviewed changes

tniessen requested a review from bnoordhuis

May 25, 2023 17:23

github-actions Bot mentioned this pull request

May 26, 2023

CI Reliability 2023-05-26 nodejs/reliability#577

Open

21 tasks

JungMinu approved these changes

May 26, 2023

View reviewed changes

mhdawson approved these changes

May 26, 2023

View reviewed changes

mhdawson left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions

mhdawson pushed a commit that referenced this pull request

May 26, 2023


          src: check node_extra_ca_certs after openssl cfg

be469d8

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: #48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

mhdawson commented May 26, 2023

Copy link

Copy Markdown

Member

Landed as be469d8

ckcr4lyf reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

mhdawson closed this

May 26, 2023

This was referenced May 27, 2023

CI Reliability 2023-05-27 nodejs/reliability#578

Open

CI Reliability 2023-05-28 nodejs/reliability#579

Open

CI Reliability 2023-05-29 nodejs/reliability#580

Open

CI Reliability 2023-05-30 nodejs/reliability#581

Open

targos pushed a commit that referenced this pull request

May 30, 2023


          src: check node_extra_ca_certs after openssl cfg

b8f4070

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: #48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

github-actions Bot mentioned this pull request

May 31, 2023

CI Reliability 2023-05-31 nodejs/reliability#582

Open

22 tasks

targos mentioned this pull request

Jun 4, 2023

v20.3.0 release proposal #48332

Merged

richardlau mentioned this pull request

Jun 5, 2023

nodejs18 and openssl 3.0.8 nodejs/help#4159

Closed

ckcr4lyf mentioned this pull request

Jun 12, 2023

NODE_EXTRA_CA_CERTS option seems to conflict with custom OpenSSL config #48143

Closed

danielleadams pushed a commit that referenced this pull request

Jul 6, 2023


          src: check node_extra_ca_certs after openssl cfg

4e31167

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: #48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

MoLow pushed a commit to MoLow/node that referenced this pull request

Jul 6, 2023


          src: check node_extra_ca_certs after openssl cfg

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: nodejs#48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

danielleadams mentioned this pull request

Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Aug 14, 2023


          src: check node_extra_ca_certs after openssl cfg

f18865b

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: nodejs#48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Aug 14, 2023


          src: check node_extra_ca_certs after openssl cfg

d2d9d10

I recently discovered that the custom NodeJS specific OpenSSL
config section in openssl.cnf would not be respected, if the
environment variable `NODE_EXTRA_CA_CERTS` was set.

This happens even if it contains an invalid value, i.e no actual
certs are read.

Someone suggested moving the checking of extra ca certs to after
the OpenSSL config is read, and this seems to work.

PR-URL: nodejs#48159
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

Reviewers

tniessen

tniessen approved these changes

JungMinu

JungMinu approved these changes

richardlau

richardlau approved these changes

bnoordhuis

Awaiting requested review from bnoordhuis

+1 more reviewer

mhdawson

mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NODE_EXTRA_CA_CERTS option seems to conflict with custom OpenSSL config

Uh oh!

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

6 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.