fix: avoid submitting when tor binary is missing (#1078)

This diff modifies tunnel, torsf, and vanilla such that: 1. we return a special error when tor is not found on PATH 2. we use such an error to avoid submitting By doing that, we save some useless submissions. Part of ooni/probe#2406 because I wrote this diff while investigating it. --------- Co-authored-by: DecFox <[email protected]>
ooni · Jun 12, 2023 · e9cc324 · e9cc324
1 parent 00e4520
commit e9cc324
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 7 deletions.
diff --git a/internal/experiment/torsf/torsf.go b/internal/experiment/torsf/torsf.go
@@ -25,7 +25,7 @@ import (
 // We may want to have a single implementation for both nettests in the future.
 
 // testVersion is the experiment version.
-const testVersion = "0.3.0"
+const testVersion = "0.4.0"
 
 // Config contains the experiment config.
 type Config struct {
@@ -82,6 +82,9 @@ type TestKeys struct {
 
 	// TransportName is always set to "snowflake" for this experiment.
 	TransportName string `json:"transport_name"`
+
+	// cannotFindTorBinary indicates that we could not find the tor binary.
+	cannotFindTorBinary bool
 }
 
 // Measurer performs the measurement.
@@ -147,6 +150,9 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		case tk := <-tkch:
 			measurement.TestKeys = tk
 			callbacks.OnProgress(1.0, "torsf experiment is finished")
+			if tk.cannotFindTorBinary {
+				return tunnel.ErrCannotFindTorBinary
+			}
 			return nil
 		case <-ticker.C:
 			if !m.config.DisableProgress {
@@ -228,7 +234,9 @@ func (m *Measurer) bootstrap(ctx context.Context, timeout time.Duration, sess mo
 			"ClientTransportPlugin", ptl.AsClientTransportPluginArgument(),
 			"Bridge", sfdialer.AsBridgeArgument(),
 		},
+		TorBinary: sess.TorBinary(),
 	})
+	tk.cannotFindTorBinary = errors.Is(err, tunnel.ErrCannotFindTorBinary)
 	tk.TorVersion = debugInfo.Version
 	m.readTorLogs(sess.Logger(), tk, debugInfo.LogFilePath)
 	if err != nil {

diff --git a/internal/experiment/torsf/torsf_test.go b/internal/experiment/torsf/torsf_test.go
@@ -25,7 +25,7 @@ func TestExperimentNameAndVersion(t *testing.T) {
 	if m.ExperimentName() != "torsf" {
 		t.Fatal("invalid experiment name")
 	}
-	if m.ExperimentVersion() != "0.3.0" {
+	if m.ExperimentVersion() != "0.4.0" {
 		t.Fatal("invalid experiment version")
 	}
 }
@@ -301,6 +301,82 @@ func TestFailureToStartTunnel(t *testing.T) {
 	}
 }
 
+func TestFailureNoTorBinary(t *testing.T) {
+	t.Run("with mocked startTunnel", func(t *testing.T) {
+		expected := tunnel.ErrCannotFindTorBinary
+		m := &Measurer{
+			config: Config{},
+			mockStartTunnel: func(
+				ctx context.Context, config *tunnel.Config) (tunnel.Tunnel, tunnel.DebugInfo, error) {
+				return nil,
+					tunnel.DebugInfo{
+						Name:        "tor",
+						LogFilePath: filepath.Join("testdata", "partial.log"),
+					}, expected
+			},
+		}
+		ctx := context.Background()
+		measurement := &model.Measurement{}
+		sess := &mockable.Session{
+			MockableLogger: model.DiscardLogger,
+		}
+		callbacks := &model.PrinterCallbacks{
+			Logger: model.DiscardLogger,
+		}
+		args := &model.ExperimentArgs{
+			Callbacks:   callbacks,
+			Measurement: measurement,
+			Session:     sess,
+		}
+		if err := m.Run(ctx, args); !errors.Is(err, expected) {
+			t.Fatal(err)
+		}
+		tk := measurement.TestKeys.(*TestKeys)
+		if tk.BootstrapTime != 0 {
+			t.Fatal("unexpected bootstrap time")
+		}
+		if tk.Error == nil || *tk.Error != "unknown-error" {
+			t.Fatal("unexpected error")
+		}
+		if tk.Failure == nil {
+			t.Fatal("unexpectedly nil failure string")
+		}
+		if *tk.Failure != "unknown_failure: tunnel: cannot find tor binary" {
+			t.Fatal("unexpected failure string", *tk.Failure)
+		}
+		if !tk.PersistentDatadir {
+			t.Fatal("unexpected persistent datadir")
+		}
+		if tk.RendezvousMethod != "domain_fronting" {
+			t.Fatal("unexpected rendezvous method")
+		}
+		if tk.Success {
+			t.Fatal("unexpected success value")
+		}
+		if !tk.cannotFindTorBinary {
+			t.Fatal("unexpected cannotFindTorBinary values")
+		}
+		if tk.Timeout != maxRuntime.Seconds() {
+			t.Fatal("unexpected timeout")
+		}
+		if count := len(tk.TorLogs); count != 6 {
+			t.Fatal("unexpected length of tor logs", count)
+		}
+		if tk.TorProgress != 15 {
+			t.Fatal("unexpected tor progress")
+		}
+		if tk.TorProgressTag != "handshake_done" {
+			t.Fatal("unexpected tor progress tag")
+		}
+		if tk.TorProgressSummary != "Handshake with a relay done" {
+			t.Fatal("unexpected tor progress tag")
+		}
+		if tk.TransportName != "snowflake" {
+			t.Fatal("invalid transport name")
+		}
+	})
+}
+
 func TestBaseTunnelDir(t *testing.T) {
 	t.Run("without persistent data dir", func(t *testing.T) {
 		m := &Measurer{

diff --git a/internal/experiment/vanillator/vanillator.go b/internal/experiment/vanillator/vanillator.go
@@ -23,7 +23,7 @@ import (
 // We may want to have a single implementation for both nettests in the future.
 
 // testVersion is the experiment version.
-const testVersion = "0.2.0"
+const testVersion = "0.3.0"
 
 // Config contains the experiment config.
 type Config struct {
@@ -68,6 +68,9 @@ type TestKeys struct {
 
 	// TransportName is always set to "vanilla" for this experiment.
 	TransportName string `json:"transport_name"`
+
+	// cannotFindTorBinary indicates that we could not find the tor binary.
+	cannotFindTorBinary bool
 }
 
 // Measurer performs the measurement.
@@ -123,6 +126,9 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		case tk := <-tkch:
 			measurement.TestKeys = tk
 			callbacks.OnProgress(1.0, "vanilla_tor experiment is finished")
+			if tk.cannotFindTorBinary {
+				return tunnel.ErrCannotFindTorBinary
+			}
 			return nil
 		case <-ticker.C:
 			if !m.config.DisableProgress {
@@ -168,7 +174,9 @@ func (m *Measurer) bootstrap(ctx context.Context, timeout time.Duration,
 		Session:   sess,
 		TunnelDir: path.Join(m.baseTunnelDir(sess), "vanillator"),
 		Logger:    sess.Logger(),
+		TorBinary: sess.TorBinary(),
 	})
+	tk.cannotFindTorBinary = errors.Is(err, tunnel.ErrCannotFindTorBinary)
 	tk.TorVersion = debugInfo.Version
 	m.readTorLogs(sess.Logger(), tk, debugInfo.LogFilePath)
 	if err != nil {

diff --git a/internal/experiment/vanillator/vanillator_test.go b/internal/experiment/vanillator/vanillator_test.go
@@ -24,7 +24,7 @@ func TestExperimentNameAndVersion(t *testing.T) {
 	if m.ExperimentName() != "vanilla_tor" {
 		t.Fatal("invalid experiment name")
 	}
-	if m.ExperimentVersion() != "0.2.0" {
+	if m.ExperimentVersion() != "0.3.0" {
 		t.Fatal("invalid experiment version")
 	}
 }
@@ -224,6 +224,76 @@ func TestFailureToStartTunnel(t *testing.T) {
 	}
 }
 
+func TestFailureNoTorBinary(t *testing.T) {
+	t.Run("with mocked startTunnel", func(t *testing.T) {
+		expected := tunnel.ErrCannotFindTorBinary
+		m := &Measurer{
+			config: Config{},
+			mockStartTunnel: func(ctx context.Context, config *tunnel.Config) (tunnel.Tunnel, tunnel.DebugInfo, error) {
+				return nil,
+					tunnel.DebugInfo{
+						Name:        "tor",
+						LogFilePath: filepath.Join("testdata", "partial.log"),
+					}, expected
+			},
+		}
+		ctx := context.Background()
+		measurement := &model.Measurement{}
+		sess := &mockable.Session{
+			MockableLogger: model.DiscardLogger,
+		}
+		callbacks := &model.PrinterCallbacks{
+			Logger: model.DiscardLogger,
+		}
+		args := &model.ExperimentArgs{
+			Callbacks:   callbacks,
+			Measurement: measurement,
+			Session:     sess,
+		}
+		if err := m.Run(ctx, args); !errors.Is(err, expected) {
+			t.Fatal("unexpected error")
+		}
+		tk := measurement.TestKeys.(*TestKeys)
+		if tk.BootstrapTime != 0 {
+			t.Fatal("unexpected bootstrap time")
+		}
+		if tk.Error == nil || *tk.Error != "unknown-error" {
+			t.Fatal("unexpected error")
+		}
+		if tk.Failure == nil {
+			t.Fatal("unexpectedly nil failure string")
+		}
+		if *tk.Failure != "unknown_failure: tunnel: cannot find tor binary" {
+			t.Fatal("unexpected failure string", *tk.Failure)
+		}
+		if tk.Success {
+			t.Fatal("unexpected success value")
+		}
+		if !tk.cannotFindTorBinary {
+			t.Fatal("unexpected cannotFindTorBinary values")
+		}
+		if tk.Timeout != maxRuntime.Seconds() {
+			t.Fatal("unexpected timeout")
+		}
+		if count := len(tk.TorLogs); count != 6 {
+			t.Fatal("unexpected length of tor logs", count)
+		}
+		if tk.TorProgress != 15 {
+			t.Fatal("unexpected tor progress")
+		}
+		if tk.TorProgressTag != "handshake_done" {
+			t.Fatal("unexpected tor progress tag")
+		}
+		if tk.TorProgressSummary != "Handshake with a relay done" {
+			t.Fatal("unexpected tor progress tag")
+		}
+		if tk.TransportName != "vanilla" {
+			t.Fatal("invalid transport name")
+		}
+
+	})
+}
+
 func TestGetSummaryKeys(t *testing.T) {
 	t.Run("in case of untyped nil TestKeys", func(t *testing.T) {
 		measurement := &model.Measurement{

diff --git a/internal/tunnel/tor.go b/internal/tunnel/tor.go
@@ -12,6 +12,11 @@ import (
 	"time"
 )
 
+// ErrCannotFindTorBinary is an error emitted when we cannot find the
+// tor binary. We use this error in vanillator and torsf to detect
+// cases where this happens and avoid submitting measurements.
+var ErrCannotFindTorBinary = errors.New("tunnel: cannot find tor binary")
+
 // torProcess is a running tor process.
 type torProcess interface {
 	io.Closer

diff --git a/internal/tunnel/tor_test.go b/internal/tunnel/tor_test.go
@@ -8,7 +8,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"syscall"
 	"testing"
 
 	"github.com/cretz/bine/control"
@@ -83,7 +82,7 @@ func TestTorBinaryNotFoundFailure(t *testing.T) {
 		TorBinary: "/nonexistent/directory/tor",
 		TunnelDir: "testdata",
 	})
-	if !errors.Is(err, syscall.ENOENT) {
+	if !errors.Is(err, ErrCannotFindTorBinary) {
 		t.Fatal("not the error we expected", err)
 	}
 	if tun != nil {

diff --git a/internal/tunnel/tordesktop.go b/internal/tunnel/tordesktop.go
@@ -10,6 +10,7 @@ package tunnel
 //
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/cretz/bine/tor"
@@ -21,7 +22,7 @@ func getTorStartConf(config *Config, dataDir string, extraArgs []string) (*tor.S
 	exePath, err := config.torBinary()
 	if err != nil {
 		config.logger().Warnf("cannot find tor binary: %s", err.Error())
-		return nil, err
+		return nil, fmt.Errorf("%w: %s", ErrCannotFindTorBinary, err.Error())
 	}
 	config.logger().Infof("tunnel: tor: exec: %s %s %s", exePath,
 		dataDir, strings.Join(extraArgs, " "))