From fb871902ed9b385f68d932c514c4d4dc5bcad92b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 24 Jan 2024 08:03:58 +0100 Subject: [PATCH] fix(oohelperd,netemx): construct equivalent HTTPTransports (#1464) This diff modifies oohelperd and netemx to ensure we construct equivalent HTTPTransports. Let's show that this is actually the case. Let's start from the HTTP/1.1 and HTTP2 transport. This is what `oohelperd` does after this diff: ```Go NewHTTPClient: func(logger model.Logger) model.HTTPClient { // TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPTransportWithResolver has QUIRKS and // we should evaluate whether we can avoid using it here return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) }, ``` This is what `netemx` does after this diff: ```Go handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ) } ``` We're using the same (now public) `NewHTTPClientWithTransportFactory` function. So, what remains to be done to reach a QED is to show that this code called by `oohelperd`: ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) ``` is equivalent to this code called by `netemx`: ```Go func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ``` This is evident if we expand `netxlite.NewHTTPTransportWithResolver`, whose implementation is: ```Go func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) } ``` in fact, the following lines of code called from `oohelperd`: ```Go dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) ``` are equivalent to this `netemx` code: ```Go dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) ``` Modulo the fact that `netemx` code is using methods of the `*netxlite.Netx` structure rather than bare functions. Let's now inspect how we construct HTTP3. This is what `oohelperd` does: ```Go NewHTTP3Client: func(logger model.Logger) model.HTTPClient { return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) }, ``` This is what `netemx` does: ```Go handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) } ``` Because we're using the same `NewHTTPClientWithTransportFactory` factory, we need to show that `oohelperd`'s ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) ``` is equivalent to `netemx`'s ```Go return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) ``` To show that we need to expand `NewHTTP3TransportWithResolver`, which reads: ```Go func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) } ``` And then we can conclude that we're good because the code invoked by `oohelperd`: ```Go qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) ``` is equivalent to the code invoked by `netemx`: ```Go qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) ``` modulo the fact that `netemx` is using methods defined by the `netx` object. Extracted from https://github.com/ooni/probe-cli/pull/1462. Part of https://github.com/ooni/probe/issues/2652. Part of https://github.com/ooni/probe/issues/1517. --- internal/netemx/oohelperd.go | 41 +++++++++++++++-------------------- internal/oohelperd/handler.go | 9 ++++---- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/internal/netemx/oohelperd.go b/internal/netemx/oohelperd.go index 9baa2588fb..4947bd951e 100644 --- a/internal/netemx/oohelperd.go +++ b/internal/netemx/oohelperd.go @@ -3,14 +3,12 @@ package netemx import ( "fmt" "net/http" - "net/http/cookiejar" "github.com/ooni/netem" "github.com/ooni/probe-cli/v3/internal/logx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" "github.com/ooni/probe-cli/v3/internal/oohelperd" - "golang.org/x/net/publicsuffix" ) // OOHelperDFactory is the factory to create an [http.Handler] implementing the OONI Web Connectivity @@ -21,7 +19,7 @@ var _ HTTPHandlerFactory = &OOHelperDFactory{} // NewHandler implements QAEnvHTTPHandlerFactory.NewHandler. func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem.UNetStack) http.Handler { - netx := netxlite.Netx{Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: unet}} + netx := &netxlite.Netx{Underlying: &netxlite.NetemUnderlyingNetworkAdapter{UNet: unet}} handler := oohelperd.NewHandler() handler.BaseLogger = &logx.PrefixLogger{ @@ -46,29 +44,26 @@ func (f *OOHelperDFactory) NewHandler(env NetStackServerFactoryEnv, unet *netem. } handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient { - cookieJar, _ := cookiejar.New(&cookiejar.Options{ - PublicSuffixList: publicsuffix.List, - }) - // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransportStdlib is QUIRKY but we probably - // don't care about using a QUIRKY function here - return &http.Client{ - Transport: netx.NewHTTPTransportStdlib(logger), - CheckRedirect: nil, - Jar: cookieJar, - Timeout: 0, - } + return oohelperd.NewHTTPClientWithTransportFactory( + logger, + func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { + dialer := netx.NewDialerWithResolver(dl, r) + tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) + // TODO(https://github.com/ooni/probe/issues/2534): NewHTTPTransport is QUIRKY but + // we probably don't care about using a QUIRKY function here + return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) + }, + ) } handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient { - cookieJar, _ := cookiejar.New(&cookiejar.Options{ - PublicSuffixList: publicsuffix.List, - }) - return &http.Client{ - Transport: netx.NewHTTP3TransportStdlib(logger), - CheckRedirect: nil, - Jar: cookieJar, - Timeout: 0, - } + return oohelperd.NewHTTPClientWithTransportFactory( + logger, + func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { + qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) + return netxlite.NewHTTP3Transport(dl, qd, nil) + }, + ) } return handler diff --git a/internal/oohelperd/handler.go b/internal/oohelperd/handler.go index a18a2da113..989e3a4daa 100644 --- a/internal/oohelperd/handler.go +++ b/internal/oohelperd/handler.go @@ -80,14 +80,14 @@ func NewHandler() *Handler { NewHTTPClient: func(logger model.Logger) model.HTTPClient { // TODO(https://github.com/ooni/probe/issues/2534): the NewHTTPTransportWithResolver has QUIRKS and // we should evaluate whether we can avoid using it here - return newHTTPClientWithTransportFactory( + return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) }, NewHTTP3Client: func(logger model.Logger) model.HTTPClient { - return newHTTPClientWithTransportFactory( + return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) @@ -218,8 +218,9 @@ func newCookieJar() *cookiejar.Jar { })) } -// newHTTPClientWithTransportFactory creates a new HTTP client. -func newHTTPClientWithTransportFactory( +// NewHTTPClientWithTransportFactory creates a new HTTP client +// using the given [model.HTTPTransport] factory. +func NewHTTPClientWithTransportFactory( logger model.Logger, txpFactory func(model.DebugLogger, model.Resolver) model.HTTPTransport, ) model.HTTPClient {