From 4b769564372dd34f52aa38673db8efa27f008a9f Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Mon, 17 Jun 2024 20:15:37 -0400 Subject: [PATCH 1/8] Use Psiphon clientlib --- x/go.mod | 2 +- x/go.sum | 4 +- x/psiphon/psiphon.go | 179 +++++++++----------------------------- x/psiphon/psiphon_test.go | 75 +++------------- 4 files changed, 55 insertions(+), 205 deletions(-) diff --git a/x/go.mod b/x/go.mod index 640b9c73..df1b2ca4 100644 --- a/x/go.mod +++ b/x/go.mod @@ -6,7 +6,7 @@ require ( github.com/Jigsaw-Code/outline-sdk v0.0.16 // Use github.com/Psiphon-Labs/psiphon-tunnel-core@staging-client as per // https://github.com/Psiphon-Labs/psiphon-tunnel-core/?tab=readme-ov-file#using-psiphon-with-go-modules - github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240522172529-8fcc4b9a51cf + github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0 github.com/songgao/water v0.0.0-20200317203138-2b4b6d7c09d8 github.com/stretchr/testify v1.9.0 github.com/vishvananda/netlink v1.1.0 diff --git a/x/go.sum b/x/go.sum index fe9203a3..1d0208da 100644 --- a/x/go.sum +++ b/x/go.sum @@ -18,8 +18,8 @@ github.com/Psiphon-Labs/goptlib v0.0.0-20200406165125-c0e32a7a3464 h1:VmnMMMheFX github.com/Psiphon-Labs/goptlib v0.0.0-20200406165125-c0e32a7a3464/go.mod h1:Pe5BqN2DdIdChorAXl6bDaQd/wghpCleJfid2NoSli0= github.com/Psiphon-Labs/psiphon-tls v0.0.0-20240424193802-52b2602ec60c h1:+SEszyxW7yu+smufzSlAszj/WmOYJ054DJjb5jllulc= github.com/Psiphon-Labs/psiphon-tls v0.0.0-20240424193802-52b2602ec60c/go.mod h1:AaKKoshr8RI1LZTheeNDtNuZ39qNVPWVK4uir2c2XIs= -github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240522172529-8fcc4b9a51cf h1:qXrGUIY9MMXIWqOmWv84qjVa8XLLjcOb+S5TEpZjFpA= -github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240522172529-8fcc4b9a51cf/go.mod h1:Z5txHi6IF67uDg206QnSxkgE1I3FJUDDJ3n0pa+bKRs= +github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0 h1:zAAZmBGHfH2BfRI5UlSJqIKs8xwE/afddEhfg3AVVu8= +github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0/go.mod h1:Z5txHi6IF67uDg206QnSxkgE1I3FJUDDJ3n0pa+bKRs= github.com/Psiphon-Labs/quic-go v0.0.0-20240424181006-45545f5e1536 h1:pM5ex1QufkHV8lDR6Tc1Crk1bW5lYZjrFIJGZNBWE9k= github.com/Psiphon-Labs/quic-go v0.0.0-20240424181006-45545f5e1536/go.mod h1:2MTiPsgoOqWs3Bo6Xr3ElMBX6zzfjd3YkDFpQJLwHdQ= github.com/andybalholm/brotli v1.0.5 h1:8uQZIdzKmjc/iuPu7O2ioW48L81FgatrcpfFmiq/cCs= diff --git a/x/psiphon/psiphon.go b/x/psiphon/psiphon.go index 31064699..bd6fe4fd 100644 --- a/x/psiphon/psiphon.go +++ b/x/psiphon/psiphon.go @@ -18,25 +18,23 @@ import ( "context" "encoding/json" "errors" - "fmt" - "io" "net" + "runtime" + "strings" "sync" + "unicode" "github.com/Jigsaw-Code/outline-sdk/transport" "github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" - psi "github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon" ) // The single [Dialer] we can have. -var singletonDialer = Dialer{ - setNoticeWriter: psi.SetNoticeWriter, -} +var singletonDialer Dialer var ( errNotStartedDial = errors.New("dialer has not been started yet") errNotStartedStop = errors.New("tried to stop dialer that is not running") - errTunnelTimeout = errors.New("tunnel establishment timed out") + errAlreadyStarted = errors.New("dialer has already started") ) // DialerConfig specifies the parameters for [Dialer]. @@ -62,11 +60,7 @@ type Dialer struct { // Controls the Dialer state and Psiphon's global state. mu sync.Mutex // Used by DialStream. - controller *psi.Controller - // Used by Stop. - stop func() - // Allows for overriding the global notice writer for testing. - setNoticeWriter func(io.Writer) + tunnel *clientlib.PsiphonTunnel } var _ transport.StreamDialer = (*Dialer)(nil) @@ -76,166 +70,73 @@ var _ transport.StreamDialer = (*Dialer)(nil) // you will need to add it independently. func (d *Dialer) DialStream(unusedContext context.Context, addr string) (transport.StreamConn, error) { d.mu.Lock() - controller := d.controller + tunnel := d.tunnel d.mu.Unlock() - if controller == nil { + if tunnel == nil { return nil, errNotStartedDial } - netConn, err := controller.Dial(addr, nil) + netConn, err := tunnel.Dial(addr) if err != nil { return nil, err } return streamConn{netConn}, nil } -func newPsiphonConfig(config *DialerConfig) (*psi.Config, error) { - if config == nil { - return nil, errors.New("config must not be nil") - } - // Validate keys. We parse as a map first because we need to check for the existence - // of certain keys. - var configMap map[string]interface{} - if err := json.Unmarshal(config.ProviderConfig, &configMap); err != nil { - return nil, fmt.Errorf("failed to parse config: %w", err) - } - for key, value := range configMap { - switch key { - case "DisableLocalHTTPProxy", "DisableLocalSocksProxy": - b, ok := value.(bool) - if !ok { - return nil, fmt.Errorf("field %v must be a boolean", key) - } - if b != true { - return nil, fmt.Errorf("field %v must be true if set", key) - } - case "DataRootDirectory": - return nil, errors.New("field DataRootDirectory must not be set in the provider config. Specify it in the DialerConfig instead.") - } - } - - // Parse provider config. - pConfig, err := psi.LoadConfig(config.ProviderConfig) - if err != nil { - return nil, fmt.Errorf("config load failed: %w", err) +func getClientPlatform() string { + clientPlatformAllowChars := func(r rune) bool { + return !unicode.IsSpace(r) && r != '_' } - - // Force some Psiphon config defaults for the Outline SDK case. - pConfig.DisableLocalHTTPProxy = true - pConfig.DisableLocalSocksProxy = true - pConfig.DataRootDirectory = config.DataRootDirectory - - return pConfig, nil + goos := strings.Join(strings.FieldsFunc(runtime.GOOS, clientPlatformAllowChars), "-") + goarch := strings.Join(strings.FieldsFunc(runtime.GOARCH, clientPlatformAllowChars), "-") + return "outline-sdk_" + goos + "_" + goarch } +// Allows for overriding in tests. +var startTunnel = clientlib.StartTunnel + // Start configures and runs the Dialer. It must be called before you can use the Dialer. It returns when the tunnel is ready. func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { - pConfig, err := newPsiphonConfig(config) - if err != nil { - return err - } - - // Will receive a value if an error occurs during the connection sequence. - // It will be closed on succesful connection. - errCh := make(chan error) - - // Start returns either when a tunnel is ready, or an error happens, whichever comes first. - // When emitting the errors, we use a select statement to ensure the channel is being listened - // on, to avoid a deadlock after the initial error. - go func() { - onTunnel := func() { - select { - case errCh <- nil: - default: - } - } - err := d.runController(ctx, pConfig, onTunnel) - select { - case errCh <- err: - default: - } - }() - - // Wait for an active tunnel or error - return <-errCh -} - -func (d *Dialer) runController(ctx context.Context, pConfig *psi.Config, onTunnel func()) error { d.mu.Lock() defer d.mu.Unlock() - if d.stop != nil { - return errors.New("tried to start dialer that is alread running") - } - ctx, cancel := context.WithCancelCause(ctx) - defer cancel(context.Canceled) - controllerDone := make(chan struct{}) - defer close(controllerDone) - d.stop = func() { - // Tell controller to stop. - cancel(context.Canceled) - // Wait for controller to return. - <-controllerDone - } - // Set up NoticeWriter to receive events. - d.setNoticeWriter(psi.NewNoticeReceiver( - func(notice []byte) { - var event clientlib.NoticeEvent - err := json.Unmarshal(notice, &event) - if err != nil { - // This is unexpected and probably indicates something fatal has occurred. - // We'll interpret it as a connection error and abort. - cancel(fmt.Errorf("failed to unmarshal notice JSON: %w", err)) - return - } - switch event.Type { - case "EstablishTunnelTimeout": - cancel(errTunnelTimeout) - case "Tunnels": - count := event.Data["count"].(float64) - if count > 0 { - onTunnel() - } - } - })) - defer psi.SetNoticeWriter(io.Discard) - - err := pConfig.Commit(true) - if err != nil { - return fmt.Errorf("failed to commit config: %w", err) + if d.tunnel != nil { + return errAlreadyStarted } - err = psi.OpenDataStore(&psi.Config{DataRootDirectory: pConfig.DataRootDirectory}) - if err != nil { - return fmt.Errorf("failed to open data store: %w", err) + trueValue := true + clientPlatform := getClientPlatform() + // Note that these parameters override anything in the provider config. + params := clientlib.Parameters{ + DataRootDirectory: &config.DataRootDirectory, + ClientPlatform: &clientPlatform, + // Disable Psiphon's local proxy servers, which we don't use. + DisableLocalSocksProxy: &trueValue, + DisableLocalHTTPProxy: &trueValue, } - defer psi.CloseDataStore() - controller, err := psi.NewController(pConfig) + tunnel, err := startTunnel(ctx, config.ProviderConfig, "", params, nil, nil) if err != nil { - return fmt.Errorf("failed to create Controller: %w", err) + if ctx.Err() != nil { + return context.Cause(ctx) + } + return err } - d.controller = controller - d.mu.Unlock() + d.tunnel = tunnel - controller.Run(ctx) - - d.mu.Lock() - d.controller = nil - d.stop = nil - return context.Cause(ctx) + return nil } // Stop stops the Dialer background processes, releasing resources and allowing it to be reconfigured. // It returns when the Dialer is completely stopped. func (d *Dialer) Stop() error { d.mu.Lock() - stop := d.stop - d.stop = nil + tunnel := d.tunnel + d.tunnel = nil d.mu.Unlock() - if stop == nil { + if tunnel == nil { return errNotStartedStop } - stop() + tunnel.Stop() return nil } diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index 7f8bbd55..432c0a4d 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -19,13 +19,11 @@ package psiphon import ( "context" "encoding/json" - "io" "os" "testing" "time" - psi "github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon" - + "github.com/Psiphon-Labs/psiphon-tunnel-core/ClientLibrary/clientlib" "github.com/stretchr/testify/require" ) @@ -41,76 +39,27 @@ func newTestConfig(tb testing.TB) (*DialerConfig, func()) { }, func() { os.RemoveAll(tempDir) } } -func TestNewPsiphonConfig_ParseCorrectly(t *testing.T) { - config, err := newPsiphonConfig(&DialerConfig{ - ProviderConfig: json.RawMessage(`{ - "PropagationChannelId": "ID1", - "SponsorId": "ID2" - }`), - }) - require.NoError(t, err) - require.Equal(t, "ID1", config.PropagationChannelId) - require.Equal(t, "ID2", config.SponsorId) -} - -func TestNewPsiphonConfig_AcceptOkOptions(t *testing.T) { - _, err := newPsiphonConfig(&DialerConfig{ - ProviderConfig: json.RawMessage(`{ - "DisableLocalHTTPProxy": true, - "DisableLocalSocksProxy": true - }`)}) - require.NoError(t, err) -} - -func TestNewPsiphonConfig_RejectBadOptions(t *testing.T) { - _, err := newPsiphonConfig(&DialerConfig{ - ProviderConfig: json.RawMessage(`{"DisableLocalHTTPProxy": false}`)}) - require.Error(t, err) - - _, err = newPsiphonConfig(&DialerConfig{ - ProviderConfig: json.RawMessage(`{"DisableLocalSocksProxy": false}`)}) - require.Error(t, err) - require.Error(t, err) -} - -func TestDialer_StartSuccessful(t *testing.T) { - // Create minimal config. +func TestDialer_Start_Successful(t *testing.T) { cfg, delete := newTestConfig(t) defer delete() - // Intercept notice writer. dialer := GetSingletonDialer() - wCh := make(chan io.Writer) - dialer.setNoticeWriter = func(w io.Writer) { - wCh <- w + startTunnel = func(ctx context.Context, configJSON []byte, embeddedServerEntryList string, params clientlib.Parameters, paramsDelta clientlib.ParametersDelta, noticeReceiver func(clientlib.NoticeEvent)) (retTunnel *clientlib.PsiphonTunnel, retErr error) { + return &clientlib.PsiphonTunnel{}, nil } defer func() { - dialer.setNoticeWriter = psi.SetNoticeWriter + startTunnel = clientlib.StartTunnel }() - errCh := make(chan error) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - go func() { - errCh <- dialer.Start(ctx, cfg) - }() - - // We use a select because the error may happen before the notice writer is set. - select { - case w := <-wCh: - // Notify fake tunnel establishment once we have the notice writer. - psi.SetNoticeWriter(w) - psi.NoticeTunnels(1) - case err := <-errCh: - t.Fatalf("Got error from Start: %v", err) - } - - err := <-errCh - require.NoError(t, err) + require.NoError(t, dialer.Start(ctx, cfg)) + require.ErrorIs(t, dialer.Start(ctx, cfg), errAlreadyStarted) require.NoError(t, dialer.Stop()) + require.ErrorIs(t, dialer.Stop(), errNotStartedStop) } -func TestDialerStart_Cancelled(t *testing.T) { +func TestDialer_Start_Cancelled(t *testing.T) { cfg, delete := newTestConfig(t) defer delete() errCh := make(chan error) @@ -123,7 +72,7 @@ func TestDialerStart_Cancelled(t *testing.T) { require.ErrorIs(t, err, context.Canceled) } -func TestDialerStart_Timeout(t *testing.T) { +func TestDialer_Start_Timeout(t *testing.T) { cfg, delete := newTestConfig(t) defer delete() errCh := make(chan error) @@ -136,12 +85,12 @@ func TestDialerStart_Timeout(t *testing.T) { require.ErrorIs(t, err, context.DeadlineExceeded) } -func TestDialerDialStream_NotStarted(t *testing.T) { +func TestDialer_DialStream_NotStarted(t *testing.T) { _, err := GetSingletonDialer().DialStream(context.Background(), "") require.ErrorIs(t, err, errNotStartedDial) } -func TestDialerStop_NotStarted(t *testing.T) { +func TestDialer_Stop_NotStarted(t *testing.T) { err := GetSingletonDialer().Stop() require.ErrorIs(t, err, errNotStartedStop) } From 3f322612f92fb96b409ba4125707f794a7a8178c Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Mon, 17 Jun 2024 21:40:59 -0400 Subject: [PATCH 2/8] Improve test --- x/psiphon/psiphon.go | 37 +++++++++++++++++-------- x/psiphon/psiphon_test.go | 58 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/x/psiphon/psiphon.go b/x/psiphon/psiphon.go index bd6fe4fd..aa71c3be 100644 --- a/x/psiphon/psiphon.go +++ b/x/psiphon/psiphon.go @@ -60,7 +60,12 @@ type Dialer struct { // Controls the Dialer state and Psiphon's global state. mu sync.Mutex // Used by DialStream. - tunnel *clientlib.PsiphonTunnel + tunnel psiphonTunnel +} + +type psiphonTunnel interface { + Dial(remoteAddr string) (net.Conn, error) + Stop() } var _ transport.StreamDialer = (*Dialer)(nil) @@ -92,20 +97,16 @@ func getClientPlatform() string { } // Allows for overriding in tests. -var startTunnel = clientlib.StartTunnel - -// Start configures and runs the Dialer. It must be called before you can use the Dialer. It returns when the tunnel is ready. -func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { - d.mu.Lock() - defer d.mu.Unlock() +var startTunnel func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) = psiphonStartTunnel - if d.tunnel != nil { - return errAlreadyStarted +func psiphonStartTunnel(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + if config == nil { + return nil, errors.New("config must not be nil") } - trueValue := true - clientPlatform := getClientPlatform() // Note that these parameters override anything in the provider config. + clientPlatform := getClientPlatform() + trueValue := true params := clientlib.Parameters{ DataRootDirectory: &config.DataRootDirectory, ClientPlatform: &clientPlatform, @@ -114,7 +115,19 @@ func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { DisableLocalHTTPProxy: &trueValue, } - tunnel, err := startTunnel(ctx, config.ProviderConfig, "", params, nil, nil) + return clientlib.StartTunnel(ctx, config.ProviderConfig, "", params, nil, nil) +} + +// Start configures and runs the Dialer. It must be called before you can use the Dialer. It returns when the tunnel is ready. +func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { + d.mu.Lock() + defer d.mu.Unlock() + + if d.tunnel != nil { + return errAlreadyStarted + } + + tunnel, err := startTunnel(ctx, config) if err != nil { if ctx.Err() != nil { return context.Cause(ctx) diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index 432c0a4d..9c0e49aa 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -19,6 +19,7 @@ package psiphon import ( "context" "encoding/json" + "net" "os" "testing" "time" @@ -44,19 +45,27 @@ func TestDialer_Start_Successful(t *testing.T) { defer delete() dialer := GetSingletonDialer() - startTunnel = func(ctx context.Context, configJSON []byte, embeddedServerEntryList string, params clientlib.Parameters, paramsDelta clientlib.ParametersDelta, noticeReceiver func(clientlib.NoticeEvent)) (retTunnel *clientlib.PsiphonTunnel, retErr error) { + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { return &clientlib.PsiphonTunnel{}, nil } defer func() { - startTunnel = clientlib.StartTunnel + startTunnel = psiphonStartTunnel }() ctx, cancel := context.WithCancel(context.Background()) defer cancel() require.NoError(t, dialer.Start(ctx, cfg)) + require.NotNil(t, dialer.tunnel) require.ErrorIs(t, dialer.Start(ctx, cfg), errAlreadyStarted) require.NoError(t, dialer.Stop()) + require.Nil(t, dialer.tunnel) require.ErrorIs(t, dialer.Stop(), errNotStartedStop) + require.NoError(t, dialer.Start(ctx, cfg)) + require.NoError(t, dialer.Stop()) +} + +func TestDialer_Start_NilConfig(t *testing.T) { + require.Error(t, GetSingletonDialer().Start(context.Background(), nil)) } func TestDialer_Start_Cancelled(t *testing.T) { @@ -85,9 +94,50 @@ func TestDialer_Start_Timeout(t *testing.T) { require.ErrorIs(t, err, context.DeadlineExceeded) } -func TestDialer_DialStream_NotStarted(t *testing.T) { - _, err := GetSingletonDialer().DialStream(context.Background(), "") +type noopTunnel struct { + stopped bool +} + +func (t *noopTunnel) Dial(addr string) (net.Conn, error) { + return nil, nil +} + +func (t *noopTunnel) Stop() { + t.stopped = true +} + +func TestDialer_DialStream(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + dialer := GetSingletonDialer() + + // Dial before Start. + _, err := dialer.DialStream(ctx, "") require.ErrorIs(t, err, errNotStartedDial) + + var tunnel noopTunnel + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + tunnel.stopped = false + return &tunnel, nil + } + defer func() { + startTunnel = psiphonStartTunnel + }() + // Make sure it works on restarts. + for i := 0; i < 2; i++ { + // Dial after Start. + require.NoError(t, dialer.Start(ctx, nil)) + require.False(t, tunnel.stopped) + _, err = dialer.DialStream(ctx, "") + require.NoError(t, err) + + // Dial after Stop. + require.NoError(t, dialer.Stop()) + require.True(t, tunnel.stopped) + _, err = dialer.DialStream(nil, "") + require.ErrorIs(t, err, errNotStartedDial) + } } func TestDialer_Stop_NotStarted(t *testing.T) { From 96a290a5fc0d08833c0e22cc5f2013064ba804a5 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Mon, 17 Jun 2024 21:49:11 -0400 Subject: [PATCH 3/8] More tests --- x/psiphon/psiphon_test.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index 9c0e49aa..c29fd0b9 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -19,6 +19,7 @@ package psiphon import ( "context" "encoding/json" + "errors" "net" "os" "testing" @@ -94,15 +95,16 @@ func TestDialer_Start_Timeout(t *testing.T) { require.ErrorIs(t, err, context.DeadlineExceeded) } -type noopTunnel struct { +type errorTunnel struct { + err error stopped bool } -func (t *noopTunnel) Dial(addr string) (net.Conn, error) { - return nil, nil +func (t *errorTunnel) Dial(addr string) (net.Conn, error) { + return nil, t.err } -func (t *noopTunnel) Stop() { +func (t *errorTunnel) Stop() { t.stopped = true } @@ -116,7 +118,7 @@ func TestDialer_DialStream(t *testing.T) { _, err := dialer.DialStream(ctx, "") require.ErrorIs(t, err, errNotStartedDial) - var tunnel noopTunnel + var tunnel errorTunnel startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { tunnel.stopped = false return &tunnel, nil @@ -140,6 +142,28 @@ func TestDialer_DialStream(t *testing.T) { } } +func TestDialer_DialStream_Error(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + dialer := GetSingletonDialer() + tunnel := errorTunnel{ + err: errors.New("failed to dial"), + } + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + tunnel.stopped = false + return &tunnel, nil + } + defer func() { + startTunnel = psiphonStartTunnel + }() + require.NoError(t, dialer.Start(ctx, nil)) + require.False(t, tunnel.stopped) + _, err := dialer.DialStream(ctx, "") + require.Equal(t, tunnel.err, err) + require.NoError(t, dialer.Stop()) +} + func TestDialer_Stop_NotStarted(t *testing.T) { err := GetSingletonDialer().Stop() require.ErrorIs(t, err, errNotStartedStop) From fa6713deb2c906688c2e096316c3ef6be10d6a50 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Tue, 18 Jun 2024 01:03:14 -0400 Subject: [PATCH 4/8] Tweaks --- x/psiphon/psiphon.go | 69 +++++++++++++++++++++++++++++---------- x/psiphon/psiphon_test.go | 32 ++++++++++++++---- 2 files changed, 78 insertions(+), 23 deletions(-) diff --git a/x/psiphon/psiphon.go b/x/psiphon/psiphon.go index aa71c3be..021d68ab 100644 --- a/x/psiphon/psiphon.go +++ b/x/psiphon/psiphon.go @@ -61,6 +61,8 @@ type Dialer struct { mu sync.Mutex // Used by DialStream. tunnel psiphonTunnel + // Used by Stop. + stop func() } type psiphonTunnel interface { @@ -120,36 +122,69 @@ func psiphonStartTunnel(ctx context.Context, config *DialerConfig) (psiphonTunne // Start configures and runs the Dialer. It must be called before you can use the Dialer. It returns when the tunnel is ready. func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { - d.mu.Lock() - defer d.mu.Unlock() + resultCh := make(chan error) + go func() { + d.mu.Lock() + defer d.mu.Unlock() + + if d.stop != nil { + resultCh <- errAlreadyStarted + return + } - if d.tunnel != nil { - return errAlreadyStarted - } + ctx, cancel := context.WithCancel(ctx) + defer cancel() + tunnelDone := make(chan struct{}) + defer close(tunnelDone) + d.stop = func() { + // Tell start to stop. + cancel() + // Wait for tunnel to be done. + <-tunnelDone + } + defer func() { + // Cleanup. + d.stop = nil + }() + + d.mu.Unlock() + + tunnel, err := startTunnel(ctx, config) + + d.mu.Lock() - tunnel, err := startTunnel(ctx, config) - if err != nil { if ctx.Err() != nil { - return context.Cause(ctx) + err = context.Cause(ctx) } - return err - } - d.tunnel = tunnel - - return nil + resultCh <- err + if err != nil { + return + } + d.tunnel = tunnel + defer func() { + d.tunnel = nil + tunnel.Stop() + }() + + d.mu.Unlock() + // wait for Stop + <-ctx.Done() + d.mu.Lock() + }() + return <-resultCh } // Stop stops the Dialer background processes, releasing resources and allowing it to be reconfigured. // It returns when the Dialer is completely stopped. func (d *Dialer) Stop() error { d.mu.Lock() - tunnel := d.tunnel - d.tunnel = nil + stop := d.stop d.mu.Unlock() - if tunnel == nil { + + if stop == nil { return errNotStartedStop } - tunnel.Stop() + stop() return nil } diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index c29fd0b9..dea132d0 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -42,9 +42,6 @@ func newTestConfig(tb testing.TB) (*DialerConfig, func()) { } func TestDialer_Start_Successful(t *testing.T) { - cfg, delete := newTestConfig(t) - defer delete() - dialer := GetSingletonDialer() startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { return &clientlib.PsiphonTunnel{}, nil @@ -55,14 +52,37 @@ func TestDialer_Start_Successful(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - require.NoError(t, dialer.Start(ctx, cfg)) + require.NoError(t, dialer.Start(ctx, nil)) require.NotNil(t, dialer.tunnel) - require.ErrorIs(t, dialer.Start(ctx, cfg), errAlreadyStarted) + require.ErrorIs(t, dialer.Start(ctx, nil), errAlreadyStarted) require.NoError(t, dialer.Stop()) require.Nil(t, dialer.tunnel) require.ErrorIs(t, dialer.Stop(), errNotStartedStop) - require.NoError(t, dialer.Start(ctx, cfg)) + require.NoError(t, dialer.Start(ctx, nil)) + require.NoError(t, dialer.Stop()) +} + +func TestDialer_StopOnStart(t *testing.T) { + dialer := GetSingletonDialer() + startCalled := make(chan struct{}) + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + startCalled <- struct{}{} + select { + case <-ctx.Done(): + return nil, context.Cause(ctx) + } + } + defer func() { + startTunnel = psiphonStartTunnel + }() + + resultCh := make(chan error) + go func() { + resultCh <- dialer.Start(context.Background(), nil) + }() + <-startCalled require.NoError(t, dialer.Stop()) + require.Error(t, <-resultCh) } func TestDialer_Start_NilConfig(t *testing.T) { From f9114f73f32200b1dd04d0f17f38fa06942bde35 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Tue, 18 Jun 2024 01:11:14 -0400 Subject: [PATCH 5/8] 100% coverage --- x/psiphon/psiphon_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index dea132d0..bd68ba88 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -151,8 +151,10 @@ func TestDialer_DialStream(t *testing.T) { // Dial after Start. require.NoError(t, dialer.Start(ctx, nil)) require.False(t, tunnel.stopped) - _, err = dialer.DialStream(ctx, "") + conn, err := dialer.DialStream(ctx, "") require.NoError(t, err) + require.NoError(t, conn.CloseRead()) + require.NoError(t, conn.CloseWrite()) // Dial after Stop. require.NoError(t, dialer.Stop()) From e1c1767b031dcdbf3b9bf06151f9df79e662a65e Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Tue, 18 Jun 2024 01:18:43 -0400 Subject: [PATCH 6/8] More test --- x/psiphon/psiphon_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/x/psiphon/psiphon_test.go b/x/psiphon/psiphon_test.go index bd68ba88..98b1bf05 100644 --- a/x/psiphon/psiphon_test.go +++ b/x/psiphon/psiphon_test.go @@ -85,6 +85,33 @@ func TestDialer_StopOnStart(t *testing.T) { require.Error(t, <-resultCh) } +func TestDialer_StartOnStart(t *testing.T) { + dialer := GetSingletonDialer() + startCalled := make(chan struct{}) + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + startCalled <- struct{}{} + select { + case <-ctx.Done(): + return nil, context.Cause(ctx) + } + } + defer func() { + startTunnel = psiphonStartTunnel + }() + + resultCh := make(chan error) + go func() { + resultCh <- dialer.Start(context.Background(), nil) + }() + <-startCalled + startTunnel = func(ctx context.Context, config *DialerConfig) (psiphonTunnel, error) { + return nil, errors.New("failed to start") + } + require.ErrorIs(t, dialer.Start(context.Background(), nil), errAlreadyStarted) + require.NoError(t, dialer.Stop()) + require.Error(t, <-resultCh) +} + func TestDialer_Start_NilConfig(t *testing.T) { require.Error(t, GetSingletonDialer().Start(context.Background(), nil)) } From a76b171e443f5770c3441284701da54b307a6319 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Tue, 18 Jun 2024 01:32:36 -0400 Subject: [PATCH 7/8] Fix race --- x/psiphon/psiphon.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x/psiphon/psiphon.go b/x/psiphon/psiphon.go index 021d68ab..ad35c7f9 100644 --- a/x/psiphon/psiphon.go +++ b/x/psiphon/psiphon.go @@ -156,8 +156,8 @@ func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { if ctx.Err() != nil { err = context.Cause(ctx) } - resultCh <- err if err != nil { + resultCh <- err return } d.tunnel = tunnel @@ -165,6 +165,7 @@ func (d *Dialer) Start(ctx context.Context, config *DialerConfig) error { d.tunnel = nil tunnel.Stop() }() + resultCh <- nil d.mu.Unlock() // wait for Stop From ccae237ec9a6a17935f6b023a8a70404333fe4a5 Mon Sep 17 00:00:00 2001 From: Vinicius Fortuna Date: Mon, 24 Jun 2024 23:04:25 +0300 Subject: [PATCH 8/8] Update Psiphon dependency --- x/go.mod | 2 +- x/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x/go.mod b/x/go.mod index df1b2ca4..e9c14043 100644 --- a/x/go.mod +++ b/x/go.mod @@ -6,7 +6,7 @@ require ( github.com/Jigsaw-Code/outline-sdk v0.0.16 // Use github.com/Psiphon-Labs/psiphon-tunnel-core@staging-client as per // https://github.com/Psiphon-Labs/psiphon-tunnel-core/?tab=readme-ov-file#using-psiphon-with-go-modules - github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0 + github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240619172145-03cade11f647 github.com/songgao/water v0.0.0-20200317203138-2b4b6d7c09d8 github.com/stretchr/testify v1.9.0 github.com/vishvananda/netlink v1.1.0 diff --git a/x/go.sum b/x/go.sum index 1d0208da..a8dcfab0 100644 --- a/x/go.sum +++ b/x/go.sum @@ -18,8 +18,8 @@ github.com/Psiphon-Labs/goptlib v0.0.0-20200406165125-c0e32a7a3464 h1:VmnMMMheFX github.com/Psiphon-Labs/goptlib v0.0.0-20200406165125-c0e32a7a3464/go.mod h1:Pe5BqN2DdIdChorAXl6bDaQd/wghpCleJfid2NoSli0= github.com/Psiphon-Labs/psiphon-tls v0.0.0-20240424193802-52b2602ec60c h1:+SEszyxW7yu+smufzSlAszj/WmOYJ054DJjb5jllulc= github.com/Psiphon-Labs/psiphon-tls v0.0.0-20240424193802-52b2602ec60c/go.mod h1:AaKKoshr8RI1LZTheeNDtNuZ39qNVPWVK4uir2c2XIs= -github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0 h1:zAAZmBGHfH2BfRI5UlSJqIKs8xwE/afddEhfg3AVVu8= -github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240614155336-be2427e798f0/go.mod h1:Z5txHi6IF67uDg206QnSxkgE1I3FJUDDJ3n0pa+bKRs= +github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240619172145-03cade11f647 h1:YhpvDo++9Q3FiBuaAUhrFEzEWC6es3zFohjofEwO6xg= +github.com/Psiphon-Labs/psiphon-tunnel-core v1.0.11-0.20240619172145-03cade11f647/go.mod h1:Z5txHi6IF67uDg206QnSxkgE1I3FJUDDJ3n0pa+bKRs= github.com/Psiphon-Labs/quic-go v0.0.0-20240424181006-45545f5e1536 h1:pM5ex1QufkHV8lDR6Tc1Crk1bW5lYZjrFIJGZNBWE9k= github.com/Psiphon-Labs/quic-go v0.0.0-20240424181006-45545f5e1536/go.mod h1:2MTiPsgoOqWs3Bo6Xr3ElMBX6zzfjd3YkDFpQJLwHdQ= github.com/andybalholm/brotli v1.0.5 h1:8uQZIdzKmjc/iuPu7O2ioW48L81FgatrcpfFmiq/cCs=