address requested changes after merge review:

* never return IsAnomaly=true
* early return in case the communication to the API fails
* remove (most) top level TestKeys, including custom SummaryKeys and keep urlgetter keys
* however keeps invalid CA cert handling, since unattended expired certs was a root cause
  for false positives in the past
* removes snowflake + tor handling as part of the complexity reduction of the test
* remove API response bodys from testkeys, addresses also removal of geolocation API reponse
* adapts unit tests

Author: cyBerta

internal/experiment/riseupvpn/riseupvpn.go

@@ -6,13 +6,11 @@ package riseupvpn
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/experiment/urlgetter"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
-	"github.com/ooni/probe-cli/v3/internal/tracex"
 )
 
 const (
@@ -24,19 +22,19 @@ const (
 	tcpConnect    = "tcpconnect://"
 )
 
-// EipService is the main JSON object of eip-service.json.
-type EipService struct {
+// EipServiceV3 is the main JSON object of eip-service.json.
+type EipServiceV3 struct {
 	Gateways []GatewayV3
 }
 
-// Capabilities is a list of transports a gateway supports
-type Capabilities struct {
+// CapabilitiesV3 is a list of transports a gateway supports
+type CapabilitiesV3 struct {
 	Transport []TransportV3
 }
 
 // GatewayV3 describes a gateway.
 type GatewayV3 struct {
-	Capabilities Capabilities
+	Capabilities CapabilitiesV3
 	Host         string
 	IPAddress    string `json:"ip_address"`
 	Location     string `json:"location"`
@@ -50,13 +48,6 @@ type TransportV3 struct {
 	Options   map[string]string
 }
 
-// GatewayConnection describes the connection to a riseupvpn gateway.
-type GatewayConnection struct {
-	IP            string `json:"ip"`
-	Port          int    `json:"port"`
-	TransportType string `json:"transport_type"`
-}
-
 // GatewayLoad describes the load of a single Gateway.
 type GatewayLoad struct {
 	Host     string  `json:"host"`
@@ -83,21 +74,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"`
+	APIFailure   []string `json:"api_failure"`
+	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,
+		APIFailure:   nil,
+		CACertStatus: true,
 	}
 }
 
@@ -109,11 +94,6 @@ func (tk *TestKeys) UpdateProviderAPITestKeys(v urlgetter.MultiOutput) {
 	tk.TCPConnect = append(tk.TCPConnect, v.TestKeys.TCPConnect...)
 	tk.TLSHandshakes = append(tk.TLSHandshakes, v.TestKeys.TLSHandshakes...)
 	if v.TestKeys.Failure != nil {
-		for _, request := range v.TestKeys.Requests {
-			if request.Request.URL == eipServiceURL && request.Failure != nil {
-				tk.APIStatus = "blocked"
-			}
-		}
 		tk.APIFailure = append(tk.APIFailure, *v.TestKeys.Failure)
 		return
 	}
@@ -125,42 +105,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
@@ -264,27 +208,23 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		testkeys.UpdateProviderAPITestKeys(entry)
 	}
 
-	if testkeys.APIStatus == "blocked" {
-		for i := range inputs {
-			inputs[i].Config.Tunnel = "torsf"
-		}
-
-		for entry := range multi.CollectOverall(ctx, inputs, 5, 20, "riseupvpn", callbacks) {
-			testkeys.UpdateProviderAPITestKeys(entry)
+	if testkeys.APIFailure != nil {
+		// scrub reponse bodys before early returning
+		var scrubbedRequests = []model.ArchivalHTTPRequestResult{}
+		for _, requestEntry := range testkeys.Requests {
+			requestEntry.Response.Body = model.ArchivalHTTPBody{Value: "[scrubbed]"}
+			scrubbedRequests = append(scrubbedRequests, requestEntry)
 		}
+		testkeys.Requests = scrubbedRequests
+		return nil
 	}
 
 	// test gateways now
-	testkeys.TransportStatus = map[string]string{}
 	gateways := parseGateways(testkeys)
 	openvpnEndpoints := generateMultiInputs(gateways, "openvpn")
 	obfs4Endpoints := generateMultiInputs(gateways, "obfs4")
 	overallCount := 1 + len(inputs) + len(openvpnEndpoints) + len(obfs4Endpoints)
 	startCount := 1 + len(inputs)
-	if testkeys.APIStatus == "blocked" {
-		startCount += len(inputs)
-		overallCount += len(inputs)
-	}
 
 	// measure openvpn in parallel
 	for entry := range multi.CollectOverall(
@@ -303,7 +243,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 	}
 
 	// set transport status based on gateway test results
-	testkeys.updateTransportStatus(len(openvpnEndpoints), len(obfs4Endpoints))
+	//testkeys.updateTransportStatus(len(openvpnEndpoints), len(obfs4Endpoints))
 	return nil
 }
 
@@ -333,8 +273,9 @@ func generateMultiInputs(gateways []GatewayV3, transportType string) []urlgetter
 }
 
 func parseGateways(testKeys *TestKeys) []GatewayV3 {
-	var eipService *EipService = nil
+	var eipService *EipServiceV3 = nil
 	var geoService *GeoService = nil
+	var scrubbedRequests = []model.ArchivalHTTPRequestResult{}
 	for _, requestEntry := range testKeys.Requests {
 		if requestEntry.Request.URL == eipServiceURL && requestEntry.Response.Code == 200 {
 			var err error = nil
@@ -350,12 +291,15 @@ func parseGateways(testKeys *TestKeys) []GatewayV3 {
 				testKeys.APIFailure = append(testKeys.APIFailure, "invalid_geoservice_response")
 			}
 		}
+		requestEntry.Response.Body = model.ArchivalHTTPBody{Value: "[scrubbed]"}
+		scrubbedRequests = append(scrubbedRequests, requestEntry)
 	}
+	testKeys.Requests = scrubbedRequests
 	return filterGateways(eipService, geoService)
 }
 
 // filterGateways selects a subset of available gateways supporting obfs4
-func filterGateways(eipService *EipService, geoService *GeoService) []GatewayV3 {
+func filterGateways(eipService *EipServiceV3, geoService *GeoService) []GatewayV3 {
 	var result []GatewayV3 = nil
 	if eipService != nil {
 		locations := getLocationsUnderTest(eipService, geoService)
@@ -375,7 +319,7 @@ func filterGateways(eipService *EipService, geoService *GeoService) []GatewayV3
 }
 
 // getLocationsUnderTest parses all gateways supporting obfs4 and returns the two locations having most obfs4 bridges
-func getLocationsUnderTest(eipService *EipService, geoService *GeoService) []string {
+func getLocationsUnderTest(eipService *EipServiceV3, geoService *GeoService) []string {
 	var result []string = nil
 	if eipService != nil {
 		locationMap := map[string]int{}
@@ -447,8 +391,8 @@ func (geoService *GeoService) isHealthyGateway(gateway GatewayV3) bool {
 }
 
 // DecodeEIP3 decodes eip-service.json version 3
-func DecodeEIP3(body string) (*EipService, error) {
-	var eip EipService
+func DecodeEIP3(body string) (*EipServiceV3, error) {
+	var eip EipServiceV3
 	err := json.Unmarshal([]byte(body), &eip)
 	if err != nil {
 		return nil, err
@@ -476,28 +420,11 @@ func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 // Note that this structure is part of the ABI contract with ooniprobe
 // therefore we should be careful when changing it.
 type SummaryKeys struct {
-	APIBlocked      bool              `json:"api_blocked"`
-	ValidCACert     bool              `json:"valid_ca_cert"`
-	FailingGateways int               `json:"failing_gateways"`
-	TransportStatus map[string]string `json:"transport_status"`
-	IsAnomaly       bool              `json:"-"`
+	IsAnomaly bool `json:"-"`
 }
 
 // GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
 func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
 	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
-	}
-	sk.APIBlocked = tk.APIStatus != "ok"
-	sk.ValidCACert = tk.CACertStatus
-	sk.FailingGateways = len(tk.FailingGateways)
-	sk.TransportStatus = tk.TransportStatus
-	// Note: the order in the following OR chains matter: TransportStatus
-	// is nil if APIBlocked
-	sk.IsAnomaly = (sk.APIBlocked ||
-		tk.TransportStatus["openvpn"] == "blocked" ||
-		tk.TransportStatus["obfs4"] == "blocked")
 	return sk, nil
 }