From b8a1c0e3d3d006d18f2073a26aab90bb0d55f1b2 Mon Sep 17 00:00:00 2001 From: Clint Berry Date: Tue, 28 Jul 2026 19:21:18 +0000 Subject: [PATCH] fix(sshproxy): run docker exec as the devcontainer's remoteUser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running any git command in a VS Code terminal opened against a session failed with: fatal: detected dubious ownership in repository at '/workspaces/' Devcontainer images routinely declare `USER root` and a separate `remoteUser`, and DevPod chowns the workspace tree to that remoteUser. `devpod ssh` — the browser terminal path — honors it, but the SSH proxy built every `docker exec` without `--user`, so VS Code channels landed as uid 0 in a uid 1000 tree. Git's safe.directory check compares the repo owner against the process euid, sees the mismatch, and refuses. Resolve the user from the container's devcontainer.metadata label (remoteUser, then containerUser, per the spec's precedence) and pass `--user` on every exec shape: shells, exec, SFTP, and TCP forwards. SFTP matters as much as the shell — writing files as root into a remoteUser-owned tree is the same bug with a slower fuse. Resolution fails open. No declared user, a docker inspect error, or a proxy without a workspace manager all fall back to the image's USER rather than killing the channel: guessing wrong costs a git warning, erroring out costs the whole editor session. The label value is attacker-influenceable — a session member controls the devcontainer.json in their own repo — so it is validated against a strict user/uid[:group] pattern before reaching argv. Lookups are memoized per container with a TTL, since VS Code opens many channels per connection and each resolves the user. Note `devpod.user` is not this value: it reports root on images that declare a remoteUser, so keying off it would reproduce the bug. --- CLAUDE.md | 2 +- server/internal/sshproxy/docker.go | 61 ++++++-- server/internal/sshproxy/docker_test.go | 94 +++++++++++++ server/internal/sshproxy/session.go | 6 +- server/internal/sshproxy/session_test.go | 14 +- server/internal/sshproxy/sftp.go | 4 +- server/internal/sshproxy/sftp_test.go | 10 +- server/internal/sshproxy/tcpip.go | 3 +- server/internal/sshproxy/tcpip_test.go | 4 +- server/internal/workspace/container_user.go | 132 ++++++++++++++++++ .../internal/workspace/container_user_test.go | 80 +++++++++++ server/internal/workspace/manager.go | 7 + 12 files changed, 387 insertions(+), 30 deletions(-) create mode 100644 server/internal/sshproxy/docker_test.go create mode 100644 server/internal/workspace/container_user.go create mode 100644 server/internal/workspace/container_user_test.go diff --git a/CLAUDE.md b/CLAUDE.md index ee49765..177755e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -227,7 +227,7 @@ DEUCE_WS_ALLOWED_ORIGINS=vmname.exe.xyz The browser **Terminal panel** uses `devpod ssh` (VM sshd → DevPod's in-container shim → user shell). The **Open-in-VS-Code** path uses `docker exec`. They diverge on: - **Environment:** `docker exec` does not set `SSH_*` vars; the SSH proxy explicitly forwards only `VSCODE_*`, `LANG`, `LC_*`, `TERM`, `HOME`, `USER`, `SHELL` (allowlist defeats `LD_PRELOAD` injection). The browser terminal inherits whatever DevPod forwards. -- **UID:** `devpod ssh` runs as the devcontainer's `remoteUser` (typically `vscode`/`node`). `docker exec` defaults to the image's `USER` directive (which is often the same user, but not guaranteed — verify per image). +- **UID:** both paths run as the devcontainer's `remoteUser` (typically `vscode`/`node`). `devpod ssh` honors it natively; the SSH proxy resolves it from the container's `devcontainer.metadata` label and passes `--user` (`workspace.ContainerUser`). Without that, `docker exec` would fall back to the image's `USER` — commonly `root` — and git would reject the `remoteUser`-owned workspace with `fatal: detected dubious ownership`. A container declaring neither `remoteUser` nor `containerUser` still falls back to the image's `USER`. Note the `devpod.user` label is *not* this value; it reports `root` on images that declare a `remoteUser`. - **Shell startup:** `docker exec -it /bin/bash -l` is a login shell; the terminal panel's interactive bash is non-login. Users may see "my git config works in the terminal but not VS Code" surprises — document the divergence in user-facing docs. Convergence (route the terminal panel through the SSH proxy too) is reserved as a v2 cleanup. diff --git a/server/internal/sshproxy/docker.go b/server/internal/sshproxy/docker.go index e78e97d..c5424b9 100644 --- a/server/internal/sshproxy/docker.go +++ b/server/internal/sshproxy/docker.go @@ -2,6 +2,7 @@ package sshproxy import ( "context" + "log/slog" "os/exec" "strconv" "strings" @@ -76,6 +77,27 @@ type envEntry struct { Value string } +// resolveExecUser returns the user docker exec should run as for this +// container, or "" to leave the image's USER in place. +// +// Deliberately fail-open: a devcontainer that declares no remoteUser, a +// docker inspect that errors, or a proxy built without a workspace manager +// (tests) all degrade to the previous behaviour rather than killing the +// channel. The cost of guessing wrong is a git ownership warning; the cost +// of erroring out is an unusable editor session. +func (s *Server) resolveExecUser(ctx context.Context, container string) string { + if s.workspaces == nil { + return "" + } + user, err := s.workspaces.ContainerUser(ctx, container) + if err != nil { + slog.Warn("ssh: could not resolve container exec user; using image default", + "container", container, "error", err) + return "" + } + return user +} + // execMode discriminates the docker-exec invocation shapes we use. type execMode int @@ -97,11 +119,13 @@ const ( // matters because docker exec spawns child processes inside the container // namespace but the host-side `docker` binary itself may leave grandchild // monitors around. -func buildExecCmd(ctx context.Context, bin, container, command string, mode execMode, env []string) *exec.Cmd { +// `user` is the devcontainer's remoteUser (see workspace.ContainerUser); +// empty leaves the image's USER in place. +func buildExecCmd(ctx context.Context, bin, container, command string, mode execMode, env []string, user string) *exec.Cmd { if bin == "" { bin = defaultDockerBin } - args := dockerArgs(container, command, mode) + args := dockerArgs(container, command, mode, user) cmd := exec.CommandContext(ctx, bin, args...) cmd.Env = env cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} @@ -131,14 +155,19 @@ func buildExecCmd(ctx context.Context, bin, container, command string, mode exec // the inputs go through ssh.Unmarshal of a typed payload struct (host // is a length-prefixed string, port is a uint32), so the only attack // surface is the validation rules — not shell-parsing. -func buildTCPForwardCmd(ctx context.Context, bin, container, host string, port uint32) *exec.Cmd { +func buildTCPForwardCmd(ctx context.Context, bin, container, host string, port uint32, user string) *exec.Cmd { if bin == "" { bin = defaultDockerBin } script := "exec 3<>/dev/tcp/" + host + "/" + strconv.FormatUint(uint64(port), 10) + " || exit 1\n" + "( cat <&3; kill -TERM $$ 2>/dev/null ) &\n" + "cat >&3\n" - cmd := exec.CommandContext(ctx, bin, "exec", "-i", container, "bash", "-c", script) + args := []string{"exec"} + if user != "" { + args = append(args, "--user", user) + } + args = append(args, "-i", container, "bash", "-c", script) + cmd := exec.CommandContext(ctx, bin, args...) cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} return cmd } @@ -146,17 +175,29 @@ func buildTCPForwardCmd(ctx context.Context, bin, container, host string, port u // dockerArgs builds the argv slice for a docker exec invocation in the // given mode. Exposed for unit tests so they can assert the exact argv // without running docker. -func dockerArgs(container, command string, mode execMode) []string { +// `user`, when non-empty, is passed as `--user` so the exec runs as the +// devcontainer's remoteUser rather than the image's USER. Those differ on +// most devcontainer images (USER root + remoteUser vscode), and the mismatch +// makes git reject the workspace as dubiously-owned. Empty means "leave the +// image's USER alone" — an empty `--user` would make docker reject the exec. +func dockerArgs(container, command string, mode execMode, user string) []string { + // The flag belongs to `docker exec`, so it must precede the container + // name; anything after that is the in-container command. + pre := []string{"exec"} + if user != "" { + pre = append(pre, "--user", user) + } + switch mode { case execModePTYShell: - return []string{"exec", "-it", container, "/bin/bash", "-l"} + return append(pre, "-it", container, "/bin/bash", "-l") case execModeNonPTYShell: - return []string{"exec", "-i", container, "/bin/bash", "-l"} + return append(pre, "-i", container, "/bin/bash", "-l") case execModePTYExec: - return []string{"exec", "-it", container, "/bin/sh", "-c", command} + return append(pre, "-it", container, "/bin/sh", "-c", command) case execModeSFTP: - return []string{"exec", "-i", container, "/usr/lib/openssh/sftp-server", "-e"} + return append(pre, "-i", container, "/usr/lib/openssh/sftp-server", "-e") default: // execModeNonPTY - return []string{"exec", "-i", container, "/bin/sh", "-c", command} + return append(pre, "-i", container, "/bin/sh", "-c", command) } } diff --git a/server/internal/sshproxy/docker_test.go b/server/internal/sshproxy/docker_test.go new file mode 100644 index 0000000..9ebf091 --- /dev/null +++ b/server/internal/sshproxy/docker_test.go @@ -0,0 +1,94 @@ +package sshproxy + +import ( + "context" + "slices" + "strings" + "testing" +) + +// allModes covers every docker-exec shape the proxy builds. A missing +// --user in any one of them puts that channel back on the image's USER, +// which is what produced `fatal: detected dubious ownership` in VS Code +// terminals: the image declares USER root while the workspace tree is +// owned by the devcontainer's remoteUser. +var allModes = []struct { + name string + mode execMode +}{ + {"non-pty", execModeNonPTY}, + {"pty-exec", execModePTYExec}, + {"pty-shell", execModePTYShell}, + {"non-pty-shell", execModeNonPTYShell}, + {"sftp", execModeSFTP}, +} + +func TestDockerArgsRunsAsResolvedUser(t *testing.T) { + for _, m := range allModes { + t.Run(m.name, func(t *testing.T) { + args := dockerArgs("alice", "echo hi", m.mode, "vscode") + + i := slices.Index(args, "--user") + if i < 0 { + t.Fatalf("dockerArgs(%s) has no --user: %#v", m.name, args) + } + if i+1 >= len(args) || args[i+1] != "vscode" { + t.Fatalf("dockerArgs(%s) --user value wrong: %#v", m.name, args) + } + // The flag belongs to `docker exec`, so it has to precede the + // container name — after it, docker treats it as part of the + // in-container command. + c := slices.Index(args, "alice") + if c < 0 || i > c { + t.Errorf("dockerArgs(%s) --user must precede the container: %#v", m.name, args) + } + }) + } +} + +func TestDockerArgsOmitsUserWhenUnknown(t *testing.T) { + // No declared user means today's behaviour: let the image's USER stand. + // Passing an empty --user would make docker reject the exec outright. + for _, m := range allModes { + t.Run(m.name, func(t *testing.T) { + args := dockerArgs("alice", "echo hi", m.mode, "") + + if slices.Contains(args, "--user") { + t.Errorf("dockerArgs(%s) sent --user with no resolved user: %#v", m.name, args) + } + if slices.Contains(args, "") { + t.Errorf("dockerArgs(%s) has an empty argv slot: %#v", m.name, args) + } + }) + } +} + +func TestBuildExecCmdPassesUser(t *testing.T) { + cmd := buildExecCmd(context.Background(), "docker", "alice", "echo hi", execModePTYShell, nil, "vscode") + + if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") { + t.Errorf("buildExecCmd argv missing user: %#v", cmd.Args) + } +} + +func TestBuildSFTPCmdPassesUser(t *testing.T) { + // SFTP writes files into the workspace, so running it as the wrong user + // leaves root-owned files in a remoteUser-owned tree. + cmd := buildSFTPCmd(context.Background(), "docker", "alice", "vscode") + + if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") { + t.Errorf("buildSFTPCmd argv missing user: %#v", cmd.Args) + } +} + +func TestBuildTCPForwardCmdPassesUser(t *testing.T) { + cmd := buildTCPForwardCmd(context.Background(), "docker", "alice", "127.0.0.1", 8080, "vscode") + + if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") { + t.Errorf("buildTCPForwardCmd argv missing user: %#v", cmd.Args) + } + // The forwarding script must survive the added flag intact. + if !strings.Contains(strings.Join(cmd.Args, " "), "/dev/tcp/127.0.0.1/8080") { + t.Errorf("buildTCPForwardCmd lost its script: %#v", cmd.Args) + } +} diff --git a/server/internal/sshproxy/session.go b/server/internal/sshproxy/session.go index 72087a5..90c61f7 100644 --- a/server/internal/sshproxy/session.go +++ b/server/internal/sshproxy/session.go @@ -330,14 +330,16 @@ func (s *Server) runSessionChannel( return } + user := s.resolveExecUser(chanCtx, container) + if mode == execModeSFTP { // SFTP doesn't honor client-supplied env vars and forwarding // LANG/TERM to the docker CLI would strip its PATH. Use a // dedicated builder that leaves cmd.Env nil (inherit parent). - cmd = buildSFTPCmd(chanCtx, s.dockerBin, container) + cmd = buildSFTPCmd(chanCtx, s.dockerBin, container, user) } else { env := filterEnv(envBuf) - cmd = buildExecCmd(chanCtx, s.dockerBin, container, command, mode, env) + cmd = buildExecCmd(chanCtx, s.dockerBin, container, command, mode, env, user) } if mode == execModePTYShell || mode == execModePTYExec { diff --git a/server/internal/sshproxy/session_test.go b/server/internal/sshproxy/session_test.go index 4a17e27..53d668b 100644 --- a/server/internal/sshproxy/session_test.go +++ b/server/internal/sshproxy/session_test.go @@ -25,7 +25,7 @@ import ( // ---------------------------------------------------------------------- func TestDockerArgs_NonPTY(t *testing.T) { - got := dockerArgs("alice", "echo hi", execModeNonPTY) + got := dockerArgs("alice", "echo hi", execModeNonPTY, "") want := []string{"exec", "-i", "alice", "/bin/sh", "-c", "echo hi"} if !reflect.DeepEqual(got, want) { t.Errorf("dockerArgs(non-pty):\n got: %#v\nwant: %#v", got, want) @@ -33,7 +33,7 @@ func TestDockerArgs_NonPTY(t *testing.T) { } func TestDockerArgs_PTYShell(t *testing.T) { - got := dockerArgs("alice", "", execModePTYShell) + got := dockerArgs("alice", "", execModePTYShell, "") want := []string{"exec", "-it", "alice", "/bin/bash", "-l"} if !reflect.DeepEqual(got, want) { t.Errorf("dockerArgs(pty-shell):\n got: %#v\nwant: %#v", got, want) @@ -45,7 +45,7 @@ func TestDockerArgs_PTYShell(t *testing.T) { // pty driver can't echo VS Code's piped install-script bytes back and // corrupt the byte stream it parses. func TestDockerArgs_NonPTYShell(t *testing.T) { - got := dockerArgs("alice", "", execModeNonPTYShell) + got := dockerArgs("alice", "", execModeNonPTYShell, "") want := []string{"exec", "-i", "alice", "/bin/bash", "-l"} if !reflect.DeepEqual(got, want) { t.Errorf("dockerArgs(non-pty-shell):\n got: %#v\nwant: %#v", got, want) @@ -53,7 +53,7 @@ func TestDockerArgs_NonPTYShell(t *testing.T) { } func TestDockerArgs_PTYExec(t *testing.T) { - got := dockerArgs("alice", "ls /", execModePTYExec) + got := dockerArgs("alice", "ls /", execModePTYExec, "") want := []string{"exec", "-it", "alice", "/bin/sh", "-c", "ls /"} if !reflect.DeepEqual(got, want) { t.Errorf("dockerArgs(pty-exec):\n got: %#v\nwant: %#v", got, want) @@ -61,7 +61,7 @@ func TestDockerArgs_PTYExec(t *testing.T) { } func TestDockerArgs_SFTP(t *testing.T) { - got := dockerArgs("alice", "", execModeSFTP) + got := dockerArgs("alice", "", execModeSFTP, "") want := []string{"exec", "-i", "alice", "/usr/lib/openssh/sftp-server", "-e"} if !reflect.DeepEqual(got, want) { t.Errorf("dockerArgs(sftp):\n got: %#v\nwant: %#v", got, want) @@ -71,7 +71,7 @@ func TestDockerArgs_SFTP(t *testing.T) { func TestBuildExecCmd_SetsPgidAndEnv(t *testing.T) { ctx := context.Background() env := []string{"LANG=C", "VSCODE_X=1"} - cmd := buildExecCmd(ctx, "/usr/bin/docker", "alice", "echo hi", execModeNonPTY, env) + cmd := buildExecCmd(ctx, "/usr/bin/docker", "alice", "echo hi", execModeNonPTY, env, "") if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid { t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr) } @@ -84,7 +84,7 @@ func TestBuildExecCmd_SetsPgidAndEnv(t *testing.T) { } func TestBuildExecCmd_EmptyBinUsesDefault(t *testing.T) { - cmd := buildExecCmd(context.Background(), "", "alice", "echo hi", execModeNonPTY, nil) + cmd := buildExecCmd(context.Background(), "", "alice", "echo hi", execModeNonPTY, nil, "") if got := cmd.Args[0]; got != defaultDockerBin && !strings.HasSuffix(got, defaultDockerBin) { t.Errorf("empty bin should fall back to %q, got %q", defaultDockerBin, got) } diff --git a/server/internal/sshproxy/sftp.go b/server/internal/sshproxy/sftp.go index dc78daa..467bb99 100644 --- a/server/internal/sshproxy/sftp.go +++ b/server/internal/sshproxy/sftp.go @@ -51,11 +51,11 @@ import ( // - No env is forwarded. SFTP doesn't honor client-supplied env vars, // and the U8 allowlist would just be noise here. // - argv MUST be `-i`, never `-it`. See file-level comment above. -func buildSFTPCmd(ctx context.Context, dockerBin, container string) *exec.Cmd { +func buildSFTPCmd(ctx context.Context, dockerBin, container, user string) *exec.Cmd { if dockerBin == "" { dockerBin = defaultDockerBin } - cmd := exec.CommandContext(ctx, dockerBin, dockerArgs(container, "", execModeSFTP)...) + cmd := exec.CommandContext(ctx, dockerBin, dockerArgs(container, "", execModeSFTP, user)...) cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} return cmd } diff --git a/server/internal/sshproxy/sftp_test.go b/server/internal/sshproxy/sftp_test.go index 28a2508..cfd53b5 100644 --- a/server/internal/sshproxy/sftp_test.go +++ b/server/internal/sshproxy/sftp_test.go @@ -21,7 +21,7 @@ import ( // ---------------------------------------------------------------------- func TestBuildSFTPCmd_ArgvIsDockerExecIWithSFTPServerE(t *testing.T) { - cmd := buildSFTPCmd(context.Background(), "/usr/bin/docker", "session-container") + cmd := buildSFTPCmd(context.Background(), "/usr/bin/docker", "session-container", "") // argv[0] is the binary path; argv[1:] is what we passed. wantArgs := []string{"/usr/bin/docker", "exec", "-i", "session-container", "/usr/lib/openssh/sftp-server", "-e"} @@ -34,7 +34,7 @@ func TestBuildSFTPCmd_NeverUsesPTYFlag(t *testing.T) { // SFTP is binary framing; -it would CRLF-translate and corrupt // packets. The regression risk is that someone "fixes" interactive // SFTP by adding -t, so we lock the constraint down explicitly. - cmd := buildSFTPCmd(context.Background(), "docker", "alice") + cmd := buildSFTPCmd(context.Background(), "docker", "alice", "") for _, a := range cmd.Args { if a == "-it" || a == "-t" { t.Errorf("SFTP argv must not include %q (PTY would corrupt binary framing): %#v", a, cmd.Args) @@ -45,7 +45,7 @@ func TestBuildSFTPCmd_NeverUsesPTYFlag(t *testing.T) { func TestBuildSFTPCmd_NoShellWrapper(t *testing.T) { // Argv must hit sftp-server directly, not via /bin/sh -c. A shell // wrapper would buffer stdout and add a syscall layer for no gain. - cmd := buildSFTPCmd(context.Background(), "docker", "alice") + cmd := buildSFTPCmd(context.Background(), "docker", "alice", "") for _, a := range cmd.Args { if a == "/bin/sh" || a == "/bin/bash" || a == "-c" { t.Errorf("SFTP argv must not invoke a shell, got: %#v", cmd.Args) @@ -54,7 +54,7 @@ func TestBuildSFTPCmd_NoShellWrapper(t *testing.T) { } func TestBuildSFTPCmd_EmptyBinUsesDefault(t *testing.T) { - cmd := buildSFTPCmd(context.Background(), "", "alice") + cmd := buildSFTPCmd(context.Background(), "", "alice", "") if got := cmd.Args[0]; got != defaultDockerBin && !strings.HasSuffix(got, defaultDockerBin) { t.Errorf("empty bin should fall back to %q, got %q", defaultDockerBin, got) } @@ -63,7 +63,7 @@ func TestBuildSFTPCmd_EmptyBinUsesDefault(t *testing.T) { func TestBuildSFTPCmd_SetsPgid(t *testing.T) { // Setpgid: true is required for the channel-close cleanup path — // kill(-pid) reaches the docker CLI and its grandchildren. - cmd := buildSFTPCmd(context.Background(), "docker", "alice") + cmd := buildSFTPCmd(context.Background(), "docker", "alice", "") if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid { t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr) } diff --git a/server/internal/sshproxy/tcpip.go b/server/internal/sshproxy/tcpip.go index 3a287c5..658941a 100644 --- a/server/internal/sshproxy/tcpip.go +++ b/server/internal/sshproxy/tcpip.go @@ -84,7 +84,8 @@ func (s *Server) runDirectTCPIPChannel( return } - cmd := buildTCPForwardCmd(chanCtx, s.dockerBin, container, payload.DestHost, payload.DestPort) + user := s.resolveExecUser(chanCtx, container) + cmd := buildTCPForwardCmd(chanCtx, s.dockerBin, container, payload.DestHost, payload.DestPort, user) stdinPipe, err := cmd.StdinPipe() if err != nil { diff --git a/server/internal/sshproxy/tcpip_test.go b/server/internal/sshproxy/tcpip_test.go index 34ab61d..45184eb 100644 --- a/server/internal/sshproxy/tcpip_test.go +++ b/server/internal/sshproxy/tcpip_test.go @@ -75,7 +75,7 @@ func TestValidateLoopbackDest_RejectsBadPorts(t *testing.T) { // directions — are all locked. func TestBuildTCPForwardCmd_Argv(t *testing.T) { ctx := context.Background() - cmd := buildTCPForwardCmd(ctx, "/usr/bin/docker", "alice", "127.0.0.1", 40301) + cmd := buildTCPForwardCmd(ctx, "/usr/bin/docker", "alice", "127.0.0.1", 40301, "") if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid { t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr) @@ -109,7 +109,7 @@ func TestBuildTCPForwardCmd_Argv(t *testing.T) { // TestBuildExecCmd_EmptyBinUsesDefault — empty `bin` should resolve to // the package default ("docker" from $PATH). func TestBuildTCPForwardCmd_EmptyBinUsesDefault(t *testing.T) { - cmd := buildTCPForwardCmd(context.Background(), "", "alice", "127.0.0.1", 22) + cmd := buildTCPForwardCmd(context.Background(), "", "alice", "127.0.0.1", 22, "") if len(cmd.Args) < 1 { t.Fatalf("empty argv: %v", cmd.Args) } diff --git a/server/internal/workspace/container_user.go b/server/internal/workspace/container_user.go new file mode 100644 index 0000000..9f83157 --- /dev/null +++ b/server/internal/workspace/container_user.go @@ -0,0 +1,132 @@ +package workspace + +import ( + "context" + "encoding/json" + "log/slog" + "os/exec" + "regexp" + "strings" + "time" +) + +// validExecUser matches what `docker exec --user` accepts: a name or numeric +// id, optionally with a group. Deliberately strict — the value originates in +// a container label, which a session member controls through the +// devcontainer.json in their own repository. It reaches an argv slot rather +// than a shell, so this is defence in depth, not the only barrier; the +// leading-character rule is what stops a value from reading as a flag. +var validExecUser = regexp.MustCompile(`^[a-zA-Z0-9_][a-zA-Z0-9_.-]*(:[a-zA-Z0-9_][a-zA-Z0-9_.-]*)?$`) + +// containerUserTTL bounds how long a resolved user is reused. The value can +// only change when the devcontainer is rebuilt, which yields a new container, +// but container names can be recycled — so the cache expires rather than +// living for the process lifetime. +const containerUserTTL = 5 * time.Minute + +type containerUserEntry struct { + user string + resolved time.Time +} + +// ContainerUser resolves the user that `docker exec` should run as for the +// given container. +// +// This is not the same as the image's USER. Devcontainers routinely ship +// `USER root` and declare a separate `remoteUser` that the devcontainer +// tooling switches to; DevPod chowns the workspace tree to that user. A +// `docker exec` without --user therefore lands as root in a tree owned by +// someone else, and git refuses to operate on it: +// +// fatal: detected dubious ownership in repository at '/workspaces/' +// +// Returns "" when no user is declared, which callers should treat as "leave +// the image's USER alone" rather than as an error. +// +// Note the `devpod.user` label is NOT this value — it reports root on images +// that declare a remoteUser, so using it would reproduce the bug above. +func (m *Manager) ContainerUser(ctx context.Context, container string) (string, error) { + if !validContainerName.MatchString(container) { + return "", ErrInvalidContainerName + } + + m.userMu.Lock() + entry, ok := m.userCache[container] + m.userMu.Unlock() + if ok && time.Since(entry.resolved) < containerUserTTL { + return entry.user, nil + } + + cmd := exec.CommandContext(ctx, "docker", "inspect", + "--format", `{{index .Config.Labels "devcontainer.metadata"}}`, + container, + ) + output, err := cmd.CombinedOutput() + if err != nil { + return "", err + } + + user := execUserFromMetadata(strings.TrimSpace(string(output))) + + m.userMu.Lock() + if m.userCache == nil { + m.userCache = make(map[string]containerUserEntry) + } + m.userCache[container] = containerUserEntry{user: user, resolved: time.Now()} + m.userMu.Unlock() + + return user, nil +} + +// execUserFromMetadata extracts the exec user from a `devcontainer.metadata` +// label. The label is the merged metadata array the devcontainer spec +// defines: later entries override earlier ones, and `remoteUser` outranks +// `containerUser`. Returns "" for absent, malformed, or implausible values — +// every failure degrades to "leave the image's USER alone". +func execUserFromMetadata(label string) string { + if label == "" { + return "" + } + + var entries []struct { + RemoteUser *string `json:"remoteUser"` + ContainerUser *string `json:"containerUser"` + } + if err := json.Unmarshal([]byte(label), &entries); err != nil { + // Some tooling writes a bare object instead of the spec's array. + var single struct { + RemoteUser *string `json:"remoteUser"` + ContainerUser *string `json:"containerUser"` + } + if err := json.Unmarshal([]byte(label), &single); err != nil { + slog.Debug("devcontainer.metadata is not valid JSON", "error", err) + return "" + } + entries = append(entries, single) + } + + var remote, container string + for _, e := range entries { + // An entry that omits the key must not clear a value an earlier + // entry set, so only a present, non-empty string overrides. + if e.RemoteUser != nil && *e.RemoteUser != "" { + remote = *e.RemoteUser + } + if e.ContainerUser != nil && *e.ContainerUser != "" { + container = *e.ContainerUser + } + } + + user := remote + if user == "" { + user = container + } + if user == "" { + return "" + } + if !validExecUser.MatchString(user) { + slog.Warn("devcontainer metadata declared an implausible exec user; ignoring", "user", user) + return "" + } + return user +} diff --git a/server/internal/workspace/container_user_test.go b/server/internal/workspace/container_user_test.go new file mode 100644 index 0000000..8816198 --- /dev/null +++ b/server/internal/workspace/container_user_test.go @@ -0,0 +1,80 @@ +package workspace + +import "testing" + +func TestExecUserFromMetadata(t *testing.T) { + tests := []struct { + name string + label string + want string + }{ + { + // The shape DevPod actually produces: an array of merged + // devcontainer metadata entries, one of which carries remoteUser. + name: "remoteUser in a multi-entry array", + label: `[{"id":"ghcr.io/devcontainers/features/common-utils:2"},{"id":"ghcr.io/devcontainers/features/git:1"},{"remoteUser":"vscode"},{"entrypoint":"/usr/local/share/docker-init.sh"}]`, + want: "vscode", + }, + { + name: "remoteUser wins over containerUser", + label: `[{"containerUser":"node"},{"remoteUser":"vscode"}]`, + want: "vscode", + }, + { + name: "containerUser is the fallback when no remoteUser is declared", + label: `[{"containerUser":"node"}]`, + want: "node", + }, + { + // Metadata entries merge with later ones overriding earlier. + name: "later entries override earlier ones", + label: `[{"remoteUser":"vscode"},{"remoteUser":"devuser"}]`, + want: "devuser", + }, + { + name: "an empty override does not clobber an earlier value", + label: `[{"remoteUser":"vscode"},{"remoteUser":""}]`, + want: "vscode", + }, + { + name: "a bare object is accepted as well as an array", + label: `{"remoteUser":"vscode"}`, + want: "vscode", + }, + { + name: "numeric uid", + label: `[{"remoteUser":"1000"}]`, + want: "1000", + }, + { + name: "user:group form", + label: `[{"remoteUser":"vscode:vscode"}]`, + want: "vscode:vscode", + }, + {name: "no user declared", label: `[{"id":"some-feature"}]`, want: ""}, + {name: "empty array", label: `[]`, want: ""}, + {name: "empty label", label: ``, want: ""}, + {name: "malformed json", label: `[{"remoteUser":`, want: ""}, + {name: "wrong value type", label: `[{"remoteUser":123}]`, want: ""}, + { + // The label is attacker-influenceable: a session member controls + // the devcontainer.json in their own repo. The value lands in an + // argv slot, so reject anything that isn't a plausible user spec + // rather than trusting it. + name: "leading dash rejected so it cannot read as a flag", + label: `[{"remoteUser":"--privileged"}]`, + want: "", + }, + {name: "whitespace rejected", label: `[{"remoteUser":"vscode root"}]`, want: ""}, + {name: "shell metacharacters rejected", label: `[{"remoteUser":"a;b"}]`, want: ""}, + {name: "path traversal rejected", label: `[{"remoteUser":"../root"}]`, want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := execUserFromMetadata(tt.label); got != tt.want { + t.Errorf("execUserFromMetadata(%q) = %q, want %q", tt.label, got, tt.want) + } + }) + } +} diff --git a/server/internal/workspace/manager.go b/server/internal/workspace/manager.go index b02054c..aa5aaab 100644 --- a/server/internal/workspace/manager.go +++ b/server/internal/workspace/manager.go @@ -65,6 +65,13 @@ type Manager struct { githubToken string gitOnce sync.Once gitEnv []string + + // userMu guards userCache, which memoizes ContainerUser lookups. + // VS Code Remote-SSH opens many channels per connection and each one + // resolves the exec user, so an uncached `docker inspect` per channel + // would add real latency to every terminal and port forward. + userMu sync.Mutex + userCache map[string]containerUserEntry } func NewManager(bin, provider, githubToken string) *Manager {