From d9aff19be50c1b4c72dc7928ca43a85fe42e2060 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 3 Apr 2021 21:25:08 +0200 Subject: [PATCH] refactor(tunnel): simplify tor implementation (#290) Simplify interaction within the package by avoiding to have a tor specific config. Use a Config instead. Part of https://github.com/ooni/probe/issues/985. --- internal/engine/tunnel/config.go | 37 +++++++++++ internal/engine/tunnel/psiphon.go | 6 +- internal/engine/tunnel/psiphon_test.go | 6 +- internal/engine/tunnel/tor.go | 64 ++++++------------- internal/engine/tunnel/tor_test.go | 86 ++++++++++++++------------ internal/engine/tunnel/tunnel.go | 2 +- 6 files changed, 109 insertions(+), 92 deletions(-) diff --git a/internal/engine/tunnel/config.go b/internal/engine/tunnel/config.go index ff2f310..4d19f99 100644 --- a/internal/engine/tunnel/config.go +++ b/internal/engine/tunnel/config.go @@ -4,6 +4,8 @@ import ( "context" "os" + "github.com/cretz/bine/control" + "github.com/cretz/bine/tor" "github.com/ooni/psiphon/oopsi/github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" ) @@ -29,6 +31,17 @@ type Config struct { // testStartPsiphon allows us to mock psiphon's clientlib.StartTunnel. testStartPsiphon func(ctx context.Context, config []byte, workdir string) (*clientlib.PsiphonTunnel, error) + + // testTorStart allows us to mock tor.Start. + testTorStart func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) + + // testTorEnableNetwork allows us to fake a failure when + // telling to the tor daemon to enable the network. + testTorEnableNetwork func(ctx context.Context, tor *tor.Tor, wait bool) error + + // testTorGetInfo allows us to fake a failure when + // getting info from the tor control port. + testTorGetInfo func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) } // mkdirAll calls either testMkdirAll or os.MkdirAll. @@ -56,3 +69,27 @@ func (c *Config) startPsiphon(ctx context.Context, config []byte, return clientlib.StartTunnel(ctx, config, "", clientlib.Parameters{ DataRootDirectory: &workdir}, nil, nil) } + +// torStart calls either testTorStart or tor.Start. +func (c *Config) torStart(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + if c.testTorStart != nil { + return c.testTorStart(ctx, conf) + } + return tor.Start(ctx, conf) +} + +// torEnableNetwork calls either testTorEnableNetwork or tor.EnableNetwork. +func (c *Config) torEnableNetwork(ctx context.Context, tor *tor.Tor, wait bool) error { + if c.testTorEnableNetwork != nil { + return c.testTorEnableNetwork(ctx, tor, wait) + } + return tor.EnableNetwork(ctx, wait) +} + +// torGetInfo calls either testTorGetInfo or ctrl.GetInfo. +func (c *Config) torGetInfo(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + if c.testTorGetInfo != nil { + return c.testTorGetInfo(ctrl, keys...) + } + return ctrl.GetInfo(keys...) +} diff --git a/internal/engine/tunnel/psiphon.go b/internal/engine/tunnel/psiphon.go index 73676e8..f0a7416 100644 --- a/internal/engine/tunnel/psiphon.go +++ b/internal/engine/tunnel/psiphon.go @@ -13,10 +13,14 @@ import ( // psiphonTunnel is a psiphon tunnel type psiphonTunnel struct { - tunnel *clientlib.PsiphonTunnel + // tunnel is the underlying psiphon tunnel + tunnel *clientlib.PsiphonTunnel + + // duration is the duration of the bootstrap duration time.Duration } +// makeworkingdir creates the working directory func makeworkingdir(config *Config) (string, error) { const testdirname = "oonipsiphon" workdir := filepath.Join(config.WorkDir, testdirname) diff --git a/internal/engine/tunnel/psiphon_test.go b/internal/engine/tunnel/psiphon_test.go index 40a1624..74ccba9 100644 --- a/internal/engine/tunnel/psiphon_test.go +++ b/internal/engine/tunnel/psiphon_test.go @@ -26,7 +26,7 @@ func TestPsiphonFetchPsiphonConfigFailure(t *testing.T) { } } -func TestPsiphonMakeMkdirAllFailure(t *testing.T) { +func TestPsiphonMkdirAllFailure(t *testing.T) { expected := errors.New("mocked error") sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), @@ -45,7 +45,7 @@ func TestPsiphonMakeMkdirAllFailure(t *testing.T) { } } -func TestPsiphonMakeRemoveAllFailure(t *testing.T) { +func TestPsiphonRemoveAllFailure(t *testing.T) { expected := errors.New("mocked error") sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), @@ -64,7 +64,7 @@ func TestPsiphonMakeRemoveAllFailure(t *testing.T) { } } -func TestPsiphonMakeStartFailure(t *testing.T) { +func TestPsiphonStartFailure(t *testing.T) { expected := errors.New("mocked error") sess := &mockable.Session{ MockableFetchPsiphonConfigResult: []byte(`{}`), diff --git a/internal/engine/tunnel/tor.go b/internal/engine/tunnel/tor.go index 6002c73..9a54763 100644 --- a/internal/engine/tunnel/tor.go +++ b/internal/engine/tunnel/tor.go @@ -8,23 +8,28 @@ import ( "strings" "time" - "github.com/cretz/bine/control" "github.com/cretz/bine/tor" ) // torProcess is a running tor process type torProcess interface { + // Close kills the running tor process Close() error } // torTunnel is the Tor tunnel type torTunnel struct { + // bootstrapTime is the duration of the bootstrap bootstrapTime time.Duration - instance torProcess - proxy *url.URL + + // instance is the running tor instance + instance torProcess + + // proxy is the SOCKS5 proxy URL + proxy *url.URL } -// BootstrapTime is the bootstrsap time +// BootstrapTime returns the bootstrap time func (tt *torTunnel) BootstrapTime() (duration time.Duration) { if tt != nil { duration = tt.bootstrapTime @@ -47,47 +52,23 @@ func (tt *torTunnel) Stop() { } } -// torStartConfig contains the configuration for StartWithConfig -type torStartConfig struct { - Sess Session - Start func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) - EnableNetwork func(ctx context.Context, tor *tor.Tor, wait bool) error - GetInfo func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) -} - -// torStart starts the tor tunnel -func torStart(ctx context.Context, sess Session) (Tunnel, error) { - return torStartWithConfig(ctx, torStartConfig{ - Sess: sess, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { - return tor.Start(ctx, conf) - }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { - return tor.EnableNetwork(ctx, wait) - }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { - return ctrl.GetInfo(keys...) - }, - }) -} - -// torStartWithConfig is a configurable torStart for testing -func torStartWithConfig(ctx context.Context, config torStartConfig) (Tunnel, error) { +// torStart starts the tor tunnel. +func torStart(ctx context.Context, config *Config) (Tunnel, error) { select { case <-ctx.Done(): return nil, ctx.Err() // allows to write unit tests using this code default: } - logfile := LogFile(config.Sess) - extraArgs := append([]string{}, config.Sess.TorArgs()...) + logfile := LogFile(config.Session) + extraArgs := append([]string{}, config.Session.TorArgs()...) extraArgs = append(extraArgs, "Log") extraArgs = append(extraArgs, "notice stderr") extraArgs = append(extraArgs, "Log") extraArgs = append(extraArgs, fmt.Sprintf(`notice file %s`, logfile)) - instance, err := config.Start(ctx, &tor.StartConf{ - DataDir: path.Join(config.Sess.TempDir(), "tor"), + instance, err := config.torStart(ctx, &tor.StartConf{ + DataDir: path.Join(config.Session.TempDir(), "tor"), ExtraArgs: extraArgs, - ExePath: config.Sess.TorBinary(), + ExePath: config.Session.TorBinary(), NoHush: true, }) if err != nil { @@ -95,13 +76,13 @@ func torStartWithConfig(ctx context.Context, config torStartConfig) (Tunnel, err } instance.StopProcessOnClose = true start := time.Now() - if err := config.EnableNetwork(ctx, instance, true); err != nil { + if err := config.torEnableNetwork(ctx, instance, true); err != nil { instance.Close() return nil, err } stop := time.Now() // Adapted from - info, err := config.GetInfo(instance.Control, "net/listeners/socks") + info, err := config.torGetInfo(instance.Control, "net/listeners/socks") if err != nil { instance.Close() return nil, err @@ -127,12 +108,3 @@ func torStartWithConfig(ctx context.Context, config torStartConfig) (Tunnel, err func LogFile(sess Session) string { return path.Join(sess.TempDir(), "tor.log") } - -// newTorTunnel creates a new torTunnel -func newTorTunnel(bootstrapTime time.Duration, instance torProcess, proxy *url.URL) *torTunnel { - return &torTunnel{ - bootstrapTime: bootstrapTime, - instance: instance, - proxy: proxy, - } -} diff --git a/internal/engine/tunnel/tor_test.go b/internal/engine/tunnel/tor_test.go index e14bab2..c0b9fda 100644 --- a/internal/engine/tunnel/tor_test.go +++ b/internal/engine/tunnel/tor_test.go @@ -23,7 +23,11 @@ func (c *Closer) Close() error { func TestTorTunnelNonNil(t *testing.T) { closer := new(Closer) proxy := &url.URL{Scheme: "x", Host: "10.0.0.1:443"} - tun := newTorTunnel(128, closer, proxy) + tun := &torTunnel{ + bootstrapTime: 128, + instance: closer, + proxy: proxy, + } if tun.BootstrapTime() != 128 { t.Fatal("not the bootstrap time we expected") } @@ -50,7 +54,7 @@ func TestTorTunnelNil(t *testing.T) { func TestTorStartWithCancelledContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - tun, err := torStart(ctx, &mockable.Session{}) + tun, err := torStart(ctx, &Config{Session: &mockable.Session{}}) if !errors.Is(err, context.Canceled) { t.Fatal("not the error we expected") } @@ -59,12 +63,12 @@ func TestTorStartWithCancelledContext(t *testing.T) { } } -func TestTorStartWithConfigStartFailure(t *testing.T) { +func TestTorStartStartFailure(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return nil, expected }, }) @@ -76,15 +80,15 @@ func TestTorStartWithConfigStartFailure(t *testing.T) { } } -func TestTorStartWithConfigEnableNetworkFailure(t *testing.T) { +func TestTorStartEnableNetworkFailure(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return expected }, }) @@ -96,18 +100,18 @@ func TestTorStartWithConfigEnableNetworkFailure(t *testing.T) { } } -func TestTorStartWithConfigGetInfoFailure(t *testing.T) { +func TestTorStartGetInfoFailure(t *testing.T) { expected := errors.New("mocked error") ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return nil }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + testTorGetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { return nil, expected }, }) @@ -119,17 +123,17 @@ func TestTorStartWithConfigGetInfoFailure(t *testing.T) { } } -func TestTorStartWithConfigGetInfoInvalidNumberOfKeys(t *testing.T) { +func TestTorStartGetInfoInvalidNumberOfKeys(t *testing.T) { ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return nil }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + testTorGetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { return nil, nil }, }) @@ -141,17 +145,17 @@ func TestTorStartWithConfigGetInfoInvalidNumberOfKeys(t *testing.T) { } } -func TestTorStartWithConfigGetInfoInvalidKey(t *testing.T) { +func TestTorStartGetInfoInvalidKey(t *testing.T) { ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return nil }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + testTorGetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { return []*control.KeyVal{{}}, nil }, }) @@ -163,17 +167,17 @@ func TestTorStartWithConfigGetInfoInvalidKey(t *testing.T) { } } -func TestTorStartWithConfigGetInfoInvalidProxyType(t *testing.T) { +func TestTorStartGetInfoInvalidProxyType(t *testing.T) { ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return nil }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + testTorGetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { return []*control.KeyVal{{Key: "net/listeners/socks", Val: "127.0.0.1:9050"}}, nil }, }) @@ -185,17 +189,17 @@ func TestTorStartWithConfigGetInfoInvalidProxyType(t *testing.T) { } } -func TestTorStartWithConfigSuccess(t *testing.T) { +func TestTorStartUnsupportedProxy(t *testing.T) { ctx := context.Background() - tun, err := torStartWithConfig(ctx, torStartConfig{ - Sess: &mockable.Session{}, - Start: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { + tun, err := torStart(ctx, &Config{ + Session: &mockable.Session{}, + testTorStart: func(ctx context.Context, conf *tor.StartConf) (*tor.Tor, error) { return &tor.Tor{}, nil }, - EnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { + testTorEnableNetwork: func(ctx context.Context, tor *tor.Tor, wait bool) error { return nil }, - GetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { + testTorGetInfo: func(ctrl *control.Conn, keys ...string) ([]*control.KeyVal, error) { return []*control.KeyVal{{Key: "net/listeners/socks", Val: "unix:/foo/bar"}}, nil }, }) diff --git a/internal/engine/tunnel/tunnel.go b/internal/engine/tunnel/tunnel.go index d11926e..e203f70 100644 --- a/internal/engine/tunnel/tunnel.go +++ b/internal/engine/tunnel/tunnel.go @@ -41,7 +41,7 @@ func Start(ctx context.Context, config *Config) (Tunnel, error) { return enforceNilContract(tun, err) case "tor": logger.Infof("starting %s tunnel; please be patient...", config.Name) - tun, err := torStart(ctx, config.Session) + tun, err := torStart(ctx, config) return enforceNilContract(tun, err) default: return nil, errors.New("unsupported tunnel")