src: check node_extra_ca_certs after openssl config #48159
Closed
Conversation
This file contains 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
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.
|
Review requested:
|
nodejs-github-bot
added
c++
richardlau
approved these changes
May 24, 2023
github-actions
bot
removed
the
request-ci
21 tasks
tniessen
approved these changes
May 25, 2023
21 tasks
JungMinu
approved these changes
May 26, 2023
mhdawson
pushed a commit
that referenced
this pull request
May 26, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
|
Landed as be469d8 |
This was referenced May 27, 2023
targos
pushed a commit
that referenced
this pull request
May 30, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
22 tasks
danielleadams
pushed a commit
that referenced
this pull request
Jul 6, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
MoLow
pushed a commit
to MoLow/node
that referenced
this pull request
Jul 6, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Ceres6
pushed a commit
to Ceres6/node
that referenced
this pull request
Aug 14, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Ceres6
pushed a commit
to Ceres6/node
that referenced
this pull request
Aug 14, 2023
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 <rlau@redhat.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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_CERTSwas 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_CERTSset 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