fix(gpg): resolve tmp file collision breaking GPG forwarding for browser IDEs - #869
fix(gpg): resolve tmp file collision breaking GPG forwarding for browser IDEs#869skevetter wants to merge 16 commits into
Conversation
Required to keep the tree building after Task 1's CreateSSHCommand signature change; full Task 2 scope (tests, browser_tunnel.go audit) is out of scope for this task.
…ith non-root remoteUser
…ustification acquireGPGSetupLock's os.Chmod(0o666) ran unconditionally, so a non-owning second acquirer (e.g. a different container user than the one that created the lock file) would get EPERM even when the mode was already correct. Skip the chmod when the file's mode already matches, and only attempt (and propagate errors from) it when a real change is needed.
Resolves revive's argument-limit without a nolint suppression: the function's 5 positional args (3 of them same-typed strings) become one sshCommandArgsParams struct, removing the transposition hazard along with the lint finding.
Reasoning is preserved (multi-user /tmp collision, umask vs chmod ownership semantics, EPERM-on-redundant-chmod) — condensed to the essential point rather than removed.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesNon-root workspace support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant setupBrowserIDE
participant BrowserTunnel
participant SSHServer
participant GPGForwarding
setupBrowserIDE->>BrowserTunnel: Start browser IDE with vscode user
BrowserTunnel->>SSHServer: Create SSH command for vscode
SSHServer->>GPGForwarding: Forward the GPG connection
GPGForwarding-->>setupBrowserIDE: Verify the forwarded secret key
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
skevetter
changed the title
…ode assertion os.Chmod follows symlinks; the GPG setup lock path is a fixed, predictable, world-writable /tmp path, so a symlink planted there could redirect the chmod onto an arbitrary root-owned file. lockFileNeedsChmod now Lstats and refuses to proceed if the path is a symlink. Also compares the activity-file test's mode assertion against the literal 0o666 instead of the production constant it's meant to be checking. Addresses CodeRabbit findings on PR #869.
A lock file left behind by a pre-fix binary (or created by a different user) could sit at 0600. flock's own open() would EACCES on it before acquireGPGSetupLock's existing post-lock chmod logic ever ran, wedging a non-owning acquirer exactly like the original bug this branch fixes. widenStaleLockFile runs before the flock attempt: it chmods a restrictive existing lock file to 0666, escalating to sudo when the current process doesn't own it. A sudo failure is non-fatal — the subsequent flock attempt surfaces a clearer error if widening didn't help. Addresses the Major follow-up noted on PR #869's CodeRabbit review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/internal/agentworkspace/setup_gpg.go`:
- Around line 162-189: Update widenStaleLockFile to accept a context and invoke
the sudo chmod fallback through exec.CommandContext with a bounded timeout,
including sudo’s non-interactive -n option so unavailable credentials fail
immediately. Update its call site to pass the existing context while preserving
the current non-fatal logging and error behavior.
In `@pkg/tunnel/browser_test.go`:
- Around line 66-155: Add the missing testCtxName declaration in
pkg/tunnel/browser_test.go before the helper functions baseSSHArgs and
baseParams use it, using the existing test constant/declaration style and the
context value expected by these tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c3362ead-0eed-492b-ab6d-489f9e0c386c
📒 Files selected for processing (10)
cmd/internal/agentworkspace/setup_gpg.gocmd/internal/agentworkspace/setup_gpg_test.gocmd/internal/ssh_server_test.goe2e/tests/ide/browser_returns.goe2e/tests/ide/testdata-gpg-nonroot/.devcontainer.jsonpkg/gpg/forward_test.gopkg/ide/opener/browser_tunnel_test.gopkg/ide/opener/opener.gopkg/tunnel/browser.gopkg/tunnel/browser_test.go
| func widenStaleLockFile() error { | ||
| needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("stat lock file: %w", err) | ||
| } | ||
| if !needsChmod { | ||
| return nil | ||
| } | ||
|
|
||
| // #nosec G302 -- 0666 is intentional; see acquireGPGSetupLock. | ||
| if err := os.Chmod(gpgSetupLockPath, 0o666); err == nil { | ||
| return nil | ||
| } else if !errors.Is(err, fs.ErrPermission) { | ||
| return fmt.Errorf("widen stale lock file: %w", err) | ||
| } | ||
|
|
||
| //nolint:gosec // gpgSetupLockPath is a fixed path, not user input | ||
| if err := exec.Command("sudo", "chmod", "0666", gpgSetupLockPath).Run(); err != nil { | ||
| log.Debugf( | ||
| "sudo chmod stale gpg setup lock (non-fatal, flock will surface the real error): %v", | ||
| err, | ||
| ) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'setup_gpg\.go$|agentworkspace' || true
echo "== target file outline/sections =="
target="cmd/internal/agentworkspace/setup_gpg.go"
if [ -f "$target" ]; then
wc -l "$target"
ast-grep outline "$target" || true
sed -n '1,240p' "$target" | cat -n
fi
echo "== call sites and context usage =="
rg -n "widenStaleLockFile|acquireGPGSetupLock|lockCtx|WithTimeout|CommandContext|sudo.*chmod" cmd/internal/agentworkspace -S || trueRepository: devsy-org/devsy
Length of output: 13461
🌐 Web query:
Go os/exec CommandContext Cancel returns context canceled error documentation
💡 Result:
When using exec.CommandContext in Go, if the context is canceled, the command's Cancel function is invoked [1][2]. By default, this function calls cmd.Process.Kill() [1][3]. Consequently, Wait and similar methods (like Run) typically return an error related to the process being killed (e.g., signal: killed) rather than a simple context.Canceled error [4]. According to the official os/exec documentation, if the command exits with a success status after Cancel is called, and Cancel does not return an error equivalent to os.ErrProcessDone, then Wait will return a non-nil error wrapping either the error returned by Cancel or the context's error [1][5]. However, in practice, because the default behavior involves killing the process, the error returned to the caller is often the exit error of the process itself [4]. Key points regarding this behavior: - Default Cancellation: CommandContext defaults to cmd.Process.Kill(), which results in the process being terminated by a signal [1][6]. - Error Reporting: Users often find that the returned error is signal: killed rather than the context's error [4]. This is because the process termination is treated as the primary outcome of the command's execution once the context is triggered [7][4]. - Customization: You can change this behavior by providing a custom Cancel function or modifying the command fields before starting it [1][2]. - Context Inspection: If you need to distinguish between a context cancellation and other causes of termination, you can check ctx.Err() explicitly after receiving an error from Run() or Wait() [7]. This behavior has been the subject of discussion in the Go community regarding whether it should explicitly return context.Canceled, but the current implementation maintains the signal-based exit error [4].
Citations:
- 1: https://pkg.go.dev/os/exec
- 2: https://github.com/golang/go/blob/master/src/os/exec/exec.go
- 3: https://pkg.go.dev/os/exec?GOOS=windows
- 4: os/exec: CommandContext should return "context canceled" when the context is canceled golang/go#69585
- 5: https://github.com/golang/go/blob/go1.26.5/src/os/exec/exec.go
- 6: https://tip.golang.org/pkg/exec/
- 7: os/exec: CommandContext does not forward the context's error on timeout golang/go#21880
Bound the sudo chmod fallback with a context timeout.
widenStaleLockFile is called before lockCtx starts, and exec.Command(...).Run() can wait indefinitely. If sudo is not passwordless or authentication needs an interactive prompt/network check, the 30-second lock timeout will be exceeded before TryLockContext can run.
Use a bounded context for this call, and prefer -n so sudo fails immediately when no cached credentials are available.
🔒️ Proposed fix to bound the sudo fallback with a timeout
-func widenStaleLockFile() error {
+func widenStaleLockFile(ctx context.Context) error {
needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666)
if err != nil {
if os.IsNotExist(err) {
return nil
}
return fmt.Errorf("stat lock file: %w", err)
}
if !needsChmod {
return nil
}
// `#nosec` G302 -- 0666 is intentional; see acquireGPGSetupLock.
if err := os.Chmod(gpgSetupLockPath, 0o666); err == nil {
return nil
} else if !errors.Is(err, fs.ErrPermission) {
return fmt.Errorf("widen stale lock file: %w", err)
}
+ sudoCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
+ defer cancel()
//nolint:gosec // gpgSetupLockPath is a fixed path, not user input
- if err := exec.Command("sudo", "chmod", "0666", gpgSetupLockPath).Run(); err != nil {
+ if err := exec.CommandContext(sudoCtx, "sudo", "-n", "chmod", "0666", gpgSetupLockPath).Run(); err != nil {
log.Debugf(
"sudo chmod stale gpg setup lock (non-fatal, flock will surface the real error): %v",
err,
)
}
return nil
}This also requires updating the call site to pass ctx:
- if err := widenStaleLockFile(); err != nil {
+ if err := widenStaleLockFile(ctx); err != nil {
return nil, err
}🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 174-174: File mode grants world-writable permission; restrict the mode so other users cannot modify the file (e.g. 0o644 / 0o600).
Context: os.Chmod(gpgSetupLockPath, 0o666)
Note: [CWE-276] Incorrect Default Permissions.
(world-writable-chmod-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/internal/agentworkspace/setup_gpg.go` around lines 162 - 189, Update
widenStaleLockFile to accept a context and invoke the sudo chmod fallback
through exec.CommandContext with a bounded timeout, including sudo’s
non-interactive -n option so unavailable credentials fail immediately. Update
its call site to pass the existing context while preserving the current
non-fatal logging and error behavior.
| func baseSSHArgs(ctx, user, ws string) []string { | ||
| return []string{ | ||
| "workspace", "ssh", "--user=root", "--agent-forwarding=false", | ||
| "workspace", "ssh", "--user=" + user, "--agent-forwarding=false", | ||
| "--start-services=false", "--context", ctx, ws, | ||
| } | ||
| } | ||
|
|
||
| func baseParams(user string) sshCommandArgsParams { | ||
| return sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, user: user, | ||
| } | ||
| } | ||
|
|
||
| func TestBuildSSHCommandArgs(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| context string | ||
| workspace string | ||
| debug bool | ||
| extraArgs []string | ||
| expected []string | ||
| name string | ||
| params sshCommandArgsParams | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "basic", context: "default", workspace: "my-workspace", | ||
| expected: baseSSHArgs("default", "my-workspace"), | ||
| name: "basic root user", | ||
| params: baseParams(testUserRoot), | ||
| expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "non-root workspace user", | ||
| params: baseParams(testUserVSCode), | ||
| expected: baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "empty user falls back to root", | ||
| params: baseParams(""), | ||
| expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), | ||
| }, | ||
| { | ||
| name: "with debug", context: "default", workspace: "my-workspace", | ||
| debug: true, | ||
| expected: append(baseSSHArgs("default", "my-workspace"), "--debug"), | ||
| name: "with debug", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, | ||
| user: testUserVSCode, debug: true, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", | ||
| ), | ||
| }, | ||
| { | ||
| name: "with extra args", context: "prod", workspace: "ws", | ||
| extraArgs: []string{"--stdio", "--log-output=raw"}, | ||
| expected: append(baseSSHArgs("prod", "ws"), "--stdio", "--log-output=raw"), | ||
| name: "with extra args", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: "prod", workspace: "ws", user: testUserVSCode, | ||
| extraArgs: []string{"--stdio", "--log-output=raw"}, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs("prod", testUserVSCode, "ws"), "--stdio", "--log-output=raw", | ||
| ), | ||
| }, | ||
| { | ||
| name: "with debug and extra args", context: "default", workspace: "my-workspace", | ||
| debug: true, extraArgs: []string{"--stdio"}, | ||
| expected: append(baseSSHArgs("default", "my-workspace"), "--debug", "--stdio"), | ||
| name: "with debug and extra args", | ||
| params: sshCommandArgsParams{ | ||
| clientContext: testCtxName, workspace: testWorkspaceName, user: testUserVSCode, | ||
| debug: true, extraArgs: []string{"--stdio"}, | ||
| }, | ||
| expected: append( | ||
| baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", "--stdio", | ||
| ), | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := buildSSHCommandArgs(tt.context, tt.workspace, tt.debug, tt.extraArgs) | ||
| got := buildSSHCommandArgs(tt.params) | ||
| assert.Equal(t, tt.expected, got) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestBuildBackhaulCmd_UsesResolvedRemoteUser(t *testing.T) { | ||
| writer := &bytes.Buffer{} | ||
| cmd := buildBackhaulCmd(context.Background(), backhaulCmdParams{ | ||
| execPath: "/usr/bin/true", | ||
| remoteUser: testUserVSCode, | ||
| client: fakeWorkspaceClient{}, | ||
| authSockID: "sock123", | ||
| writer: writer, | ||
| }) | ||
|
|
||
| joined := strings.Join(cmd.Args, " ") | ||
| assert.Contains(t, joined, "--user vscode", | ||
| "backhaul connection must use the resolved workspace user, not root, "+ | ||
| "so it doesn't fight the primary tunnel's ssh-server/setup-gpg sessions "+ | ||
| "over the shared /tmp coordination files") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm testCtxName is declared in pkg/tunnel/browser_test.go.
rg -n 'testCtxName' pkg/tunnel/browser_test.goRepository: devsy-org/devsy
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Inspect the test file declarations and all references to testCtxName.
set -euo pipefail
echo "Declaration / reference occurrences in pkg/tunnel/browser_test.go:"
rg -n '\b(testCtxName|testUserRoot|testUserVSCode|testWorkspaceName)\b' pkg/tunnel/browser_test.go || true
echo
echo "Top of file:"
sed -n '1,110p' pkg/tunnel/browser_test.goRepository: devsy-org/devsy
Length of output: 4270
Define testCtxName before using it.
baseSSHArgs and baseParams reference testCtxName, but pkg/tunnel/browser_test.go does not declare it. Add the missing declaration to avoid a compile-time failure for this test file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/tunnel/browser_test.go` around lines 66 - 155, Add the missing
testCtxName declaration in pkg/tunnel/browser_test.go before the helper
functions baseSSHArgs and baseParams use it, using the existing test
constant/declaration style and the context value expected by these tests.
Without a context or sudo -n, a sudo prompt for credentials could block acquireGPGSetupLock indefinitely, defeating gpgSetupLockTimeout entirely. exec.CommandContext ties the subprocess to the caller's existing lock timeout; -n makes sudo fail immediately instead of prompting. Addresses a CodeRabbit finding on PR #869. The paired finding (missing testCtxName in pkg/tunnel/browser_test.go) is a false positive: the constant is declared in the same package's services_test.go:13 and the package already builds and vets clean.
1 participant
Summary
Browser-based IDE workspaces (openvscode, code-server, vscode-web, jupyter, marimo) opened two independent SSH sessions into the dev container as different OS users: the primary browser-IDE tunnel was hardcoded to
root, while the GPG-agent-forwarding tunnel correctly resolved and used the workspace'sremoteUser. Both sessions wrote to the same owner-exclusive files under/tmp(devsy-gpg-setup.lock,devsy.activity), so whichever session's user created the file first locked the other out withEACCES/EPERM— surfacing aspermission denied,operation not permitted, and ultimately "GPG agent forwarding failed ... continuing without it" for any browser IDE against a devcontainer with a non-rootremoteUser.buildSSHCommandArgs/CreateSSHCommand, plus thestartFleetcall site), so it matches the GPG-forwarding tunnel's user./tmp/devsy-gpg-setup.lockworld-lockable (0o666) as defense-in-depth, with a stat-first check so a non-owning second acquirer doesn't hit a redundant, EPERM-pronechmod.0o666.--ssh-gpg-forwardingagainst aremoteUser-set devcontainer) — independently run against real Docker containers and confirmed passing.Test plan
go build ./...,go vet ./...,go test ./...all pass.golangci-lint run --new-from-rev=<base> ./...reports 0 new issues.ide/sshlabels pass, including the new regression spec and the pre-existing GPG specs.--ide=vscode-web --ide-launch=headless --ssh-gpg-forwardingagainst aremoteUser-set devcontainer):gpg -Kinside the container lists the forwarded key with zero occurrences of the three failure substrings.Summary by CodeRabbit