fix(enginenetx): use dns policy with proxy (+renames) (ooni#1315)

This diff modifies how we construct a `*Network` to use a very simple
DNS-only policy when using a proxy.

We don't need to try anything fancy when we're using a proxy because we
assume the proxy knows how to do circumvention and we don't to get in
the proxy way.

While there, recognize that the static policy was named very similarly
to the stats policy, so rename the former user policy, such that there
is more lexicographical difference.

Part of ooni/probe#2531

internal/enginenetx/httpsdialer.go

@@ -71,7 +71,7 @@ func (dt *httpsDialerTactic) String() string {
 // `sni=` before the SNI and `verify=` before the verify hostname.
 //
 // We should be careful not to change this format unless we also change the
-// format version used by static policies and by the state management.
+// format version used by user policies and by the state management.
 func (dt *httpsDialerTactic) tacticSummaryKey() string {
 	return fmt.Sprintf(
 		"%v sni=%v verify=%v",

internal/enginenetx/network.go

@@ -94,15 +94,12 @@ func NewNetwork(
 	// Create manager for keeping track of statistics
 	stats := newStatsManager(kvStore, logger)
 
-	// TODO(bassosimone): the documentation says we MAY avoid specific policies
-	// when using a proxy, should we actually implement that?
-
 	// Create a TLS dialer ONLY used for dialing TLS connections. This dialer will use
 	// happy-eyeballs and possibly custom policies for dialing TLS connections.
 	httpsDialer := newHTTPSDialer(
 		logger,
 		&netxlite.Netx{Underlying: nil}, // nil means using netxlite's singleton
-		newHTTPSDialerPolicy(kvStore, logger, resolver, stats),
+		newHTTPSDialerPolicy(kvStore, logger, proxyURL, resolver, stats),
 		stats,
 	)
 
@@ -148,17 +145,24 @@ func NewNetwork(
 func newHTTPSDialerPolicy(
 	kvStore model.KeyValueStore,
 	logger model.Logger,
+	proxyURL *url.URL, // optional!
 	resolver model.Resolver,
 	stats *statsManager,
 ) httpsDialerPolicy {
+	// in case there's a proxy URL, we're going to trust the proxy to do the right thing and
+	// know what it's doing, hence we'll have a very simple DNS policy
+	if proxyURL != nil {
+		return &dnsPolicy{logger, resolver}
+	}
+
 	// create a composed fallback TLS dialer policy
 	fallback := &statsPolicy{
 		Fallback: &beaconsPolicy{Fallback: &dnsPolicy{logger, resolver}},
 		Stats:    stats,
 	}
 
 	// make sure we honor a user-provided policy
-	policy, err := newStaticPolicy(kvStore, fallback)
+	policy, err := newUserPolicy(kvStore, fallback)
 	if err != nil {
 		return fallback
 	}

internal/enginenetx/network_internal_test.go

@@ -3,6 +3,8 @@ package enginenetx
 import (
 	"context"
 	"encoding/json"
+	"fmt"
+	"net/url"
 	"sync"
 	"testing"
 
@@ -110,7 +112,7 @@ func TestNetworkUnit(t *testing.T) {
 			{
 				name: "when there's a user-provided policy",
 				kvStore: func() model.KeyValueStore {
-					policy := &staticPolicyRoot{
+					policy := &userPolicyRoot{
 						DomainEndpoints: map[string][]*httpsDialerTactic{
 							"www.example.com:443": {{
 								Address:        netemx.AddressApiOONIIo,
@@ -120,11 +122,11 @@ func TestNetworkUnit(t *testing.T) {
 								VerifyHostname: "api.ooni.io",
 							}},
 						},
-						Version: staticPolicyVersion,
+						Version: userPolicyVersion,
 					}
 					rawPolicy := runtimex.Try1(json.Marshal(policy))
 					kvStore := &kvstore.Memory{}
-					runtimex.Try0(kvStore.Set(staticPolicyKey, rawPolicy))
+					runtimex.Try0(kvStore.Set(userPolicyKey, rawPolicy))
 					return kvStore
 				},
 				expectStatus: 404,
@@ -168,3 +170,86 @@ func TestNetworkUnit(t *testing.T) {
 		}
 	})
 }
+
+// Make sure we get the correct policy type depending on how we call newHTTPSDialerPolicy
+func TestNewHTTPSDialerPolicy(t *testing.T) {
+	// testcase is a test case implemented by this function
+	type testcase struct {
+		// name is the name of the test case
+		name string
+
+		// kvStore constructs the kvstore to use
+		kvStore func() model.KeyValueStore
+
+		// proxyURL is the OPTIONAL proxy URL to use
+		proxyURL *url.URL
+
+		// expectType is the string representation of the
+		// type constructed using these params
+		expectType string
+	}
+
+	minimalUserPolicy := []byte(`{"Version":3}`)
+
+	cases := []testcase{{
+		name: "when there is a proxy URL and there is a user policy",
+		kvStore: func() model.KeyValueStore {
+			store := &kvstore.Memory{}
+			// this policy is mostly empty but it's enough to load
+			runtimex.Try0(store.Set(userPolicyKey, minimalUserPolicy))
+			return store
+		},
+		proxyURL: &url.URL{
+			Scheme: "socks5",
+			Host:   "127.0.0.1:9050",
+			Path:   "/",
+		},
+		expectType: "*enginenetx.dnsPolicy",
+	}, {
+		name: "when there is a proxy URL and there is no user policy",
+		kvStore: func() model.KeyValueStore {
+			return &kvstore.Memory{}
+		},
+		proxyURL: &url.URL{
+			Scheme: "socks5",
+			Host:   "127.0.0.1:9050",
+			Path:   "/",
+		},
+		expectType: "*enginenetx.dnsPolicy",
+	}, {
+		name: "when there is no proxy URL and there is a user policy",
+		kvStore: func() model.KeyValueStore {
+			store := &kvstore.Memory{}
+			// this policy is mostly empty but it's enough to load
+			runtimex.Try0(store.Set(userPolicyKey, minimalUserPolicy))
+			return store
+		},
+		proxyURL:   nil,
+		expectType: "*enginenetx.userPolicy",
+	}, {
+		name: "when there is no proxy URL and there is no user policy",
+		kvStore: func() model.KeyValueStore {
+			return &kvstore.Memory{}
+		},
+		proxyURL:   nil,
+		expectType: "*enginenetx.statsPolicy",
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+
+			p := newHTTPSDialerPolicy(
+				tc.kvStore(),
+				model.DiscardLogger,
+				tc.proxyURL,       // possibly nil
+				&mocks.Resolver{}, // we are not using `out` so it does not matter
+				&statsManager{},   // ditto
+			)
+
+			got := fmt.Sprintf("%T", p)
+			if diff := cmp.Diff(tc.expectType, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}

internal/enginenetx/statsmanager_test.go

@@ -58,7 +58,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 			name: "with TCP connect failure",
 			URL:  "https://api.ooni.io/",
 			initialPolicy: func() []byte {
-				p0 := &staticPolicyRoot{
+				p0 := &userPolicyRoot{
 					DomainEndpoints: map[string][]*httpsDialerTactic{
 						// This policy has a different SNI and VerifyHostname, which gives
 						// us confidence that the stats are using the latter
@@ -70,7 +70,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 							VerifyHostname: "api.ooni.io",
 						}},
 					},
-					Version: staticPolicyVersion,
+					Version: userPolicyVersion,
 				}
 				return runtimex.Try1(json.Marshal(p0))
 			},
@@ -110,7 +110,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 			name: "with TLS handshake failure",
 			URL:  "https://api.ooni.io/",
 			initialPolicy: func() []byte {
-				p0 := &staticPolicyRoot{
+				p0 := &userPolicyRoot{
 					DomainEndpoints: map[string][]*httpsDialerTactic{
 						// This policy has a different SNI and VerifyHostname, which gives
 						// us confidence that the stats are using the latter
@@ -122,7 +122,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 							VerifyHostname: "api.ooni.io",
 						}},
 					},
-					Version: staticPolicyVersion,
+					Version: userPolicyVersion,
 				}
 				return runtimex.Try1(json.Marshal(p0))
 			},
@@ -161,7 +161,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 			name: "with TLS verification failure",
 			URL:  "https://api.ooni.io/",
 			initialPolicy: func() []byte {
-				p0 := &staticPolicyRoot{
+				p0 := &userPolicyRoot{
 					DomainEndpoints: map[string][]*httpsDialerTactic{
 						// This policy has a different SNI and VerifyHostname, which gives
 						// us confidence that the stats are using the latter
@@ -173,7 +173,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 							VerifyHostname: "api.ooni.io",
 						}},
 					},
-					Version: staticPolicyVersion,
+					Version: userPolicyVersion,
 				}
 				return runtimex.Try1(json.Marshal(p0))
 			},
@@ -219,7 +219,7 @@ func TestNetworkCollectsStats(t *testing.T) {
 
 			initialPolicy := tc.initialPolicy()
 			t.Logf("initialPolicy: %s", string(initialPolicy))
-			if err := kvStore.Set(staticPolicyKey, initialPolicy); err != nil {
+			if err := kvStore.Set(userPolicyKey, initialPolicy); err != nil {
 				t.Fatal(err)
 			}
 

internal/enginenetx/staticpolicy.go → internal/enginenetx/userpolicy.go

@@ -1,8 +1,8 @@
 package enginenetx
 
 //
-// static policy - the possibility of loading a static policy from a JSON
-// document named `httpsdialerstatic.conf` in $OONI_HOME/engine that contains
+// user policy - the possibility of loading a user policy from a JSON
+// document named `httpsdialer.conf` in $OONI_HOME/engine that contains
 // a specific policy for TLS dialing for specific endpoints.
 //
 // This policy helps a lot with exploration and experimentation.
@@ -18,88 +18,88 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
-// staticPolicy is an [httpsDialerPolicy] incorporating verbatim
-// a static policy loaded from the engine's key-value store.
+// userPolicy is an [httpsDialerPolicy] incorporating verbatim
+// a user policy loaded from the engine's key-value store.
 //
 // This policy is very useful for exploration and experimentation.
-type staticPolicy struct {
-	// Fallback is the fallback policy in case the static one does not
+type userPolicy struct {
+	// Fallback is the fallback policy in case the user one does not
 	// contain a rule for a specific domain.
 	Fallback httpsDialerPolicy
 
-	// Root is the root of the statically loaded policy.
-	Root *staticPolicyRoot
+	// Root is the root of the user policy loaded from disk.
+	Root *userPolicyRoot
 }
 
-// staticPolicyKey is the kvstore key used to retrieve the static policy.
-const staticPolicyKey = "httpsdialerstatic.conf"
+// userPolicyKey is the kvstore key used to retrieve the user policy.
+const userPolicyKey = "httpsdialer.conf"
 
-// errStaticPolicyWrongVersion means that the static policy document has the wrong version number.
-var errStaticPolicyWrongVersion = errors.New("wrong static policy version")
+// errUserPolicyWrongVersion means that the user policy document has the wrong version number.
+var errUserPolicyWrongVersion = errors.New("wrong user policy version")
 
-// newStaticPolicy attempts to constructs a static policy using a given fallback
+// newUserPolicy attempts to constructs a user policy using a given fallback
 // policy and either returns a good policy or an error. The typical error case is the one
-// in which there's no httpsDialerStaticPolicyKey in the key-value store.
-func newStaticPolicy(
-	kvStore model.KeyValueStore, fallback httpsDialerPolicy) (*staticPolicy, error) {
-	// attempt to read the static policy bytes from the kvstore
-	data, err := kvStore.Get(staticPolicyKey)
+// in which there's no httpsDialerUserPolicyKey in the key-value store.
+func newUserPolicy(
+	kvStore model.KeyValueStore, fallback httpsDialerPolicy) (*userPolicy, error) {
+	// attempt to read the user policy bytes from the kvstore
+	data, err := kvStore.Get(userPolicyKey)
 	if err != nil {
 		return nil, err
 	}
 
-	// attempt to parse the static policy using human-readable JSON
-	var root staticPolicyRoot
+	// attempt to parse the user policy using human-readable JSON
+	var root userPolicyRoot
 	if err := hujsonx.Unmarshal(data, &root); err != nil {
 		return nil, err
 	}
 
 	// make sure the version is OK
-	if root.Version != staticPolicyVersion {
+	if root.Version != userPolicyVersion {
 		err := fmt.Errorf(
 			"%s: %w: expected=%d got=%d",
-			staticPolicyKey,
-			errStaticPolicyWrongVersion,
-			staticPolicyVersion,
+			userPolicyKey,
+			errUserPolicyWrongVersion,
+			userPolicyVersion,
 			root.Version,
 		)
 		return nil, err
 	}
 
-	out := &staticPolicy{
+	out := &userPolicy{
 		Fallback: fallback,
 		Root:     &root,
 	}
 	return out, nil
 }
 
-// staticPolicyVersion is the current version of the static policy file.
-const staticPolicyVersion = 3
+// userPolicyVersion is the current version of the user policy file.
+const userPolicyVersion = 3
 
-// staticPolicyRoot is the root of a statically loaded policy.
-type staticPolicyRoot struct {
+// userPolicyRoot is the root of the user policy.
+type userPolicyRoot struct {
 	// DomainEndpoints maps each domain endpoint to its policies.
 	DomainEndpoints map[string][]*httpsDialerTactic
 
 	// Version is the data structure version.
 	Version int
 }
 
-var _ httpsDialerPolicy = &staticPolicy{}
+var _ httpsDialerPolicy = &userPolicy{}
 
 // LookupTactics implements httpsDialerPolicy.
-func (ldp *staticPolicy) LookupTactics(
+func (ldp *userPolicy) LookupTactics(
 	ctx context.Context, domain string, port string) <-chan *httpsDialerTactic {
 	// check whether an entry exists in the user-provided map, which MAY be nil
-	// if/when the user has chosen the static policy to be as such
+	// if/when the user has chosen their policy to be as such
 	tactics, found := ldp.Root.DomainEndpoints[net.JoinHostPort(domain, port)]
 	if !found {
 		return ldp.Fallback.LookupTactics(ctx, domain, port)
 	}
 
 	// note that we also need to fallback when the tactics contains an empty list
 	// or a list that only contains nil entries
-	tactics = staticPolicyRemoveNilEntries(tactics)
+	tactics = userPolicyRemoveNilEntries(tactics)
 	if len(tactics) <= 0 {
 		return ldp.Fallback.LookupTactics(ctx, domain, port)
 	}
@@ -115,7 +115,7 @@ func (ldp *staticPolicy) LookupTactics(
 	return out
 }
 
-func staticPolicyRemoveNilEntries(input []*httpsDialerTactic) (output []*httpsDialerTactic) {
+func userPolicyRemoveNilEntries(input []*httpsDialerTactic) (output []*httpsDialerTactic) {
 	for _, entry := range input {
 		if entry != nil {
 			output = append(output, entry)