Fix TLS bug for TLS over TLS proxy #65
Merged
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
Without this fix, you see this error when attempting to use an HTTPS proxy. `Error [ERR_TLS_CERT_ALTNAME_INVALID]: Hostname/IP does not match certificate's altnames: Host: foo.com. is not in the cert's altnames: DNS:*.bar.com, DNS:bar.com` The `servername` attribute is able to fix this behavior when passed to the underlying `tls.connect()` call inside of the `https` module. (see the Node [docs](https://nodejs.org/api/https.html#httpsrequesturl-options-callback)) This was very painful to debug but now I understand how `CONNECT` works extremely well! Cheers. FIxes delvedor#43
Add unit tests, backport Free's fix for Node v10, setup workflow for windows env var
|
You can use the following package instead of needing to create two separate npm scripts to set environment variables: |
|
thanks @ckcr4lyf I had planned to do the exact same pull request today :) |
delvedor
requested changes
May 2, 2022
delvedor
changed the title
This was referenced May 2, 2022
Closed
|
Thanks for landing this! I didn't mean to ghost y'all -- I just don't see GitHub's notifications unless I look for them. Please email me if you need a response from me in the future -- I'll see that immediately. 🙏 |
|
FYI: There's some weird setting in Github that after 10 years of opening an account, these notifications need to be reactivated, I had the same issue until I found this setting a year or two ago @freeqaz |
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.
First off, thanks to @freeqaz for the initial investigation and fix (#51). Since they seem MIA, I've decided to write the unit tests for it since I want to be able to use the fix!!!
Previously the tests were using
process.env.NODE_TLS_REJECT_UNAUTHORIZED = 0, which bypassed all TLS checks. Additionally, the self-signed cert was for fastify.In order to properly test the TLS part, including handshakes and correct SNI, I generated two certs for "fake" domains (b9d8ae1#diff-56d361e6893cf9d961de277f3986602b66ace5d977e446d4161c664a95a48f5bR13) , and then for the tests used the node option
NODE_EXTRA_CA_CERTSto tell it to trust these certs.I also had to monkey patch DNS lookups in node.js to make these fake domains resolve to
127.0.0.1, which seems to work fine.I then changed the test proxy / server to use their respective TLS keys & certs, and used the fake domain wherever the hostname was needed.
The scope of the bug can be realized by running the new tests, with Free's fix commented out:
Other misc:
test-cistep which is used, while keepingtesteasy to run locally on unix systems (with the CA var included)Cheers