From 65df4399bfd4ad08249c5f60cb6825fe6c98328b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 16:49:42 +0200 Subject: [PATCH 1/3] refactor(probeservices): use better naming (#1628) Calling the endpoint base URL (e.g., `https://www.example.com/`) "endpoint" could cause confusion because usually an API endpoint is something like `https://www.example.com/api/v1/check-in`. To obviate this semantic issue, this diff attempts to avoid calling base URLs as endpoint throughout the tree. I am going to account this work as part of https://github.com/ooni/backend/issues/754 because this is one of the possibly issues on which we can account this work. The original conversation with @ainghazal, which prodded me to make these changes, was related to his feedback after implementing https://github.com/ooni/probe-cli/pull/1625. --- .../engine/experiment_integration_test.go | 4 +- internal/engine/session.go | 4 +- internal/engine/session_integration_test.go | 2 +- internal/probeservices/benchselect.go | 23 ++-- internal/probeservices/probeservices.go | 20 +-- internal/probeservices/probeservices_test.go | 116 +++++++++--------- 6 files changed, 86 insertions(+), 83 deletions(-) diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index 1d9a7f91a3..26a37509b4 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -3,6 +3,7 @@ package engine import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "os" @@ -11,6 +12,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/probeservices" "github.com/ooni/probe-cli/v3/internal/registry" ) @@ -384,7 +386,7 @@ func TestOpenReportNewClientFailure(t *testing.T) { Type: "antani", } err = exp.OpenReportContext(context.Background()) - if err.Error() != "probe services: unsupported endpoint type" { + if !errors.Is(err, probeservices.ErrUnsupportedServiceType) { t.Fatal(err) } } diff --git a/internal/engine/session.go b/internal/engine/session.go index 1a6bc5443b..fbd17d22bf 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -673,8 +673,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error { if selected == nil { return ErrAllProbeServicesFailed } - s.logger.Infof("session: using probe services: %+v", selected.Endpoint) - s.selectedProbeService = &selected.Endpoint + s.logger.Infof("session: using probe services: %+v", selected.Service) + s.selectedProbeService = &selected.Service s.availableTestHelpers = selected.TestHelpers return nil } diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go index 77a1e22fc4..9f211e930e 100644 --- a/internal/engine/session_integration_test.go +++ b/internal/engine/session_integration_test.go @@ -483,7 +483,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) { svc.Type = "antani" // should really not be supported for a long time } client, err := sess.newOrchestraClient(context.Background()) - if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) { + if !errors.Is(err, probeservices.ErrUnsupportedServiceType) { t.Fatal("not the error we expected") } if client != nil { diff --git a/internal/probeservices/benchselect.go b/internal/probeservices/benchselect.go index 154cc4a2af..894100d256 100644 --- a/internal/probeservices/benchselect.go +++ b/internal/probeservices/benchselect.go @@ -19,8 +19,8 @@ func Default() []model.OOAPIService { }} } -// SortEndpoints gives priority to https, then cloudfronted, then onion. -func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) { +// SortServices gives priority to https, then cloudfronted, then onion. +func SortServices(in []model.OOAPIService) (out []model.OOAPIService) { for _, entry := range in { if entry.Type == "https" { out = append(out, entry) @@ -39,7 +39,7 @@ func SortEndpoints(in []model.OOAPIService) (out []model.OOAPIService) { return } -// OnlyHTTPS returns the HTTPS endpoints only. +// OnlyHTTPS returns the HTTPS services only. func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) { for _, entry := range in { if entry.Type == "https" { @@ -49,9 +49,9 @@ func OnlyHTTPS(in []model.OOAPIService) (out []model.OOAPIService) { return } -// OnlyFallbacks returns the fallback endpoints only. +// OnlyFallbacks returns the fallback services only. func OnlyFallbacks(in []model.OOAPIService) (out []model.OOAPIService) { - for _, entry := range SortEndpoints(in) { + for _, entry := range SortServices(in) { if entry.Type != "https" { out = append(out, entry) } @@ -67,15 +67,16 @@ type Candidate struct { // Err indicates whether the service works. Err error - // Endpoint is the service endpoint. - Endpoint model.OOAPIService + // Service is the service to use. + Service model.OOAPIService - // TestHelpers contains the data returned by the endpoint. + // TestHelpers contains the data returned by the service when + // querying the /api/v1/test-helpers API endpoint. TestHelpers map[string][]model.OOAPIService } func (c *Candidate) try(ctx context.Context, sess Session) { - client, err := NewClient(sess, c.Endpoint) + client, err := NewClient(sess, c.Service) if err != nil { c.Err = err return @@ -85,11 +86,11 @@ func (c *Candidate) try(ctx context.Context, sess Session) { c.Duration = time.Since(start) c.Err = err c.TestHelpers = testhelpers - sess.Logger().Debugf("probe services: %+v: %+v %s", c.Endpoint, err, c.Duration) + sess.Logger().Debugf("probe services: %+v: %+v %s", c.Service, err, c.Duration) } func try(ctx context.Context, sess Session, svc model.OOAPIService) *Candidate { - candidate := &Candidate{Endpoint: svc} + candidate := &Candidate{Service: svc} candidate.try(ctx, sess) return candidate } diff --git a/internal/probeservices/probeservices.go b/internal/probeservices/probeservices.go index 16c8a195cf..c7313610c8 100644 --- a/internal/probeservices/probeservices.go +++ b/internal/probeservices/probeservices.go @@ -1,6 +1,6 @@ // Package probeservices contains code to contact OONI probe services. // -// The probe services are HTTPS endpoints distributed across a bunch of data +// The probe services are HTTPS services distributed across a bunch of data // centres implementing a bunch of OONI APIs. When started, OONI will benchmark // the available probe services and select the fastest one. Eventually all the // possible OONI APIs will run as probe services. @@ -32,8 +32,8 @@ import ( ) var ( - // ErrUnsupportedEndpoint indicates that we don't support this endpoint type. - ErrUnsupportedEndpoint = errors.New("probe services: unsupported endpoint type") + // ErrUnsupportedServiceType indicates that we don't support this service type. + ErrUnsupportedServiceType = errors.New("probe services: unsupported service type") // ErrUnsupportedCloudFrontAddress indicates that we don't support this // cloudfront address (e.g. wrong scheme, presence of port). @@ -90,11 +90,11 @@ func (c Client) GetCredsAndAuth() (*model.OOAPILoginCredentials, *model.OOAPILog return creds, auth, nil } -// NewClient creates a new client for the specified probe services endpoint. This -// function fails, e.g., we don't support the specified endpoint. -func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { +// NewClient creates a new client for the specified probe services service. This +// function fails, e.g., we don't support the specified service. +func NewClient(sess Session, service model.OOAPIService) (*Client, error) { client := &Client{ - BaseURL: endpoint.Address, + BaseURL: service.Address, HTTPClient: sess.DefaultHTTPClient(), Host: "", KVStore: sess.KeyValueStore(), @@ -104,7 +104,7 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { StateFile: NewStateFile(sess.KeyValueStore()), UserAgent: sess.UserAgent(), } - switch endpoint.Type { + switch service.Type { case "https": return client, nil case "cloudfront": @@ -119,13 +119,13 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) { return nil, ErrUnsupportedCloudFrontAddress } client.Host = URL.Hostname() - URL.Host = endpoint.Front + URL.Host = service.Front client.BaseURL = URL.String() if _, err := url.Parse(client.BaseURL); err != nil { return nil, err } return client, nil default: - return nil, ErrUnsupportedEndpoint + return nil, ErrUnsupportedServiceType } } diff --git a/internal/probeservices/probeservices_test.go b/internal/probeservices/probeservices_test.go index dcb32667bf..5f31d75c8d 100644 --- a/internal/probeservices/probeservices_test.go +++ b/internal/probeservices/probeservices_test.go @@ -47,13 +47,13 @@ func TestNewClientHTTPS(t *testing.T) { } } -func TestNewClientUnsupportedEndpoint(t *testing.T) { +func TestNewClientUnsupportedService(t *testing.T) { client, err := NewClient( &mockable.Session{}, model.OOAPIService{ Address: "https://x.org", Type: "onion", }) - if !errors.Is(err, ErrUnsupportedEndpoint) { + if !errors.Is(err, ErrUnsupportedServiceType) { t.Fatal("not the error we expected") } if client != nil { @@ -193,7 +193,7 @@ func TestDefaultProbeServicesWorkAsIntended(t *testing.T) { } } -func TestSortEndpoints(t *testing.T) { +func TestSortServices(t *testing.T) { in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -216,7 +216,7 @@ func TestSortEndpoints(t *testing.T) { Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", }} - out := SortEndpoints(in) + out := SortServices(in) diff := cmp.Diff(out, expect) if diff != "" { t.Fatal(diff) @@ -259,7 +259,7 @@ func TestOnlyHTTPS(t *testing.T) { } func TestOnlyFallbacks(t *testing.T) { - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -293,7 +293,7 @@ func TestOnlyFallbacks(t *testing.T) { } func TestTryAllCanceledContext(t *testing.T) { - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -328,11 +328,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[0].Err, context.Canceled) { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://ams-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[0].Service.Address != "https://ams-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[1].Duration <= 0 { @@ -341,11 +341,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[1].Err, context.Canceled) { t.Fatal("invalid error") } - if out[1].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[1].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[1].Endpoint.Address != "https://hkg-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[1].Service.Address != "https://hkg-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[2].Duration <= 0 { @@ -354,11 +354,11 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[2].Err, context.Canceled) { t.Fatal("invalid error") } - if out[2].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[2].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[2].Endpoint.Address != "https://mia-ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[2].Service.Address != "https://mia-ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[3].Duration <= 0 { @@ -367,28 +367,28 @@ func TestTryAllCanceledContext(t *testing.T) { if !errors.Is(out[3].Err, context.Canceled) { t.Fatal("invalid error") } - if out[3].Endpoint.Type != "cloudfront" { - t.Fatal("invalid endpoint type") + if out[3].Service.Type != "cloudfront" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Front != "dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Front != "dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } // - // Note: here duration may be zero because the endpoint is not supported + // Note: here duration may be zero because the service is not supported // and so we don't basically do anything. But it also may be nonzero since // we also run tests in the cloud, which is slower than my desktop. So, I // have not written a specific test concerning out[4].Duration. - if !errors.Is(out[4].Err, ErrUnsupportedEndpoint) { + if !errors.Is(out[4].Err, ErrUnsupportedServiceType) { t.Fatal("invalid error") } - if out[4].Endpoint.Type != "onion" { - t.Fatal("invalid endpoint type") + if out[4].Service.Type != "onion" { + t.Fatal("invalid service type") } - if out[4].Endpoint.Address != "httpo://jehhrikjjqrlpufu.onion" { - t.Fatal("invalid endpoint type") + if out[4].Service.Address != "httpo://jehhrikjjqrlpufu.onion" { + t.Fatal("invalid service type") } } @@ -396,7 +396,7 @@ func TestTryAllIntegrationWeRaceForFastestHTTPS(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -423,11 +423,11 @@ func TestTryAllIntegrationWeRaceForFastestHTTPS(t *testing.T) { if out[0].Err != nil { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://api.ooni.io" { - t.Fatal("invalid endpoint address") + if out[0].Service.Address != "https://api.ooni.io" { + t.Fatal("invalid service address") } } @@ -435,7 +435,7 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } - // put onion first so we also verify that we sort the endpoints + // put onion first so we also verify that we sort the services in := []model.OOAPIService{{ Type: "onion", Address: "httpo://jehhrikjjqrlpufu.onion", @@ -468,11 +468,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[0].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[0].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[0].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[0].Endpoint.Address != "https://ps-nonexistent.ooni.io" { - t.Fatal("invalid endpoint type") + if out[0].Service.Address != "https://ps-nonexistent.ooni.io" { + t.Fatal("invalid service type") } // if out[1].Duration <= 0 { @@ -481,11 +481,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[1].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[1].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[1].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[1].Endpoint.Address != "https://hkg-ps-nonexistent.ooni.nu" { - t.Fatal("invalid endpoint type") + if out[1].Service.Address != "https://hkg-ps-nonexistent.ooni.nu" { + t.Fatal("invalid service type") } // if out[2].Duration <= 0 { @@ -494,11 +494,11 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if !strings.HasSuffix(out[2].Err.Error(), "no such host") { t.Fatal("invalid error") } - if out[2].Endpoint.Type != "https" { - t.Fatal("invalid endpoint type") + if out[2].Service.Type != "https" { + t.Fatal("invalid service type") } - if out[2].Endpoint.Address != "https://mia-ps2-nonexistent.ooni.nu" { - t.Fatal("invalid endpoint type") + if out[2].Service.Address != "https://mia-ps2-nonexistent.ooni.nu" { + t.Fatal("invalid service type") } // if out[3].Duration <= 0 { @@ -507,13 +507,13 @@ func TestTryAllIntegrationWeFallback(t *testing.T) { if out[3].Err != nil { t.Fatal("invalid error") } - if out[3].Endpoint.Type != "cloudfront" { - t.Fatal("invalid endpoint type") + if out[3].Service.Type != "cloudfront" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { - t.Fatal("invalid endpoint type") + if out[3].Service.Address != "https://dkyhjv0wpi2dk.cloudfront.net" { + t.Fatal("invalid service type") } - if out[3].Endpoint.Front != "dkyhjv0wpi2dk.cloudfront.net" { + if out[3].Service.Front != "dkyhjv0wpi2dk.cloudfront.net" { t.Fatal("invalid front") } } @@ -537,32 +537,32 @@ func TestSelectBestOnlyFailures(t *testing.T) { func TestSelectBestSelectsTheFastest(t *testing.T) { in := []*Candidate{{ Duration: 10 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps1.ooni.nonexistent", Type: "https", }, }, { Duration: 4 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps2.ooni.nonexistent", Type: "https", }, }, { Duration: 7 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps3.ooni.nonexistent", Type: "https", }, }, { Duration: 11 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps4.ooni.nonexistent", Type: "https", }, }} expected := &Candidate{ Duration: 4 * time.Millisecond, - Endpoint: model.OOAPIService{ + Service: model.OOAPIService{ Address: "https://ps2.ooni.nonexistent", Type: "https", }, From acab9021c2039426b66ea5ff3300ed6c0031bf2e Mon Sep 17 00:00:00 2001 From: Ain Ghazal <99027643+ainghazal@users.noreply.github.com> Date: Tue, 25 Jun 2024 18:27:24 +0200 Subject: [PATCH 2/3] feat(openvpn): implement richer input (#1625) This commit: 1. modifies `./internal/registry` and its `openvpn.go` file such that `openvpn` has its own private target loader; 2. modifies `./internal/experiment/openvpn` to use the richer input targets to merge the options for the openvpn experiment. 3. removes cache from session after fetching openvpn config ## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: https://github.com/ooni/probe/issues/2600 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request - [x] if you changed code inside an experiment, make sure you bump its version number --------- Co-authored-by: Simone Basso --- internal/engine/session.go | 19 +- internal/experiment/dnscheck/dnscheck_test.go | 2 +- internal/experiment/openvpn/endpoint.go | 124 ++------ internal/experiment/openvpn/endpoint_test.go | 128 +++----- internal/experiment/openvpn/openvpn.go | 234 ++++----------- internal/experiment/openvpn/openvpn_test.go | 284 +++++------------- internal/experiment/openvpn/richerinput.go | 149 +++++++++ .../experiment/openvpn/richerinput_test.go | 218 ++++++++++++++ internal/model/experiment.go | 3 + internal/registry/openvpn.go | 5 +- internal/targetloading/targetloading.go | 57 +--- internal/targetloading/targetloading_test.go | 101 +------ 12 files changed, 593 insertions(+), 731 deletions(-) create mode 100644 internal/experiment/openvpn/richerinput.go create mode 100644 internal/experiment/openvpn/richerinput_test.go diff --git a/internal/engine/session.go b/internal/engine/session.go index fbd17d22bf..740ed549ee 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -66,7 +66,6 @@ type Session struct { softwareName string softwareVersion string tempDir string - vpnConfig map[string]model.OOAPIVPNProviderConfig // closeOnce allows us to call Close just once. closeOnce sync.Once @@ -178,7 +177,6 @@ func NewSession(ctx context.Context, config SessionConfig) (*Session, error) { torArgs: config.TorArgs, torBinary: config.TorBinary, tunnelDir: config.TunnelDir, - vpnConfig: make(map[string]model.OOAPIVPNProviderConfig), } proxyURL := config.ProxyURL if proxyURL != nil { @@ -381,15 +379,23 @@ func (s *Session) FetchTorTargets( // internal cache. We do this to avoid hitting the API for every input. func (s *Session) FetchOpenVPNConfig( ctx context.Context, provider, cc string) (*model.OOAPIVPNProviderConfig, error) { - if config, ok := s.vpnConfig[provider]; ok { - return &config, nil - } clnt, err := s.newOrchestraClient(ctx) if err != nil { return nil, err } - // we cannot lock earlier because newOrchestraClient locks the mutex. + // ensure that we have fetched the location before fetching openvpn configuration. + if err := s.MaybeLookupLocationContext(ctx); err != nil { + return nil, err + } + + // IMPORTANT! + // + // We cannot lock earlier because newOrchestraClient and + // MaybeLookupLocation both lock the mutex. + // + // TODO(bassosimone,DecFox): we should consider using the same strategy we used for the + // experiments, where we separated mutable state into dedicated types. defer s.mu.Unlock() s.mu.Lock() @@ -397,7 +403,6 @@ func (s *Session) FetchOpenVPNConfig( if err != nil { return nil, err } - s.vpnConfig[provider] = config return &config, nil } diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index 8f706ffa40..bf8f895554 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -60,7 +60,7 @@ func TestDNSCheckFailsWithInvalidInputType(t *testing.T) { } err := measurer.Run(context.Background(), args) if !errors.Is(err, ErrInvalidInputType) { - t.Fatal("expected no input error") + t.Fatal("expected invalid-input-type error") } } diff --git a/internal/experiment/openvpn/endpoint.go b/internal/experiment/openvpn/endpoint.go index 6289d75cda..6c5157d525 100644 --- a/internal/experiment/openvpn/endpoint.go +++ b/internal/experiment/openvpn/endpoint.go @@ -1,22 +1,21 @@ package openvpn import ( - "encoding/base64" - "errors" "fmt" - "math/rand" "net" "net/url" + "slices" "strings" vpnconfig "github.com/ooni/minivpn/pkg/config" vpntracex "github.com/ooni/minivpn/pkg/tracex" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) var ( - // ErrBadBase64Blob is the error returned when we cannot decode an option passed as base64. - ErrBadBase64Blob = errors.New("wrong base64 encoding") + ErrInputRequired = targetloading.ErrInputRequired + ErrInvalidInput = targetloading.ErrInvalidInput ) // endpoint is a single endpoint to be probed. @@ -49,6 +48,9 @@ type endpoint struct { // "openvpn://provider.corp/?address=1.2.3.4:1194&transport=udp // "openvpn+obfs4://provider.corp/address=1.2.3.4:1194?&cert=deadbeef&iat=0" func newEndpointFromInputString(uri string) (*endpoint, error) { + if uri == "" { + return nil, ErrInputRequired + } parsedURL, err := url.Parse(uri) if err != nil { return nil, fmt.Errorf("%w: %s", ErrInvalidInput, err) @@ -146,90 +148,31 @@ func (e *endpoint) AsInputURI() string { return url.String() } -// endpointList is a list of endpoints. -type endpointList []*endpoint - -// DefaultEndpoints contains a subset of known endpoints to be used if no input is passed to the experiment and -// the backend query fails for whatever reason. We risk distributing endpoints that can go stale, so we should be careful about -// the stability of the endpoints selected here, but in restrictive environments it's useful to have something -// to probe in absence of an useful OONI API. Valid credentials are still needed, though. -var DefaultEndpoints = endpointList{ - { - Provider: "riseup", - IPAddr: "51.15.187.53", - Port: "1194", - Protocol: "openvpn", - Transport: "tcp", - }, - { - Provider: "riseup", - IPAddr: "51.15.187.53", - Port: "1194", - Protocol: "openvpn", - Transport: "udp", - }, -} - -// Shuffle randomizes the order of items in the endpoint list. -func (e endpointList) Shuffle() endpointList { - rand.Shuffle(len(e), func(i, j int) { - e[i], e[j] = e[j], e[i] - }) - return e -} - -// defaultOptionsByProvider is a map containing base config for -// all the known providers. We extend this base config with credentials coming -// from the OONI API. -var defaultOptionsByProvider = map[string]*vpnconfig.OpenVPNOptions{ - "riseupvpn": { - Auth: "SHA512", - Cipher: "AES-256-GCM", - }, -} - // APIEnabledProviders is the list of providers that the stable API Endpoint knows about. // This array will be a subset of the keys in defaultOptionsByProvider, but it might make sense // to still register info about more providers that the API officially knows about. var APIEnabledProviders = []string{ - // TODO(ainghazal): fix the backend so that we can remove the spurious "vpn" suffix here. "riseupvpn", } -// isValidProvider returns true if the provider is found as key in the registry of defaultOptionsByProvider. -// TODO(ainghazal): consolidate with list of enabled providers from the API viewpoint. +// isValidProvider returns true if the provider is found as key in the array of [APIEnabledProviders]. func isValidProvider(provider string) bool { - _, ok := defaultOptionsByProvider[provider] - return ok + return slices.Contains(APIEnabledProviders, provider) } -// getOpenVPNConfig gets a properly configured [*vpnconfig.Config] object for the given endpoint. -// To obtain that, we merge the endpoint specific configuration with base options. -// Base options are hardcoded for the moment, for comparability among different providers. -// We can add them to the OONI API and as extra cli options if ever needed. -func getOpenVPNConfig( +// newOpenVPNConfig returns a properly configured [*vpnconfig.Config] object for the given endpoint. +// To obtain that, we merge the endpoint specific configuration with the options passed as richer input targets. +func newOpenVPNConfig( tracer *vpntracex.Tracer, logger model.Logger, endpoint *endpoint, - creds *vpnconfig.OpenVPNOptions) (*vpnconfig.Config, error) { - // TODO(ainghazal): use merge ability in vpnconfig.OpenVPNOptions merge (pending PR) + config *Config) (*vpnconfig.Config, error) { + provider := endpoint.Provider if !isValidProvider(provider) { return nil, fmt.Errorf("%w: unknown provider: %s", ErrInvalidInput, provider) } - baseOptions := defaultOptionsByProvider[provider] - - if baseOptions == nil { - return nil, fmt.Errorf("empty baseOptions for provider: %s", provider) - } - if baseOptions.Cipher == "" { - return nil, fmt.Errorf("empty cipher for provider: %s", provider) - } - if baseOptions.Auth == "" { - return nil, fmt.Errorf("empty auth for provider: %s", provider) - } - cfg := vpnconfig.NewConfig( vpnconfig.WithLogger(logger), vpnconfig.WithOpenVPNOptions( @@ -239,14 +182,13 @@ func getOpenVPNConfig( Port: endpoint.Port, Proto: vpnconfig.Proto(endpoint.Transport), - // options coming from the default known values. - Cipher: baseOptions.Cipher, - Auth: baseOptions.Auth, - - // auth coming from passed credentials. - CA: creds.CA, - Cert: creds.Cert, - Key: creds.Key, + // options and credentials come from the experiment + // richer input targets. + Cipher: config.Cipher, + Auth: config.Auth, + CA: []byte(config.SafeCA), + Cert: []byte(config.SafeCert), + Key: []byte(config.SafeKey), }, ), vpnconfig.WithHandshakeTracer(tracer), @@ -254,27 +196,3 @@ func getOpenVPNConfig( return cfg, nil } - -// maybeExtractBase64Blob is used to pass credentials as command-line options. -func maybeExtractBase64Blob(val string) (string, error) { - s := strings.TrimPrefix(val, "base64:") - if len(s) == len(val) { - // no prefix, so we'll treat this as a pem-encoded credential. - return s, nil - } - dec, err := base64.URLEncoding.DecodeString(strings.TrimSpace(s)) - if err != nil { - return "", fmt.Errorf("%w: %s", ErrBadBase64Blob, err) - } - return string(dec), nil -} - -func isValidProtocol(s string) bool { - if strings.HasPrefix(s, "openvpn://") { - return true - } - if strings.HasPrefix(s, "openvpn+obfs4://") { - return true - } - return false -} diff --git a/internal/experiment/openvpn/endpoint_test.go b/internal/experiment/openvpn/endpoint_test.go index bc33301419..bfd3eb6027 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -3,12 +3,10 @@ package openvpn import ( "errors" "fmt" - "sort" "testing" "time" "github.com/google/go-cmp/cmp" - vpnconfig "github.com/ooni/minivpn/pkg/config" vpntracex "github.com/ooni/minivpn/pkg/tracex" ) @@ -23,7 +21,25 @@ func Test_newEndpointFromInputString(t *testing.T) { wantErr error }{ { - name: "valid endpoint returns good endpoint", + name: "empty input returns error", + args: args{""}, + want: nil, + wantErr: ErrInputRequired, + }, + { + name: "invalid protocol returns error", + args: args{"bad://foo.bar"}, + want: nil, + wantErr: ErrInvalidInput, + }, + { + name: "uri with illegal chars returns error", + args: args{"openvpn://\x7f/#"}, + want: nil, + wantErr: ErrInvalidInput, + }, + { + name: "valid input uri returns good endpoint", args: args{"openvpn://riseupvpn.corp/?address=1.1.1.1:1194&transport=tcp"}, want: &endpoint{ IPAddr: "1.1.1.1", @@ -253,16 +269,6 @@ func Test_endpoint_String(t *testing.T) { } } -func Test_endpointList_Shuffle(t *testing.T) { - shuffled := DefaultEndpoints.Shuffle() - sort.Slice(shuffled, func(i, j int) bool { - return shuffled[i].IPAddr < shuffled[j].IPAddr - }) - if diff := cmp.Diff(shuffled, DefaultEndpoints); diff != "" { - t.Error(diff) - } -} - func Test_isValidProvider(t *testing.T) { if valid := isValidProvider("riseupvpn"); !valid { t.Fatal("riseup is the only valid provider now") @@ -272,7 +278,7 @@ func Test_isValidProvider(t *testing.T) { } } -func Test_getVPNConfig(t *testing.T) { +func Test_newVPNConfig(t *testing.T) { tracer := vpntracex.NewTracer(time.Now()) e := &endpoint{ Provider: "riseupvpn", @@ -280,13 +286,16 @@ func Test_getVPNConfig(t *testing.T) { Port: "443", Transport: "udp", } - creds := &vpnconfig.OpenVPNOptions{ - CA: []byte("ca"), - Cert: []byte("cert"), - Key: []byte("key"), + + config := &Config{ + Auth: "SHA512", + Cipher: "AES-256-GCM", + SafeCA: "ca", + SafeCert: "cert", + SafeKey: "key", } - cfg, err := getOpenVPNConfig(tracer, nil, e, creds) + cfg, err := newOpenVPNConfig(tracer, nil, e, config) if err != nil { t.Fatalf("did not expect error, got: %v", err) } @@ -311,18 +320,18 @@ func Test_getVPNConfig(t *testing.T) { if transport := cfg.OpenVPNOptions().Proto; string(transport) != e.Transport { t.Errorf("expected transport %s, got %s", e.Transport, transport) } - if diff := cmp.Diff(cfg.OpenVPNOptions().CA, creds.CA); diff != "" { + if diff := cmp.Diff(cfg.OpenVPNOptions().CA, []byte(config.SafeCA)); diff != "" { t.Error(diff) } - if diff := cmp.Diff(cfg.OpenVPNOptions().Cert, creds.Cert); diff != "" { + if diff := cmp.Diff(cfg.OpenVPNOptions().Cert, []byte(config.SafeCert)); diff != "" { t.Error(diff) } - if diff := cmp.Diff(cfg.OpenVPNOptions().Key, creds.Key); diff != "" { + if diff := cmp.Diff(cfg.OpenVPNOptions().Key, []byte(config.SafeKey)); diff != "" { t.Error(diff) } } -func Test_getVPNConfig_with_unknown_provider(t *testing.T) { +func Test_mergeOpenVPNConfig_with_unknown_provider(t *testing.T) { tracer := vpntracex.NewTracer(time.Now()) e := &endpoint{ Provider: "nsa", @@ -330,76 +339,13 @@ func Test_getVPNConfig_with_unknown_provider(t *testing.T) { Port: "443", Transport: "udp", } - creds := &vpnconfig.OpenVPNOptions{ - CA: []byte("ca"), - Cert: []byte("cert"), - Key: []byte("key"), + cfg := &Config{ + SafeCA: "ca", + SafeCert: "cert", + SafeKey: "key", } - _, err := getOpenVPNConfig(tracer, nil, e, creds) + _, err := newOpenVPNConfig(tracer, nil, e, cfg) if !errors.Is(err, ErrInvalidInput) { t.Fatalf("expected invalid input error, got: %v", err) } - -} - -func Test_extractBase64Blob(t *testing.T) { - t.Run("decode good blob", func(t *testing.T) { - blob := "base64:dGhlIGJsdWUgb2N0b3B1cyBpcyB3YXRjaGluZw==" - decoded, err := maybeExtractBase64Blob(blob) - if decoded != "the blue octopus is watching" { - t.Fatal("could not decoded blob correctly") - } - if err != nil { - t.Fatal("should not fail with first blob") - } - }) - t.Run("try decode without prefix", func(t *testing.T) { - blob := "dGhlIGJsdWUgb2N0b3B1cyBpcyB3YXRjaGluZw==" - dec, err := maybeExtractBase64Blob(blob) - if err != nil { - t.Fatal("should fail without prefix") - } - if dec != blob { - t.Fatal("decoded should be the same") - } - }) - t.Run("bad base64 blob should fail", func(t *testing.T) { - blob := "base64:dGhlIGJsdWUgb2N0b3B1cyBpcyB3YXRjaGluZw" - _, err := maybeExtractBase64Blob(blob) - if !errors.Is(err, ErrBadBase64Blob) { - t.Fatal("bad blob should fail without prefix") - } - }) - t.Run("decode empty blob", func(t *testing.T) { - blob := "base64:" - _, err := maybeExtractBase64Blob(blob) - if err != nil { - t.Fatal("empty blob should not fail") - } - }) - t.Run("illegal base64 data should fail", func(t *testing.T) { - blob := "base64:==" - _, err := maybeExtractBase64Blob(blob) - if !errors.Is(err, ErrBadBase64Blob) { - t.Fatal("bad base64 data should fail") - } - }) -} - -func Test_IsValidProtocol(t *testing.T) { - t.Run("openvpn is valid", func(t *testing.T) { - if !isValidProtocol("openvpn://foobar.bar") { - t.Error("openvpn:// should be a valid protocol") - } - }) - t.Run("openvpn+obfs4 is valid", func(t *testing.T) { - if !isValidProtocol("openvpn+obfs4://foobar.bar") { - t.Error("openvpn+obfs4:// should be a valid protocol") - } - }) - t.Run("openvpn+other is not valid", func(t *testing.T) { - if isValidProtocol("openvpn+ss://foobar.bar") { - t.Error("openvpn+ss:// should not be a valid protocol") - } - }) } diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 2d2af0e01b..0cf8255f43 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -3,14 +3,12 @@ package openvpn import ( "context" - "errors" - "fmt" "strconv" - "strings" "time" "github.com/ooni/probe-cli/v3/internal/measurexlite" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" vpnconfig "github.com/ooni/minivpn/pkg/config" vpntracex "github.com/ooni/minivpn/pkg/tracex" @@ -18,8 +16,13 @@ import ( ) const ( - testVersion = "0.1.2" - openVPNProcol = "openvpn" + testName = "openvpn" + testVersion = "0.1.3" + openVPNProtocol = "openvpn" +) + +var ( + ErrInvalidInputType = targetloading.ErrInvalidInputType ) // Config contains the experiment config. @@ -92,41 +95,23 @@ func (tk *TestKeys) AllConnectionsSuccessful() bool { } // Measurer performs the measurement. -type Measurer struct { - config Config - testName string -} +type Measurer struct{} // NewExperimentMeasurer creates a new ExperimentMeasurer. -func NewExperimentMeasurer(config Config, testName string) model.ExperimentMeasurer { - return Measurer{config: config, testName: testName} +func NewExperimentMeasurer() model.ExperimentMeasurer { + return &Measurer{} } // ExperimentName implements model.ExperimentMeasurer.ExperimentName. -func (m Measurer) ExperimentName() string { - return m.testName +func (m *Measurer) ExperimentName() string { + return testName } // ExperimentVersion implements model.ExperimentMeasurer.ExperimentVersion. -func (m Measurer) ExperimentVersion() string { +func (m *Measurer) ExperimentVersion() string { return testVersion } -var ( - // ErrInvalidInput is returned if we failed to parse the input to obtain an endpoint we can measure. - ErrInvalidInput = errors.New("invalid input") -) - -func parseEndpoint(m *model.Measurement) (*endpoint, error) { - if m.Input != "" { - if ok := isValidProtocol(string(m.Input)); !ok { - return nil, ErrInvalidInput - } - return newEndpointFromInputString(string(m.Input)) - } - return nil, fmt.Errorf("%w: %s", ErrInvalidInput, "input is mandatory") -} - // AuthMethod is the authentication method used by a provider. type AuthMethod string @@ -138,130 +123,6 @@ var ( AuthUserPass = AuthMethod("userpass") ) -var providerAuthentication = map[string]AuthMethod{ - "riseup": AuthCertificate, - "tunnelbear": AuthUserPass, - "surfshark": AuthUserPass, -} - -func hasCredentialsInOptions(cfg Config, method AuthMethod) bool { - switch method { - case AuthCertificate: - ok := cfg.SafeCA != "" && cfg.SafeCert != "" && cfg.SafeKey != "" - return ok - default: - return false - } -} - -// MaybeGetCredentialsFromOptions overrides authentication info with what user provided in options. -// Each certificate/key can be encoded in base64 so that a single option can be safely represented as command line options. -// This function returns no error if there are no credentials in the passed options, only if failing to parse them. -func MaybeGetCredentialsFromOptions(cfg Config, opts *vpnconfig.OpenVPNOptions, method AuthMethod) (bool, error) { - if ok := hasCredentialsInOptions(cfg, method); !ok { - return false, nil - } - ca, err := maybeExtractBase64Blob(cfg.SafeCA) - if err != nil { - return false, err - } - opts.CA = []byte(ca) - - key, err := maybeExtractBase64Blob(cfg.SafeKey) - if err != nil { - return false, err - } - opts.Key = []byte(key) - - cert, err := maybeExtractBase64Blob(cfg.SafeCert) - if err != nil { - return false, err - } - opts.Cert = []byte(cert) - return true, nil -} - -func (m *Measurer) getCredentialsFromAPI( - ctx context.Context, - sess model.ExperimentSession, - provider string, - opts *vpnconfig.OpenVPNOptions) error { - // We expect the credentials from the API response to be encoded as the direct PEM serialization. - apiCreds, err := m.FetchProviderCredentials(ctx, sess, provider) - // TODO(ainghazal): validate credentials have the info we expect, certs are not expired etc. - if err != nil { - sess.Logger().Warnf("Error fetching credentials from API: %s", err.Error()) - return err - } - sess.Logger().Infof("Got credentials from provider: %s", provider) - - opts.CA = []byte(apiCreds.Config.CA) - opts.Cert = []byte(apiCreds.Config.Cert) - opts.Key = []byte(apiCreds.Config.Key) - return nil -} - -// GetCredentialsFromOptionsOrAPI attempts to find valid credentials for the given provider, either -// from the passed Options (cli, oonirun), or from a remote call to the OONI API endpoint. -func (m *Measurer) GetCredentialsFromOptionsOrAPI( - ctx context.Context, - sess model.ExperimentSession, - provider string) (*vpnconfig.OpenVPNOptions, error) { - - method, ok := providerAuthentication[strings.TrimSuffix(provider, "vpn")] - if !ok { - return nil, fmt.Errorf("%w: provider auth unknown: %s", ErrInvalidInput, provider) - } - - // Empty options object to fill with credentials. - creds := &vpnconfig.OpenVPNOptions{} - - switch method { - case AuthCertificate: - ok, err := MaybeGetCredentialsFromOptions(m.config, creds, method) - if err != nil { - return nil, err - } - if ok { - return creds, nil - } - // No options passed, so let's get the credentials that inputbuilder should have cached - // for us after hitting the OONI API. - if err := m.getCredentialsFromAPI(ctx, sess, provider, creds); err != nil { - return nil, err - } - return creds, nil - - default: - return nil, fmt.Errorf("%w: method not implemented (%s)", ErrInvalidInput, method) - } - -} - -// mergeOpenVPNConfig attempts to get credentials from Options or API, and then -// constructs a [*vpnconfig.Config] instance after merging the credentials passed by options or API response. -// It also returns an error if the operation fails. -func (m *Measurer) mergeOpenVPNConfig( - ctx context.Context, - sess model.ExperimentSession, - endpoint *endpoint, - tracer *vpntracex.Tracer) (*vpnconfig.Config, error) { - - logger := sess.Logger() - - credentials, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, endpoint.Provider) - if err != nil { - return nil, err - } - - openvpnConfig, err := getOpenVPNConfig(tracer, logger, endpoint, credentials) - if err != nil { - return nil, err - } - // TODO(ainghazal): sanity check (Remote, Port, Proto etc + missing certs) - return openvpnConfig, nil -} - // connectAndHandshake dials a connection and attempts an OpenVPN handshake using that dialer. func (m *Measurer) connectAndHandshake( ctx context.Context, @@ -286,17 +147,7 @@ func (m *Measurer) connectAndHandshake( handshakeEvents := handshakeTracer.Trace() port, _ := strconv.Atoi(endpoint.Port) - var ( - tFirst float64 - tLast float64 - bootstrapTime float64 - ) - - if len(handshakeEvents) > 0 { - tFirst = handshakeEvents[0].AtTime - tLast = handshakeEvents[len(handshakeEvents)-1].AtTime - bootstrapTime = tLast - tFirst - } + t0, t, bootstrapTime := TimestampsFromHandshake(handshakeEvents) return &SingleConnection{ TCPConnect: trace.FirstTCPConnectOrNil(), @@ -313,8 +164,8 @@ func (m *Measurer) connectAndHandshake( Auth: openvpnConfig.OpenVPNOptions().Auth, Compression: string(openvpnConfig.OpenVPNOptions().Compress), }, - T0: tFirst, - T: tLast, + T0: t0, + T: t, Tags: []string{}, TransactionID: index, }, @@ -322,6 +173,22 @@ func (m *Measurer) connectAndHandshake( } } +// TimestampsFromHandshake returns the t0, t and duration of the handshake events. +// If the passed events are a zero-len array, all of the results will be zero. +func TimestampsFromHandshake(events []*vpntracex.Event) (float64, float64, float64) { + var ( + t0 float64 + t float64 + duration float64 + ) + if len(events) > 0 { + t0 = events[0].AtTime + t = events[len(events)-1].AtTime + duration = t - t0 + } + return t0, t, duration +} + // FetchProviderCredentials will extract credentials from the configuration we gathered for a given provider. func (m *Measurer) FetchProviderCredentials( ctx context.Context, @@ -339,14 +206,14 @@ func (m *Measurer) FetchProviderCredentials( // Run implements model.ExperimentMeasurer.Run. // A single run expects exactly ONE input (endpoint), but we can modify whether // to test different transports by settings options. -func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { +func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { callbacks := args.Callbacks measurement := args.Measurement sess := args.Session - endpoint, err := parseEndpoint(measurement) - if err != nil { - return err + // 0. fail if there's no richer input target + if args.Target == nil { + return targetloading.ErrInputRequired } tk := NewTestKeys() @@ -355,22 +222,39 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { idx := int64(1) handshakeTracer := vpntracex.NewTracerWithTransactionID(zeroTime, idx) - openvpnConfig, err := m.mergeOpenVPNConfig(ctx, sess, endpoint, handshakeTracer) + // 1. build the input + target, ok := args.Target.(*Target) + if !ok { + return targetloading.ErrInvalidInputType + } + config, input := target.Options, target.URL + + // 2. obtain the endpoint representation from the input URL + endpoint, err := newEndpointFromInputString(input) + if err != nil { + return err + } + + // TODO(ainghazal): validate we have valid config for each endpoint. + // TODO(ainghazal): validate hostname is a valid IP (ipv4 or 6) + // TODO(ainghazal): decide what to do if we have expired certs (abort one measurement or abort the whole experiment?) + + // 3. build openvpn config from endpoint and options + openvpnConfig, err := newOpenVPNConfig(handshakeTracer, sess.Logger(), endpoint, config) if err != nil { return err } sess.Logger().Infof("Probing endpoint %s", endpoint.String()) + // 4. initiate openvpn handshake against endpoint connResult := m.connectAndHandshake(ctx, zeroTime, idx, sess.Logger(), endpoint, openvpnConfig, handshakeTracer) tk.AddConnectionTestKeys(connResult) tk.Success = tk.AllConnectionsSuccessful() callbacks.OnProgress(1.0, "All endpoints probed") - measurement.TestKeys = tk - // TODO(ainghazal): validate we have valid config for each endpoint. - // TODO(ainghazal): validate hostname is a valid IP (ipv4 or 6) - // TODO(ainghazal): decide what to do if we have expired certs (abort one measurement or abort the whole experiment?) + // 5. assign the testkeys + measurement.TestKeys = tk // Note: if here we return an error, the parent code will assume // something fundamental was wrong and we don't have a measurement diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index ed0f2b9adb..e45b52ae75 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -6,12 +6,13 @@ import ( "testing" "time" + "github.com/apex/log" "github.com/google/go-cmp/cmp" - vpnconfig "github.com/ooni/minivpn/pkg/config" vpntracex "github.com/ooni/minivpn/pkg/tracex" "github.com/ooni/probe-cli/v3/internal/experiment/openvpn" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) func makeMockSession() *mocks.Session { @@ -35,11 +36,11 @@ func makeMockSession() *mocks.Session { } func TestNewExperimentMeasurer(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn") + m := openvpn.NewExperimentMeasurer() if m.ExperimentName() != "openvpn" { t.Fatal("invalid ExperimentName") } - if m.ExperimentVersion() != "0.1.2" { + if m.ExperimentVersion() != "0.1.3" { t.Fatal("invalid ExperimentVersion") } } @@ -60,194 +61,6 @@ func TestNewTestKeys(t *testing.T) { } } -func TestMaybeGetCredentialsFromOptions(t *testing.T) { - t.Run("cert auth returns false if cert, key and ca are not all provided", func(t *testing.T) { - cfg := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9v", - } - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, &vpnconfig.OpenVPNOptions{}, openvpn.AuthCertificate) - if err != nil { - t.Fatal("should not raise error") - } - if ok { - t.Fatal("expected false") - } - }) - t.Run("cert auth returns ok if cert, key and ca are all provided", func(t *testing.T) { - cfg := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9v", - SafeKey: "base64:Zm9v", - } - opts := &vpnconfig.OpenVPNOptions{} - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, opts, openvpn.AuthCertificate) - if err != nil { - t.Fatalf("expected err=nil, got %v", err) - } - if !ok { - t.Fatal("expected true") - } - if diff := cmp.Diff(opts.CA, []byte("foo")); diff != "" { - t.Fatal(diff) - } - if diff := cmp.Diff(opts.Cert, []byte("foo")); diff != "" { - t.Fatal(diff) - } - if diff := cmp.Diff(opts.Key, []byte("foo")); diff != "" { - t.Fatal(diff) - } - }) - t.Run("cert auth returns false and error if CA base64 is bad blob", func(t *testing.T) { - cfg := openvpn.Config{ - SafeCA: "base64:Zm9vaaa", - SafeCert: "base64:Zm9v", - SafeKey: "base64:Zm9v", - } - opts := &vpnconfig.OpenVPNOptions{} - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, opts, openvpn.AuthCertificate) - if ok { - t.Fatal("expected false") - } - if !errors.Is(err, openvpn.ErrBadBase64Blob) { - t.Fatalf("expected err=ErrBase64Blob, got %v", err) - } - }) - t.Run("cert auth returns false and error if key base64 is bad blob", func(t *testing.T) { - cfg := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9v", - SafeKey: "base64:Zm9vaaa", - } - opts := &vpnconfig.OpenVPNOptions{} - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, opts, openvpn.AuthCertificate) - if ok { - t.Fatal("expected false") - } - if !errors.Is(err, openvpn.ErrBadBase64Blob) { - t.Fatalf("expected err=ErrBase64Blob, got %v", err) - } - }) - t.Run("cert auth returns false and error if cert base64 is bad blob", func(t *testing.T) { - cfg := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9vaaa", - SafeKey: "base64:Zm9v", - } - opts := &vpnconfig.OpenVPNOptions{} - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, opts, openvpn.AuthCertificate) - if ok { - t.Fatal("expected false") - } - if !errors.Is(err, openvpn.ErrBadBase64Blob) { - t.Fatalf("expected err=ErrBase64Blob, got %v", err) - } - }) - t.Run("userpass auth returns error, not yet implemented", func(t *testing.T) { - cfg := openvpn.Config{} - ok, err := openvpn.MaybeGetCredentialsFromOptions(cfg, &vpnconfig.OpenVPNOptions{}, openvpn.AuthUserPass) - if ok { - t.Fatal("expected false") - } - if err != nil { - t.Fatalf("expected err=nil, got %v", err) - } - }) -} - -func TestGetCredentialsFromOptionsOrAPI(t *testing.T) { - t.Run("non-registered provider raises error", func(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn").(openvpn.Measurer) - ctx := context.Background() - sess := makeMockSession() - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "nsa") - if !errors.Is(err, openvpn.ErrInvalidInput) { - t.Fatalf("expected err=ErrInvalidInput, got %v", err) - } - if opts != nil { - t.Fatal("expected opts=nil") - } - }) - t.Run("providers with userpass auth method raise error, not yet implemented", func(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn").(openvpn.Measurer) - ctx := context.Background() - sess := makeMockSession() - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "tunnelbear") - if !errors.Is(err, openvpn.ErrInvalidInput) { - t.Fatalf("expected err=ErrInvalidInput, got %v", err) - } - if opts != nil { - t.Fatal("expected opts=nil") - } - }) - t.Run("known cert auth provider and creds in options is ok", func(t *testing.T) { - config := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9v", - SafeKey: "base64:Zm9v", - } - m := openvpn.NewExperimentMeasurer(config, "openvpn").(openvpn.Measurer) - ctx := context.Background() - sess := makeMockSession() - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "riseup") - if err != nil { - t.Fatalf("expected err=nil, got %v", err) - } - if opts == nil { - t.Fatal("expected non-nil options") - } - }) - t.Run("known cert auth provider and bad creds in options returns error", func(t *testing.T) { - config := openvpn.Config{ - SafeCA: "base64:Zm9v", - SafeCert: "base64:Zm9v", - SafeKey: "base64:Zm9vaaa", - } - m := openvpn.NewExperimentMeasurer(config, "openvpn").(openvpn.Measurer) - ctx := context.Background() - sess := makeMockSession() - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "riseup") - if !errors.Is(err, openvpn.ErrBadBase64Blob) { - t.Fatalf("expected err=ErrBadBase64, got %v", err) - } - if opts != nil { - t.Fatal("expected nil opts") - } - }) - t.Run("known cert auth provider with null options hits the api", func(t *testing.T) { - config := openvpn.Config{} - m := openvpn.NewExperimentMeasurer(config, "openvpn").(openvpn.Measurer) - ctx := context.Background() - sess := makeMockSession() - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "riseup") - if err != nil { - t.Fatalf("expected err=nil, got %v", err) - } - if opts == nil { - t.Fatalf("expected not-nil options, got %v", opts) - } - }) - t.Run("known cert auth provider with null options hits the api and raises error if api fails", func(t *testing.T) { - config := openvpn.Config{} - m := openvpn.NewExperimentMeasurer(config, "openvpn").(openvpn.Measurer) - ctx := context.Background() - - someError := errors.New("some error") - sess := makeMockSession() - sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) { - return nil, someError - } - - opts, err := m.GetCredentialsFromOptionsOrAPI(ctx, sess, "riseup") - if !errors.Is(err, someError) { - t.Fatalf("expected err=someError, got %v", err) - } - if opts != nil { - t.Fatalf("expected nil options, got %v", opts) - } - }) -} - func TestAddConnectionTestKeys(t *testing.T) { t.Run("append tcp connection result to empty keys", func(t *testing.T) { tk := openvpn.NewTestKeys() @@ -364,20 +177,37 @@ func TestAllConnectionsSuccessful(t *testing.T) { }) } -func TestBadInputFailure(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn") +func TestOpenVPNFailsWithInvalidInputType(t *testing.T) { + measurer := openvpn.NewExperimentMeasurer() + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: new(model.Measurement), + Session: makeMockSession(), + Target: &model.OOAPIURLInfo{}, // not the input type we expect + } + err := measurer.Run(context.Background(), args) + if !errors.Is(err, openvpn.ErrInvalidInputType) { + t.Fatal("expected input error") + } +} + +func TestBadTargetURLFailure(t *testing.T) { + m := openvpn.NewExperimentMeasurer() ctx := context.Background() sess := makeMockSession() callbacks := model.NewPrinterCallbacks(sess.Logger()) measurement := new(model.Measurement) - measurement.Input = "openvpn://badprovider/?address=aa" args := &model.ExperimentArgs{ Callbacks: callbacks, Measurement: measurement, Session: sess, + Target: &openvpn.Target{ + URL: "openvpn://badprovider/?address=aa", + Options: &openvpn.Config{}, + }, } err := m.Run(ctx, args) - if !errors.Is(err, openvpn.ErrInvalidInput) { + if !errors.Is(err, targetloading.ErrInvalidInput) { t.Fatalf("expected ErrInvalidInput, got %v", err) } } @@ -391,9 +221,7 @@ func TestVPNInput(t *testing.T) { func TestMeasurer_FetchProviderCredentials(t *testing.T) { t.Run("Measurer.FetchProviderCredentials calls method in session", func(t *testing.T) { - m := openvpn.NewExperimentMeasurer( - openvpn.Config{}, - "openvpn").(openvpn.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() _, err := m.FetchProviderCredentials( @@ -406,9 +234,7 @@ func TestMeasurer_FetchProviderCredentials(t *testing.T) { t.Run("Measurer.FetchProviderCredentials raises error if API calls fail", func(t *testing.T) { someError := errors.New("unexpected") - m := openvpn.NewExperimentMeasurer( - openvpn.Config{}, - "openvpn").(openvpn.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) { @@ -424,24 +250,66 @@ func TestMeasurer_FetchProviderCredentials(t *testing.T) { } func TestSuccess(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{ - Provider: "riseup", - SafeCA: "base64:Zm9v", - SafeKey: "base64:Zm9v", - SafeCert: "base64:Zm9v", - }, "openvpn") + m := openvpn.NewExperimentMeasurer() ctx := context.Background() sess := makeMockSession() callbacks := model.NewPrinterCallbacks(sess.Logger()) measurement := new(model.Measurement) - measurement.Input = "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp" args := &model.ExperimentArgs{ Callbacks: callbacks, Measurement: measurement, Session: sess, + Target: &openvpn.Target{ + URL: "openvpn://riseupvpn.corp/?address=127.0.0.1:9989&transport=tcp", + Options: &openvpn.Config{}, + }, } err := m.Run(ctx, args) if err != nil { t.Fatal(err) } } + +func TestTimestampsFromHandshake(t *testing.T) { + t.Run("with more than a single event (common case)", func(t *testing.T) { + events := []*vpntracex.Event{{AtTime: 0}, {AtTime: 1}, {AtTime: 2}} + t0, tlast, duration := openvpn.TimestampsFromHandshake(events) + if t0 != 0 { + t.Fatal("expected t0 == 0") + } + if tlast != 2.0 { + t.Fatal("expected t == 2") + } + if duration != 2 { + t.Fatal("expected duration == 2") + } + }) + + t.Run("with a single event", func(t *testing.T) { + events := []*vpntracex.Event{{AtTime: 1}} + t0, tlast, duration := openvpn.TimestampsFromHandshake(events) + if t0 != 1.0 { + t.Fatal("expected t0 == 1.0") + } + if tlast != 1.0 { + t.Fatal("expected t == 1.0") + } + if duration != 0 { + t.Fatal("expected duration == 0") + } + }) + + t.Run("with no events", func(t *testing.T) { + events := []*vpntracex.Event{} + t0, tlast, duration := openvpn.TimestampsFromHandshake(events) + if t0 != 0 { + t.Fatal("expected t0 == 0") + } + if tlast != 0 { + t.Fatal("expected t == 0") + } + if duration != 0 { + t.Fatal("expected duration == 0") + } + }) +} diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go new file mode 100644 index 0000000000..050673b359 --- /dev/null +++ b/internal/experiment/openvpn/richerinput.go @@ -0,0 +1,149 @@ +package openvpn + +import ( + "context" + "fmt" + + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/reflectx" + "github.com/ooni/probe-cli/v3/internal/targetloading" +) + +// defaultProvider is the provider we will request from API in case we got no provider set +// in the CLI options. +var defaultProvider = "riseupvpn" + +// providerAuthentication is a map so that we know which kind of credentials we +// need to fill in the openvpn options for each known provider. +var providerAuthentication = map[string]AuthMethod{ + "riseupvpn": AuthCertificate, + "tunnelbearvpn": AuthUserPass, + "surfsharkvpn": AuthUserPass, +} + +// Target is a richer-input target that this experiment should measure. +type Target struct { + // Options contains the configuration. + Options *Config + + // URL is the input URL. + URL string +} + +var _ model.ExperimentTarget = &Target{} + +// Category implements [model.ExperimentTarget]. +func (t *Target) Category() string { + return model.DefaultCategoryCode +} + +// Country implements [model.ExperimentTarget]. +func (t *Target) Country() string { + return model.DefaultCountryCode +} + +// Input implements [model.ExperimentTarget]. +func (t *Target) Input() string { + return t.URL +} + +// String implements [model.ExperimentTarget]. +func (t *Target) String() string { + return t.URL +} + +// NewLoader constructs a new [model.ExperimentTargerLoader] instance. +// +// This function PANICS if options is not an instance of [*openvpn.Config]. +func NewLoader(loader *targetloading.Loader, gopts any) model.ExperimentTargetLoader { + // Panic if we cannot convert the options to the expected type. + // + // We do not expect a panic here because the type is managed by the registry package. + options := gopts.(*Config) + + // Construct the proper loader instance. + return &targetLoader{ + loader: loader, + options: options, + session: loader.Session, + } +} + +// targetLoader loads targets for this experiment. +type targetLoader struct { + loader *targetloading.Loader + options *Config + session targetloading.Session +} + +// Load implements model.ExperimentTargetLoader. +func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { + // If inputs and files are all empty and there are no options, let's use the backend + if len(tl.loader.StaticInputs) <= 0 && len(tl.loader.SourceFiles) <= 0 && + reflectx.StructOrStructPtrIsZero(tl.options) { + return tl.loadFromBackend(ctx) + } + + // Otherwise, attempt to load the static inputs from CLI and files + inputs, err := targetloading.LoadStatic(tl.loader) + + // Handle the case where we couldn't load from CLI or files + if err != nil { + return nil, err + } + + // Build the list of targets that we should measure. + var targets []model.ExperimentTarget + for _, input := range inputs { + targets = append(targets, &Target{ + Options: tl.options, + URL: input, + }) + } + return targets, nil +} + +// TODO(https://github.com/ooni/probe/issues/2755): make the code that fetches experiment private +// and let the common code export just the bare minimum to make this possible. +func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.ExperimentTarget, error) { + if tl.options.Provider == "" { + tl.options.Provider = defaultProvider + } + + targets := make([]model.ExperimentTarget, 0) + provider := tl.options.Provider + + apiConfig, err := tl.session.FetchOpenVPNConfig(ctx, provider, tl.session.ProbeCC()) + if err != nil { + tl.session.Logger().Warnf("Cannot fetch openvpn config: %v", err) + return nil, err + } + + auth, ok := providerAuthentication[provider] + if !ok { + return nil, fmt.Errorf("%w: unknown authentication for provider %s", targetloading.ErrInvalidInput, provider) + } + + for _, input := range apiConfig.Inputs { + config := &Config{ + // TODO(ainghazal): Auth and Cipher are hardcoded for now. + // Backend should provide them as richer input; and if empty we can use these as defaults. + Auth: "SHA512", + Cipher: "AES-256-GCM", + } + switch auth { + case AuthCertificate: + config.SafeCA = apiConfig.Config.CA + config.SafeCert = apiConfig.Config.Cert + config.SafeKey = apiConfig.Config.Key + case AuthUserPass: + // TODO(ainghazal): implement (surfshark, etc) + } + targets = append(targets, &Target{ + URL: input, + Options: config, + }) + } + + return targets, nil +} diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go new file mode 100644 index 0000000000..4c04696635 --- /dev/null +++ b/internal/experiment/openvpn/richerinput_test.go @@ -0,0 +1,218 @@ +package openvpn + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/mocks" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" +) + +func TestTarget(t *testing.T) { + target := &Target{ + URL: "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp", + Options: &Config{ + Auth: "SHA512", + Cipher: "AES-256-GCM", + Provider: "unknown", + SafeKey: "aa", + SafeCert: "bb", + SafeCA: "cc", + }, + } + + t.Run("Category", func(t *testing.T) { + if target.Category() != model.DefaultCategoryCode { + t.Fatal("invalid Category") + } + }) + + t.Run("Country", func(t *testing.T) { + if target.Country() != model.DefaultCountryCode { + t.Fatal("invalid Country") + } + }) + + t.Run("Input", func(t *testing.T) { + if target.Input() != "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp" { + t.Fatal("invalid Input") + } + }) + + t.Run("String", func(t *testing.T) { + if target.String() != "openvpn://unknown.corp?address=1.1.1.1%3A443&transport=udp" { + t.Fatal("invalid String") + } + }) +} + +func TestNewLoader(t *testing.T) { + // create the pointers we expect to see + child := &targetloading.Loader{} + options := &Config{} + + // create the loader and cast it to its private type + loader := NewLoader(child, options).(*targetLoader) + + // make sure the loader is okay + if child != loader.loader { + t.Fatal("invalid loader pointer") + } + + // make sure the options are okay + if options != loader.options { + t.Fatal("invalid options pointer") + } +} + +func TestTargetLoaderLoad(t *testing.T) { + // testcase is a test case implemented by this function + type testcase struct { + // name is the test case name + name string + + // options contains the options to use + options *Config + + // loader is the loader to use + loader *targetloading.Loader + + // expectErr is the error we expect + expectErr error + + // expectResults contains the expected results + expectTargets []model.ExperimentTarget + } + + cases := []testcase{ + + { + name: "with options and inputs", + options: &Config{ + SafeCA: "aa", + SafeCert: "bb", + SafeKey: "cc", + Provider: "unknown", + }, + loader: &targetloading.Loader{ + ExperimentName: "openvpn", + InputPolicy: model.InputOrQueryBackend, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{ + "openvpn://unknown.corp/1.1.1.1", + }, + }, + expectErr: nil, + expectTargets: []model.ExperimentTarget{ + &Target{ + URL: "openvpn://unknown.corp/1.1.1.1", + Options: &Config{ + Provider: "unknown", + SafeCA: "aa", + SafeCert: "bb", + SafeKey: "cc", + }, + }, + }, + }, + { + name: "with just options", + options: &Config{ + Provider: "riseupvpn", + }, + loader: &targetloading.Loader{ + ExperimentName: "openvpn", + InputPolicy: model.InputOrQueryBackend, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + StaticInputs: []string{}, + SourceFiles: []string{}, + }, + expectErr: nil, + expectTargets: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // create a target loader using the given config + tl := &targetLoader{ + loader: tc.loader, + options: tc.options, + } + + // load targets + targets, err := tl.Load(context.Background()) + + // make sure error is consistent + switch { + case err == nil && tc.expectErr == nil: + // fallthrough + + case err != nil && tc.expectErr != nil: + if !errors.Is(err, tc.expectErr) { + t.Fatal("unexpected error", err) + } + // fallthrough + + default: + t.Fatal("expected", tc.expectErr, "got", err) + } + + // make sure the targets are consistent + if diff := cmp.Diff(tc.expectTargets, targets); diff != "" { + t.Fatal(diff) + } + }) + } +} + +func TestTargetLoaderLoadFromBackend(t *testing.T) { + loader := &targetloading.Loader{ + ExperimentName: "openvpn", + InputPolicy: model.InputOrQueryBackend, + Logger: model.DiscardLogger, + Session: &mocks.Session{}, + } + sess := &mocks.Session{} + sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) { + return &model.OOAPIVPNProviderConfig{ + Provider: "riseupvpn", + Config: &model.OOAPIVPNConfig{}, + Inputs: []string{ + "openvpn://target0", + "openvpn://target1", + }, + DateUpdated: time.Now(), + }, nil + } + sess.MockProbeCC = func() string { + return "IT" + } + tl := &targetLoader{ + loader: loader, + options: &Config{}, + session: sess, + } + targets, err := tl.Load(context.Background()) + if err != nil { + t.Fatal("expected no error") + } + fmt.Println("targets", targets) + if len(targets) != 2 { + t.Fatal("expected 2 targets") + } + if targets[0].String() != "openvpn://target0" { + t.Fatal("expected openvpn://target0") + } + if targets[1].String() != "openvpn://target1" { + t.Fatal("expected openvpn://target1") + } + +} diff --git a/internal/model/experiment.go b/internal/model/experiment.go index c15bc3ee2d..5aada21ff4 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -278,6 +278,9 @@ type ExperimentTargetLoaderSession interface { // Logger returns the logger to use. Logger() Logger + + // ProbeCC returns the probe country code. + ProbeCC() string } // ExperimentOptionInfo contains info about an experiment option. diff --git a/internal/registry/openvpn.go b/internal/registry/openvpn.go index 8bf107630c..ea6c6be765 100644 --- a/internal/registry/openvpn.go +++ b/internal/registry/openvpn.go @@ -14,15 +14,14 @@ func init() { AllExperiments[canonicalName] = func() *Factory { return &Factory{ build: func(config interface{}) model.ExperimentMeasurer { - return openvpn.NewExperimentMeasurer( - *config.(*openvpn.Config), "openvpn", - ) + return openvpn.NewExperimentMeasurer() }, canonicalName: canonicalName, config: &openvpn.Config{}, enabledByDefault: true, interruptible: true, inputPolicy: model.InputOrQueryBackend, + newLoader: openvpn.NewLoader, } } } diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 54540f54a7..bdddb675f7 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -10,14 +10,13 @@ import ( "net/url" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/experiment/openvpn" "github.com/ooni/probe-cli/v3/internal/experimentname" "github.com/ooni/probe-cli/v3/internal/fsx" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/stuninput" ) -// These errors are returned by the [*Loader]. +// These errors are returned by the [*Loader] or the experiment execution. var ( ErrNoURLsReturned = errors.New("no URLs returned") ErrDetectedEmptyFile = errors.New("file did not contain any input") @@ -25,6 +24,7 @@ var ( ErrNoInputExpected = errors.New("we did not expect any input") ErrNoStaticInput = errors.New("no static input for this experiment") ErrInvalidInputType = errors.New("invalid richer input type") + ErrInvalidInput = errors.New("input does not conform to spec") ) // Session is the session according to a [*Loader] instance. @@ -162,7 +162,8 @@ func (il *Loader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTar if err != nil || len(inputs) > 0 { return inputs, err } - return il.loadRemote(ctx) + // This assumes that the default experiment for loading remote targets is WebConnectivity. + return il.loadRemoteWebConnectivity(ctx) } // TODO(https://github.com/ooni/probe/issues/1390): we need to @@ -308,21 +309,6 @@ func LoadStatic(config *Loader) ([]string, error) { return inputs, nil } -// loadRemote loads inputs from a remote source. -func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { - switch experimentname.Canonicalize(il.ExperimentName) { - case "openvpn": - // TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass - // the desired provider here, if known. We only have one provider for now, so I'm acting like YAGNI. Another - // option is perhaps to coalesce all the known providers per proto into a single API call and let client - // pick whatever they want. - // This will likely improve after Richer Input is available. - return il.loadRemoteOpenVPN(ctx) - default: - return il.loadRemoteWebConnectivity(ctx) - } -} - // loadRemoteWebConnectivity loads webconnectivity inputs from a remote source. func (il *Loader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) { config := il.CheckInConfig @@ -360,41 +346,6 @@ func modelOOAPIURLInfoToModelExperimentTarget( return } -// loadRemoteOpenVPN loads openvpn inputs from a remote source. -func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) { - // VPN Inputs do not match exactly the semantics expected from [model.OOAPIURLInfo], - // since OOAPIURLInfo is oriented towards webconnectivity, - // but we force VPN targets in the URL and ignore all the other fields. - // There's still some level of impedance mismatch here, since it's also possible that - // the user of the library wants to use remotes by unknown providers passed via cli options, - // oonirun etc; in that case we'll need to extract the provider annotation from the URLs. - urls := make([]model.OOAPIURLInfo, 0) - - // The openvpn experiment contains an array of the providers that the API knows about. - // We try to get all the remotes from the API for the list of enabled providers. - for _, provider := range openvpn.APIEnabledProviders { - // fetchOpenVPNConfig ultimately uses an internal cache in the session to avoid - // hitting the API too many times. - reply, err := il.fetchOpenVPNConfig(ctx, provider) - if err != nil { - output := modelOOAPIURLInfoToModelExperimentTarget(urls) - return output, err - } - for _, input := range reply.Inputs { - urls = append(urls, model.OOAPIURLInfo{URL: input}) - } - } - - if len(urls) <= 0 { - // At some point we might want to return [openvpn.DefaultEndpoints], - // but for now we're assuming that no targets means we've disabled - // the experiment on the backend. - return nil, ErrNoURLsReturned - } - output := modelOOAPIURLInfoToModelExperimentTarget(urls) - return output, nil -} - // checkIn executes the check-in and filters the returned URLs to exclude // the URLs that are not part of the requested categories. This is done for // robustness, just in case we or the API do something wrong. diff --git a/internal/targetloading/targetloading_test.go b/internal/targetloading/targetloading_test.go index 0147ddb5ba..9cc9c0350e 100644 --- a/internal/targetloading/targetloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -9,7 +9,6 @@ import ( "strings" "syscall" "testing" - "time" "github.com/apex/log" "github.com/google/go-cmp/cmp" @@ -530,6 +529,9 @@ type TargetLoaderMockableSession struct { // It should be nil when Error is non-nil. FetchOpenVPNConfigOutput *model.OOAPIVPNProviderConfig + // ProbeCountryCode is the probe country code + ProbeCountryCode string + // Error is the error to be returned by CheckIn. It // should be nil when Output is not-nil. Error error @@ -551,6 +553,10 @@ func (sess *TargetLoaderMockableSession) FetchOpenVPNConfig( return sess.FetchOpenVPNConfigOutput, sess.Error } +func (sess *TargetLoaderMockableSession) ProbeCC() string { + return sess.ProbeCountryCode +} + // Logger implements [Session]. func (sess *TargetLoaderMockableSession) Logger() model.Logger { // Such that we see some logs when running tests @@ -563,7 +569,7 @@ func TestTargetLoaderCheckInFailure(t *testing.T) { Error: io.EOF, }, } - out, err := il.loadRemote(context.Background()) + out, err := il.loadRemoteWebConnectivity(context.Background()) if !errors.Is(err, io.EOF) { t.Fatalf("not the error we expected: %+v", err) } @@ -580,7 +586,7 @@ func TestTargetLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { }, }, } - out, err := il.loadRemote(context.Background()) + out, err := il.loadRemoteWebConnectivity(context.Background()) if !errors.Is(err, ErrNoURLsReturned) { t.Fatalf("not the error we expected: %+v", err) } @@ -599,7 +605,7 @@ func TestTargetLoaderCheckInSuccessWithNoURLs(t *testing.T) { }, }, } - out, err := il.loadRemote(context.Background()) + out, err := il.loadRemoteWebConnectivity(context.Background()) if !errors.Is(err, ErrNoURLsReturned) { t.Fatalf("not the error we expected: %+v", err) } @@ -632,7 +638,7 @@ func TestTargetLoaderCheckInSuccessWithSomeURLs(t *testing.T) { }, }, } - out, err := il.loadRemote(context.Background()) + out, err := il.loadRemoteWebConnectivity(context.Background()) if err != nil { t.Fatal(err) } @@ -641,91 +647,6 @@ func TestTargetLoaderCheckInSuccessWithSomeURLs(t *testing.T) { } } -func TestTargetLoaderOpenVPNSuccessWithNoInput(t *testing.T) { - il := &Loader{ - ExperimentName: "openvpn", - InputPolicy: model.InputOrQueryBackend, - Session: &TargetLoaderMockableSession{ - Error: nil, - FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ - Provider: "riseup", - Inputs: []string{ - "openvpn://foo.corp/?address=1.1.1.1:1194&transport=tcp", - }, - DateUpdated: time.Now(), - }, - }, - } - _, err := il.loadRemote(context.Background()) - if err != nil { - t.Fatal("we did not expect an error") - } -} - -func TestTargetLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { - il := &Loader{ - ExperimentName: "openvpn", - InputPolicy: model.InputOrQueryBackend, - Session: &TargetLoaderMockableSession{ - Error: nil, - FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ - Provider: "riseupvpn", - Inputs: []string{ - "openvpn://foo.corp/?address=1.2.3.4:1194&transport=tcp", - }, - DateUpdated: time.Now(), - }, - }, - } - out, err := il.loadRemote(context.Background()) - if err != nil { - t.Fatal("we did not expect an error") - } - if len(out) != 1 { - t.Fatal("we expected output of len=1") - } -} - -func TestTargetLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { - expected := errors.New("mocked API error") - il := &Loader{ - ExperimentName: "openvpn", - InputPolicy: model.InputOrQueryBackend, - Session: &TargetLoaderMockableSession{ - Error: expected, - }, - } - out, err := il.loadRemote(context.Background()) - if err != expected { - t.Fatal("we expected an error") - } - if len(out) != 0 { - t.Fatal("we expected no fallback URLs") - } -} - -func TestTargetLoaderOpenVPNWithNoReturnedURLs(t *testing.T) { - il := &Loader{ - ExperimentName: "openvpn", - InputPolicy: model.InputOrQueryBackend, - Session: &TargetLoaderMockableSession{ - FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ - Provider: "riseupvpn", - Config: &model.OOAPIVPNConfig{}, - Inputs: []string{}, - DateUpdated: time.Time{}, - }, - }, - } - out, err := il.loadRemote(context.Background()) - if !errors.Is(err, ErrNoURLsReturned) { - t.Fatal("unexpected a error") - } - if len(out) != 0 { - t.Fatal("we expected no fallback URLs") - } -} - func TestPreventMistakesWithCategories(t *testing.T) { input := []model.OOAPIURLInfo{{ CategoryCode: "NEWS", From 6f5ebc3cdec50d3cdbc1652f3ca14a96259df30c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 27 Jun 2024 12:03:40 +0200 Subject: [PATCH 3/3] feat(oonirun): allow true JSON richer input (#1629) Closes https://github.com/ooni/probe/issues/2759 --- internal/engine/experimentbuilder.go | 31 +++- internal/engine/experimentbuilder_test.go | 118 +++++++++++++ internal/mocks/experimentbuilder.go | 12 +- internal/mocks/experimentbuilder_test.go | 14 ++ internal/model/experiment.go | 10 ++ internal/oonirun/experiment.go | 34 +++- internal/oonirun/experiment_test.go | 110 +++++++++++- internal/oonirun/v1_test.go | 4 + internal/oonirun/v2.go | 5 +- internal/oonirun/v2_test.go | 31 ++-- internal/registry/factory.go | 53 +++++- internal/registry/factory_test.go | 196 ++++++++++++++++++++-- internal/registry/portfiltering.go | 4 +- internal/registry/telegram.go | 4 +- internal/registry/webconnectivity.go | 4 +- 15 files changed, 572 insertions(+), 58 deletions(-) diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 330777957b..7c9469a856 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -5,11 +5,19 @@ package engine // import ( + "encoding/json" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/registry" ) -// experimentBuilder implements ExperimentBuilder. +// TODO(bassosimone,DecFox): we should eventually finish merging the code in +// file with the code inside the ./internal/registry package. +// +// If there's time, this could happen at the end of the current (as of 2024-06-27) +// richer input work, otherwise any time in the future is actually fine. + +// experimentBuilder implements [model.ExperimentBuilder]. // // This type is now just a tiny wrapper around registry.Factory. type experimentBuilder struct { @@ -24,37 +32,42 @@ type experimentBuilder struct { var _ model.ExperimentBuilder = &experimentBuilder{} -// Interruptible implements ExperimentBuilder.Interruptible. +// Interruptible implements [model.ExperimentBuilder]. func (b *experimentBuilder) Interruptible() bool { return b.factory.Interruptible() } -// InputPolicy implements ExperimentBuilder.InputPolicy. +// InputPolicy implements [model.ExperimentBuilder]. func (b *experimentBuilder) InputPolicy() model.InputPolicy { return b.factory.InputPolicy() } -// Options implements ExperimentBuilder.Options. +// Options implements [model.ExperimentBuilder]. func (b *experimentBuilder) Options() (map[string]model.ExperimentOptionInfo, error) { return b.factory.Options() } -// SetOptionAny implements ExperimentBuilder.SetOptionAny. +// SetOptionAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionAny(key string, value any) error { return b.factory.SetOptionAny(key, value) } -// SetOptionsAny implements ExperimentBuilder.SetOptionsAny. +// SetOptionsAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionsAny(options map[string]any) error { return b.factory.SetOptionsAny(options) } -// SetCallbacks implements ExperimentBuilder.SetCallbacks. +// SetOptionsJSON implements [model.ExperimentBuilder]. +func (b *experimentBuilder) SetOptionsJSON(value json.RawMessage) error { + return b.factory.SetOptionsJSON(value) +} + +// SetCallbacks implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { b.callbacks = callbacks } -// NewExperiment creates the experiment +// NewExperiment creates a new [model.Experiment] instance. func (b *experimentBuilder) NewExperiment() model.Experiment { measurer := b.factory.NewExperimentMeasurer() experiment := newExperiment(b.session, measurer) @@ -67,7 +80,7 @@ func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoader return b.factory.NewTargetLoader(config) } -// newExperimentBuilder creates a new experimentBuilder instance. +// newExperimentBuilder creates a new [*experimentBuilder] instance. func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) { factory, err := registry.NewFactory(name, session.kvStore, session.logger) if err != nil { diff --git a/internal/engine/experimentbuilder_test.go b/internal/engine/experimentbuilder_test.go index d81e0bb7bb..6d1c5e4f83 100644 --- a/internal/engine/experimentbuilder_test.go +++ b/internal/engine/experimentbuilder_test.go @@ -2,9 +2,11 @@ package engine import ( "context" + "encoding/json" "errors" "testing" + "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -49,3 +51,119 @@ func TestExperimentBuilderEngineWebConnectivity(t *testing.T) { t.Fatal("expected zero length targets") } } + +func TestExperimentBuilderBasicOperations(t *testing.T) { + // create a session for testing that does not use the network at all + sess := newSessionForTestingNoLookups(t) + + // create an experiment builder for example + builder, err := sess.NewExperimentBuilder("example") + if err != nil { + t.Fatal(err) + } + + // example should be interruptible + t.Run("Interruptible", func(t *testing.T) { + if !builder.Interruptible() { + t.Fatal("example should be interruptible") + } + }) + + // we expect to see the InputNone input policy + t.Run("InputPolicy", func(t *testing.T) { + if builder.InputPolicy() != model.InputNone { + t.Fatal("unexpectyed input policy") + } + }) + + // get the options and check whether they are what we expect + t.Run("Options", func(t *testing.T) { + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "Good day from the example experiment!"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set a specific existing option + t.Run("SetOptionAny", func(t *testing.T) { + if err := builder.SetOptionAny("Message", "foobar"); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: false}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set all options at the same time + t.Run("SetOptions", func(t *testing.T) { + inputs := map[string]any{ + "Message": "foobar", + "ReturnError": true, + } + if err := builder.SetOptionsAny(inputs); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // we can set all options using JSON + t.Run("SetOptionsJSON", func(t *testing.T) { + inputs := json.RawMessage(`{ + "Message": "foobar", + "ReturnError": true + }`) + if err := builder.SetOptionsJSON(inputs); err != nil { + t.Fatal(err) + } + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + expectOptions := map[string]model.ExperimentOptionInfo{ + "Message": {Doc: "Message to emit at test completion", Type: "string", Value: "foobar"}, + "ReturnError": {Doc: "Toogle to return a mocked error", Type: "bool", Value: true}, + "SleepTime": {Doc: "Amount of time to sleep for in nanosecond", Type: "int64", Value: int64(1000000000)}, + } + if diff := cmp.Diff(expectOptions, options); diff != "" { + t.Fatal(diff) + } + }) + + // TODO(bassosimone): we could possibly add more checks here. I am not doing this + // right now, because https://github.com/ooni/probe-cli/pull/1629 mostly cares about + // providing input and the rest of the codebase did not change. + // + // Also, it would make sense to eventually merge experimentbuilder.go with the + // ./internal/registry package, which also has coverage. + // + // In conclusion, our main objective for now is to make sure we don't screw the + // pooch when setting options using the experiment builder. +} diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index 1f6a27187f..bcb9652745 100644 --- a/internal/mocks/experimentbuilder.go +++ b/internal/mocks/experimentbuilder.go @@ -1,6 +1,10 @@ package mocks -import "github.com/ooni/probe-cli/v3/internal/model" +import ( + "encoding/json" + + "github.com/ooni/probe-cli/v3/internal/model" +) // ExperimentBuilder mocks model.ExperimentBuilder. type ExperimentBuilder struct { @@ -14,6 +18,8 @@ type ExperimentBuilder struct { MockSetOptionsAny func(options map[string]any) error + MockSetOptionsJSON func(value json.RawMessage) error + MockSetCallbacks func(callbacks model.ExperimentCallbacks) MockNewExperiment func() model.Experiment @@ -43,6 +49,10 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error { return eb.MockSetOptionsAny(options) } +func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error { + return eb.MockSetOptionsJSON(value) +} + func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { eb.MockSetCallbacks(callbacks) } diff --git a/internal/mocks/experimentbuilder_test.go b/internal/mocks/experimentbuilder_test.go index 55ba1783f9..7e90fff66b 100644 --- a/internal/mocks/experimentbuilder_test.go +++ b/internal/mocks/experimentbuilder_test.go @@ -1,6 +1,7 @@ package mocks import ( + "encoding/json" "errors" "testing" @@ -72,6 +73,19 @@ func TestExperimentBuilder(t *testing.T) { } }) + t.Run("SetOptionsJSON", func(t *testing.T) { + expected := errors.New("mocked error") + eb := &ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + return expected + }, + } + err := eb.SetOptionsJSON([]byte(`{}`)) + if !errors.Is(err, expected) { + t.Fatal("unexpected value") + } + }) + t.Run("SetCallbacks", func(t *testing.T) { var called bool eb := &ExperimentBuilder{ diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 5aada21ff4..b514df3dec 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -7,6 +7,7 @@ package model import ( "context" + "encoding/json" "errors" "fmt" ) @@ -235,6 +236,12 @@ type ExperimentBuilder interface { // the SetOptionAny method for more information. SetOptionsAny(options map[string]any) error + // SetOptionsJSON uses the given [json.RawMessage] to initialize fields + // of the configuration for running the experiment. The [json.RawMessage], if + // not empty, MUST contain a serialization of the experiment config's + // type. An empty [json.RawMessage] will silently be ignored. + SetOptionsJSON(value json.RawMessage) error + // SetCallbacks sets the experiment's interactive callbacks. SetCallbacks(callbacks ExperimentCallbacks) @@ -290,6 +297,9 @@ type ExperimentOptionInfo struct { // Type contains the type. Type string + + // Value contains the current option value. + Value any } // ExperimentTargetLoader loads targets from local or remote sources. diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index c4613107d7..1fe1c70795 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -6,6 +6,7 @@ package oonirun import ( "context" + "encoding/json" "fmt" "math/rand" "strings" @@ -25,9 +26,18 @@ type Experiment struct { // Annotations contains OPTIONAL Annotations for the experiment. Annotations map[string]string - // ExtraOptions contains OPTIONAL extra options for the experiment. + // ExtraOptions contains OPTIONAL extra options that modify the + // default experiment-specific configuration. We apply + // the changes described by this field after using the InitialOptions + // field to initialize the experiment-specific configuration. ExtraOptions map[string]any + // InitialOptions contains an OPTIONAL [json.RawMessage] object + // used to initialize the default experiment-specific + // configuration. After we have initialized the configuration + // as such, we then apply the changes described by the ExtraOptions. + InitialOptions json.RawMessage + // Inputs contains the OPTIONAL experiment Inputs Inputs []string @@ -82,16 +92,18 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // TODO(bassosimone,DecFox): when we're executed by OONI Run v2, it probably makes - // slightly more sense to set options from a json.RawMessage because the current - // command line limitation is that it's hard to set non scalar parameters and instead - // with using OONI Run v2 we can completely bypass such a limitation. + // TODO(bassosimone): we need another patch after the current one + // to correctly serialize the options as configured using InitialOptions + // and ExtraOptions otherwise the Measurement.Options field turns out + // to always be empty and this is highly suboptimal for us. + // + // The next patch is https://github.com/ooni/probe-cli/pull/1630. // 2. configure experiment's options // // This MUST happen before loading targets because the options will // possibly be used to produce richer input targets. - if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { + if err := ed.setOptions(builder); err != nil { return err } @@ -142,6 +154,16 @@ func (ed *Experiment) Run(ctx context.Context) error { return inputProcessor.Run(ctx) } +func (ed *Experiment) setOptions(builder model.ExperimentBuilder) error { + // We first unmarshal the InitialOptions into the experiment + // configuration and afterwards we modify the configuration using + // the values contained inside the ExtraOptions field. + if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil { + return err + } + return builder.SetOptionsAny(ed.ExtraOptions) +} + // inputProcessor is an alias for model.ExperimentInputProcessor type inputProcessor = model.ExperimentInputProcessor diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 1fd38abb0b..cbe24d803b 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -2,12 +2,15 @@ package oonirun import ( "context" + "encoding/json" "errors" "reflect" "sort" "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/ooni/probe-cli/v3/internal/engine" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/testingx" @@ -16,6 +19,7 @@ import ( func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { shuffledInputsPrev := experimentShuffledInputs.Load() var calledSetOptionsAny int + var calledSetOptionsJSON int var failedToSubmit int var calledKibiBytesReceived int var calledKibiBytesSent int @@ -40,6 +44,10 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + calledSetOptionsJSON++ + return nil + }, MockSetOptionsAny: func(options map[string]any) error { calledSetOptionsAny++ return nil @@ -109,6 +117,9 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { if calledSetOptionsAny < 1 { t.Fatal("should have called SetOptionsAny") } + if calledSetOptionsJSON < 1 { + t.Fatal("should have called SetOptionsJSON") + } if calledKibiBytesReceived < 1 { t.Fatal("did not call KibiBytesReceived") } @@ -117,6 +128,67 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { } } +// This test ensures that we honour InitialOptions then ExtraOptions. +func TestExperimentSetOptions(t *testing.T) { + + // create the Experiment we're using for this test + exp := &Experiment{ + ExtraOptions: map[string]any{ + "Message": "jarjarbinks", + }, + InitialOptions: []byte(`{"Message": "foobar", "ReturnError": true}`), + Name: "example", + + // TODO(bassosimone): A zero-value session works here. The proper change + // however would be to write a engine.NewExperimentBuilder factory that takes + // as input an interface for the session. This would help testing. + Session: &engine.Session{}, + } + + // create the experiment builder manually + builder, err := exp.newExperimentBuilder(exp.Name) + if err != nil { + t.Fatal(err) + } + + // invoke the method we're testing + if err := exp.setOptions(builder); err != nil { + t.Fatal(err) + } + + // obtain the options + options, err := builder.Options() + if err != nil { + t.Fatal(err) + } + + // describe what we expect to happen + // + // we basically want ExtraOptions to override InitialOptions + expect := map[string]model.ExperimentOptionInfo{ + "Message": { + Doc: "Message to emit at test completion", + Type: "string", + Value: string("jarjarbinks"), // set by ExtraOptions + }, + "ReturnError": { + Doc: "Toogle to return a mocked error", + Type: "bool", + Value: bool(true), // set by InitialOptions + }, + "SleepTime": { + Doc: "Amount of time to sleep for in nanosecond", + Type: "int64", + Value: int64(1000000000), // still the default nonzero value + }, + } + + // make sure the result equals expectation + if diff := cmp.Diff(expect, options); diff != "" { + t.Fatal(diff) + } +} + func Test_experimentOptionsToStringList(t *testing.T) { type args struct { options map[string]any @@ -198,10 +270,34 @@ func TestExperimentRun(t *testing.T) { args: args{}, expectErr: errMocked, }, { - name: "cannot set options", + name: "cannot set InitialOptions", + fields: fields{ + newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { + eb := &mocks.ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + return errMocked + }, + } + return eb, nil + }, + newTargetLoaderFn: func(builder model.ExperimentBuilder) targetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return []model.ExperimentTarget{}, nil + }, + } + }, + }, + args: args{}, + expectErr: errMocked, + }, { + name: "cannot set ExtraOptions", fields: fields{ newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return errMocked }, @@ -226,6 +322,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -255,6 +354,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -298,6 +400,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -344,6 +449,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 910d910b0c..b1109a8efa 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -2,6 +2,7 @@ package oonirun import ( "context" + "encoding/json" "errors" "net/http" "strings" @@ -23,6 +24,9 @@ func newMinimalFakeSession() *mocks.Session { MockInputPolicy: func() model.InputPolicy { return model.InputNone }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 4330b07b76..6552ad582e 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -54,7 +54,7 @@ type V2Nettest struct { // `Safe` will be available for the experiment run, but omitted from // the serialized Measurement that the experiment builder will submit // to the OONI backend. - Options map[string]any `json:"options"` + Options json.RawMessage `json:"options"` // TestName contains the nettest name. TestName string `json:"test_name"` @@ -183,7 +183,8 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri // construct an experiment from the current nettest exp := &Experiment{ Annotations: config.Annotations, - ExtraOptions: nettest.Options, + ExtraOptions: make(map[string]any), + InitialOptions: nettest.Options, Inputs: nettest.Inputs, InputFilePaths: nil, MaxRuntime: config.MaxRuntime, diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index d849c6d088..b77b9b7bf7 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "testing" - "time" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" @@ -27,9 +26,9 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -73,9 +72,9 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -132,9 +131,9 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -220,9 +219,9 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "", // empty! }}, } @@ -374,6 +373,9 @@ func TestV2MeasureDescriptor(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputNone }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -426,7 +428,7 @@ func TestV2MeasureDescriptor(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{}, + Options: json.RawMessage(`{}`), TestName: "example", }}, } @@ -532,5 +534,4 @@ func TestV2DescriptorCacheLoad(t *testing.T) { t.Fatal("expected nil cache") } }) - } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index ee73e95a8f..813e9f7a44 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -5,6 +5,7 @@ package registry // import ( + "encoding/json" "errors" "fmt" "math" @@ -105,22 +106,47 @@ var ( // Options returns the options exposed by this experiment. func (b *Factory) Options() (map[string]model.ExperimentOptionInfo, error) { + // create the result value result := make(map[string]model.ExperimentOptionInfo) + + // make sure we're dealing with a pointer ptrinfo := reflect.ValueOf(b.config) if ptrinfo.Kind() != reflect.Ptr { return nil, ErrConfigIsNotAStructPointer } - structinfo := ptrinfo.Elem().Type() - if structinfo.Kind() != reflect.Struct { + + // obtain information about the value and its type + valueinfo := ptrinfo.Elem() + typeinfo := valueinfo.Type() + + // make sure we're dealing with a struct + if typeinfo.Kind() != reflect.Struct { return nil, ErrConfigIsNotAStructPointer } - for i := 0; i < structinfo.NumField(); i++ { - field := structinfo.Field(i) - result[field.Name] = model.ExperimentOptionInfo{ - Doc: field.Tag.Get("ooni"), - Type: field.Type.String(), + + // cycle through the fields + for i := 0; i < typeinfo.NumField(); i++ { + fieldType, fieldValue := typeinfo.Field(i), valueinfo.Field(i) + + // do not include private fields into our list of fields + if !fieldType.IsExported() { + continue + } + + // skip fields that are missing an `ooni` tag + docs := fieldType.Tag.Get("ooni") + if docs == "" { + continue + } + + // create a description of this field + result[fieldType.Name] = model.ExperimentOptionInfo{ + Doc: docs, + Type: fieldType.Type.String(), + Value: fieldValue.Interface(), } } + return result, nil } @@ -228,6 +254,19 @@ func (b *Factory) SetOptionsAny(options map[string]any) error { return nil } +// SetOptionsJSON unmarshals the given [json.RawMessage] inside +// the experiment specific configuration. +func (b *Factory) SetOptionsJSON(value json.RawMessage) error { + // handle the case where the options are empty + if len(value) <= 0 { + return nil + } + + // otherwise unmarshal into the configuration, which we assume + // to be a pointer to a structure. + return json.Unmarshal(value, b.config) +} + // fieldbyname return v's field whose name is equal to the given key. func (b *Factory) fieldbyname(v interface{}, key string) (reflect.Value, error) { // See https://stackoverflow.com/a/6396678/4354461 diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index c92f031577..d4a3d8368c 100644 --- a/internal/registry/factory_test.go +++ b/internal/registry/factory_test.go @@ -2,10 +2,12 @@ package registry import ( "context" + "encoding/json" "errors" "fmt" "math" "os" + "reflect" "testing" "github.com/apex/log" @@ -19,14 +21,23 @@ import ( "github.com/ooni/probe-cli/v3/internal/targetloading" ) -type fakeExperimentConfig struct { - Chan chan any `ooni:"we cannot set this"` - String string `ooni:"a string"` - Truth bool `ooni:"something that no-one knows"` - Value int64 `ooni:"a number"` -} +func TestFactoryOptions(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + // values that should be included into the Options return value + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + + // values that should not be included because they're private + private int64 `ooni:"a private number"` + + // values that should not be included because they lack "ooni"'s tag + Invisible int64 + } -func TestExperimentBuilderOptions(t *testing.T) { t.Run("when config is not a pointer", func(t *testing.T) { b := &Factory{ config: 17, @@ -55,7 +66,14 @@ func TestExperimentBuilderOptions(t *testing.T) { }) t.Run("when config is a pointer to struct", func(t *testing.T) { - config := &fakeExperimentConfig{} + config := &fakeExperimentConfig{ + Chan: make(chan any), + String: "foobar", + Truth: true, + Value: 177114, + private: 55, + Invisible: 9876, + } b := &Factory{ config: config, } @@ -63,6 +81,7 @@ func TestExperimentBuilderOptions(t *testing.T) { if err != nil { t.Fatal(err) } + for name, value := range options { switch name { case "Chan": @@ -72,6 +91,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "chan interface {}" { t.Fatal("invalid type", value.Type) } + if value.Value.(chan any) == nil { + t.Fatal("expected non-nil channel here") + } + case "String": if value.Doc != "a string" { t.Fatal("invalid doc") @@ -79,6 +102,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "string" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(string); v != "foobar" { + t.Fatal("unexpected string value", v) + } + case "Truth": if value.Doc != "something that no-one knows" { t.Fatal("invalid doc") @@ -86,6 +113,10 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "bool" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(bool); !v { + t.Fatal("unexpected bool value", v) + } + case "Value": if value.Doc != "a number" { t.Fatal("invalid doc") @@ -93,14 +124,27 @@ func TestExperimentBuilderOptions(t *testing.T) { if value.Type != "int64" { t.Fatal("invalid type", value.Type) } + if v := value.Value.(int64); v != 177114 { + t.Fatal("unexpected int64 value", v) + } + default: - t.Fatal("unknown name", name) + t.Fatal("unexpected option name", name) } } }) } -func TestExperimentBuilderSetOptionAny(t *testing.T) { +func TestFactorySetOptionAny(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + } + var inputs = []struct { TestCaseName string InitialConfig any @@ -366,7 +410,17 @@ func TestExperimentBuilderSetOptionAny(t *testing.T) { } } -func TestExperimentBuilderSetOptionsAny(t *testing.T) { +func TestFactorySetOptionsAny(t *testing.T) { + + // the fake configuration we're using in this test + type fakeExperimentConfig struct { + // values that should be included into the Options return value + Chan chan any `ooni:"we cannot set this"` + String string `ooni:"a string"` + Truth bool `ooni:"something that no-one knows"` + Value int64 `ooni:"a number"` + } + b := &Factory{config: &fakeExperimentConfig{}} t.Run("we correctly handle an empty map", func(t *testing.T) { @@ -409,6 +463,107 @@ func TestExperimentBuilderSetOptionsAny(t *testing.T) { }) } +func TestFactorySetOptionsJSON(t *testing.T) { + + // PersonRecord is a fake experiment configuration. + // + // Note how the `ooni` tag here is missing because we don't care + // about whether such a tag is present when using JSON. + type PersonRecord struct { + Name string + Age int64 + Friends []string + } + + // testcase is a test case for this function. + type testcase struct { + // name is the name of the test case + name string + + // mutableConfig is the config in which we should unmarshal the JSON + mutableConfig *PersonRecord + + // rawJSON contains the raw JSON to unmarshal into mutableConfig + rawJSON json.RawMessage + + // expectErr is the error we expect + expectErr error + + // expectRecord is what we expectRecord to see in the end + expectRecord *PersonRecord + } + + cases := []testcase{ + { + name: "we correctly accept zero-length options", + mutableConfig: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + rawJSON: []byte{}, + expectRecord: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + }, + + { + name: "we return an error on JSON parsing error", + mutableConfig: &PersonRecord{}, + rawJSON: []byte(`{`), + expectErr: errors.New("unexpected end of JSON input"), + expectRecord: &PersonRecord{}, + }, + + { + name: "we correctly unmarshal into the existing config", + mutableConfig: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"bar", "baz"}, + }, + rawJSON: []byte(`{"Friends":["foo","oof"]}`), + expectErr: nil, + expectRecord: &PersonRecord{ + Name: "foo", + Age: 55, + Friends: []string{"foo", "oof"}, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + + // create the factory to use + factory := &Factory{config: tc.mutableConfig} + + // unmarshal into the mutableConfig + err := factory.SetOptionsJSON(tc.rawJSON) + + // make sure the error is the one we actually expect + switch { + case err == nil && tc.expectErr == nil: + if diff := cmp.Diff(tc.expectRecord, tc.mutableConfig); diff != "" { + t.Fatal(diff) + } + return + + case err != nil && tc.expectErr != nil: + if err.Error() != tc.expectErr.Error() { + t.Fatal("expected", tc.expectErr, "got", err) + } + return + + default: + t.Fatal("expected", tc.expectErr, "got", err) + } + }) + } +} + func TestNewFactory(t *testing.T) { // experimentSpecificExpectations contains expectations for an experiment type experimentSpecificExpectations struct { @@ -942,3 +1097,22 @@ func TestFactoryNewTargetLoader(t *testing.T) { } }) } + +// This test is important because SetOptionsJSON assumes that the experiment +// config is a struct pointer into which it is possible to write +func TestExperimentConfigIsAlwaysAPointerToStruct(t *testing.T) { + for name, ffunc := range AllExperiments { + t.Run(name, func(t *testing.T) { + factory := ffunc() + config := factory.config + ctype := reflect.TypeOf(config) + if ctype.Kind() != reflect.Pointer { + t.Fatal("expected a pointer") + } + ctype = ctype.Elem() + if ctype.Kind() != reflect.Struct { + t.Fatal("expected a struct") + } + }) + } +} diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 8c6a857df8..2d7c67f250 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return portfiltering.NewExperimentMeasurer( - config.(portfiltering.Config), + *config.(*portfiltering.Config), ) }, canonicalName: canonicalName, - config: portfiltering.Config{}, + config: &portfiltering.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone, diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 987e640935..bf4f71aed1 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return telegram.NewExperimentMeasurer( - config.(telegram.Config), + *config.(*telegram.Config), ) }, canonicalName: canonicalName, - config: telegram.Config{}, + config: &telegram.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone, diff --git a/internal/registry/webconnectivity.go b/internal/registry/webconnectivity.go index 470bad802b..0c2d79367d 100644 --- a/internal/registry/webconnectivity.go +++ b/internal/registry/webconnectivity.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return webconnectivity.NewExperimentMeasurer( - config.(webconnectivity.Config), + *config.(*webconnectivity.Config), ) }, canonicalName: canonicalName, - config: webconnectivity.Config{}, + config: &webconnectivity.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputOrQueryBackend,