refactor(riseupvpn): handle failing API and simplify test keys (#1363)

This diff incorporates part of what has been implemented by @cyBerta in
#1125 in response to my review as
well as additional changes based on my own feelings about what is
correct to do here.

Compared to the original diff, these are the changes that I implemented:

1. I have omitted the work to fetch from riseup geo service and figure
out the correct gateways to test. The main reason for not including this
body of work has been to reduce the size of the diff and the amount of
code to deal with.

2. I modified the logic related to failures in fetching the CA and
communicating with riseup services. The test fails immediately if we
cannot fetch the proper CA or we cannot contact riseup services. I did
not feel comfortable disabling the CA to access riseup services and
connecting to the TCP endpoints discovered w/o CA verification.

3. In the test keys, I renamed `api_failure` to `api_failures` because I
do not think it's optimal to keep the same name while the type has
changed from `*string` to `[]string`.

The spirit of the changes is not directly compatible with what we
discussed with @cyBerta. The main difference is in my decision to fail
early in case we miss the preconditions. As I wrote in
#1125 (review),
I think we should be using richer input (and start with its simplest
form) to provision to probes the data they need to perform this
experiment. By provisioning the data ourselves, we remove the coupling
between getting the CA, accessing riseup services to get information on
what gateways we should measure, and measure the gateways, which makes
the experiment several orders of magnitude more robust.

Unfortunately, I do not have time, in this cycle, to perform all this
richer input work. We'll try again for 3.20.

This work is part of ooni/probe#1432.

While there, I forced null callbacks when performing the CA fetch and
contacting riseup services, otherwise we end up printing a non-monotonic
progress status. Admittedly, also omitting to provide progress about
these two operations is bad, but I think we won't be able to provide
monotonic progress until we know what we should fetch in advance.

---------

Co-authored-by: cyBerta <cyberta@riseup.net>

Author: bassosimone

internal/experiment/riseupvpn/riseupvpn.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/experiment/urlgetter"
-	"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
@@ -63,21 +62,15 @@ type Config struct {
 // TestKeys contains riseupvpn test keys.
 type TestKeys struct {
 	urlgetter.TestKeys
-	APIFailure      *string             `json:"api_failure"`
-	APIStatus       string              `json:"api_status"`
-	CACertStatus    bool                `json:"ca_cert_status"`
-	FailingGateways []GatewayConnection `json:"failing_gateways"`
-	TransportStatus map[string]string   `json:"transport_status"`
+	APIFailures  []string `json:"api_failures"`
+	CACertStatus bool     `json:"ca_cert_status"`
 }
 
 // NewTestKeys creates new riseupvpn TestKeys.
 func NewTestKeys() *TestKeys {
 	return &TestKeys{
-		APIFailure:      nil,
-		APIStatus:       "ok",
-		CACertStatus:    true,
-		FailingGateways: nil,
-		TransportStatus: nil,
+		APIFailures:  []string{},
+		CACertStatus: true,
 	}
 }
 
@@ -88,12 +81,8 @@ func (tk *TestKeys) UpdateProviderAPITestKeys(v urlgetter.MultiOutput) {
 	tk.Requests = append(tk.Requests, v.TestKeys.Requests...)
 	tk.TCPConnect = append(tk.TCPConnect, v.TestKeys.TCPConnect...)
 	tk.TLSHandshakes = append(tk.TLSHandshakes, v.TestKeys.TLSHandshakes...)
-	if tk.APIStatus != "ok" {
-		return // we already flipped the state
-	}
 	if v.TestKeys.Failure != nil {
-		tk.APIStatus = "blocked"
-		tk.APIFailure = v.TestKeys.Failure
+		tk.APIFailures = append(tk.APIFailures, *v.TestKeys.Failure)
 		return
 	}
 }
@@ -104,42 +93,6 @@ func (tk *TestKeys) UpdateProviderAPITestKeys(v urlgetter.MultiOutput) {
 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...)
-	for _, tcpConnect := range v.TestKeys.TCPConnect {
-		if !tcpConnect.Status.Success {
-			gatewayConnection := newGatewayConnection(tcpConnect, transportType)
-			tk.FailingGateways = append(tk.FailingGateways, *gatewayConnection)
-		}
-	}
-}
-
-func (tk *TestKeys) updateTransportStatus(openvpnGatewayCount, obfs4GatewayCount int) {
-	failingOpenvpnGateways, failingObfs4Gateways := 0, 0
-	for _, gw := range tk.FailingGateways {
-		if gw.TransportType == "openvpn" {
-			failingOpenvpnGateways++
-		} else if gw.TransportType == "obfs4" {
-			failingObfs4Gateways++
-		}
-	}
-	if failingOpenvpnGateways < openvpnGatewayCount {
-		tk.TransportStatus["openvpn"] = "ok"
-	} else {
-		tk.TransportStatus["openvpn"] = "blocked"
-	}
-	if failingObfs4Gateways < obfs4GatewayCount {
-		tk.TransportStatus["obfs4"] = "ok"
-	} else {
-		tk.TransportStatus["obfs4"] = "blocked"
-	}
-}
-
-func newGatewayConnection(
-	tcpConnect tracex.TCPConnectEntry, transportType string) *GatewayConnection {
-	return &GatewayConnection{
-		IP:            tcpConnect.IP,
-		Port:          tcpConnect.Port,
-		TransportType: transportType,
-	}
 }
 
 // AddCACertFetchTestKeys adds generic urlgetter.Get() testKeys to riseupvpn specific test keys
@@ -149,11 +102,6 @@ func (tk *TestKeys) AddCACertFetchTestKeys(testKeys urlgetter.TestKeys) {
 	tk.Requests = append(tk.Requests, testKeys.Requests...)
 	tk.TCPConnect = append(tk.TCPConnect, testKeys.TCPConnect...)
 	tk.TLSHandshakes = append(tk.TLSHandshakes, testKeys.TLSHandshakes...)
-	if testKeys.Failure != nil {
-		tk.APIStatus = "blocked"
-		tk.APIFailure = tk.Failure
-		tk.CACertStatus = false
-	}
 }
 
 // Measurer performs the measurement.
@@ -206,20 +154,31 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 			FailOnHTTPError: true,
 		}},
 	}
