From 94a4067eb2b679dba87fe37b376374d3cf8fe5b1 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 10:12:29 +0200 Subject: [PATCH 01/13] feat: block file transfers --- agent/agent.go | 4 ++++ agent/agent_test.go | 45 ++++++++++++++++++++++++++++++++++++++ agent/agentssh/agentssh.go | 41 ++++++++++++++++++++++++++++++++++ cli/agent.go | 11 +++++++++- 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/agent/agent.go b/agent/agent.go index c7a785f8d5da1..5512f04db28ea 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -91,6 +91,7 @@ type Options struct { ModifiedProcesses chan []*agentproc.Process // ProcessManagementTick is used for testing process priority management. ProcessManagementTick <-chan time.Time + BlockFileTransfer bool } type Client interface { @@ -184,6 +185,7 @@ func New(options Options) Agent { modifiedProcs: options.ModifiedProcesses, processManagementTick: options.ProcessManagementTick, logSender: agentsdk.NewLogSender(options.Logger), + blockFileTransfer: options.BlockFileTransfer, prometheusRegistry: prometheusRegistry, metrics: newAgentMetrics(prometheusRegistry), @@ -239,6 +241,7 @@ type agent struct { sessionToken atomic.Pointer[string] sshServer *agentssh.Server sshMaxTimeout time.Duration + blockFileTransfer bool lifecycleUpdate chan struct{} lifecycleReported chan codersdk.WorkspaceAgentLifecycle @@ -277,6 +280,7 @@ func (a *agent) init() { AnnouncementBanners: func() *[]codersdk.BannerConfig { return a.announcementBanners.Load() }, UpdateEnv: a.updateCommandEnv, WorkingDirectory: func() string { return a.manifest.Load().Directory }, + BlockFileTransfer: a.blockFileTransfer, }) if err != nil { panic(err) diff --git a/agent/agent_test.go b/agent/agent_test.go index a008a60a2362e..cca50366f8ab4 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -970,6 +970,51 @@ func TestAgent_SCP(t *testing.T) { require.NoError(t, err) } +func TestAgent_FileTransferBlocked(t *testing.T) { + t.Parallel() + + content := "hello world" + + t.Run("SCP", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + //nolint:dogsled + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { + o.BlockFileTransfer = true + }) + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshClient.Close() + scpClient, err := scp.NewClientBySSH(sshClient) + require.NoError(t, err) + defer scpClient.Close() + tempFile := filepath.Join(t.TempDir(), "scp") + err = scpClient.CopyFile(context.Background(), strings.NewReader(content), tempFile, "0755") + require.Error(t, err) + require.Contains(t, err.Error(), agentssh.BlockedFileTransferErrorMessage) + }) + + t.Run("SFTP", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + //nolint:dogsled + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { + o.BlockFileTransfer = true + }) + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshClient.Close() + _, err = sftp.NewClient(sshClient) + require.NoError(t, err) + }) +} + func TestAgent_EnvironmentVariables(t *testing.T) { t.Parallel() key := "EXAMPLE" diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 54e5a3f41223e..deb7a0a849966 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -52,6 +52,11 @@ const ( // MagicProcessCmdlineJetBrains is a string in a process's command line that // uniquely identifies it as JetBrains software. MagicProcessCmdlineJetBrains = "idea.vendor.name=JetBrains" + + // BlockedFileTransferErrorCode indicates that SSH server restricted the raw command from performing + // the file transfer. + BlockedFileTransferErrorCode = 2 + BlockedFileTransferErrorMessage = "File transfer has been disabled." ) // Config sets configuration parameters for the agent SSH server. @@ -74,6 +79,8 @@ type Config struct { // X11SocketDir is the directory where X11 sockets are created. Default is // /tmp/.X11-unix. X11SocketDir string + // BlockFileTransfer restricts use of file transfer applications. + BlockFileTransfer bool } type Server struct { @@ -272,6 +279,14 @@ func (s *Server) sessionHandler(session ssh.Session) { extraEnv = append(extraEnv, fmt.Sprintf("DISPLAY=:%d.0", x11.ScreenNumber)) } + if s.fileTransferBlocked(session) { + // Response format: \n + errorMessage := fmt.Sprintf("\x02%s\n", BlockedFileTransferErrorMessage) + _, _ = session.Write([]byte(errorMessage)) + _ = session.Exit(BlockedFileTransferErrorCode) + return + } + switch ss := session.Subsystem(); ss { case "": case "sftp": @@ -322,6 +337,32 @@ func (s *Server) sessionHandler(session ssh.Session) { _ = session.Exit(0) } +func (s *Server) fileTransferBlocked(session ssh.Session) bool { + if !s.config.BlockFileTransfer { + return false // file transfers are permitted + } + // File transfers are restricted. + + if session.Subsystem() == "sftp" { + return true // sftp mode is forbidden + } + + cmd := session.Command() + if len(cmd) == 0 { + return false // no command? + } + + c := cmd[0] + c = filepath.Base(c) // in case the binary is absolute path, /usr/sbin/scp + + switch c { + case "nc", "rsync", "scp", "sftp": + return true // forbidden command + default: + return false + } +} + func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, extraEnv []string) (retErr error) { ctx := session.Context() env := append(session.Environ(), extraEnv...) diff --git a/cli/agent.go b/cli/agent.go index 1f91f1c98bb8d..0eb4d1b7a8db0 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -48,6 +48,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { slogHumanPath string slogJSONPath string slogStackdriverPath string + blockFileTransfer bool ) cmd := &serpent.Command{ Use: "agent", @@ -314,6 +315,8 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { // Intentionally set this to nil. It's mainly used // for testing. ModifiedProcesses: nil, + + BlockFileTransfer: blockFileTransfer, }) promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger) @@ -412,11 +415,17 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { { Name: "Stackdriver Log Location", Description: "Output Stackdriver compatible logs to a given file.", - Flag: "log-stackdriver", Env: "CODER_AGENT_LOGGING_STACKDRIVER", Default: "", Value: serpent.StringOf(&slogStackdriverPath), }, + { + Flag: "block-file-transfer", + Default: "false", + Env: "CODER_BLOCK_FILE_TRANSFER", + Description: "Block file transfer using known applications: nc, rsync, scp, sftp", + Value: serpent.BoolOf(&blockFileTransfer), + }, } return cmd From 9915cdea1bcfcdd8a0e0bbc1bbb8937a1a15f647 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 10:54:59 +0200 Subject: [PATCH 02/13] commands --- agent/agent_test.go | 53 ++++++++++++++++++++++++++------------ agent/agentssh/agentssh.go | 9 ++++--- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index cca50366f8ab4..7cccccf8ee5eb 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -973,16 +973,14 @@ func TestAgent_SCP(t *testing.T) { func TestAgent_FileTransferBlocked(t *testing.T) { t.Parallel() - content := "hello world" - - t.Run("SCP", func(t *testing.T) { + t.Run("SCP with go-scp package", func(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() //nolint:dogsled - conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) { o.BlockFileTransfer = true }) sshClient, err := conn.SSHClient(ctx) @@ -992,26 +990,47 @@ func TestAgent_FileTransferBlocked(t *testing.T) { require.NoError(t, err) defer scpClient.Close() tempFile := filepath.Join(t.TempDir(), "scp") - err = scpClient.CopyFile(context.Background(), strings.NewReader(content), tempFile, "0755") + err = scpClient.CopyFile(context.Background(), strings.NewReader("hello world"), tempFile, "0755") require.Error(t, err) require.Contains(t, err.Error(), agentssh.BlockedFileTransferErrorMessage) }) - t.Run("SFTP", func(t *testing.T) { + t.Run("Forbidden commands", func(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) - defer cancel() + commands := []string{"nc", "rsync", "scp", "sftp"} + for _, c := range commands { + c := c + t.Run(c, func(t *testing.T) { + t.Parallel() - //nolint:dogsled - conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(c *agenttest.Client, o *agent.Options) { - o.BlockFileTransfer = true - }) - sshClient, err := conn.SSHClient(ctx) - require.NoError(t, err) - defer sshClient.Close() - _, err = sftp.NewClient(sshClient) - require.NoError(t, err) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + //nolint:dogsled + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) { + o.BlockFileTransfer = true + }) + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshClient.Close() + + session, err := sshClient.NewSession() + require.NoError(t, err) + defer session.Close() + + stdout, err := session.StdoutPipe() + require.NoError(t, err) + + err = session.Start(c) + require.NoError(t, err) + defer session.Close() + + errorMessage, err := io.ReadAll(stdout) + require.NoError(t, err) + require.Contains(t, string(errorMessage), agentssh.BlockedFileTransferErrorMessage) + }) + } }) } diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index deb7a0a849966..e9f9583ff1905 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -337,16 +337,17 @@ func (s *Server) sessionHandler(session ssh.Session) { _ = session.Exit(0) } +// fileTransferBlocked method checks if the file transfer commands should be blocked. +// It does not block SFTP sessions, VS Code may still use this protocol. +// +// Warning: consider this mechanism as "Do not trespass" sign. If a user needs a more sophisticated +// and battle-proof solution, consider the full endpoint security. func (s *Server) fileTransferBlocked(session ssh.Session) bool { if !s.config.BlockFileTransfer { return false // file transfers are permitted } // File transfers are restricted. - if session.Subsystem() == "sftp" { - return true // sftp mode is forbidden - } - cmd := session.Command() if len(cmd) == 0 { return false // no command? From abd97df2dfcb5bd27c674791dcdac88f0348bb4b Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 11:09:57 +0200 Subject: [PATCH 03/13] fix --- cli/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/agent.go b/cli/agent.go index 0eb4d1b7a8db0..7f277c636bb19 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -423,7 +423,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { Flag: "block-file-transfer", Default: "false", Env: "CODER_BLOCK_FILE_TRANSFER", - Description: "Block file transfer using known applications: nc, rsync, scp, sftp", + Description: "Block file transfer using known applications: nc, rsync, scp, sftp.", Value: serpent.BoolOf(&blockFileTransfer), }, } From 00e0a8a907e0f60817c35c75cb6c338ad2d85361 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 12:10:20 +0200 Subject: [PATCH 04/13] fix --- cli/agent.go | 1 + cli/testdata/coder_agent_--help.golden | 3 +++ 2 files changed, 4 insertions(+) diff --git a/cli/agent.go b/cli/agent.go index 7f277c636bb19..bb8d028cef391 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -415,6 +415,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { { Name: "Stackdriver Log Location", Description: "Output Stackdriver compatible logs to a given file.", + Flag: "log-stackdriver", Env: "CODER_AGENT_LOGGING_STACKDRIVER", Default: "", Value: serpent.StringOf(&slogStackdriverPath), diff --git a/cli/testdata/coder_agent_--help.golden b/cli/testdata/coder_agent_--help.golden index 372395c4ba5fe..84eaf3129759b 100644 --- a/cli/testdata/coder_agent_--help.golden +++ b/cli/testdata/coder_agent_--help.golden @@ -18,6 +18,9 @@ OPTIONS: --auth string, $CODER_AGENT_AUTH (default: token) Specify the authentication type to use for the agent. + --block-file-transfer bool, $CODER_BLOCK_FILE_TRANSFER (default: false) + Block file transfer using known applications: nc, rsync, scp, sftp. + --debug-address string, $CODER_AGENT_DEBUG_ADDRESS (default: 127.0.0.1:2113) The bind address to serve a debug HTTP server. From fa0010d90508955f48a633385a7b3fc02f77cbeb Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 14:05:05 +0200 Subject: [PATCH 05/13] Fix --- agent/agent_test.go | 27 +++++++++++++++++++++++++-- agent/agentssh/agentssh.go | 18 +++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 7cccccf8ee5eb..9ed9c99a1d4ae 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -973,6 +973,24 @@ func TestAgent_SCP(t *testing.T) { func TestAgent_FileTransferBlocked(t *testing.T) { t.Parallel() + t.Run("SFTP", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + //nolint:dogsled + conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) { + o.BlockFileTransfer = true + }) + sshClient, err := conn.SSHClient(ctx) + require.NoError(t, err) + defer sshClient.Close() + _, err = sftp.NewClient(sshClient) + require.Error(t, err) + require.Contains(t, err.Error(), "unexpected EOF") + }) + t.Run("SCP with go-scp package", func(t *testing.T) { t.Parallel() @@ -1026,9 +1044,14 @@ func TestAgent_FileTransferBlocked(t *testing.T) { require.NoError(t, err) defer session.Close() - errorMessage, err := io.ReadAll(stdout) + msg, err := io.ReadAll(stdout) require.NoError(t, err) - require.Contains(t, string(errorMessage), agentssh.BlockedFileTransferErrorMessage) + errorMessage := string(msg) + + // NOTE: Checking content of the error message is flaky. Sometimes it catches "EOF" or "Process terminate with status code 2". + isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || + strings.Contains(errorMessage, "EOF") + require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) }) } }) diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index e9f9583ff1905..e7b4dff097bb1 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -279,13 +279,19 @@ func (s *Server) sessionHandler(session ssh.Session) { extraEnv = append(extraEnv, fmt.Sprintf("DISPLAY=:%d.0", x11.ScreenNumber)) } + s.logger.Warn(ctx, "fileTransferBlocked", slog.F("session", session)) if s.fileTransferBlocked(session) { - // Response format: \n - errorMessage := fmt.Sprintf("\x02%s\n", BlockedFileTransferErrorMessage) - _, _ = session.Write([]byte(errorMessage)) + s.logger.Warn(ctx, "fileTransferBlocked", slog.F("go ", "yes")) + + if session.Subsystem() == "" { // sftp does not expect error, otherwise it fails with "package too long" + // Response format: \n + errorMessage := fmt.Sprintf("\x02%s\n", BlockedFileTransferErrorMessage) + _, _ = session.Write([]byte(errorMessage)) + } _ = session.Exit(BlockedFileTransferErrorCode) return } + s.logger.Warn(ctx, "fileTransferBlocked end") switch ss := session.Subsystem(); ss { case "": @@ -338,8 +344,6 @@ func (s *Server) sessionHandler(session ssh.Session) { } // fileTransferBlocked method checks if the file transfer commands should be blocked. -// It does not block SFTP sessions, VS Code may still use this protocol. -// // Warning: consider this mechanism as "Do not trespass" sign. If a user needs a more sophisticated // and battle-proof solution, consider the full endpoint security. func (s *Server) fileTransferBlocked(session ssh.Session) bool { @@ -348,6 +352,10 @@ func (s *Server) fileTransferBlocked(session ssh.Session) bool { } // File transfers are restricted. + if session.Subsystem() == "sftp" { + return true + } + cmd := session.Command() if len(cmd) == 0 { return false // no command? From dbaa7637e00c39cf127f71755d1819912479d33b Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 14:07:35 +0200 Subject: [PATCH 06/13] fix --- agent/agent_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 9ed9c99a1d4ae..8dfa26ed79cf5 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1048,9 +1048,10 @@ func TestAgent_FileTransferBlocked(t *testing.T) { require.NoError(t, err) errorMessage := string(msg) - // NOTE: Checking content of the error message is flaky. Sometimes it catches "EOF" or "Process terminate with status code 2". + // NOTE: Checking content of the error message is flaky. It can catch: "File transfer has been disabled", "EOF", or "Process exited with status 2". isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || - strings.Contains(errorMessage, "EOF") + strings.Contains(errorMessage, "EOF") || + strings.Contains(errorMessage, "Process exited with status 2") require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) }) } From 94926bd8d0bb548ccf1b0a4e79cd82a0e60db3ae Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 14:37:58 +0200 Subject: [PATCH 07/13] fix --- agent/agent_test.go | 18 ++++++++++-------- agent/agentssh/agentssh.go | 4 +--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 8dfa26ed79cf5..adcd54210ad4d 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1010,7 +1010,7 @@ func TestAgent_FileTransferBlocked(t *testing.T) { tempFile := filepath.Join(t.TempDir(), "scp") err = scpClient.CopyFile(context.Background(), strings.NewReader("hello world"), tempFile, "0755") require.Error(t, err) - require.Contains(t, err.Error(), agentssh.BlockedFileTransferErrorMessage) + assertFileTransferBlocked(t, err.Error()) }) t.Run("Forbidden commands", func(t *testing.T) { @@ -1046,18 +1046,20 @@ func TestAgent_FileTransferBlocked(t *testing.T) { msg, err := io.ReadAll(stdout) require.NoError(t, err) - errorMessage := string(msg) - - // NOTE: Checking content of the error message is flaky. It can catch: "File transfer has been disabled", "EOF", or "Process exited with status 2". - isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || - strings.Contains(errorMessage, "EOF") || - strings.Contains(errorMessage, "Process exited with status 2") - require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) + assertFileTransferBlocked(t, string(msg)) }) } }) } +func assertFileTransferBlocked(t *testing.T, errorMessage string) { + // NOTE: Checking content of the error message is flaky. It can catch: "File transfer has been disabled", "EOF", or "Process exited with status 2". + isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || + strings.Contains(errorMessage, "EOF") || + strings.Contains(errorMessage, "Process exited with status 2") + require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) +} + func TestAgent_EnvironmentVariables(t *testing.T) { t.Parallel() key := "EXAMPLE" diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index e7b4dff097bb1..c5c5f5f9158fa 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -279,9 +279,8 @@ func (s *Server) sessionHandler(session ssh.Session) { extraEnv = append(extraEnv, fmt.Sprintf("DISPLAY=:%d.0", x11.ScreenNumber)) } - s.logger.Warn(ctx, "fileTransferBlocked", slog.F("session", session)) if s.fileTransferBlocked(session) { - s.logger.Warn(ctx, "fileTransferBlocked", slog.F("go ", "yes")) + s.logger.Warn(ctx, "file transfer blocked", slog.F("session_subsystem", session.Subsystem()), slog.F("raw_command", session.RawCommand())) if session.Subsystem() == "" { // sftp does not expect error, otherwise it fails with "package too long" // Response format: \n @@ -291,7 +290,6 @@ func (s *Server) sessionHandler(session ssh.Session) { _ = session.Exit(BlockedFileTransferErrorCode) return } - s.logger.Warn(ctx, "fileTransferBlocked end") switch ss := session.Subsystem(); ss { case "": From d92c070ac64330fcd509fd564f7a03aa49375ff6 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 14:54:34 +0200 Subject: [PATCH 08/13] fix --- agent/agent_test.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index adcd54210ad4d..9937cfc8b34b3 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -973,6 +973,15 @@ func TestAgent_SCP(t *testing.T) { func TestAgent_FileTransferBlocked(t *testing.T) { t.Parallel() + assertFileTransferBlocked := func(t *testing.T, errorMessage string) { + // NOTE: Checking content of the error message is flaky. It can catch different responses. + isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || + strings.Contains(errorMessage, "unexpected EOF") || + strings.Contains(errorMessage, "failed to send packet: EOF") || + strings.Contains(errorMessage, "Process exited with status 2") + require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) + } + t.Run("SFTP", func(t *testing.T) { t.Parallel() @@ -988,7 +997,7 @@ func TestAgent_FileTransferBlocked(t *testing.T) { defer sshClient.Close() _, err = sftp.NewClient(sshClient) require.Error(t, err) - require.Contains(t, err.Error(), "unexpected EOF") + assertFileTransferBlocked(t, err.Error()) }) t.Run("SCP with go-scp package", func(t *testing.T) { @@ -1052,14 +1061,6 @@ func TestAgent_FileTransferBlocked(t *testing.T) { }) } -func assertFileTransferBlocked(t *testing.T, errorMessage string) { - // NOTE: Checking content of the error message is flaky. It can catch: "File transfer has been disabled", "EOF", or "Process exited with status 2". - isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || - strings.Contains(errorMessage, "EOF") || - strings.Contains(errorMessage, "Process exited with status 2") - require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) -} - func TestAgent_EnvironmentVariables(t *testing.T) { t.Parallel() key := "EXAMPLE" From 47ee9b0aafadf1822436a84e691c650c402f830b Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Fri, 7 Jun 2024 15:01:45 +0200 Subject: [PATCH 09/13] fix --- agent/agent_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 9937cfc8b34b3..ec463fa8616a2 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -976,8 +976,7 @@ func TestAgent_FileTransferBlocked(t *testing.T) { assertFileTransferBlocked := func(t *testing.T, errorMessage string) { // NOTE: Checking content of the error message is flaky. It can catch different responses. isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || - strings.Contains(errorMessage, "unexpected EOF") || - strings.Contains(errorMessage, "failed to send packet: EOF") || + strings.Contains(errorMessage, "EOF") || strings.Contains(errorMessage, "Process exited with status 2") require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) } From 4e457613ef9b49786e5429c1cf23f5c75ba2e5d1 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 10 Jun 2024 12:16:49 +0200 Subject: [PATCH 10/13] PR comments --- agent/agent_test.go | 8 ++++++-- agent/agentssh/agentssh.go | 8 +++++--- cli/agent.go | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index ec463fa8616a2..2be9fadb1fb13 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -974,10 +974,14 @@ func TestAgent_FileTransferBlocked(t *testing.T) { t.Parallel() assertFileTransferBlocked := func(t *testing.T, errorMessage string) { - // NOTE: Checking content of the error message is flaky. It can catch different responses. + // NOTE: Checking content of the error message is flaky. Most likely there is a race condition, which results + // in stopping the client in different phases, and returning different errors: + // - client read the full error message: File transfer has been disabled. + // - client's stream was terminated before reading the error message: EOF + // - client just read the error code (Windows): Process exited with status 65 isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) || strings.Contains(errorMessage, "EOF") || - strings.Contains(errorMessage, "Process exited with status 2") + strings.Contains(errorMessage, "Process exited with status 65") require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage)) } diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index c5c5f5f9158fa..9dfd601d92b97 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -55,7 +55,7 @@ const ( // BlockedFileTransferErrorCode indicates that SSH server restricted the raw command from performing // the file transfer. - BlockedFileTransferErrorCode = 2 + BlockedFileTransferErrorCode = 65 // Error code: host not allowed to connect BlockedFileTransferErrorMessage = "File transfer has been disabled." ) @@ -342,8 +342,10 @@ func (s *Server) sessionHandler(session ssh.Session) { } // fileTransferBlocked method checks if the file transfer commands should be blocked. -// Warning: consider this mechanism as "Do not trespass" sign. If a user needs a more sophisticated -// and battle-proof solution, consider the full endpoint security. +// +// Warning: consider this mechanism as "Do not trespass" sign, as a violator can still ssh to the host, +// smuggle the `scp` binary, or just manually send files outside with `curl` or `ftp`. +// If a user needs a more sophisticated and battle-proof solution, consider full endpoint security. func (s *Server) fileTransferBlocked(session ssh.Session) bool { if !s.config.BlockFileTransfer { return false // file transfers are permitted diff --git a/cli/agent.go b/cli/agent.go index bb8d028cef391..2d63b070e03ca 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -423,7 +423,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { { Flag: "block-file-transfer", Default: "false", - Env: "CODER_BLOCK_FILE_TRANSFER", + Env: "CODER_AGENT_BLOCK_FILE_TRANSFER", Description: "Block file transfer using known applications: nc, rsync, scp, sftp.", Value: serpent.BoolOf(&blockFileTransfer), }, From d852417092d65e5b60199550a81a705be83378aa Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 10 Jun 2024 13:00:31 +0200 Subject: [PATCH 11/13] WIP --- agent/agent_test.go | 4 +--- agent/agentssh/agentssh.go | 13 ++++++++----- cli/agent.go | 3 ++- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 2be9fadb1fb13..f9f7042c9b0f5 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1028,9 +1028,7 @@ func TestAgent_FileTransferBlocked(t *testing.T) { t.Run("Forbidden commands", func(t *testing.T) { t.Parallel() - commands := []string{"nc", "rsync", "scp", "sftp"} - for _, c := range commands { - c := c + for _, c := range agentssh.BlockedFileTransferCommands { t.Run(c, func(t *testing.T) { t.Parallel() diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 9dfd601d92b97..5903220975b8c 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -59,6 +59,9 @@ const ( BlockedFileTransferErrorMessage = "File transfer has been disabled." ) +// BlockedFileTransferCommands contains a list of restricted file transfer commands. +var BlockedFileTransferCommands = []string{"nc", "rsync", "scp", "sftp"} + // Config sets configuration parameters for the agent SSH server. type Config struct { // MaxTimeout sets the absolute connection timeout, none if empty. If set to @@ -364,12 +367,12 @@ func (s *Server) fileTransferBlocked(session ssh.Session) bool { c := cmd[0] c = filepath.Base(c) // in case the binary is absolute path, /usr/sbin/scp - switch c { - case "nc", "rsync", "scp", "sftp": - return true // forbidden command - default: - return false + for _, cmd := range BlockedFileTransferCommands { + if cmd == c { + return true + } } + return false } func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, extraEnv []string) (retErr error) { diff --git a/cli/agent.go b/cli/agent.go index 2d63b070e03ca..942700258eaf8 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -27,6 +27,7 @@ import ( "cdr.dev/slog/sloggers/slogstackdriver" "github.com/coder/coder/v2/agent" "github.com/coder/coder/v2/agent/agentproc" + "github.com/coder/coder/v2/agent/agentssh" "github.com/coder/coder/v2/agent/reaper" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/codersdk" @@ -424,7 +425,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { Flag: "block-file-transfer", Default: "false", Env: "CODER_AGENT_BLOCK_FILE_TRANSFER", - Description: "Block file transfer using known applications: nc, rsync, scp, sftp.", + Description: fmt.Sprintf("Block file transfer using known applications: %s", strings.Join(agentssh.BlockedFileTransferCommands, ",")), Value: serpent.BoolOf(&blockFileTransfer), }, } From 96ca9aea417c5e2d61fe115b887ea4a9576d3648 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 10 Jun 2024 13:19:18 +0200 Subject: [PATCH 12/13] Fix --- cli/agent.go | 2 +- cli/testdata/coder_agent_--help.golden | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/agent.go b/cli/agent.go index 942700258eaf8..5465aeedd9302 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -425,7 +425,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command { Flag: "block-file-transfer", Default: "false", Env: "CODER_AGENT_BLOCK_FILE_TRANSFER", - Description: fmt.Sprintf("Block file transfer using known applications: %s", strings.Join(agentssh.BlockedFileTransferCommands, ",")), + Description: fmt.Sprintf("Block file transfer using known applications: %s.", strings.Join(agentssh.BlockedFileTransferCommands, ",")), Value: serpent.BoolOf(&blockFileTransfer), }, } diff --git a/cli/testdata/coder_agent_--help.golden b/cli/testdata/coder_agent_--help.golden index 84eaf3129759b..d6982fda18e7c 100644 --- a/cli/testdata/coder_agent_--help.golden +++ b/cli/testdata/coder_agent_--help.golden @@ -18,8 +18,8 @@ OPTIONS: --auth string, $CODER_AGENT_AUTH (default: token) Specify the authentication type to use for the agent. - --block-file-transfer bool, $CODER_BLOCK_FILE_TRANSFER (default: false) - Block file transfer using known applications: nc, rsync, scp, sftp. + --block-file-transfer bool, $CODER_AGENT_BLOCK_FILE_TRANSFER (default: false) + Block file transfer using known applications: nc,rsync,scp,sftp. --debug-address string, $CODER_AGENT_DEBUG_ADDRESS (default: 127.0.0.1:2113) The bind address to serve a debug HTTP server. From 19fe5cfdaf8686e32d9dc8fdbf655577893947bb Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Mon, 10 Jun 2024 13:31:06 +0200 Subject: [PATCH 13/13] fix lint --- agent/agent_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index f9f7042c9b0f5..4b0712bcf93c6 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -1050,6 +1050,7 @@ func TestAgent_FileTransferBlocked(t *testing.T) { stdout, err := session.StdoutPipe() require.NoError(t, err) + //nolint:govet // we don't need `c := c` in Go 1.22 err = session.Start(c) require.NoError(t, err) defer session.Close()