Fix SSH relay failure when config has LocalForward/DynamicForward directives#2635
Fix SSH relay failure when config has LocalForward/DynamicForward directives#2635bIackr0se wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
…ectives The reverse relay SSH fallback (used when ControlMaster dynamic forwarding is unavailable) spawns a standalone SSH connection with -S none. This connection reads ~/.ssh/config and attempts to establish ALL configured forwards (LocalForward, DynamicForward, RemoteForward), which conflict with ports already bound by the primary SSH connection. With ExitOnForwardFailure=yes, the relay process exits before the RemoteForward can be established. Fix: Use -F /dev/null to bypass ~/.ssh/config entirely for the standalone relay connection. Transport-critical options (ProxyJump, ProxyCommand, IdentityFile, HostKeyAlgorithms, KexAlgorithms, Ciphers, etc.) are resolved via ssh -G and passed explicitly, preserving connectivity without config-file forwards. ProxyJump hosts are also resolved via ssh -G so that Host aliases work even though -F /dev/null is propagated to jump host SSH processes. Additionally, add controlpath to the backgroundSSHOptions filter (both static and instance versions). Previously, -o ControlPath=... could appear after -S none in the relay command, which on macOS can re-enable mux attachment despite the explicit -S none. Fixes manaflow-ai#2596
|
@bIackr0se is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
📝 WalkthroughWalkthroughAdded Changes
Sequence DiagramsequenceDiagram
participant Controller as Workspace Controller
participant SSHProbe as ssh -G Probe
participant SSHRelay as Reverse Relay SSH
participant RemoteHost as Remote Host
Controller->>SSHProbe: Run `ssh -G <host>` (5s timeout) and per ProxyJump hop
alt Probe succeeds
SSHProbe-->>Controller: Parsed transport options (ProxyJump, IdentityFile, HostKey/KEX/Cipher, StrictHostKeyChecking...)
Controller->>Controller: Build explicit relay args, resolve jump chain hop-by-hop
else Probe fails
SSHProbe-->>Controller: Error/timeout
Controller->>Controller: Fall back to explicit port/identity options only
end
Controller->>SSHRelay: Spawn `ssh -F /dev/null -S none` + resolved args + `-R` reverse forward
SSHRelay->>RemoteHost: Establish standalone reverse connection (no user config forwards)
RemoteHost-->>SSHRelay: Reverse forward established
SSHRelay-->>Controller: Relay ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR fixes SSH relay failures caused by config-file forwarding directives (
Confidence Score: 4/5Do not merge until the port-dropping regression is fixed; the relay silently connects on port 22 for all users with a non-default port when ssh -G succeeds. One clear P1 regression in parseRelayTransportOptions: the configured port is skipped but never re-added, breaking any non-default-port setup in the happy path. The rest of the implementation — -F /dev/null bypass, ProxyJump resolution, controlpath filter — is well-structured and directly addresses the reported failure modes. Sources/Workspace.swift — specifically the port case in parseRelayTransportOptions (line 4101) and the overall resolvedRelayTransportOptions return path. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmux App
participant SSHg as ssh -G (probe)
participant Parse as parseRelayTransportOptions
participant Relay as ssh Relay Process
App->>SSHg: ssh -G [-p port] [-i key] destination
SSHg-->>App: hostname, user, port, proxyjump,<br/>identityfile, hostkeyalgorithms, ...
App->>Parse: parse ssh -G stdout
Parse-->>App: [-o Hostname=...] [-J resolved] [-i ...]<br/>[-o HostKeyAlgorithms=...]
Note over Parse,App: ⚠️ port case skipped when<br/>configuration.port != nil<br/>but never re-added elsewhere
App->>Relay: ssh -N -T -S none -F /dev/null<br/><resolved options> -o ExitOnForwardFailure=yes<br/>-R 127.0.0.1:PORT:127.0.0.1:LOCAL destination
Relay-->>App: reverse forward established (or fails on port 22)
Reviews (1): Last reviewed commit: "Fix SSH relay failure when config has Lo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "port": | ||
| // Only add if not already set via configuration.port | ||
| if configuration.port == nil, let portNum = Int(value), portNum != 22 { | ||
| args += ["-p", value] | ||
| } |
There was a problem hiding this comment.
configuration.port is set
The comment says the port is "already set via configuration.port," but it never is — neither resolvedRelayTransportOptions nor reverseRelayArguments re-adds it. Any user with a non-default port (e.g. port 2222) whose ssh -G succeeds will get a relay command that omits -p entirely and defaults to port 22, causing the relay connection to fail. The fallback path (explicitRelayTransportOptions) correctly includes the port, so only the happy path is broken.
| case "port": | |
| // Only add if not already set via configuration.port | |
| if configuration.port == nil, let portNum = Int(value), portNum != 22 { | |
| args += ["-p", value] | |
| } | |
| case "port": | |
| // ssh -G reports the effective port. Prefer the explicitly configured port | |
| // (it was already passed to the ssh -G probe), then fall back to the | |
| // resolved value if it differs from the SSH default. | |
| let effectivePort = configuration.port.map(String.init) ?? value | |
| if let portNum = Int(effectivePort), portNum != 22 { | |
| args += ["-p", effectivePort] | |
| } |
Sources/Workspace.swift
Outdated
| "-o", "ConnectTimeout=6", | ||
| "-o", "ServerAliveInterval=20", | ||
| "-o", "ServerAliveCountMax=2", | ||
| "-o", "StrictHostKeyChecking=accept-new", |
There was a problem hiding this comment.
StrictHostKeyChecking=accept-new is a silent TOFU downgrade
With -F /dev/null the relay falls back to system known-hosts files (or those resolved from ssh -G). For a host that has never been seen before, accept-new auto-trusts the key without any warning. The user's original config may have had StrictHostKeyChecking=yes or ask; this override silently weakens it. Consider propagating the user's StrictHostKeyChecking value from the ssh -G output (it's not in relayTransportKeys), or leaving StrictHostKeyChecking unset so the resolved userknownhostsfile + system policy apply naturally.
![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
🧹 Nitpick comments (1)
Sources/Workspace.swift (1)
1073-1077: Consider sharing the background SSH control-option denylist.
controlpathnow has to stay in sync in bothWorkspaceRemoteSSHBatchCommandBuilderandWorkspaceRemoteSessionController. Pulling that set into one shared helper/constant would make the next mux-option fix much harder to miss.Also applies to: 4288-4297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 1073 - 1077, The denylist set batchSSHControlOptionKeys is duplicated across WorkspaceRemoteSSHBatchCommandBuilder and WorkspaceRemoteSessionController; extract this Set<String> into a single shared constant or helper (e.g., a private static let in a new SSHConstants or Workspace+SSH extension) and replace both usages to reference that single symbol so controlpath (and other control options) stay in sync across both builders/controllers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 4112-4117: The current ProxyJump reconstruction (called from the
"proxyjump" case using resolveProxyJumpChain) drops per-hop SSH config because
the parser only extracts hostname, user and port (and uses a default: break),
and it also fails to bracket IPv6 literals when a non-default port is present;
update resolveProxyJumpChain (and the parsing logic that iterates ssh -G output)
to preserve all per-host fields (e.g., IdentityFile, CertificateFile,
ProxyCommand, UserKnownHostsFile, HostKeyAlias, HostKeyAlgorithms, etc.) when
rebuilding each hop instead of discarding unknown keys, and when formatting a
hop include IPv6 brackets around the host part whenever the host contains ":"
and a non-default port is appended (format user@[ipv6]:port); ensure the rebuilt
jump spec passes through hop-specific options so each spawned jump process
receives its per-hop config.
- Around line 4019-4026: The relayTransportKeys whitelist used by
parseRelayTransportOptions must include the missing keys so user SSH config
values pass through: add "stricthostkeychecking", "certificatefile", and
"identityagent" to the relayTransportKeys set/array; update
parseRelayTransportOptions/resolvedRelayTransportOptions usage so these keys are
preserved when filtering ssh -G output. Change the hard-coded "-o
StrictHostKeyChecking=accept-new" in resolvedRelayTransportOptions (or where
args is built) to only append that option if no StrictHostKeyChecking value was
obtained from parseRelayTransportOptions/ssh -G. Also broaden
explicitRelayTransportOptions fallback to consider and return CertificateFile
and IdentityAgent when ssh -G fails so those options are recovered.
---
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 1073-1077: The denylist set batchSSHControlOptionKeys is
duplicated across WorkspaceRemoteSSHBatchCommandBuilder and
WorkspaceRemoteSessionController; extract this Set<String> into a single shared
constant or helper (e.g., a private static let in a new SSHConstants or
Workspace+SSH extension) and replace both usages to reference that single symbol
so controlpath (and other control options) stay in sync across both
builders/controllers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98db35f5-8dfb-47ef-9550-5d94b5e33da3
📒 Files selected for processing (2)
Sources/Workspace.swiftdocs/remote-daemon-spec.md
Fix P1 port regression: when configuration.port is set, the ssh -G resolved port was skipped but never re-added. Now uses configuration.port as the effective port with ssh -G value as fallback. Fix P2 StrictHostKeyChecking TOFU downgrade: no longer hard-codes accept-new when the user's SSH config has an explicit policy. The resolved value from ssh -G is propagated; accept-new is only applied as a default when no policy was configured. Fix IPv6 ProxyJump formatting: bracket IPv6 literals when a non-default port is appended (user@[2001:db8::1]:2222 instead of the malformed user@2001:db8::1:2222). Add missing transport keys: certificatefile, identityagent, and stricthostkeychecking are now included in the relayTransportKeys whitelist so they pass through from ssh -G output.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/Workspace.swift (2)
4036-4039:⚠️ Potential issue | 🟠 MajorKeep explicit
StrictHostKeyCheckingon thessh -Gfallback path.When the probe fails, the fallback only carries port/identity and hard-codes
hasStrictHostKeyChecking: false, so an explicitStrictHostKeyChecking=yes|noinconfiguration.sshOptionsgets replaced byaccept-new. That silently changes the user's host-key policy on the very path that's supposed to be the conservative fallback.Also applies to: 4066-4073, 4211-4220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 4036 - 4039, The fallback path currently unconditionally injects "-o StrictHostKeyChecking=accept-new" when resolved.hasStrictHostKeyChecking is false, which overwrites an explicit user value from configuration.sshOptions; change the check so you only append the default if StrictHostKeyChecking was neither resolved nor explicitly provided by the user (i.e., `if !resolved.hasStrictHostKeyChecking && !configuration.sshOptions.containsKey("StrictHostKeyChecking")` or equivalent), preserving any explicit "yes" or "no" the user set; apply the same fix to the other fallback sites that use resolved.hasStrictHostKeyChecking (the blocks around the other two occurrences).
4166-4203:⚠️ Potential issue | 🟠 Major
ProxyJumphop-specific transport config is still being dropped.
resolveProxyJumpChain(_:)collapses each hop touser@host:port, so the newly whitelisted auth/host-key options only apply to the final destination. If OpenSSH propagates the outer-F /dev/nullinto the implicit jump SSH processes, hop-specificIdentityFile,CertificateFile,HostKeyAlias,UserKnownHostsFile,ProxyCommand, etc. still vanish here, and bastion chains that rely on per-hop config will keep failing.In OpenSSH, when the outer command uses `ssh -F /dev/null -J hop1,hop2 target`, do the implicit jump SSH processes also ignore `~/.ssh/config`, and can hop-specific settings like `IdentityFile`, `CertificateFile`, `HostKeyAlias`, or `UserKnownHostsFile` be represented in the `-J` string itself?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 4166 - 4203, resolveProxyJumpChain(_:) currently collapses each hop to user@host:port after running ssh -G, which drops hop-specific transport options; change the logic in the hops.map closure (the block that calls sshExec(arguments: ["-G", hop], timeout: 5) and parses result.stdout) to detect any presence of per-hop config keys (e.g. "identityfile", "certificatefile", "hostkeyalias", "userknownhostsfile", "proxycommand", "localforward", etc.) in the ssh -G output and if any are present, do not replace the original hop string (return hop) so per-hop config is preserved; only perform the collapsing to hostname/user/port when ssh -G yields hostname/user/port and none of those hop-specific keys are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 1073-1077: The current batchSSHControlOptionKeys set includes
"controlpath", causing batch-mode helpers (batchSSHControlOptionKeys) to strip
ControlPath and breaking reverseRelayControlMasterArguments and related batch
SSH/SCP helpers that need ControlPath to reuse the existing control socket;
remove "controlpath" from the batchSSHControlOptionKeys set and, if the relay
case still requires stripping ControlPath for the standalone relay (-S none),
create a separate relay-only filter (e.g., relaySSHControlOptionKeys) used only
by the relay code path so reverseRelayControlMasterArguments and batch helpers
keep ControlPath intact.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 4036-4039: The fallback path currently unconditionally injects "-o
StrictHostKeyChecking=accept-new" when resolved.hasStrictHostKeyChecking is
false, which overwrites an explicit user value from configuration.sshOptions;
change the check so you only append the default if StrictHostKeyChecking was
neither resolved nor explicitly provided by the user (i.e., `if
!resolved.hasStrictHostKeyChecking &&
!configuration.sshOptions.containsKey("StrictHostKeyChecking")` or equivalent),
preserving any explicit "yes" or "no" the user set; apply the same fix to the
other fallback sites that use resolved.hasStrictHostKeyChecking (the blocks
around the other two occurrences).
- Around line 4166-4203: resolveProxyJumpChain(_:) currently collapses each hop
to user@host:port after running ssh -G, which drops hop-specific transport
options; change the logic in the hops.map closure (the block that calls
sshExec(arguments: ["-G", hop], timeout: 5) and parses result.stdout) to detect
any presence of per-hop config keys (e.g. "identityfile", "certificatefile",
"hostkeyalias", "userknownhostsfile", "proxycommand", "localforward", etc.) in
the ssh -G output and if any are present, do not replace the original hop string
(return hop) so per-hop config is preserved; only perform the collapsing to
hostname/user/port when ssh -G yields hostname/user/port and none of those
hop-specific keys are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…tKeyChecking fallback Revert controlpath from batchSSHControlOptionKeys (both static and instance). Adding it there broke the preferred ControlMaster path and batch SSH/SCP helpers that need ControlPath to attach to the existing control socket. The original secondary bug (ControlPath leaking into the standalone relay command) is now moot since the relay uses -F /dev/null and builds args from ssh -G output, not sshCommonArguments. Harden StrictHostKeyChecking fallback: when ssh -G fails and we fall back to explicit options, also check configuration.sshOptions for an explicit StrictHostKeyChecking value before defaulting to accept-new. This prevents silently downgrading a user's host-key policy on the conservative fallback path. Document ProxyJump per-hop config limitation: the -J string format cannot carry hop-specific IdentityFile, CertificateFile, or HostKeyAlias. The preferred ControlMaster path does not have this limitation. The SSH agent handles authentication for most cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
4035-4039:⚠️ Potential issue | 🟠 MajorKeep relay-safe explicit SSH options in the
ssh -Gfallback.When
ssh -Gfails,explicitRelayTransportOptions()only replays-p/-i. That drops explicit CLI options likeUser,StrictHostKeyChecking,ProxyJump/ProxyCommand,HostKeyAlias,IdentitiesOnly, and known-hosts overrides, so this fallback no longer preserves the old standalone-relay behavior. The check at Lines 4037-4039 only suppressesaccept-new; it still loses the user's actual host-key policy.Suggested fix
private func explicitRelayTransportOptions() -> [String] { var args: [String] = [] if let port = configuration.port { args += ["-p", String(port)] } if let identityFile = configuration.identityFile, !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { args += ["-i", identityFile] } + let relayDisallowedOptionKeys: Set<String> = [ + "controlmaster", + "controlpersist", + "controlpath", + ] + for option in normalizedSSHOptions(configuration.sshOptions) { + guard let key = sshOptionKey(option), + !relayDisallowedOptionKeys.contains(key) else { continue } + args += ["-o", option] + } return args }Also applies to: 4067-4074, 4219-4231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 4035 - 4039, explicitRelayTransportOptions() currently only replays -p/-i when ssh -G fails, losing user-provided SSH options like User, StrictHostKeyChecking, ProxyJump/ProxyCommand, HostKeyAlias, IdentitiesOnly and known-hosts overrides; update that function so when resolved.* flags (e.g., resolved.hasStrictHostKeyChecking) do not indicate a resolved value, you scan configuration.sshOptions (using hasSSHOptionKey and the same sshOptions parsing helpers) and append "-o", "Key=Value" entries for any of these relay-safe keys (User, StrictHostKeyChecking with actual user value, ProxyJump, ProxyCommand, HostKeyAlias, IdentitiesOnly, UserKnownHostsFile/GlobalKnownHostsFile, etc.) to args, and only add the default StrictHostKeyChecking=accept-new if neither resolved nor configuration.sshOptions supplies it; apply the same fix to the other analogous code paths (the blocks around explicitRelayTransportOptions usage mentioned in the comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 4133-4138: The fast-path in resolveProxyJumpChain is incorrectly
skipping hops that look like user@host:port even when host is an SSH config
alias; update resolveProxyJumpChain (the code that currently returns hop
unchanged for matches) to parse each hop into user, host, and port, then only
skip resolution when the host is a literal IP (match IPv4 like
\d+\.\d+\.\d+\.\d+ or bracketed IPv6 like [::1]); otherwise pass the extracted
host through the existing SSH-config resolution logic and rebuild the hop
(user@resolvedHost:port) before returning/adding to args (see the "proxyjump"
case and args += ["-J", resolved] for where this resolved value is used).
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 4035-4039: explicitRelayTransportOptions() currently only replays
-p/-i when ssh -G fails, losing user-provided SSH options like User,
StrictHostKeyChecking, ProxyJump/ProxyCommand, HostKeyAlias, IdentitiesOnly and
known-hosts overrides; update that function so when resolved.* flags (e.g.,
resolved.hasStrictHostKeyChecking) do not indicate a resolved value, you scan
configuration.sshOptions (using hasSSHOptionKey and the same sshOptions parsing
helpers) and append "-o", "Key=Value" entries for any of these relay-safe keys
(User, StrictHostKeyChecking with actual user value, ProxyJump, ProxyCommand,
HostKeyAlias, IdentitiesOnly, UserKnownHostsFile/GlobalKnownHostsFile, etc.) to
args, and only add the default StrictHostKeyChecking=accept-new if neither
resolved nor configuration.sshOptions supplies it; apply the same fix to the
other analogous code paths (the blocks around explicitRelayTransportOptions
usage mentioned in the comment).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| case "proxyjump": | ||
| if value.lowercased() != "none" { | ||
| // Resolve the ProxyJump chain so jump hosts don't need config-file | ||
| // resolution (since -F /dev/null is propagated to jump connections). | ||
| let resolved = resolveProxyJumpChain(value) | ||
| args += ["-J", resolved] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'EOF'
Host jumpalias
HostName bastion.example.com
Host target
ProxyJump bob@jumpalias:2222
EOF
echo 'proxyjump emitted for target:'
ssh -F "$tmp" -G target | awk '/^proxyjump /{print}'
echo
echo 'resolved jumpalias host:'
ssh -F "$tmp" -G jumpalias | awk '/^(hostname|user|port) /{print}'Repository: manaflow-ai/cmux
Length of output: 355
🏁 Script executed:
grep -n "resolveProxyJumpChain" Sources/Workspace.swift | head -20Repository: manaflow-ai/cmux
Length of output: 201
🏁 Script executed:
sed -n '4100,4200p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 5003
🏁 Script executed:
ast-grep --pattern 'func resolveProxyJumpChain($$$) {
$$$
}'Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
sed -n '4171,4220p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 2262
🏁 Script executed:
sed -n '4176,4180p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 392
🏁 Script executed:
rg "proxyjump|resolveProxyJumpChain" -n Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 262
The fast-path skips resolving ProxyJump hops that contain SSH config aliases.
The pattern check at lines 4176–4180 returns hop unchanged if it matches user@host:port, assuming it's already fully resolved. However, the host portion can still be an SSH config alias (e.g., bob@jumpalias:2222 where jumpalias is a Host alias). This bypasses the resolution logic below and passes the unresolved alias to -J, causing the spawned jump SSH to fail since it inherits -F /dev/null.
The shell test demonstrates this: ssh -G target reports proxyjump bob@jumpalias:2222, but jumpalias resolves to bastion.example.com when checked separately. Parse and resolve the host portion of the hop, or only skip literal IP addresses (e.g., user@192.0.2.1:2222 or user@[::1]:2222).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 4133 - 4138, The fast-path in
resolveProxyJumpChain is incorrectly skipping hops that look like user@host:port
even when host is an SSH config alias; update resolveProxyJumpChain (the code
that currently returns hop unchanged for matches) to parse each hop into user,
host, and port, then only skip resolution when the host is a literal IP (match
IPv4 like \d+\.\d+\.\d+\.\d+ or bracketed IPv6 like [::1]); otherwise pass the
extracted host through the existing SSH-config resolution logic and rebuild the
hop (user@resolvedHost:port) before returning/adding to args (see the
"proxyjump" case and args += ["-J", resolved] for where this resolved value is
used).
1 participant
Summary
Fixes #2596 -
cmux sshrelay fails when SSH config hasLocalForward,DynamicForward, orRemoteForwarddirectives.Problem
The reverse relay SSH fallback (used when ControlMaster dynamic forwarding is unavailable) spawns a standalone connection with
-S none. This connection reads~/.ssh/configand attempts to establish all configured forwards, which conflict with ports already bound by the primary SSH connection. WithExitOnForwardFailure=yes, the relay dies before its ownRemoteForwardcan be established.Additionally,
backgroundSSHOptions()filterscontrolmasterandcontrolpersistbut notcontrolpath. This causes-o ControlPath=...to appear after-S nonein the relay command, which on macOS can re-enable mux attachment despite the explicit-S none.Fix
Primary: Bypass SSH config for standalone relay
Use
-F /dev/nullto prevent the relay SSH process from reading~/.ssh/configentirely. Transport-critical options are resolved viassh -Gand passed explicitly:hostname,user,port- connection targetproxyjump,proxycommand- multi-hop routingidentityfile,identitiesonly,certificatefile,identityagent- authenticationhostkeyalgorithms,kexalgorithms,ciphers,macs- crypto negotiationhostkeyalias,userknownhostsfile,globalknownhostsfile- host key verificationpubkeyacceptedalgorithms,preferredauthentications- auth methodsstricthostkeychecking- host key policy (see security note below)ProxyJump hosts are also resolved via
ssh -Gso that SSH configHostaliases work even though-F /dev/nullis propagated to jump host SSH processes. IPv6 literals are correctly bracketed when a non-default port is present (user@[2001:db8::1]:2222).Graceful fallback: if
ssh -Gfails, falls back to using only the explicitly configured port and identity file (same behavior as before, minus the config-file forwards).Secondary: Filter
controlpathfrom background SSH optionsAdded
controlpathto thebatchSSHControlOptionKeysfilter set in both the static (WorkspaceRemoteSSHBatchCommandBuilder) and instance (backgroundSSHOptions) methods. This prevents stale ControlPath options from leaking into standalone relay commands.Security note: StrictHostKeyChecking policy propagation
The previous code hard-coded
-o StrictHostKeyChecking=accept-newfor all background SSH operations. ThesshCommonArgumentsguard (if !hasSSHOptionKey(...)) only checkedconfiguration.sshOptions(explicit--ssh-optionCLI flags), not the full effective SSH config from~/.ssh/config.This means users who set
StrictHostKeyChecking yesin their SSH config (a deliberate security hardening to reject unknown host keys) had that policy silently downgraded toaccept-new(TOFU) for the relay connection. On first connection to a new host, this creates a window for MITM key substitution that the user explicitly opted out of.This PR resolves the issue by reading
StrictHostKeyCheckingfrom thessh -Geffective config output and propagating the users configured policy to the relay.accept-newis only applied as a default when no policy is configured, which matches user expectation for automated background connections.Testing
swiftc -parse)-N/-Rpatterns which are preserved)Reproduction
See #2596 for detailed reproduction steps. The fix eliminates all three failure modes:
LocalForward/DynamicForwardno longer conflict with bound portsRemoteForwardno longer duplicates existing forwardsControlPathoption no longer re-enables mux attachment after-S noneSummary by CodeRabbit