feat(testingx): introduce more comprehensive HTTP(S) proxy #1274
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
bassosimone
commented
Sep 15, 2023
bassosimone
changed the title
bassosimone
commented
Sep 15, 2023
bassosimone
commented
Sep 15, 2023
bassosimone
changed the title
bassosimone
added a commit
that referenced
this pull request
Sep 15, 2023
This diff replaces `testingx.HTTPHandlerProxy` with `testingx.NewHTTPProxyHandler` as the proxy used for implementing netemx scenarios and removes `testingx.HTTPHandlerProxy`. We introduced `testingx.NewHTTPProxyHandler` in #1274. It is a more comprehensive proxy because it supports both proxying via HTTP header and CONNECT proxying. While there, emit more clear debug messages during the TLS handshake. While there, fix how we're skipping tests in `testingx` and `testingproxy` because otherwise we end up skipping the netem tests that we should not be skipping. (Noticed of this because the coverage dropped!) Reference issue: ooni/probe#2531 Overall objective: good testing for proxying for when we introduce beacons.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
We want more comprehensive testing of how we use proxies during the bootstrap. Tests should encompass both SOCKS5 and HTTP(S) proxies. Tests should support using both the host network and using netem. This diff starts paving the way for improving proxy testing, by introducing in `./internal/testingx/httpproxy.go` an HTTP(S) proxy implementation supporting both the case of HTTP over HTTP(S) proxy (where you use the host header) and HTTPS over HTTP(S) proxy (where the client issues a `CONNECT` request to the remote endpoint). The previous implementation (in `./internal/testingx/httptestx.go`) is still there, for now. We used the previous implementation, which only supported the host header, as a starting point for writing the new one. More in detail, this diff introduces the new proxy and its tests. Because testing the proxy functionality is a bit complex, I chose to use a separate package and also write tests for the tests. Obviously, we're still using `netxlite` as the underlying library, so there is some circularity in testing, but `netxlite` also has its own tests, so we should probably be fine. The separate package with tests is `./internal/testingproxy`. While working on this package, I realized the need to forward the CA used by the proxy. This happens in `./internal/testingproxy/hosthttps.go`. If we do not do this, we see the following failure: ``` === RUN TestWorkingAsIntended/fetching_https://www.example.com/_using_the_host_network_and_an_HTTPS_proxy 2023/09/15 15:28:39 debug > GET https://www.example.com/ 2023/09/15 15:28:39 debug > 2023/09/15 15:28:39 dialerWithAssertions: verified that the network is tcp as expected 2023/09/15 15:28:39 dialerWithAssertions: verified that the address is 127.0.0.1 as expected 2023/09/15 15:28:39 debug dial 127.0.0.1:61772/tcp... 2023/09/15 15:28:39 debug dial_address 127.0.0.1:61772/tcp... 2023/09/15 15:28:39 debug dial_address 127.0.0.1:61772/tcp... ok in 212.083µs 2023/09/15 15:28:39 debug dial 127.0.0.1:61772/tcp... ok in 222.208µs 2023/09/15 15:28:39 debug tls {sni=127.0.0.1 next=[]}... 2023/09/15 15:28:39 debug tls {sni=127.0.0.1 next=[]}... ssl_unknown_authority in 3.687875ms [...] ``` In other words, the TLS config we're using does not know the proxy CA. So, in this diff you also see the required work to forward the proxy CA, including the related netem changes in ooni/netem#38. The reference issue is ooni/probe#2531.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
This diff replaces `testingx.HTTPHandlerProxy` with `testingx.NewHTTPProxyHandler` as the proxy used for implementing netemx scenarios and removes `testingx.HTTPHandlerProxy`. We introduced `testingx.NewHTTPProxyHandler` in ooni#1274. It is a more comprehensive proxy because it supports both proxying via HTTP header and CONNECT proxying. While there, emit more clear debug messages during the TLS handshake. While there, fix how we're skipping tests in `testingx` and `testingproxy` because otherwise we end up skipping the netem tests that we should not be skipping. (Noticed of this because the coverage dropped!) Reference issue: ooni/probe#2531 Overall objective: good testing for proxying for when we introduce beacons.
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.
We want more comprehensive testing of how we use proxies during the bootstrap. Tests should encompass both SOCKS5 and HTTP(S) proxies. Tests should support using both the host network and using netem.
This diff starts paving the way for improving proxy testing, by introducing in
./internal/testingx/httpproxy.goan HTTP(S) proxy implementation supporting both the case of HTTP over HTTP(S) proxy (where you use the host header) and HTTPS over HTTP(S) proxy (where the client issues aCONNECTrequest to the remote endpoint).The previous implementation (in
./internal/testingx/httptestx.go) is still there, for now. We used the previous implementation, which only supported the host header, as a starting point for writing the new one.More in detail, this diff introduces the new proxy and its tests. Because testing the proxy functionality is a bit complex, I chose to use a separate package and also write tests for the tests. Obviously, we're still using
netxliteas the underlying library, so there is some circularity in testing, butnetxlitealso has its own tests, so we should probably be fine.The separate package with tests is
./internal/testingproxy.While working on this package, I realized the need to forward the CA used by the proxy. This happens in
./internal/testingproxy/hosthttps.go. If we do not do this, we see the following failure:In other words, the TLS config we're using does not know the proxy CA. So, in this diff you also see the required work to forward the proxy CA, including the related netem changes in ooni/netem#38.
The reference issue is ooni/probe#2531.