diff --git a/internal/engine/session.go b/internal/engine/session.go index fbd17d22b..740ed549e 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 8f706ffa4..bf8f89555 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 6289d75cd..6c5157d52 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 bc3330141..bfd3eb602 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 2d2af0e01..0cf8255f4 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 ed0f2b9ad..e45b52ae7 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 000000000..050673b35 --- /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 000000000..4c0469663 --- /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 c15bc3ee2..5aada21ff 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 8bf107630..ea6c6be76 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 54540f54a..bdddb675f 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 0147ddb5b..9cc9c0350 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",