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.

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)
 }

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

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)
+}

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,
+			Dial: (&http3Dialer{dialer}).dial,
-			TLSClientConfig: tlsConfig,
+			// 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,
 		},
 	}
 }

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()
+}