diff --git a/internal/netxlite/httpfactory.go b/internal/netxlite/httpfactory.go index 3e69de9808..b39500271f 100644 --- a/internal/netxlite/httpfactory.go +++ b/internal/netxlite/httpfactory.go @@ -1,12 +1,96 @@ package netxlite import ( - "net/http" + "net/url" + oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" ) -// NewHTTPClient creates a new, wrapped HTTPClient using the given transport. -func NewHTTPClient(txp model.HTTPTransport) model.HTTPClient { - return WrapHTTPClient(&http.Client{Transport: txp}) +// HTTPTransportOption is an initialization option for [NewHTTPTransport]. +type HTTPTransportOption func(txp *oohttp.Transport) + +// NewHTTPTransport is the high-level factory to create a [model.HTTPTransport] using +// github.com/ooni/oohttp as the HTTP library with HTTP/1.1 and HTTP2 support. +// +// This transport is suitable for HTTP2 and HTTP/1.1 using any TLS +// library, including, e.g., github.com/ooni/oocrypto. +// +// This factory clones the default github.com/ooni/oohttp transport and +// configures the provided dialer and TLS dialer by setting the .DialContext +// and .DialTLSContext fields of the transport. We also wrap the provided +// dialers to address https://github.com/ooni/probe/issues/1609. +// +// Apart from that, the only non-default options set by this factory are these: +// +// 1. the .Proxy field is set to nil, so by default we DO NOT honour the +// HTTP_PROXY and HTTPS_PROXY environment variables, which is required if +// we want to use this code for measuring; +// +// 2. the .ForceAttemptHTTP2 field is set to true; +// +// 3. the .DisableCompression field is set to true, again required if we +// want to use this code for measuring, and we should make sure the defaults +// we're using are suitable for measuring, since the impact of making a +// mistake in measuring code is a data quality issue 😅. +// +// The returned transport supports logging and error wrapping because +// internally this function calls [WrapHTTPTransport] before we return. +// +// This factory is the RECOMMENDED way of creating a [model.HTTPTransport]. +func NewHTTPTransportWithOptions(logger model.Logger, + dialer model.Dialer, tlsDialer model.TLSDialer, options ...HTTPTransportOption) model.HTTPTransport { + // Using oohttp to support any TLS library. + txp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() + + // This wrapping ensures that we always have a timeout when we + // are using HTTP; see https://github.com/ooni/probe/issues/1609. + dialer = &httpDialerWithReadTimeout{dialer} + txp.DialContext = dialer.DialContext + tlsDialer = &httpTLSDialerWithReadTimeout{tlsDialer} + txp.DialTLSContext = tlsDialer.DialTLSContext + + // As documented, disable proxies and force HTTP/2 + txp.DisableCompression = true + txp.Proxy = nil + txp.ForceAttemptHTTP2 = true + + // Apply all the required options + for _, option := range options { + option(txp) + } + + // Return a fully wrapped HTTP transport + return WrapHTTPTransport(logger, &httpTransportConnectionsCloser{ + HTTPTransport: &httpTransportStdlib{&oohttp.StdlibTransport{Transport: txp}}, + Dialer: dialer, + TLSDialer: tlsDialer, + }) +} + +// HTTPTransportOptionProxyURL configures the transport to use the given proxyURL +// or disables proxying (already the default) if the proxyURL is nil. +func HTTPTransportOptionProxyURL(proxyURL *url.URL) HTTPTransportOption { + return func(txp *oohttp.Transport) { + txp.Proxy = func(r *oohttp.Request) (*url.URL, error) { + // "If Proxy is nil or returns a nil *URL, no proxy is used." + return proxyURL, nil + } + } +} + +// HTTPTransportOptionMaxConnsPerHost configures the .MaxConnPerHosts field, which +// otherwise uses the default set in github.com/ooni/oohttp. +func HTTPTransportOptionMaxConnsPerHost(value int) HTTPTransportOption { + return func(txp *oohttp.Transport) { + txp.MaxConnsPerHost = value + } +} + +// HTTPTransportOptionDisableCompression configures the .DisableCompression field, which +// otherwise is set to true, so that this code is ready for measuring out of the box. +func HTTPTransportOptionDisableCompression(value bool) HTTPTransportOption { + return func(txp *oohttp.Transport) { + txp.DisableCompression = value + } } diff --git a/internal/netxlite/httpfactory_test.go b/internal/netxlite/httpfactory_test.go new file mode 100644 index 0000000000..c02955c11c --- /dev/null +++ b/internal/netxlite/httpfactory_test.go @@ -0,0 +1,169 @@ +package netxlite + +import ( + "crypto/tls" + "net/url" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + oohttp "github.com/ooni/oohttp" + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/model" +) + +func TestNewHTTPTransportWithOptions(t *testing.T) { + + t.Run("make sure that we get the correct types and settings", func(t *testing.T) { + expectDialer := &mocks.Dialer{} + expectTLSDialer := &mocks.TLSDialer{} + expectLogger := model.DiscardLogger + txp := NewHTTPTransportWithOptions(expectLogger, expectDialer, expectTLSDialer) + + // undo the results of the netxlite.WrapTransport function + txpLogger := txp.(*httpTransportLogger) + if txpLogger.Logger != expectLogger { + t.Fatal("invalid logger") + } + txpErrWrapper := txpLogger.HTTPTransport.(*httpTransportErrWrapper) + + // make sure we correctly configured dialer and TLS dialer + txpCloser := txpErrWrapper.HTTPTransport.(*httpTransportConnectionsCloser) + timeoutDialer := txpCloser.Dialer.(*httpDialerWithReadTimeout) + childDialer := timeoutDialer.Dialer + if childDialer != expectDialer { + t.Fatal("invalid dialer") + } + timeoutTLSDialer := txpCloser.TLSDialer.(*httpTLSDialerWithReadTimeout) + childTLSDialer := timeoutTLSDialer.TLSDialer + if childTLSDialer != expectTLSDialer { + t.Fatal("invalid TLS dialer") + } + + // make sure there's the stdlib adapter + stdlibAdapter := txpCloser.HTTPTransport.(*httpTransportStdlib) + oohttpStdlibAdapter := stdlibAdapter.StdlibTransport + underlying := oohttpStdlibAdapter.Transport + + // now let's check that everything is configured as intended + expectedTxp := oohttp.DefaultTransport.(*oohttp.Transport).Clone() + diff := cmp.Diff( + expectedTxp, + underlying, + cmpopts.IgnoreUnexported(oohttp.Transport{}), + cmpopts.IgnoreUnexported(tls.Config{}), + cmpopts.IgnoreFields( + oohttp.Transport{}, + "DialContext", + "DialTLSContext", + "DisableCompression", + "Proxy", + "ForceAttemptHTTP2", + ), + ) + if diff != "" { + t.Fatal(diff) + } + + // finish checking by explicitly inspecting the fields we modify + if underlying.DialContext == nil { + t.Fatal("expected non-nil .DialContext") + } + if underlying.DialTLSContext == nil { + t.Fatal("expected non-nil .DialTLSContext") + } + if underlying.Proxy != nil { + t.Fatal("expected nil .Proxy") + } + if !underlying.ForceAttemptHTTP2 { + t.Fatal("expected true .ForceAttemptHTTP2") + } + if !underlying.DisableCompression { + t.Fatal("expected true .DisableCompression") + } + }) + + unwrap := func(txp model.HTTPTransport) *oohttp.Transport { + txpLogger := txp.(*httpTransportLogger) + txpErrWrapper := txpLogger.HTTPTransport.(*httpTransportErrWrapper) + txpCloser := txpErrWrapper.HTTPTransport.(*httpTransportConnectionsCloser) + stdlibAdapter := txpCloser.HTTPTransport.(*httpTransportStdlib) + oohttpStdlibAdapter := stdlibAdapter.StdlibTransport + return oohttpStdlibAdapter.Transport + } + + t.Run("make sure HTTPTransportOptionProxyURL is WAI", func(t *testing.T) { + runWithURL := func(expectedURL *url.URL) { + expectDialer := &mocks.Dialer{} + expectTLSDialer := &mocks.TLSDialer{} + expectLogger := model.DiscardLogger + txp := NewHTTPTransportWithOptions( + expectLogger, + expectDialer, + expectTLSDialer, + HTTPTransportOptionProxyURL(expectedURL), + ) + underlying := unwrap(txp) + if underlying.Proxy == nil { + t.Fatal("expected non-nil .Proxy") + } + got, err := underlying.Proxy(&oohttp.Request{}) + if err != nil { + t.Fatal(err) + } + if got != expectedURL { + t.Fatal("not the expected URL") + } + } + + runWithURL(&url.URL{}) + + runWithURL(nil) + }) + + t.Run("make sure HTTPTransportOptionMaxConnsPerHost is WAI", func(t *testing.T) { + runWithValue := func(expectedValue int) { + expectDialer := &mocks.Dialer{} + expectTLSDialer := &mocks.TLSDialer{} + expectLogger := model.DiscardLogger + txp := NewHTTPTransportWithOptions( + expectLogger, + expectDialer, + expectTLSDialer, + HTTPTransportOptionMaxConnsPerHost(expectedValue), + ) + underlying := unwrap(txp) + got := underlying.MaxConnsPerHost + if got != expectedValue { + t.Fatal("not the expected value") + } + } + + runWithValue(100) + + runWithValue(10) + }) + + t.Run("make sure HTTPTransportDisableCompression is WAI", func(t *testing.T) { + runWithValue := func(expectedValue bool) { + expectDialer := &mocks.Dialer{} + expectTLSDialer := &mocks.TLSDialer{} + expectLogger := model.DiscardLogger + txp := NewHTTPTransportWithOptions( + expectLogger, + expectDialer, + expectTLSDialer, + HTTPTransportOptionDisableCompression(expectedValue), + ) + underlying := unwrap(txp) + got := underlying.DisableCompression + if got != expectedValue { + t.Fatal("not the expected value") + } + } + + runWithValue(true) + + runWithValue(false) + }) +} diff --git a/internal/netxlite/httpquirks.go b/internal/netxlite/httpquirks.go index 7ab3af1f62..d4c26945e0 100644 --- a/internal/netxlite/httpquirks.go +++ b/internal/netxlite/httpquirks.go @@ -5,8 +5,12 @@ package netxlite // // Ideally, we should not modify this code or apply minimal and obvious changes. // +// TODO(https://github.com/ooni/probe/issues/2534) +// import ( + "net/http" + oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -129,3 +133,12 @@ func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient { func NewHTTPClientWithResolver(logger model.Logger, reso model.Resolver) model.HTTPClient { return NewHTTPClient(NewHTTPTransportWithResolver(logger, reso)) } + +// NewHTTPClient creates a new, wrapped HTTPClient using the given transport. +// +// This function behavior is QUIRKY because it does not configure a cookie jar, which +// is probably not the right thing to do in many cases, but legacy code MAY depend +// on this behavior. TODO(https://github.com/ooni/probe/issues/2534). +func NewHTTPClient(txp model.HTTPTransport) model.HTTPClient { + return WrapHTTPClient(&http.Client{Transport: txp}) +}