Skip to content

Commit b046ae5

Browse files
ericpaulsenericpaulsen
committed
Merge branch 'main' into validated-architecture
2 parents 199e83d + d0fc81a commit b046ae5

File tree

223 files changed

+6575
-4899
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

223 files changed

+6575
-4899
lines changed

‎.github/workflows/ci.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ jobs:
170170
171171
# Check for any typos
172172
- name: Check for typos
173-
uses: crate-ci/typos@v1.21.0
173+
uses: crate-ci/typos@v1.22.3
174174
with:
175175
config: .github/workflows/typos.toml
176176

@@ -922,7 +922,7 @@ jobs:
922922
uses: actions/dependency-review-action@v4.3.2
923923
with:
924924
allow-licenses: Apache-2.0, BSD-2-Clause, BSD-3-Clause, CC0-1.0, ISC, MIT, MIT-0, MPL-2.0
925-
allow-dependencies-licenses: "pkg:golang/github.com/coder/wgtunnel@0.1.13-0.20240522110300-ade90dfb2da0"
925+
allow-dependencies-licenses: "pkg:golang/github.com/coder/wgtunnel@0.1.13-0.20240522110300-ade90dfb2da0, pkg:npm/pako@1.0.11"
926926
license-check: true
927927
vulnerability-check: false
928928
- name: "Report"

‎.github/workflows/security.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ jobs:
114114
echo "image=$(cat "$image_job")" >> $GITHUB_OUTPUT
115115
116116
- name: Run Trivy vulnerability scanner
117-
uses: aquasecurity/trivy-action@fd25fed6972e341ff0007ddb61f77e88103953c2
117+
uses: aquasecurity/trivy-action@595be6a0f6560a0a8fc419ddf630567fc623531d
118118
with:
119119
image-ref: ${{ steps.build.outputs.image }}
120120
format: sarif

‎README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,4 +125,4 @@ contributions!
125125

126126
## Hiring
127127

