ooni/probe-cli v3.8.0

-----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCAAdFiEEc4h3qmyCnyakMcX0gLaRJ3cz2VsFAmBPa/oACgkQgLaRJ3cz
 2Vu4qQ/+NKzgVlYLsS0JHmFCB2JGFy7LusCQhrNGapZfdw1nj5q+OaJ7q2whkT62
 pdciRdV8sa7rYe4jnf5a1DGj1F0ijV0Zvpx1oJPAwlyY1XhIdlSD3S1N6eS6mpW7
 6fxvMjQKErsZrZkTB9oIU1b70Tl6nCZUnSep0Y2eMwJg2/RtoT4JCyh4yBeq0iQ9
 Mb+b85Q8wHIlUryPz8vvNEx5pI48m7M9o5sl+Rp1HCUdPNmO5zUrrAzyFch9H6+I
 Qr0OOz0YHZHYESJ0gpuJ2lvQNMH6J/f8omv8kGVuVw9NaU2yM7hh2NJmXunfp9C3
 lPIGK1tFIb+kPRYtEzDy8eZ3Y+49WcOUmKl4d+O4FIi/T/issswaXRNMSBhr5//8
 QVw0FxCIXKjV9tzHK5c4JmLeQtR/OpYLlr420pSvn7uZ9h9WYjtkzVzl5wLqd45E
 w8/LBUAWa1rKD+OnHqVpP+A359s5QqIZkiBzxOpYZBRX4k1VzsBiK+JvF095O/AP
 KjS1xmVn5w9RTUESKamNdMwf4fJBFs8TbHxOdfCa7lD26H63UPGtoJE3kurbyRGK
 DVJRnNwWE32au3PupuCvkqDkIJWXPrjzx3i+i4ryNPxv2ZXM+Lmwl0GM/qZyyV5N
 JAlCiIf0J4V8yQOiVsyUWJ0PfGqjASG2rQxzMsHHnLcIAvrnzIM=
 =ywZe
 -----END PGP SIGNATURE-----

Merge tag 'v3.8.0' into mobile-staging

ooni/probe-cli v3.8.0

.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

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

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
 	}

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(

internal/engine/experiment/stunreachability/stunreachability.go

@@ -19,8 +19,8 @@ import (
 )
 
 const (
-	testName    = "stun_reachability"
+	testName    = "stunreachability"
-	testVersion = "0.1.0"
+	testVersion = "0.2.0"
 )
 
 // Config contains the experiment config.
@@ -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) {

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")
 	}
 }

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
+	// Resolver is the resolver we should use when
-	// we will use the http.DefaultClient.
+	// making requests for discovering the IP. When
-	HTTPClient *http.Client
+	// 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,
+			Resolver:  config.Resolver,
-			Logger:     config.Logger,
+			Logger:    config.Logger,
-			UserAgent:  config.UserAgent,
+			UserAgent: config.UserAgent,
 		},
 		probeASNLookupper:    mmdbLookupper{},
 		resolverASNLookupper: mmdbLookupper{},

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")
+	}
+}

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
+	// Resolver is the resolver to use for HTTP.
-	HTTPClient *http.Client
+	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

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,
-		Logger:     log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
-		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,
-		Logger:     log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
-		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,
-		Logger:     log.Log,
+		UserAgent: "ooniprobe-engine/0.1.0",
-		UserAgent:  "ooniprobe-engine/0.1.0",
 	}).doWithCustomFunc(ctx, invalidIPLookup)
 	if !errors.Is(err, ErrInvalidIPAddress) {
 		t.Fatal("expected an error here")

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

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
 }
@@ -490,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(),
 	}))

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")

internal/version/version.go

@@ -3,5 +3,5 @@ package version
 
 const (
 	// Version is the software version
-	Version = "3.7.0"
+	Version = "3.8.0"
 )

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.
+// not referenced by other variables, so the Go GC can finalize it. This
-//
+// is what you would normally done with Java/ObjC.
-// 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.
 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
+// Geolocate performs a geolocate operation and returns the results.
-// is (in Java terminology) synchronized with the session instance.
+//
+// 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
+// Submit submits the given measurement and returns the results.
-// Java terminology) synchronized with the Session instance.
+//
+// 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
+	// Charging indicates whether the phone is charging.
-	OnWiFi          bool                          // OnWiFi indicate if the phone is actually connected to a WiFi network
+	Charging bool
-	Platform        string                        // Platform of the probe
+
-	RunType         string                        // RunType
+	// OnWiFi indicates whether the phone is using the Wi-Fi.
-	SoftwareName    string                        // SoftwareName of the probe
+	OnWiFi bool
-	SoftwareVersion string                        // SoftwareVersion of the probe
+
-	WebConnectivity *CheckInConfigWebConnectivity // WebConnectivity class contain an array of categories
+	// 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)
+	return &CheckInInfoWebConnectivity{
-	out.ReportID = ckw.ReportID
+		ReportID: ckw.ReportID,
-	out.URLs = ckw.URLs
+		URLs:     ckw.URLs,
-	return out
+	}
 }
 
-// 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
+// CheckIn calls the check-in API. Both ctx and config MUST NOT be nil. This
-// The config argument contains the mandatory settings.
+// function will fail if config is missing required settings. The return value
-// Returns the list of tests to run and the URLs, on success, or an explanatory error, in case of failure.
+// 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,