feat(enginenetx): use the new HTTPSDialer (#1295)

This commit refactors how we construct the *Network used by the OONI
engine so that the HTTPSTransport we use relies on the new HTTPSDialer
as opposed to using netxlite's TLS dialing facilities.

This new HTTPSDialer has been specifically written to integrate TCP and
TLS dialing and facilitate circumvention.

The current implementation uses a "null" policy which makes it roughly
equivalent to the previous behavior, at least functionally, tough we are
now doing a variation of happy eyeballs where to try to ~aggressively
dial more connections as we see that previous connections fail to dial.

Specifically, if a TLS connection has not succeded within 300
milliseconds (probably a low value?), then we attempt dialing with
another available IP address.

The new code documents extensively what we are doing and some current
limitations, including references to the tracking issues.

This also diff fixes two issues we discovered when integrating the
HTTPSDialer with the rest of OONI Probe:

1. #1295 (comment)

2. #1295 (comment)

While there, use `log.Log` more frequently in testing to interpret what
is going wrong.

Part of ooni/probe#2531

internal/enginenetx/httpsdialer_internal_test.go

@@ -20,10 +20,10 @@ func TestHTTPSDialerTacticsEmitter(t *testing.T) {
 		hd := &HTTPSDialer{
 			idGenerator: &atomic.Int64{},
 			logger:      model.DiscardLogger,
+			netx:        &netxlite.Netx{Underlying: nil}, // nil means: use netxlite's singleton
 			policy:      &HTTPSDialerNullPolicy{},
 			resolver:    netxlite.NewStdlibResolver(model.DiscardLogger),
 			rootCAs:     netxlite.NewMozillaCertPool(),
-			unet:        &netxlite.DefaultTProxy{},
 			wg:          &sync.WaitGroup{},
 		}
 

internal/enginenetx/httpsdialer_test.go

@@ -2,14 +2,17 @@ package enginenetx_test
 
 import (
 	"context"
+	"crypto/x509"
 	"encoding/json"
+	"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/enginenetx"
+	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/netemx"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
@@ -63,7 +66,7 @@ func (st *httpsDialerCancelingContextStatsTracker) OnSuccess(tactic *enginenetx.
 	}
 }
 
-func TestHTTPSDialerWAI(t *testing.T) {
+func TestHTTPSDialerNetemQA(t *testing.T) {
 	// testcase is a test case implemented by this function
 	type testcase struct {
 		// name is the name of the test case
@@ -383,10 +386,10 @@ func TestHTTPSDialerWAI(t *testing.T) {
 				// create the TLS dialer
 				dialer := enginenetx.NewHTTPSDialer(
 					log.Log,
+					netx,
 					tc.policy,
 					resolver,
 					tc.stats,
-					unet,
 				)
 				defer dialer.CloseIdleConnections()
 
@@ -603,3 +606,48 @@ func TestHTTPSDialerTactic(t *testing.T) {
 		}
 	})
 }
+
+func TestHTTPSDialerHostNetworkQA(t *testing.T) {
+	t.Run("HTTPSDialerNullPolicy allows connecting to https://127.0.0.1/ using a custom CA", func(t *testing.T) {
+		ca := netem.MustNewCA()
+		server := testingx.MustNewHTTPServerTLS(
+			testingx.HTTPHandlerBlockpage451(),
+			ca,
+			"server.local",
+		)
+		defer server.Close()
+
+		tproxy := &netxlite.DefaultTProxy{}
+
+		// The resolver we're creating here reproduces the test case described by
+		// https://github.com/ooni/probe-cli/pull/1295#issuecomment-1731243994
+		resolver := netxlite.MaybeWrapWithBogonResolver(true, netxlite.NewStdlibResolver(log.Log))
+
+		httpsDialer := enginenetx.NewHTTPSDialer(
+			log.Log,
+			&netxlite.Netx{Underlying: &mocks.UnderlyingNetwork{
+				MockDefaultCertPool: func() *x509.CertPool {
+					return ca.DefaultCertPool() // just override the CA
+				},
+				MockDialTimeout:                tproxy.DialTimeout,
+				MockDialContext:                tproxy.DialContext,
+				MockListenTCP:                  tproxy.ListenTCP,
+				MockListenUDP:                  tproxy.ListenUDP,
+				MockGetaddrinfoLookupANY:       tproxy.GetaddrinfoLookupANY,
+				MockGetaddrinfoResolverNetwork: tproxy.GetaddrinfoResolverNetwork,
+			}},
+			&enginenetx.HTTPSDialerNullPolicy{},
+			resolver,
+			&enginenetx.HTTPSDialerNullStatsTracker{},
+		)
+
+		URL := runtimex.Try1(url.Parse(server.URL))
+
+		ctx := context.Background()
+		tlsConn, err := httpsDialer.DialTLSContext(ctx, "tcp", URL.Host)
+		if err != nil {
+			t.Fatal(err)
+		}
+		tlsConn.Close()
+	})
+}

internal/enginenetx/httpsdialercore.go

@@ -113,6 +113,9 @@ type HTTPSDialer struct {
 	// logger is the logger to use.
 	logger model.Logger
 
+	// netx is the [*netxlite.Netx] to use.
+	netx *netxlite.Netx
+
 	// policy defines the dialing policy to use.
 	policy HTTPSDialerPolicy
 
@@ -125,9 +128,6 @@ type HTTPSDialer struct {
 	// stats tracks what happens while dialing.
 	stats HTTPSDialerStatsTracker
 
-	// unet is the underlying network.
-	unet model.UnderlyingNetwork
-
 	// wg is the wait group for knowing when all goroutines
 	// started in the background joined (for testing).
 	wg *sync.WaitGroup
@@ -139,34 +139,34 @@ type HTTPSDialer struct {
 //
 // - logger is the logger to use for logging;
 //
+// - netx is the [*netxlite.Netx] to use;
+//
 // - policy defines the dialer policy;
 //
 // - resolver is the resolver to use;
 //
-// - stats tracks what happens while we're dialing;
-//
-// - unet is the underlying network to use.
+// - stats tracks what happens while we're dialing.
 //
 // The returned [*HTTPSDialer] would use the underlying network's
 // DefaultCertPool to create and cache the cert pool to use.
 func NewHTTPSDialer(
 	logger model.Logger,
+	netx *netxlite.Netx,
 	policy HTTPSDialerPolicy,
 	resolver model.Resolver,
 	stats HTTPSDialerStatsTracker,
-	unet model.UnderlyingNetwork,
 ) *HTTPSDialer {
 	return &HTTPSDialer{
 		idGenerator: &atomic.Int64{},
 		logger: &logx.PrefixLogger{
 			Prefix: "HTTPSDialer: ",
 			Logger: logger,
 		},
+		netx:     netx,
 		policy:   policy,
 		resolver: resolver,
-		rootCAs:  unet.DefaultCertPool(),
+		rootCAs:  netx.MaybeCustomUnderlyingNetwork().Get().DefaultCertPool(),
 		stats:    stats,
-		unet:     unet,
 		wg:       &sync.WaitGroup{},
 	}
 }
@@ -205,12 +205,16 @@ func (hd *HTTPSDialer) DialTLSContext(ctx context.Context, network string, endpo
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
+	// See https://github.com/ooni/probe-cli/pull/1295#issuecomment-1731243994 for context
+	// on why here we MUST make sure we short-circuit IP addresses.
+	resoWithShortCircuit := &netxlite.ResolverShortCircuitIPAddr{Resolver: hd.resolver}
+
 	logger := &logx.PrefixLogger{
 		Prefix: fmt.Sprintf("[#%d] ", hd.idGenerator.Add(1)),
 		Logger: hd.logger,
 	}
 	ol := logx.NewOperationLogger(logger, "LookupTactics: %s", net.JoinHostPort(hostname, port))
-	tactics, err := hd.policy.LookupTactics(ctx, hostname, port, hd.resolver)
+	tactics, err := hd.policy.LookupTactics(ctx, hostname, port, resoWithShortCircuit)
 	if err != nil {
 		ol.Stop(err)
 		return nil, err
@@ -332,12 +336,9 @@ func (hd *HTTPSDialer) dialTLS(
 	// tell the tactic that we're starting
 	hd.stats.OnStarting(tactic)
 
-	// create a network abstraction using the underlying network
-	netx := &netxlite.Netx{Underlying: hd.unet}
-
 	// create dialer and establish TCP connection
 	ol := logx.NewOperationLogger(logger, "TCPConnect %s", tactic.Endpoint)
-	dialer := netx.NewDialerWithoutResolver(logger)
+	dialer := hd.netx.NewDialerWithoutResolver(logger)
 	tcpConn, err := dialer.DialContext(ctx, "tcp", tactic.Endpoint)
 	ol.Stop(err)
 
@@ -363,7 +364,7 @@ func (hd *HTTPSDialer) dialTLS(
 		tlsConfig.ServerName,
 		tlsConfig.NextProtos,
 	)
-	thx := netx.NewTLSHandshakerStdlib(logger)
+	thx := hd.netx.NewTLSHandshakerStdlib(logger)
 	tlsConn, err := thx.Handshake(ctx, tcpConn, tlsConfig)
 	ol.Stop(err)
 

internal/enginenetx/network.go

@@ -72,14 +72,58 @@ func NewNetwork(
 	proxyURL *url.URL,
 	resolver model.Resolver,
 ) *Network {
+	// Create a dialer ONLY used for dialing unencrypted TCP connections. The common use
+	// case of this Network is to dial encrypted connections. For this reason, here it is
+	// reasonably fine to use the legacy sequential dialer implemented in netxlite.
 	dialer := netxlite.NewDialerWithResolver(logger, resolver)
-	handshaker := netxlite.NewTLSHandshakerStdlib(logger)
-	tlsDialer := netxlite.NewTLSDialer(dialer, handshaker)
+
+	// Create a TLS dialer ONLY used for dialing TLS connections. This dialer will use
+	// happy-eyeballs and possibly custom policies for dialing TLS connections.
+	//
+	// Additionally, please note the following limitations (to be overcome through
+	// future refactoring of this func):
+	//
+	// - for now, we're using a "null" policy that does happy eyeballs but otherwise
+	// does not use beacons or other TLS handshake tricks;
+	//
+	// - for now, we're using a "null" stats tracker, meaning we don't track stats.
+	httpsDialer := NewHTTPSDialer(
+		logger,
+		&netxlite.Netx{Underlying: nil}, // nil means using netxlite's singleton
+		&HTTPSDialerNullPolicy{},
+		resolver,
+		&HTTPSDialerNullStatsTracker{},
+	)
+
+	// Here we're creating a "new style" HTTPS transport, which has less
+	// restrictions compared to the "old style" one.
+	//
+	// Note that:
+	//
+	// - we're enabling compression, which is desiredable since this transport
+	// is not made for measuring and compression is good(TM);
+	//
+	// - if proxyURL is nil, the proxy option is equivalent to disabling
+	// the proxy, otherwise it means that we're using the ooni/oohttp library
+	// to dial for proxies, which has some restrictions.
+	//
+	// In particular, the returned transport uses dialer for dialing with
+	// cleartext proxies (e.g., socks5 and http) and httpsDialer for dialing
+	// with encrypted proxies (e.g., https). After this has happened,
+	// the code currently falls back to using the standard library's tls
+	// client code for establishing TLS connections over the proxy. The main
+	// implication here is that we're not using our custom mozilla CA for
+	// validating TLS certificates, rather we're using the system's cert store.
+	//
+	// Fixing this issue is TODO(https://github.com/ooni/probe/issues/2536).
 	txp := netxlite.NewHTTPTransportWithOptions(
-		logger, dialer, tlsDialer,
+		logger, dialer, httpsDialer,
 		netxlite.HTTPTransportOptionDisableCompression(false),
-		netxlite.HTTPTransportOptionProxyURL(proxyURL), // nil implies "no proxy"
+		netxlite.HTTPTransportOptionProxyURL(proxyURL),
 	)
+
+	// Make sure we count the bytes sent and received as part of the session
 	txp = bytecounter.WrapHTTPTransport(txp, counter)
+
 	return &Network{txp}
 }

internal/enginenetx/network_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/enginenetx"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/measurexlite"
-	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netemx"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/testingsocks5"
@@ -29,9 +28,9 @@ func TestNetworkQA(t *testing.T) {
 			txp := enginenetx.NewNetwork(
 				bytecounter.New(),
 				&kvstore.Memory{},
-				model.DiscardLogger,
+				log.Log,
 				nil,
-				netxlite.NewStdlibResolver(model.DiscardLogger),
+				netxlite.NewStdlibResolver(log.Log),
 			)
 			client := txp.NewHTTPClient()
 			resp, err := client.Get("https://www.example.com/")
@@ -66,13 +65,13 @@ func TestNetworkQA(t *testing.T) {
 			txp := enginenetx.NewNetwork(
 				bytecounter.New(),
 				&kvstore.Memory{},
-				model.DiscardLogger,
+				log.Log,
 				&url.URL{
 					Scheme: "socks5",
 					Host:   net.JoinHostPort(env.ClientStack.IPAddress(), "9050"),
 					Path:   "/",
 				},
-				netxlite.NewStdlibResolver(model.DiscardLogger),
+				netxlite.NewStdlibResolver(log.Log),
 			)
 			client := txp.NewHTTPClient()
 
@@ -135,13 +134,13 @@ func TestNetworkQA(t *testing.T) {
 			txp := enginenetx.NewNetwork(
 				bytecounter.New(),
 				&kvstore.Memory{},
-				model.DiscardLogger,
+				log.Log,
 				&url.URL{
 					Scheme: "http",
 					Host:   net.JoinHostPort(env.ClientStack.IPAddress(), "8080"),
 					Path:   "/",
 				},
-				netxlite.NewStdlibResolver(model.DiscardLogger),
+				netxlite.NewStdlibResolver(log.Log),
 			)
 			client := txp.NewHTTPClient()
 
@@ -206,13 +205,13 @@ func TestNetworkQA(t *testing.T) {
 			txp := enginenetx.NewNetwork(
 				bytecounter.New(),
 				&kvstore.Memory{},
-				model.DiscardLogger,
+				log.Log,
 				&url.URL{
 					Scheme: "https",
 					Host:   net.JoinHostPort(env.ClientStack.IPAddress(), "4443"),
 					Path:   "/",
 				},
-				netxlite.NewStdlibResolver(model.DiscardLogger),
+				netxlite.NewStdlibResolver(log.Log),
 			)
 			client := txp.NewHTTPClient()
 
@@ -261,9 +260,9 @@ func TestNetworkQA(t *testing.T) {
 		txp := enginenetx.NewNetwork(
 			bytecounter.New(),
 			&kvstore.Memory{},
-			model.DiscardLogger,
+			log.Log,
 			nil,
-			netxlite.NewStdlibResolver(model.DiscardLogger),
+			netxlite.NewStdlibResolver(log.Log),
 		)
 		client := txp.NewHTTPClient()
 		if client.Jar == nil {

internal/model/netx.go

@@ -133,8 +133,17 @@ type DialerWrapper interface {
 
 // SimpleDialer establishes network connections.
 type SimpleDialer interface {
-	// DialContext behaves like net.Dialer.DialContext.
-	DialContext(ctx context.Context, network, address string) (net.Conn, error)
+	// DialContext creates a new TCP/UDP connection like [net.DialContext] would do.
+	//
+	// The endpoint is an endpoint like the ones accepted by [net.DialContext]. For example,
+	// x.org:443, 130.192.91.211:443 and [::1]:443. Note that IPv6 addrs are quoted.
+	//
+	// This function MUST gracefully handle the case where the endpoint contains an IPv4
+	// or IPv6 address by skipping DNS resolution and directly using the endpoint.
+	//
+	// See https://github.com/ooni/probe-cli/pull/1295#issuecomment-1731243994 for more
+	// details on why DialContext MUST do that.
+	DialContext(ctx context.Context, network, endpoint string) (net.Conn, error)
 }
 
 // Dialer is a SimpleDialer with the possibility of closing open connections.
@@ -258,7 +267,9 @@ type QUICDialer interface {
 
 // Resolver performs domain name resolutions.
 type Resolver interface {
-	// LookupHost behaves like net.Resolver.LookupHost.
+	// LookupHost resolves the given hostname to IP addreses. This function SHOULD handle the
+	// case in which hostname is an IP address by returning a 1-element list containing the hostname,
+	// for consistency with [net.Resolver] behaviour.
 	LookupHost(ctx context.Context, hostname string) (addrs []string, err error)
 
 	// Network returns the resolver type. It should be one of:
@@ -315,6 +326,15 @@ type TLSDialer interface {
 
 	// DialTLSContext dials a TLS connection. This method will always return
 	// to you a oohttp.TLSConn, so you can always safely cast to it.
+	//
+	// The endpoint is an endpoint like the ones accepted by [net.DialContext]. For example,
+	// x.org:443, 130.192.91.211:443 and [::1]:443. Note that IPv6 addrs are quoted.
+	//
+	// This function MUST gracefully handle the case where the endpoint contains an IPv4
+	// or IPv6 address by skipping DNS resolution and directly using the endpoint.
+	//
+	// See https://github.com/ooni/probe-cli/pull/1295#issuecomment-1731243994 for more
+	// details on why DialTLSContext MUST do that.
 	DialTLSContext(ctx context.Context, network, address string) (net.Conn, error)
 }