128-
Apply [here](https://cdr.co/github-apply) if you're interested in joining our team.
128+
Apply [here](https://jobs.ashbyhq.com/coder?utm_source=github&utm_medium=readme&utm_campaign=unknown) if you're interested in joining our team.

‎agent/agent.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ type Options struct {
9191
ModifiedProcesses chan []*agentproc.Process
9292
// ProcessManagementTick is used for testing process priority management.
9393
ProcessManagementTick <-chan time.Time
94+
BlockFileTransfer bool
9495
}
9596

9697
type Client interface {
@@ -184,6 +185,7 @@ func New(options Options) Agent {
184185
modifiedProcs: options.ModifiedProcesses,
185186
processManagementTick: options.ProcessManagementTick,
186187
logSender: agentsdk.NewLogSender(options.Logger),
188+
blockFileTransfer: options.BlockFileTransfer,
187189

188190
prometheusRegistry: prometheusRegistry,
189191
metrics: newAgentMetrics(prometheusRegistry),
@@ -239,6 +241,7 @@ type agent struct {
239241
sessionToken atomic.Pointer[string]
240242
sshServer *agentssh.Server
241243
sshMaxTimeout time.Duration
244+
blockFileTransfer bool
242245

243246
lifecycleUpdate chan struct{}
244247
lifecycleReported chan codersdk.WorkspaceAgentLifecycle
@@ -277,6 +280,7 @@ func (a *agent) init() {
277280
AnnouncementBanners: func() *[]codersdk.BannerConfig { return a.announcementBanners.Load() },
278281
UpdateEnv: a.updateCommandEnv,
279282
WorkingDirectory: func() string { return a.manifest.Load().Directory },
283+
BlockFileTransfer: a.blockFileTransfer,
280284
})
281285
if err != nil {
282286
panic(err)

‎agent/agent_test.go

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -970,6 +970,99 @@ func TestAgent_SCP(t *testing.T) {
970970
require.NoError(t, err)
971971
}
972972

973+
func TestAgent_FileTransferBlocked(t *testing.T) {
974+
t.Parallel()
975+
976+
assertFileTransferBlocked := func(t *testing.T, errorMessage string) {
977+
// NOTE: Checking content of the error message is flaky. Most likely there is a race condition, which results
978+
// in stopping the client in different phases, and returning different errors:
979+
// - client read the full error message: File transfer has been disabled.
980+
// - client's stream was terminated before reading the error message: EOF
981+
// - client just read the error code (Windows): Process exited with status 65
982+
isErr := strings.Contains(errorMessage, agentssh.BlockedFileTransferErrorMessage) ||
983+
strings.Contains(errorMessage, "EOF") ||
984+
strings.Contains(errorMessage, "Process exited with status 65")
985+
require.True(t, isErr, fmt.Sprintf("Message: "+errorMessage))
986+
}
987+
988+
t.Run("SFTP", func(t *testing.T) {
989+
t.Parallel()
990+
991+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
992+
defer cancel()
993+
994+
//nolint:dogsled
995+
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
996+
o.BlockFileTransfer = true
997+
})
998+
sshClient, err := conn.SSHClient(ctx)
999+
require.NoError(t, err)
1000+
defer sshClient.Close()
1001+
_, err = sftp.NewClient(sshClient)
1002+
require.Error(t, err)
1003+
assertFileTransferBlocked(t, err.Error())
1004+
})
1005+
1006+
t.Run("SCP with go-scp package", func(t *testing.T) {
1007+
t.Parallel()
1008+
1009+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1010+
defer cancel()
1011+
1012+
//nolint:dogsled
1013+
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
1014+
o.BlockFileTransfer = true
1015+
})
1016+
sshClient, err := conn.SSHClient(ctx)
1017+
require.NoError(t, err)
1018+
defer sshClient.Close()
1019+
scpClient, err := scp.NewClientBySSH(sshClient)
1020+
require.NoError(t, err)
1021+
defer scpClient.Close()
1022+
tempFile := filepath.Join(t.TempDir(), "scp")
1023+
err = scpClient.CopyFile(context.Background(), strings.NewReader("hello world"), tempFile, "0755")
1024+
require.Error(t, err)
1025+
assertFileTransferBlocked(t, err.Error())
1026+
})
1027+
1028+
t.Run("Forbidden commands", func(t *testing.T) {
1029+
t.Parallel()
1030+
1031+
for _, c := range agentssh.BlockedFileTransferCommands {
1032+
t.Run(c, func(t *testing.T) {
1033+
t.Parallel()
1034+
1035+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1036+
defer cancel()
1037+
1038+
//nolint:dogsled
1039+
conn, _, _, _, _ := setupAgent(t, agentsdk.Manifest{}, 0, func(_ *agenttest.Client, o *agent.Options) {
1040+
o.BlockFileTransfer = true
1041+
})
1042+
sshClient, err := conn.SSHClient(ctx)
1043+
require.NoError(t, err)
1044+
defer sshClient.Close()
1045+
1046+
session, err := sshClient.NewSession()
1047+
require.NoError(t, err)
1048+
defer session.Close()
1049+
1050+
stdout, err := session.StdoutPipe()
1051+
require.NoError(t, err)
1052+
1053+
//nolint:govet // we don't need `c := c` in Go 1.22
1054+
err = session.Start(c)
1055+
require.NoError(t, err)
1056+
defer session.Close()
1057+
1058+
msg, err := io.ReadAll(stdout)
1059+
require.NoError(t, err)
1060+
assertFileTransferBlocked(t, string(msg))
1061+
})
1062+
}
1063+
})
1064+
}
1065+
9731066
func TestAgent_EnvironmentVariables(t *testing.T) {
9741067
t.Parallel()
9751068
key := "EXAMPLE"

‎agent/agentssh/agentssh.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,16 @@ const (
5252
// MagicProcessCmdlineJetBrains is a string in a process's command line that
5353
// uniquely identifies it as JetBrains software.
5454
MagicProcessCmdlineJetBrains = "idea.vendor.name=JetBrains"
55+
56+
// BlockedFileTransferErrorCode indicates that SSH server restricted the raw command from performing
57+
// the file transfer.
58+
BlockedFileTransferErrorCode = 65 // Error code: host not allowed to connect
59+
BlockedFileTransferErrorMessage = "File transfer has been disabled."
5560
)
5661

62+
// BlockedFileTransferCommands contains a list of restricted file transfer commands.
63+
var BlockedFileTransferCommands = []string{"nc", "rsync", "scp", "sftp"}
64+
5765
// Config sets configuration parameters for the agent SSH server.
5866
type Config struct {
5967
// MaxTimeout sets the absolute connection timeout, none if empty. If set to
@@ -74,6 +82,8 @@ type Config struct {
7482
// X11SocketDir is the directory where X11 sockets are created. Default is
7583
// /tmp/.X11-unix.
7684
X11SocketDir string
85+
// BlockFileTransfer restricts use of file transfer applications.
86+
BlockFileTransfer bool
7787
}
7888

7989
type Server struct {
@@ -272,6 +282,18 @@ func (s *Server) sessionHandler(session ssh.Session) {
272282
extraEnv = append(extraEnv, fmt.Sprintf("DISPLAY=:%d.0", x11.ScreenNumber))
273283
}
274284

285+
if s.fileTransferBlocked(session) {
286+
s.logger.Warn(ctx, "file transfer blocked", slog.F("session_subsystem", session.Subsystem()), slog.F("raw_command", session.RawCommand()))
287+
288+
if session.Subsystem() == "" { // sftp does not expect error, otherwise it fails with "package too long"
289+
// Response format: <status_code><message body>\n
290+
errorMessage := fmt.Sprintf("\x02%s\n", BlockedFileTransferErrorMessage)
291+
_, _ = session.Write([]byte(errorMessage))
292+
}
293+
_ = session.Exit(BlockedFileTransferErrorCode)
294+
return
295+
}
296+
275297
switch ss := session.Subsystem(); ss {
276298
case "":
277299
case "sftp":
@@ -322,6 +344,37 @@ func (s *Server) sessionHandler(session ssh.Session) {
322344
_ = session.Exit(0)
323345
}
324346

347+
// fileTransferBlocked method checks if the file transfer commands should be blocked.
348+
//
349+
// Warning: consider this mechanism as "Do not trespass" sign, as a violator can still ssh to the host,
350+
// smuggle the `scp` binary, or just manually send files outside with `curl` or `ftp`.
351+
// If a user needs a more sophisticated and battle-proof solution, consider full endpoint security.
352+
func (s *Server) fileTransferBlocked(session ssh.Session) bool {
353+
if !s.config.BlockFileTransfer {
354+
return false // file transfers are permitted
355+
}
356+
// File transfers are restricted.
357+
358+
if session.Subsystem() == "sftp" {
359+
return true
360+
}
361+
362+
cmd := session.Command()
363+
if len(cmd) == 0 {
364+
return false // no command?
365+
}
366+
367+
c := cmd[0]
368+
c = filepath.Base(c) // in case the binary is absolute path, /usr/sbin/scp
369+
370+
for _, cmd := range BlockedFileTransferCommands {
371+
if cmd == c {
372+
return true
373+
}
374+
}
375+
return false
376+
}
377+
325378
func (s *Server) sessionStart(logger slog.Logger, session ssh.Session, extraEnv []string) (retErr error) {
326379
ctx := session.Context()
327380
env := append(session.Environ(), extraEnv...)

‎cli/agent.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"cdr.dev/slog/sloggers/slogstackdriver"
2828
"github.com/coder/coder/v2/agent"
2929
"github.com/coder/coder/v2/agent/agentproc"
30+
"github.com/coder/coder/v2/agent/agentssh"
3031
"github.com/coder/coder/v2/agent/reaper"
3132
"github.com/coder/coder/v2/buildinfo"
3233
"github.com/coder/coder/v2/codersdk"
@@ -48,6 +49,7 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
4849
slogHumanPath string
4950
slogJSONPath string
5051
slogStackdriverPath string
52+
blockFileTransfer bool
5153
)
5254
cmd := &serpent.Command{
5355
Use: "agent",
@@ -314,6 +316,8 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
314316
// Intentionally set this to nil. It's mainly used
315317
// for testing.
316318
ModifiedProcesses: nil,
319+
320+
BlockFileTransfer: blockFileTransfer,
317321
})
318322

319323
promHandler := agent.PrometheusMetricsHandler(prometheusRegistry, logger)
@@ -417,6 +421,13 @@ func (r *RootCmd) workspaceAgent() *serpent.Command {
417421
Default: "",
418422
Value: serpent.StringOf(&slogStackdriverPath),
419423
},
424+
{
425+
Flag: "block-file-transfer",
426+
Default: "false",
427+
Env: "CODER_AGENT_BLOCK_FILE_TRANSFER",
428+
Description: fmt.Sprintf("Block file transfer using known applications: %s.", strings.Join(agentssh.BlockedFileTransferCommands, ",")),
429+
Value: serpent.BoolOf(&blockFileTransfer),
430+
},
420431
}
421432

422433
return cmd

‎cli/cliui/output.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"reflect"
88
"strings"
99

10+
"github.com/jedib0t/go-pretty/v6/table"
1011
"golang.org/x/xerrors"
1112

1213
"github.com/coder/serpent"
@@ -143,7 +144,11 @@ func (f *tableFormat) AttachOptions(opts *serpent.OptionSet) {
143144

144145
// Format implements OutputFormat.
145146
func (f *tableFormat) Format(_ context.Context, data any) (string, error) {
146-
return DisplayTable(data, f.sort, f.columns)
147+
headers := make(table.Row, len(f.allColumns))
148+
for i, header := range f.allColumns {
149+
headers[i] = header
150+
}
151+
return renderTable(data, f.sort, headers, f.columns)
147152
}
148153

149154
type jsonFormat struct{}

0 commit comments

Comments
 (0)