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

This diff incorporates part of what has been implemented by @cyBerta in
#1125 in response to my review.

I have omitted the work to use the geo service to figure out which are
the correct gateways to test for now, which allows me to simplify the
diff that I am committing to the master branch.

The spirit of the changes is the following:

1. reduce the test keys to the minimum required by riseup to
process the experiment results;

2. skip TLS verification if we cannot fetch the CA certificate
and unconditionally fetch information about gateways.

The major difference between this diff and the above-mentioned pull
request is that I did not feel good to keep the `api_failure` name
with a different type, which seems like a very breaking change,
and instead I introduced a new type called `api_failures`.

This work is part of ooni/probe#1432.

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,22 +154,24 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 			FailOnHTTPError: true,
 		}},
 	}
-	for entry := range multi.CollectOverall(ctx, inputs, 0, 20, "riseupvpn", callbacks) {
+
+	nullCallbacks := model.NewPrinterCallbacks(model.DiscardLogger)
+	noTLSVerify := true
+	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.
-			return nil
+			testkeys.CACertStatus = false
+			testkeys.APIFailures = append(testkeys.APIFailures, *tk.Failure)
+			continue
 		}
 		if ok := certPool.AppendCertsFromPEM([]byte(tk.HTTPResponseBody)); !ok {
 			testkeys.CACertStatus = false
-			testkeys.APIStatus = "blocked"
-			errorValue := "invalid_ca"
-			testkeys.APIFailure = &errorValue
-			return nil
+			testkeys.APIFailures = append(testkeys.APIFailures, "invalid_ca")
+			continue
 		}
+		// We have a CA so we can verify certificates
+		noTLSVerify = false
 	}
 
 	// Now test the service endpoints using the above-fetched CA
@@ -232,24 +182,26 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 			CertPool:        certPool,
 			Method:          "GET",
 			FailOnHTTPError: true,
+			NoTLSVerify:     noTLSVerify,
 		}},
 		{Target: eipServiceURL, Config: urlgetter.Config{
 			CertPool:        certPool,
 			Method:          "GET",
 			FailOnHTTPError: true,
+			NoTLSVerify:     noTLSVerify,
 		}},
 		{Target: geoServiceURL, Config: urlgetter.Config{
 			CertPool:        certPool,
 			Method:          "GET",
 			FailOnHTTPError: true,
+			NoTLSVerify:     noTLSVerify,
 		}},
 	}
-	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)
 	}
 
 	// test gateways now
-	testkeys.TransportStatus = map[string]string{}
 	gateways := parseGateways(testkeys)
 	openvpnEndpoints := generateMultiInputs(gateways, "openvpn")
 	obfs4Endpoints := generateMultiInputs(gateways, "obfs4")
@@ -272,8 +224,6 @@ 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))
 	return nil
 }