From d2a4d80f65e8fd362a9707865e7eb5ecee4e55ea Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 15 Sep 2023 17:20:24 +0200 Subject: [PATCH] cleanup: use testingx.NewHTTPProxyHandler as proxy (#1277) 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 https://github.com/ooni/probe-cli/pull/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: https://github.com/ooni/probe/issues/2531 Overall objective: good testing for proxying for when we introduce beacons. --- internal/netemx/scenario.go | 2 +- internal/netxlite/tls.go | 6 +- internal/testingproxy/qa_test.go | 8 +- internal/testingx/httpproxy_test.go | 8 +- internal/testingx/httptestx.go | 69 ---------- internal/testingx/httptestx_test.go | 198 ---------------------------- 6 files changed, 12 insertions(+), 279 deletions(-) diff --git a/internal/netemx/scenario.go b/internal/netemx/scenario.go index 17d90bc9a8..c4a8173ff7 100644 --- a/internal/netemx/scenario.go +++ b/internal/netemx/scenario.go @@ -223,7 +223,7 @@ func MustNewScenario(config []*ScenarioDomainAddresses) *QAEnv { opts = append(opts, QAEnvOptionNetStack(addr, &HTTPCleartextServerFactory{ Factory: HTTPHandlerFactoryFunc(func(env NetStackServerFactoryEnv, stack *netem.UNetStack) http.Handler { - return testingx.HTTPHandlerProxy(env.Logger(), &netxlite.Netx{ + return testingx.NewHTTPProxyHandler(env.Logger(), &netxlite.Netx{ Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: stack}}) }), Ports: []int{80}, diff --git a/internal/netxlite/tls.go b/internal/netxlite/tls.go index 15bf0a10be..9cace9a9cc 100644 --- a/internal/netxlite/tls.go +++ b/internal/netxlite/tls.go @@ -255,18 +255,18 @@ func (h *tlsHandshakerLogger) Handshake( ctx context.Context, conn net.Conn, config *tls.Config, ) (net.Conn, tls.ConnectionState, error) { h.DebugLogger.Debugf( - "tls {sni=%s next=%+v}...", config.ServerName, config.NextProtos) + "tls_handshake {sni=%s next=%+v}...", config.ServerName, config.NextProtos) start := time.Now() tlsconn, state, err := h.TLSHandshaker.Handshake(ctx, conn, config) elapsed := time.Since(start) if err != nil { h.DebugLogger.Debugf( - "tls {sni=%s next=%+v}... %s in %s", config.ServerName, + "tls_handshake {sni=%s next=%+v}... %s in %s", config.ServerName, config.NextProtos, err, elapsed) return nil, tls.ConnectionState{}, err } h.DebugLogger.Debugf( - "tls {sni=%s next=%+v}... ok in %s {next=%s cipher=%s v=%s}", + "tls_handshake {sni=%s next=%+v}... ok in %s {next=%s cipher=%s v=%s}", config.ServerName, config.NextProtos, elapsed, state.NegotiatedProtocol, TLSCipherSuiteString(state.CipherSuite), TLSVersionString(state.Version)) diff --git a/internal/testingproxy/qa_test.go b/internal/testingproxy/qa_test.go index 3e4019068a..93095ccd61 100644 --- a/internal/testingproxy/qa_test.go +++ b/internal/testingproxy/qa_test.go @@ -8,11 +8,11 @@ import ( func TestWorkingAsIntended(t *testing.T) { for _, testCase := range testingproxy.AllTestCases { - short := testCase.Short() - if !short && testing.Short() { - t.Skip("skip test in short mode") - } t.Run(testCase.Name(), func(t *testing.T) { + short := testCase.Short() + if !short && testing.Short() { + t.Skip("skip test in short mode") + } testCase.Run(t) }) } diff --git a/internal/testingx/httpproxy_test.go b/internal/testingx/httpproxy_test.go index 4b2592b318..40cbaeb719 100644 --- a/internal/testingx/httpproxy_test.go +++ b/internal/testingx/httpproxy_test.go @@ -17,11 +17,11 @@ import ( func TestHTTPProxyHandler(t *testing.T) { for _, testCase := range testingproxy.AllTestCases { - short := testCase.Short() - if !short && testing.Short() { - t.Skip("skip test in short mode") - } t.Run(testCase.Name(), func(t *testing.T) { + short := testCase.Short() + if !short && testing.Short() { + t.Skip("skip test in short mode") + } testCase.Run(t) }) } diff --git a/internal/testingx/httptestx.go b/internal/testingx/httptestx.go index fb7480d4c7..488fe1d186 100644 --- a/internal/testingx/httptestx.go +++ b/internal/testingx/httptestx.go @@ -3,12 +3,10 @@ package testingx import ( "crypto/tls" "crypto/x509" - "io" "net" "net/http" "net/url" - "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/optional" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -176,70 +174,3 @@ func httpHandlerHijack(w http.ResponseWriter, r *http.Request, policy string) { // nothing } } - -// TODO(bassosimone): eventually we may want to have a model type -// that models the equivalent of [netxlite.Netx]. - -// HTTPHandlerProxyNetx is [netxlite.Netx] as seen by [HTTPHandlerProxy]. -type HTTPHandlerProxyNetx interface { - NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport -} - -// HTTPHandlerProxy is a handler implementing an HTTP proxy using the host header -// to determine who to connect to. We additionally use the via header to avoid sending -// requests to ourself. Please, note that we designed this proxy ONLY to be used for -// testing purposes and that it's rather simplistic. -func HTTPHandlerProxy(logger model.Logger, netx HTTPHandlerProxyNetx) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { - // reject requests that already visited the proxy and requests we cannot route - if req.Host == "" || req.Header.Get("Via") != "" { - rw.WriteHeader(http.StatusBadRequest) - return - } - - // be explicit about not supporting request bodies - if req.Method != http.MethodGet { - rw.WriteHeader(http.StatusNotImplemented) - return - } - - // clone the request before modifying it - req = req.Clone(req.Context()) - - // include proxy header to prevent sending requests to ourself - req.Header.Add("Via", "testingx/0.1.0") - - // fix: "http: Request.RequestURI can't be set in client requests" - req.RequestURI = "" - - // fix: `http: unsupported protocol scheme ""` - req.URL.Host = req.Host - - // fix: "http: no Host in request URL" - req.URL.Scheme = "http" - - logger.Debugf("PROXY: sending request: %s", req) - - // create HTTP client using netx - txp := netx.NewHTTPTransportStdlib(logger) - - // obtain response - resp, err := txp.RoundTrip(req) - if err != nil { - logger.Warnf("PROXY: request failed: %s", err.Error()) - rw.WriteHeader(http.StatusBadGateway) - return - } - - // write response - rw.WriteHeader(resp.StatusCode) - for key, values := range resp.Header { - for _, value := range values { - rw.Header().Add(key, value) - } - } - - // write response body - _, _ = io.Copy(rw, resp.Body) - }) -} diff --git a/internal/testingx/httptestx_test.go b/internal/testingx/httptestx_test.go index c34e6676a0..9c00133fa6 100644 --- a/internal/testingx/httptestx_test.go +++ b/internal/testingx/httptestx_test.go @@ -10,15 +10,12 @@ import ( "io" "net" "net/http" - "net/http/httptest" - "net/url" "testing" "time" "github.com/apex/log" "github.com/google/go-cmp/cmp" "github.com/ooni/netem" - "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/runtimex" "github.com/ooni/probe-cli/v3/internal/testingx" @@ -476,198 +473,3 @@ func TestHTTPTestxWithNetem(t *testing.T) { }) } } - -func TestHTTPHandlerProxy(t *testing.T) { - expectedBody := []byte("Google is built by a large team of engineers, designers, researchers, robots, and others in many different sites across the globe. It is updated continuously, and built with more tools and technologies than we can shake a stick at. If you'd like to help us out, see careers.google.com.\n") - - type testcase struct { - name string - construct func() (*netxlite.Netx, string, []io.Closer) - short bool - } - - testcases := []testcase{ - { - name: "using the real network", - construct: func() (*netxlite.Netx, string, []io.Closer) { - var closers []io.Closer - - netx := &netxlite.Netx{ - Underlying: nil, // so we're using the real network - } - - proxyServer := testingx.MustNewHTTPServer(testingx.HTTPHandlerProxy(log.Log, netx)) - closers = append(closers, proxyServer) - - return netx, proxyServer.URL, closers - }, - short: false, - }, - - { - name: "using netem", - construct: func() (*netxlite.Netx, string, []io.Closer) { - var closers []io.Closer - - topology := runtimex.Try1(netem.NewStarTopology(log.Log)) - closers = append(closers, topology) - - wwwStack := runtimex.Try1(topology.AddHost("142.251.209.14", "142.251.209.14", &netem.LinkConfig{})) - proxyStack := runtimex.Try1(topology.AddHost("10.0.0.1", "142.251.209.14", &netem.LinkConfig{})) - clientStack := runtimex.Try1(topology.AddHost("10.0.0.2", "142.251.209.14", &netem.LinkConfig{})) - - dnsConfig := netem.NewDNSConfig() - dnsConfig.AddRecord("www.google.com", "", "142.251.209.14") - dnsServer := runtimex.Try1(netem.NewDNSServer(log.Log, wwwStack, "142.251.209.14", dnsConfig)) - closers = append(closers, dnsServer) - - wwwServer := testingx.MustNewHTTPServerEx( - &net.TCPAddr{IP: net.IPv4(142, 251, 209, 14), Port: 80}, - wwwStack, - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Write(expectedBody) - }), - ) - closers = append(closers, wwwServer) - - proxyServer := testingx.MustNewHTTPServerEx( - &net.TCPAddr{IP: net.IPv4(10, 0, 0, 1), Port: 80}, - proxyStack, - testingx.HTTPHandlerProxy(log.Log, &netxlite.Netx{ - Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: proxyStack}, - }), - ) - closers = append(closers, proxyServer) - - clientNet := &netxlite.Netx{Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: clientStack}} - return clientNet, proxyServer.URL, closers - }, - short: true, - }} - - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - if !tc.short && testing.Short() { - t.Skip("skip test in short mode") - } - - netx, proxyURL, closers := tc.construct() - defer func() { - for _, closer := range closers { - closer.Close() - } - }() - - URL := runtimex.Try1(url.Parse(proxyURL)) - URL.Path = "/humans.txt" - - req := runtimex.Try1(http.NewRequest("GET", URL.String(), nil)) - req.Host = "www.google.com" - - //log.SetLevel(log.DebugLevel) - - txp := netx.NewHTTPTransportStdlib(log.Log) - client := netxlite.NewHTTPClient(txp) - - resp, err := client.Do(req) - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() - - if resp.StatusCode != 200 { - t.Fatal("expected to see 200, got", resp.StatusCode) - } - - t.Logf("%+v", resp) - - body, err := netxlite.ReadAllContext(req.Context(), resp.Body) - if err != nil { - t.Fatal(err) - } - - t.Logf("%s", string(body)) - - if diff := cmp.Diff(expectedBody, body); diff != "" { - t.Fatal(diff) - } - }) - } - - t.Run("rejects requests without a host header", func(t *testing.T) { - rr := httptest.NewRecorder() - netx := &netxlite.Netx{Underlying: &mocks.UnderlyingNetwork{ - // all nil: panic if we hit the network - }} - handler := testingx.HTTPHandlerProxy(log.Log, netx) - req := &http.Request{ - Host: "", // explicitly empty - } - handler.ServeHTTP(rr, req) - res := rr.Result() - if res.StatusCode != http.StatusBadRequest { - t.Fatal("unexpected status code", res.StatusCode) - } - }) - - t.Run("rejects requests with a via header", func(t *testing.T) { - rr := httptest.NewRecorder() - netx := &netxlite.Netx{Underlying: &mocks.UnderlyingNetwork{ - // all nil: panic if we hit the network - }} - handler := testingx.HTTPHandlerProxy(log.Log, netx) - req := &http.Request{ - Host: "www.example.com", - Header: http.Header{ - "Via": {"antani/0.1.0"}, - }, - } - handler.ServeHTTP(rr, req) - res := rr.Result() - if res.StatusCode != http.StatusBadRequest { - t.Fatal("unexpected status code", res.StatusCode) - } - }) - - t.Run("rejects requests with a POST method", func(t *testing.T) { - rr := httptest.NewRecorder() - netx := &netxlite.Netx{Underlying: &mocks.UnderlyingNetwork{ - // all nil: panic if we hit the network - }} - handler := testingx.HTTPHandlerProxy(log.Log, netx) - req := &http.Request{ - Host: "www.example.com", - Header: http.Header{}, - Method: http.MethodPost, - } - handler.ServeHTTP(rr, req) - res := rr.Result() - if res.StatusCode != http.StatusNotImplemented { - t.Fatal("unexpected status code", res.StatusCode) - } - }) - - t.Run("returns 502 when the round trip fails", func(t *testing.T) { - rr := httptest.NewRecorder() - netx := &netxlite.Netx{Underlying: &mocks.UnderlyingNetwork{ - MockGetaddrinfoLookupANY: func(ctx context.Context, domain string) ([]string, string, error) { - return nil, "", errors.New("mocked error") - }, - MockGetaddrinfoResolverNetwork: func() string { - return "antani" - }, - }} - handler := testingx.HTTPHandlerProxy(log.Log, netx) - req := &http.Request{ - Host: "www.example.com", - Header: http.Header{}, - Method: http.MethodGet, - URL: &url.URL{}, - } - handler.ServeHTTP(rr, req) - res := rr.Result() - if res.StatusCode != http.StatusBadGateway { - t.Fatal("unexpected status code", res.StatusCode) - } - }) -}