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

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
 }