fix(sessionresolver): honour the proxy (#250)

In reality, we are not going to use the sessionresolver when we're
using a proxy (I just tested). But, it nonetheless feels a lot more
robust to write a correct sessionresolver that handles the proxy
in the most correct way. That is, the sessionresolver will now skip
all the entries that cannot use a socks5 proxy (including among them
also the system resolver). What's more, it will construct a child
resolver that propagates the proxy.

We have confidence that this holds true because we have added a test
ensuring that we are really using the configured proxy.

See https://github.com/ooni/probe/issues/1381

internal/engine/internal/sessionresolver/resolvermaker.go

@@ -88,6 +88,7 @@ func (r *Resolver) newresolver(URL string) (childResolver, error) {
 		ByteCounter:  r.byteCounter(),
 		HTTP3Enabled: h3,
 		Logger:       r.logger(),
+		ProxyURL:     r.ProxyURL,
 	}, URL)
 }
 

internal/engine/internal/sessionresolver/sessionresolver.go

@@ -25,6 +25,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"net/url"
 	"sync"
 	"time"
 
@@ -39,6 +40,7 @@ type Resolver struct {
 	ByteCounter    *bytecounter.Counter // optional
 	KVStore        KVStore              // optional
 	Logger         Logger               // optional
+	ProxyURL       *url.URL             // optional
 	codec          codec
 	dnsClientMaker dnsclientmaker
 	mu             sync.Mutex
@@ -71,6 +73,9 @@ func (r *Resolver) LookupHost(ctx context.Context, hostname string) ([]string, e
 	defer r.writestate(state)
 	me := multierror.New(ErrLookupHost)
 	for _, e := range state {
+		if r.shouldSkipWithProxy(e) {
+			continue // we cannot proxy this URL so ignore it
+		}
 		addrs, err := r.lookupHost(ctx, e, hostname)
 		if err == nil {
 			return addrs, nil
@@ -80,6 +85,19 @@ func (r *Resolver) LookupHost(ctx context.Context, hostname string) ([]string, e
 	return nil, me
 }
 
+func (r *Resolver) shouldSkipWithProxy(e *resolverinfo) bool {
+	URL, err := url.Parse(e.URL)
+	if err != nil {
+		return true // please skip
+	}
+	switch URL.Scheme {
+	case "https", "dot", "tcp":
+		return false // we can handle this
+	default:
+		return true // please skip
+	}
+}
+
 func (r *Resolver) lookupHost(ctx context.Context, ri *resolverinfo, hostname string) ([]string, error) {
 	const ewma = 0.9 // the last sample is very important
 	re, err := r.getresolver(ri.URL)

internal/engine/internal/sessionresolver/sessionresolver_test.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"errors"
 	"net"
+	"net/url"
 	"strings"
+	"sync/atomic"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -247,3 +249,92 @@ func TestMaybeConfusionManyEntries(t *testing.T) {
 		t.Fatal("unexpected state[3].URL")
 	}
 }
+
+func TestResolverWorksWithProxy(t *testing.T) {
+	var (
+		works      int32
+		startuperr = make(chan error)
+		listench   = make(chan net.Listener)
+		done       = make(chan interface{})
+	)
+	// proxy implementation
+	go func() {
+		defer close(done)
+		lconn, err := net.Listen("tcp", "127.0.0.1:0")
+		startuperr <- err
+		if err != nil {
+			return
+		}
+		listench <- lconn
+		for {
+			conn, err := lconn.Accept()
+			if err != nil {
+				// We assume this is when we were told to
+				// shutdown by the main goroutine.
+				return
+			}
+			atomic.AddInt32(&works, 1)
+			conn.Close()
+		}
+	}()
+	// make sure we could start the proxy
+	if err := <-startuperr; err != nil {
+		t.Fatal(err)
+	}
+	listener := <-listench
+	// use the proxy
+	reso := &Resolver{ProxyURL: &url.URL{
+		Scheme: "socks5",
+		Host:   listener.Addr().String(),
+	}}
+	ctx := context.Background()
+	addrs, err := reso.LookupHost(ctx, "ooni.org")
+	// cleanly shutdown the listener
+	listener.Close()
+	<-done
+	// check results
+	if !errors.Is(err, ErrLookupHost) {
+		t.Fatal("not the error we expected")
+	}
+	if addrs != nil {
+		t.Fatal("expected nil addrs")
+	}
+	if works < 1 {
+		t.Fatal("expected to see a positive number of entries here")
+	}
+}
+
+func TestShouldSkipWithProxyWorks(t *testing.T) {
+	expect := []struct {
+		url    string
+		result bool
+	}{{
+		url:    "\t",
+		result: true,
+	}, {
+		url:    "https://dns.google/dns-query",
+		result: false,
+	}, {
+		url:    "dot://dns.google/",
+		result: false,
+	}, {
+		url:    "http3://dns.google/dns-query",
+		result: true,
+	}, {
+		url:    "tcp://dns.google/",
+		result: false,
+	}, {
+		url:    "udp://dns.google/",
+		result: true,
+	}, {
+		url:    "system:///",
+		result: true,
+	}}
+	reso := &Resolver{}
+	for _, e := range expect {
+		out := reso.shouldSkipWithProxy(&resolverinfo{URL: e.url})
+		if out != e.result {
+			t.Fatal("unexpected result for", e)
+		}
+	}
+}

internal/engine/session.go

@@ -108,14 +108,15 @@ func NewSession(config SessionConfig) (*Session, error) {
 		ByteCounter:  sess.byteCounter,
 		BogonIsError: true,
 		Logger:       sess.logger,
+		ProxyURL:     config.ProxyURL,
 	}
 	sess.resolver = &sessionresolver.Resolver{
 		ByteCounter: sess.byteCounter,
 		KVStore:     config.KVStore,
 		Logger:      sess.logger,
+		ProxyURL:    config.ProxyURL,
 	}
 	httpConfig.FullResolver = sess.resolver
-	httpConfig.ProxyURL = config.ProxyURL // no need to proxy the resolver
 	sess.httpDefaultTransport = netx.NewHTTPTransport(httpConfig)
 	return sess, nil
 }