refactor(riseupvpn): minor changes and annotations (#275)

internal/engine/experiment/riseupvpn/riseupvpn.go

@@ -24,12 +24,12 @@ const (
 	tcpConnect    = "tcpconnect://"
 )
 
-// EipService main json object of eip-service.json
+// EipService is the main JSON object of eip-service.json.
 type EipService struct {
 	Gateways []GatewayV3
 }
 
-// GatewayV3 json obj Version 3
+// GatewayV3 describes a gateway.
 type GatewayV3 struct {
 	Capabilities struct {
 		Transport []TransportV3
@@ -38,7 +38,7 @@ type GatewayV3 struct {
 	IPAddress string `json:"ip_address"`
 }
 
-// TransportV3 json obj Version 3
+// TransportV3 describes a transport.
 type TransportV3 struct {
 	Type      string
 	Protocols []string
@@ -46,7 +46,7 @@ type TransportV3 struct {
 	Options   map[string]string
 }
 
-// GatewayConnection describes the connection to a riseupvpn gateway
+// GatewayConnection describes the connection to a riseupvpn gateway.
 type GatewayConnection struct {
 	IP            string `json:"ip"`
 	Port          int    `json:"port"`
@@ -96,8 +96,9 @@ func (tk *TestKeys) UpdateProviderAPITestKeys(v urlgetter.MultiOutput) {
 	}
 }
 
-// AddGatewayConnectTestKeys updates the TestKeys using the given MultiOutput result of gateway connectivity testing.
+// AddGatewayConnectTestKeys updates the TestKeys using the given MultiOutput
-// Sets TransportStatus to "ok" if any successful TCP connection could be made
+// result of gateway connectivity testing. Sets TransportStatus to "ok" if
+// any successful TCP connection could be made
 func (tk *TestKeys) AddGatewayConnectTestKeys(v urlgetter.MultiOutput, transportType string) {
 	tk.NetworkEvents = append(tk.NetworkEvents, v.TestKeys.NetworkEvents...)
 	tk.TCPConnect = append(tk.TCPConnect, v.TestKeys.TCPConnect...)
@@ -107,10 +108,9 @@ func (tk *TestKeys) AddGatewayConnectTestKeys(v urlgetter.MultiOutput, transport
 			tk.FailingGateways = append(tk.FailingGateways, *gatewayConnection)
 		}
 	}
-	return
 }
 
-func (tk *TestKeys) updateTransportStatus(openvpnGatewayCount int, obfs4GatewayCount int) {
+func (tk *TestKeys) updateTransportStatus(openvpnGatewayCount, obfs4GatewayCount int) {
 	failingOpenvpnGateways, failingObfs4Gateways := 0, 0
 	for _, gw := range tk.FailingGateways {
 		if gw.TransportType == "openvpn" {
@@ -119,13 +119,11 @@ func (tk *TestKeys) updateTransportStatus(openvpnGatewayCount int, obfs4GatewayC
 			failingObfs4Gateways++
 		}
 	}
-
 	if failingOpenvpnGateways < openvpnGatewayCount {
 		tk.TransportStatus["openvpn"] = "ok"
 	} else {
 		tk.TransportStatus["openvpn"] = "blocked"
 	}
-
 	if failingObfs4Gateways < obfs4GatewayCount {
 		tk.TransportStatus["obfs4"] = "ok"
 	} else {
@@ -133,7 +131,8 @@ func (tk *TestKeys) updateTransportStatus(openvpnGatewayCount int, obfs4GatewayC
 	}
 }
 
-func newGatewayConnection(tcpConnect archival.TCPConnectEntry, transportType string) *GatewayConnection {
+func newGatewayConnection(
+	tcpConnect archival.TCPConnectEntry, transportType string) *GatewayConnection {
 	return &GatewayConnection{
 		IP:            tcpConnect.IP,
 		Port:          tcpConnect.Port,
@@ -141,7 +140,7 @@ func newGatewayConnection(tcpConnect archival.TCPConnectEntry, transportType str
 	}
 }
 
-// AddCACertFetchTestKeys Adding generic urlgetter.Get() testKeys to riseupvpn specific test keys
+// AddCACertFetchTestKeys adds generic urlgetter.Get() testKeys to riseupvpn specific test keys
 func (tk *TestKeys) AddCACertFetchTestKeys(testKeys urlgetter.TestKeys) {
 	tk.NetworkEvents = append(tk.NetworkEvents, testKeys.NetworkEvents...)
 	tk.Queries = append(tk.Queries, testKeys.Queries...)
@@ -155,7 +154,7 @@ func (tk *TestKeys) AddCACertFetchTestKeys(testKeys urlgetter.TestKeys) {
 	}
 }
 
-// Measurer performs the measurement
+// Measurer performs the measurement.
 type Measurer struct {
 	// Config contains the experiment settings. If empty we
 	// will be using default settings.
@@ -165,17 +164,17 @@ type Measurer struct {
 	Getter urlgetter.MultiGetter
 }
 
-// ExperimentName implements ExperimentMeasurer.ExperimentName
+// ExperimentName implements ExperimentMeasurer.ExperimentName.
 func (m Measurer) ExperimentName() string {
 	return testName
 }
 
-// ExperimentVersion implements ExperimentMeasurer.ExperimentVersion
+// ExperimentVersion implements ExperimentMeasurer.ExperimentVersion.
 func (m Measurer) ExperimentVersion() string {
 	return testVersion
 }
 
-// Run implements ExperimentMeasurer.Run
+// Run implements ExperimentMeasurer.Run.
 func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 	measurement *model.Measurement, callbacks model.ExperimentCallbacks) error {
 	ctx, cancel := context.WithTimeout(ctx, 90*time.Second)
@@ -184,12 +183,20 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 	measurement.TestKeys = testkeys
 	urlgetter.RegisterExtensions(measurement)
 
-	caTarget := "https://black.riseup.net/ca.crt"
 	certPool := netx.NewDefaultCertPool()
 
-	multi := urlgetter.Multi{Begin: measurement.MeasurementStartTimeSaved, Getter: m.Getter, Session: sess}
+	// used multiple times below
-	inputs := []urlgetter.MultiInput{
+	multi := urlgetter.Multi{
-		{Target: caTarget, Config: urlgetter.Config{
+		Begin:   measurement.MeasurementStartTimeSaved,
+		Getter:  m.Getter,
+		Session: sess,
+	}
+
+	// See if we can get the certificate first
+	caTarget := "https://black.riseup.net/ca.crt"
+	inputs := []urlgetter.MultiInput{{
+		Target: caTarget,
+		Config: urlgetter.Config{
 			Method:          "GET",
 			FailOnHTTPError: true,
 		}},
@@ -198,9 +205,11 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 		tk := entry.TestKeys
 		testkeys.AddCACertFetchTestKeys(tk)
 		if tk.Failure != nil {
+			// TODO(bassosimone,cyberta): should we update the testkeys
+			// in this case (e.g., APIFailure?)
+			// See https://github.com/ooni/probe/issues/1432.
 			return nil
 		}
-
 		if ok := certPool.AppendCertsFromPEM([]byte(tk.HTTPResponseBody)); !ok {
 			testkeys.CACertStatus = false
 			testkeys.APIStatus = "blocked"
@@ -210,8 +219,8 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 		}
 	}
 
+	// Now test the service endpoints using the above-fetched CA
 	inputs = []urlgetter.MultiInput{
-
 		// Here we need to provide the method explicitly. See
 		// https://github.com/ooni/probe-engine/issues/827.
 		{Target: providerURL, Config: urlgetter.Config{
@@ -230,8 +239,6 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 			FailOnHTTPError: true,
 		}},
 	}
-	multi = urlgetter.Multi{Begin: measurement.MeasurementStartTimeSaved, Getter: m.Getter, Session: sess}
-
 	for entry := range multi.CollectOverall(ctx, inputs, 1, 50, "riseupvpn", callbacks) {
 		testkeys.UpdateProviderAPITestKeys(entry)
 	}
@@ -244,20 +251,21 @@ func (m Measurer) Run(ctx context.Context, sess model.ExperimentSession,
 	overallCount := 1 + len(inputs) + len(openvpnEndpoints) + len(obfs4Endpoints)
 
 	// measure openvpn in parallel
-	multi = urlgetter.Multi{Begin: measurement.MeasurementStartTimeSaved, Getter: m.Getter, Session: sess}
+	for entry := range multi.CollectOverall(
-	for entry := range multi.CollectOverall(ctx, openvpnEndpoints, 1+len(inputs), overallCount, "riseupvpn", callbacks) {
+		ctx, openvpnEndpoints, 1+len(inputs), overallCount, "riseupvpn", callbacks) {
 		testkeys.AddGatewayConnectTestKeys(entry, "openvpn")
 	}
 
 	// measure obfs4 in parallel
-	multi = urlgetter.Multi{Begin: measurement.MeasurementStartTimeSaved, Getter: m.Getter, Session: sess}
+	// TODO(bassosimone): when urlgetter is able to do obfs4 handshakes, here
-	for entry := range multi.CollectOverall(ctx, obfs4Endpoints, 1+len(inputs)+len(openvpnEndpoints), overallCount, "riseupvpn", callbacks) {
+	// can possibly also test for the obfs4 handshake.
+	for entry := range multi.CollectOverall(
+		ctx, obfs4Endpoints, 1+len(inputs)+len(openvpnEndpoints), overallCount, "riseupvpn", callbacks) {
 		testkeys.AddGatewayConnectTestKeys(entry, "obfs4")
 	}
 
 	// set transport status based on gateway test results
 	testkeys.updateTransportStatus(len(openvpnEndpoints), len(obfs4Endpoints))
-
 	return nil
 }
 
@@ -289,6 +297,9 @@ func generateMultiInputs(gateways []GatewayV3, transportType string) []urlgetter
 func parseGateways(testKeys *TestKeys) []GatewayV3 {
 	for _, requestEntry := range testKeys.Requests {
 		if requestEntry.Request.URL == eipServiceURL && requestEntry.Failure == nil {
+			// TODO(bassosimone,cyberta): is it reasonable that we discard
+			// the error when the JSON we fetched cannot be parsed?
+			// See https://github.com/ooni/probe/issues/1432
 			eipService, err := DecodeEIP3(requestEntry.Response.Body.Value)
 			if err == nil {
 				return eipService.Gateways
@@ -336,7 +347,10 @@ func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, e
 	sk.ValidCACert = tk.CACertStatus
 	sk.FailingGateways = len(tk.FailingGateways)
 	sk.TransportStatus = tk.TransportStatus
-	sk.IsAnomaly = (sk.APIBlocked == true || tk.CACertStatus == false ||
+	// Note: the order in the following OR chains matter: TransportStatus
-		tk.TransportStatus["openvpn"] == "blocked" || tk.TransportStatus["obfs4"] == "blocked")
+	// is nil if APIBlocked or !CACertStatus
+	sk.IsAnomaly = (sk.APIBlocked || !tk.CACertStatus ||
+		tk.TransportStatus["openvpn"] == "blocked" ||
+		tk.TransportStatus["obfs4"] == "blocked")
 	return sk, nil
 }

internal/engine/experiment/riseupvpn/riseupvpn_test.go

@@ -3,20 +3,20 @@ package riseupvpn_test
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"strconv"
 	"strings"
 	"testing"
 
-	"github.com/ooni/probe-cli/v3/internal/engine/netx/archival"
-
 	"github.com/apex/log"
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/riseupvpn"
 	"github.com/ooni/probe-cli/v3/internal/engine/experiment/urlgetter"
 	"github.com/ooni/probe-cli/v3/internal/engine/internal/mockable"
 	"github.com/ooni/probe-cli/v3/internal/engine/model"
+	"github.com/ooni/probe-cli/v3/internal/engine/netx/archival"
 	"github.com/ooni/probe-cli/v3/internal/engine/netx/errorx"
 )
 
@@ -176,6 +176,10 @@ UN9SaWRlWKSdP4haujnzCoJbM7dU9bjvlGZNyXEekgeT0W2qFeGGp+yyUWw8tNsp
 0BuC1b7uW/bBn/xKm319wXVDvBgZgcktMolak39V7DVO
 -----END CERTIFICATE-----`
 
+	// TODO(bassosimone): maybe we can switch this test to internal
+	// testing (since now it's all unit tested!) and just use the
+	// same constants that are used in riseupvpn.go.
+
 	eipserviceurl = "https://api.black.riseup.net:443/3/config/eip-service.json"
 	providerurl   = "https://riseup.net/provider.json"
 	geoserviceurl = "https://api.black.riseup.net:9001/json"
@@ -320,10 +324,7 @@ func TestInvalidCaCert(t *testing.T) {
 			obfs4url1:     true,
 		}),
 	}
-
+	ctx := context.Background()
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
 	sess := &mockable.Session{MockableLogger: log.Log}
 	measurement := new(model.Measurement)
 	callbacks := model.NewPrinterCallbacks(log.Log)
@@ -338,6 +339,12 @@ func TestInvalidCaCert(t *testing.T) {
 	if tk.APIStatus != "blocked" {
 		t.Fatal("ApiStatus should be blocked")
 	}
+	if tk.FailingGateways != nil {
+		t.Fatal("invalid FailingGateways")
+	}
+	if tk.TransportStatus != nil {
+		t.Fatal("invalid TransportStatus")
+	}
 }
 
 func TestFailureCaCertFetch(t *testing.T) {
@@ -365,6 +372,12 @@ func TestFailureCaCertFetch(t *testing.T) {
 	if len(tk.Requests) > 1 {
 		t.Fatal("Unexpected requests")
 	}
+	if tk.FailingGateways != nil {
+		t.Fatal("invalid FailingGateways")
+	}
+	if tk.TransportStatus != nil {
+		t.Fatal("invalid TransportStatus")
+	}
 }
 
 func TestFailureEipServiceBlocked(t *testing.T) {
@@ -379,7 +392,7 @@ func TestFailureEipServiceBlocked(t *testing.T) {
 	}))
 	tk := measurement.TestKeys.(*riseupvpn.TestKeys)
 	if tk.CACertStatus != true {
-		t.Fatal("invalid CACertStatus ")
+		t.Fatal("invalid CACertStatus")
 	}
 
 	for _, entry := range tk.Requests {
@@ -582,9 +595,7 @@ func TestMissingTransport(t *testing.T) {
 		}),
 	}
 
-	ctx, cancel := context.WithCancel(context.Background())
+	ctx := context.Background()
-	defer cancel()
-
 	sess := &mockable.Session{MockableLogger: log.Log}
 	measurement := new(model.Measurement)
 	callbacks := model.NewPrinterCallbacks(log.Log)
@@ -600,7 +611,6 @@ func TestMissingTransport(t *testing.T) {
 	if _, found := tk.TransportStatus["obfs"]; found {
 		t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus))
 	}
-
 }
 
 func TestSummaryKeysInvalidType(t *testing.T) {
@@ -708,7 +718,7 @@ func generateMockGetter(requestResponse map[string]string, responseStatus map[st
 		responseBody, foundRequest := requestResponse[url]
 		isSuccessStatus, foundStatus := responseStatus[url]
 		if !foundRequest || !foundStatus {
-			return urlgetter.DefaultMultiGetter(ctx, g)
+			return urlgetter.TestKeys{}, errors.New("request or status not found")
 		}
 
 		var failure *string
@@ -768,6 +778,7 @@ func generateMockGetter(requestResponse map[string]string, responseStatus map[st
 		return tk, nil
 	}
 }
+
 func generateDefaultMockGetter(responseStatuses map[string]bool) urlgetter.MultiGetter {
 	return generateMockGetter(RequestResponse, responseStatuses)
 }