fix(gpg): agent forwarding - #790
Merged
Merged
Conversation
GPG agent forwarding into the container points gpg at the forwarded host socket via a symlink (~/.gnupg/S.gpg-agent). Setup wrote only "use-agent" to gpg.conf, never "no-autostart". Whenever gpg ran while the forwarded socket was momentarily unavailable, it autostarted a local gpg-agent, which replaced the symlink with an empty local socket and permanently broke forwarding for the session — gpg -K showed nothing and signing failed with "No secret key". Write no-autostart (the GnuPG-recommended setting for agent forwarding) so gpg only ever uses the forwarded agent. Verified end to end: with the forwarded socket and no-autostart, gpg -K lists the secret keys and a commit is GPG-signed and verifies.
ForwardAgent launched the background gpg-forwarding SSH process once in a detached goroutine with no restart. A single dropped connection (e.g. a laptop roaming between networks) killed the forward permanently until the workspace was reopened, and interactive sessions that reuse the forwarded socket then broke too. Supervise the process: restart it whenever it exits, with exponential backoff (reset after a stable run), tied to the caller's context so it stops cleanly when the workspace is closed. Combined with no-autostart, gpg forwarding now self-heals after transient connectivity loss.
✅ Deploy Preview for images-devsy-sh canceled.
|
📝 WalkthroughWalkthroughGPG forwarding now accepts contexts, supervises and restarts its process, expands configuration and socket setup, and uses package-level agent operations. E2E coverage imports a controlled key and verifies forwarded secret-key access by fingerprint. ChangesGPG forwarding flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
|
The existing gpg-forwarding e2e only asserted the workspace boots with the flag set; it never ran gpg, so a broken forward passed. Add a functional check: import the test key on the host, up with --ssh-gpg-forwarding, then assert the container's forwarded agent exposes the secret key (the exact symptom that regressed). Replaces the unused, incorrect DevsySSHGpgTestKey.
- pass backoff bounds into superviseForward via a struct instead of test-mutable package globals - dedupe the gpg stdin-import logic (runGpgWithStdin) and wrap both import errors consistently - simplify SetupGpgConf (rely on O_CREATE instead of stat+create) - clarify IsGpgTunnelRunning (TrimSpace != "" instead of len > 1) - drop the pointless slice-spread on gpgconf --kill - correct the stale "hardlinks" comment (symlinks)
- convert StopGpgAgent/SetupGpgConf/gpgConfigPath (which never used the GPGConf receiver) to package functions; update call sites - build the su -c wrapper with shellescape.QuoteCommand instead of hand-rolled fmt.Sprintf quoting - extract appendMissingDirectives to keep SetupGpgConf within complexity
skevetter
changed the title
skevetter
marked this pull request as ready for review
July 28, 2026 00:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/framework/command.go (1)
483-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemote stderr is discarded, hiding forwarding failure details.
2>/dev/nulldrops gpg's own error output on the remote side, and the local stderr return value is discarded (_). If the fingerprint check fails, the resulting error only says "not reachable" with no clue why (agent not found vs. socket permission vs. no secret keys imported), making CI failures harder to diagnose.♻️ Suggested fix: capture stderr for debugging
- stdout, _, err := f.ExecCommandCapture(ctx, []string{ + stdout, stderr, err := f.ExecCommandCapture(ctx, []string{ cmdWorkspace, cmdSSH, names.Flag(names.AgentForwarding), names.Flag(names.SSHGPGForwarding), flagCommand, - "gpg --list-secret-keys --with-colons 2>/dev/null", + "gpg --list-secret-keys --with-colons", workspace, }) if err != nil { - return fmt.Errorf("list secret keys in container: %w", err) + return fmt.Errorf("list secret keys in container: %s: %w", stderr, err) }🤖 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 `@e2e/framework/command.go` around lines 483 - 507, Update DevsySSHGpgSecretKeyForwarded to preserve remote gpg diagnostics by removing the 2>/dev/null redirection and capturing the stderr returned by ExecCommandCapture. Include that stderr in the fingerprint-mismatch error alongside stdout, while retaining the existing command-error handling and successful fingerprint check.
🤖 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 `@e2e/tests/ssh/ssh.go`:
- Around line 25-26: Remove the committed GPG private-key fixture from the
testdata used by the SSH e2e flow. Update the setup around gpgTestKeyFingerprint
and the GPG forwarding test to generate a disposable key at runtime, or
otherwise exclude the private fixture from committed testdata while preserving
the test’s behavior.
---
Nitpick comments:
In `@e2e/framework/command.go`:
- Around line 483-507: Update DevsySSHGpgSecretKeyForwarded to preserve remote
gpg diagnostics by removing the 2>/dev/null redirection and capturing the stderr
returned by ExecCommandCapture. Include that stderr in the fingerprint-mismatch
error alongside stdout, while retaining the existing command-error handling and
successful fingerprint check.
🪄 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: ba8c8b73-1942-4426-8ee8-49c8822c3441
📒 Files selected for processing (8)
cmd/internal/agentworkspace/setup_gpg.goe2e/framework/command.goe2e/tests/ssh/ssh.gopkg/gpg/forward.gopkg/gpg/forward_test.gopkg/gpg/gpg_forwarding.gopkg/gpg/gpg_forwarding_test.gopkg/ide/opener/opener.go
Comment on lines
+25
to
+26
| gpgTestKeyFingerprint = "07F681B9FD6C3411F679BFD1F51769DB572DDD3F" | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether a static private key fixture is committed to the repo.
fd -i 'gpg-private' tests/ssh/testdataRepository: devsy-org/devsy
Length of output: 259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files count: '
git ls-files | wc -l
printf '\nCandidate ssh.go files:\n'
fd -i '^ssh\.go$|testdata$|gpg-private' . || true
printf '\nMatch gpg-private/fingerprint/gpg-forwarding:\n'
rg -n "gpg-private|gpg-forwarding|gpgTestKeyFingerprint|ImportGpgKey" . || trueRepository: devsy-org/devsy
Length of output: 2428
Don’t commit the GPG test private key fixture.
e2e/tests/ssh/testdata/gpg-forwarding/gpg-private.key is checked into the repo and imported by the e2e flow, so this shouldn’t be categorized as a secrets leak at line 25. Instead, flag this test fixture under the key fixture location as a cleanup: generate the disposable GPG key at setup time or exclude the private fixture from committed testdata.
🧰 Tools
🪛 Betterleaks (1.6.1)
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 `@e2e/tests/ssh/ssh.go` around lines 25 - 26, Remove the committed GPG
private-key fixture from the testdata used by the SSH e2e flow. Update the setup
around gpgTestKeyFingerprint and the GPG forwarding test to generate a
disposable key at runtime, or otherwise exclude the private fixture from
committed testdata while preserving the test’s behavior.
Source: Linters/SAST tools
Forwarding mirrored the host's socket path inside the container (e.g. /Users/<user>/.gnupg/... on a macOS host) and then symlinked to it. Those mirrored ancestor dirs are created root-owned, so a non-root workspace user cannot traverse them to reach the socket, and GPG forwarding silently failed (gpg -K empty, "No secret key"). The previous attempt to chmod the mirrored host dirs treated the symptom. Do what standard GPG agent forwarding does instead: forward the host's extra socket to a fixed container-local path the workspace user can reach, symlink gpg's own socket locations (~/.gnupg, /run/user/<uid>/gnupg) to it, and take ownership of the socket (the ssh server binds it as root). No host-path mirroring, no permission changes to OS directories. Run the gpg-forwarding e2e as a non-root user so this failure mode is covered (the container's home is likewise root-owned, reproducing it).
skevetter
marked this pull request as draft
July 28, 2026 01:18
skevetter
marked this pull request as ready for review
July 28, 2026 01:26
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workspace/ssh.go`:
- Around line 748-751: Update the ReverseForwardPorts mapping in the GPG
forwarding setup to use gpgExtraSocketPath as the host side and
gpg.ContainerSocketPath as the container side, preserving the A:B ordering
expected by ParsePortSpec.
🪄 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: 7e62fa29-8fe0-49d3-ac31-649d0c55f1be
📒 Files selected for processing (4)
cmd/workspace/ssh.goe2e/tests/ssh/testdata/gpg-forwarding/.devcontainer.jsone2e/tests/ssh/testdata/gpg-forwarding/devcontainer.jsonpkg/gpg/gpg_forwarding.go
Comment on lines
+748
to
+751
| cmd.ReverseForwardPorts = append( | ||
| cmd.ReverseForwardPorts, | ||
| gpg.ContainerSocketPath+":"+gpgExtraSocketPath, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ParsePortSpec's field-assignment order for "A:B" strings.
rg -n "func ParsePortSpec" -A 40 --type=go
rg -n "type Mapping struct" -A 15 --type=go
rg -n "ReverseForwardPorts" -B2 -A5 --type=go -g '!*_test.go'Repository: devsy-org/devsy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files around ssh/devssh/gpg/port:"
git ls-files | rg 'cmd/workspace/ssh\.go|devssh|gpg|port|setup_gpg|ssh\.go$' | sed -n '1,200p'
echo
echo "ssh.go outline around relevant functions:"
if [ -f cmd/workspace/ssh.go ]; then
wc -l cmd/workspace/ssh.go
rg -n "ReverseForwardPorts|ParsePortSpec|Mapping|setup-gpg|ReversePortForward|Reverse forwarding" cmd/workspace/ssh.go
fiRepository: devsy-org/devsy
Length of output: 3381
🏁 Script executed:
#!/bin/bash
set -u
echo "pkg/port/parse.go:"
sed -n '1,220p' pkg/port/parse.go
echo
echo "pkg/port/port.go:"
sed -n '1,180p' pkg/port/port.go
echo
echo "cmd/workspace/ssh.go relevant sections:"
sed -n '1,130p' cmd/workspace/ssh.go
sed -n '430,470p' cmd/workspace/ssh.go
sed -n '730,770p' cmd/workspace/ssh.go
echo
echo "cmd/internal/agentworkspace/setup_gpg.go:"
sed -n '1,220p' cmd/internal/agentworkspace/setup_gpg.go
echo
echo "pkg/gpg/gpg_forwarding.go relevant socket symlink section:"
sed -n '130,200p' pkg/gpg/gpg_forwarding.go
echo
echo "Behavioral check of ParsePortSpec assignments for simple A:B strings:"
python3 - <<'PY'
from pathlib import Path
src = Path("pkg/port/parse.go").read_text()
print("ParsePortSpec source excerpt:")
start = src.index("func ParsePortSpec(s string)")
print(src[start:].split("func ")[0][:2200])
PYRepository: devsy-org/devsy
Length of output: 16493
Reverse-forward mapping order is correct for GPG forwarding.
ParsePortSpec assigns a 2-part string A:B to mapping.Host.Address = A and mapping.Container.Address = B, so the current gpg.ContainerSocketPath+":"+gpgExtraSocketPath would bind Host = /tmp/S.gpg-agent and Container = detected host socket path, which is the wrong direction. Keep the suggested ordering gpgExtraSocketPath+":"+gpg.ContainerSocketPath if this is still present.
🤖 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/workspace/ssh.go` around lines 748 - 751, Update the ReverseForwardPorts
mapping in the GPG forwarding setup to use gpgExtraSocketPath as the host side
and gpg.ContainerSocketPath as the container side, preserving the A:B ordering
expected by ParsePortSpec.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates all GPG agent-forwarding fixes. Supersedes #789.
Problems
gpg -Kempty,No secret key, even though the host held the keys and the socket was forwarded.Summary by CodeRabbit
Summary of updates
New Features
Bug Fixes
Tests