-
Notifications
You must be signed in to change notification settings - Fork 936
Fix SSH relay failure when config has LocalForward/DynamicForward directives #2635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
c62c6d9
886b0ca
2d0298b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1073,6 +1073,7 @@ enum WorkspaceRemoteSSHBatchCommandBuilder { | |
| private static let batchSSHControlOptionKeys: Set<String> = [ | ||
| "controlmaster", | ||
| "controlpersist", | ||
| "controlpath", | ||
| ] | ||
|
|
||
| static func daemonTransportArguments( | ||
|
|
@@ -4006,20 +4007,219 @@ final class WorkspaceRemoteSessionController { | |
| } | ||
| } | ||
|
|
||
| /// Resolved SSH transport options with metadata about what was set. | ||
| private struct ResolvedTransportOptions { | ||
| let args: [String] | ||
| let hasStrictHostKeyChecking: Bool | ||
| } | ||
|
|
||
| private func reverseRelayArguments(relayPort: Int, localRelayPort: Int) -> [String] { | ||
| // Fallback standalone transport when dynamic forwarding through an existing | ||
| // control master is unavailable. | ||
| var args: [String] = ["-N", "-T", "-S", "none"] | ||
| args += sshCommonArguments(batchMode: true) | ||
| // | ||
| // Uses -F /dev/null to bypass ~/.ssh/config entirely, preventing config-file | ||
| // LocalForward/DynamicForward/RemoteForward directives from conflicting with | ||
| // ports already bound by the primary SSH connection. Transport-critical options | ||
| // (ProxyJump, ProxyCommand, HostKeyAlgorithms, etc.) are resolved via ssh -G | ||
| // and passed explicitly so connectivity is preserved without config-file forwards. | ||
| let resolved = resolvedRelayTransportOptions() | ||
| var args: [String] = ["-N", "-T", "-S", "none", "-F", "/dev/null"] | ||
| args += resolved.args | ||
| args += [ | ||
| "-o", "ConnectTimeout=6", | ||
| "-o", "ServerAliveInterval=20", | ||
| "-o", "ServerAliveCountMax=2", | ||
| "-o", "BatchMode=yes", | ||
| "-o", "ExitOnForwardFailure=yes", | ||
| "-o", "RequestTTY=no", | ||
| ] | ||
| // Only add StrictHostKeyChecking default if not resolved from user config. | ||
| if !resolved.hasStrictHostKeyChecking { | ||
| args += ["-o", "StrictHostKeyChecking=accept-new"] | ||
| } | ||
| args += [ | ||
| "-R", "127.0.0.1:\(relayPort):127.0.0.1:\(localRelayPort)", | ||
| configuration.destination, | ||
| ] | ||
| return args | ||
| } | ||
|
|
||
| /// Resolve transport-critical SSH options for the relay by running `ssh -G`. | ||
| /// This extracts options like ProxyJump, ProxyCommand, IdentityFile, HostKeyAlgorithms, | ||
| /// etc. from the user's SSH config without carrying over forwarding directives. | ||
| private func resolvedRelayTransportOptions() -> ResolvedTransportOptions { | ||
| // Build the ssh -G probe command with the same port/identity/options as the | ||
| // primary connection so Host/Match blocks resolve identically. | ||
| var probeArgs: [String] = ["-G"] | ||
| if let port = configuration.port { | ||
| probeArgs += ["-p", String(port)] | ||
| } | ||
| if let identityFile = configuration.identityFile, | ||
| !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| probeArgs += ["-i", identityFile] | ||
| } | ||
| for option in backgroundSSHOptions(configuration.sshOptions) { | ||
| probeArgs += ["-o", option] | ||
| } | ||
| probeArgs.append(configuration.destination) | ||
|
|
||
| guard let result = try? sshExec(arguments: probeArgs, timeout: 5), | ||
| result.status == 0 else { | ||
| debugLog("remote.relay.sshG.failed falling back to explicit config only") | ||
| // Fallback: use just the explicitly configured options without ssh -G resolution. | ||
| return ResolvedTransportOptions( | ||
| args: explicitRelayTransportOptions(), | ||
| hasStrictHostKeyChecking: false | ||
| ) | ||
| } | ||
|
|
||
| return parseRelayTransportOptions(from: result.stdout) | ||
| } | ||
|
|
||
| /// Keys from `ssh -G` output that are needed for transport but NOT forwarding. | ||
| private static let relayTransportKeys: Set<String> = [ | ||
| "hostname", | ||
| "user", | ||
| "port", | ||
| "proxyjump", | ||
| "proxycommand", | ||
| "identityfile", | ||
| "identitiesonly", | ||
| "certificatefile", | ||
| "identityagent", | ||
| "hostkeyalgorithms", | ||
| "kexalgorithms", | ||
| "ciphers", | ||
| "macs", | ||
| "hostkeyalias", | ||
| "userknownhostsfile", | ||
| "globalknownhostsfile", | ||
| "pubkeyacceptedalgorithms", | ||
| "preferredauthentications", | ||
| "stricthostkeychecking", | ||
| ] | ||
|
|
||
| /// Parse `ssh -G` output and extract transport-critical options as `-o Key=Value` pairs. | ||
| private func parseRelayTransportOptions(from sshGOutput: String) -> ResolvedTransportOptions { | ||
| var args: [String] = [] | ||
| var hasStrictHostKeyChecking = false | ||
|
|
||
| for line in sshGOutput.split(separator: "\n") { | ||
| let parts = line.split(maxSplits: 1, whereSeparator: \.isWhitespace) | ||
| guard parts.count == 2 else { continue } | ||
| let key = parts[0].lowercased() | ||
| let value = parts[1].trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| guard Self.relayTransportKeys.contains(key), !value.isEmpty else { continue } | ||
|
|
||
| switch key { | ||
| case "hostname": | ||
| args += ["-o", "Hostname=\(value)"] | ||
| case "user": | ||
| args += ["-l", value] | ||
| case "port": | ||
| // Prefer explicitly configured port; fall back to ssh -G resolved value. | ||
| let effectivePort = configuration.port.map(String.init) ?? value | ||
| if let portNum = Int(effectivePort), portNum != 22 { | ||
| args += ["-p", effectivePort] | ||
| } | ||
| case "identityfile": | ||
| // ssh -G may emit multiple identity files; include all that exist on disk. | ||
| let expanded = (value as NSString).expandingTildeInPath | ||
| if FileManager.default.fileExists(atPath: expanded) { | ||
| args += ["-i", value] | ||
| } | ||
| 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] | ||
|
Comment on lines
+4133
to
+4138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 The shell test demonstrates this: 🤖 Prompt for AI Agents |
||
| } | ||
| case "proxycommand": | ||
| if value.lowercased() != "none" { | ||
| args += ["-o", "ProxyCommand=\(value)"] | ||
| } | ||
| case "stricthostkeychecking": | ||
| hasStrictHostKeyChecking = true | ||
| args += ["-o", "StrictHostKeyChecking=\(value)"] | ||
| default: | ||
| args += ["-o", "\(parts[0])=\(value)"] | ||
| } | ||
| } | ||
|
|
||
| return ResolvedTransportOptions(args: args, hasStrictHostKeyChecking: hasStrictHostKeyChecking) | ||
| } | ||
|
|
||
| /// Resolve a ProxyJump chain by running `ssh -G` on each hop to get its | ||
| /// actual hostname, user, and port. This is needed because `-F /dev/null` | ||
| /// is propagated to jump host SSH processes, preventing config-file Host | ||
| /// alias resolution. | ||
| /// | ||
| /// Input: "jumphost,bastion" (comma-separated hops, may be config aliases) | ||
| /// Output: "admin@jump.example.com:22,user@bastion.example.com:22" (resolved) | ||
| private func resolveProxyJumpChain(_ chain: String) -> String { | ||
| let hops = chain.split(separator: ",").map { | ||
| $0.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| } | ||
|
|
||
| let resolved = hops.map { hop -> String in | ||
| // If the hop already looks fully qualified (user@host:port), pass through. | ||
| if hop.contains("@") && (hop.contains("]:") || (!hop.contains("[") && hop.last?.isNumber == true && hop.split(separator: ":").count == 2)) { | ||
| return hop | ||
| } | ||
| // Try to resolve via ssh -G | ||
| guard let result = try? sshExec(arguments: ["-G", hop], timeout: 5), | ||
| result.status == 0 else { | ||
| return hop // Can't resolve; pass through as-is | ||
| } | ||
| var hostname: String? | ||
| var user: String? | ||
| var port: String? | ||
| for line in result.stdout.split(separator: "\n") { | ||
| let parts = line.split(maxSplits: 1, whereSeparator: \.isWhitespace) | ||
| guard parts.count == 2 else { continue } | ||
| let key = parts[0].lowercased() | ||
| let val = parts[1].trimmingCharacters(in: .whitespacesAndNewlines) | ||
| switch key { | ||
| case "hostname": hostname = val | ||
| case "user": user = val | ||
| case "port": port = val | ||
| default: break | ||
| } | ||
| } | ||
| guard let resolvedHost = hostname else { return hop } | ||
| var resolved = "" | ||
| if let user { resolved += "\(user)@" } | ||
| // Bracket IPv6 literals when a non-default port is appended. | ||
| let needsPort = port != nil && port != "22" | ||
| let isIPv6 = resolvedHost.contains(":") | ||
| if isIPv6 && needsPort { | ||
| resolved += "[\(resolvedHost)]:\(port!)" | ||
| } else { | ||
| resolved += resolvedHost | ||
| if needsPort { resolved += ":\(port!)" } | ||
| } | ||
| return resolved | ||
| } | ||
|
|
||
| return resolved.joined(separator: ",") | ||
| } | ||
|
|
||
| /// Fallback transport options when ssh -G is unavailable. Uses only the | ||
| /// explicitly configured port and identity file. | ||
| 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] | ||
| } | ||
| return args | ||
| } | ||
|
|
||
| private func startReverseRelayViaControlMasterLocked(forwardSpec: String) -> Bool { | ||
| guard let arguments = WorkspaceRemoteSSHBatchCommandBuilder.reverseRelayControlMasterArguments( | ||
| configuration: configuration, | ||
|
|
@@ -4119,6 +4319,7 @@ final class WorkspaceRemoteSessionController { | |
| let batchSSHControlOptionKeys: Set<String> = [ | ||
| "controlmaster", | ||
| "controlpersist", | ||
| "controlpath", | ||
| ] | ||
| return normalizedSSHOptions(options).filter { option in | ||
| guard let key = sshOptionKey(option) else { return false } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.