From 58788d3a83cf6574b9eb21cddc5ff5bcd64d9288 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 8 Mar 2021 14:46:48 +0100
Subject: [PATCH 1/9] chore: we're now at v3.8.0-alpha (#245)

---
 internal/version/version.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/version/version.go b/internal/version/version.go
index 9f7674a..94a0d37 100644
--- a/internal/version/version.go
+++ b/internal/version/version.go
@@ -3,5 +3,5 @@ package version
 
 const (
 	// Version is the software version
-	Version = "3.7.0"
+	Version = "3.8.0-alpha"
 )

From 0477903187db4f0dd75b03c6d16f393a9467aaf3 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 8 Mar 2021 14:52:04 +0100
Subject: [PATCH 2/9] fix(android): remove pin to specific NDK version (#246)

* fix(android): remove pin to specific NDK version

See https://github.com/ooni/probe-engine/issues/1179

* fix(android): see if we can trigger workflow with this rule
---
 .github/workflows/android.yml |  5 +++--
 build-android.bash            | 24 +-----------------------
 2 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/.github/workflows/android.yml b/.github/workflows/android.yml
index 9933c84..5c30899 100644
--- a/.github/workflows/android.yml
+++ b/.github/workflows/android.yml
@@ -3,7 +3,8 @@ on:
   push:
     branches:
       - mobile-staging
-      - 'release/**'
+      - "release/**"
+      - "**android**"
 jobs:
   test:
     runs-on: macos-latest
@@ -14,7 +15,7 @@ jobs:
       - uses: actions/checkout@v2
       - run: brew install --cask android-sdk
       - run: echo y | sdkmanager --install "platforms;android-29"
-      - run: echo y | sdkmanager --install "ndk;21.3.6528147"
+      - run: echo y | sdkmanager --install "ndk-bundle"
       - run: ./build-android.bash
         env:
           ANDROID_HOME: /usr/local/Caskroom/android-sdk/4333796
diff --git a/build-android.bash b/build-android.bash
index a96d759..2b5e9ab 100755
--- a/build-android.bash
+++ b/build-android.bash
@@ -11,7 +11,7 @@ if [ -z "$ANDROID_HOME" -o "$1" = "--help" ]; then
     echo ""
     echo "Then make sure you install the required packages:"
     echo ""
-    echo "sdkmanager --install 'build-tools;29.0.3' 'ndk;21.3.6528147'"
+    echo "sdkmanager --install 'build-tools;29.0.3' 'ndk-bundle'"
     echo ""
     echo "or, if you already installed, that you're up to date:"
     echo ""
@@ -22,28 +22,6 @@ if [ -z "$ANDROID_HOME" -o "$1" = "--help" ]; then
     echo ""
     exit 1
 fi
-if [ -d $ANDROID_HOME/ndk-bundle ]; then
-    echo ""
-    echo "FATAL: currently we need 'ndk;21.3.6528147' instead of ndk-bundle"
-    echo ""
-    echo "See https://github.com/ooni/probe-engine/issues/1179."
-    echo ""
-    echo "To fix: sdkmanager --uninstall ndk-bundle"
-    echo ""
-    exit 1
-fi
-export ANDROID_NDK_HOME=$ANDROID_HOME/ndk/21.3.6528147
-if [ ! -d $ANDROID_NDK_HOME ]; then
-    echo ""
-    echo "FATAL: currently we need 'ndk;21.3.6528147'"
-    echo ""
-    echo "See https://github.com/ooni/probe-engine/issues/1179."
-    echo ""
-    echo "To fix: sdkmanager --install 'ndk;21.3.6528147'"
-    echo ""
-    exit 1
-fi
-
 topdir=$(cd $(dirname $0) && pwd -P)
 set -x
 export PATH=$(go env GOPATH)/bin:$PATH

From 4da372a84d17139f4564b7a9e505310edbf07c15 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 8 Mar 2021 18:31:42 +0100
Subject: [PATCH 3/9] feat(libminiooni): implement --version and --limit (#247)

See https://github.com/ooni/probe/issues/1380
---
 internal/libminiooni/libminiooni.go | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/internal/libminiooni/libminiooni.go b/internal/libminiooni/libminiooni.go
index 3360215..df6c540 100644
--- a/internal/libminiooni/libminiooni.go
+++ b/internal/libminiooni/libminiooni.go
@@ -43,6 +43,7 @@ type Options struct {
 	HomeDir          string
 	Inputs           []string
 	InputFilePaths   []string
+	Limit            int64
 	NoJSON           bool
 	NoCollector      bool
 	ProbeServicesURL string
@@ -54,6 +55,7 @@ type Options struct {
 	TorBinary        string
 	Tunnel           string
 	Verbose          bool
+	Version          bool
 	Yes              bool
 }
 
@@ -87,6 +89,10 @@ func init() {
 		&globalOptions.Inputs, "input", 'i',
 		"Add test-dependent input to the test input", "INPUT",
 	)
+	getopt.FlagLong(
+		&globalOptions.Limit, "limit", 0,
+		"Limit the number of URLs tested by Web Connectivity", "N",
+	)
 	getopt.FlagLong(
 		&globalOptions.NoJSON, "no-json", 'N', "Disable writing to disk",
 	)
@@ -126,6 +132,9 @@ func init() {
 	getopt.FlagLong(
 		&globalOptions.Verbose, "verbose", 'v', "Increase verbosity",
 	)
+	getopt.FlagLong(
+		&globalOptions.Version, "version", 0, "Print version and exit",
+	)
 	getopt.FlagLong(
 		&globalOptions.Yes, "yes", 0, "I accept the risk of running OONI",
 	)
@@ -149,6 +158,10 @@ func fatalIfFalse(cond bool, msg string) {
 // integrate this function to either handle the panic of ignore it.
 func Main() {
 	getopt.Parse()
+	if globalOptions.Version {
+		fmt.Printf("%s\n", version.Version)
+		os.Exit(0)
+	}
 	fatalIfFalse(len(getopt.Args()) == 1, "Missing experiment name")
 	MainWithConfiguration(getopt.Arg(0), globalOptions)
 }
@@ -362,7 +375,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) {
 		SourceFiles:  currentOptions.InputFilePaths,
 		InputPolicy:  builder.InputPolicy(),
 		Session:      sess,
-		URLLimit:     17,
+		URLLimit:     currentOptions.Limit,
 	})
 	inputs, err := inputLoader.Load(context.Background())
 	fatalOnError(err, "cannot load inputs")

From f0110fe85a878f646f34c1ca486feeefd162776d Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Wed, 10 Mar 2021 10:39:57 +0100
Subject: [PATCH 4/9] 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/sessionresolver/resolvermaker.go |  1 +
 .../sessionresolver/sessionresolver.go        | 18 ++++
 .../sessionresolver/sessionresolver_test.go   | 91 +++++++++++++++++++
 internal/engine/session.go                    |  3 +-
 4 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/internal/engine/internal/sessionresolver/resolvermaker.go b/internal/engine/internal/sessionresolver/resolvermaker.go
index 95e30f6..1719c6e 100644
--- a/internal/engine/internal/sessionresolver/resolvermaker.go
+++ b/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)
 }
 
diff --git a/internal/engine/internal/sessionresolver/sessionresolver.go b/internal/engine/internal/sessionresolver/sessionresolver.go
index 8613a21..9b9af01 100644
--- a/internal/engine/internal/sessionresolver/sessionresolver.go
+++ b/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)
diff --git a/internal/engine/internal/sessionresolver/sessionresolver_test.go b/internal/engine/internal/sessionresolver/sessionresolver_test.go
index b83cde8..b2b28d3 100644
--- a/internal/engine/internal/sessionresolver/sessionresolver_test.go
+++ b/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)
+		}
+	}
+}
diff --git a/internal/engine/session.go b/internal/engine/session.go
index f6c8e69..ccd200a 100644
--- a/internal/engine/session.go
+++ b/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
 }

