From 82305424ffcf78be009860b5fa4b5191843b3510 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 11 Oct 2023 19:20:25 +0200 Subject: [PATCH] x --- internal/experiment/riseupvpn/riseupvpn.go | 17 +-- .../experiment/riseupvpn/riseupvpn_test.go | 105 +++++++++++++----- 2 files changed, 86 insertions(+), 36 deletions(-) diff --git a/internal/experiment/riseupvpn/riseupvpn.go b/internal/experiment/riseupvpn/riseupvpn.go index 0a9e69b261..528995eed2 100644 --- a/internal/experiment/riseupvpn/riseupvpn.go +++ b/internal/experiment/riseupvpn/riseupvpn.go @@ -6,8 +6,6 @@ package riseupvpn import ( "context" "encoding/json" - "errors" - "fmt" "time" "github.com/ooni/probe-cli/v3/internal/experiment/urlgetter" @@ -126,9 +124,6 @@ func (m Measurer) ExperimentVersion() string { return testVersion } -// ErrBootstrap indicates that the riseupvpn experiment failed to bootstrap correctly. -var ErrBootstrap = errors.New("riseupvn: bootstrap failure") - // Run implements ExperimentMeasurer.Run. func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { callbacks := args.Callbacks @@ -160,15 +155,15 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { }}, } - // Q: why fail the experiment if we cannot fetch the CA or the config? Cannot we just + // 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 testing targets to probes using check-in v2 (aka richer input). + // 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) { @@ -177,12 +172,12 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if tk.Failure != nil { testkeys.CACertStatus = false testkeys.APIFailures = append(testkeys.APIFailures, *tk.Failure) - return fmt.Errorf("%w: %s", ErrBootstrap, *tk.Failure) + return nil } if ok := certPool.AppendCertsFromPEM([]byte(tk.HTTPResponseBody)); !ok { testkeys.CACertStatus = false testkeys.APIFailures = append(testkeys.APIFailures, "invalid_ca") - return fmt.Errorf("%w: invalid_ca", ErrBootstrap) + return nil } } @@ -210,7 +205,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { testkeys.UpdateProviderAPITestKeys(entry) tk := entry.TestKeys if tk.Failure != nil { - return fmt.Errorf("%w: %s", ErrBootstrap, *tk.Failure) + return nil } } diff --git a/internal/experiment/riseupvpn/riseupvpn_test.go b/internal/experiment/riseupvpn/riseupvpn_test.go index 50c0fde67a..9370aeb75f 100644 --- a/internal/experiment/riseupvpn/riseupvpn_test.go +++ b/internal/experiment/riseupvpn/riseupvpn_test.go @@ -212,7 +212,7 @@ func TestNewExperimentMeasurer(t *testing.T) { func TestGood(t *testing.T) { // the gateaway openvpnurl2 is filtered out, since it doesn't support additionally obfs4 - measurement, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: true, @@ -222,9 +222,6 @@ func TestGood(t *testing.T) { obfs4url1: true, obfs4url2: false, })) - if err != nil { - t.Fatal(err) - } tk := measurement.TestKeys.(*riseupvpn.TestKeys) if tk.Agent != "" { @@ -343,13 +340,17 @@ func TestInvalidCaCert(t *testing.T) { Session: sess, } err := measurer.Run(ctx, args) - if !errors.Is(err, riseupvpn.ErrBootstrap) { - t.Fatal("unexpected error", err) + if err != nil { + t.Fatal(err) + } + tk := measurement.TestKeys.(*riseupvpn.TestKeys) + if tk.CACertStatus == true { + t.Fatal("unexpected CaCertStatus") } } func TestFailureCaCertFetch(t *testing.T) { - _, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: false, eipserviceurl: true, providerurl: true, @@ -358,13 +359,27 @@ func TestFailureCaCertFetch(t *testing.T) { openvpnurl2: true, obfs4url1: true, })) - if !errors.Is(err, riseupvpn.ErrBootstrap) { - t.Fatal("unexpected error", err) + + tk := measurement.TestKeys.(*riseupvpn.TestKeys) + if tk.CACertStatus != false { + t.Fatal("invalid CACertStatus ") + } + + 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("Expected a single request in this case") + } + 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") + } } } func TestFailureEipServiceBlocked(t *testing.T) { - _, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: false, providerurl: true, @@ -373,13 +388,26 @@ func TestFailureEipServiceBlocked(t *testing.T) { openvpnurl2: true, obfs4url1: true, })) - if !errors.Is(err, riseupvpn.ErrBootstrap) { - t.Fatal("unexpected error", err) + tk := measurement.TestKeys.(*riseupvpn.TestKeys) + if tk.CACertStatus != true { + t.Fatal("invalid CACertStatus") + } + + for _, entry := range tk.Requests { + if entry.Request.URL == "https://api.black.riseup.net:443/3/config/eip-service.json" { + if entry.Failure == nil { + t.Fatal("Failure for " + entry.Request.URL + " should not be null") + } + } + } + + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } func TestFailureProviderUrlBlocked(t *testing.T) { - _, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: false, @@ -388,13 +416,27 @@ func TestFailureProviderUrlBlocked(t *testing.T) { openvpnurl2: true, obfs4url1: true, })) - if !errors.Is(err, riseupvpn.ErrBootstrap) { - t.Fatal("unexpected error", err) + tk := measurement.TestKeys.(*riseupvpn.TestKeys) + + for _, entry := range tk.Requests { + if entry.Request.URL == "https://riseup.net/provider.json" { + if entry.Failure == nil { + t.Fatal("Failure for " + entry.Request.URL + " should not be null") + } + } + } + + if tk.CACertStatus != true { + t.Fatal("invalid CACertStatus ") + } + + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } func TestFailureGeoIpServiceBlocked(t *testing.T) { - _, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: true, @@ -403,13 +445,26 @@ func TestFailureGeoIpServiceBlocked(t *testing.T) { openvpnurl2: true, obfs4url1: true, })) - if !errors.Is(err, riseupvpn.ErrBootstrap) { - t.Fatal("unexpected error", err) + tk := measurement.TestKeys.(*riseupvpn.TestKeys) + if tk.CACertStatus != true { + t.Fatal("invalid CACertStatus ") + } + + for _, entry := range tk.Requests { + if entry.Request.URL == "https://api.black.riseup.net:9001/json" { + if entry.Failure == nil { + t.Fatal("Failure for " + entry.Request.URL + " should not be null") + } + } + } + + if len(tk.APIFailures) <= 0 { + t.Fatal("APIFailures should not be empty") } } func TestFailureGateway1TransportNOK(t *testing.T) { - measurement, err := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ + measurement := runDefaultMockTest(t, generateDefaultMockGetter(map[string]bool{ cacerturl: true, eipserviceurl: true, providerurl: true, @@ -419,10 +474,6 @@ func TestFailureGateway1TransportNOK(t *testing.T) { obfs4url1: true, obfs4url2: false, })) - if err != nil { - t.Fatal(err) - } - tk := measurement.TestKeys.(*riseupvpn.TestKeys) if tk.CACertStatus != true { t.Fatal("invalid CACertStatus ") @@ -528,7 +579,7 @@ func generateDefaultMockGetter(responseStatuses map[string]bool) urlgetter.Multi return generateMockGetter(RequestResponse, responseStatuses) } -func runDefaultMockTest(t *testing.T, multiGetter urlgetter.MultiGetter) (*model.Measurement, error) { +func runDefaultMockTest(t *testing.T, multiGetter urlgetter.MultiGetter) *model.Measurement { measurer := riseupvpn.Measurer{ Config: riseupvpn.Config{}, Getter: multiGetter, @@ -541,5 +592,9 @@ func runDefaultMockTest(t *testing.T, multiGetter urlgetter.MultiGetter) (*model Session: &mocks.Session{MockLogger: func() model.Logger { return log.Log }}, } err := measurer.Run(context.Background(), args) - return measurement, err + + if err != nil { + t.Fatal(err) + } + return measurement }