From 0667a17c429fe8719b89ae85e4dc99df833aad4e Mon Sep 17 00:00:00 2001 From: Raing5Days Date: Thu, 29 Oct 2026 00:27:53 +0800 Subject: [PATCH 1/4] feat(remote-ssh): carry SSH transports over ProxyCommand processes Add OpenSSH-style ProxyCommand support to the remote SSH service, first batch (backend only): - Thread an optional proxyCommand through SSHConnectionConfig, SSHConfigEntry, SavedConnection, and SavedConnectionRecord with serde defaults so older payloads and connections.json stay loadable, and include it in connection drift detection. - Spawn the ProxyCommand through the shared process manager (cmd.exe /C with raw args on Windows, sh -c elsewhere), expand %h/%p/%r/%% tokens, drain stderr into the log plus a bounded tail, and carry the SSH handshake over the joined stdout/stdin via the existing establish_stream_session path. - Own the child process for the whole session lifetime: unpublished handshakes, published connections, and disconnect/reconnect paths all kill it, and a reconnect keeps a stale proxy alive only while its old transport still serves in-flight work. Failures report the proxy exit code and stderr tail instead of a bare handshake error. - Merge ~/.ssh/config with its Include tree (recursive, glob, cycle and depth guarded) before parsing, making Coder/AtomGit style hosts visible; ProxyCommand stays exclusive with ProxyJump and container targets, rejected with explicit errors for now. The connection dialog field for editing proxyCommand lands in a follow-up batch; ~/.ssh/config hosts already carry it end to end. --- Cargo.lock | 1 + .../src/remote_ssh.rs | 1 + .../services/services-integrations/Cargo.toml | 3 +- .../src/remote_persistence.rs | 2 + .../src/remote_ssh/manager.rs | 352 ++++++++++++++++-- .../manager/tests/workspace_sftp.rs | 2 + .../src/remote_ssh/mod.rs | 4 + .../src/remote_ssh/proxy_command.rs | 303 +++++++++++++++ .../src/remote_ssh/ssh_config_loader.rs | 348 +++++++++++++++++ .../src/remote_ssh/types.rs | 15 + .../remote_ssh_contracts.rs | 71 ++++ .../remote_ssh_disabled_contracts.rs | 1 + 12 files changed, 1070 insertions(+), 33 deletions(-) create mode 100644 src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs create mode 100644 src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs diff --git a/Cargo.lock b/Cargo.lock index 15fe2a204e..63c9a64329 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6759,6 +6759,7 @@ dependencies = [ "futures", "futures-util", "git2", + "glob", "hex", "hkdf", "hostname", diff --git a/src/crates/services/legacy-migration-adapters/src/remote_ssh.rs b/src/crates/services/legacy-migration-adapters/src/remote_ssh.rs index 13ccbaa156..9554b46cd6 100644 --- a/src/crates/services/legacy-migration-adapters/src/remote_ssh.rs +++ b/src/crates/services/legacy-migration-adapters/src/remote_ssh.rs @@ -1201,6 +1201,7 @@ mod tests { default_workspace: Some("/srv/project".to_string()), last_connected: Some(1), proxy_jump: Some("jump.example.invalid".to_string()), + proxy_command: None, container: None, options: owner::SshConnectionOptionsRecord::default(), } diff --git a/src/crates/services/services-integrations/Cargo.toml b/src/crates/services/services-integrations/Cargo.toml index 719799ae24..46771da3ed 100644 --- a/src/crates/services/services-integrations/Cargo.toml +++ b/src/crates/services/services-integrations/Cargo.toml @@ -36,6 +36,7 @@ chrono = { workspace = true, optional = true } dunce = { workspace = true, optional = true } futures = { workspace = true, optional = true } futures-util = { workspace = true, optional = true } +glob = { workspace = true, optional = true } fs2 = { workspace = true, optional = true } git2 = { workspace = true, optional = true } hostname = { workspace = true, optional = true } @@ -511,7 +512,7 @@ product-full = [ "workspace-search", ] skillhub = ["openbitfun-services-core/tls-provider", "reqwest", "reqwest/json", "reqwest/query", "reqwest/rustls-no-provider", "reqwest/stream", "zip", "tempfile", "tokio/fs", "tokio/io-util"] -ssh_config = ["dep:ssh_config"] +ssh_config = ["dep:ssh_config", "dep:glob"] [dev-dependencies] tempfile = { workspace = true } diff --git a/src/crates/services/services-integrations/src/remote_persistence.rs b/src/crates/services/services-integrations/src/remote_persistence.rs index cb98da8c58..d96788e167 100644 --- a/src/crates/services/services-integrations/src/remote_persistence.rs +++ b/src/crates/services/services-integrations/src/remote_persistence.rs @@ -618,6 +618,8 @@ pub struct SavedConnectionRecord { pub last_connected: Option, #[serde(default)] pub proxy_jump: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub proxy_command: Option, #[serde(default)] pub container: Option, #[serde(default)] diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 898f5a0761..a426618564 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -2,7 +2,10 @@ //! //! This module manages SSH connections using the pure-Russ SSH implementation +use super::proxy_command::{expand_tokens, ProxyCommandChild}; use super::sftp_file::ManagedSftpSession; +#[cfg(feature = "ssh_config")] +use super::ssh_config_loader::load_merged_config; use crate::remote_ssh::password_vault::SSHPasswordVault; use crate::remote_ssh::types::{ ConnectionTestReport, ConnectionTestStage, ContainerAccess, ContainerWorkspaceConfig, @@ -455,12 +458,23 @@ fn parse_ssh_config_value(value: &str) -> Option<&str> { value.split_whitespace().next() } +/// Extract a whole-line command value such as `ProxyCommand`'s. +/// +/// Unlike [`parse_ssh_config_value`], the value spans every remaining token of +/// the line. A single pair of surrounding double quotes is stripped so +/// `ProxyCommand "coder ssh --stdio %h"` keeps its inner quoting intact. #[cfg(feature = "ssh_config")] -fn strip_utf8_bom(content: String) -> String { - content - .strip_prefix('\u{feff}') - .unwrap_or(&content) - .to_string() +fn parse_ssh_config_command_value(value: &str) -> Option { + let value = value.trim(); + if value.is_empty() { + return None; + } + if let Some(inner) = value.strip_prefix('"') { + if let Some(stripped) = inner.strip_suffix('"') { + return Some(stripped.to_string()); + } + } + Some(value.to_string()) } /// Manually parse `~/.ssh/config` content into Host blocks with their direct settings. @@ -484,6 +498,7 @@ fn parse_ssh_config_manually(content: &str) -> Vec { let mut block_identity_file: Option = None; let mut block_certificate_file: Option = None; let mut block_proxy_jump: Option = None; + let mut block_proxy_command: Option = None; for line in content.lines() { let line = line.trim(); @@ -508,6 +523,7 @@ fn parse_ssh_config_manually(content: &str) -> Vec { agent: Some(true), certificate_file: block_certificate_file.take(), proxy_jump: block_proxy_jump.take(), + proxy_command: block_proxy_command.take(), }); } @@ -519,6 +535,7 @@ fn parse_ssh_config_manually(content: &str) -> Vec { block_identity_file = None; block_certificate_file = None; block_proxy_jump = None; + block_proxy_command = None; } else if current_host.is_some() { // Track details within the current Host block if keyword.eq_ignore_ascii_case("HostName") { @@ -535,6 +552,12 @@ fn parse_ssh_config_manually(content: &str) -> Vec { parse_ssh_config_value(value).map(|s| shellexpand::tilde(s).to_string()); } else if keyword.eq_ignore_ascii_case("ProxyJump") { block_proxy_jump = parse_ssh_config_value(value).map(ToOwned::to_owned); + } else if keyword.eq_ignore_ascii_case("ProxyCommand") { + // Keep the raw command text: `%h`/`%p`/`%r` tokens are expanded + // at connect time, and shell quoting must survive intact — + // unlike single-token directives, the whole remainder of the + // line is the value. + block_proxy_command = parse_ssh_config_command_value(value); } } } @@ -550,6 +573,7 @@ fn parse_ssh_config_manually(content: &str) -> Vec { agent: Some(true), certificate_file: block_certificate_file, proxy_jump: block_proxy_jump, + proxy_command: block_proxy_command, }); } @@ -582,6 +606,9 @@ struct ActiveConnection { bounded_sftp_session: Arc, #[allow(dead_code)] server_key: Option, + /// ProxyCommand process carrying the SSH transport; dropping it kills the + /// proxy. Absent for every non-proxy transport. + proxy_child: Option, /// Liveness flag; flipped to false from `SSHHandler::disconnected`. /// Allows `is_connected` and SFTP/exec entry points to detect a dead session /// without waiting for the next failed I/O. @@ -662,6 +689,9 @@ struct EstablishedSession { server_info: Option, effective_config: SSHConnectionConfig, unpublished: UnpublishedSshSession, + /// ProxyCommand process carrying the SSH transport, kept alive exactly as + /// long as the session. `None` for every non-proxy transport. + proxy_child: Option, } /// Owns authenticated transports until the complete chain is published. A @@ -672,6 +702,9 @@ struct UnpublishedSshSession { // Creation order is first jump through final target. One guard owns the // whole chain so cancellation cannot race independent hop cleanups. handles: Vec>>, + // ProxyCommand children spawned for transports that have not been + // published yet; the guard's Drop tears them down with the handles. + proxy_children: Vec, } impl UnpublishedSshSession { @@ -681,6 +714,11 @@ impl UnpublishedSshSession { fn disarm(&mut self) { self.handles.clear(); + self.proxy_children.clear(); + } + + fn take_proxy_child(&mut self) -> Option { + self.proxy_children.pop() } } @@ -2410,18 +2448,13 @@ impl SSHConnectionManager { .map(|p| p.join(".ssh").join("config")) .unwrap_or_default(); - if !ssh_config_path.exists() { - log::debug!("SSH config not found at {:?}", ssh_config_path); - return SSHConfigLookupResult { - found: false, - config: None, - }; - } - - let config_content = match tokio::fs::read_to_string(&ssh_config_path).await { - Ok(c) => strip_utf8_bom(c), - Err(e) => { - log::warn!("Failed to read SSH config: {:?}", e); + // Load the user config merged with its `Include` tree; ecosystem + // tooling (Coder, AtomGit, devenv) writes Host blocks into included + // files, and `ssh_config` 0.1 cannot expand them itself. + let config_content = match load_merged_config(&ssh_config_path) { + Some(content) => content, + None => { + log::debug!("SSH config not found at {:?}", ssh_config_path); return SSHConfigLookupResult { found: false, config: None, @@ -2455,6 +2488,8 @@ impl SSHConnectionManager { .map(|f| shellexpand::tilde(f).to_string()); let has_proxy_command = ssh_cfg_has(&host_settings, "ProxyCommand"); let proxy_jump = ssh_cfg_get(&host_settings, "ProxyJump").map(ToOwned::to_owned); + let proxy_command = + ssh_cfg_get(&host_settings, "ProxyCommand").map(ToOwned::to_owned); return SSHConfigLookupResult { found: true, @@ -2467,6 +2502,7 @@ impl SSHConnectionManager { agent: if has_proxy_command { None } else { Some(true) }, certificate_file, proxy_jump, + proxy_command, }), }; } @@ -2504,15 +2540,11 @@ impl SSHConnectionManager { .map(|p| p.join(".ssh").join("config")) .unwrap_or_default(); - if !ssh_config_path.exists() { - log::debug!("SSH config not found at {:?}", ssh_config_path); - return Vec::new(); - } - - let config_content = match tokio::fs::read_to_string(&ssh_config_path).await { - Ok(c) => strip_utf8_bom(c), - Err(e) => { - log::warn!("Failed to read SSH config: {:?}", e); + // Merged view including the `Include` tree; see `get_ssh_config`. + let config_content = match load_merged_config(&ssh_config_path) { + Some(content) => content, + None => { + log::debug!("SSH config not found at {:?}", ssh_config_path); return Vec::new(); } }; @@ -2555,6 +2587,9 @@ impl SSHConnectionManager { if let Some(proxy_jump) = ssh_cfg_get(&settings, "ProxyJump") { entry.proxy_jump = Some(proxy_jump.to_string()); } + if let Some(proxy_command) = ssh_cfg_get(&settings, "ProxyCommand") { + entry.proxy_command = Some(proxy_command.to_string()); + } } } } @@ -2959,6 +2994,7 @@ impl SSHConnectionManager { default_workspace: config.default_workspace.clone(), last_connected: Some(chrono::Utc::now().timestamp() as u64), proxy_jump: config.proxy_jump.clone(), + proxy_command: config.proxy_command.clone(), container: config.container.clone(), wsl: config.wsl.clone(), options: config.options.clone(), @@ -3089,6 +3125,7 @@ impl SSHConnectionManager { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: established.proxy_child, alive: established.alive, }, ) @@ -3120,8 +3157,14 @@ impl SSHConnectionManager { .proxy_jump .as_deref() .is_some_and(|jump| !jump.trim().is_empty()) + || config + .proxy_command + .as_deref() + .is_some_and(|command| !command.trim().is_empty()) { - anyhow::bail!("WSL cannot be combined with Docker or SSH jump hosts"); + anyhow::bail!( + "WSL cannot be combined with Docker, SSH jump hosts, or ProxyCommand" + ); } let server_info = super::wsl::probe(wsl, Duration::from_secs(timeout_secs)).await?; return Ok(EstablishedSession { @@ -3131,6 +3174,7 @@ impl SSHConnectionManager { server_info: Some(server_info), effective_config: config.clone(), unpublished: UnpublishedSshSession::default(), + proxy_child: None, }); } if config.uses_local_docker() { @@ -3158,9 +3202,36 @@ impl SSHConnectionManager { server_info: Some(server_info), effective_config: resolved_container_config(config, ContainerAccess::DockerExec), unpublished: UnpublishedSshSession::default(), + proxy_child: None, }); } + if let Some(proxy_command) = config + .proxy_command + .as_deref() + .map(str::trim) + .filter(|command| !command.is_empty()) + { + // Deliberately exclusive combinations for this release: a proxy + // command already owns the transport to the target, so layering a + // jump chain or a container target on top has no tested semantics. + if config + .proxy_jump + .as_deref() + .is_some_and(|jump| !jump.trim().is_empty()) + { + anyhow::bail!( + "ProxyCommand and ProxyJump cannot be combined; remove one of them from the connection" + ); + } + if config.container.is_some() { + anyhow::bail!("ProxyCommand cannot be combined with a Docker container target"); + } + return self + .establish_proxy_command_session(config, proxy_command, timeout_secs) + .await; + } + let mut unpublished = UnpublishedSshSession::default(); let jumps = self .resolve_proxy_jump_chain(config) @@ -3192,6 +3263,7 @@ impl SSHConnectionManager { server_info: container_info, effective_config, unpublished, + proxy_child: None, }); } } @@ -3217,6 +3289,7 @@ impl SSHConnectionManager { config.clone() }, unpublished, + proxy_child: None, }); } @@ -3323,6 +3396,7 @@ impl SSHConnectionManager { server_info: container_info, effective_config, unpublished, + proxy_child: None, }); } } @@ -3348,6 +3422,7 @@ impl SSHConnectionManager { config.clone() }, unpublished, + proxy_child: None, }) } @@ -3423,6 +3498,7 @@ impl SSHConnectionManager { server_info, effective_config, unpublished, + proxy_child: None, }) } Err(error) => { @@ -3589,6 +3665,7 @@ impl SSHConnectionManager { auth, default_workspace: None, proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: config.options.clone(), @@ -3685,6 +3762,78 @@ impl SSHConnectionManager { .await } + /// Build a fresh SSH session whose transport is the stdin/stdout of a + /// local ProxyCommand process (OpenSSH semantics). + /// + /// Reusing `establish_stream_session` keeps handshake timeout, known-hosts + /// verification, and authentication identical to the direct path. The child + /// process is owned by `UnpublishedSshSession` until the session is fully + /// established, so every failure path kills it; afterwards the guard moves + /// into the returned `EstablishedSession` (and from there into the + /// published connection record), so disconnects and app shutdown kill it. + async fn establish_proxy_command_session( + &self, + config: &SSHConnectionConfig, + proxy_command: &str, + timeout_secs: u64, + ) -> anyhow::Result { + let stage = format!("proxy command {}", connection_label(config)); + let mut unpublished = UnpublishedSshSession::default(); + + let expanded = expand_tokens(proxy_command, &config.host, config.port, &config.username); + let mut child = ProxyCommandChild::spawn(&expanded, &stage) + .map_err(|error| tag_failed_stage("proxy command", error))?; + let stream = child + .take_stream() + .map_err(|error| tag_failed_stage("proxy command", error))?; + unpublished.proxy_children.push(child); + + let session = self + .establish_stream_session(config, stream, timeout_secs, &stage, &mut unpublished) + .await; + match session { + Ok((handle, alive, server_info)) => { + let proxy_child = unpublished + .take_proxy_child() + .expect("proxy child was pushed before the handshake"); + Ok(EstablishedSession { + handle: Some(handle), + jump_handles: Vec::new(), + alive, + server_info, + effective_config: config.clone(), + unpublished, + proxy_child: Some(proxy_child), + }) + } + Err(error) => { + let detail = match unpublished.take_proxy_child() { + Some(child) => { + let (exit_code, stderr_tail) = child.finish().await; + let mut parts = Vec::new(); + match exit_code { + Some(code) => { + parts.push(format!("proxy command exited with code {code}")) + } + None => parts + .push("proxy command was terminated before completing".to_string()), + } + if !stderr_tail.is_empty() { + parts.push(format!("stderr: {stderr_tail}")); + } + parts.join("; ") + } + None => String::new(), + }; + if detail.is_empty() { + Err(error) + } else { + Err(anyhow!("{error} ({detail})")) + } + } + } + } + async fn probe_remote_container( &self, handle: &Arc>, @@ -4409,6 +4558,7 @@ impl SSHConnectionManager { auth, default_workspace: saved.default_workspace, proxy_jump: saved.proxy_jump, + proxy_command: saved.proxy_command, container: saved.container, wsl: saved.wsl, options: saved.options, @@ -4556,23 +4706,25 @@ impl SSHConnectionManager { // session is usually dead, but "usually" covers neither config drift // (where the old session is perfectly healthy) nor a half-open link the // server still counts against its session limit. - let (stale_handle, stale_jump_handles) = { + let (stale_handle, stale_jump_handles, stale_proxy_child) = { let mut guard = self.connections.write().await; established.unpublished.disarm(); if let Some(conn) = guard.get_mut(connection_id) { let stale_handle = conn.handle.take(); let stale_jump_handles = std::mem::take(&mut conn.jump_handles); + let stale_proxy_child = conn.proxy_child.take(); conn.handle = established.handle; conn.jump_handles = established.jump_handles; conn.config = config; conn.effective_config = established.effective_config; conn.alive = established.alive; + conn.proxy_child = established.proxy_child; if let Some(si) = server_info.as_ref() { conn.server_info = Some(si.clone()); } conn.sftp_session = Arc::new(SftpCache::new()); conn.bounded_sftp_session = Arc::new(BoundedSftpCache::new()); - (stale_handle, stale_jump_handles) + (stale_handle, stale_jump_handles, stale_proxy_child) } else { let replaced = guard.insert( connection_id.to_string(), @@ -4585,12 +4737,16 @@ impl SSHConnectionManager { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: established.proxy_child, alive: established.alive, }, ); match replaced { - Some(previous) => (previous.handle, previous.jump_handles), - None => (None, Vec::new()), + Some(previous) => { + let replaced_child = previous.proxy_child; + (previous.handle, previous.jump_handles, replaced_child) + } + None => (None, Vec::new(), None), } } }; @@ -4612,6 +4768,26 @@ impl SSHConnectionManager { ); Self::shutdown_handle_chain(stale_chain).await; + // The stale ProxyCommand process is the old transport's other half: + // close it only once that transport was actually unused, and keep it + // alive otherwise — the same lease the skipped goodbye grants the old + // handle while in-flight work drains. + if let Some(child) = stale_proxy_child { + if stale_handle + .as_ref() + .map_or(true, |handle| Arc::strong_count(handle) == 1) + { + drop(child); + } else { + log::info!( + "SSH session {} reconnect keeps the previous ProxyCommand process alive \ + while its old transport still serves in-flight work", + connection_id + ); + std::mem::forget(child); + } + } + log::info!("SSH session {} reconnected successfully", connection_id); Ok(()) } @@ -7003,6 +7179,7 @@ mod tests { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -7503,6 +7680,7 @@ mod tests { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: Some(ContainerWorkspaceConfig { name: "fixture".into(), access: ContainerAccess::DockerExec, @@ -7526,6 +7704,7 @@ mod tests { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: None, alive: Arc::new(AtomicBool::new(true)), }, ); @@ -7751,6 +7930,7 @@ mod tests { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: Some(ContainerWorkspaceConfig { name: "fixture".into(), access: ContainerAccess::DockerExec, @@ -7775,6 +7955,7 @@ mod tests { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: None, alive: Arc::new(AtomicBool::new(true)), }, ); @@ -8231,7 +8412,7 @@ mod tests { #[test] #[cfg(feature = "ssh_config")] fn ssh_config_fallback_accepts_utf8_bom_and_defaults_to_agent() { - let content = strip_utf8_bom( + let content = crate::remote_ssh::ssh_config_loader::strip_utf8_bom( "\u{feff}Host 跳板\n HostName jump.example.com\n User 构建\n".to_string(), ); let entries = parse_ssh_config_manually(&content); @@ -8271,6 +8452,7 @@ mod tests { }, default_workspace: Some("/srv/project".to_string()), proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -8292,6 +8474,7 @@ mod tests { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: None, alive: alive.clone(), }, ); @@ -8339,6 +8522,106 @@ mod tests { ); } + #[test] + fn parses_proxy_command_as_a_whole_line_value() { + let hosts = parse_ssh_config_manually( + "Host coder-x\n ProxyCommand \"C:\\tools\\coder.exe\" ssh --stdio %h\n IdentityFile ~/.ssh/id_ed25519\n", + ); + assert_eq!(hosts.len(), 1); + // The full remainder of the line survives, inner quotes included. + assert_eq!( + hosts[0].proxy_command.as_deref(), + Some("\"C:\\tools\\coder.exe\" ssh --stdio %h") + ); + + let hosts = + parse_ssh_config_manually("Host quoted\n ProxyCommand \"coder ssh --stdio %h\"\n"); + assert_eq!( + hosts[0].proxy_command.as_deref(), + Some("coder ssh --stdio %h") + ); + + let hosts = parse_ssh_config_manually("Host plain\n HostName plain.example\n"); + assert_eq!(hosts[0].proxy_command, None); + } + + fn proxy_command_fixture_config(command: &str) -> SSHConnectionConfig { + SSHConnectionConfig { + id: "proxy-fixture".to_string(), + name: "proxy fixture".to_string(), + host: "coder-vscode.coder.sage.org.ai--abc".to_string(), + port: 22, + username: "coder".to_string(), + auth: SSHAuthMethod::PrivateKey { + key_path: "/nonexistent/key".to_string(), + passphrase: None, + certificate_path: None, + }, + default_workspace: None, + proxy_jump: None, + proxy_command: Some(command.to_string()), + container: None, + wsl: None, + options: Default::default(), + } + } + + #[tokio::test] + async fn proxy_command_transport_reports_proxy_exit_code() { + let temp = tempfile::tempdir().unwrap(); + let manager = SSHConnectionManager::new(temp.path().into()); + let config = proxy_command_fixture_config("exit 7"); + + let error = manager + .establish_session(&config, 10) + .await + .err() + .expect("a proxy that exits immediately cannot complete a handshake"); + let message = error.to_string(); + assert!( + message.contains("proxy command exited with code 7"), + "unexpected error: {message}" + ); + } + + #[tokio::test] + async fn proxy_command_rejects_jump_and_container_combinations() { + let temp = tempfile::tempdir().unwrap(); + let manager = SSHConnectionManager::new(temp.path().into()); + + let mut config = proxy_command_fixture_config("exit 0"); + config.proxy_jump = Some("jump.example".to_string()); + let error = manager + .establish_session(&config, 10) + .await + .err() + .expect("ProxyCommand plus ProxyJump is rejected in this release"); + assert!( + error.to_string().contains("ProxyCommand and ProxyJump"), + "unexpected error: {error}" + ); + + let mut config = proxy_command_fixture_config("exit 0"); + config.container = Some(ContainerWorkspaceConfig { + name: "dev".to_string(), + access: ContainerAccess::DockerExec, + local: false, + docker_path: "docker".to_string(), + shell: "/bin/sh".to_string(), + user: None, + interactive: false, + }); + let error = manager + .establish_session(&config, 10) + .await + .err() + .expect("ProxyCommand plus a container target is rejected in this release"); + assert!( + error.to_string().contains("Docker container"), + "unexpected error: {error}" + ); + } + #[test] fn parses_docker_published_ipv4_and_ipv6_endpoints() { assert_eq!( @@ -8829,6 +9112,7 @@ mod tests { }, default_workspace: Some("/tmp/openbitfun-remote-workspace".to_string()), proxy_jump: None, + proxy_command: None, container: Some(ContainerWorkspaceConfig { name: container_name, access: ContainerAccess::DockerExec, @@ -9086,6 +9370,7 @@ mod tests { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -9115,6 +9400,7 @@ mod tests { }, default_workspace: Some("/root/project".to_string()), proxy_jump: Some("jump-a,jump-b".to_string()), + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -9205,6 +9491,7 @@ mod tests { }, default_workspace: Some("/workspace".to_string()), proxy_jump: Some("jump.example.com".to_string()), + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -9250,6 +9537,7 @@ mod tests { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs index 8b4cec0dca..2d9e75dd89 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager/tests/workspace_sftp.rs @@ -369,6 +369,7 @@ impl Fixture { }, default_workspace: None, proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), @@ -384,6 +385,7 @@ impl Fixture { sftp_session: Arc::new(SftpCache::new()), bounded_sftp_session: Arc::new(BoundedSftpCache::new()), server_key: None, + proxy_child: None, alive: Arc::new(AtomicBool::new(true)), }, ); diff --git a/src/crates/services/services-integrations/src/remote_ssh/mod.rs b/src/crates/services/services-integrations/src/remote_ssh/mod.rs index 8281c36ef6..b84030d1f2 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/mod.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/mod.rs @@ -31,6 +31,8 @@ mod password_vault; #[cfg(feature = "remote-ssh-concrete")] mod port_forward; #[cfg(feature = "remote-ssh-concrete")] +mod proxy_command; +#[cfg(feature = "remote-ssh-concrete")] #[cfg(feature = "remote-ssh-concrete")] mod release_verify; #[cfg(feature = "remote-ssh-concrete")] @@ -41,6 +43,8 @@ mod remote_exec_runtime_port; pub mod remote_fs; #[cfg(feature = "remote-ssh-concrete")] pub mod remote_terminal; +#[cfg(all(feature = "remote-ssh-concrete", feature = "ssh_config"))] +mod ssh_config_loader; pub use file_name_search::{search_remote_file_names, RemoteFileNameSearch}; pub use paths::*; diff --git a/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs new file mode 100644 index 0000000000..e54a990732 --- /dev/null +++ b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs @@ -0,0 +1,303 @@ +//! `ProxyCommand` transport support. +//! +//! OpenSSH runs `ProxyCommand` as a local shell process and carries the SSH +//! protocol over its stdin/stdout. This module does the same for OpenBitFun's +//! russh client: the manager spawns the command here and hands the joined +//! stdout/stdin to `establish_stream_session`, which keeps handshake timeouts, +//! known-hosts checks, and authentication identical to the direct path. +//! +//! Platform notes: +//! - Windows runs the command through `cmd.exe /C` with the raw string (no +//! extra MS quoting), matching how OpenSSH's Windows build passes +//! `ProxyCommand` along; Unix uses `sh -c`. +//! - `%h`, `%p`, `%r`, and `%%` are expanded before the shell sees the string. +//! - stderr is drained continuously into the log plus a small tail buffer: +//! proxies like `coder ssh --stdio` log there, and a full pipe would block +//! the child mid-handshake. + +use std::collections::VecDeque; +use std::process::Stdio; +use std::sync::{Arc, Mutex}; + +use anyhow::{anyhow, Context}; +use openbitfun_services_core::process_manager; +use tokio::io::{join, AsyncBufReadExt, BufReader, Join}; +use tokio::process::{Child, ChildStderr, ChildStdin, ChildStdout}; + +/// How many recent stderr lines are kept for error enrichment. +const STDERR_TAIL_LINES: usize = 16; +/// Cap a single buffered stderr line so a chatty proxy cannot grow the tail +/// without bound. +const STDERR_LINE_MAX_CHARS: usize = 400; +/// Grace period for a proxy to exit on its own before it is killed, so its +/// real exit code survives as the diagnostic. +const EXIT_DRAIN_GRACE: std::time::Duration = std::time::Duration::from_secs(2); + +/// Owns a spawned proxy process for the lifetime of the SSH session carried +/// over its stdio. +/// +/// The child is killed when the guard is dropped, whether that happens because +/// the connection was closed, the handshake failed, or the session was torn +/// down by [`crate::remote_ssh::manager::SSHConnectionManager`]. +pub(crate) struct ProxyCommandChild { + child: Option, + stream: Option<(ChildStdout, ChildStdin)>, + stderr_tail: Arc>>, + drain: Option>, + label: String, +} + +impl ProxyCommandChild { + /// Spawn `command` through the platform shell with piped stdio. + /// + /// `label` identifies the connection in logs; the command text is kept out + /// of error messages because ProxyCommand lines sometimes embed tokens. + pub(crate) fn spawn(command: &str, label: &str) -> anyhow::Result { + let mut cmd = process_manager::create_tokio_command(proxy_shell_program()); + configure_shell_args(&mut cmd, command); + cmd.stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + // Safety net for any path that drops the guard without an explicit + // `finish`; the guard's own Drop reaps as well. + .kill_on_drop(true); + let mut child = cmd + .spawn() + .with_context(|| format!("Failed to start proxy command for {label}"))?; + let stdout = child + .stdout + .take() + .ok_or_else(|| anyhow!("Proxy command for {label} has no stdout pipe"))?; + let stdin = child + .stdin + .take() + .ok_or_else(|| anyhow!("Proxy command for {label} has no stdin pipe"))?; + let stderr = child + .stderr + .take() + .ok_or_else(|| anyhow!("Proxy command for {label} has no stderr pipe"))?; + + let stderr_tail = Arc::new(Mutex::new(VecDeque::new())); + let drain = spawn_stderr_drain(stderr, stderr_tail.clone(), label.to_string()); + Ok(Self { + child: Some(child), + stream: Some((stdout, stdin)), + stderr_tail, + drain: Some(drain), + label: label.to_string(), + }) + } + + /// Take the joined stdout/stdin stream for the SSH handshake. + pub(crate) fn take_stream(&mut self) -> anyhow::Result> { + let (stdout, stdin) = self + .stream + .take() + .ok_or_else(|| anyhow!("Proxy command stream for {} was already taken", self.label))?; + Ok(join(stdout, stdin)) + } + + /// Kill the process (unless it already exited), reap it, and report the + /// exit code plus the buffered stderr tail for error messages. + pub(crate) async fn finish(mut self) -> (Option, String) { + let mut exit_code = None; + if let Some(child) = self.child.as_mut() { + // Give a fast-exiting proxy a moment to report its own exit code — + // that code is the actual diagnostic. Only a proxy that is still + // alive (wedged on a black hole, say) gets killed, and a killed + // proxy reports no code. + match tokio::time::timeout(EXIT_DRAIN_GRACE, child.wait()).await { + Ok(Ok(status)) => exit_code = status.code(), + _ => { + let _ = child.start_kill(); + if let Ok(status) = child.wait().await { + exit_code = status.code(); + } + } + } + } + // The drain task ends on stderr EOF, which follows process exit; wait + // for it so the tail reflects every line the proxy produced. + if let Some(drain) = self.drain.take() { + let _ = tokio::time::timeout(EXIT_DRAIN_GRACE, drain).await; + } + (exit_code, self.stderr_tail_text()) + } + + fn stderr_tail_text(&self) -> String { + match self.stderr_tail.lock() { + Ok(tail) => tail + .iter() + .map(String::as_str) + .collect::>() + .join("\n"), + Err(_) => String::new(), + } + } +} + +impl Drop for ProxyCommandChild { + fn drop(&mut self) { + let Some(mut child) = self.child.take() else { + return; + }; + let _ = child.start_kill(); + // Reap asynchronously so a killed child does not linger as a zombie on + // Unix. Without an active runtime there is nothing to await: the kill + // already happened, and `kill_on_drop` remains armed as a backstop. + if let Ok(runtime) = tokio::runtime::Handle::try_current() { + runtime.spawn(async move { + let _ = child.wait().await; + }); + } + } +} + +/// Expand OpenSSH ProxyCommand tokens: `%h` (target host), `%p` (port), `%r` +/// (user), and `%%` (literal `%`). Unknown escapes stay literal, matching the +/// conservative end of OpenSSH's behavior for this release. +pub(crate) fn expand_tokens(command: &str, host: &str, port: u16, user: &str) -> String { + let mut expanded = String::with_capacity(command.len()); + let mut chars = command.chars(); + while let Some(c) = chars.next() { + if c != '%' { + expanded.push(c); + continue; + } + match chars.next() { + Some('h') => expanded.push_str(host), + Some('p') => expanded.push_str(&port.to_string()), + Some('r') => expanded.push_str(user), + Some('%') => expanded.push('%'), + Some(other) => { + expanded.push('%'); + expanded.push(other); + } + None => expanded.push('%'), + } + } + expanded +} + +fn proxy_shell_program() -> &'static str { + #[cfg(windows)] + { + "cmd.exe" + } + #[cfg(not(windows))] + { + "sh" + } +} + +fn configure_shell_args(cmd: &mut tokio::process::Command, command: &str) { + #[cfg(windows)] + { + // cmd.exe parses everything after /C itself, so the command must reach + // it raw: the standard Windows argument quoting would wrap it in extra + // quotes and mangle the quoting conventions ProxyCommand authors + // already rely on. + cmd.raw_arg("/C"); + cmd.raw_arg(command); + } + #[cfg(not(windows))] + { + cmd.arg("-c").arg(command); + } +} + +/// Drain the proxy's stderr forever: one line at a time into the log, with the +/// most recent lines kept in a bounded tail for connection error messages. +/// Returns the task handle; the task ends when stderr hits EOF (process exit). +fn spawn_stderr_drain( + stderr: ChildStderr, + tail: Arc>>, + label: String, +) -> tokio::task::JoinHandle<()> { + tokio::spawn(async move { + let mut reader = BufReader::new(stderr); + let mut line = String::new(); + loop { + line.clear(); + match reader.read_line(&mut line).await { + Ok(0) | Err(_) => break, + Ok(_) => { + let trimmed = line.trim_end(); + if !trimmed.is_empty() { + log::debug!("proxy command {label}: {trimmed}"); + } + if let Ok(mut tail) = tail.lock() { + if tail.len() == STDERR_TAIL_LINES { + tail.pop_front(); + } + let mut owned = trimmed.to_string(); + if owned.chars().count() > STDERR_LINE_MAX_CHARS { + owned = owned.chars().take(STDERR_LINE_MAX_CHARS).collect(); + owned.push_str(" [truncated]"); + } + tail.push_back(owned); + } + } + } + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn expands_tokens_and_keeps_unknown_escapes_literal() { + assert_eq!( + expand_tokens("coder ssh --stdio %h", "coder-x", 22, "alice"), + "coder ssh --stdio coder-x" + ); + assert_eq!( + expand_tokens("connect %r@%h:%p", "target", 2222, "ops"), + "connect ops@target:2222" + ); + assert_eq!(expand_tokens("100%% sure", "h", 22, "u"), "100% sure"); + // Unrecognized escapes survive untouched. + assert_eq!(expand_tokens("tool %j %n", "h", 22, "u"), "tool %j %n"); + assert_eq!(expand_tokens("trailing %", "h", 22, "u"), "trailing %"); + } + + #[cfg(windows)] + #[tokio::test] + async fn finish_reports_exit_code_and_stderr_tail() { + let mut child = + ProxyCommandChild::spawn("echo proxy-err-line 1>&2 & exit 7", "test-connection") + .unwrap(); + let _stream = child.take_stream().unwrap(); + let (exit_code, tail) = child.finish().await; + assert_eq!(exit_code, Some(7)); + assert!(tail.contains("proxy-err-line"), "stderr tail was: {tail}"); + } + + #[cfg(not(windows))] + #[tokio::test] + async fn finish_reports_exit_code_and_stderr_tail() { + let mut child = + ProxyCommandChild::spawn("echo proxy-err-line >&2; exit 7", "test-connection").unwrap(); + let _stream = child.take_stream().unwrap(); + let (exit_code, tail) = child.finish().await; + assert_eq!(exit_code, Some(7)); + assert!(tail.contains("proxy-err-line"), "stderr tail was: {tail}"); + } + + #[tokio::test] + async fn drop_kills_a_still_running_proxy() { + let script = if cfg!(windows) { + "ping -n 30 127.0.0.1 >nul" + } else { + "sleep 30" + }; + let guard = ProxyCommandChild::spawn(script, "long-running").unwrap(); + drop(guard); + // `finish` was never called; the child must not outlive the guard. A + // leaked process would show up as a dangling `ping`/`sleep` beyond this + // test's lifetime, so no explicit assertion is needed beyond the test + // finishing while the child is still running. + } +} diff --git a/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs new file mode 100644 index 0000000000..9fe2e8cd7f --- /dev/null +++ b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs @@ -0,0 +1,348 @@ +//! Merged `~/.ssh/config` loading with `Include` expansion. +//! +//! The `ssh_config` crate (0.1) parses a single file and has no `Include` +//! support, but ecosystem tooling (Coder's `config-ssh`, AtomGit, devenv) writes +//! Host blocks into separate files pulled in through `Include`. Hosts that only +//! exist behind an `Include` were therefore invisible to both the host list and +//! `get_ssh_config`. This loader merges the main file with its include tree +//! in place, preserving OpenSSH's order semantics (first occurrence of a +//! setting wins). + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +/// OpenSSH processes nested `Include` directives recursively. The cap only +/// exists to turn accidental self-reference into a bounded warning instead of a +/// hang; real-world configs stay a handful of levels deep. +const MAX_INCLUDE_DEPTH: usize = 8; + +/// Merge `config_path` with the files pulled in by its `Include` directives. +/// +/// Returns `None` when the main file is missing or unreadable, mirroring the +/// previous single-file behavior. Individual includes that fail to expand are +/// skipped with a warning so one broken include cannot hide every other host. +pub(crate) fn load_merged_config(config_path: &Path) -> Option { + let mut visited = HashSet::new(); + let mut merged = String::new(); + expand_file( + config_path, + 0, + config_path.parent(), + &mut visited, + &mut merged, + ); + if merged.trim().is_empty() { + None + } else { + Some(merged) + } +} + +/// Strip a leading UTF-8 BOM if present. Windows editors (and some SSH tooling) +/// still write one. +pub(crate) fn strip_utf8_bom(content: String) -> String { + content + .strip_prefix('\u{feff}') + .unwrap_or(&content) + .to_string() +} + +fn expand_file( + path: &Path, + depth: usize, + base_dir: Option<&Path>, + visited: &mut HashSet, + merged: &mut String, +) { + if depth > MAX_INCLUDE_DEPTH { + log::warn!( + "SSH config include chain deeper than {} levels at {}; skipping the rest", + MAX_INCLUDE_DEPTH, + path.display() + ); + return; + } + // Deduplicate on the canonical path so symlink or `..` cycles, plus plain + // repeated includes, cannot loop forever. Skipping a repeat is harmless: + // first-occurrence-wins parsing keeps the first copy authoritative anyway. + let canonical = match path.canonicalize() { + Ok(canonical) => canonical, + Err(error) => { + if depth == 0 { + log::debug!("SSH config not readable at {}: {}", path.display(), error); + } else { + log::warn!( + "SSH config include not readable at {}: {}", + path.display(), + error + ); + } + return; + } + }; + if !visited.insert(canonical) { + return; + } + let content = match std::fs::read_to_string(path) { + Ok(content) => strip_utf8_bom(content), + Err(error) => { + log::warn!( + "SSH config include not readable at {}: {}", + path.display(), + error + ); + return; + } + }; + + for line in content.lines() { + let trimmed = line.trim(); + if trimmed.is_empty() || trimmed.starts_with('#') { + merged.push_str(line); + merged.push('\n'); + continue; + } + match include_value(trimmed) { + Some(value) => { + for target in resolve_include_targets(value, base_dir) { + expand_file(&target, depth + 1, base_dir, visited, merged); + } + } + None => { + merged.push_str(line); + merged.push('\n'); + } + } + } +} + +/// Extract the value part of an `Include` directive, or `None` for any other +/// line. Accepts both `Include value` and `Include=value`; ssh_config keywords +/// are case-insensitive. +fn include_value(line: &str) -> Option<&str> { + let (keyword, value) = match line.split_once(char::is_whitespace) { + Some((keyword, value)) => (keyword, value), + None => line.split_once('=')?, + }; + if keyword.eq_ignore_ascii_case("include") { + Some(value.trim()) + } else { + None + } +} + +/// Resolve one `Include` value into concrete file paths, in order. +/// +/// Tokens may be double-quoted, may start with `~`, and may contain glob +/// patterns. Relative paths resolve against the user config directory +/// (`~/.ssh`), matching OpenSSH. Glob matches are yielded in sorted order by +/// the `glob` crate, which keeps merged output deterministic. +fn resolve_include_targets(value: &str, base_dir: Option<&Path>) -> Vec { + let mut targets = Vec::new(); + for token in split_quoted_tokens(value) { + let expanded = shellexpand::tilde(&token).into_owned(); + let candidate = PathBuf::from(&expanded); + let absolute = match base_dir { + Some(base_dir) if candidate.is_relative() => base_dir.join(candidate), + _ => candidate, + }; + // The glob crate wants forward slashes even on Windows. + let pattern = absolute.to_string_lossy().replace('\\', "/"); + if !pattern.contains(['*', '?', '[']) { + targets.push(absolute); + continue; + } + match glob::glob(&pattern) { + Ok(paths) => targets.extend(paths.filter_map(Result::ok)), + Err(error) => log::warn!( + "SSH config include pattern '{}' is invalid: {}", + token, + error + ), + } + } + targets +} + +/// Split on whitespace, keeping double-quoted spans (quote characters +/// themselves are dropped) as single tokens. +fn split_quoted_tokens(value: &str) -> Vec { + let mut tokens = Vec::new(); + let mut current = String::new(); + let mut in_quotes = false; + let mut has_token = false; + for ch in value.chars() { + match ch { + '"' => { + in_quotes = !in_quotes; + has_token = true; + } + c if c.is_whitespace() && !in_quotes => { + if has_token { + tokens.push(std::mem::take(&mut current)); + has_token = false; + } + } + c => { + current.push(c); + has_token = true; + } + } + } + if has_token { + tokens.push(current); + } + tokens +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + + fn unique_temp_dir(label: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "openbitfun-ssh-config-loader-{label}-{}-{}", + std::process::id(), + chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default() + )); + fs::create_dir_all(&dir).unwrap(); + dir + } + + fn write(path: &Path, content: &str) { + if let Some(parent) = path.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(path, content).unwrap(); + } + + #[test] + fn merges_included_files_at_the_include_position() { + let dir = unique_temp_dir("position"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Host main-only\n HostName main.example\n\nInclude coder.conf\n\nHost after-include\n HostName tail.example\n", + ); + write( + &ssh_dir.join("coder.conf"), + "Host coder-workspace\n ProxyCommand coder ssh --stdio %h\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + let main_pos = merged.find("Host main-only").unwrap(); + let coder_pos = merged.find("Host coder-workspace").unwrap(); + let tail_pos = merged.find("Host after-include").unwrap(); + assert!(main_pos < coder_pos && coder_pos < tail_pos); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn resolves_relative_includes_against_the_ssh_directory() { + let dir = unique_temp_dir("relative"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Include extra/host.conf\nHost base\n HostName base.example\n", + ); + write( + &ssh_dir.join("extra").join("host.conf"), + "Host nested\n HostName nested.example\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + assert!(merged.contains("Host nested")); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn expands_globs_in_sorted_order() { + let dir = unique_temp_dir("glob"); + let ssh_dir = dir.join(".ssh"); + write(&ssh_dir.join("config"), "Include parts/*.conf\n"); + write(&ssh_dir.join("parts").join("b.conf"), "Host from-b\n"); + write(&ssh_dir.join("parts").join("a.conf"), "Host from-a\n"); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + let a_pos = merged.find("Host from-a").unwrap(); + let b_pos = merged.find("Host from-b").unwrap(); + assert!(a_pos < b_pos, "glob matches must be merged in sorted order"); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn keeps_parsing_when_an_include_is_missing_or_loops() { + let dir = unique_temp_dir("cycle"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Include missing.conf\nInclude cycle-a.conf\nHost base\n HostName base.example\n", + ); + write( + &ssh_dir.join("cycle-a.conf"), + "Include cycle-b.conf\nHost a\n", + ); + write( + &ssh_dir.join("cycle-b.conf"), + "Include cycle-a.conf\nHost b\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + assert!(merged.contains("Host a")); + assert!(merged.contains("Host b")); + assert_eq!(merged.matches("Host a").count(), 1); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn returns_none_when_the_main_config_is_missing() { + let dir = unique_temp_dir("missing-main"); + assert!(load_merged_config(&dir.join(".ssh").join("config")).is_none()); + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn strips_utf8_bom_from_the_main_config() { + let dir = unique_temp_dir("bom"); + let ssh_dir = dir.join(".ssh"); + write(&ssh_dir.join("config"), "\u{feff}Host bom-host\n"); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + assert!(merged.starts_with("Host bom-host")); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn include_value_accepts_keyword_forms_case_insensitively() { + assert_eq!(include_value("Include coder.conf"), Some("coder.conf")); + assert_eq!(include_value("include=parts/*.conf"), Some("parts/*.conf")); + assert_eq!( + include_value("INCLUDE \"path with spaces.conf\" "), + Some("\"path with spaces.conf\"") + ); + assert_eq!(include_value("HostName example"), None); + assert_eq!(include_value("Host=example"), None); + } + + #[test] + fn quoted_include_tokens_stay_whole() { + let dir = unique_temp_dir("quoted"); + let ssh_dir = dir.join(".ssh"); + write(&ssh_dir.join("my hosts.conf"), "Host quoted\n"); + write( + &ssh_dir.join("config"), + "Include \"my hosts.conf\"\nHost base\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + assert!(merged.contains("Host quoted")); + + fs::remove_dir_all(&dir).unwrap(); + } +} diff --git a/src/crates/services/services-integrations/src/remote_ssh/types.rs b/src/crates/services/services-integrations/src/remote_ssh/types.rs index db4a1b9c29..0a965352f5 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/types.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/types.rs @@ -52,6 +52,15 @@ pub struct SSHConnectionConfig { /// SSH config entry, so every hop may use a different user and identity. #[serde(default)] pub proxy_jump: Option, + /// OpenSSH-compatible `ProxyCommand`: a local shell command whose stdin/stdout + /// carries the SSH transport to the target (e.g. `coder ssh --stdio`). + /// + /// `%h` (target host), `%p` (port), `%r` (user), and `%%` are expanded before + /// the command runs. Mutually exclusive with `proxy_jump` and container + /// targets; the target host is typically a bare `~/.ssh/config` alias with no + /// `HostName`, because the proxy command resolves the real endpoint itself. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub proxy_command: Option, /// Optional Docker container that becomes the effective workspace target. #[serde(default)] pub container: Option, @@ -74,6 +83,7 @@ impl SSHConnectionConfig { && self.username == other.username && self.auth.connection_params_equal(&other.auth) && self.proxy_jump == other.proxy_jump + && self.proxy_command == other.proxy_command && self.container == other.container && self.wsl == other.wsl && self.options == other.options @@ -361,6 +371,8 @@ pub struct SavedConnection { pub last_connected: Option, #[serde(default)] pub proxy_jump: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub proxy_command: Option, #[serde(default)] pub container: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -608,6 +620,9 @@ pub struct SSHConfigEntry { /// OpenSSH ProxyJump chain, preserving aliases and order. #[serde(default)] pub proxy_jump: Option, + /// OpenSSH ProxyCommand resolved for this host, if any. + #[serde(default)] + pub proxy_command: Option, } /// Result of looking up SSH config for a host diff --git a/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_contracts.rs b/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_contracts.rs index afe5c7b17b..b9cf3083bf 100644 --- a/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_contracts.rs +++ b/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_contracts.rs @@ -85,6 +85,7 @@ fn remote_target_contract_uses_proxy_jump_and_kebab_case_container_access() { }, default_workspace: Some("/workspace".to_string()), proxy_jump: Some("jump1,jump2".to_string()), + proxy_command: None, container: Some(ContainerWorkspaceConfig { name: "trainer-dev".to_string(), access: ContainerAccess::DockerExec, @@ -109,6 +110,76 @@ fn remote_target_contract_uses_proxy_jump_and_kebab_case_container_access() { ); } +#[test] +fn remote_target_contract_carries_proxy_command_and_stays_back_compatible() { + // A payload that names a ProxyCommand must round-trip through the camelCase + // wire shape and mark the config as drift-changed when the command changes. + let config: SSHConnectionConfig = serde_json::from_value(serde_json::json!({ + "id": "conn-1", + "name": "coder", + "host": "coder-vscode.coder.sage.org.ai--abc", + "port": 22, + "username": "alice", + "auth": { "type": "PrivateKey", "keyPath": "~/.ssh/coder_deploy" }, + "defaultWorkspace": "/home/coder", + "proxyCommand": "\"coder\" --url https://coder.example:9443 ssh --stdio %h" + })) + .unwrap(); + assert_eq!( + config.proxy_command.as_deref(), + Some("\"coder\" --url https://coder.example:9443 ssh --stdio %h") + ); + + let json = serde_json::to_value(&config).unwrap(); + assert_eq!( + json["proxyCommand"], + "\"coder\" --url https://coder.example:9443 ssh --stdio %h" + ); + let round_trip: SSHConnectionConfig = serde_json::from_value(json).unwrap(); + assert_eq!( + round_trip.proxy_command.as_deref(), + config.proxy_command.as_deref() + ); + assert!(config.connection_params_equal(&round_trip)); + + // Older clients and persisted files predate the field: it must deserialize + // as absent, and two such configs must still compare equal. + let legacy: SSHConnectionConfig = serde_json::from_value(serde_json::json!({ + "id": "conn-1", + "name": "coder", + "host": "coder-vscode.coder.sage.org.ai--abc", + "port": 22, + "username": "alice", + "auth": { "type": "PrivateKey", "keyPath": "~/.ssh/coder_deploy" }, + "defaultWorkspace": "/home/coder" + })) + .unwrap(); + assert_eq!(legacy.proxy_command, None); + assert!(!config.connection_params_equal(&legacy)); +} + +#[test] +#[cfg(feature = "remote-persistence")] +fn saved_connection_record_tolerates_legacy_payloads_without_proxy_command() { + let saved: openbitfun_services_integrations::remote_persistence::SavedConnectionRecord = + serde_json::from_value(serde_json::json!({ + "id": "conn-1", + "name": "coder", + "host": "coder-vscode.coder.sage.org.ai--abc", + "port": 22, + "username": "alice", + "authType": { "type": "PrivateKey", "keyPath": "~/.ssh/coder_deploy" }, + "defaultWorkspace": "/home/coder", + "lastConnected": 1 + })) + .unwrap(); + assert_eq!(saved.proxy_command, None); + assert_eq!(saved.proxy_jump, None); + + let written = serde_json::to_value(&saved).unwrap(); + assert!(written.get("proxyCommand").is_none()); +} + #[test] fn remote_workspace_defaults_keep_older_files_loadable() { let workspace: RemoteWorkspace = serde_json::from_value(serde_json::json!({ diff --git a/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_disabled_contracts.rs b/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_disabled_contracts.rs index 80300b009f..e2d557bbb2 100644 --- a/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_disabled_contracts.rs +++ b/src/crates/services/services-integrations/tests/remote_ssh_contracts/remote_ssh_disabled_contracts.rs @@ -21,6 +21,7 @@ fn test_config() -> SSHConnectionConfig { }, default_workspace: Some("/repo".to_string()), proxy_jump: None, + proxy_command: None, container: None, wsl: None, options: Default::default(), From a9171a36a8e8065fd44b5193e7e8af6c9771ecbc Mon Sep 17 00:00:00 2001 From: Raing5Days Date: Thu, 29 Oct 2026 22:17:24 +0800 Subject: [PATCH 2/4] fix(remote-ssh): propagate proxy command through port forwarding --- src/apps/desktop/src/api/ssh_api.rs | 1 + .../src/agentic/tools/implementations/port_forward_tool.rs | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/apps/desktop/src/api/ssh_api.rs b/src/apps/desktop/src/api/ssh_api.rs index 555c6ac74b..6cf11d3e9b 100644 --- a/src/apps/desktop/src/api/ssh_api.rs +++ b/src/apps/desktop/src/api/ssh_api.rs @@ -1506,6 +1506,7 @@ mod tests { }, default_workspace: Some("/workspace".to_string()), proxy_jump: None, + proxy_command: None, container: Some(ContainerWorkspaceConfig { name: "dev".to_string(), access: ContainerAccess::DockerExec, diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/port_forward_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/port_forward_tool.rs index f7d532a6a2..8658eeb465 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/port_forward_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/port_forward_tool.rs @@ -234,7 +234,10 @@ impl PortForwardTool { username, auth, default_workspace: None, - proxy_jump: config_entry.and_then(|entry| entry.proxy_jump), + proxy_jump: config_entry + .as_ref() + .and_then(|entry| entry.proxy_jump.clone()), + proxy_command: config_entry.and_then(|entry| entry.proxy_command), container: None, wsl: None, options: SSHConnectionOptions::default(), From 34f8a8be77776cdf9d9f60b31c0d6c858e103038 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Mon, 2 Nov 2026 10:22:27 +0800 Subject: [PATCH 3/4] fix(remote-ssh): resolve ProxyCommand lines and reap proxy process trees - Replace the ssh_config 0.1 crate (pairwise tokenizer, equality-only Host matching, last value wins) with a line-based resolver over the Include-merged text: Host wildcards and negation, first value wins, whole-line ProxyCommand, and ProxyCommand none. - Restore the parent Host/Match context after each Include, detect cycles on the recursion stack only, cap depth at 16, accept Include=, and load the config on the blocking pool. - Supervise ProxyCommand through ProcessTreeChild: sh -c "exec ..." in its own process group on Unix, a kill-on-close Job on Windows. Tear down the whole tree on drop, failure, and disconnect; on reconnect keep a displaced proxy only until its old transport drains instead of leaking it. - Reject %h/%r substitutions containing shell metacharacters (CVE-2023-51385) before anything runs. - Fail ProxyJump hops whose alias resolves to a ProxyCommand explicitly. - Advertise ssh_proxy_command_v1 from desktop hosts and document it. - Send the unpublished-session goodbye before tearing down proxy children and drop a duplicated cfg attribute. --- Cargo.lock | 7 - Cargo.toml | 1 - .../remote-workspace-transport.md | 24 + .../core-boundaries/rules/feature-rules.mjs | 1 - .../generated/remote-surface-registry.json | 4 +- .../src/remote_surface/capabilities.rs | 7 +- .../services/services-integrations/Cargo.toml | 3 +- .../src/remote_ssh/manager.rs | 454 +++++++++++------- .../src/remote_ssh/mod.rs | 1 - .../src/remote_ssh/proxy_command.rs | 316 +++++++++--- .../src/remote_ssh/ssh_config_loader.rs | 348 ++++++++++++-- .../api/generated/remoteSurface.ts | 4 +- 12 files changed, 886 insertions(+), 284 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 63c9a64329..b70be614e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6801,7 +6801,6 @@ dependencies = [ "sherpa-onnx", "sioc", "sse-stream", - "ssh_config", "tar", "tempfile", "terminal-core", @@ -10033,12 +10032,6 @@ dependencies = [ "zeroize", ] -[[package]] -name = "ssh_config" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "08044686521f352c6d1f522ba349c3dd52813819fa8353b60bd3d2107f6dca0a" - [[package]] name = "stable_deref_trait" version = "1.2.1" diff --git a/Cargo.toml b/Cargo.toml index 7af9353485..357d59464b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -309,7 +309,6 @@ russh = "0.45" russh-sftp = "2.1" russh-keys = "0.45" shellexpand = "3" -ssh_config = "0.1" rustls = { version = "0.23", default-features = false } [patch.crates-io] diff --git a/docs/architecture/remote-workspace-transport.md b/docs/architecture/remote-workspace-transport.md index 28ad3c833b..ee98b74753 100644 --- a/docs/architecture/remote-workspace-transport.md +++ b/docs/architecture/remote-workspace-transport.md @@ -115,6 +115,30 @@ unsupported by the Product Operation Registry. Existing Remote Control sessions reuse the bound workspace providers, and Detached Dispatch does not create WSL profiles. Native WSL has no SSH transport for port forwarding. +## ProxyCommand + +The additive `proxyCommand` profile field (and `~/.ssh/config` `ProxyCommand`) +carries the SSH transport over a local process's stdin/stdout, as OpenSSH does +(for example `coder ssh --stdio %h`). Missing `proxyCommand` preserves direct +dialing; connection drift includes the field. `%h`, `%p`, `%r`, and `%%` are +expanded first, and a host or user that would be substituted must match +`[A-Za-z0-9._\-@:%\[\]]` (OpenSSH 9.6, CVE-2023-51385) or the connection fails +before anything runs. Unix runs `sh -c "exec …"` in its own process group; +Windows runs `cmd.exe /C` inside a kill-on-close Job. Disconnect, failed +handshakes, and reconnect tear the whole process tree down; a reconnect keeps a +displaced proxy only until its old transport has no users left. + +ProxyCommand is exclusive with ProxyJump, containers, and WSL. A ProxyJump hop +whose `~/.ssh/config` alias resolves to a ProxyCommand fails with an explicit +unsupported error instead of dialing the hop directly. `~/.ssh/config` values +come from a line-based resolver over the `Include`-merged text (Host patterns +with `*`, `?`, and `!`, first value wins, whole-line `ProxyCommand`, `none` +disables). + +The host advertises `ssh_proxy_command_v1`. Older hosts ignore the unknown +field and would dial directly, so controllers must not send `proxyCommand` to a +peer that lacks the capability. CLI peer setup is unsupported, as for WSL. + ## ProxyJump The comma-separated jump chain is resolved left to right. Each token can be a diff --git a/scripts/core-boundaries/rules/feature-rules.mjs b/scripts/core-boundaries/rules/feature-rules.mjs index 2866ffcdee..052e0d6b11 100644 --- a/scripts/core-boundaries/rules/feature-rules.mjs +++ b/scripts/core-boundaries/rules/feature-rules.mjs @@ -353,7 +353,6 @@ export const optionalDependencyFeatureOwnerRules = [ { depName: 'sherpa-onnx', ownerFeatures: ['speech'] }, { depName: 'shellexpand', ownerFeatures: ['remote-ssh-concrete'] }, { depName: 'sse-stream', ownerFeatures: ['mcp'] }, - { depName: 'ssh_config', ownerFeatures: ['remote-ssh-concrete', 'ssh_config'] }, { depName: 'terminal-core', ownerFeatures: ['remote-ssh', 'remote-ssh-concrete'] }, { depName: 'tar', ownerFeatures: ['speech'] }, { depName: 'thiserror', ownerFeatures: ['account-identity', 'browser-control', 'git', 'hook-import', 'miniapp-market', 'plugin-source', 'remote-connect', 'remote-ssh', 'remote-ssh-concrete', 'review-platform', 'speech', 'web-tools', 'workspace-search'] }, diff --git a/src/crates/contracts/product-domains/src/generated/remote-surface-registry.json b/src/crates/contracts/product-domains/src/generated/remote-surface-registry.json index fa268d4b0c..23824875d5 100644 --- a/src/crates/contracts/product-domains/src/generated/remote-surface-registry.json +++ b/src/crates/contracts/product-domains/src/generated/remote-surface-registry.json @@ -1,6 +1,6 @@ { "schemaVersion": 1, - "digest": "fnv1a64:ee62faff5c97f12a", + "digest": "fnv1a64:fc41c72be879cce0", "retiredCommandPrefixes": [ { "prefix": "lsp_", @@ -33,6 +33,7 @@ "miniapp_agent_context_files_v1", "miniapp_agent_images_v1", "wsl_workspaces_v1", + "ssh_proxy_command_v1", "computer_use_sharing_v1", "product_control_v1", "remote_connect_control_v1", @@ -75,6 +76,7 @@ "miniapp_agent_context_files_v1", "miniapp_agent_images_v1", "wsl_workspaces_v1", + "ssh_proxy_command_v1", "computer_use_sharing_v1", "product_control_v1", "remote_connect_control_v1", diff --git a/src/crates/contracts/product-domains/src/remote_surface/capabilities.rs b/src/crates/contracts/product-domains/src/remote_surface/capabilities.rs index 038a3dd378..a24431f8d2 100644 --- a/src/crates/contracts/product-domains/src/remote_surface/capabilities.rs +++ b/src/crates/contracts/product-domains/src/remote_surface/capabilities.rs @@ -69,6 +69,9 @@ pub enum PeerHostCapability { MiniappAgentImagesV1, /// WSL profiles and discovery are understood; availability is probed on the host. WslWorkspacesV1, + /// Saved SSH profiles may carry `proxyCommand`; the host runs it as the + /// SSH transport. Older hosts drop the field and dial directly. + SshProxyCommandV1, /// User-selected native window sharing and explicit permission receipts. ComputerUseSharingV1, /// `product_control_invoke` is implemented (shared config contract). @@ -129,6 +132,7 @@ impl PeerHostCapability { Self::MiniappAgentContextFilesV1, Self::MiniappAgentImagesV1, Self::WslWorkspacesV1, + Self::SshProxyCommandV1, Self::ComputerUseSharingV1, Self::ProductControlV1, Self::RemoteConnectControlV1, @@ -174,6 +178,7 @@ impl PeerHostCapability { Self::MiniappAgentContextFilesV1 => "miniapp_agent_context_files_v1", Self::MiniappAgentImagesV1 => "miniapp_agent_images_v1", Self::WslWorkspacesV1 => "wsl_workspaces_v1", + Self::SshProxyCommandV1 => "ssh_proxy_command_v1", Self::ComputerUseSharingV1 => "computer_use_sharing_v1", Self::ProductControlV1 => "product_control_v1", Self::RemoteConnectControlV1 => "remote_connect_control_v1", @@ -201,7 +206,7 @@ impl PeerHostCapability { const DESKTOP_CAPABILITIES: &[PeerHostCapability] = PeerHostCapability::ALL; -/// The CLI peer host has no MiniApp runtime, WSL connection setup, host-native +/// The CLI peer host has no MiniApp runtime, WSL or SSH connection setup, host-native /// ProductControl providers, or presentation surface. const CLI_CAPABILITIES: &[PeerHostCapability] = &[ PeerHostCapability::ModelRoutingV1, diff --git a/src/crates/services/services-integrations/Cargo.toml b/src/crates/services/services-integrations/Cargo.toml index 46771da3ed..6e3a86f6eb 100644 --- a/src/crates/services/services-integrations/Cargo.toml +++ b/src/crates/services/services-integrations/Cargo.toml @@ -76,7 +76,6 @@ russh = { workspace = true, optional = true } russh-sftp = { workspace = true, optional = true } russh-keys = { workspace = true, optional = true } shellexpand = { workspace = true, optional = true } -ssh_config = { workspace = true, optional = true } terminal-core = { path = "../terminal", optional = true } x25519-dalek = { workspace = true, optional = true } @@ -512,7 +511,7 @@ product-full = [ "workspace-search", ] skillhub = ["openbitfun-services-core/tls-provider", "reqwest", "reqwest/json", "reqwest/query", "reqwest/rustls-no-provider", "reqwest/stream", "zip", "tempfile", "tokio/fs", "tokio/io-util"] -ssh_config = ["dep:ssh_config", "dep:glob"] +ssh_config = ["dep:glob"] [dev-dependencies] tempfile = { workspace = true } diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index a426618564..0757364789 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -5,7 +5,9 @@ use super::proxy_command::{expand_tokens, ProxyCommandChild}; use super::sftp_file::ManagedSftpSession; #[cfg(feature = "ssh_config")] -use super::ssh_config_loader::load_merged_config; +use super::ssh_config_loader::{ + is_host_pattern, load_merged_config_async, resolve_host_settings, split_directive, +}; use crate::remote_ssh::password_vault::SSHPasswordVault; use crate::remote_ssh::types::{ ConnectionTestReport, ConnectionTestStage, ContainerAccess, ContainerWorkspaceConfig, @@ -22,8 +24,6 @@ use russh_keys::PublicKeyBase64; use russh_sftp::client::error::Error as SftpError; use russh_sftp::client::{RawSftpSession, SftpSession}; use russh_sftp::protocol::{File as SftpFile, StatusCode as SftpStatusCode}; -#[cfg(feature = "ssh_config")] -use ssh_config::SSHConfig; use std::collections::HashMap; use std::process::Stdio; use std::sync::atomic::{AtomicBool, Ordering}; @@ -36,6 +36,9 @@ use tokio::time::{Duration, Instant}; const SSH_COMMAND_WAIT_POLL_INTERVAL: Duration = Duration::from_millis(100); const SSH_COMMAND_INTERRUPT_DRAIN_GRACE: Duration = Duration::from_millis(500); const SSH_COMMAND_OWNER_DRAIN_GRACE: Duration = Duration::from_secs(2); +/// How often a displaced ProxyCommand process checks whether its old transport +/// has drained after a reconnect. +const STALE_PROXY_POLL_INTERVAL: Duration = Duration::from_secs(2); /// The caller owns cancellation, while the spawned transport task owns the /// channel until interruption and draining have finished. The caller may stop @@ -422,24 +425,53 @@ fn truncate_at_char_boundary(s: &str, max_bytes: usize) -> &str { &s[..end] } -/// OpenSSH keyword matching is case-insensitive, but `ssh_config` stores keys as written in the file -/// (e.g. `HostName` vs `Hostname`). Resolve by ASCII case-insensitive compare. +/// Single-argument value of a resolved directive (see +/// [`resolve_host_settings`], which keys by lowercase keyword). #[cfg(feature = "ssh_config")] -fn ssh_cfg_get<'a>( - settings: &std::collections::HashMap<&'a str, &'a str>, - canonical_key: &str, +fn resolved_value<'a>( + settings: &'a std::collections::HashMap, + key: &str, ) -> Option<&'a str> { settings - .iter() - .find(|(k, _)| k.eq_ignore_ascii_case(canonical_key)) - .map(|(_, v)| *v) + .get(key) + .and_then(|value| parse_ssh_config_value(value)) } +/// Resolved `ProxyCommand`, or `None` when absent or explicitly `none`. #[cfg(feature = "ssh_config")] -fn ssh_cfg_has(settings: &std::collections::HashMap<&str, &str>, canonical_key: &str) -> bool { +fn resolved_proxy_command(settings: &std::collections::HashMap) -> Option { settings - .keys() - .any(|k| k.eq_ignore_ascii_case(canonical_key)) + .get("proxycommand") + .and_then(|value| parse_ssh_config_command_value(value)) + .filter(|command| !command.eq_ignore_ascii_case("none")) +} + +/// Build an [`SSHConfigEntry`] from resolved settings for `host`. +#[cfg(feature = "ssh_config")] +fn ssh_config_entry_from_settings( + host: &str, + settings: &std::collections::HashMap, +) -> SSHConfigEntry { + let proxy_command = resolved_proxy_command(settings); + SSHConfigEntry { + host: host.to_string(), + hostname: resolved_value(settings, "hostname").map(ToOwned::to_owned), + port: resolved_value(settings, "port").and_then(|port| port.parse::().ok()), + user: resolved_value(settings, "user").map(ToOwned::to_owned), + identity_file: resolved_value(settings, "identityfile") + .map(|file| shellexpand::tilde(file).to_string()), + agent: if proxy_command.is_some() { + None + } else { + Some(true) + }, + certificate_file: resolved_value(settings, "certificatefile") + .map(|file| shellexpand::tilde(file).to_string()), + proxy_jump: resolved_value(settings, "proxyjump") + .filter(|jump| !jump.eq_ignore_ascii_case("none")) + .map(ToOwned::to_owned), + proxy_command, + } } /// Extract the first value from an SSH config directive line, handling double-quoted strings. @@ -479,15 +511,10 @@ fn parse_ssh_config_command_value(value: &str) -> Option { /// Manually parse `~/.ssh/config` content into Host blocks with their direct settings. /// -/// This is a fallback for when `SSHConfig::parse_str` fails — which happens when the -/// config contains directives (e.g. `Include`, `Match`) before the first `Host` block, -/// because `ssh_config` 0.1's `EntryParser` returns `InvalidHostEntry` for any key-value -/// pair without a preceding `Host` directive, and `parse_str` fails the entire operation -/// on the first parser error. -/// -/// Only direct settings within each `Host` block are captured; pattern-matched settings -/// from wildcard blocks (e.g. `Host *`) are not applied. Use `SSHConfig::query()` as a -/// supplementary source when `parse_str` succeeds. +/// This enumerates the connectable aliases (`ssh_config` 0.1 cannot list +/// hosts). Only direct settings within each `Host` block are captured; +/// [`resolve_host_settings`] supplies the OpenSSH-resolved values that callers +/// layer on top. A `Match` line closes the current Host block. #[cfg(feature = "ssh_config")] fn parse_ssh_config_manually(content: &str) -> Vec { let mut hosts = Vec::new(); @@ -506,12 +533,12 @@ fn parse_ssh_config_manually(content: &str) -> Vec { continue; } - let (keyword, value) = match line.split_once(char::is_whitespace) { - Some((k, v)) => (k, v.trim()), + let (keyword, value) = match split_directive(line) { + Some((k, v)) => (k, v), None => continue, }; - if keyword.eq_ignore_ascii_case("Host") { + if keyword.eq_ignore_ascii_case("Host") || keyword.eq_ignore_ascii_case("Match") { // Save previous host block if let Some(host) = current_host.take() { hosts.push(SSHConfigEntry { @@ -527,8 +554,18 @@ fn parse_ssh_config_manually(content: &str) -> Vec { }); } - // Start new host block — take the first alias - current_host = value.split_whitespace().next().map(|s| s.to_string()); + // Start new host block — take the first connectable alias. Pure + // pattern blocks (`Host *`, `Host !x *.corp`) and `Match` blocks + // name no host, so their settings are not attributed to anyone. + current_host = if keyword.eq_ignore_ascii_case("Host") { + value + .split(|c: char| c.is_whitespace() || c == ',') + .map(|alias| alias.trim_matches('"')) + .find(|alias| !alias.is_empty() && !is_host_pattern(alias)) + .map(|s| s.to_string()) + } else { + None + }; block_hostname = None; block_port = None; block_user = None; @@ -728,11 +765,18 @@ impl Drop for UnpublishedSshSession { return; } let handles = std::mem::take(&mut self.handles); + // The proxy processes carry the transports being told goodbye, so + // they move into the task and are torn down only after it; dropping + // them here would cut the pipe before the disconnect message is sent. + let proxy_children = std::mem::take(&mut self.proxy_children); match tokio::runtime::Handle::try_current() { Ok(runtime) => { runtime.spawn(async move { let chain = handles.iter().rev().map(Arc::as_ref).collect(); SSHConnectionManager::shutdown_handle_chain(chain).await; + for child in proxy_children { + child.shutdown().await; + } }); } Err(error) => log::warn!( @@ -2451,77 +2495,45 @@ impl SSHConnectionManager { // Load the user config merged with its `Include` tree; ecosystem // tooling (Coder, AtomGit, devenv) writes Host blocks into included // files, and `ssh_config` 0.1 cannot expand them itself. - let config_content = match load_merged_config(&ssh_config_path) { - Some(content) => content, - None => { - log::debug!("SSH config not found at {:?}", ssh_config_path); - return SSHConfigLookupResult { - found: false, - config: None, - }; - } + let Some(config_content) = load_merged_config_async(ssh_config_path.clone()).await else { + log::debug!("SSH config not found at {:?}", ssh_config_path); + return SSHConfigLookupResult { + found: false, + config: None, + }; }; + Self::lookup_ssh_config_in(&config_content, host) + } - // Try to parse with the ssh_config crate for pattern-matching query support. - // If parsing fails, fall back to manual parsing. - let parsed_config = SSHConfig::parse_str(&config_content).ok(); - if parsed_config.is_none() { - log::debug!("SSHConfig::parse_str failed, using manual fallback"); - } - - // Try pattern-matched query first (handles Host wildcards like `Host *.example.com`) - if let Some(ref config) = parsed_config { - let host_settings = config.query(host); - if !host_settings.is_empty() { - log::debug!( - "Found SSH config for host: {} with {} settings", - host, - host_settings.len() - ); - - let hostname = ssh_cfg_get(&host_settings, "HostName").map(|s| s.to_string()); - let user = ssh_cfg_get(&host_settings, "User").map(|s| s.to_string()); - let port = ssh_cfg_get(&host_settings, "Port").and_then(|s| s.parse::().ok()); - let identity_file = ssh_cfg_get(&host_settings, "IdentityFile") - .map(|f| shellexpand::tilde(f).to_string()); - let certificate_file = ssh_cfg_get(&host_settings, "CertificateFile") - .map(|f| shellexpand::tilde(f).to_string()); - let has_proxy_command = ssh_cfg_has(&host_settings, "ProxyCommand"); - let proxy_jump = ssh_cfg_get(&host_settings, "ProxyJump").map(ToOwned::to_owned); - let proxy_command = - ssh_cfg_get(&host_settings, "ProxyCommand").map(ToOwned::to_owned); - - return SSHConfigLookupResult { - found: true, - config: Some(SSHConfigEntry { - host: host.to_string(), - hostname, - port, - user, - identity_file, - agent: if has_proxy_command { None } else { Some(true) }, - certificate_file, - proxy_jump, - proxy_command, - }), - }; - } - } - - // Fallback: manual lookup for exact host name match - let hosts = parse_ssh_config_manually(&config_content); - if let Some(entry) = hosts.into_iter().find(|e| e.host == host) { - log::debug!("Found SSH config for host: {} (manual fallback)", host); - return SSHConfigLookupResult { + /// Resolve `host` against already-merged config text with OpenSSH + /// semantics (pattern matching, first value wins, whole-line values). + #[cfg(feature = "ssh_config")] + fn lookup_ssh_config_in(config_content: &str, host: &str) -> SSHConfigLookupResult { + let settings = resolve_host_settings(config_content, host); + let entry = ssh_config_entry_from_settings(host, &settings); + let found = entry.hostname.is_some() + || entry.port.is_some() + || entry.user.is_some() + || entry.identity_file.is_some() + || entry.certificate_file.is_some() + || entry.proxy_jump.is_some() + || entry.proxy_command.is_some(); + if found { + log::debug!( + "Found SSH config for host: {} with {} settings", + host, + settings.len() + ); + SSHConfigLookupResult { found: true, config: Some(entry), - }; - } - - log::debug!("No SSH config found for host: {}", host); - SSHConfigLookupResult { - found: false, - config: None, + } + } else { + log::debug!("No SSH config found for host: {}", host); + SSHConfigLookupResult { + found: false, + config: None, + } } } @@ -2541,63 +2553,44 @@ impl SSHConnectionManager { .unwrap_or_default(); // Merged view including the `Include` tree; see `get_ssh_config`. - let config_content = match load_merged_config(&ssh_config_path) { - Some(content) => content, - None => { - log::debug!("SSH config not found at {:?}", ssh_config_path); - return Vec::new(); - } + let Some(config_content) = load_merged_config_async(ssh_config_path.clone()).await else { + log::debug!("SSH config not found at {:?}", ssh_config_path); + return Vec::new(); }; - - // Try to parse with the ssh_config crate for pattern-matching query support. - // If parsing fails (e.g. `Include`/`Match` directives before the first `Host` - // block cause ssh_config 0.1 to return InvalidHostEntry), fall back to manual - // line-by-line parsing. - let parsed_config = SSHConfig::parse_str(&config_content).ok(); - if parsed_config.is_none() { - log::debug!("SSHConfig::parse_str failed, using manual fallback"); - } - - // Always do manual parsing to get the host list (SSHConfig doesn't expose all hosts). - let mut hosts = parse_ssh_config_manually(&config_content); - - // When the ssh_config crate parsed successfully, override with pattern-matched - // details (e.g. settings from `Host *` blocks that apply to all hosts). - if let Some(ref config) = parsed_config { - for entry in &mut hosts { - let settings = config.query(&entry.host); - if !settings.is_empty() { - if let Some(h) = ssh_cfg_get(&settings, "HostName") { - entry.hostname = Some(h.to_string()); - } - if let Some(p) = - ssh_cfg_get(&settings, "Port").and_then(|s| s.parse::().ok()) - { - entry.port = Some(p); - } - if let Some(u) = ssh_cfg_get(&settings, "User") { - entry.user = Some(u.to_string()); - } - if let Some(f) = ssh_cfg_get(&settings, "IdentityFile") { - entry.identity_file = Some(shellexpand::tilde(f).to_string()); - } - if let Some(f) = ssh_cfg_get(&settings, "CertificateFile") { - entry.certificate_file = Some(shellexpand::tilde(f).to_string()); - } - if let Some(proxy_jump) = ssh_cfg_get(&settings, "ProxyJump") { - entry.proxy_jump = Some(proxy_jump.to_string()); - } - if let Some(proxy_command) = ssh_cfg_get(&settings, "ProxyCommand") { - entry.proxy_command = Some(proxy_command.to_string()); - } - } - } - } - + let hosts = Self::list_ssh_config_hosts_in(&config_content); log::debug!("Found {} hosts in SSH config", hosts.len()); hosts } + /// Enumerate connectable aliases from merged config text, each resolved + /// with OpenSSH semantics. Aliases repeated across blocks (or re-emitted + /// after an `Include`) are listed once. + #[cfg(feature = "ssh_config")] + fn list_ssh_config_hosts_in(config_content: &str) -> Vec { + let mut seen = std::collections::HashSet::new(); + parse_ssh_config_manually(config_content) + .into_iter() + .filter(|entry| seen.insert(entry.host.clone())) + .map(|entry| { + let settings = resolve_host_settings(config_content, &entry.host); + let resolved = ssh_config_entry_from_settings(&entry.host, &settings); + SSHConfigEntry { + hostname: resolved.hostname.or(entry.hostname), + port: resolved.port.or(entry.port), + user: resolved.user.or(entry.user), + identity_file: resolved.identity_file.or(entry.identity_file), + certificate_file: resolved.certificate_file.or(entry.certificate_file), + proxy_jump: resolved.proxy_jump, + // The resolver is authoritative for ProxyCommand: it honors + // `ProxyCommand none` and first-value-wins across blocks. + agent: resolved.agent, + proxy_command: resolved.proxy_command, + host: entry.host, + } + }) + .collect() + } + #[cfg(not(feature = "ssh_config"))] pub async fn list_ssh_config_hosts(&self) -> Vec { Vec::new() @@ -3656,6 +3649,19 @@ impl SSHConnectionManager { fallback_key_path: Some("~/.ssh/id_rsa".to_string()), } }; + if entry + .as_ref() + .is_some_and(|entry| entry.proxy_command.is_some()) + { + // Dialing the hop directly would silently bypass the transport + // the user configured for it; say so instead. + anyhow::bail!( + "ProxyJump entry '{}' uses a ProxyCommand in ~/.ssh/config, which is not \ + supported for jump hosts yet; connect to that host directly with its \ + ProxyCommand or remove it from the jump chain", + value + ); + } result.push(SSHConnectionConfig { id: format!("{}-jump-{}", config.id, index + 1), name: value.to_string(), @@ -3780,8 +3786,10 @@ impl SSHConnectionManager { let stage = format!("proxy command {}", connection_label(config)); let mut unpublished = UnpublishedSshSession::default(); - let expanded = expand_tokens(proxy_command, &config.host, config.port, &config.username); + let expanded = expand_tokens(proxy_command, &config.host, config.port, &config.username) + .map_err(|error| tag_failed_stage("proxy command", error))?; let mut child = ProxyCommandChild::spawn(&expanded, &stage) + .await .map_err(|error| tag_failed_stage("proxy command", error))?; let stream = child .take_stream() @@ -4387,6 +4395,30 @@ impl SSHConnectionManager { /// closing hop 1 first would cut the channel the later disconnects still /// have to travel over and turn an orderly shutdown back into N abandoned /// sessions. + /// Keep a displaced ProxyCommand process alive until the old transport it + /// carries has no users left (or the proxy exits by itself), then tear the + /// process tree down and reap it. + fn reap_proxy_after_transport_drains( + connection_id: String, + transport: std::sync::Weak>, + mut child: ProxyCommandChild, + ) { + tokio::spawn(async move { + let mut interval = tokio::time::interval(STALE_PROXY_POLL_INTERVAL); + loop { + interval.tick().await; + if transport.strong_count() == 0 || child.has_exited() { + break; + } + } + child.shutdown().await; + log::debug!( + "SSH session {} released the previous ProxyCommand process", + connection_id + ); + }); + } + async fn shutdown_handle_chain(handles: Vec<&Handle>) { for handle in handles { Self::shutdown_handle(handle).await; @@ -4773,18 +4805,21 @@ impl SSHConnectionManager { // alive otherwise — the same lease the skipped goodbye grants the old // handle while in-flight work drains. if let Some(child) = stale_proxy_child { - if stale_handle - .as_ref() - .map_or(true, |handle| Arc::strong_count(handle) == 1) - { - drop(child); - } else { - log::info!( - "SSH session {} reconnect keeps the previous ProxyCommand process alive \ - while its old transport still serves in-flight work", - connection_id - ); - std::mem::forget(child); + match stale_handle { + Some(handle) if Arc::strong_count(&handle) > 1 => { + log::info!( + "SSH session {} reconnect keeps the previous ProxyCommand process alive \ + while its old transport still serves in-flight work", + connection_id + ); + Self::reap_proxy_after_transport_drains( + connection_id.to_string(), + Arc::downgrade(&handle), + child, + ); + drop(handle); + } + _ => child.shutdown().await, } } @@ -8522,6 +8557,99 @@ mod tests { ); } + /// Coder's `config-ssh` output: a multi-word ProxyCommand followed by + /// more keys in the same block, plus a catch-all block below it. This + /// config has a Host before every directive, which is exactly the shape + /// the old `ssh_config` 0.1 path parsed "successfully" while pairing + /// `ProxyCommand` with `/path/coder` and shifting every later key. + #[cfg(feature = "ssh_config")] + const CODER_STYLE_CONFIG: &str = "\ +Host coder.* + ConnectTimeout 0 + ProxyCommand /usr/local/bin/coder --global-config \"/Users/me/Library/Application Support/coderv2\" ssh --stdio --ssh-host-prefix coder. %h + StrictHostKeyChecking no + UserKnownHostsFile /dev/null + User coder + LogLevel ERROR + +Host coder.dev + HostName ignored-because-proxy-resolves.example + +Host plain + HostName plain.example + Port 2222 + ProxyCommand none + +Host * + User fallback + ProxyCommand /usr/bin/nc %h %p +"; + + #[test] + #[cfg(feature = "ssh_config")] + fn get_ssh_config_resolves_multi_word_proxy_command_without_shifting_keys() { + let result = SSHConnectionManager::lookup_ssh_config_in(CODER_STYLE_CONFIG, "coder.dev"); + assert!(result.found); + let entry = result.config.unwrap(); + assert_eq!( + entry.proxy_command.as_deref(), + Some( + "/usr/local/bin/coder --global-config \"/Users/me/Library/Application Support/coderv2\" ssh --stdio --ssh-host-prefix coder. %h" + ) + ); + // Keys after ProxyCommand keep their own values, and the first + // obtained value wins over `Host *`. + assert_eq!(entry.user.as_deref(), Some("coder")); + assert_eq!( + entry.hostname.as_deref(), + Some("ignored-because-proxy-resolves.example") + ); + assert_eq!(entry.port, None); + assert_eq!(entry.agent, None); + + // `ProxyCommand none` in a specific block disables the catch-all. + let plain = SSHConnectionManager::lookup_ssh_config_in(CODER_STYLE_CONFIG, "plain") + .config + .unwrap(); + assert_eq!(plain.proxy_command, None); + assert_eq!(plain.port, Some(2222)); + assert_eq!(plain.user.as_deref(), Some("fallback")); + + // Wildcard matching applies to aliases not listed anywhere. + let other = SSHConnectionManager::lookup_ssh_config_in(CODER_STYLE_CONFIG, "elsewhere") + .config + .unwrap(); + assert_eq!(other.proxy_command.as_deref(), Some("/usr/bin/nc %h %p")); + } + + #[test] + #[cfg(feature = "ssh_config")] + fn list_ssh_config_hosts_resolves_each_alias_with_openssh_semantics() { + let hosts = SSHConnectionManager::list_ssh_config_hosts_in(CODER_STYLE_CONFIG); + let names: Vec<_> = hosts.iter().map(|entry| entry.host.as_str()).collect(); + // Pure pattern blocks name no connectable host. + assert_eq!(names, vec!["coder.dev", "plain"]); + let coder = &hosts[0]; + assert!(coder + .proxy_command + .as_deref() + .is_some_and(|command| command.ends_with("--ssh-host-prefix coder. %h"))); + assert_eq!(coder.user.as_deref(), Some("coder")); + assert_eq!(hosts[1].proxy_command, None); + assert_eq!(hosts[1].port, Some(2222)); + } + + #[test] + #[cfg(feature = "ssh_config")] + fn list_ssh_config_hosts_deduplicates_repeated_aliases() { + let hosts = SSHConnectionManager::list_ssh_config_hosts_in( + "Host a\n User first\nHost a\n User second\nMatch host a\n Port 1\n", + ); + assert_eq!(hosts.len(), 1); + assert_eq!(hosts[0].user.as_deref(), Some("first")); + assert_eq!(hosts[0].port, None); + } + #[test] fn parses_proxy_command_as_a_whole_line_value() { let hosts = parse_ssh_config_manually( @@ -8570,7 +8698,11 @@ mod tests { async fn proxy_command_transport_reports_proxy_exit_code() { let temp = tempfile::tempdir().unwrap(); let manager = SSHConnectionManager::new(temp.path().into()); - let config = proxy_command_fixture_config("exit 7"); + let config = proxy_command_fixture_config(if cfg!(windows) { + "exit 7" + } else { + "sh -c 'exit 7'" + }); let error = manager .establish_session(&config, 10) diff --git a/src/crates/services/services-integrations/src/remote_ssh/mod.rs b/src/crates/services/services-integrations/src/remote_ssh/mod.rs index b84030d1f2..b9efac1515 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/mod.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/mod.rs @@ -33,7 +33,6 @@ mod port_forward; #[cfg(feature = "remote-ssh-concrete")] mod proxy_command; #[cfg(feature = "remote-ssh-concrete")] -#[cfg(feature = "remote-ssh-concrete")] mod release_verify; #[cfg(feature = "remote-ssh-concrete")] mod remote_exec; diff --git a/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs index e54a990732..735a87136c 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs @@ -7,10 +7,17 @@ //! known-hosts checks, and authentication identical to the direct path. //! //! Platform notes: +//! - Unix runs `sh -c "exec "` exactly like OpenSSH, so the proxy +//! replaces the shell. The process is supervised by +//! [`ProcessTreeChild`]: it leads its own process group and the whole group +//! (including any grandchildren the proxy forks) is killed on shutdown. //! - Windows runs the command through `cmd.exe /C` with the raw string (no //! extra MS quoting), matching how OpenSSH's Windows build passes -//! `ProxyCommand` along; Unix uses `sh -c`. +//! `ProxyCommand` along. [`ProcessTreeChild`] assigns it to a Job object with +//! `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so closing the Job ends the tree. //! - `%h`, `%p`, `%r`, and `%%` are expanded before the shell sees the string. +//! Hosts and users containing shell metacharacters are rejected first +//! (OpenSSH 9.6, CVE-2023-51385). //! - stderr is drained continuously into the log plus a small tail buffer: //! proxies like `coder ssh --stdio` log there, and a full pipe would block //! the child mid-handshake. @@ -18,11 +25,13 @@ use std::collections::VecDeque; use std::process::Stdio; use std::sync::{Arc, Mutex}; +use std::time::Duration; use anyhow::{anyhow, Context}; use openbitfun_services_core::process_manager; +use openbitfun_services_core::process_tree::ProcessTreeChild; use tokio::io::{join, AsyncBufReadExt, BufReader, Join}; -use tokio::process::{Child, ChildStderr, ChildStdin, ChildStdout}; +use tokio::process::{ChildStderr, ChildStdin, ChildStdout}; /// How many recent stderr lines are kept for error enrichment. const STDERR_TAIL_LINES: usize = 16; @@ -31,16 +40,18 @@ const STDERR_TAIL_LINES: usize = 16; const STDERR_LINE_MAX_CHARS: usize = 400; /// Grace period for a proxy to exit on its own before it is killed, so its /// real exit code survives as the diagnostic. -const EXIT_DRAIN_GRACE: std::time::Duration = std::time::Duration::from_secs(2); +const EXIT_DRAIN_GRACE: Duration = Duration::from_secs(2); +/// SIGTERM-to-SIGKILL grace when tearing down a proxy process group. +const TERMINATE_GRACE: Duration = Duration::from_millis(500); -/// Owns a spawned proxy process for the lifetime of the SSH session carried -/// over its stdio. +/// Owns a spawned proxy process tree for the lifetime of the SSH session +/// carried over its stdio. /// -/// The child is killed when the guard is dropped, whether that happens because +/// The tree is killed when the guard is dropped, whether that happens because /// the connection was closed, the handshake failed, or the session was torn /// down by [`crate::remote_ssh::manager::SSHConnectionManager`]. pub(crate) struct ProxyCommandChild { - child: Option, + tree: Option, stream: Option<(ChildStdout, ChildStdin)>, stderr_tail: Arc>>, drain: Option>, @@ -52,35 +63,29 @@ impl ProxyCommandChild { /// /// `label` identifies the connection in logs; the command text is kept out /// of error messages because ProxyCommand lines sometimes embed tokens. - pub(crate) fn spawn(command: &str, label: &str) -> anyhow::Result { + pub(crate) async fn spawn(command: &str, label: &str) -> anyhow::Result { let mut cmd = process_manager::create_tokio_command(proxy_shell_program()); configure_shell_args(&mut cmd, command); cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - // Safety net for any path that drops the guard without an explicit - // `finish`; the guard's own Drop reaps as well. - .kill_on_drop(true); - let mut child = cmd - .spawn() + .stderr(Stdio::piped()); + let mut tree = ProcessTreeChild::spawn(&mut cmd) + .await .with_context(|| format!("Failed to start proxy command for {label}"))?; - let stdout = child - .stdout - .take() + let stdout = tree + .take_stdout() .ok_or_else(|| anyhow!("Proxy command for {label} has no stdout pipe"))?; - let stdin = child - .stdin - .take() + let stdin = tree + .take_stdin() .ok_or_else(|| anyhow!("Proxy command for {label} has no stdin pipe"))?; - let stderr = child - .stderr - .take() + let stderr = tree + .take_stderr() .ok_or_else(|| anyhow!("Proxy command for {label} has no stderr pipe"))?; let stderr_tail = Arc::new(Mutex::new(VecDeque::new())); let drain = spawn_stderr_drain(stderr, stderr_tail.clone(), label.to_string()); Ok(Self { - child: Some(child), + tree: Some(tree), stream: Some((stdout, stdin)), stderr_tail, drain: Some(drain), @@ -97,22 +102,49 @@ impl ProxyCommandChild { Ok(join(stdout, stdin)) } - /// Kill the process (unless it already exited), reap it, and report the - /// exit code plus the buffered stderr tail for error messages. - pub(crate) async fn finish(mut self) -> (Option, String) { + /// Whether the proxy's direct child has exited. + pub(crate) fn has_exited(&mut self) -> bool { + self.tree + .as_mut() + .is_none_or(|tree| matches!(tree.try_wait(), Ok(Some(_)) | Err(_))) + } + + /// Give the proxy a short grace to exit on its own, then tear down the + /// whole process tree, reap it, and report the exit code plus the + /// buffered stderr tail for error messages. + pub(crate) async fn finish(self) -> (Option, String) { + self.finish_with_grace(EXIT_DRAIN_GRACE).await + } + + /// Tear the process tree down immediately and reap it. + pub(crate) async fn shutdown(self) { + let _ = self.finish_with_grace(Duration::ZERO).await; + } + + async fn finish_with_grace(mut self, grace: Duration) -> (Option, String) { let mut exit_code = None; - if let Some(child) = self.child.as_mut() { - // Give a fast-exiting proxy a moment to report its own exit code — - // that code is the actual diagnostic. Only a proxy that is still - // alive (wedged on a black hole, say) gets killed, and a killed - // proxy reports no code. - match tokio::time::timeout(EXIT_DRAIN_GRACE, child.wait()).await { - Ok(Ok(status)) => exit_code = status.code(), - _ => { - let _ = child.start_kill(); - if let Ok(status) = child.wait().await { - exit_code = status.code(); - } + if let Some(mut tree) = self.tree.take() { + // A fast-exiting proxy reports its own exit code — that code is + // the actual diagnostic. A proxy that is still alive (wedged on a + // black hole, say) gets killed and reports no code. + if !grace.is_zero() { + if let Ok(Ok(status)) = tokio::time::timeout(grace, tree.wait()).await { + exit_code = status.code(); + } + } + // Always clean up the process group / Job: the shell may have + // exited while a forked helper still holds the pipes. + if let Err(error) = tree.terminate(TERMINATE_GRACE).await { + log::debug!( + "Proxy command cleanup for {} reported: {}", + self.label, + error + ); + } + if exit_code.is_none() { + if let Ok(Some(status)) = tree.try_wait() { + // A signal-killed proxy has no code; only a real exit does. + exit_code = status.code(); } } } @@ -138,25 +170,43 @@ impl ProxyCommandChild { impl Drop for ProxyCommandChild { fn drop(&mut self) { - let Some(mut child) = self.child.take() else { + let Some(mut tree) = self.tree.take() else { return; }; - let _ = child.start_kill(); // Reap asynchronously so a killed child does not linger as a zombie on - // Unix. Without an active runtime there is nothing to await: the kill - // already happened, and `kill_on_drop` remains armed as a backstop. + // Unix. Without an active runtime `ProcessTreeChild`'s own Drop still + // kills the process group / closes the Job immediately. if let Ok(runtime) = tokio::runtime::Handle::try_current() { runtime.spawn(async move { - let _ = child.wait().await; + let _ = tree.terminate(Duration::ZERO).await; }); } } } +/// Characters OpenSSH 9.6+ accepts in a host or user that is substituted into +/// a shell command (CVE-2023-51385); anything else could be interpreted by +/// the shell. +fn is_shell_safe_token(value: &str) -> bool { + !value.is_empty() + && value.chars().all(|c| { + c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-' | '@' | ':' | '%' | '[' | ']') + }) +} + /// Expand OpenSSH ProxyCommand tokens: `%h` (target host), `%p` (port), `%r` /// (user), and `%%` (literal `%`). Unknown escapes stay literal, matching the /// conservative end of OpenSSH's behavior for this release. -pub(crate) fn expand_tokens(command: &str, host: &str, port: u16, user: &str) -> String { +/// +/// A host or user that is about to be substituted must consist of +/// `[A-Za-z0-9._-@:%\[\]]` only; anything else is rejected before expansion +/// so a crafted host name cannot inject shell syntax. +pub(crate) fn expand_tokens( + command: &str, + host: &str, + port: u16, + user: &str, +) -> anyhow::Result { let mut expanded = String::with_capacity(command.len()); let mut chars = command.chars(); while let Some(c) = chars.next() { @@ -165,9 +215,25 @@ pub(crate) fn expand_tokens(command: &str, host: &str, port: u16, user: &str) -> continue; } match chars.next() { - Some('h') => expanded.push_str(host), + Some('h') => { + if !is_shell_safe_token(host) { + anyhow::bail!( + "Host name contains characters that are not allowed in a ProxyCommand (%h); \ + use only letters, digits, and . _ - @ : % [ ]" + ); + } + expanded.push_str(host); + } Some('p') => expanded.push_str(&port.to_string()), - Some('r') => expanded.push_str(user), + Some('r') => { + if !is_shell_safe_token(user) { + anyhow::bail!( + "User name contains characters that are not allowed in a ProxyCommand (%r); \ + use only letters, digits, and . _ - @ : % [ ]" + ); + } + expanded.push_str(user); + } Some('%') => expanded.push('%'), Some(other) => { expanded.push('%'); @@ -176,7 +242,7 @@ pub(crate) fn expand_tokens(command: &str, host: &str, port: u16, user: &str) -> None => expanded.push('%'), } } - expanded + Ok(expanded) } fn proxy_shell_program() -> &'static str { @@ -202,7 +268,9 @@ fn configure_shell_args(cmd: &mut tokio::process::Command, command: &str) { } #[cfg(not(windows))] { - cmd.arg("-c").arg(command); + // Same as OpenSSH's `sshconnect.c`: `exec` makes the proxy replace the + // shell, so signals and exit status belong to the proxy itself. + cmd.arg("-c").arg(format!("exec {command}")); } } @@ -250,17 +318,62 @@ mod tests { #[test] fn expands_tokens_and_keeps_unknown_escapes_literal() { assert_eq!( - expand_tokens("coder ssh --stdio %h", "coder-x", 22, "alice"), + expand_tokens("coder ssh --stdio %h", "coder-x", 22, "alice").unwrap(), "coder ssh --stdio coder-x" ); assert_eq!( - expand_tokens("connect %r@%h:%p", "target", 2222, "ops"), + expand_tokens("connect %r@%h:%p", "target", 2222, "ops").unwrap(), "connect ops@target:2222" ); - assert_eq!(expand_tokens("100%% sure", "h", 22, "u"), "100% sure"); + assert_eq!( + expand_tokens("100%% sure", "h", 22, "u").unwrap(), + "100% sure" + ); // Unrecognized escapes survive untouched. - assert_eq!(expand_tokens("tool %j %n", "h", 22, "u"), "tool %j %n"); - assert_eq!(expand_tokens("trailing %", "h", 22, "u"), "trailing %"); + assert_eq!( + expand_tokens("tool %j %n", "h", 22, "u").unwrap(), + "tool %j %n" + ); + assert_eq!( + expand_tokens("trailing %", "h", 22, "u").unwrap(), + "trailing %" + ); + assert_eq!( + expand_tokens("nc %h %p", "[fe80::1%en0]", 22, "u").unwrap(), + "nc [fe80::1%en0] 22" + ); + } + + #[test] + fn rejects_shell_metacharacters_in_substituted_host_and_user() { + for host in [ + "host;rm -rf ~", + "$(id)", + "`id`", + "a b", + "a|b", + "a&b", + "a'b", + "a\"b", + "a\\b", + "a\nb", + "", + ] { + assert!( + expand_tokens("nc %h %p", host, 22, "u").is_err(), + "host {host:?} must be rejected" + ); + } + assert!(expand_tokens("connect %r@%h", "ok", 22, "bad;user").is_err()); + // Unused tokens are not validated: nothing is substituted. + assert_eq!( + expand_tokens("coder ssh --stdio fixed", "bad;host", 22, "bad user").unwrap(), + "coder ssh --stdio fixed" + ); + assert_eq!( + expand_tokens("nc %h", "user@host-1.example_x:22", 22, "u").unwrap(), + "nc user@host-1.example_x:22" + ); } #[cfg(windows)] @@ -268,6 +381,7 @@ mod tests { async fn finish_reports_exit_code_and_stderr_tail() { let mut child = ProxyCommandChild::spawn("echo proxy-err-line 1>&2 & exit 7", "test-connection") + .await .unwrap(); let _stream = child.take_stream().unwrap(); let (exit_code, tail) = child.finish().await; @@ -278,26 +392,96 @@ mod tests { #[cfg(not(windows))] #[tokio::test] async fn finish_reports_exit_code_and_stderr_tail() { + // `exec` applies to the first command, so wrap the compound script in + // its own shell the way a real ProxyCommand would. let mut child = - ProxyCommandChild::spawn("echo proxy-err-line >&2; exit 7", "test-connection").unwrap(); + ProxyCommandChild::spawn("sh -c 'echo proxy-err-line >&2; exit 7'", "test-connection") + .await + .unwrap(); let _stream = child.take_stream().unwrap(); let (exit_code, tail) = child.finish().await; assert_eq!(exit_code, Some(7)); assert!(tail.contains("proxy-err-line"), "stderr tail was: {tail}"); } + #[cfg(unix)] + async fn wait_for_pids(child: &ProxyCommandChild, count: usize) -> Vec { + for _ in 0..200 { + let pids: Vec = child + .stderr_tail_text() + .lines() + .filter_map(|line| line.trim().parse().ok()) + .collect(); + if pids.len() >= count { + return pids; + } + tokio::time::sleep(Duration::from_millis(25)).await; + } + panic!("proxy did not report its pids"); + } + + #[cfg(unix)] + fn pid_is_alive(pid: u32) -> bool { + // `kill -0` succeeds for a live process (or an unreaped zombie). + process_manager::create_command("kill") + .arg("-0") + .arg(pid.to_string()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + .map(|status| status.success()) + .unwrap_or(false) + } + + #[cfg(unix)] + async fn assert_pids_gone(pids: &[u32]) { + for _ in 0..200 { + if pids.iter().all(|pid| !pid_is_alive(*pid)) { + return; + } + tokio::time::sleep(Duration::from_millis(25)).await; + } + let alive: Vec<_> = pids.iter().filter(|pid| pid_is_alive(**pid)).collect(); + panic!("proxy processes still alive: {alive:?}"); + } + + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn drop_kills_a_still_running_proxy_and_its_grandchildren() { + // The proxy shell forks a long-lived grandchild and reports both pids. + let guard = ProxyCommandChild::spawn( + "sh -c 'sleep 300 & echo $! >&2; echo $$ >&2; wait'", + "long-running", + ) + .await + .unwrap(); + let pids = wait_for_pids(&guard, 2).await; + assert!(pids.iter().all(|pid| pid_is_alive(*pid))); + drop(guard); + assert_pids_gone(&pids).await; + } + + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn shutdown_kills_a_grandchild_that_outlives_the_proxy_shell() { + // The direct child exits immediately but leaves a helper holding the + // pipes; the process-group cleanup must still reach it. + let guard = + ProxyCommandChild::spawn("sh -c 'sleep 300 & echo $! >&2; exit 0'", "orphaning") + .await + .unwrap(); + let pids = wait_for_pids(&guard, 1).await; + guard.shutdown().await; + assert_pids_gone(&pids).await; + } + + #[cfg(windows)] #[tokio::test] async fn drop_kills_a_still_running_proxy() { - let script = if cfg!(windows) { - "ping -n 30 127.0.0.1 >nul" - } else { - "sleep 30" - }; - let guard = ProxyCommandChild::spawn(script, "long-running").unwrap(); - drop(guard); - // `finish` was never called; the child must not outlive the guard. A - // leaked process would show up as a dangling `ping`/`sleep` beyond this - // test's lifetime, so no explicit assertion is needed beyond the test - // finishing while the child is still running. + let mut guard = ProxyCommandChild::spawn("ping -n 30 127.0.0.1 >nul", "long-running") + .await + .unwrap(); + assert!(!guard.has_exited()); + guard.shutdown().await; } } diff --git a/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs index 9fe2e8cd7f..cac1ecc504 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs @@ -1,20 +1,28 @@ -//! Merged `~/.ssh/config` loading with `Include` expansion. +//! Merged `~/.ssh/config` loading with `Include` expansion, plus an +//! OpenSSH-style per-host resolver over the merged text. //! //! The `ssh_config` crate (0.1) parses a single file and has no `Include` //! support, but ecosystem tooling (Coder's `config-ssh`, AtomGit, devenv) writes //! Host blocks into separate files pulled in through `Include`. Hosts that only //! exist behind an `Include` were therefore invisible to both the host list and -//! `get_ssh_config`. This loader merges the main file with its include tree -//! in place, preserving OpenSSH's order semantics (first occurrence of a -//! setting wins). +//! `get_ssh_config`. This loader inlines the include tree at each `Include` +//! position. OpenSSH evaluates an included file inside the parent's current +//! `Host`/`Match` context and restores that context afterwards, so the loader +//! re-emits the parent's active `Host`/`Match` line (or `Host *` at top level) +//! after every expansion. +//! +//! The crate is also unusable for value lookup: it tokenizes keys and values +//! pairwise (a multi-word `ProxyCommand` shifts every following key), matches +//! `Host` patterns only by string equality, and lets the last value win. +//! [`resolve_host_settings`] replaces it with a line-based resolver that keeps +//! whole-line values and applies OpenSSH's first-obtained-value-wins rule. -use std::collections::HashSet; +use std::collections::HashMap; use std::path::{Path, PathBuf}; -/// OpenSSH processes nested `Include` directives recursively. The cap only -/// exists to turn accidental self-reference into a bounded warning instead of a -/// hang; real-world configs stay a handful of levels deep. -const MAX_INCLUDE_DEPTH: usize = 8; +/// OpenSSH processes nested `Include` directives recursively and caps the +/// depth at 16 (`READCONF_MAX_DEPTH`). +const MAX_INCLUDE_DEPTH: usize = 16; /// Merge `config_path` with the files pulled in by its `Include` directives. /// @@ -22,13 +30,13 @@ const MAX_INCLUDE_DEPTH: usize = 8; /// previous single-file behavior. Individual includes that fail to expand are /// skipped with a warning so one broken include cannot hide every other host. pub(crate) fn load_merged_config(config_path: &Path) -> Option { - let mut visited = HashSet::new(); + let mut stack = Vec::new(); let mut merged = String::new(); expand_file( config_path, 0, config_path.parent(), - &mut visited, + &mut stack, &mut merged, ); if merged.trim().is_empty() { @@ -38,6 +46,18 @@ pub(crate) fn load_merged_config(config_path: &Path) -> Option { } } +/// Async wrapper: include expansion does blocking filesystem and glob IO, so +/// it runs on the blocking pool instead of a runtime worker. +pub(crate) async fn load_merged_config_async(config_path: PathBuf) -> Option { + match tokio::task::spawn_blocking(move || load_merged_config(&config_path)).await { + Ok(merged) => merged, + Err(error) => { + log::warn!("SSH config loading task failed: {}", error); + None + } + } +} + /// Strip a leading UTF-8 BOM if present. Windows editors (and some SSH tooling) /// still write one. pub(crate) fn strip_utf8_bom(content: String) -> String { @@ -51,7 +71,7 @@ fn expand_file( path: &Path, depth: usize, base_dir: Option<&Path>, - visited: &mut HashSet, + stack: &mut Vec, merged: &mut String, ) { if depth > MAX_INCLUDE_DEPTH { @@ -62,9 +82,9 @@ fn expand_file( ); return; } - // Deduplicate on the canonical path so symlink or `..` cycles, plus plain - // repeated includes, cannot loop forever. Skipping a repeat is harmless: - // first-occurrence-wins parsing keeps the first copy authoritative anyway. + // Cycle detection only looks at the current recursion stack: including the + // same file again from a different Host block is legitimate and must apply + // its settings in that block's context too. let canonical = match path.canonicalize() { Ok(canonical) => canonical, Err(error) => { @@ -80,7 +100,11 @@ fn expand_file( return; } }; - if !visited.insert(canonical) { + if stack.contains(&canonical) { + log::warn!( + "SSH config include cycle at {}; skipping the repeated file", + path.display() + ); return; } let content = match std::fs::read_to_string(path) { @@ -94,7 +118,12 @@ fn expand_file( return; } }; + stack.push(canonical); + // The `Host`/`Match` line currently in effect in *this* file; restored + // after each include so the included file's own blocks do not swallow the + // parent's following settings. + let mut current_context: Option = None; for line in content.lines() { let trimmed = line.trim(); if trimmed.is_empty() || trimmed.starts_with('#') { @@ -102,33 +131,133 @@ fn expand_file( merged.push('\n'); continue; } - match include_value(trimmed) { - Some(value) => { + if let Some((keyword, value)) = split_directive(trimmed) { + if keyword.eq_ignore_ascii_case("include") { for target in resolve_include_targets(value, base_dir) { - expand_file(&target, depth + 1, base_dir, visited, merged); + expand_file(&target, depth + 1, base_dir, stack, merged); } - } - None => { - merged.push_str(line); + merged.push_str(current_context.as_deref().unwrap_or("Host *")); merged.push('\n'); + continue; + } + if keyword.eq_ignore_ascii_case("host") || keyword.eq_ignore_ascii_case("match") { + current_context = Some(trimmed.to_string()); } } + merged.push_str(line); + merged.push('\n'); } + stack.pop(); } -/// Extract the value part of an `Include` directive, or `None` for any other -/// line. Accepts both `Include value` and `Include=value`; ssh_config keywords -/// are case-insensitive. -fn include_value(line: &str) -> Option<&str> { - let (keyword, value) = match line.split_once(char::is_whitespace) { - Some((keyword, value)) => (keyword, value), - None => line.split_once('=')?, - }; - if keyword.eq_ignore_ascii_case("include") { - Some(value.trim()) - } else { - None +/// Split one non-comment config line into keyword and value. OpenSSH accepts +/// `Key value`, `Key=value`, and `Key = value`. +pub(crate) fn split_directive(line: &str) -> Option<(&str, &str)> { + let line = line.trim(); + let keyword_end = line.find(|c: char| c.is_whitespace() || c == '=')?; + let keyword = &line[..keyword_end]; + if keyword.is_empty() { + return None; } + let rest = line[keyword_end..].trim_start(); + let rest = rest.strip_prefix('=').unwrap_or(rest).trim(); + Some((keyword, rest)) +} + +/// Settings OpenSSH would apply to `host`, keyed by lowercase keyword. +/// +/// Values are the raw remainder of the directive line (trimmed), so +/// multi-word values such as `ProxyCommand` survive intact; callers pick the +/// first token where a directive takes a single argument. The first value +/// obtained for a keyword wins, exactly like OpenSSH. Lines before the first +/// `Host`/`Match` apply to every host. `Match all` applies; any other `Match` +/// criteria cannot be evaluated here and are treated as not matching. +pub(crate) fn resolve_host_settings(content: &str, host: &str) -> HashMap { + let mut settings = HashMap::new(); + let mut active = true; + for line in content.lines() { + let trimmed = line.trim(); + if trimmed.is_empty() || trimmed.starts_with('#') { + continue; + } + let Some((keyword, value)) = split_directive(trimmed) else { + continue; + }; + if keyword.eq_ignore_ascii_case("host") { + active = host_patterns_match(value, host); + continue; + } + if keyword.eq_ignore_ascii_case("match") { + active = value + .split_whitespace() + .next() + .is_some_and(|criterion| criterion.eq_ignore_ascii_case("all")); + continue; + } + if active && !value.is_empty() { + settings + .entry(keyword.to_ascii_lowercase()) + .or_insert_with(|| value.to_string()); + } + } + settings +} + +/// OpenSSH `Host` matching: the line matches when any positive pattern +/// matches and no negated (`!`) pattern does. Patterns support `*` and `?`, +/// compared case-insensitively, and may be comma- or whitespace-separated. +pub(crate) fn host_patterns_match(patterns: &str, host: &str) -> bool { + let host = host.to_ascii_lowercase(); + let mut matched = false; + for pattern in patterns + .split(|c: char| c.is_whitespace() || c == ',') + .map(|pattern| pattern.trim_matches('"')) + .filter(|pattern| !pattern.is_empty()) + { + let (negated, pattern) = match pattern.strip_prefix('!') { + Some(rest) => (true, rest), + None => (false, pattern), + }; + if wildcard_match(&pattern.to_ascii_lowercase(), &host) { + if negated { + return false; + } + matched = true; + } + } + matched +} + +/// Whether a `Host` pattern token is a wildcard/negation rather than a +/// connectable alias. +pub(crate) fn is_host_pattern(token: &str) -> bool { + token.starts_with('!') || token.contains(['*', '?']) +} + +fn wildcard_match(pattern: &str, text: &str) -> bool { + let pattern: Vec = pattern.chars().collect(); + let text: Vec = text.chars().collect(); + let (mut p, mut t) = (0usize, 0usize); + let mut star: Option<(usize, usize)> = None; + while t < text.len() { + if p < pattern.len() && (pattern[p] == '?' || pattern[p] == text[t]) { + p += 1; + t += 1; + } else if p < pattern.len() && pattern[p] == '*' { + star = Some((p, t)); + p += 1; + } else if let Some((star_p, star_t)) = star { + p = star_p + 1; + t = star_t + 1; + star = Some((star_p, star_t + 1)); + } else { + return false; + } + } + while p < pattern.len() && pattern[p] == '*' { + p += 1; + } + p == pattern.len() } /// Resolve one `Include` value into concrete file paths, in order. @@ -294,7 +423,9 @@ mod tests { let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); assert!(merged.contains("Host a")); assert!(merged.contains("Host b")); - assert_eq!(merged.matches("Host a").count(), 1); + // The cycle is cut on the recursion stack: a -> b -> (a skipped). + assert_eq!(merged.matches("Host a\n").count(), 1); + assert_eq!(merged.matches("Host b\n").count(), 1); fs::remove_dir_all(&dir).unwrap(); } @@ -319,15 +450,148 @@ mod tests { } #[test] - fn include_value_accepts_keyword_forms_case_insensitively() { - assert_eq!(include_value("Include coder.conf"), Some("coder.conf")); - assert_eq!(include_value("include=parts/*.conf"), Some("parts/*.conf")); + fn split_directive_accepts_keyword_forms() { assert_eq!( - include_value("INCLUDE \"path with spaces.conf\" "), - Some("\"path with spaces.conf\"") + split_directive("Include coder.conf"), + Some(("Include", "coder.conf")) ); - assert_eq!(include_value("HostName example"), None); - assert_eq!(include_value("Host=example"), None); + assert_eq!( + split_directive("include=parts/*.conf"), + Some(("include", "parts/*.conf")) + ); + assert_eq!( + split_directive("Include = parts/*.conf"), + Some(("Include", "parts/*.conf")) + ); + assert_eq!( + split_directive("INCLUDE \"path with spaces.conf\" "), + Some(("INCLUDE", "\"path with spaces.conf\"")) + ); + assert_eq!(split_directive("Host=example"), Some(("Host", "example"))); + assert_eq!(split_directive("Lonely"), None); + } + + #[test] + fn include_forms_with_equals_are_expanded() { + let dir = unique_temp_dir("equals"); + let ssh_dir = dir.join(".ssh"); + write(&ssh_dir.join("a.conf"), "Host from-a\n"); + write(&ssh_dir.join("b.conf"), "Host from-b\n"); + write( + &ssh_dir.join("config"), + "Include = a.conf\nInclude=b.conf\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + assert!(merged.contains("Host from-a")); + assert!(merged.contains("Host from-b")); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn restores_the_parent_host_context_after_an_include() { + let dir = unique_temp_dir("context"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Host parent\n Include inner.conf\n User parent-user\n", + ); + write( + &ssh_dir.join("inner.conf"), + "HostName parent.example\nHost inner\n User inner-user\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + let parent = resolve_host_settings(&merged, "parent"); + assert_eq!( + parent.get("hostname").map(String::as_str), + Some("parent.example") + ); + // Without context restoration `User parent-user` would land in `Host inner`. + assert_eq!(parent.get("user").map(String::as_str), Some("parent-user")); + let inner = resolve_host_settings(&merged, "inner"); + assert_eq!(inner.get("user").map(String::as_str), Some("inner-user")); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn top_level_include_restores_global_context() { + let dir = unique_temp_dir("global-context"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Include hosts.conf\nServerAliveInterval 30\n", + ); + write(&ssh_dir.join("hosts.conf"), "Host only-one\n User one\n"); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + let other = resolve_host_settings(&merged, "anything"); + assert_eq!( + other.get("serveraliveinterval").map(String::as_str), + Some("30") + ); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn same_file_may_be_included_from_different_blocks() { + let dir = unique_temp_dir("reinclude"); + let ssh_dir = dir.join(".ssh"); + write(&ssh_dir.join("common.conf"), "User shared\n"); + write( + &ssh_dir.join("config"), + "Host one\n Include common.conf\nHost two\n Include common.conf\n", + ); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + for host in ["one", "two"] { + let settings = resolve_host_settings(&merged, host); + assert_eq!(settings.get("user").map(String::as_str), Some("shared")); + } + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn resolver_keeps_whole_line_values_and_first_value_wins() { + let content = "Host coder.*\n ProxyCommand /usr/bin/coder --global-config \"/x y\" ssh --stdio --ssh-host-prefix coder. %h\n User coder\n StrictHostKeyChecking no\nHost *\n User fallback\n ProxyCommand other %h\n"; + let settings = resolve_host_settings(content, "coder.workspace"); + assert_eq!( + settings.get("proxycommand").map(String::as_str), + Some("/usr/bin/coder --global-config \"/x y\" ssh --stdio --ssh-host-prefix coder. %h") + ); + assert_eq!(settings.get("user").map(String::as_str), Some("coder")); + assert_eq!( + settings.get("stricthostkeychecking").map(String::as_str), + Some("no") + ); + let fallback = resolve_host_settings(content, "plain"); + assert_eq!(fallback.get("user").map(String::as_str), Some("fallback")); + } + + #[test] + fn host_patterns_support_wildcards_and_negation() { + assert!(host_patterns_match("*.example.com", "a.example.com")); + assert!(host_patterns_match("A?C", "abc")); + assert!(!host_patterns_match("*.example.com", "example.org")); + assert!(host_patterns_match("* !bastion", "web")); + assert!(!host_patterns_match("* !bastion", "bastion")); + assert!(host_patterns_match("one,two", "two")); + assert!(!host_patterns_match("!only", "other")); + assert!(is_host_pattern("*")); + assert!(is_host_pattern("!x")); + assert!(!is_host_pattern("coder.ws")); + } + + #[test] + fn match_all_applies_and_other_match_blocks_do_not() { + let content = "Match host foo\n User from-match\nMatch all\n Port 2200\n"; + let settings = resolve_host_settings(content, "foo"); + assert_eq!(settings.get("user"), None); + assert_eq!(settings.get("port").map(String::as_str), Some("2200")); } #[test] diff --git a/src/web-ui/src/infrastructure/api/generated/remoteSurface.ts b/src/web-ui/src/infrastructure/api/generated/remoteSurface.ts index 8d9d4fe2a2..a6ad4c0583 100644 --- a/src/web-ui/src/infrastructure/api/generated/remoteSurface.ts +++ b/src/web-ui/src/infrastructure/api/generated/remoteSurface.ts @@ -1,6 +1,6 @@ // Generated by scripts/generate-interactive-capabilities.mjs; do not edit. // Source: openbitfun_product_domains::remote_surface (Product Operation Registry). -export const REMOTE_SURFACE_REGISTRY_DIGEST = "fnv1a64:ee62faff5c97f12a" as const; +export const REMOTE_SURFACE_REGISTRY_DIGEST = "fnv1a64:fc41c72be879cce0" as const; /** * Registered Tauri commands the Peer Device controller keeps on the controller @@ -194,6 +194,7 @@ export const PEER_HOST_CAPABILITY_IDS = [ "miniapp_agent_context_files_v1", "miniapp_agent_images_v1", "wsl_workspaces_v1", + "ssh_proxy_command_v1", "computer_use_sharing_v1", "product_control_v1", "remote_connect_control_v1", @@ -243,6 +244,7 @@ export const PEER_HOST_ADVERTISED_CAPABILITIES: Readonly< "miniapp_agent_context_files_v1", "miniapp_agent_images_v1", "wsl_workspaces_v1", + "ssh_proxy_command_v1", "computer_use_sharing_v1", "product_control_v1", "remote_connect_control_v1", From 05a84655481da8b5f1e90d4e4a9c74cb91231e53 Mon Sep 17 00:00:00 2001 From: Bob Lee Date: Mon, 2 Nov 2026 12:41:17 +0800 Subject: [PATCH 4/4] fix(remote-ssh): align ProxyCommand config resolution with OpenSSH - Run Windows proxies through cmd.exe /d /v:off /s /c with one outer quote pair so quoted programs and arguments survive. - Accumulate IdentityFile/CertificateFile and prefer the block naming the host for single-value consumers. - Keep ProxyCommand verbatim; strip inline comments on Host lines (and on ProxyCommand on Windows). - Let ProxyJump and ProxyCommand compete: first obtained wins. - Report found for any matched setting, including Host * defaults. - Pass the includer's Host/Match context into nested Includes. - Warn, and name the ignored ProxyCommand on dial failure, when a profile without proxyCommand targets an alias that has one; correct the docs. --- .../remote-workspace-transport.md | 53 +++-- .../src/remote_ssh/manager.rs | 200 ++++++++++++++---- .../src/remote_ssh/proxy_command.rs | 54 ++++- .../src/remote_ssh/ssh_config_loader.rs | 180 ++++++++++++++-- 4 files changed, 402 insertions(+), 85 deletions(-) diff --git a/docs/architecture/remote-workspace-transport.md b/docs/architecture/remote-workspace-transport.md index ee98b74753..55e9624f8a 100644 --- a/docs/architecture/remote-workspace-transport.md +++ b/docs/architecture/remote-workspace-transport.md @@ -117,27 +117,42 @@ profiles. Native WSL has no SSH transport for port forwarding. ## ProxyCommand -The additive `proxyCommand` profile field (and `~/.ssh/config` `ProxyCommand`) -carries the SSH transport over a local process's stdin/stdout, as OpenSSH does -(for example `coder ssh --stdio %h`). Missing `proxyCommand` preserves direct -dialing; connection drift includes the field. `%h`, `%p`, `%r`, and `%%` are -expanded first, and a host or user that would be substituted must match -`[A-Za-z0-9._\-@:%\[\]]` (OpenSSH 9.6, CVE-2023-51385) or the connection fails -before anything runs. Unix runs `sh -c "exec …"` in its own process group; -Windows runs `cmd.exe /C` inside a kill-on-close Job. Disconnect, failed -handshakes, and reconnect tear the whole process tree down; a reconnect keeps a -displaced proxy only until its old transport has no users left. - -ProxyCommand is exclusive with ProxyJump, containers, and WSL. A ProxyJump hop -whose `~/.ssh/config` alias resolves to a ProxyCommand fails with an explicit -unsupported error instead of dialing the hop directly. `~/.ssh/config` values -come from a line-based resolver over the `Include`-merged text (Host patterns -with `*`, `?`, and `!`, first value wins, whole-line `ProxyCommand`, `none` -disables). +The additive `proxyCommand` profile field carries the SSH transport over a +local process's stdin/stdout, as OpenSSH does (for example +`coder ssh --stdio %h`). The Services layer and the port-forward tool's +`~/.ssh/config` path honor it; the desktop connection dialog does not send +`proxyCommand` yet, so profiles created there still dial directly. When such a +profile targets a `~/.ssh/config` alias that has a ProxyCommand, the host logs +a warning and a failed dial names the ignored ProxyCommand instead of +reporting only a network error. Missing `proxyCommand` preserves direct +dialing; connection drift includes the field. + +`%h`, `%p`, `%r`, and `%%` are expanded first, and a host or user that would be +substituted must match `[A-Za-z0-9._\-@:%\[\]]` (OpenSSH 9.6, CVE-2023-51385) +or the connection fails before anything runs. Unix runs `sh -c "exec …"` in its +own process group; Windows runs `cmd.exe /d /v:off /s /c "…"` inside a +kill-on-close Job. Disconnect, failed handshakes, and reconnect tear the whole +process tree down; a reconnect keeps a displaced proxy only until its old +transport has no users left. + +A saved profile with both ProxyCommand and ProxyJump, or with a container or +WSL target, is rejected. A ProxyJump hop whose `~/.ssh/config` alias resolves +to a ProxyCommand fails with an explicit unsupported error instead of dialing +the hop directly. + +`~/.ssh/config` values come from a line-based resolver over the +`Include`-merged text. Include runs inside the includer's `Host`/`Match` +context, which is restored afterwards. Host patterns support `*`, `?`, `!`, +and inline `#` comments. The first value wins, except that `IdentityFile` and +`CertificateFile` accumulate; single-key consumers take the first value from a +block that names the host exactly. `ProxyCommand` is kept verbatim (`none` +disables it), and ProxyJump and ProxyCommand compete: whichever is obtained +first wins. The host advertises `ssh_proxy_command_v1`. Older hosts ignore the unknown -field and would dial directly, so controllers must not send `proxyCommand` to a -peer that lacks the capability. CLI peer setup is unsupported, as for WSL. +field and would dial directly, so a future controller UI must not send +`proxyCommand` to a peer that lacks the capability. CLI peer setup is +unsupported, as for WSL. ## ProxyJump diff --git a/src/crates/services/services-integrations/src/remote_ssh/manager.rs b/src/crates/services/services-integrations/src/remote_ssh/manager.rs index 0757364789..bced7092fb 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/manager.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/manager.rs @@ -7,6 +7,7 @@ use super::sftp_file::ManagedSftpSession; #[cfg(feature = "ssh_config")] use super::ssh_config_loader::{ is_host_pattern, load_merged_config_async, resolve_host_settings, split_directive, + strip_trailing_comment, ResolvedHostSettings, }; use crate::remote_ssh::password_vault::SSHPasswordVault; use crate::remote_ssh::types::{ @@ -428,48 +429,59 @@ fn truncate_at_char_boundary(s: &str, max_bytes: usize) -> &str { /// Single-argument value of a resolved directive (see /// [`resolve_host_settings`], which keys by lowercase keyword). #[cfg(feature = "ssh_config")] -fn resolved_value<'a>( - settings: &'a std::collections::HashMap, - key: &str, -) -> Option<&'a str> { +fn resolved_value<'a>(settings: &'a ResolvedHostSettings, key: &str) -> Option<&'a str> { settings .get(key) .and_then(|value| parse_ssh_config_value(value)) } -/// Resolved `ProxyCommand`, or `None` when absent or explicitly `none`. +/// Resolve the competing `ProxyJump` / `ProxyCommand` pair the way OpenSSH +/// does: whichever is obtained first wins and the later one is dropped +/// (`none` counts as obtained). Returns `(proxy_jump, proxy_command)`. #[cfg(feature = "ssh_config")] -fn resolved_proxy_command(settings: &std::collections::HashMap) -> Option { - settings - .get("proxycommand") - .and_then(|value| parse_ssh_config_command_value(value)) - .filter(|command| !command.eq_ignore_ascii_case("none")) +fn resolved_proxy_pair(settings: &ResolvedHostSettings) -> (Option, Option) { + let jump = settings.first("proxyjump"); + let command = settings.first("proxycommand"); + let (jump, command) = match (jump, command) { + (Some(jump), Some(command)) if jump.order < command.order => (Some(jump), None), + (Some(_), Some(command)) => (None, Some(command)), + pair => pair, + }; + let jump = jump + .and_then(|jump| parse_ssh_config_value(&jump.value)) + .filter(|jump| !jump.eq_ignore_ascii_case("none")) + .map(ToOwned::to_owned); + let command = command + .and_then(|command| parse_ssh_config_command_value(&command.value)) + .filter(|command| !command.eq_ignore_ascii_case("none")); + (jump, command) } /// Build an [`SSHConfigEntry`] from resolved settings for `host`. #[cfg(feature = "ssh_config")] -fn ssh_config_entry_from_settings( - host: &str, - settings: &std::collections::HashMap, -) -> SSHConfigEntry { - let proxy_command = resolved_proxy_command(settings); +fn ssh_config_entry_from_settings(host: &str, settings: &ResolvedHostSettings) -> SSHConfigEntry { + let (proxy_jump, proxy_command) = resolved_proxy_pair(settings); + // IdentityFile/CertificateFile accumulate in OpenSSH; the entry carries + // one, so prefer the block that names this host over `Host *` defaults. + let preferred_file = |key: &str| { + settings + .preferred(key) + .and_then(|value| parse_ssh_config_value(value)) + .map(|file| shellexpand::tilde(file).to_string()) + }; SSHConfigEntry { host: host.to_string(), hostname: resolved_value(settings, "hostname").map(ToOwned::to_owned), port: resolved_value(settings, "port").and_then(|port| port.parse::().ok()), user: resolved_value(settings, "user").map(ToOwned::to_owned), - identity_file: resolved_value(settings, "identityfile") - .map(|file| shellexpand::tilde(file).to_string()), + identity_file: preferred_file("identityfile"), agent: if proxy_command.is_some() { None } else { Some(true) }, - certificate_file: resolved_value(settings, "certificatefile") - .map(|file| shellexpand::tilde(file).to_string()), - proxy_jump: resolved_value(settings, "proxyjump") - .filter(|jump| !jump.eq_ignore_ascii_case("none")) - .map(ToOwned::to_owned), + certificate_file: preferred_file("certificatefile"), + proxy_jump, proxy_command, } } @@ -493,19 +505,21 @@ fn parse_ssh_config_value(value: &str) -> Option<&str> { /// Extract a whole-line command value such as `ProxyCommand`'s. /// /// Unlike [`parse_ssh_config_value`], the value spans every remaining token of -/// the line. A single pair of surrounding double quotes is stripped so -/// `ProxyCommand "coder ssh --stdio %h"` keeps its inner quoting intact. +/// the line and is kept verbatim, quotes included, as OpenSSH does: stripping +/// an outer quote pair would corrupt `"a b" ssh "%h"`. POSIX `sh` treats an +/// inline ` # comment` as a comment itself; `cmd.exe` does not, so it is +/// removed on Windows. #[cfg(feature = "ssh_config")] fn parse_ssh_config_command_value(value: &str) -> Option { + let value = if cfg!(windows) { + super::ssh_config_loader::strip_trailing_comment(value) + } else { + value + }; let value = value.trim(); if value.is_empty() { return None; } - if let Some(inner) = value.strip_prefix('"') { - if let Some(stripped) = inner.strip_suffix('"') { - return Some(stripped.to_string()); - } - } Some(value.to_string()) } @@ -558,7 +572,7 @@ fn parse_ssh_config_manually(content: &str) -> Vec { // pattern blocks (`Host *`, `Host !x *.corp`) and `Match` blocks // name no host, so their settings are not attributed to anyone. current_host = if keyword.eq_ignore_ascii_case("Host") { - value + strip_trailing_comment(value) .split(|c: char| c.is_whitespace() || c == ',') .map(|alias| alias.trim_matches('"')) .find(|alias| !alias.is_empty() && !is_host_pattern(alias)) @@ -2511,14 +2525,9 @@ impl SSHConnectionManager { fn lookup_ssh_config_in(config_content: &str, host: &str) -> SSHConfigLookupResult { let settings = resolve_host_settings(config_content, host); let entry = ssh_config_entry_from_settings(host, &settings); - let found = entry.hostname.is_some() - || entry.port.is_some() - || entry.user.is_some() - || entry.identity_file.is_some() - || entry.certificate_file.is_some() - || entry.proxy_jump.is_some() - || entry.proxy_command.is_some(); - if found { + // Any matched setting counts, even only `Host *` defaults such as + // `AddKeysToAgent`: callers use `found` to pick agent authentication. + if !settings.is_empty() { log::debug!( "Found SSH config for host: {} with {} settings", host, @@ -3231,9 +3240,18 @@ impl SSHConnectionManager { .await .map_err(|error| tag_failed_stage("configuration", error))?; if jumps.is_empty() { + let ignored_proxy_command = self.ssh_config_proxy_command_ignored(config).await; let (handle, alive, mut server_info) = self .establish_direct_session(config, timeout_secs, &mut unpublished) .await + .map_err(|error| match &ignored_proxy_command { + Some(alias) => anyhow!( + "{error} (~/.ssh/config defines a ProxyCommand for '{alias}', but this \ + connection dials the host directly; this connection profile does not \ + carry the ProxyCommand)" + ), + None => error, + }) .map_err(|error| tag_failed_stage("target", error))?; if config .container @@ -3768,6 +3786,32 @@ impl SSHConnectionManager { .await } + /// When a connection without `proxy_command` targets a `~/.ssh/config` + /// alias that resolves to a ProxyCommand, warn loudly: the profile dials + /// directly, which is not what `ssh ` would do. Returns the alias + /// so a dial failure can say why. The command text is never logged. + async fn ssh_config_proxy_command_ignored( + &self, + config: &SSHConnectionConfig, + ) -> Option { + if config + .proxy_command + .as_deref() + .is_some_and(|command| !command.trim().is_empty()) + { + return None; + } + let lookup = self.get_ssh_config(&config.host).await; + lookup.config.as_ref()?.proxy_command.as_ref()?; + log::warn!( + "SSH connection {} targets host '{}', which has a ProxyCommand in ~/.ssh/config; \ + the connection profile carries no ProxyCommand, so the host is dialed directly", + config.id, + config.host + ); + Some(config.host.clone()) + } + /// Build a fresh SSH session whose transport is the stdin/stdout of a /// local ProxyCommand process (OpenSSH semantics). /// @@ -8639,6 +8683,79 @@ Host * assert_eq!(hosts[1].port, Some(2222)); } + #[test] + #[cfg(feature = "ssh_config")] + fn identity_file_prefers_the_block_naming_the_host() { + let content = "Host *\n IdentityFile ~/.ssh/default\nHost foo # work box\n IdentityFile /keys/foo-1\n IdentityFile /keys/foo-2\n CertificateFile /keys/foo-cert.pub\n"; + let entry = SSHConnectionManager::lookup_ssh_config_in(content, "foo") + .config + .unwrap(); + assert_eq!(entry.identity_file.as_deref(), Some("/keys/foo-1")); + assert_eq!( + entry.certificate_file.as_deref(), + Some("/keys/foo-cert.pub") + ); + let settings = resolve_host_settings(content, "foo"); + let files: Vec<_> = settings + .all("identityfile") + .iter() + .map(|value| value.value.as_str()) + .collect(); + assert_eq!(files, vec!["~/.ssh/default", "/keys/foo-1", "/keys/foo-2"]); + // Hosts without a specific block still get the default. + let other = SSHConnectionManager::lookup_ssh_config_in(content, "bar") + .config + .unwrap(); + assert!(other + .identity_file + .as_deref() + .is_some_and(|file| file.ends_with(".ssh/default"))); + // The trailing comment on the Host line is not a pattern. + let hosts = SSHConnectionManager::list_ssh_config_hosts_in(content); + assert_eq!(hosts.len(), 1); + assert_eq!(hosts[0].host, "foo"); + } + + #[test] + #[cfg(feature = "ssh_config")] + fn proxy_jump_and_proxy_command_first_obtained_wins() { + let content = "Host target\n ProxyJump bastion\nHost *\n ProxyCommand nc %h %p\n"; + let entry = SSHConnectionManager::lookup_ssh_config_in(content, "target") + .config + .unwrap(); + assert_eq!(entry.proxy_jump.as_deref(), Some("bastion")); + assert_eq!(entry.proxy_command, None); + + let content = "Host target\n ProxyCommand nc %h %p\nHost *\n ProxyJump bastion\n"; + let entry = SSHConnectionManager::lookup_ssh_config_in(content, "target") + .config + .unwrap(); + assert_eq!(entry.proxy_jump, None); + assert_eq!(entry.proxy_command.as_deref(), Some("nc %h %p")); + + // `ProxyCommand none` first still suppresses a later ProxyJump. + let content = "Host target\n ProxyCommand none\nHost *\n ProxyJump bastion\n"; + let entry = SSHConnectionManager::lookup_ssh_config_in(content, "target") + .config + .unwrap(); + assert_eq!(entry.proxy_jump, None); + assert_eq!(entry.proxy_command, None); + } + + #[test] + #[cfg(feature = "ssh_config")] + fn wildcard_only_settings_still_count_as_found() { + let result = SSHConnectionManager::lookup_ssh_config_in( + "Host *\n AddKeysToAgent yes\n", + "anything", + ); + assert!(result.found); + assert_eq!(result.config.unwrap().agent, Some(true)); + assert!( + !SSHConnectionManager::lookup_ssh_config_in("Host other\n User x\n", "anything").found + ); + } + #[test] #[cfg(feature = "ssh_config")] fn list_ssh_config_hosts_deduplicates_repeated_aliases() { @@ -8662,11 +8779,12 @@ Host * Some("\"C:\\tools\\coder.exe\" ssh --stdio %h") ); - let hosts = - parse_ssh_config_manually("Host quoted\n ProxyCommand \"coder ssh --stdio %h\"\n"); + // Kept verbatim like OpenSSH: stripping the outer quotes of + // `"a b" ssh "%h"` would corrupt it. + let hosts = parse_ssh_config_manually("Host quoted\n ProxyCommand \"a b\" ssh \"%h\"\n"); assert_eq!( hosts[0].proxy_command.as_deref(), - Some("coder ssh --stdio %h") + Some("\"a b\" ssh \"%h\"") ); let hosts = parse_ssh_config_manually("Host plain\n HostName plain.example\n"); diff --git a/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs index 735a87136c..fdd1457ff7 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/proxy_command.rs @@ -11,9 +11,9 @@ //! replaces the shell. The process is supervised by //! [`ProcessTreeChild`]: it leads its own process group and the whole group //! (including any grandchildren the proxy forks) is killed on shutdown. -//! - Windows runs the command through `cmd.exe /C` with the raw string (no -//! extra MS quoting), matching how OpenSSH's Windows build passes -//! `ProxyCommand` along. [`ProcessTreeChild`] assigns it to a Job object with +//! - Windows runs the command through `cmd.exe /d /v:off /s /c ""` +//! with the raw string (no extra MS quoting), so quoted executable paths +//! and quoted arguments survive. [`ProcessTreeChild`] assigns it to a Job object with //! `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE`, so closing the Job ends the tree. //! - `%h`, `%p`, `%r`, and `%%` are expanded before the shell sees the string. //! Hosts and users containing shell metacharacters are rejected first @@ -259,12 +259,11 @@ fn proxy_shell_program() -> &'static str { fn configure_shell_args(cmd: &mut tokio::process::Command, command: &str) { #[cfg(windows)] { - // cmd.exe parses everything after /C itself, so the command must reach + // cmd.exe parses everything after /c itself, so the command must reach // it raw: the standard Windows argument quoting would wrap it in extra // quotes and mangle the quoting conventions ProxyCommand authors - // already rely on. - cmd.raw_arg("/C"); - cmd.raw_arg(command); + // already rely on. See `windows_cmd_arguments` for the flags. + cmd.raw_arg(windows_cmd_arguments(command)); } #[cfg(not(windows))] { @@ -274,6 +273,20 @@ fn configure_shell_args(cmd: &mut tokio::process::Command, command: &str) { } } +/// Raw `cmd.exe` argument string for a ProxyCommand. +/// +/// - `/d` skips AutoRun registry commands, whose output would otherwise land +/// on stdout and corrupt the SSH stream. +/// - `/v:off` keeps `!` literal (no delayed expansion). +/// - `/s /c ""`: with `/s`, cmd strips exactly the outer quote pair we +/// add and keeps the command verbatim. Without it, a command with more than +/// two quotes, such as `"C:\x\coder.exe" --global-config "C:\cfg" ssh`, +/// loses its first and last quote and no longer runs. +#[cfg_attr(not(windows), allow(dead_code))] +fn windows_cmd_arguments(command: &str) -> String { + format!("/d /v:off /s /c \"{command}\"") +} + /// Drain the proxy's stderr forever: one line at a time into the log, with the /// most recent lines kept in a bounded tail for connection error messages. /// Returns the task handle; the task ends when stderr hits EOF (process exit). @@ -376,6 +389,33 @@ mod tests { ); } + #[test] + fn windows_cmd_arguments_wrap_the_command_in_one_outer_quote_pair() { + let command = r#""C:\x\coder.exe" --global-config "C:\Users\me\coderv2" ssh --stdio host"#; + assert_eq!( + windows_cmd_arguments(command), + format!("/d /v:off /s /c \"{command}\"") + ); + } + + #[cfg(windows)] + #[tokio::test] + async fn quoted_program_with_two_quoted_arguments_runs_verbatim() { + // Four quotes, starting with a quoted program: without `/s` and the + // outer pair, cmd.exe strips the first and last quote and fails. + let mut child = ProxyCommandChild::spawn( + r#""%ComSpec%" /d /c echo "quoted-one" "quoted-two" 1>&2 & exit 7"#, + "test-connection", + ) + .await + .unwrap(); + let _stream = child.take_stream().unwrap(); + let (exit_code, tail) = child.finish().await; + assert_eq!(exit_code, Some(7)); + assert!(tail.contains("quoted-one"), "stderr tail was: {tail}"); + assert!(tail.contains("quoted-two"), "stderr tail was: {tail}"); + } + #[cfg(windows)] #[tokio::test] async fn finish_reports_exit_code_and_stderr_tail() { diff --git a/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs index cac1ecc504..dd32928949 100644 --- a/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs +++ b/src/crates/services/services-integrations/src/remote_ssh/ssh_config_loader.rs @@ -35,6 +35,7 @@ pub(crate) fn load_merged_config(config_path: &Path) -> Option { expand_file( config_path, 0, + None, config_path.parent(), &mut stack, &mut merged, @@ -70,6 +71,7 @@ pub(crate) fn strip_utf8_bom(content: String) -> String { fn expand_file( path: &Path, depth: usize, + inherited_context: Option<&str>, base_dir: Option<&Path>, stack: &mut Vec, merged: &mut String, @@ -123,7 +125,9 @@ fn expand_file( // The `Host`/`Match` line currently in effect in *this* file; restored // after each include so the included file's own blocks do not swallow the // parent's following settings. - let mut current_context: Option = None; + // An included file starts inside the includer's block, so an Include + // nested in it must restore that block too — not a global `Host *`. + let mut current_context: Option = inherited_context.map(ToOwned::to_owned); for line in content.lines() { let trimmed = line.trim(); if trimmed.is_empty() || trimmed.starts_with('#') { @@ -134,7 +138,14 @@ fn expand_file( if let Some((keyword, value)) = split_directive(trimmed) { if keyword.eq_ignore_ascii_case("include") { for target in resolve_include_targets(value, base_dir) { - expand_file(&target, depth + 1, base_dir, stack, merged); + expand_file( + &target, + depth + 1, + current_context.as_deref(), + base_dir, + stack, + merged, + ); } merged.push_str(current_context.as_deref().unwrap_or("Host *")); merged.push('\n'); @@ -164,18 +175,83 @@ pub(crate) fn split_directive(line: &str) -> Option<(&str, &str)> { Some((keyword, rest)) } -/// Settings OpenSSH would apply to `host`, keyed by lowercase keyword. +/// One obtained directive value. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct ResolvedValue { + /// Raw remainder of the directive line, trimmed. + pub value: String, + /// Whether the value came from a `Host` block that names this host + /// exactly (no wildcard), as opposed to `Host *`-style or global lines. + pub specific: bool, + /// Directive line order within the merged text; lets callers apply + /// OpenSSH's "whichever is specified first" rule across keywords. + pub order: usize, +} + +/// Settings OpenSSH would apply to one host, keyed by lowercase keyword. +#[derive(Debug, Clone, Default)] +pub(crate) struct ResolvedHostSettings { + values: HashMap>, +} + +impl ResolvedHostSettings { + /// The effective value: the first one obtained, as OpenSSH applies it. + pub(crate) fn get(&self, key: &str) -> Option<&String> { + self.first(key).map(|value| &value.value) + } + + pub(crate) fn first(&self, key: &str) -> Option<&ResolvedValue> { + self.values.get(key).and_then(|values| values.first()) + } + + /// Every value obtained for `key`, in order. Only accumulating keywords + /// (see [`is_accumulating_keyword`]) can hold more than one. + pub(crate) fn all(&self, key: &str) -> &[ResolvedValue] { + self.values.get(key).map(Vec::as_slice).unwrap_or(&[]) + } + + /// For accumulating keywords consumed as a single value: the first value + /// from a block naming the host exactly, else the first value overall. + pub(crate) fn preferred(&self, key: &str) -> Option<&String> { + let all = self.all(key); + all.iter() + .find(|value| value.specific) + .or_else(|| all.first()) + .map(|value| &value.value) + } + + pub(crate) fn is_empty(&self) -> bool { + self.values.is_empty() + } + + pub(crate) fn len(&self) -> usize { + self.values.len() + } +} + +/// Keywords OpenSSH accumulates instead of applying first-value-wins. +fn is_accumulating_keyword(keyword: &str) -> bool { + matches!( + keyword, + "identityfile" | "certificatefile" | "localforward" | "remoteforward" | "dynamicforward" + ) +} + +/// Settings OpenSSH would apply to `host`. /// /// Values are the raw remainder of the directive line (trimmed), so /// multi-word values such as `ProxyCommand` survive intact; callers pick the /// first token where a directive takes a single argument. The first value -/// obtained for a keyword wins, exactly like OpenSSH. Lines before the first -/// `Host`/`Match` apply to every host. `Match all` applies; any other `Match` -/// criteria cannot be evaluated here and are treated as not matching. -pub(crate) fn resolve_host_settings(content: &str, host: &str) -> HashMap { - let mut settings = HashMap::new(); +/// obtained for a keyword wins, exactly like OpenSSH, except for accumulating +/// keywords such as `IdentityFile`, which keep every value in order. Lines +/// before the first `Host`/`Match` apply to every host. `Match all` applies; +/// any other `Match` criteria cannot be evaluated here and are treated as not +/// matching. +pub(crate) fn resolve_host_settings(content: &str, host: &str) -> ResolvedHostSettings { + let mut settings = ResolvedHostSettings::default(); let mut active = true; - for line in content.lines() { + let mut specific = false; + for (order, line) in content.lines().enumerate() { let trimmed = line.trim(); if trimmed.is_empty() || trimmed.starts_with('#') { continue; @@ -184,7 +260,9 @@ pub(crate) fn resolve_host_settings(content: &str, host: &str) -> HashMap HashMap &str { + let mut in_quotes = false; + let mut previous_is_space = true; + for (index, ch) in value.char_indices() { + match ch { + '"' => in_quotes = !in_quotes, + '#' if !in_quotes && previous_is_space => return value[..index].trim_end(), + _ => {} + } + previous_is_space = ch.is_whitespace(); + } + value.trim_end() +} + +fn host_pattern_tokens(patterns: &str) -> impl Iterator { + patterns + .split(|c: char| c.is_whitespace() || c == ',') + .map(|pattern| pattern.trim_matches('"')) + .filter(|pattern| !pattern.is_empty()) +} + +fn host_named_exactly(patterns: &str, host: &str) -> bool { + host_pattern_tokens(patterns) + .any(|pattern| !is_host_pattern(pattern) && pattern.eq_ignore_ascii_case(host)) +} + /// OpenSSH `Host` matching: the line matches when any positive pattern /// matches and no negated (`!`) pattern does. Patterns support `*` and `?`, /// compared case-insensitively, and may be comma- or whitespace-separated. pub(crate) fn host_patterns_match(patterns: &str, host: &str) -> bool { let host = host.to_ascii_lowercase(); let mut matched = false; - for pattern in patterns - .split(|c: char| c.is_whitespace() || c == ',') - .map(|pattern| pattern.trim_matches('"')) - .filter(|pattern| !pattern.is_empty()) - { + for pattern in host_pattern_tokens(strip_trailing_comment(patterns)) { let (negated, pattern) = match pattern.strip_prefix('!') { Some(rest) => (true, rest), None => (false, pattern), @@ -536,6 +646,40 @@ mod tests { fs::remove_dir_all(&dir).unwrap(); } + #[test] + fn nested_includes_restore_the_includers_block_not_global() { + let dir = unique_temp_dir("nested-context"); + let ssh_dir = dir.join(".ssh"); + write( + &ssh_dir.join("config"), + "Host parent\n Include middle.conf\n", + ); + write( + &ssh_dir.join("middle.conf"), + "Include leaf.conf\nUser parent-only\n", + ); + write(&ssh_dir.join("leaf.conf"), "Port 2201\n"); + + let merged = load_merged_config(&ssh_dir.join("config")).unwrap(); + let parent = resolve_host_settings(&merged, "parent"); + assert_eq!(parent.get("user").map(String::as_str), Some("parent-only")); + assert_eq!(parent.get("port").map(String::as_str), Some("2201")); + // A re-emitted `Host *` would have made these global. + let other = resolve_host_settings(&merged, "other"); + assert!(other.is_empty(), "settings leaked globally: {other:?}"); + + fs::remove_dir_all(&dir).unwrap(); + } + + #[test] + fn strips_inline_comments_outside_quotes() { + assert_eq!(strip_trailing_comment("foo bar # note"), "foo bar"); + assert_eq!(strip_trailing_comment("\"a # b\" c"), "\"a # b\" c"); + assert_eq!(strip_trailing_comment("url#frag"), "url#frag"); + assert!(host_patterns_match("foo # comment", "foo")); + assert!(!host_patterns_match("foo # comment", "comment")); + } + #[test] fn same_file_may_be_included_from_different_blocks() { let dir = unique_temp_dir("reinclude");