From a9b3a3b3a583436b6d91111e4e6760d2b2304315 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 4 May 2021 08:14:25 +0200 Subject: [PATCH] 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 | 1 + internal/engine/tunnel/config.go | 49 +++++++++++++++++++ internal/engine/tunnel/config_test.go | 36 ++++++++++++++ internal/engine/tunnel/tor.go | 10 +++- .../engine/tunnel/tor_integration_test.go | 8 +-- internal/engine/tunnel/tor_test.go | 37 ++++++++++++++ 6 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 internal/engine/tunnel/config_test.go diff --git a/internal/engine/session.go b/internal/engine/session.go index 079d1f6..11e42b9 100644 --- a/internal/engine/session.go +++ b/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, diff --git a/internal/engine/tunnel/config.go b/internal/engine/tunnel/config.go index 9ccd1aa..a85056f 100644 --- a/internal/engine/tunnel/config.go +++ b/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 { diff --git a/internal/engine/tunnel/config_test.go b/internal/engine/tunnel/config_test.go new file mode 100644 index 0000000..0654038 --- /dev/null +++ b/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") + } +} diff --git a/internal/engine/tunnel/tor.go b/internal/engine/tunnel/tor.go index 96e299c..fdc59ff 100644 --- a/internal/engine/tunnel/tor.go +++ b/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 { diff --git a/internal/engine/tunnel/tor_integration_test.go b/internal/engine/tunnel/tor_integration_test.go index 15ed457..f52702c 100644 --- a/internal/engine/tunnel/tor_integration_test.go +++ b/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 { diff --git a/internal/engine/tunnel/tor_test.go b/internal/engine/tunnel/tor_test.go index c180041..35dc6be 100644 --- a/internal/engine/tunnel/tor_test.go +++ b/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 },