-	for entry := range multi.CollectOverall(ctx, inputs, 0, 20, "riseupvpn", callbacks) {
+
+	// Q: why returning early if we cannot fetch the CA or the config? Cannot we just
+	// disable certificate verification and fetch the config?
+	//
+	// A: I do not feel comfortable with fetching without verying the certificates since
+	// this means the experiment could be person-in-the-middled and forced to perform TCP
+	// connect to arbitrary hosts, which maybe is harmless but still a bummer.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2559): solve this problem by serving the
+	// correct CA and the endpoints to probes using check-in v2 (aka richer input).
+
+	nullCallbacks := model.NewPrinterCallbacks(model.DiscardLogger)
+	for entry := range multi.CollectOverall(ctx, inputs, 0, 20, "riseupvpn", nullCallbacks) {
 		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.
+			testkeys.CACertStatus = false
+			testkeys.APIFailures = append(testkeys.APIFailures, *tk.Failure)
+			// Note well: returning nil here causes the measurement to be submitted.
 			return nil
 		}
 		if ok := certPool.AppendCertsFromPEM([]byte(tk.HTTPResponseBody)); !ok {
 			testkeys.CACertStatus = false
-			testkeys.APIStatus = "blocked"
-			errorValue := "invalid_ca"
-			testkeys.APIFailure = &errorValue
+			testkeys.APIFailures = append(testkeys.APIFailures, "invalid_ca")
+			// Note well: returning nil here causes the measurement to be submitted.
 			return nil
 		}
 	}
@@ -244,12 +203,16 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 			FailOnHTTPError: true,
 		}},
 	}
-	for entry := range multi.CollectOverall(ctx, inputs, 1, 20, "riseupvpn", callbacks) {
+	for entry := range multi.CollectOverall(ctx, inputs, 1, 20, "riseupvpn", nullCallbacks) {
 		testkeys.UpdateProviderAPITestKeys(entry)
+		tk := entry.TestKeys
+		if tk.Failure != nil {
+			// Note well: returning nil here causes the measurement to be submitted.
+			return nil
+		}
 	}
 
 	// test gateways now
-	testkeys.TransportStatus = map[string]string{}
 	gateways := parseGateways(testkeys)
 	openvpnEndpoints := generateMultiInputs(gateways, "openvpn")
 	obfs4Endpoints := generateMultiInputs(gateways, "obfs4")
@@ -272,8 +235,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		testkeys.AddGatewayConnectTestKeys(entry, "obfs4")
 	}
 
-	// set transport status based on gateway test results
-	testkeys.updateTransportStatus(len(openvpnEndpoints), len(obfs4Endpoints))
+	// Note well: returning nil here causes the measurement to be submitted.
 	return nil
 }