From 44a9419e859c1ec1600cc127b33316682bb7bc9f Mon Sep 17 00:00:00 2001 From: Ain Ghazal Date: Tue, 11 Jun 2024 19:04:21 +0200 Subject: [PATCH 01/18] feat(openvpn): implement richer input 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 --- internal/engine/session.go | 6 - internal/experiment/openvpn/endpoint.go | 70 ++---- internal/experiment/openvpn/endpoint_test.go | 78 ++---- internal/experiment/openvpn/openvpn.go | 189 ++++----------- internal/experiment/openvpn/openvpn_test.go | 222 ++---------------- internal/experiment/openvpn/richerinput.go | 146 ++++++++++++ .../experiment/openvpn/richerinput_test.go | 175 ++++++++++++++ internal/registry/openvpn.go | 5 +- internal/targetloading/targetloading.go | 6 +- 9 files changed, 415 insertions(+), 482 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 1a6bc5443b..0f41f5f61a 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,9 +379,6 @@ 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 @@ -397,7 +392,6 @@ func (s *Session) FetchOpenVPNConfig( if err != nil { return nil, err } - s.vpnConfig[provider] = config return &config, nil } diff --git a/internal/experiment/openvpn/endpoint.go b/internal/experiment/openvpn/endpoint.go index 6289d75cda..abd6c53459 100644 --- a/internal/experiment/openvpn/endpoint.go +++ b/internal/experiment/openvpn/endpoint.go @@ -1,12 +1,12 @@ package openvpn import ( - "encoding/base64" "errors" "fmt" "math/rand" "net" "net/url" + "slices" "strings" vpnconfig "github.com/ooni/minivpn/pkg/config" @@ -178,16 +178,6 @@ func (e endpointList) Shuffle() endpointList { 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. @@ -196,40 +186,25 @@ var APIEnabledProviders = []string{ "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( +// mergeOpenVPNConfig gets 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 mergeOpenVPNConfig( tracer *vpntracex.Tracer, logger model.Logger, endpoint *endpoint, - creds *vpnconfig.OpenVPNOptions) (*vpnconfig.Config, error) { + config *Config) (*vpnconfig.Config, error) { + // TODO(ainghazal): use merge ability in vpnconfig.OpenVPNOptions merge (pending PR) 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 +214,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), @@ -255,20 +229,6 @@ 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 diff --git a/internal/experiment/openvpn/endpoint_test.go b/internal/experiment/openvpn/endpoint_test.go index bc33301419..57243eeae0 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -8,7 +8,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - vpnconfig "github.com/ooni/minivpn/pkg/config" vpntracex "github.com/ooni/minivpn/pkg/tracex" ) @@ -272,7 +271,7 @@ func Test_isValidProvider(t *testing.T) { } } -func Test_getVPNConfig(t *testing.T) { +func Test_mergeVPNConfig(t *testing.T) { tracer := vpntracex.NewTracer(time.Now()) e := &endpoint{ Provider: "riseupvpn", @@ -280,13 +279,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 := mergeOpenVPNConfig(tracer, nil, e, config) if err != nil { t.Fatalf("did not expect error, got: %v", err) } @@ -311,18 +313,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,62 +332,18 @@ 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 := mergeOpenVPNConfig(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") { diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 2d2af0e01b..db87cd558e 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -4,13 +4,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 +17,19 @@ import ( ) const ( - testVersion = "0.1.2" - openVPNProcol = "openvpn" + testName = "openvpn" + testVersion = "0.1.3" + openVPNProtocol = "openvpn" +) + +// errors are in addition to any other errors returned by the low level packages +// that are used by this experiment to implement its functionality. +var ( + // ErrInputRequired is returned when the experiment is not passed any input. + ErrInputRequired = targetloading.ErrInputRequired + + // ErrInvalidInput is returned if we failed to parse the input to obtain an endpoint we can measure. + ErrInvalidInput = errors.New("invalid input") ) // Config contains the experiment config. @@ -93,18 +103,16 @@ func (tk *TestKeys) AllConnectionsSuccessful() bool { // Measurer performs the measurement. type Measurer struct { - config Config - testName string } // 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 + return testName } // ExperimentVersion implements model.ExperimentMeasurer.ExperimentVersion. @@ -112,19 +120,14 @@ 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)) +func parseEndpoint(input string) (*endpoint, error) { + if input == "" { + return nil, ErrInputRequired + } + if ok := isValidProtocol(input); !ok { + return nil, ErrInvalidInput } - return nil, fmt.Errorf("%w: %s", ErrInvalidInput, "input is mandatory") + return newEndpointFromInputString(input) } // AuthMethod is the authentication method used by a provider. @@ -138,130 +141,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, @@ -344,9 +223,9 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { measurement := args.Measurement sess := args.Session - endpoint, err := parseEndpoint(measurement) - if err != nil { - return err + // 0. obtain the richer input target, config, and input or panic + if args.Target == nil { + return ErrInputRequired } tk := NewTestKeys() @@ -355,10 +234,21 @@ 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) + // build the input + target := args.Target.(*Target) + config, input := target.Options, target.URL + sess.Logger().Infof("openvpn: using richer input: %+v", input) + + endpoint, err := parseEndpoint(input) if err != nil { return err } + + openvpnConfig, err := mergeOpenVPNConfig(handshakeTracer, sess.Logger(), endpoint, config) + if err != nil { + return err + } + sess.Logger().Infof("Probing endpoint %s", endpoint.String()) connResult := m.connectAndHandshake(ctx, zeroTime, idx, sess.Logger(), endpoint, openvpnConfig, handshakeTracer) @@ -366,6 +256,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { tk.Success = tk.AllConnectionsSuccessful() callbacks.OnProgress(1.0, "All endpoints probed") + measurement.TestKeys = tk // TODO(ainghazal): validate we have valid config for each endpoint. diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index ed0f2b9adb..50c084665f 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -7,7 +7,6 @@ import ( "time" "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" @@ -35,11 +34,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 +59,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,17 +175,20 @@ func TestAllConnectionsSuccessful(t *testing.T) { }) } -func TestBadInputFailure(t *testing.T) { - m := openvpn.NewExperimentMeasurer(openvpn.Config{}, "openvpn") +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) { @@ -391,9 +205,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 +218,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,21 +234,19 @@ 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 { diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go new file mode 100644 index 0000000000..e3262e1126 --- /dev/null +++ b/internal/experiment/openvpn/richerinput.go @@ -0,0 +1,146 @@ +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(ainghazal): we might want to get both the BaseURL and the HTTPClient from the session, +// and then deal with the openvpn-specific API calls ourselves within the boundaries of the experiment. +func (tl *targetLoader) loadFromBackend(_ context.Context) ([]model.ExperimentTarget, error) { + if tl.options.Provider == "" { + tl.options.Provider = defaultProvider + } + + targets := make([]model.ExperimentTarget, 0) + provider := tl.options.Provider + + // TODO(ainghazal): pass country code too (from session?) + apiConfig, err := tl.session.FetchOpenVPNConfig(context.Background(), provider, "XX") + if err != nil { + return nil, err + } + + auth, ok := providerAuthentication[provider] + if !ok { + return nil, fmt.Errorf("%w: unknown authentication for provider %s", ErrInvalidInput, provider) + } + + for _, input := range apiConfig.Inputs { + config := &Config{ + // Auth and Cipher are hardcoded for now. + 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 + } + 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..a9b74b1c84 --- /dev/null +++ b/internal/experiment/openvpn/richerinput_test.go @@ -0,0 +1,175 @@ +package openvpn + +import ( + "context" + "errors" + "testing" + + "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 + // + // note that we use a default test input for results predictability + // since the static list may change over time + 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) + } + }) + } +} 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..fd3b16cbf9 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -10,7 +10,8 @@ import ( "net/url" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/experiment/openvpn" + // FIXME - move this to the experiment + // "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" @@ -372,7 +373,8 @@ func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarg // 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 { + providers := []string{} + for _, provider := range providers { // fetchOpenVPNConfig ultimately uses an internal cache in the session to avoid // hitting the API too many times. reply, err := il.fetchOpenVPNConfig(ctx, provider) From b8ac182ecb6b382f958137a6d72992c81d215cbb Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Fri, 21 Jun 2024 19:23:02 +0200 Subject: [PATCH 02/18] revert previous changes to targetloading the separation of loadRemote by experiment name won't be needed anymore after transitioning to richerinput. --- internal/targetloading/targetloading.go | 56 +----------- internal/targetloading/targetloading_test.go | 94 +------------------- 2 files changed, 6 insertions(+), 144 deletions(-) diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index fd3b16cbf9..5706c70f61 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -10,8 +10,6 @@ import ( "net/url" "github.com/apex/log" - // FIXME - move this to the experiment - // "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" @@ -163,7 +161,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 @@ -309,21 +308,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 @@ -361,42 +345,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. - providers := []string{} - for _, provider := range providers { - // 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..bde59fd51b 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" @@ -563,7 +562,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 +579,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 +598,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 +631,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 +640,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 183c7abfaa2d9995ccc5d8afb2deedef4c335ede Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 01:30:49 +0200 Subject: [PATCH 03/18] tests: add test for query backend --- .../experiment/openvpn/richerinput_test.go | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index a9b74b1c84..83782d2bed 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -3,7 +3,9 @@ package openvpn import ( "context" "errors" + "fmt" "testing" + "time" "github.com/google/go-cmp/cmp" "github.com/ooni/probe-cli/v3/internal/mocks" @@ -173,3 +175,44 @@ func TestTargetLoaderLoad(t *testing.T) { }) } } + +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 + } + 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") + } + +} From a4bca298318f77af3cb04b6e6be0a52d9df63a0c Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 11:50:16 +0200 Subject: [PATCH 04/18] remove unneeded function --- internal/experiment/openvpn/endpoint.go | 13 ++----- internal/experiment/openvpn/endpoint_test.go | 39 ++++++++++---------- internal/experiment/openvpn/openvpn.go | 12 +----- 3 files changed, 23 insertions(+), 41 deletions(-) diff --git a/internal/experiment/openvpn/endpoint.go b/internal/experiment/openvpn/endpoint.go index abd6c53459..0502b1a391 100644 --- a/internal/experiment/openvpn/endpoint.go +++ b/internal/experiment/openvpn/endpoint.go @@ -49,6 +49,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) @@ -228,13 +231,3 @@ func mergeOpenVPNConfig( return cfg, 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 57243eeae0..162eadada1 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -22,7 +22,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", @@ -341,23 +359,4 @@ func Test_mergeOpenVPNConfig_with_unknown_provider(t *testing.T) { if !errors.Is(err, ErrInvalidInput) { t.Fatalf("expected invalid input error, got: %v", err) } - -} - -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 db87cd558e..d4e457021a 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -120,16 +120,6 @@ func (m Measurer) ExperimentVersion() string { return testVersion } -func parseEndpoint(input string) (*endpoint, error) { - if input == "" { - return nil, ErrInputRequired - } - if ok := isValidProtocol(input); !ok { - return nil, ErrInvalidInput - } - return newEndpointFromInputString(input) -} - // AuthMethod is the authentication method used by a provider. type AuthMethod string @@ -239,7 +229,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { config, input := target.Options, target.URL sess.Logger().Infof("openvpn: using richer input: %+v", input) - endpoint, err := parseEndpoint(input) + endpoint, err := newEndpointFromInputString(input) if err != nil { return err } From 9a2a1b66054de75cdf489e5ff8e257e681e182b5 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 12:06:33 +0200 Subject: [PATCH 05/18] add test for timestamps --- internal/experiment/openvpn/openvpn.go | 30 ++++++++++++--------- internal/experiment/openvpn/openvpn_test.go | 15 +++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index d4e457021a..17deee8c53 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -155,17 +155,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(), @@ -182,8 +172,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, }, @@ -191,6 +181,20 @@ func (m *Measurer) connectAndHandshake( } } +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, diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 50c084665f..ae47891af5 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -253,3 +253,18 @@ func TestSuccess(t *testing.T) { t.Fatal(err) } } + +func TestTimestampsFromHandshake(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 t0 == 2") + } + if duration != 2 { + t.Fatal("expected duration == 2") + } + +} From 038890fcc7bb2617ce272faf414aaaa7cf5da7cd Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 12:11:55 +0200 Subject: [PATCH 06/18] typo in test --- internal/experiment/openvpn/openvpn_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index ae47891af5..3c3ca3ed5b 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -261,7 +261,7 @@ func TestTimestampsFromHandshake(t *testing.T) { t.Fatal("expected t0 == 0") } if tlast != 2.0 { - t.Fatal("expected t0 == 2") + t.Fatal("expected t == 2") } if duration != 2 { t.Fatal("expected duration == 2") From bac3681d93f21afc44eb11123f80396dc0cf753c Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 19:03:13 +0200 Subject: [PATCH 07/18] address review comments --- internal/engine/session.go | 3 +++ internal/experiment/openvpn/endpoint.go | 8 +++---- internal/experiment/openvpn/openvpn.go | 24 ++++++++++----------- internal/experiment/openvpn/openvpn_test.go | 18 +++++++++++++++- internal/experiment/openvpn/richerinput.go | 8 +++---- internal/model/experiment.go | 3 +++ internal/targetloading/targetloading.go | 3 ++- 7 files changed, 45 insertions(+), 22 deletions(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index 0f41f5f61a..2bdd1bace2 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -384,6 +384,9 @@ func (s *Session) FetchOpenVPNConfig( return nil, err } + // ensure that we have fetched the location before fetching openvpn configuration. + s.MaybeLookupLocationContext(ctx) + // we cannot lock earlier because newOrchestraClient locks the mutex. defer s.mu.Unlock() s.mu.Lock() diff --git a/internal/experiment/openvpn/endpoint.go b/internal/experiment/openvpn/endpoint.go index 0502b1a391..336823dde0 100644 --- a/internal/experiment/openvpn/endpoint.go +++ b/internal/experiment/openvpn/endpoint.go @@ -1,7 +1,6 @@ package openvpn import ( - "errors" "fmt" "math/rand" "net" @@ -12,11 +11,12 @@ import ( 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. @@ -194,7 +194,7 @@ func isValidProvider(provider string) bool { return slices.Contains(APIEnabledProviders, provider) } -// mergeOpenVPNConfig gets a properly configured [*vpnconfig.Config] object for the given endpoint. +// mergeOpenVPNConfig 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 mergeOpenVPNConfig( tracer *vpntracex.Tracer, diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 17deee8c53..ac79649741 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -3,7 +3,6 @@ package openvpn import ( "context" - "errors" "strconv" "time" @@ -22,14 +21,8 @@ const ( openVPNProtocol = "openvpn" ) -// errors are in addition to any other errors returned by the low level packages -// that are used by this experiment to implement its functionality. var ( - // ErrInputRequired is returned when the experiment is not passed any input. - ErrInputRequired = targetloading.ErrInputRequired - - // ErrInvalidInput is returned if we failed to parse the input to obtain an endpoint we can measure. - ErrInvalidInput = errors.New("invalid input") + ErrInvalidInputType = targetloading.ErrInvalidInputType ) // Config contains the experiment config. @@ -219,7 +212,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { // 0. obtain the richer input target, config, and input or panic if args.Target == nil { - return ErrInputRequired + return targetloading.ErrInputRequired } tk := NewTestKeys() @@ -228,29 +221,36 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { idx := int64(1) handshakeTracer := vpntracex.NewTracerWithTransactionID(zeroTime, idx) - // build the input - target := args.Target.(*Target) + // 1. build the input + target, ok := args.Target.(*Target) + if !ok { + return targetloading.ErrInvalidInputType + } + config, input := target.Options, target.URL sess.Logger().Infof("openvpn: using richer input: %+v", input) + // 2. obtain the endpoint representation from the input URL endpoint, err := newEndpointFromInputString(input) if err != nil { return err } + // 3. build openvpn config from endpoint and options openvpnConfig, err := mergeOpenVPNConfig(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") + // 5. assign the testkeys measurement.TestKeys = tk // TODO(ainghazal): validate we have valid config for each endpoint. diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 3c3ca3ed5b..57651ba969 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -6,11 +6,13 @@ import ( "testing" "time" + "github.com/apex/log" "github.com/google/go-cmp/cmp" 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 { @@ -175,6 +177,20 @@ func TestAllConnectionsSuccessful(t *testing.T) { }) } +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() @@ -191,7 +207,7 @@ func TestBadTargetURLFailure(t *testing.T) { }, } err := m.Run(ctx, args) - if !errors.Is(err, openvpn.ErrInvalidInput) { + if !errors.Is(err, targetloading.ErrInvalidInput) { t.Fatalf("expected ErrInvalidInput, got %v", err) } } diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go index e3262e1126..4e6e30834c 100644 --- a/internal/experiment/openvpn/richerinput.go +++ b/internal/experiment/openvpn/richerinput.go @@ -105,7 +105,7 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err // TODO(ainghazal): we might want to get both the BaseURL and the HTTPClient from the session, // and then deal with the openvpn-specific API calls ourselves within the boundaries of the experiment. -func (tl *targetLoader) loadFromBackend(_ context.Context) ([]model.ExperimentTarget, error) { +func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.ExperimentTarget, error) { if tl.options.Provider == "" { tl.options.Provider = defaultProvider } @@ -113,15 +113,15 @@ func (tl *targetLoader) loadFromBackend(_ context.Context) ([]model.ExperimentTa targets := make([]model.ExperimentTarget, 0) provider := tl.options.Provider - // TODO(ainghazal): pass country code too (from session?) - apiConfig, err := tl.session.FetchOpenVPNConfig(context.Background(), provider, "XX") + 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", ErrInvalidInput, provider) + return nil, fmt.Errorf("%w: unknown authentication for provider %s", targetloading.ErrInvalidInput, provider) } for _, input := range apiConfig.Inputs { 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/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 5706c70f61..bdddb675f7 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -16,7 +16,7 @@ import ( "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") @@ -24,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. From 9a649d6a877d05d0de0318096658ac10f7c28167 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 19:18:44 +0200 Subject: [PATCH 08/18] remove default endpoints --- internal/experiment/openvpn/endpoint.go | 41 ++------------------ internal/experiment/openvpn/endpoint_test.go | 4 +- internal/experiment/openvpn/openvpn.go | 2 +- internal/experiment/openvpn/openvpn_test.go | 1 - 4 files changed, 6 insertions(+), 42 deletions(-) diff --git a/internal/experiment/openvpn/endpoint.go b/internal/experiment/openvpn/endpoint.go index 336823dde0..6c5157d525 100644 --- a/internal/experiment/openvpn/endpoint.go +++ b/internal/experiment/openvpn/endpoint.go @@ -2,7 +2,6 @@ package openvpn import ( "fmt" - "math/rand" "net" "net/url" "slices" @@ -149,60 +148,26 @@ 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 -} - // 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 array of APIEnabledProviders +// isValidProvider returns true if the provider is found as key in the array of [APIEnabledProviders]. func isValidProvider(provider string) bool { return slices.Contains(APIEnabledProviders, provider) } -// mergeOpenVPNConfig returns a properly configured [*vpnconfig.Config] object for the given endpoint. +// 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 mergeOpenVPNConfig( +func newOpenVPNConfig( tracer *vpntracex.Tracer, logger model.Logger, endpoint *endpoint, config *Config) (*vpnconfig.Config, error) { - // TODO(ainghazal): use merge ability in vpnconfig.OpenVPNOptions merge (pending PR) provider := endpoint.Provider if !isValidProvider(provider) { return nil, fmt.Errorf("%w: unknown provider: %s", ErrInvalidInput, provider) diff --git a/internal/experiment/openvpn/endpoint_test.go b/internal/experiment/openvpn/endpoint_test.go index 162eadada1..2cd7ba8af7 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -306,7 +306,7 @@ func Test_mergeVPNConfig(t *testing.T) { SafeKey: "key", } - cfg, err := mergeOpenVPNConfig(tracer, nil, e, config) + cfg, err := newOpenVPNConfig(tracer, nil, e, config) if err != nil { t.Fatalf("did not expect error, got: %v", err) } @@ -355,7 +355,7 @@ func Test_mergeOpenVPNConfig_with_unknown_provider(t *testing.T) { SafeCert: "cert", SafeKey: "key", } - _, err := mergeOpenVPNConfig(tracer, nil, e, cfg) + _, err := newOpenVPNConfig(tracer, nil, e, cfg) if !errors.Is(err, ErrInvalidInput) { t.Fatalf("expected invalid input error, got: %v", err) } diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index ac79649741..a51f8d9277 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -237,7 +237,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { } // 3. build openvpn config from endpoint and options - openvpnConfig, err := mergeOpenVPNConfig(handshakeTracer, sess.Logger(), endpoint, config) + openvpnConfig, err := newOpenVPNConfig(handshakeTracer, sess.Logger(), endpoint, config) if err != nil { return err } diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 57651ba969..77f71411df 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -282,5 +282,4 @@ func TestTimestampsFromHandshake(t *testing.T) { if duration != 2 { t.Fatal("expected duration == 2") } - } From 12752f3ae895df6927676982b69b8c5b68e3a24d Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 19:30:11 +0200 Subject: [PATCH 09/18] address review comments --- internal/experiment/openvpn/endpoint_test.go | 13 +------------ internal/experiment/openvpn/openvpn.go | 19 ++++++++++--------- internal/experiment/openvpn/openvpn_test.go | 4 ++-- internal/experiment/openvpn/richerinput.go | 9 ++++++--- .../experiment/openvpn/richerinput_test.go | 3 --- 5 files changed, 19 insertions(+), 29 deletions(-) diff --git a/internal/experiment/openvpn/endpoint_test.go b/internal/experiment/openvpn/endpoint_test.go index 2cd7ba8af7..bfd3eb6027 100644 --- a/internal/experiment/openvpn/endpoint_test.go +++ b/internal/experiment/openvpn/endpoint_test.go @@ -3,7 +3,6 @@ package openvpn import ( "errors" "fmt" - "sort" "testing" "time" @@ -270,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") @@ -289,7 +278,7 @@ func Test_isValidProvider(t *testing.T) { } } -func Test_mergeVPNConfig(t *testing.T) { +func Test_newVPNConfig(t *testing.T) { tracer := vpntracex.NewTracer(time.Now()) e := &endpoint{ Provider: "riseupvpn", diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index a51f8d9277..e2d8a9fd4c 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -95,21 +95,20 @@ func (tk *TestKeys) AllConnectionsSuccessful() bool { } // Measurer performs the measurement. -type Measurer struct { -} +type Measurer struct{} // NewExperimentMeasurer creates a new ExperimentMeasurer. func NewExperimentMeasurer() model.ExperimentMeasurer { - return Measurer{} + return &Measurer{} } // ExperimentName implements model.ExperimentMeasurer.ExperimentName. -func (m Measurer) ExperimentName() string { +func (m *Measurer) ExperimentName() string { return testName } // ExperimentVersion implements model.ExperimentMeasurer.ExperimentVersion. -func (m Measurer) ExperimentVersion() string { +func (m *Measurer) ExperimentVersion() string { return testVersion } @@ -174,6 +173,8 @@ 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 @@ -236,6 +237,10 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { 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 { @@ -253,10 +258,6 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { // 5. assign the testkeys 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?) - // Note: if here we return an error, the parent code will assume // something fundamental was wrong and we don't have a measurement // to submit to the OONI collector. Keep this in mind when you diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 77f71411df..6c25c90ae0 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -221,7 +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.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() _, err := m.FetchProviderCredentials( @@ -234,7 +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.Measurer) + m := openvpn.NewExperimentMeasurer().(*openvpn.Measurer) sess := makeMockSession() sess.MockFetchOpenVPNConfig = func(context.Context, string, string) (*model.OOAPIVPNProviderConfig, error) { diff --git a/internal/experiment/openvpn/richerinput.go b/internal/experiment/openvpn/richerinput.go index 4e6e30834c..050673b359 100644 --- a/internal/experiment/openvpn/richerinput.go +++ b/internal/experiment/openvpn/richerinput.go @@ -103,8 +103,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err return targets, nil } -// TODO(ainghazal): we might want to get both the BaseURL and the HTTPClient from the session, -// and then deal with the openvpn-specific API calls ourselves within the boundaries of the experiment. +// 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 @@ -126,7 +126,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment for _, input := range apiConfig.Inputs { config := &Config{ - // Auth and Cipher are hardcoded for now. + // 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", } @@ -135,6 +136,8 @@ func (tl *targetLoader) loadFromBackend(ctx context.Context) ([]model.Experiment 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, diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 83782d2bed..2fc0042469 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -142,9 +142,6 @@ func TestTargetLoaderLoad(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { // create a target loader using the given config - // - // note that we use a default test input for results predictability - // since the static list may change over time tl := &targetLoader{ loader: tc.loader, options: tc.options, From ffd68cd688e63c3aa1479d76056b74d8e6c92699 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 20:07:53 +0200 Subject: [PATCH 10/18] update mockable session --- internal/targetloading/targetloading_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/targetloading/targetloading_test.go b/internal/targetloading/targetloading_test.go index bde59fd51b..9cc9c0350e 100644 --- a/internal/targetloading/targetloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -529,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 @@ -550,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 From e90eaea50933a49952040c992907bc38ca3e867f Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 20:17:08 +0200 Subject: [PATCH 11/18] add mockProbeCC --- internal/experiment/openvpn/richerinput_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/experiment/openvpn/richerinput_test.go b/internal/experiment/openvpn/richerinput_test.go index 2fc0042469..4c04696635 100644 --- a/internal/experiment/openvpn/richerinput_test.go +++ b/internal/experiment/openvpn/richerinput_test.go @@ -192,6 +192,9 @@ func TestTargetLoaderLoadFromBackend(t *testing.T) { DateUpdated: time.Now(), }, nil } + sess.MockProbeCC = func() string { + return "IT" + } tl := &targetLoader{ loader: loader, options: &Config{}, From 0eac508c7e5682af2a825eb2d03e839411126c89 Mon Sep 17 00:00:00 2001 From: ain ghazal Date: Mon, 24 Jun 2024 20:18:46 +0200 Subject: [PATCH 12/18] phrasing --- internal/experiment/openvpn/openvpn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index e2d8a9fd4c..bd43a71ca1 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -211,7 +211,7 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { measurement := args.Measurement sess := args.Session - // 0. obtain the richer input target, config, and input or panic + // 0. fail if there's no richer input target if args.Target == nil { return targetloading.ErrInputRequired } From abfee9baa1a1f58e51b5ccbc33502231d8dc2347 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 17:47:24 +0200 Subject: [PATCH 13/18] x --- internal/experiment/dnscheck/dnscheck_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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") } } From 8cce08fd62540900ebd0bad3ebbb2ca2ba7242b1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 17:51:23 +0200 Subject: [PATCH 14/18] x --- internal/experiment/openvpn/openvpn_test.go | 52 ++++++++++++++++----- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/internal/experiment/openvpn/openvpn_test.go b/internal/experiment/openvpn/openvpn_test.go index 6c25c90ae0..e45b52ae75 100644 --- a/internal/experiment/openvpn/openvpn_test.go +++ b/internal/experiment/openvpn/openvpn_test.go @@ -271,15 +271,45 @@ func TestSuccess(t *testing.T) { } func TestTimestampsFromHandshake(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 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") + } + }) } From 1726fc4e119c88bfa579df6228bc423fbd4b324e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 17:52:43 +0200 Subject: [PATCH 15/18] x --- internal/experiment/openvpn/openvpn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index bd43a71ca1..4876fceebc 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -206,7 +206,7 @@ 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 From ec4873069e9a42a22d14d8f25015b7d44d0bd959 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 17:59:58 +0200 Subject: [PATCH 16/18] x --- internal/engine/session.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index e4a66cd0dc..f8c2b71394 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -385,9 +385,17 @@ func (s *Session) FetchOpenVPNConfig( } // ensure that we have fetched the location before fetching openvpn configuration. - s.MaybeLookupLocationContext(ctx) + if err := s.MaybeLookupLocationContext(ctx); err != nil { + return nil, err + } - // we cannot lock earlier because newOrchestraClient locks the mutex. + // 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() From 6e14b1f0ffed3b34ed452be0c82ad35e961330a6 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 18:00:22 +0200 Subject: [PATCH 17/18] x --- internal/engine/session.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/engine/session.go b/internal/engine/session.go index f8c2b71394..740ed549ee 100644 --- a/internal/engine/session.go +++ b/internal/engine/session.go @@ -394,7 +394,7 @@ func (s *Session) FetchOpenVPNConfig( // 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 + // 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() From a3f0932ac35a41c499cb28feed7e711ebf16d23c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 25 Jun 2024 18:07:54 +0200 Subject: [PATCH 18/18] x --- internal/experiment/openvpn/openvpn.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/experiment/openvpn/openvpn.go b/internal/experiment/openvpn/openvpn.go index 4876fceebc..0cf8255f43 100644 --- a/internal/experiment/openvpn/openvpn.go +++ b/internal/experiment/openvpn/openvpn.go @@ -227,9 +227,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { if !ok { return targetloading.ErrInvalidInputType } - config, input := target.Options, target.URL - sess.Logger().Infof("openvpn: using richer input: %+v", input) // 2. obtain the endpoint representation from the input URL endpoint, err := newEndpointFromInputString(input)