diff --git a/internal/experiment/riseupvpn/riseupvpn.go b/internal/experiment/riseupvpn/riseupvpn.go index 893255262b..80dbced9aa 100644 --- a/internal/experiment/riseupvpn/riseupvpn.go +++ b/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 } diff --git a/internal/experiment/riseupvpn/riseupvpn_test.go b/internal/experiment/riseupvpn/riseupvpn_test.go index 4055749150..9370aeb75f 100644 --- a/internal/experiment/riseupvpn/riseupvpn_test.go +++ b/internal/experiment/riseupvpn/riseupvpn_test.go @@ -2,7 +2,6 @@ package riseupvpn_test import ( "context" - "encoding/json" "errors" "fmt" "io" @@ -13,8 +12,8 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/experiment/riseupvpn" "github.com/ooni/probe-cli/v3/internal/experiment/urlgetter" - "github.com/ooni/probe-cli/v3/internal/legacy/mockable" "github.com/ooni/probe-cli/v3/internal/legacy/tracex" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" ) @@ -141,8 +140,9 @@ const ( "serial": 3, "version": 3 }` - geoservice = `{"ip":"51.15.0.88","cc":"NL","city":"Haarlem","lat":52.381,"lon":4.6275,"gateways":["test1.riseup.net","test2.riseup.net"]}` - cacert = `-----BEGIN CERTIFICATE----- + geoservice = `{"ip":"51.15.0.88","cc":"NL","city":"Haarlem","lat":52.381,"lon":4.6275,"gateways":["test1.riseup.net","test2.riseup.net"]}` + geoService_update = `{"ip":"51.15.0.88","cc":"NL","city":"Haarlem","lat":52.381,"lon":4.6275,"gateways":["test1.riseup.net","test2.riseup.net"], "sortedGateways": [{ "host": "test1.riseup.net", "fullness": 0.2, "overload": false }, { "host": "test2.riseup.net", "fullness": 0.9, "overload": true }]}` + cacert = `-----BEGIN CERTIFICATE----- MIIFjTCCA3WgAwIBAgIBATANBgkqhkiG9w0BAQ0FADBZMRgwFgYDVQQKDA9SaXNl dXAgTmV0d29ya3MxGzAZBgNVBAsMEmh0dHBzOi8vcmlzZXVwLm5ldDEgMB4GA1UE AwwXUmlzZXVwIE5ldHdvcmtzIFJvb3QgQ0EwHhcNMTQwNDI4MDAwMDAwWhcNMjQw @@ -183,9 +183,10 @@ UN9SaWRlWKSdP4haujnzCoJbM7dU9bjvlGZNyXEekgeT0W2qFeGGp+yyUWw8tNsp providerurl = "https://riseup.net/provider.json" geoserviceurl = "https://api.black.riseup.net:9001/json" cacerturl = "https://black.riseup.net/ca.crt" - openvpnurl1 = "tcpconnect://234.345.234.345:443" - openvpnurl2 = "tcpconnect://123.456.123.456:443" + openvpnurl1 = "tcpconnect://234.345.234.345:443" // "Seattle" + openvpnurl2 = "tcpconnect://123.456.123.456:443" // "Paris" obfs4url1 = "tcpconnect://234.345.234.345:23042" + obfs4url2 = "tcpconnect://123.456.123.456:444" ) var RequestResponse = map[string]string{ @@ -196,6 +197,7 @@ var RequestResponse = map[string]string{ openvpnurl1: "", openvpnurl2: "", obfs4url1: "", + obfs4url2: "", } func TestNewExperimentMeasurer(t *testing.T) { @@ -209,14 +211,16 @@ func TestNewExperimentMeasurer(t *testing.T) { } func TestGood(t *testing.T) { + // the gateaway openvpnurl2 is filtered out, since it doesn't support additionally obfs4 measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: true, geoserviceurl: true, openvpnurl1: true, - openvpnurl2: true, + openvpnurl2: false, obfs4url1: true, + obfs4url2: false, })) tk := measurement.TestKeys.(*riseupvpn.TestKeys) @@ -229,23 +233,21 @@ func TestGood(t *testing.T) { if tk.Failure != nil { t.Fatal("unexpected Failure") } - if tk.APIFailure != nil { + if len(tk.APIFailures) != 0 { t.Fatal("unexpected ApiFailure") } - if tk.APIStatus != "ok" { - t.Fatal("unexpected ApiStatus") - } if tk.CACertStatus != true { t.Fatal("unexpected CaCertStatus") } - if tk.FailingGateways != nil { - t.Fatal("unexpected FailingGateways value") - } - if tk.TransportStatus == nil { - t.Fatal("unexpected nil TransportStatus struct ") + + hasOpenvpn1 := false + for _, tcpConnect := range tk.TCPConnect { + if tcpConnect.IP == "234.345.234.345" { + hasOpenvpn1 = true + } } - if tk.TransportStatus["openvpn"] != "ok" { - t.Fatal("unexpected openvpn transport status") + if !hasOpenvpn1 { + t.Fatalf("Gateway tests should run %t", hasOpenvpn1) } } @@ -256,7 +258,7 @@ func TestUpdateWithMixedResults(t *testing.T) { tk.UpdateProviderAPITestKeys(urlgetter.MultiOutput{ Input: urlgetter.MultiInput{ Config: urlgetter.Config{Method: "GET"}, - Target: "https://api.black.riseup.net:443/3/config/eip-service.json", + Target: "https://riseup.net/provider.json", }, TestKeys: urlgetter.TestKeys{ HTTPResponseStatus: 200, @@ -265,9 +267,17 @@ func TestUpdateWithMixedResults(t *testing.T) { tk.UpdateProviderAPITestKeys(urlgetter.MultiOutput{ Input: urlgetter.MultiInput{ Config: urlgetter.Config{Method: "GET"}, - Target: "https://riseup.net/provider.json", + Target: "https://api.black.riseup.net:443/3/config/eip-service.json", }, TestKeys: urlgetter.TestKeys{ + Requests: []model.ArchivalHTTPRequestResult{ + { + Request: model.ArchivalHTTPRequest{URL: "https://api.black.riseup.net:443/3/config/eip-service.json"}, + Failure: (func() *string { + s := "eof" + return &s + })(), + }}, FailedOperation: (func() *string { s := netxlite.HTTPRoundTripOperation return &s @@ -287,18 +297,10 @@ func TestUpdateWithMixedResults(t *testing.T) { HTTPResponseStatus: 200, }, }) - if tk.APIStatus != "blocked" { - t.Fatal("ApiStatus should be blocked") - } - if *tk.APIFailure != netxlite.FailureEOFError { + + if len(tk.APIFailures) != 1 || tk.APIFailures[0] != netxlite.FailureEOFError { t.Fatal("invalid ApiFailure") } - if tk.FailingGateways != nil { - t.Fatal("invalid FailingGateways") - } - if tk.TransportStatus != nil { - t.Fatal("invalid TransportStatus") - } } func TestInvalidCaCert(t *testing.T) { @@ -310,6 +312,7 @@ func TestInvalidCaCert(t *testing.T) { openvpnurl1: "", openvpnurl2: "", obfs4url1: "", + obfs4url2: "", } measurer := riseupvpn.Measurer{ Config: riseupvpn.Config{}, @@ -318,13 +321,17 @@ func TestInvalidCaCert(t *testing.T) { eipserviceurl: true, providerurl: true, geoserviceurl: true, - openvpnurl1: false, - openvpnurl2: true, + openvpnurl1: true, + openvpnurl2: false, // filtered out, no obfs4 support obfs4url1: true, + obfs4url2: false, // filtered out }), } ctx := context.Background() - sess := &mockable.Session{MockableLogger: log.Log} + sess := &mocks.Session{MockLogger: func() model.Logger { + return model.DiscardLogger + }} + measurement := new(model.Measurement) callbacks := model.NewPrinterCallbacks(log.Log) args := &model.ExperimentArgs{ @@ -340,15 +347,6 @@ func TestInvalidCaCert(t *testing.T) { if tk.CACertStatus == true { t.Fatal("unexpected CaCertStatus") } - 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) { @@ -366,21 +364,17 @@ func TestFailureCaCertFetch(t *testing.T) { if tk.CACertStatus != false { t.Fatal("invalid CACertStatus ") } - if tk.APIStatus != "blocked" { - t.Fatal("invalid ApiStatus") - } - if tk.APIFailure != nil { - t.Fatal("ApiFailure should be null") + if len(tk.APIFailures) != 1 || tk.APIFailures[0] != io.EOF.Error() { + t.Fatal("APIFailures should not be empty", tk.APIFailures) } - if len(tk.Requests) > 1 { - t.Fatal("Unexpected requests") + if len(tk.Requests) != 1 { + t.Fatal("Expected a single request in this case") } - if tk.FailingGateways != nil { - t.Fatal("invalid FailingGateways") - } - if tk.TransportStatus != nil { - t.Fatal("invalid TransportStatus") + for _, tcpConnect := range tk.TCPConnect { + if tcpConnect.IP == openvpnurl1 || tcpConnect.IP == openvpnurl2 || tcpConnect.IP == obfs4url1 || tcpConnect.IP == obfs4url2 { + t.Fatal("No gateaway tests should be run if API fails") + } } } @@ -407,12 +401,8 @@ func TestFailureEipServiceBlocked(t *testing.T) { } } - if tk.APIStatus != "blocked" { - t.Fatal("invalid ApiStatus") - } - - if tk.APIFailure == nil { - t.Fatal("ApiFailure should not be null") + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } @@ -439,12 +429,9 @@ func TestFailureProviderUrlBlocked(t *testing.T) { if tk.CACertStatus != true { t.Fatal("invalid CACertStatus ") } - if tk.APIStatus != "blocked" { - t.Fatal("invalid ApiStatus") - } - if tk.APIFailure == nil { - t.Fatal("ApiFailure should not be null") + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } @@ -471,154 +458,52 @@ func TestFailureGeoIpServiceBlocked(t *testing.T) { } } - if tk.APIStatus != "blocked" { - t.Fatal("invalid ApiStatus") - } - - if tk.APIFailure == nil { - t.Fatal("ApiFailure should not be null") + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } -func TestFailureGateway1(t *testing.T) { +func TestFailureGateway1TransportNOK(t *testing.T) { measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: true, geoserviceurl: true, - openvpnurl1: false, + openvpnurl1: false, // failed gateway openvpnurl2: true, obfs4url1: true, + obfs4url2: false, })) tk := measurement.TestKeys.(*riseupvpn.TestKeys) if tk.CACertStatus != true { t.Fatal("invalid CACertStatus ") } - if tk.FailingGateways == nil || len(tk.FailingGateways) != 1 { - t.Fatal("unexpected amount of failing gateways") - } - - gw := tk.FailingGateways[0] - if gw.IP != "234.345.234.345" { - t.Fatal("invalid failed gateway ip: " + fmt.Sprint(gw.IP)) - } - if gw.Port != 443 { - t.Fatal("invalid failed gateway port: " + fmt.Sprint(gw.Port)) - } - if gw.TransportType != "openvpn" { - t.Fatal("invalid failed transport type: " + fmt.Sprint(gw.TransportType)) - } - - if tk.APIStatus == "blocked" { - t.Fatal("invalid ApiStatus") - } - - if tk.APIFailure != nil { - t.Fatal("ApiFailure should be null") - } - - if tk.TransportStatus == nil || tk.TransportStatus["openvpn"] == "blocked" { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) - } - - if tk.TransportStatus == nil || tk.TransportStatus["obfs4"] == "blocked" { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) - } -} - -func TestFailureTransport(t *testing.T) { - measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ - cacerturl: true, - eipserviceurl: true, - providerurl: true, - geoserviceurl: true, - openvpnurl1: false, - openvpnurl2: false, - obfs4url1: false, - })) - tk := measurement.TestKeys.(*riseupvpn.TestKeys) - - if tk.TransportStatus == nil || tk.TransportStatus["openvpn"] != "blocked" { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) - } - - if tk.TransportStatus == nil || tk.TransportStatus["obfs4"] != "blocked" { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) - } -} - -func TestMissingTransport(t *testing.T) { - eipService, err := riseupvpn.DecodeEIPServiceV3(eipservice) - if err != nil { - t.Fatal("Preconditions for the test are not met.") - } - - //remove obfs4 capability from 2. gateway so that our - //mock provider supports only openvpn - index := -1 - transports := eipService.Gateways[1].Capabilities.Transport - for i, transport := range transports { - if transport.Type == "obfs4" { - index = i - break + for _, tcpConnect := range tk.TCPConnect { + if !tcpConnect.Status.Success { + if tcpConnect.IP != "234.345.234.345" { + t.Fatal("invalid failed gateway ip: " + fmt.Sprint(tcpConnect.IP)) + } + if tcpConnect.Port != 443 { + t.Fatal("invalid failed gateway port: " + fmt.Sprint(tcpConnect.Port)) + } } } - if index == -1 { - t.Fatal("Preconditions for the test are not met. Default eipservice string should contain obfs4 transport.") - } - - transports[index] = transports[len(transports)-1] - transports = transports[:len(transports)-1] - eipService.Gateways[1].Capabilities.Transport = transports - eipservicejson, err := json.Marshal(eipservice) - if err != nil { - t.Fatal(err) - } - requestResponseMap := map[string]string{ - eipserviceurl: string(eipservicejson), - providerurl: provider, - geoserviceurl: geoservice, - cacerturl: cacert, - openvpnurl1: "", - openvpnurl2: "", - obfs4url1: "", - } - - measurer := riseupvpn.Measurer{ - Config: riseupvpn.Config{}, - Getter: generateMockGetter(requestResponseMap, map[string]bool{ - cacerturl: true, - eipserviceurl: true, - providerurl: true, - geoserviceurl: true, - openvpnurl1: true, - openvpnurl2: true, - obfs4url1: false, - }), + if len(tk.APIFailures) != 0 { + t.Fatal("APIFailures should be empty") } +} - ctx := context.Background() - sess := &mockable.Session{MockableLogger: log.Log} +func TestSummaryKeysAlwaysReturnIsAnomalyFalse(t *testing.T) { measurement := new(model.Measurement) - callbacks := model.NewPrinterCallbacks(log.Log) - args := &model.ExperimentArgs{ - Callbacks: callbacks, - Measurement: measurement, - Session: sess, - } - err = measurer.Run(ctx, args) + m := &riseupvpn.Measurer{} + result, err := m.GetSummaryKeys(measurement) if err != nil { - t.Fatal(err) - } - tk := measurement.TestKeys.(*riseupvpn.TestKeys) - if tk.TransportStatus == nil || tk.TransportStatus["openvpn"] != "blocked" { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) + t.Fatal("GetSummaryKeys should never return an error") } - - if _, found := tk.TransportStatus["obfs"]; found { - t.Fatal("invalid TransportStatus: " + fmt.Sprint(tk.TransportStatus)) + if result.(riseupvpn.SummaryKeys).IsAnomaly { + t.Fatal("GetSummaryKeys should never return IsAnomaly true") } } @@ -681,6 +566,7 @@ func generateMockGetter(requestResponse map[string]string, responseStatus map[st responseBody, ), BodyIsTruncated: false, + Code: responseStatus, }}, }, TCPConnect: []tracex.TCPConnectEntry{tcpConnect}, @@ -703,9 +589,7 @@ func runDefaultMockTest(t *testing.T, multiGetter urlgetter.MultiGetter) *model. args := &model.ExperimentArgs{ Callbacks: model.NewPrinterCallbacks(log.Log), Measurement: measurement, - Session: &mockable.Session{ - MockableLogger: log.Log, - }, + Session: &mocks.Session{MockLogger: func() model.Logger { return log.Log }}, } err := measurer.Run(context.Background(), args)