fix(geolocate): do resolver lookup with proxy (#306)

The use cases for using a proxy become more clear over time. When I
originally wrote the proxy code the idea was to use the proxy to proxy
both the communication with the backend and measurements.

It become increasingly clear that we _only_ want to proxy the
communication with the backends. Therefore, there's no point in
skipping the resolver lookup step when we use a proxy.

Part of https://github.com/ooni/probe/issues/985

internal/engine/geolocate/geolocate.go

@@ -60,8 +60,8 @@ type Results struct {
 	// CountryCode is the country code
 	CountryCode string
 
-	// DidResolverLookup indicates whether we did a resolver lookup.
+	// didResolverLookup indicates whether we did a resolver lookup.
-	DidResolverLookup bool
+	didResolverLookup bool
 
 	// NetworkName is the network name
 	NetworkName string
@@ -109,10 +109,6 @@ type Resolver interface {
 
 // Config contains configuration for a geolocate Task.
 type Config struct {
-	// EnableResolverLookup indicates whether we want to
-	// perform the optional resolver lookup.
-	EnableResolverLookup bool
-
 	// Resolver is the resolver we should use when
 	// making requests for discovering the IP. When
 	// this field is not set, we use the stdlib.
@@ -147,12 +143,7 @@ func NewTask(config Config) (*Task, error) {
 	}
 	return &Task{
 		countryLookupper:     mmdbLookupper{},
-		enableResolverLookup: config.EnableResolverLookup,
+		probeIPLookupper:     ipLookupClient(config),
-		probeIPLookupper: ipLookupClient{
-			Resolver:  config.Resolver,
-			Logger:    config.Logger,
-			UserAgent: config.UserAgent,
-		},
 		probeASNLookupper:    mmdbLookupper{},
 		resolverASNLookupper: mmdbLookupper{},
 		resolverIPLookupper:  resolverLookupClient{},
@@ -163,7 +154,6 @@ func NewTask(config Config) (*Task, error) {
 // instance of Task using the NewTask factory.
 type Task struct {
 	countryLookupper     countryLookupper
-	enableResolverLookup bool
 	probeIPLookupper     probeIPLookupper
 	probeASNLookupper    asnLookupper
 	resolverASNLookupper asnLookupper
@@ -198,25 +188,23 @@ func (op Task) Run(ctx context.Context) (*Results, error) {
 		return out, fmt.Errorf("lookupProbeCC failed: %w", err)
 	}
 	out.CountryCode = cc
-	if op.enableResolverLookup {
+	out.didResolverLookup = true
-		out.DidResolverLookup = true
+	// Note: ignoring the result of lookupResolverIP and lookupASN
-		// Note: ignoring the result of lookupResolverIP and lookupASN
+	// here is intentional. We don't want this (~minor) failure
-		// here is intentional. We don't want this (~minor) failure
+	// to influence the result of the overall lookup. Another design
-		// to influence the result of the overall lookup. Another design
+	// here could be that of retrying the operation N times?
-		// here could be that of retrying the operation N times?
+	resolverIP, err := op.resolverIPLookupper.LookupResolverIP(ctx)
-		resolverIP, err := op.resolverIPLookupper.LookupResolverIP(ctx)
+	if err != nil {
-		if err != nil {
+		return out, nil // intentional
-			return out, nil
-		}
-		out.ResolverIP = resolverIP
-		resolverASN, resolverNetworkName, err := op.resolverASNLookupper.LookupASN(
-			out.ResolverIP,
-		)
-		if err != nil {
-			return out, nil
-		}
-		out.ResolverASN = resolverASN
-		out.ResolverNetworkName = resolverNetworkName
 	}
+	out.ResolverIP = resolverIP
+	resolverASN, resolverNetworkName, err := op.resolverASNLookupper.LookupASN(
+		out.ResolverIP,
+	)
+	if err != nil {
+		return out, nil // intentional
+	}
+	out.ResolverASN = resolverASN
+	out.ResolverNetworkName = resolverNetworkName
 	return out, nil
 }

internal/engine/geolocate/geolocate_test.go

@@ -148,11 +148,10 @@ func (c taskResolverIPLookupper) LookupResolverIP(ctx context.Context) (string,
 func TestLocationLookupCannotLookupResolverIP(t *testing.T) {
 	expected := errors.New("mocked error")
 	op := Task{
-		probeIPLookupper:     taskProbeIPLookupper{ip: "1.2.3.4"},
+		probeIPLookupper:    taskProbeIPLookupper{ip: "1.2.3.4"},
-		probeASNLookupper:    taskASNLookupper{asn: 1234, name: "1234.com"},
+		probeASNLookupper:   taskASNLookupper{asn: 1234, name: "1234.com"},
-		countryLookupper:     taskCCLookupper{cc: "IT"},
+		countryLookupper:    taskCCLookupper{cc: "IT"},
-		resolverIPLookupper:  taskResolverIPLookupper{err: expected},
+		resolverIPLookupper: taskResolverIPLookupper{err: expected},
-		enableResolverLookup: true,
 	}
 	ctx := context.Background()
 	out, err := op.Run(ctx)
@@ -171,7 +170,7 @@ func TestLocationLookupCannotLookupResolverIP(t *testing.T) {
 	if out.ProbeIP != "1.2.3.4" {
 		t.Fatal("invalid ProbeIP value")
 	}
-	if out.DidResolverLookup != true {
+	if out.didResolverLookup != true {
 		t.Fatal("invalid DidResolverLookup value")
 	}
 	if out.ResolverASN != DefaultResolverASN {
@@ -193,7 +192,6 @@ func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) {
 		countryLookupper:     taskCCLookupper{cc: "IT"},
 		resolverIPLookupper:  taskResolverIPLookupper{ip: "4.3.2.1"},
 		resolverASNLookupper: taskASNLookupper{err: expected},
-		enableResolverLookup: true,
 	}
 	ctx := context.Background()
 	out, err := op.Run(ctx)
@@ -212,7 +210,7 @@ func TestLocationLookupCannotLookupResolverNetworkName(t *testing.T) {
 	if out.ProbeIP != "1.2.3.4" {
 		t.Fatal("invalid ProbeIP value")
 	}
-	if out.DidResolverLookup != true {
+	if out.didResolverLookup != true {
 		t.Fatal("invalid DidResolverLookup value")
 	}
 	if out.ResolverASN != DefaultResolverASN {
@@ -233,7 +231,6 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
 		countryLookupper:     taskCCLookupper{cc: "IT"},
 		resolverIPLookupper:  taskResolverIPLookupper{ip: "4.3.2.1"},
 		resolverASNLookupper: taskASNLookupper{asn: 4321, name: "4321.com"},
-		enableResolverLookup: true,
 	}
 	ctx := context.Background()
 	out, err := op.Run(ctx)
@@ -252,7 +249,7 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
 	if out.ProbeIP != "1.2.3.4" {
 		t.Fatal("invalid ProbeIP value")
 	}
-	if out.DidResolverLookup != true {
+	if out.didResolverLookup != true {
 		t.Fatal("invalid DidResolverLookup value")
 	}
 	if out.ResolverASN != 4321 {
@@ -266,49 +263,8 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
 	}
 }
 
-func TestLocationLookupSuccessWithoutResolverLookup(t *testing.T) {
-	op := Task{
-		probeIPLookupper:     taskProbeIPLookupper{ip: "1.2.3.4"},
-		probeASNLookupper:    taskASNLookupper{asn: 1234, name: "1234.com"},
-		countryLookupper:     taskCCLookupper{cc: "IT"},
-		resolverIPLookupper:  taskResolverIPLookupper{ip: "4.3.2.1"},
-		resolverASNLookupper: taskASNLookupper{asn: 4321, name: "4321.com"},
-	}
-	ctx := context.Background()
-	out, err := op.Run(ctx)
-	if err != nil {
-		t.Fatalf("not the error we expected: %+v", err)
-	}
-	if out.ASN != 1234 {
-		t.Fatal("invalid ASN value")
-	}
-	if out.CountryCode != "IT" {
-		t.Fatal("invalid CountryCode value")
-	}
-	if out.NetworkName != "1234.com" {
-		t.Fatal("invalid NetworkName value")
-	}
-	if out.ProbeIP != "1.2.3.4" {
-		t.Fatal("invalid ProbeIP value")
-	}
-	if out.DidResolverLookup != false {
-		t.Fatal("invalid DidResolverLookup value")
-	}
-	if out.ResolverASN != DefaultResolverASN {
-		t.Fatalf("invalid ResolverASN value: %+v", out.ResolverASN)
-	}
-	if out.ResolverIP != DefaultResolverIP {
-		t.Fatalf("invalid ResolverIP value: %+v", out.ResolverIP)
-	}
-	if out.ResolverNetworkName != DefaultResolverNetworkName {
-		t.Fatal("invalid ResolverNetworkName value")
-	}
-}
-
 func TestSmoke(t *testing.T) {
-	config := Config{
+	config := Config{}
-		EnableResolverLookup: true,
-	}
 	task := Must(NewTask(config))
 	result, err := task.Run(context.Background())
 	if err != nil {

internal/engine/session.go

@@ -644,13 +644,10 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
 // LookupLocationContext performs a location lookup. If you want memoisation
 // of the results, you should use MaybeLookupLocationContext.
 func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results, error) {
-	// Implementation note: we don't perform the lookup of the resolver IP
-	// when we are using a proxy because that might leak information.
 	task := geolocate.Must(geolocate.NewTask(geolocate.Config{
-		EnableResolverLookup: s.proxyURL == nil,
+		Logger:    s.Logger(),
-		Logger:               s.Logger(),
+		Resolver:  s.resolver,
-		Resolver:             s.resolver,
+		UserAgent: s.UserAgent(),
-		UserAgent:            s.UserAgent(),
 	}))
 	return task.Run(ctx)
 }