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 {