From fbee736e90674f214df08497386be39134d5f896 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Wed, 10 Mar 2021 12:01:08 +0100
Subject: [PATCH 5/9] fix(geolocate): no proxy when discovering our IP address
 (#251)

* fix(geolocate): no proxy when discovering our IP address

The use case of --proxy is that you cannot contact the OONI
backend otherwise. It is wrong, though, using the proxy when
discovering our IP address. The measurement won't use the
proxy anyway. Therefore, we need to use the IP address that
is performing the measurement. Not the one of the proxy.

What's more, stun is not using a proxy. Therefore, it does
not make much sense that http IP resolvers use a proxy. This
leads to inconsistencies. So, here's anothe reason why this
patch is a good thing (TM).

Finally, because knowing the IP address enables us to sanitize
the data, it's important we discover the correct IP.

Now, up until this point, the `--proxy` option has mostly
been a developers toy. But, users have asked us to have the
possibility of configuring a proxy.

This explains why I have been looking into making `--proxy`
right for a couple of hours now.

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

* fix(session): properly configure the IP lookupper
---
 internal/engine/geolocate/geolocate.go      | 34 +++++++++++++++------
 internal/engine/geolocate/geolocate_test.go |  7 +++++
 internal/engine/geolocate/iplookup.go       | 17 ++++++++---
 internal/engine/geolocate/iplookup_test.go  | 16 ++++------
 internal/engine/geolocate/stun.go           |  5 +++
 internal/engine/session.go                  |  2 +-
 6 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/internal/engine/geolocate/geolocate.go b/internal/engine/geolocate/geolocate.go
index d1371ae..15378cb 100644
--- a/internal/engine/geolocate/geolocate.go
+++ b/internal/engine/geolocate/geolocate.go
@@ -5,9 +5,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
 
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx"
 	"github.com/ooni/probe-cli/v3/internal/engine/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/version"
 )
