Fix TLS bug for TLS over TLS proxy

ckcr4lyf commented 2 years ago

First off, thanks to @freeqaz for the initial investigation and fix (https://github.com/delvedor/hpagent/pull/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 (https://github.com/delvedor/hpagent/commit/b9d8ae175b04cf610a2377f129ade722421d282b#diff-56d361e6893cf9d961de277f3986602b66ace5d977e446d4161c664a95a48f5bR13) , and then for the tests used the node option NODE_EXTRA_CA_CERTS to 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:

To pass the env var for windows test in CI, I changed workflow to add env var, and added a test-ci step which is used, while keeping test easy to run locally on unix systems (with the CA var included)
The tests showed that in Node 10, Free's fix was not sufficient due to how it passes the HTTP hostname to HTTPS proxy! I added the servername option to that class as well.

Cheers

Enzime commented 2 years ago

You can use the following package instead of needing to create two separate npm scripts to set environment variables:

https://www.npmjs.com/package/cross-env

arnaudruffin commented 2 years ago

thanks @ckcr4lyf I had planned to do the exact same pull request today :)

freeqaz commented 2 years ago

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. :pray:

binarymist commented 1 year ago

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

delvedor / hpagent

Fix TLS bug for TLS over TLS proxy #65