fix: use SSH_AGENT_FORWARDING context option in generated ssh config - #899
Conversation
The ~/.ssh/config entries Devsy writes hardcoded ForwardAgent yes, ignoring the SSH_AGENT_FORWARDING context option. Disabling agent forwarding in the context had no effect on the generated config, breaking SSH access for anyone who turned the option off.
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughSSH agent-forwarding settings now flow from workspace context into SSH configuration generation. Generated proxy-command and tunnel configurations emit ChangesSSH Agent Forwarding
Estimated code review effort: 2 (Simple) | ~10 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 images-devsy-sh canceled.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/ssh/config_test.go (1)
304-326: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover regeneration with forwarding disabled.
The new disabled case starts with an empty configuration. It does not verify that regeneration replaces an existing generated block containing
ForwardAgent yes. Add a case with an existing Devsy host for the same workspace, setagentForwardingtofalse, and assert that the regenerated block containsForwardAgent no.🤖 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/ssh/config_test.go` around lines 304 - 326, Add a regeneration test alongside the existing agent-forwarding cases in the test table, using an existing Devsy-generated host block for the same workspace that contains ForwardAgent yes, then set agentForwarding to false and assert the regenerated block replaces it with ForwardAgent no.pkg/ssh/config_tunnel_test.go (1)
172-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a disabled tunnel-mode test.
All tunnel fixtures set
agentForwardingtotrue. The disabled case inpkg/ssh/config_test.gocovers the proxy-command path only. Add a tunnel-mode case withagentForwarding: falseand assertForwardAgent no.🤖 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/ssh/config_tunnel_test.go` around lines 172 - 180, Add a disabled tunnel-mode case to TestBuildTunnelConfigLines using agentForwarding: false, and assert the generated configuration contains ForwardAgent no. Keep the existing enabled tunnel fixture and assertions unchanged.
🤖 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 `@pkg/ssh/config.go`:
- Around line 178-184: Update the SSH configuration insertion flow around
sshConfigBuilder.addSSHOptions and findInsertPosition so generated Devsy host
entries are placed before any earlier matching generic or override Host block,
ensuring their ForwardAgent value takes precedence. Add a fixture covering an
existing Host * ForwardAgent yes rule and verify the generated Devsy host block
is inserted ahead of it.
---
Nitpick comments:
In `@pkg/ssh/config_test.go`:
- Around line 304-326: Add a regeneration test alongside the existing
agent-forwarding cases in the test table, using an existing Devsy-generated host
block for the same workspace that contains ForwardAgent yes, then set
agentForwarding to false and assert the regenerated block replaces it with
ForwardAgent no.
In `@pkg/ssh/config_tunnel_test.go`:
- Around line 172-180: Add a disabled tunnel-mode case to
TestBuildTunnelConfigLines using agentForwarding: false, and assert the
generated configuration contains ForwardAgent no. Keep the existing enabled
tunnel fixture and assertions unchanged.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 579a33c6-2f6a-4a28-9347-75aea774a1ca
📒 Files selected for processing (4)
cmd/workspace/up/configure.gopkg/ssh/config.gopkg/ssh/config_test.gopkg/ssh/config_tunnel_test.go
| func (b *sshConfigBuilder) addSSHOptions(provider string, agentForwarding bool) *sshConfigBuilder { | ||
| forwardAgent := "no" | ||
| if agentForwarding { | ||
| forwardAgent = "yes" | ||
| } | ||
| b.lines = append(b.lines, | ||
| " ForwardAgent yes", | ||
| " ForwardAgent "+forwardAgent, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 20 'findInsertPosition|mergeSSHConfig|Host \*|ForwardAgent' --glob '*.go' .
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
cat > "$tmp" <<'EOF'
Host *
ForwardAgent yes
Host testhost
ForwardAgent no
EOF
if command -v ssh >/dev/null 2>&1; then
actual="$(ssh -G -F "$tmp" testhost 2>/dev/null | awk '$1 == "forwardagent" {print $2; exit}')"
test "$actual" = no
fiRepository: devsy-org/devsy
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages =="
find . -maxdepth 3 -type f -name 'go.mod' | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,120p" "$0"'
echo
echo "== relevant config.go section =="
sed -n '258,321p' pkg/ssh/config.go
echo
echo "== call sites for addHostSection / mergeSSHConfig / findInsertPosition =="
rg -n -C 3 'addHostSection|mergeSSHConfig|findInsertPosition|addHostParams' --glob '*.go' .
echo
echo "== fixture-style SSH precedence probe with ssh -G =="
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat > "$tmp/hostfile" <<'EOF'
Host *
ForwardAgent yes
Host testhost
ForwardAgent no
EOF
if command -v ssh >/dev/null 2>&1; then
echo "--- ssh -G hostfile prefix-only testhost ---"
actual="$(ssh -G -F "$tmp/hostfile" testhost 2>/dev/null | awk '$1 == "forwardagent" {print tolower($2); exit}')"
echo "forwardagent=$actual"
if [ "$actual" != "no" ]; then
echo "RESULT: ssh -G keeps ForwardAgent=yes for testhost when Host * precedes host-specific ForwardAgent no"
exit 3
else
echo "RESULT: ssh -G applied host-specific ForwardAgent=no"
fi
else
echo "ssh command unavailable"
fiRepository: devsy-org/devsy
Length of output: 17817
🌐 Web query:
OpenSSH ssh_config Host blocks first last more specific override ForwardAgent
💡 Result:
In OpenSSH configuration files, for each parameter, the first obtained value is used [1][2]. Because SSH parses the configuration file from top to bottom, this means that more specific Host or Match blocks should be placed at the beginning of the file, while more general or default configurations (such as those using Host *) should be placed at the end [3][1][4]. Regarding ForwardAgent and other configuration directives, if multiple Host or Match blocks match the target host, the first block encountered that defines a value for a specific parameter will take precedence [2][5]. Subsequent blocks that match the same host will not override that parameter [1][6][5]. To ensure a specific configuration (like ForwardAgent yes) applies to a specific host while having different settings for other hosts, you must structure your file so the specific host block appears before any broader blocks that might otherwise set a different value for the same parameter [2][4][5]. If you have a general block that sets a parameter you want to override for a specific host, you must either move the specific block above the general one or, if that is not possible, explicitly set the parameter in the specific block to the desired value (e.g., ForwardAgent yes) [2][5].
Citations:
- 1: https://manpages.debian.org/bookworm/openssh-client/ssh_config.5
- 2: https://unix.stackexchange.com/questions/804141/does-the-order-of-wildcard-host-blocks-matter-in-ssh-config
- 3: https://github.com/openssh/openssh-portable/blob/ad16a84e64a8cf1c69c63de3fb9008320a37009c/ssh_config.5
- 4: https://wiert.me/2020/06/12/ssh_config-section-order-is-important-the-first-setting-obtained-from-a-host-match-section-applies/
- 5: https://superuser.com/questions/1800419/ssh-config-matching-rules
- 6: https://serverfault.com/questions/1112330/ssh-config-file-correct-syntax-of-match-host-vs-host-to-utilize-actual-addre
Keep the generated Devsy host block before any earlier matching generic rule.
findInsertPosition inserts before the first existing Host line, so an existing Host * ForwardAgent yes block follows the generated Host testhost ForwardAgent no. Because SSH uses the first value for a matching option, ForwardAgent no is ignored and forwarding remains enabled. Add a fixture with an earlier generic Host * ForwardAgent yes, or prevent adding generic/override blocks before Devsy host entries.
🤖 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/ssh/config.go` around lines 178 - 184, Update the SSH configuration
insertion flow around sshConfigBuilder.addSSHOptions and findInsertPosition so
generated Devsy host entries are placed before any earlier matching generic or
override Host block, ensuring their ForwardAgent value takes precedence. Add a
fixture covering an existing Host * ForwardAgent yes rule and verify the
generated Devsy host block is inserted ahead of it.
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. A stacked PR containing fixes has been created.
Time taken: |
Summary
~/.ssh/configForwardAgentline was hardcoded toyes, ignoring theSSH_AGENT_FORWARDINGcontext option (already respected by devsy's own proxied SSH client).addSSHOptionsnow writesForwardAgent yes/nobased on the context setting, threaded throughSSHConfigParams→addHostParams.Fixes #898
Summary by CodeRabbit