@@ -51,7 +51,12 @@ var (
 
 // Logger is the definition of Logger used by this package.
 type Logger interface {
+	Debug(msg string)
 	Debugf(format string, v ...interface{})
+	Info(msg string)
+	Infof(format string, v ...interface{})
+	Warn(msg string)
+	Warnf(format string, v ...interface{})
 }
 
 // Results contains geolocate results
@@ -115,15 +120,23 @@ type ResourcesManager interface {
 	MaybeUpdateResources(ctx context.Context) error
 }
 
+// Resolver is a DNS resolver.
+type Resolver interface {
+	LookupHost(ctx context.Context, domain string) ([]string, error)
+	Network() string
+	Address() string
+}
+
 // Config contains configuration for a geolocate Task.
 type Config struct {
 	// EnableResolverLookup indicates whether we want to
 	// perform the optional resolver lookup.
 	EnableResolverLookup bool
 
-	// HTTPClient is the HTTP client to use. If not set, then
-	// we will use the http.DefaultClient.
-	HTTPClient *http.Client
+	// Resolver is the resolver we should use when
+	// making requests for discovering the IP. When
+	// this field is not set, we use the stdlib.
+	Resolver Resolver
 
 	// Logger is the logger to use. If not set, then we will
 	// use a logger that discards all messages.
@@ -146,9 +159,6 @@ func Must(task *Task, err error) *Task {
 
 // NewTask creates a new instance of Task from config.
 func NewTask(config Config) (*Task, error) {
-	if config.HTTPClient == nil {
-		config.HTTPClient = http.DefaultClient
-	}
 	if config.Logger == nil {
 		config.Logger = model.DiscardLogger
 	}
@@ -158,13 +168,17 @@ func NewTask(config Config) (*Task, error) {
 	if config.UserAgent == "" {
 		config.UserAgent = fmt.Sprintf("ooniprobe-engine/%s", version.Version)
 	}
+	if config.Resolver == nil {
+		config.Resolver = netx.NewResolver(
+			netx.Config{Logger: config.Logger})
+	}
 	return &Task{
 		countryLookupper:     mmdbLookupper{},
 		enableResolverLookup: config.EnableResolverLookup,
 		probeIPLookupper: ipLookupClient{
-			HTTPClient: config.HTTPClient,
-			Logger:     config.Logger,
-			UserAgent:  config.UserAgent,
+			Resolver:  config.Resolver,
+			Logger:    config.Logger,
+			UserAgent: config.UserAgent,
 		},
 		probeASNLookupper:    mmdbLookupper{},
 		resolverASNLookupper: mmdbLookupper{},
diff --git a/internal/engine/geolocate/geolocate_test.go b/internal/engine/geolocate/geolocate_test.go
index 99091ee..cabf2e2 100644
--- a/internal/engine/geolocate/geolocate_test.go
+++ b/internal/engine/geolocate/geolocate_test.go
@@ -393,3 +393,10 @@ func TestNewTaskWithNoResourcesManager(t *testing.T) {
 		t.Fatal("expected nil task here")
 	}
 }
+
+func TestASNStringWorks(t *testing.T) {
+	r := Results{ASN: 1234}
+	if r.ASNString() != "AS1234" {
+		t.Fatal("unexpected result")
+	}
+}
diff --git a/internal/engine/geolocate/iplookup.go b/internal/engine/geolocate/iplookup.go
index 7bb6a92..acf0222 100644
--- a/internal/engine/geolocate/iplookup.go
+++ b/internal/engine/geolocate/iplookup.go
@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/engine/internal/multierror"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx"
 )
 
 var (
@@ -65,8 +66,8 @@ var (
 )
 
 type ipLookupClient struct {
-	// HTTPClient is the HTTP client to use
-	HTTPClient *http.Client
+	// Resolver is the resolver to use for HTTP.
+	Resolver Resolver
 
 	// Logger is the logger to use
 	Logger Logger
@@ -88,7 +89,15 @@ func makeSlice() []method {
 func (c ipLookupClient) doWithCustomFunc(
 	ctx context.Context, fn lookupFunc,
 ) (string, error) {
-	ip, err := fn(ctx, c.HTTPClient, c.Logger, c.UserAgent)
+	// Implementation note: we MUST use an HTTP client that we're
+	// sure IS NOT using any proxy. To this end, we construct a
+	// client ourself that we know is not proxied.
+	clnt := &http.Client{Transport: netx.NewHTTPTransport(netx.Config{
+		Logger:       c.Logger,
+		FullResolver: c.Resolver,
+	})}
+	defer clnt.CloseIdleConnections()
+	ip, err := fn(ctx, clnt, c.Logger, c.UserAgent)
 	if err != nil {
 		return DefaultProbeIP, err
 	}
@@ -102,7 +111,7 @@ func (c ipLookupClient) doWithCustomFunc(
 func (c ipLookupClient) LookupProbeIP(ctx context.Context) (string, error) {
 	union := multierror.New(ErrAllIPLookuppersFailed)
 	for _, method := range makeSlice() {
-		c.Logger.Debugf("iplookup: using %s", method.name)
+		c.Logger.Infof("iplookup: using %s", method.name)
 		ip, err := c.doWithCustomFunc(ctx, method.fn)
 		if err == nil {
 			return ip, nil
diff --git a/internal/engine/geolocate/iplookup_test.go b/internal/engine/geolocate/iplookup_test.go
index 6d53a9c..b26ba9e 100644
--- a/internal/engine/geolocate/iplookup_test.go
+++ b/internal/engine/geolocate/iplookup_test.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"net"
-	"net/http"
 	"testing"
 
 	"github.com/apex/log"
@@ -12,9 +11,8 @@ import (
 
 func TestIPLookupGood(t *testing.T) {
 	ip, err := (ipLookupClient{
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
+		Logger:    log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
 	}).LookupProbeIP(context.Background())
 	if err != nil {
 		t.Fatal(err)
@@ -28,9 +26,8 @@ func TestIPLookupAllFailed(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // immediately cancel to cause Do() to fail
 	ip, err := (ipLookupClient{
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
+		Logger:    log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
 	}).LookupProbeIP(ctx)
 	if !errors.Is(err, context.Canceled) {
 		t.Fatal("expected an error here")
@@ -43,9 +40,8 @@ func TestIPLookupAllFailed(t *testing.T) {
 func TestIPLookupInvalidIP(t *testing.T) {
 	ctx := context.Background()
 	ip, err := (ipLookupClient{
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
+		Logger:    log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
 	}).doWithCustomFunc(ctx, invalidIPLookup)
 	if !errors.Is(err, ErrInvalidIPAddress) {
 		t.Fatal("expected an error here")
diff --git a/internal/engine/geolocate/stun.go b/internal/engine/geolocate/stun.go
index 34e9abe..90332db 100644
--- a/internal/engine/geolocate/stun.go
+++ b/internal/engine/geolocate/stun.go
@@ -7,6 +7,11 @@ import (
 	"github.com/pion/stun"
 )
 
+// TODO(bassosimone): we should modify the stun code to use
+// the session resolver rather than using its own.
+//
+// See https://github.com/ooni/probe/issues/1383.
+
 type stunClient interface {
 	Close() error
 	Start(m *stun.Message, h stun.Handler) error
diff --git a/internal/engine/session.go b/internal/engine/session.go
index ccd200a..e80d99d 100644
--- a/internal/engine/session.go
+++ b/internal/engine/session.go
@@ -491,8 +491,8 @@ func (s *Session) LookupLocationContext(ctx context.Context) (*geolocate.Results
 	// when we are using a proxy because that might leak information.
 	task := geolocate.Must(geolocate.NewTask(geolocate.Config{
 		EnableResolverLookup: s.proxyURL == nil,
-		HTTPClient:           s.DefaultHTTPClient(),
 		Logger:               s.Logger(),
+		Resolver:             s.resolver,
 		ResourcesManager:     s,
 		UserAgent:            s.UserAgent(),
 	}))

From 78b5bf0caaf163b27edb95af41b955dbf992418c Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Thu, 11 Mar 2021 09:42:23 +0100
Subject: [PATCH 6/9] refactor+doc(oonimkall): improve docs and simplify code
 (#253)

This diff has been extracted from a larger diff written for https://github.com/ooni/probe/issues/1346
---
 pkg/oonimkall/session.go | 164 ++++++++++++++++++++++++++-------------
 1 file changed, 110 insertions(+), 54 deletions(-)

diff --git a/pkg/oonimkall/session.go b/pkg/oonimkall/session.go
index ad47fc8..3a8f624 100644
--- a/pkg/oonimkall/session.go
+++ b/pkg/oonimkall/session.go
@@ -4,11 +4,10 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"fmt"
 	"runtime"
 	"sync"
 
-	engine "github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/engine/atomicx"
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
 	"github.com/ooni/probe-cli/v3/internal/engine/probeservices"
@@ -21,7 +20,7 @@ type AtomicInt64 struct {
 	*atomicx.Int64
 }
 
-// The following two variables contain metrics pertaining to the number
+// These two variables contain metrics pertaining to the number
 // of Sessions and Contexts that are currently being used.
 var (
 	ActiveSessions = &AtomicInt64{atomicx.NewInt64()}
@@ -33,11 +32,22 @@ var (
 // to this instance in the SessionConfig object. All log messages that
 // the Session will generate will be routed to this Logger.
 type Logger interface {
+	// Debug handles debug messages.
 	Debug(msg string)
+
+	// Info handles informational messages.
 	Info(msg string)
+
+	// Warn handles warning messages.
 	Warn(msg string)
 }
 
+// ExperimentCallbacks contains experiment callbacks.
+type ExperimentCallbacks interface {
+	// OnProgress provides information about an experiment progress.
+	OnProgress(percentage float64, message string)
+}
+
 // SessionConfig contains configuration for a Session. You should
 // fill all the mandatory fields and could also optionally fill some of
 // the optional fields. Then pass this struct to NewSession.
@@ -83,24 +93,18 @@ type SessionConfig struct {
 // OONI related task (e.g. geolocation). Note that the Session isn't
 // mean to be a long living object. The workflow is to create a Session,
 // do the operations you need to do with it now, then make sure it is
-// not referenced by other variables, so the Go GC can finalize it.
-//
-// Future directions
-//
-// We will eventually rewrite the code for running new experiments such
-// that a Task will be created from a Session, such that experiments
-// could share the same Session and save geolookups, etc. For now, we
-// are in the suboptimal situations where Tasks create, use, and close
-// their own session, thus running more lookups than needed.
+// not referenced by other variables, so the Go GC can finalize it. This
+// is what you would normally done with Java/ObjC.
 type Session struct {
+	// Hooks for testing (should not appear in Java/ObjC, because they
+	// cannot be automatically transformed to Java/ObjC code.)
+	TestingCheckInBeforeNewProbeServicesClient func(ctx *Context)
+	TestingCheckInBeforeCheckIn                func(ctx *Context)
+
 	cl        []context.CancelFunc
 	mtx       sync.Mutex
 	submitter *probeservices.Submitter
 	sessp     *engine.Session
-
-	// Hooks for testing (should not appear in Java/ObjC)
-	TestingCheckInBeforeNewProbeServicesClient func(ctx *Context)
-	TestingCheckInBeforeCheckIn                func(ctx *Context)
 }
 
 // NewSession creates a new session. You should use a session for running
@@ -133,6 +137,8 @@ func NewSession(config *SessionConfig) (*Session, error) {
 		return nil, err
 	}
 	sess := &Session{sessp: sessp}
+	// We use finalizers to reduce the burden of managing the
+	// session from languages with a garbage collector.
 	runtime.SetFinalizer(sess, sessionFinalizer)
 	ActiveSessions.Add(1)
 	return sess, nil
@@ -159,7 +165,9 @@ type Context struct {
 	ctx    context.Context
 }
 
-// Cancel cancels pending operations using this context.
+// Cancel cancels pending operations using this context. This method
+// is idempotent. Calling it more than once is fine. The first invocation
+// cancels the context. Subsequent invocations are no-operations.
 func (ctx *Context) Cancel() {
 	ctx.cancel()
 }
@@ -171,7 +179,8 @@ func (sess *Session) NewContext() *Context {
 
 // NewContextWithTimeout creates an new interruptible Context that will automatically
 // cancel itself after the given timeout. Setting a zero or negative timeout implies
-// there is no actual timeout configured for the Context.
+// there is no actual timeout configured for the Context, making this invocation
+// equivalent to calling NewContext().
 func (sess *Session) NewContextWithTimeout(timeout int64) *Context {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
@@ -188,7 +197,7 @@ func (sess *Session) NewContextWithTimeout(timeout int64) *Context {
 	return &Context{cancel: cancel, ctx: ctx}
 }
 
-// GeolocateResults contains the GeolocateTask results.
+// GeolocateResults contains the results of session.Geolocate.
 type GeolocateResults struct {
 	// ASN is the autonomous system number.
 	ASN string
@@ -203,15 +212,21 @@ type GeolocateResults struct {
 	Org string
 }
 
-// MaybeUpdateResources ensures that resources are up to date.
+// MaybeUpdateResources ensures that resources are up to date. This function
+// could perform network activity when we need to update resources.
+//
+// This function locks the session until it's done. That is, no other operation
+// can be performed as long as this function is pending.
 func (sess *Session) MaybeUpdateResources(ctx *Context) error {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
 	return sess.sessp.MaybeUpdateResources(ctx.ctx)
 }
 
-// Geolocate performs a geolocate operation and returns the results. This method
-// is (in Java terminology) synchronized with the session instance.
+// Geolocate performs a geolocate operation and returns the results.
+//
+// This function locks the session until it's done. That is, no other operation
+// can be performed as long as this function is pending.
 func (sess *Session) Geolocate(ctx *Context) (*GeolocateResults, error) {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
@@ -220,7 +235,7 @@ func (sess *Session) Geolocate(ctx *Context) (*GeolocateResults, error) {
 		return nil, err
 	}
 	return &GeolocateResults{
-		ASN:     fmt.Sprintf("AS%d", info.ASN),
+		ASN:     info.ASNString(),
 		Country: info.CountryCode,
 		IP:      info.ProbeIP,
 		Org:     info.NetworkName,
@@ -230,12 +245,17 @@ func (sess *Session) Geolocate(ctx *Context) (*GeolocateResults, error) {
 // SubmitMeasurementResults contains the results of a single measurement submission
 // to the OONI backends using the OONI collector API.
 type SubmitMeasurementResults struct {
+	// UpdateMeasurement is the measurement with updated report ID.
 	UpdatedMeasurement string
-	UpdatedReportID    string
+
+	// UpdatedReportID is the report ID used for the measurement.
+	UpdatedReportID string
 }
 
-// Submit submits the given measurement and returns the results. This method is (in
-// Java terminology) synchronized with the Session instance.
+// Submit submits the given measurement and returns the results.
+//
+// This function locks the session until it's done. That is, no other operation
+// can be performed as long as this function is pending.
 func (sess *Session) Submit(ctx *Context, measurement string) (*SubmitMeasurementResults, error) {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
@@ -261,12 +281,15 @@ func (sess *Session) Submit(ctx *Context, measurement string) (*SubmitMeasuremen
 	}, nil
 }
 
-// CheckInConfigWebConnectivity is the configuration for the WebConnectivity test
+// CheckInConfigWebConnectivity contains WebConnectivity
+// configuration for the check-in API.
 type CheckInConfigWebConnectivity struct {
-	CategoryCodes []string // CategoryCodes is an array of category codes
+	// CategoryCodes contains zero or more category codes (e.g. "HUMR").
+	CategoryCodes []string
 }
 
-// Add a category code to the array in CheckInConfigWebConnectivity
+// Add adds a category code to ckw.CategoryCode. This method allows you to
+// edit ckw.CategoryCodes, which is inaccessible from Java/ObjC.
 func (ckw *CheckInConfigWebConnectivity) Add(cat string) {
 	ckw.CategoryCodes = append(ckw.CategoryCodes, cat)
 }
@@ -277,36 +300,62 @@ func (ckw *CheckInConfigWebConnectivity) toModel() model.CheckInConfigWebConnect
 	}
 }
 
-// CheckInConfig contains configuration for calling the checkin API.
+// CheckInConfig contains configuration for the check-in API.
 type CheckInConfig struct {
-	Charging        bool                          // Charging indicate if the phone is actually charging
-	OnWiFi          bool                          // OnWiFi indicate if the phone is actually connected to a WiFi network
-	Platform        string                        // Platform of the probe
-	RunType         string                        // RunType
-	SoftwareName    string                        // SoftwareName of the probe
-	SoftwareVersion string                        // SoftwareVersion of the probe
-	WebConnectivity *CheckInConfigWebConnectivity // WebConnectivity class contain an array of categories
+	// Charging indicates whether the phone is charging.
+	Charging bool
+
+	// OnWiFi indicates whether the phone is using the Wi-Fi.
+	OnWiFi bool
+
+	// Platform is the mobile platform (e.g. "android")
+	Platform string
+
+	// RunType indicates whether this is an automated ("timed") run
+	// or otherwise a manual run initiated by the user.
+	RunType string
+
+	// SoftwareName is the name of the application.
+	SoftwareName string
+
+	// SoftwareVersion is the version of the application.
+	SoftwareVersion string
+
+	// WebConnectivity contains configuration items specific of
+	// the WebConnectivity experiment.
+	WebConnectivity *CheckInConfigWebConnectivity
 }
 
-// CheckInInfoWebConnectivity contains the array of URLs returned by the checkin API
+// CheckInInfoWebConnectivity contains the WebConnectivity
+// specific results of the check-in API call.
 type CheckInInfoWebConnectivity struct {
+	// ReportID is the report ID we should be using.
 	ReportID string
-	URLs     []model.URLInfo
+
+	// URLs contains the list of URLs to measure.
+	URLs []model.URLInfo
 }
 
-// URLInfo contains info on a test lists URL
+// URLInfo contains info on a specific URL to measure.
 type URLInfo struct {
+	// CategoryCode is the URL's category code (e.g. "HUMR").
 	CategoryCode string
-	CountryCode  string
-	URL          string
+
+	// CountryCode is the test list from which this URL
+	// comes from (e.g. "IT", "FR").
+	CountryCode string
+
+	// URL is the URL itself.
+	URL string
 }
 
-// Size returns the number of URLs.
+// Size returns the number of URLs included into the result.
 func (ckw *CheckInInfoWebConnectivity) Size() int64 {
 	return int64(len(ckw.URLs))
 }
 
-// At gets the URLInfo at position idx from CheckInInfoWebConnectivity.URLs
+// At returns the URLInfo at index idx. Note that this function will
+// return nil/null if the index is out of bounds.
 func (ckw *CheckInInfoWebConnectivity) At(idx int64) *URLInfo {
 	if idx < 0 || int(idx) >= len(ckw.URLs) {
 		return nil
@@ -323,20 +372,27 @@ func newCheckInInfoWebConnectivity(ckw *model.CheckInInfoWebConnectivity) *Check
 	if ckw == nil {
 		return nil
 	}
-	out := new(CheckInInfoWebConnectivity)
-	out.ReportID = ckw.ReportID
-	out.URLs = ckw.URLs
-	return out
+	return &CheckInInfoWebConnectivity{
+		ReportID: ckw.ReportID,
+		URLs:     ckw.URLs,
+	}
 }
 
-// CheckInInfo contains the return test objects from the checkin API
+// CheckInInfo contains the result of the check-in API.
 type CheckInInfo struct {
+	// WebConnectivity contains results that are specific to
+	// the WebConnectivity experiment. This field MAY be null
+	// if the server's response did not contain any info.
 	WebConnectivity *CheckInInfoWebConnectivity
 }
 
-// CheckIn function is called by probes asking if there are tests to be run
-// The config argument contains the mandatory settings.
-// Returns the list of tests to run and the URLs, on success, or an explanatory error, in case of failure.
+// CheckIn calls the check-in API. Both ctx and config MUST NOT be nil. This
+// function will fail if config is missing required settings. The return value
+// is either an error or a valid CheckInInfo instance. Beware that the returned
+// object MAY still contain nil fields depending on the server's response.
+//
+// This function locks the session until it's done. That is, no other operation
+// can be performed as long as this function is pending.
 func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo, error) {
 	sess.mtx.Lock()
 	defer sess.mtx.Unlock()
@@ -348,14 +404,14 @@ func (sess *Session) CheckIn(ctx *Context, config *CheckInConfig) (*CheckInInfo,
 		return nil, err
 	}
 	if sess.TestingCheckInBeforeNewProbeServicesClient != nil {
-		sess.TestingCheckInBeforeNewProbeServicesClient(ctx)
+		sess.TestingCheckInBeforeNewProbeServicesClient(ctx) // for testing
 	}
 	psc, err := sess.sessp.NewProbeServicesClient(ctx.ctx)
 	if err != nil {
 		return nil, err
 	}
 	if sess.TestingCheckInBeforeCheckIn != nil {
-		sess.TestingCheckInBeforeCheckIn(ctx)
+		sess.TestingCheckInBeforeCheckIn(ctx) // for testing
 	}
 	cfg := model.CheckInConfig{
 		Charging:        config.Charging,

From a02052fb0c7184889039bf045cb6ff4c1c0a0944 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Thu, 11 Mar 2021 19:35:22 +0100
Subject: [PATCH 7/9] chore: rename stun_reachability => stunreachability
 (#254)

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

Ok @hellais @FedericoCeratto
---
 cmd/ooniprobe/internal/nettests/stunreachability.go           | 2 +-
 internal/engine/allexperiments.go                             | 2 +-
 .../engine/experiment/stunreachability/stunreachability.go    | 4 ++--
 .../experiment/stunreachability/stunreachability_test.go      | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go
index 68fb40e..14aa46c 100644
--- a/cmd/ooniprobe/internal/nettests/stunreachability.go
+++ b/cmd/ooniprobe/internal/nettests/stunreachability.go
@@ -5,7 +5,7 @@ type STUNReachability struct{}
 
 // Run starts the nettest.
 func (n STUNReachability) Run(ctl *Controller) error {
-	builder, err := ctl.Session.NewExperimentBuilder("stun_reachability")
+	builder, err := ctl.Session.NewExperimentBuilder("stunreachability")
 	if err != nil {
 		return err
 	}
diff --git a/internal/engine/allexperiments.go b/internal/engine/allexperiments.go
index c937add..5b6ebfb 100644
--- a/internal/engine/allexperiments.go
+++ b/internal/engine/allexperiments.go
@@ -241,7 +241,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{
 		}
 	},
 
-	"stun_reachability": func(session *Session) *ExperimentBuilder {
+	"stunreachability": func(session *Session) *ExperimentBuilder {
 		return &ExperimentBuilder{
 			build: func(config interface{}) *Experiment {
 				return NewExperiment(session, stunreachability.NewExperimentMeasurer(
diff --git a/internal/engine/experiment/stunreachability/stunreachability.go b/internal/engine/experiment/stunreachability/stunreachability.go
index 8bd2845..e538135 100644
--- a/internal/engine/experiment/stunreachability/stunreachability.go
+++ b/internal/engine/experiment/stunreachability/stunreachability.go
@@ -19,8 +19,8 @@ import (
 )
 
 const (
-	testName    = "stun_reachability"
-	testVersion = "0.1.0"
+	testName    = "stunreachability"
+	testVersion = "0.2.0"
 )
 
 // Config contains the experiment config.
diff --git a/internal/engine/experiment/stunreachability/stunreachability_test.go b/internal/engine/experiment/stunreachability/stunreachability_test.go
index 3f9ffc5..777bb4f 100644
--- a/internal/engine/experiment/stunreachability/stunreachability_test.go
+++ b/internal/engine/experiment/stunreachability/stunreachability_test.go
@@ -18,10 +18,10 @@ import (
 
 func TestMeasurerExperimentNameVersion(t *testing.T) {
 	measurer := stunreachability.NewExperimentMeasurer(stunreachability.Config{})
-	if measurer.ExperimentName() != "stun_reachability" {
+	if measurer.ExperimentName() != "stunreachability" {
 		t.Fatal("unexpected ExperimentName")
 	}
-	if measurer.ExperimentVersion() != "0.1.0" {
+	if measurer.ExperimentVersion() != "0.2.0" {
 		t.Fatal("unexpected ExperimentVersion")
 	}
 }

From c324822870ba1fc474cc949d298476674322e7b9 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 15 Mar 2021 10:59:28 +0100
Subject: [PATCH 8/9] fix(stunreachability): avoid goroutine spin and memleak
 (#255)

This fix addresses the bug described in issue https://github.com/ooni/probe/issues/1403.
---
 .../engine/experiment/stunreachability/stunreachability.go    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/internal/engine/experiment/stunreachability/stunreachability.go b/internal/engine/experiment/stunreachability/stunreachability.go
index e538135..4d5ba5b 100644
--- a/internal/engine/experiment/stunreachability/stunreachability.go
+++ b/internal/engine/experiment/stunreachability/stunreachability.go
@@ -122,15 +122,15 @@ func (tk *TestKeys) do(
 	if err != nil {
 		return err
 	}
-	defer conn.Close()
 	newClient := stun.NewClient
 	if config.newClient != nil {
 		newClient = config.newClient
 	}
-	client, err := newClient(conn, stun.WithNoConnClose)
+	client, err := newClient(conn)
 	if err != nil {
 		return err
 	}
+	defer client.Close()
 	message := stun.MustBuild(stun.TransactionID, stun.BindingRequest)
 	ch := make(chan error)
 	err = client.Start(message, func(ev stun.Event) {

From 53e6b694cb834bb64dcf5ff77d005f56b98d2983 Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Mon, 15 Mar 2021 14:54:13 +0100
Subject: [PATCH 9/9] chore: bless 3.8.0 (#256)

---
 internal/version/version.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/version/version.go b/internal/version/version.go
index 94a0d37..0b5eeb3 100644
--- a/internal/version/version.go
+++ b/internal/version/version.go
@@ -3,5 +3,5 @@ package version
 
 const (
 	// Version is the software version
-	Version = "3.8.0-alpha"
+	Version = "3.8.0"
 )