feat(openvpn): implement richer input (#1625)

This commit: 1. modifies `./internal/registry` and its `openvpn.go` file such that `openvpn` has its own private target loader; 2. modifies `./internal/experiment/openvpn` to use the richer input targets to merge the options for the openvpn experiment. 3. removes cache from session after fetching openvpn config ## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2600 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request - [x] if you changed code inside an experiment, make sure you bump its version number --------- Co-authored-by: Simone Basso <[email protected]>
ooni · Jun 25, 2024 · acab902 · acab902
1 parent 65df439
commit acab902
Show file tree

Hide file tree

Showing 12 changed files with 593 additions and 731 deletions.
diff --git 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,23 +379,30 @@ 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()
 
 	config, err := clnt.FetchOpenVPNConfig(ctx, provider, cc)
 	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
@@ -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
@@ -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,42 +182,17 @@ 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),
 	)
 
 	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
-}