From 6895946a347041cc0f952eb105ed134f09cfe7cd Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 1 Jul 2021 15:26:08 +0200 Subject: [PATCH] refactor: introduce factory for stdlib http transport (#413) With this factory, we want to construct ourselves the TLS dialer so that we can use a dialer wrapper that always sets timeouts when reading, addressing https://github.com/ooni/probe/issues/1609. As a result, we cannot immediately replace the i/e/netx factory for creating a new HTTP transport, since the functions signatures are not directly compatible. Refactoring is part of https://github.com/ooni/probe/issues/1505. --- .../netx/httptransport/http3transport.go | 4 ++ internal/engine/netx/httptransport/system.go | 4 ++ internal/netxlite/http.go | 59 ++++++++++++++++++- internal/netxlite/http3.go | 9 ++- internal/netxlite/http_test.go | 42 +++++++++++++ 5 files changed, 115 insertions(+), 3 deletions(-) diff --git a/internal/engine/netx/httptransport/http3transport.go b/internal/engine/netx/httptransport/http3transport.go index b7b75e0..95427ac 100644 --- a/internal/engine/netx/httptransport/http3transport.go +++ b/internal/engine/netx/httptransport/http3transport.go @@ -5,6 +5,10 @@ import ( ) // NewHTTP3Transport creates a new HTTP3Transport instance. +// +// Deprecation warning +// +// New code should use netxlite.NewHTTP3Transport instead. func NewHTTP3Transport(config Config) RoundTripper { return netxlite.NewHTTP3Transport(config.QUICDialer, config.TLSConfig) } diff --git a/internal/engine/netx/httptransport/system.go b/internal/engine/netx/httptransport/system.go index 13af6fd..442cf79 100644 --- a/internal/engine/netx/httptransport/system.go +++ b/internal/engine/netx/httptransport/system.go @@ -6,6 +6,10 @@ import ( // NewSystemTransport creates a new "system" HTTP transport. That is a transport // using the Go standard library with custom dialer and TLS dialer. +// +// Deprecation warning +// +// New code should use netxlite.NewHTTPTransport instead. func NewSystemTransport(config Config) RoundTripper { txp := http.DefaultTransport.(*http.Transport).Clone() txp.DialContext = config.Dialer.DialContext diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 189d5d0..243d0ae 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -1,6 +1,12 @@ package netxlite -import "net/http" +import ( + "context" + "crypto/tls" + "net" + "net/http" + "time" +) // HTTPTransport is an http.Transport-like structure. type HTTPTransport interface { @@ -60,3 +66,54 @@ func (txp *HTTPTransportLogger) logTrip(req *http.Request) (*http.Response, erro func (txp *HTTPTransportLogger) CloseIdleConnections() { txp.HTTPTransport.CloseIdleConnections() } + +// NewHTTPTransport creates a new HTTP transport using Go stdlib. +func NewHTTPTransport(dialer Dialer, tlsConfig *tls.Config, + handshaker TLSHandshaker) HTTPTransport { + txp := http.DefaultTransport.(*http.Transport).Clone() + dialer = &httpDialerWithReadTimeout{dialer} + txp.DialContext = dialer.DialContext + txp.DialTLSContext = (&TLSDialer{ + Config: tlsConfig, + Dialer: dialer, + TLSHandshaker: handshaker, + }).DialTLSContext + // Better for Cloudflare DNS and also better because we have less + // noisy events and we can better understand what happened. + txp.MaxConnsPerHost = 1 + // The following (1) reduces the number of headers that Go will + // automatically send for us and (2) ensures that we always receive + // back the true headers, such as Content-Length. This change is + // functional to OONI's goal of observing the network. + txp.DisableCompression = true + return txp +} + +// httpDialerWithReadTimeout enforces a read timeout for all HTTP +// connections. See https://github.com/ooni/probe/issues/1609. +type httpDialerWithReadTimeout struct { + Dialer +} + +// DialContext implements Dialer.DialContext. +func (d *httpDialerWithReadTimeout) DialContext( + ctx context.Context, network, address string) (net.Conn, error) { + conn, err := d.Dialer.DialContext(ctx, network, address) + if err != nil { + return nil, err + } + return &httpConnWithReadTimeout{conn}, nil +} + +// httpConnWithReadTimeout enforces a read timeout for all HTTP +// connections. See https://github.com/ooni/probe/issues/1609. +type httpConnWithReadTimeout struct { + net.Conn +} + +// Read implements Conn.Read. +func (c *httpConnWithReadTimeout) Read(b []byte) (int, error) { + c.Conn.SetReadDeadline(time.Now().Add(30 * time.Second)) + defer c.Conn.SetReadDeadline(time.Time{}) + return c.Conn.Read(b) +} diff --git a/internal/netxlite/http3.go b/internal/netxlite/http3.go index 6914f21..cec7b40 100644 --- a/internal/netxlite/http3.go +++ b/internal/netxlite/http3.go @@ -47,8 +47,13 @@ func NewHTTP3Transport( dialer QUICContextDialer, tlsConfig *tls.Config) HTTPTransport { return &http3Transport{ child: &http3.RoundTripper{ - Dial: (&http3Dialer{dialer}).dial, - TLSClientConfig: tlsConfig, + Dial: (&http3Dialer{dialer}).dial, + // The following (1) reduces the number of headers that Go will + // automatically send for us and (2) ensures that we always receive + // back the true headers, such as Content-Length. This change is + // functional to OONI's goal of observing the network. + DisableCompression: true, + TLSClientConfig: tlsConfig, }, } } diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index 5c27bd0..8bed6b6 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -2,8 +2,10 @@ package netxlite import ( "context" + "crypto/tls" "errors" "io" + "net" "net/http" "net/url" "strings" @@ -106,3 +108,43 @@ func TestHTTPTransportLoggerCloseIdleConnections(t *testing.T) { t.Fatal("not called") } } + +func TestHTTPTransportWorks(t *testing.T) { + d := &DialerResolver{ + Dialer: DefaultDialer, + Resolver: &net.Resolver{}, + } + th := &TLSHandshakerConfigurable{} + txp := NewHTTPTransport(d, &tls.Config{}, th) + client := &http.Client{Transport: txp} + resp, err := client.Get("https://www.google.com/robots.txt") + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + txp.CloseIdleConnections() +} + +func TestHTTPTransportWithFailingDialer(t *testing.T) { + expected := errors.New("mocked error") + d := &DialerResolver{ + Dialer: &netxmocks.Dialer{ + MockDialContext: func(ctx context.Context, + network, address string) (net.Conn, error) { + return nil, expected + }, + }, + Resolver: &net.Resolver{}, + } + th := &TLSHandshakerConfigurable{} + txp := NewHTTPTransport(d, &tls.Config{}, th) + client := &http.Client{Transport: txp} + resp, err := client.Get("https://www.google.com/robots.txt") + if !errors.Is(err, expected) { + t.Fatal("not the error we expected", err) + } + if resp != nil { + t.Fatal("expected non-nil response here") + } + txp.CloseIdleConnections() +}