fix(tunnel): pass /absolute/path/to/tor to cretz/bine (#323)

* fix(tunnel): pass /absolute/path/to/tor to cretz/bine

It seems cretz/bine is not aware of https://blog.golang.org/path-security
for now. I am planning to send over a diff for that later today.

In the meanwhile, do the right thing here, and make sure that we obtain
the absolute path to the tor binary before we continue.

This work is part of https://github.com/ooni/probe-engine/issues/283.

* fix tests when tor is not installed

internal/engine/session.go

@@ -171,6 +171,7 @@ func NewSession(ctx context.Context, config SessionConfig) (*Session, error) {
 			config.Logger.Infof(
 				"starting '%s' tunnel; please be patient...", proxyURL.Scheme)
 			tunnel, err := tunnel.Start(ctx, &tunnel.Config{
+				Logger:    config.Logger,
 				Name:      proxyURL.Scheme,
 				Session:   &sessionTunnelEarlySession{},
 				TorArgs:   config.TorArgs,

internal/engine/tunnel/config.go

@@ -9,12 +9,24 @@ import (
 	"github.com/cretz/bine/control"
 	"github.com/cretz/bine/tor"
 	"github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib"
+	"golang.org/x/sys/execabs"
 )
 
+// Logger is the logger to use. Its signature is compatibile
+// with the apex/log logger signature.
+type Logger interface {
+	// Infof formats and emits an informative message
+	Infof(format string, v ...interface{})
+}
+
 // Config contains the configuration for creating a Tunnel instance. You need
 // to fill all the mandatory fields. You SHOULD NOT modify the content of this
 // structure while in use, because that may lead to data races.
 type Config struct {
+	// Logger is the logger to use. If empty we use a default
+	// implementation that does not emit any output.
+	Logger Logger
+
 	// Name is the mandatory name of the tunnel. We support
 	// "tor", "psiphon", and "fake" tunnels. You SHOULD
 	// use "fake" tunnels only for testing: they don't provide
@@ -41,6 +53,9 @@ type Config struct {
 	// Start function fails with ErrEmptyTunnelDir.
 	TunnelDir string
 
+	// testExecabsLookPath allows us to mock exeabs.LookPath
+	testExecabsLookPath func(name string) (string, error)
+
 	// testMkdirAll allows us to mock os.MkdirAll in testing code.
 	testMkdirAll func(path string, perm os.FileMode) error
 
@@ -66,6 +81,31 @@ type Config struct {
 	testTorGetInfo func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error)
 }
 
+// silentLogger is a logger that does not emit output.
+type silentLogger struct{}
+
+// Infof implements Logger.Infof.
+func (sl *silentLogger) Infof(format string, v ...interface{}) {}
+
+// defaultLogger is the default logger.
+var defaultLogger = &silentLogger{}
+
+// logger returns the logger to use.
+func (c *Config) logger() Logger {
+	if c.Logger != nil {
+		return c.Logger
+	}
+	return defaultLogger
+}
+
+// execabsLookPath calls either testExeabsLookPath or execabs.LookPath
+func (c *Config) execabsLookPath(name string) (string, error) {
+	if c.testExecabsLookPath != nil {
+		return c.testExecabsLookPath(name)
+	}
+	return execabs.LookPath(name)
+}
+
 // mkdirAll calls either testMkdirAll or os.MkdirAll.
 func (c *Config) mkdirAll(path string, perm os.FileMode) error {
 	if c.testMkdirAll != nil {
@@ -100,6 +140,15 @@ func (c *Config) startPsiphon(ctx context.Context, config []byte,
 		DataRootDirectory: &workdir}, nil, nil)
 }
 
+// torBinary returns the tor binary path, if configured, or
+// the default path, otherwise.
+func (c *Config) torBinary() string {
+	if c.TorBinary != "" {
+		return c.TorBinary
+	}
+	return "tor"
+}
+
 // torStart calls either testTorStart or tor.Start.
 func (c *Config) torStart(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 	if c.testTorStart != nil {

internal/engine/tunnel/config_test.go

@@ -0,0 +1,36 @@
+package tunnel
+
+import (
+	"testing"
+
+	"github.com/apex/log"
+)
+
+func TestConfigLoggerDefault(t *testing.T) {
+	config := &Config{}
+	if config.logger() != defaultLogger {
+		t.Fatal("not the logger we expected")
+	}
+}
+
+func TestConfigLoggerCustom(t *testing.T) {
+	config := &Config{Logger: log.Log}
+	if config.logger() != log.Log {
+		t.Fatal("not the logger we expected")
+	}
+}
+
+func TestTorBinaryNotSet(t *testing.T) {
+	config := &Config{}
+	if config.torBinary() != "tor" {
+		t.Fatal("not the result we expected")
+	}
+}
+
+func TestTorBinarySet(t *testing.T) {
+	path := "/usr/local/bin/tor"
+	config := &Config{TorBinary: path}
+	if config.torBinary() != path {
+		t.Fatal("not the result we expected")
+	}
+}

internal/engine/tunnel/tor.go

@@ -74,10 +74,18 @@ func torStart(ctx context.Context, config *Config) (Tunnel, error) {
 	extraArgs = append(extraArgs, "notice stderr")
 	extraArgs = append(extraArgs, "Log")
 	extraArgs = append(extraArgs, fmt.Sprintf(`notice file %s`, logfile))
+	// Implementation note: here we make sure that we're not going to
+	// execute a binary called "tor" in the current directory on Windows
+	// as documented in https://blog.golang.org/path-security.
+	exePath, err := config.execabsLookPath(config.torBinary())
+	if err != nil {
+		return nil, err
+	}
+	config.logger().Infof("tunnel: exec: %s %+v", exePath, extraArgs)
 	instance, err := config.torStart(ctx, &tor.StartConf{
 		DataDir:   stateDir,
 		ExtraArgs: extraArgs,
-		ExePath:   config.TorBinary,
+		ExePath:   exePath,
 		NoHush:    true,
 	})
 	if err != nil {

internal/engine/tunnel/tor_integration_test.go

@@ -3,21 +3,21 @@ package tunnel_test
 import (
 	"context"
 	"io/ioutil"
-	"os"
 	"testing"
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/engine/tunnel"
+	"golang.org/x/sys/execabs"
 )
 
 func TestTorStartStop(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	torBinaryPath := "/usr/bin/tor"
-	if m, err := os.Stat(torBinaryPath); err != nil || !m.Mode().IsRegular() {
-		t.Skip("missing precondition for the test")
+	torBinaryPath, err := execabs.LookPath("tor")
+	if err != nil {
+		t.Skip("missing precondition for the test: tor not in PATH")
 	}
 	tunnelDir, err := ioutil.TempDir("testdata", "tor")
 	if err != nil {

internal/engine/tunnel/tor_test.go

@@ -9,6 +9,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
 
 	"github.com/cretz/bine/control"
@@ -77,12 +78,30 @@ func TestTorWithEmptyTunnelDir(t *testing.T) {
 	}
 }
 
+func TestTorBinaryNotFoundFailure(t *testing.T) {
+	ctx := context.Background()
+	tun, err := torStart(ctx, &Config{
+		Session:   &mockable.Session{},
+		TorBinary: "/nonexistent/directory/tor",
+		TunnelDir: "testdata",
+	})
+	if !errors.Is(err, syscall.ENOENT) {
+		t.Fatal("not the error we expected", err)
+	}
+	if tun != nil {
+		t.Fatal("expected nil tunnel here")
+	}
+}
+
 func TestTorStartFailure(t *testing.T) {
 	expected := errors.New("mocked error")
 	ctx := context.Background()
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return nil, expected
 		},
@@ -101,6 +120,9 @@ func TestTorEnableNetworkFailure(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -122,6 +144,9 @@ func TestTorGetInfoFailure(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -145,6 +170,9 @@ func TestTorGetInfoInvalidNumberOfKeys(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -168,6 +196,9 @@ func TestTorGetInfoInvalidKey(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -191,6 +222,9 @@ func TestTorGetInfoInvalidProxyType(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},
@@ -214,6 +248,9 @@ func TestTorUnsupportedProxy(t *testing.T) {
 	tun, err := torStart(ctx, &Config{
 		Session:   &mockable.Session{},
 		TunnelDir: "testdata",
+		testExecabsLookPath: func(name string) (string, error) {
+			return "/usr/local/bin/tor", nil
+		},
 		testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) {
 			return &tor.Tor{}, nil
 		},