From 81e6e69c9d648413801b0013cb4c7fe7c8c61d80 Mon Sep 17 00:00:00 2001 From: Pavel Voronov Date: Wed, 12 Aug 2026 00:50:34 +0300 Subject: [PATCH 1/6] fix(ssh): connect ProxyJump hosts through their bastion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ssh_config parser read ProxyJump into `Host.proxy_jump`, but nothing downstream used it: the terminal refused to open such a host outright, while metrics, SFTP, snippets and key setup dialled the target address directly — past the bastion that was the only route to it. `connect_and_auth` now walks the jump chain the way `ssh -J` does. Each hop is connected and authenticated in turn, and the next one rides a `direct-tcpip` channel opened on its predecessor. The new `SshConnection` owns the bastion handles alongside the target's, since dropping one would tear down every tunnel above it; it derefs to the target handle, so callers open channels exactly as before. Chain resolution lives in the new `ssh::jump` module — pure and I/O-free, so it is unit-tested without a filesystem. A jump alias is looked up in the merged host list and inherits that entry's HostName, User, Port and IdentityFile; an alias matching no entry is used as a literal hostname. Multi-hop values, inline `user@host:port` overrides, IPv6 literals, bastions behind bastions and the `ProxyJump none` opt-out all follow OpenSSH. Cycles and chains past ten hops are reported rather than looped over. Also stops the TUI edit form from dropping ProxyJump: it has no field for it, so saving an imported host used to lose the bastion. The GUI already preserved it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 + README.md | 2 +- crates/omnyssh-core/src/ssh/jump.rs | 432 ++++++++++++++++++ crates/omnyssh-core/src/ssh/mod.rs | 1 + crates/omnyssh-core/src/ssh/pty.rs | 10 +- crates/omnyssh-core/src/ssh/session.rs | 172 ++++++- .../ui/src/lib/screens/TerminalView.svelte | 2 +- crates/omnyssh/src/app/host.rs | 5 + 8 files changed, 606 insertions(+), 29 deletions(-) create mode 100644 crates/omnyssh-core/src/ssh/jump.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f761bb..1869afa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ Versions follow [Semantic Versioning](https://semver.org/). --- +## Unreleased + +### Bug Fixes +- **`ProxyJump` hosts now connect through their bastion.** A host with `ProxyJump` in `~/.ssh/config` was parsed but never routed: the terminal refused to open it, while metrics, SFTP, snippets and key setup quietly dialled the target address direct — which for an internal host meant every connection failed or, worse, landed somewhere else on that address. Every native SSH path now walks the jump chain the way `ssh -J` does, connecting and authenticating each bastion in turn and tunnelling the next hop over it. + - The jump alias is resolved against your host list, so `ProxyJump public-proxy` picks up that entry's `HostName`, `User`, `Port` and `IdentityFile`. An alias that matches no entry is used as a literal hostname. + - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does. Chains that loop, or run past ten hops, are reported instead of hanging. + - Each hop's host key is checked against `known_hosts` under its own name, and each hop authenticates with the usual agent → identity file → default keys → password order. + - Editing an imported host in the TUI no longer drops its `ProxyJump`: the form has no field for it, so the saved copy used to lose the bastion. The GUI already preserved it. + +--- + ## 1.1.1 — 2026-07-28 ### Bug Fixes diff --git a/README.md b/README.md index 33a7370..5d0d3d3 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ The script detects your OS and architecture and installs the latest desktop buil | Linux x86_64 | `OmnySSH-x86_64.AppImage` / `.deb` | | Windows x86_64 | `OmnySSH-x86_64-setup.exe` | -No account, no login screen, no telemetry. The app opens with an empty dashboard and reads your existing `~/.ssh/config` if you have one. +No account, no login screen, no telemetry. The app opens with an empty dashboard and reads your existing `~/.ssh/config` if you have one — hosts behind a bastion (`ProxyJump`) included. --- diff --git a/crates/omnyssh-core/src/ssh/jump.rs b/crates/omnyssh-core/src/ssh/jump.rs new file mode 100644 index 0000000..de31e6d --- /dev/null +++ b/crates/omnyssh-core/src/ssh/jump.rs @@ -0,0 +1,432 @@ +//! `ProxyJump` chain resolution. +//! +//! Turns the `ProxyJump` value of a [`Host`] (`bastion`, `ops@jump:2222`, +//! `first,second`, …) into the ordered list of hosts that must be connected +//! **before** the target, nearest to the local machine first. +//! +//! Each hop is looked up in the known host list — the merged `hosts.toml` + +//! `~/.ssh/config` entries — so a jump alias inherits that entry's `HostName`, +//! `User`, `Port` and `IdentityFile`, exactly like an `ssh -J` hop resolves +//! through `ssh_config`. A hop that matches no known entry is used literally as +//! a hostname. A hop with a `ProxyJump` of its own is expanded first, so nested +//! bastions work. +//! +//! Pure and I/O-free: [`resolve_chain`] takes the known hosts as an argument so +//! it can be unit-tested without touching the filesystem. + +use std::collections::HashSet; + +use anyhow::bail; + +use crate::ssh::client::Host; + +/// Upper bound on hops in a resolved chain. Longer chains are almost certainly +/// a configuration mistake; the limit also bounds the expansion recursion. +const MAX_HOPS: usize = 10; + +/// One hop parsed from a `ProxyJump` value: `[user@]host[:port]`. +#[derive(Debug, PartialEq)] +struct JumpSpec { + user: Option, + host: String, + port: Option, +} + +/// Resolves the full jump chain for `target` against the `known` host list. +/// +/// The returned hosts are in connection order: the first entry is reached +/// directly from this machine, each subsequent one through its predecessor, and +/// `target` itself through the last. An empty vector means "connect directly" — +/// no `ProxyJump`, or the OpenSSH `ProxyJump none` opt-out. +/// +/// Every returned host has its own `proxy_jump` cleared: the chain is already +/// flattened, so a caller connecting hop by hop must not expand it again. +/// +/// # Errors +/// Returns an error when the chain references itself (a cycle) or exceeds +/// [`MAX_HOPS`] hops — both would otherwise loop forever at connect time. +pub fn resolve_chain(target: &Host, known: &[Host]) -> anyhow::Result> { + let Some(spec) = jump_value(target) else { + return Ok(Vec::new()); + }; + + let mut chain: Vec = Vec::new(); + let mut visited: HashSet = HashSet::new(); + // Seed with the target so `A -> B -> A` is caught as the cycle it is. + mark_visited(target, &mut visited); + expand(spec, known, &mut chain, &mut visited)?; + Ok(chain) +} + +/// The effective `ProxyJump` value of `host`, or `None` when it connects +/// directly. Blank values and the OpenSSH `none` opt-out both mean "direct". +fn jump_value(host: &Host) -> Option<&str> { + let value = host.proxy_jump.as_deref()?.trim(); + if value.is_empty() || value.eq_ignore_ascii_case("none") { + None + } else { + Some(value) + } +} + +/// Appends the hops of `spec` to `chain`, depth-first: a hop that jumps through +/// another host contributes that host first. +fn expand( + spec: &str, + known: &[Host], + chain: &mut Vec, + visited: &mut HashSet, +) -> anyhow::Result<()> { + for hop in parse_jump_spec(spec) { + let mut host = resolve_hop(&hop, known); + + if !mark_visited(&host, visited) { + bail!("ProxyJump cycle detected at '{}'", host.name); + } + + // A bastion reached through another bastion: connect the inner one first. + if let Some(nested) = jump_value(&host) { + expand(nested, known, chain, visited)?; + } + if chain.len() >= MAX_HOPS { + bail!("ProxyJump chain longer than {MAX_HOPS} hops"); + } + host.proxy_jump = None; + chain.push(host); + } + Ok(()) +} + +/// Turns one parsed hop into a connectable [`Host`]. +/// +/// A hop naming a known entry inherits all of its connection settings; anything +/// else becomes a bare host with default user and port. An explicit `user@` or +/// `:port` in the spec always wins over the inherited value. +fn resolve_hop(spec: &JumpSpec, known: &[Host]) -> Host { + let mut host = match known.iter().find(|h| h.name == spec.host) { + Some(h) => h.clone(), + None => Host { + name: spec.host.clone(), + hostname: spec.host.clone(), + ..Host::default() + }, + }; + if let Some(user) = &spec.user { + host.user = user.clone(); + } + if let Some(port) = spec.port { + host.port = port; + } + // A known entry may omit HostName; the alias is then the address (the same + // fallback the ssh_config parser applies). + if host.hostname.is_empty() { + host.hostname = host.name.clone(); + } + host +} + +/// Records `host` as part of the chain being built, returning `false` when it +/// was already there — a cycle. +/// +/// A hop counts as seen both by alias and by endpoint, so a loop is caught +/// whether it comes back under the same name or under a second alias for the +/// same machine. +fn mark_visited(host: &Host, visited: &mut HashSet) -> bool { + let by_name = visited.insert(format!("name:{}", host.name)); + let by_address = visited.insert(format!( + "address:{}@{}:{}", + host.user, host.hostname, host.port + )); + by_name && by_address +} + +/// Splits a `ProxyJump` value into its comma-separated hops, nearest first. +/// +/// Unparseable hops (an empty entry, a non-numeric port) are skipped rather +/// than failing the whole connection — the remaining hops still describe a +/// usable route, and an unreachable one surfaces as a normal connection error. +fn parse_jump_spec(value: &str) -> Vec { + value.split(',').filter_map(parse_hop).collect() +} + +/// Parses a single `[user@]host[:port]` hop. Bracketed IPv6 literals +/// (`[2001:db8::1]:2222`) are supported, matching `ssh -J`. +fn parse_hop(hop: &str) -> Option { + let hop = hop.trim(); + if hop.is_empty() { + return None; + } + + // Split on the last '@': a username cannot contain one, a host never does. + let (user, rest) = match hop.rsplit_once('@') { + Some((user, rest)) if !user.is_empty() => (Some(user.to_string()), rest), + _ => (None, hop), + }; + + let (host, port) = split_host_port(rest)?; + if host.is_empty() { + return None; + } + Some(JumpSpec { + user, + host: host.to_string(), + port, + }) +} + +/// Splits `host`, `host:port`, `[v6]` or `[v6]:port` into its two parts. +/// Returns `None` when a port is present but not a valid number. +fn split_host_port(rest: &str) -> Option<(&str, Option)> { + // `end` indexes the ']' relative to the stripped string, i.e. the last + // character of the address inside the brackets. + if let Some(end) = rest.strip_prefix('[').and_then(|r| r.find(']')) { + let host = &rest[1..=end]; + return match rest[end + 2..].strip_prefix(':') { + Some(port) => Some((host, Some(port.parse().ok()?))), + None => Some((host, None)), + }; + } + // An unbracketed colon separates the port only when it is the sole one; + // a bare IPv6 literal has several and carries no port. + match rest.split_once(':') { + Some((host, port)) if !port.contains(':') => Some((host, Some(port.parse().ok()?))), + Some(_) => Some((rest, None)), + None => Some((rest, None)), + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + + fn host(name: &str, hostname: &str) -> Host { + Host { + name: name.to_string(), + hostname: hostname.to_string(), + user: "ops".to_string(), + ..Host::default() + } + } + + fn jumping(name: &str, hostname: &str, via: &str) -> Host { + Host { + proxy_jump: Some(via.to_string()), + ..host(name, hostname) + } + } + + // --- spec parsing ------------------------------------------------------ + + #[test] + fn parses_a_bare_alias() { + assert_eq!( + parse_jump_spec("bastion"), + vec![JumpSpec { + user: None, + host: "bastion".into(), + port: None + }] + ); + } + + #[test] + fn parses_user_host_and_port() { + assert_eq!( + parse_jump_spec("ops@jump.example.com:2222"), + vec![JumpSpec { + user: Some("ops".into()), + host: "jump.example.com".into(), + port: Some(2222) + }] + ); + } + + #[test] + fn parses_a_multi_hop_value_in_order() { + let hops = parse_jump_spec("first, ops@second:2222"); + assert_eq!(hops.len(), 2); + assert_eq!(hops[0].host, "first"); + assert_eq!(hops[1].host, "second"); + assert_eq!(hops[1].port, Some(2222)); + } + + #[test] + fn parses_ipv6_literals() { + let bare = parse_jump_spec("2001:db8::1"); + assert_eq!(bare[0].host, "2001:db8::1"); + assert_eq!(bare[0].port, None); + + let bracketed = parse_jump_spec("ops@[2001:db8::1]:2222"); + assert_eq!(bracketed[0].host, "2001:db8::1"); + assert_eq!(bracketed[0].port, Some(2222)); + assert_eq!(bracketed[0].user.as_deref(), Some("ops")); + } + + #[test] + fn skips_unusable_hops() { + assert!(parse_jump_spec("").is_empty()); + assert!(parse_jump_spec(" , ").is_empty()); + assert!(parse_jump_spec("host:not-a-port").is_empty()); + } + + // --- chain resolution -------------------------------------------------- + + #[test] + fn no_proxy_jump_means_no_chain() { + let target = host("web", "10.0.0.1"); + assert!(resolve_chain(&target, &[]).unwrap().is_empty()); + } + + #[test] + fn proxy_jump_none_opts_out() { + let target = jumping("web", "10.0.0.1", "none"); + assert!(resolve_chain(&target, &[]).unwrap().is_empty()); + } + + #[test] + fn resolves_an_alias_against_the_known_hosts() { + // The reported bug: `ProxyJump public-proxy` where `public-proxy` is + // another entry in the same config. + let known = vec![Host { + port: 2222, + identity_file: Some("/keys/proxy".into()), + ..host("public-proxy", "proxy.example.com") + }]; + let target = jumping("internal", "192.168.100.50", "public-proxy"); + + let chain = resolve_chain(&target, &known).unwrap(); + assert_eq!(chain.len(), 1); + assert_eq!(chain[0].hostname, "proxy.example.com"); + assert_eq!(chain[0].user, "ops"); + assert_eq!(chain[0].port, 2222); + assert_eq!(chain[0].identity_file.as_deref(), Some("/keys/proxy")); + } + + #[test] + fn resolves_a_chain_parsed_straight_from_an_ssh_config() { + // End to end over the two halves that must agree: what the parser + // produces is what the resolver looks jump aliases up in. + let cfg = "\ +Host public-proxy + HostName proxy.example.com + User ops + +Host internal + HostName 192.168.100.50 + User admin + ProxyJump public-proxy +"; + let hosts = crate::config::ssh_config::parse_ssh_config(cfg); + let target = hosts.iter().find(|h| h.name == "internal").unwrap(); + + let chain = resolve_chain(target, &hosts).unwrap(); + assert_eq!(chain.len(), 1); + assert_eq!(chain[0].name, "public-proxy"); + assert_eq!(chain[0].hostname, "proxy.example.com"); + assert_eq!(chain[0].user, "ops"); + assert_eq!(chain[0].port, 22); + } + + #[test] + fn unknown_alias_falls_back_to_a_literal_host() { + let target = jumping("internal", "10.0.0.2", "jump.example.com"); + let chain = resolve_chain(&target, &[]).unwrap(); + assert_eq!(chain.len(), 1); + assert_eq!(chain[0].hostname, "jump.example.com"); + assert_eq!(chain[0].port, 22); + } + + #[test] + fn explicit_user_and_port_override_the_known_entry() { + let known = vec![Host { + port: 2222, + ..host("public-proxy", "proxy.example.com") + }]; + let target = jumping("internal", "10.0.0.2", "admin@public-proxy:2022"); + + let chain = resolve_chain(&target, &known).unwrap(); + assert_eq!(chain[0].hostname, "proxy.example.com"); // still inherited + assert_eq!(chain[0].user, "admin"); + assert_eq!(chain[0].port, 2022); + } + + #[test] + fn a_hostname_less_entry_uses_its_alias_as_the_address() { + let known = vec![Host { + hostname: String::new(), + ..host("public-proxy", "") + }]; + let target = jumping("internal", "10.0.0.2", "public-proxy"); + assert_eq!( + resolve_chain(&target, &known).unwrap()[0].hostname, + "public-proxy" + ); + } + + #[test] + fn multi_hop_chains_keep_connection_order() { + let known = vec![host("first", "10.0.0.1"), host("second", "10.0.0.2")]; + let target = jumping("internal", "10.0.0.3", "first,second"); + + let chain = resolve_chain(&target, &known).unwrap(); + let names: Vec<&str> = chain.iter().map(|h| h.name.as_str()).collect(); + assert_eq!(names, ["first", "second"]); + } + + #[test] + fn a_nested_jump_host_is_connected_first() { + let known = vec![ + host("outer", "10.0.0.1"), + jumping("inner", "10.0.0.2", "outer"), + ]; + let target = jumping("internal", "10.0.0.3", "inner"); + + let chain = resolve_chain(&target, &known).unwrap(); + let names: Vec<&str> = chain.iter().map(|h| h.name.as_str()).collect(); + assert_eq!(names, ["outer", "inner"]); + // Flattened: the caller connects hop by hop and must not re-expand. + assert!(chain.iter().all(|h| h.proxy_jump.is_none())); + } + + #[test] + fn a_self_referencing_jump_is_rejected() { + let known = vec![jumping("loop", "10.0.0.1", "loop")]; + let target = jumping("internal", "10.0.0.2", "loop"); + let err = resolve_chain(&target, &known).unwrap_err().to_string(); + assert!(err.contains("cycle"), "unexpected error: {err}"); + } + + #[test] + fn two_aliases_for_one_bastion_are_rejected() { + // Same machine, different name: still a loop, just a less obvious one. + let known = vec![ + jumping("proxy-a", "10.0.0.1", "proxy-b"), + host("proxy-b", "10.0.0.1"), + ]; + let target = jumping("internal", "10.0.0.2", "proxy-a"); + assert!(resolve_chain(&target, &known).is_err()); + } + + #[test] + fn a_jump_back_to_the_target_is_rejected() { + let known = vec![jumping("bastion", "10.0.0.1", "internal")]; + let target = jumping("internal", "10.0.0.2", "bastion"); + assert!(resolve_chain(&target, &known).is_err()); + } + + #[test] + fn an_over_long_chain_is_rejected() { + let hops: Vec = (0..MAX_HOPS + 1).map(|i| format!("h{i}")).collect(); + let known: Vec = hops + .iter() + .enumerate() + .map(|(i, name)| host(name, &format!("10.0.0.{i}"))) + .collect(); + let target = jumping("internal", "10.1.0.1", &hops.join(",")); + assert!(resolve_chain(&target, &known).is_err()); + } +} diff --git a/crates/omnyssh-core/src/ssh/mod.rs b/crates/omnyssh-core/src/ssh/mod.rs index ca293a3..85be5f0 100644 --- a/crates/omnyssh-core/src/ssh/mod.rs +++ b/crates/omnyssh-core/src/ssh/mod.rs @@ -5,6 +5,7 @@ /// discovery and Auto SSH Key Setup for secure authentication. pub mod client; pub mod discovery; +pub mod jump; pub mod key_setup; pub mod metrics; pub mod pool; diff --git a/crates/omnyssh-core/src/ssh/pty.rs b/crates/omnyssh-core/src/ssh/pty.rs index 9f166c1..971cd87 100644 --- a/crates/omnyssh-core/src/ssh/pty.rs +++ b/crates/omnyssh-core/src/ssh/pty.rs @@ -14,13 +14,12 @@ use std::sync::{Arc, Mutex}; use anyhow::{Context, Result}; -use russh::client::Handle; use russh::ChannelMsg; use tokio::sync::mpsc; use crate::event::CoreEvent; use crate::ssh::client::Host; -use crate::ssh::session::{connect_and_auth, KnownHostsHandler}; +use crate::ssh::session::{connect_and_auth, SshConnection}; /// Stable numeric identifier for a PTY session (mirrors [`crate::event::SessionId`]). pub type SessionId = u64; @@ -140,7 +139,7 @@ async fn forward_locale(channel: &russh::Channel) { /// Opens a channel and requests a remote PTY + shell (the `ssh -t` equivalent). async fn open_shell( - handle: &Handle, + handle: &SshConnection, cols: u16, rows: u16, ) -> Result> { @@ -292,11 +291,6 @@ impl PtyManager { rows: u16, tx: mpsc::Sender, ) -> Result { - // ProxyJump is not yet wired into the russh terminal path. Refuse rather - // than silently connecting direct to the wrong host. - if host.proxy_jump.is_some() { - anyhow::bail!("ProxyJump is not yet supported in the terminal"); - } let id = self.next_id; self.next_id += 1; let parser = Arc::new(Mutex::new(vt100::Parser::new(rows, cols, 1000))); diff --git a/crates/omnyssh-core/src/ssh/session.rs b/crates/omnyssh-core/src/ssh/session.rs index 829f03f..25dc131 100644 --- a/crates/omnyssh-core/src/ssh/session.rs +++ b/crates/omnyssh-core/src/ssh/session.rs @@ -4,8 +4,12 @@ //! supports connecting, executing commands, and graceful disconnect. //! Authentication order: identity file → SSH agent → failure. //! +//! Hosts with a `ProxyJump` are reached through their bastions: each hop is +//! connected and authenticated in turn, and the next hop rides a +//! `direct-tcpip` channel opened on the previous one (the `ssh -J` model). +//! //! Connection and command timeouts are enforced: -//! - Connect timeout: 10 seconds +//! - Connect timeout: 10 seconds (per hop) //! - Command timeout: 30 seconds use std::sync::Arc; @@ -98,6 +102,31 @@ impl client::Handler for KnownHostsHandler { } } +// --------------------------------------------------------------------------- +// SshConnection +// --------------------------------------------------------------------------- + +/// An authenticated russh connection to one host, plus the jump-host +/// connections it is tunnelled through (empty for a direct connection). +/// +/// The jump handles are held for the whole lifetime of the connection: each one +/// carries the `direct-tcpip` channel of the hop above it, so dropping a bastion +/// handle would tear down everything beyond it. Derefs to the target's +/// [`Handle`], so callers open channels on it exactly as before. +pub(crate) struct SshConnection { + handle: Handle, + /// Bastions, nearest-first. Never used directly — kept alive by ownership. + _jumps: Vec>, +} + +impl std::ops::Deref for SshConnection { + type Target = Handle; + + fn deref(&self) -> &Self::Target { + &self.handle + } +} + // --------------------------------------------------------------------------- // SshSession // --------------------------------------------------------------------------- @@ -110,7 +139,7 @@ impl client::Handler for KnownHostsHandler { /// Wrapped in Arc to allow sharing across multiple operations (discovery + metrics). #[derive(Clone)] pub struct SshSession { - handle: Arc>, + handle: Arc, } impl SshSession { @@ -122,12 +151,16 @@ impl SshSession { /// 3. Default key files (`~/.ssh/id_ed25519`, `id_rsa`, etc.). /// 4. Password (if provided in host config). /// + /// A host with a `ProxyJump` is reached through its bastion chain; each hop + /// authenticates the same way. + /// /// Returns an error when no method succeeds or the connection times out. /// /// # Errors - /// - Connection timeout (> 10 s) + /// - Connection timeout (> 10 s per hop) /// - Authentication failure /// - Network error + /// - An unresolvable `ProxyJump` chain (cycle or too many hops) pub async fn connect(host: &Host) -> anyhow::Result { Ok(Self { handle: Arc::new(connect_and_auth(host).await?), @@ -215,38 +248,139 @@ impl SshSession { // Connection + authentication // --------------------------------------------------------------------------- -/// Connect to `host`, verify its host key, and authenticate. +/// Per-hop budget for the TCP connect and SSH handshake. +const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); + +/// Connect to `host`, verify its host key, and authenticate — through the +/// host's `ProxyJump` chain when it has one. /// /// Shared by [`SshSession::connect`] (metrics/SFTP) and the terminal so every -/// native SSH path honors the same keys, agent, passwords, and known_hosts -/// policy. +/// native SSH path honors the same keys, agent, passwords, known_hosts policy, +/// and bastions. /// /// # Errors -/// Connection timeout (> 10 s), host-key rejection, or authentication failure. -pub(crate) async fn connect_and_auth(host: &Host) -> anyhow::Result> { - let config = Arc::new(client::Config { +/// Connection timeout (> 10 s per hop), host-key rejection, authentication +/// failure, or an unresolvable `ProxyJump` chain. +pub(crate) async fn connect_and_auth(host: &Host) -> anyhow::Result { + let chain = jump_chain(host).await?; + let config = client_config(); + + // Walk the bastions outward: the first is reached directly, every later one + // through its predecessor. The target then rides the last hop. + let mut jumps: Vec> = Vec::with_capacity(chain.len()); + for hop in &chain { + let handle = match jumps.last() { + None => connect_direct(&config, hop).await, + Some(via) => connect_tunnelled(&config, via, hop).await, + } + .with_context(|| format!("ProxyJump via '{}' failed", hop.name))?; + jumps.push(handle); + } + + let handle = match jumps.last() { + None => connect_direct(&config, host).await?, + Some(via) => connect_tunnelled(&config, via, host).await?, + }; + Ok(SshConnection { + handle, + _jumps: jumps, + }) +} + +/// The shared russh client configuration (timeouts + keepalives). +fn client_config() -> Arc { + Arc::new(client::Config { inactivity_timeout: Some(Duration::from_secs(30)), keepalive_interval: Some(Duration::from_secs(15)), keepalive_max: 3, ..Default::default() - }); + }) +} + +/// Resolves `host`'s `ProxyJump` into the hops to connect before it. +/// +/// The jump aliases are looked up in the merged host list, so a bastion defined +/// elsewhere in `~/.ssh/config` (or in `hosts.toml`) contributes its own +/// HostName/User/Port/IdentityFile. Loading is skipped entirely for the common +/// no-`ProxyJump` case, and an unreadable host list degrades to treating the +/// alias as a literal hostname rather than failing the connection. +async fn jump_chain(host: &Host) -> anyhow::Result> { + if host.proxy_jump.is_none() { + return Ok(Vec::new()); + } + // load_all_hosts() is blocking file I/O — keep it off the async worker. + let known = tokio::task::spawn_blocking(crate::config::load_all_hosts) + .await + .context("host list load panicked")? + .unwrap_or_else(|e| { + tracing::warn!(error = %e, "could not load hosts for ProxyJump resolution"); + Vec::new() + }); + crate::ssh::jump::resolve_chain(host, &known) +} +/// Opens a TCP connection to `host` and authenticates. +async fn connect_direct( + config: &Arc, + host: &Host, +) -> anyhow::Result> { let addr = format!("{}:{}", host.hostname, host.port); - let mut handle = time::timeout( - Duration::from_secs(10), - client::connect( - config, - addr, - KnownHostsHandler { - host: host.hostname.clone(), - port: host.port, - }, + let handle = time::timeout( + CONNECT_TIMEOUT, + client::connect(Arc::clone(config), addr, known_hosts_handler(host)), + ) + .await + .map_err(|_| anyhow!("SSH connection timed out (10 s)"))? + .context("SSH connection failed")?; + + finish_auth(handle, host).await +} + +/// Reaches `host` through the already-connected bastion `via`: a `direct-tcpip` +/// channel on the bastion carries a second SSH session to the target, which is +/// verified and authenticated in its own right. +async fn connect_tunnelled( + config: &Arc, + via: &Handle, + host: &Host, +) -> anyhow::Result> { + // The originator address is informational; ssh(1) reports the loopback it + // forwards from, and servers only log it. + let channel = via + .channel_open_direct_tcpip(host.hostname.clone(), host.port as u32, "127.0.0.1", 0) + .await + .with_context(|| format!("open tunnel to {}:{}", host.hostname, host.port))?; + + let handle = time::timeout( + CONNECT_TIMEOUT, + client::connect_stream( + Arc::clone(config), + channel.into_stream(), + known_hosts_handler(host), ), ) .await .map_err(|_| anyhow!("SSH connection timed out (10 s)"))? .context("SSH connection failed")?; + finish_auth(handle, host).await +} + +/// The host-key verifier for `host`. The lookup uses the target's own +/// hostname/port even over a tunnel, so `known_hosts` entries match what an +/// `ssh -J` would record. +fn known_hosts_handler(host: &Host) -> KnownHostsHandler { + KnownHostsHandler { + host: host.hostname.clone(), + port: host.port, + } +} + +/// Authenticates `handle` as `host`, converting a refusal into an error. +async fn finish_auth( + mut handle: Handle, + host: &Host, +) -> anyhow::Result> { if !authenticate(&mut handle, host).await? { return Err(anyhow!("SSH authentication failed for {}", host.name)); } diff --git a/crates/omnyssh-gui/ui/src/lib/screens/TerminalView.svelte b/crates/omnyssh-gui/ui/src/lib/screens/TerminalView.svelte index 5bb80ca..ea4f521 100644 --- a/crates/omnyssh-gui/ui/src/lib/screens/TerminalView.svelte +++ b/crates/omnyssh-gui/ui/src/lib/screens/TerminalView.svelte @@ -151,7 +151,7 @@ ready = true; if (active) term.focus(); })().catch((err) => { - // `terminal_open` itself failed (e.g. an unsupported ProxyJump host): no + // `terminal_open` itself failed (e.g. the session could not be spawned): no // PtyExited follows, so mark the tab failed here instead of leaving it hung. lastError.set(err instanceof Error ? err.message : String(err)); sessions.setStatus(session.id, 'failed'); diff --git a/crates/omnyssh/src/app/host.rs b/crates/omnyssh/src/app/host.rs index 1b2920d..f730332 100644 --- a/crates/omnyssh/src/app/host.rs +++ b/crates/omnyssh/src/app/host.rs @@ -451,6 +451,11 @@ impl App { .map(|h| h.source == HostSource::SshConfig) .unwrap_or(false); + // ProxyJump has no form field, so carry it over: editing + // an imported host would otherwise drop its bastion and + // the saved copy would try to connect direct. + host.proxy_jump = old_host.and_then(|h| h.proxy_jump.clone()); + // If editing a SSH config host, preserve original name for duplicate prevention if was_ssh_config && old_name.is_some() { host.original_ssh_host = old_name.clone(); From b547fe949e5170d575616ac2fdc44a61f30eb699 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 11:33:02 +0400 Subject: [PATCH 2/6] fix(ssh): resolve ProxyJump chains the way ssh -J does --- crates/omnyssh-core/src/ssh/jump.rs | 363 ++++++++++++++++++++-------- 1 file changed, 261 insertions(+), 102 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/jump.rs b/crates/omnyssh-core/src/ssh/jump.rs index de31e6d..bf9b6e0 100644 --- a/crates/omnyssh-core/src/ssh/jump.rs +++ b/crates/omnyssh-core/src/ssh/jump.rs @@ -8,20 +8,18 @@ //! `~/.ssh/config` entries — so a jump alias inherits that entry's `HostName`, //! `User`, `Port` and `IdentityFile`, exactly like an `ssh -J` hop resolves //! through `ssh_config`. A hop that matches no known entry is used literally as -//! a hostname. A hop with a `ProxyJump` of its own is expanded first, so nested -//! bastions work. +//! a hostname. //! //! Pure and I/O-free: [`resolve_chain`] takes the known hosts as an argument so //! it can be unit-tested without touching the filesystem. -use std::collections::HashSet; - -use anyhow::bail; +use anyhow::{bail, Context}; use crate::ssh::client::Host; -/// Upper bound on hops in a resolved chain. Longer chains are almost certainly -/// a configuration mistake; the limit also bounds the expansion recursion. +/// Upper bound on the hops in a resolved chain, and on the depth of the +/// expansion recursion. Longer chains are almost certainly a configuration +/// mistake. const MAX_HOPS: usize = 10; /// One hop parsed from a `ProxyJump` value: `[user@]host[:port]`. @@ -32,6 +30,16 @@ struct JumpSpec { port: Option, } +/// One chain walk in progress. +struct Walk { + /// Hops resolved so far, in connection order. + chain: Vec, + /// Hops whose own `ProxyJump` is being expanded right now, plus the target + /// that started the walk. Re-entering one of these is a cycle; the length + /// is the recursion depth. + active: Vec, +} + /// Resolves the full jump chain for `target` against the `known` host list. /// /// The returned hosts are in connection order: the first entry is reached @@ -43,24 +51,27 @@ struct JumpSpec { /// flattened, so a caller connecting hop by hop must not expand it again. /// /// # Errors -/// Returns an error when the chain references itself (a cycle) or exceeds -/// [`MAX_HOPS`] hops — both would otherwise loop forever at connect time. +/// Returns an error when a hop is unusable, when the chain references itself (a +/// cycle), or when it exceeds [`MAX_HOPS`] hops. A host that names a bastion +/// never resolves to an empty chain: failing is the only alternative to +/// connecting straight to the target, past the bastion that is its only route. pub fn resolve_chain(target: &Host, known: &[Host]) -> anyhow::Result> { let Some(spec) = jump_value(target) else { return Ok(Vec::new()); }; - let mut chain: Vec = Vec::new(); - let mut visited: HashSet = HashSet::new(); - // Seed with the target so `A -> B -> A` is caught as the cycle it is. - mark_visited(target, &mut visited); - expand(spec, known, &mut chain, &mut visited)?; - Ok(chain) + let mut walk = Walk { + chain: Vec::new(), + // Seed with the target so `A -> B -> A` is caught as the cycle it is. + active: vec![target.clone()], + }; + walk.expand(spec, known)?; + Ok(walk.chain) } /// The effective `ProxyJump` value of `host`, or `None` when it connects /// directly. Blank values and the OpenSSH `none` opt-out both mean "direct". -fn jump_value(host: &Host) -> Option<&str> { +pub(crate) fn jump_value(host: &Host) -> Option<&str> { let value = host.proxy_jump.as_deref()?.trim(); if value.is_empty() || value.eq_ignore_ascii_case("none") { None @@ -69,32 +80,54 @@ fn jump_value(host: &Host) -> Option<&str> { } } -/// Appends the hops of `spec` to `chain`, depth-first: a hop that jumps through -/// another host contributes that host first. -fn expand( - spec: &str, - known: &[Host], - chain: &mut Vec, - visited: &mut HashSet, -) -> anyhow::Result<()> { - for hop in parse_jump_spec(spec) { - let mut host = resolve_hop(&hop, known); - - if !mark_visited(&host, visited) { - bail!("ProxyJump cycle detected at '{}'", host.name); +impl Walk { + /// Appends the hops of `spec` to the chain, nearest hop first. + /// + /// Only the *first* hop of a list carries bastions of its own. That is what + /// `ssh` does: for `ProxyJump a,b` it reaches `b` with `-J a` on the command + /// line, and a command-line jump list makes it ignore `b`'s own configured + /// `ProxyJump`. `a` is then reached by a plain `ssh`, which does read its + /// `ProxyJump` — so nested bastions still work, one level in from each list. + fn expand(&mut self, spec: &str, known: &[Host]) -> anyhow::Result<()> { + let hops = parse_jump_spec(spec)?; + + for (index, hop) in hops.iter().enumerate() { + let mut host = resolve_hop(hop, known); + + if self.active.iter().any(|h| same_hop(h, &host)) { + bail!("ProxyJump cycle detected at '{}'", host.name); + } + // Already reached earlier in the chain: connecting it a second time + // would add a pointless hop, not close a loop. + if self.chain.iter().any(|h| same_hop(h, &host)) { + continue; + } + + let nested = if index == 0 { jump_value(&host) } else { None }; + if let Some(nested) = nested { + if self.active.len() > MAX_HOPS { + bail!("ProxyJump chain nested deeper than {MAX_HOPS} hops"); + } + self.active.push(host.clone()); + let expanded = self.expand(nested, known); + self.active.pop(); + expanded?; + } + + if self.chain.len() >= MAX_HOPS { + bail!("ProxyJump chain longer than {MAX_HOPS} hops"); + } + host.proxy_jump = None; + self.chain.push(host); } - - // A bastion reached through another bastion: connect the inner one first. - if let Some(nested) = jump_value(&host) { - expand(nested, known, chain, visited)?; - } - if chain.len() >= MAX_HOPS { - bail!("ProxyJump chain longer than {MAX_HOPS} hops"); - } - host.proxy_jump = None; - chain.push(host); + Ok(()) } - Ok(()) +} + +/// Whether two hops are the same machine — the same alias, or the same +/// endpoint reached under a second name. +fn same_hop(a: &Host, b: &Host) -> bool { + a.name == b.name || (a.user == b.user && a.hostname == b.hostname && a.port == b.port) } /// Turns one parsed hop into a connectable [`Host`]. @@ -103,13 +136,27 @@ fn expand( /// else becomes a bare host with default user and port. An explicit `user@` or /// `:port` in the spec always wins over the inherited value. fn resolve_hop(spec: &JumpSpec, known: &[Host]) -> Host { - let mut host = match known.iter().find(|h| h.name == spec.host) { + // A host imported from `~/.ssh/config` and then renamed keeps its original + // alias, which is still what every other entry's `ProxyJump` names. + let entry = known.iter().find(|h| { + h.name == spec.host || h.original_ssh_host.as_deref() == Some(spec.host.as_str()) + }); + + let mut host = match entry { Some(h) => h.clone(), - None => Host { - name: spec.host.clone(), - hostname: spec.host.clone(), - ..Host::default() - }, + None => { + // Nothing to inherit. Worth a line: an alias that was meant to match + // an entry now resolves through DNS instead. + tracing::debug!( + hop = %spec.host, + "ProxyJump hop matches no known host; using it as a hostname" + ); + Host { + name: spec.host.clone(), + hostname: spec.host.clone(), + ..Host::default() + } + } }; if let Some(user) = &spec.user { host.user = user.clone(); @@ -125,36 +172,22 @@ fn resolve_hop(spec: &JumpSpec, known: &[Host]) -> Host { host } -/// Records `host` as part of the chain being built, returning `false` when it -/// was already there — a cycle. -/// -/// A hop counts as seen both by alias and by endpoint, so a loop is caught -/// whether it comes back under the same name or under a second alias for the -/// same machine. -fn mark_visited(host: &Host, visited: &mut HashSet) -> bool { - let by_name = visited.insert(format!("name:{}", host.name)); - let by_address = visited.insert(format!( - "address:{}@{}:{}", - host.user, host.hostname, host.port - )); - by_name && by_address -} - /// Splits a `ProxyJump` value into its comma-separated hops, nearest first. /// -/// Unparseable hops (an empty entry, a non-numeric port) are skipped rather -/// than failing the whole connection — the remaining hops still describe a -/// usable route, and an unreachable one surfaces as a normal connection error. -fn parse_jump_spec(value: &str) -> Vec { - value.split(',').filter_map(parse_hop).collect() +/// # Errors +/// An unusable hop fails the whole value. Dropping it would shorten the route, +/// and dropping the only hop would connect straight to the target — past the +/// bastion the value exists to name. +fn parse_jump_spec(value: &str) -> anyhow::Result> { + value.split(',').map(parse_hop).collect() } /// Parses a single `[user@]host[:port]` hop. Bracketed IPv6 literals /// (`[2001:db8::1]:2222`) are supported, matching `ssh -J`. -fn parse_hop(hop: &str) -> Option { +fn parse_hop(hop: &str) -> anyhow::Result { let hop = hop.trim(); if hop.is_empty() { - return None; + bail!("empty ProxyJump hop"); } // Split on the last '@': a username cannot contain one, a host never does. @@ -163,11 +196,12 @@ fn parse_hop(hop: &str) -> Option { _ => (None, hop), }; - let (host, port) = split_host_port(rest)?; + let (host, port) = + split_host_port(rest).with_context(|| format!("unusable ProxyJump hop '{hop}'"))?; if host.is_empty() { - return None; + bail!("ProxyJump hop '{hop}' has no host"); } - Some(JumpSpec { + Ok(JumpSpec { user, host: host.to_string(), port, @@ -175,23 +209,34 @@ fn parse_hop(hop: &str) -> Option { } /// Splits `host`, `host:port`, `[v6]` or `[v6]:port` into its two parts. -/// Returns `None` when a port is present but not a valid number. -fn split_host_port(rest: &str) -> Option<(&str, Option)> { +fn split_host_port(rest: &str) -> anyhow::Result<(&str, Option)> { // `end` indexes the ']' relative to the stripped string, i.e. the last // character of the address inside the brackets. if let Some(end) = rest.strip_prefix('[').and_then(|r| r.find(']')) { let host = &rest[1..=end]; - return match rest[end + 2..].strip_prefix(':') { - Some(port) => Some((host, Some(port.parse().ok()?))), - None => Some((host, None)), - }; + let trailer = &rest[end + 2..]; + if trailer.is_empty() { + return Ok((host, None)); + } + let port = trailer + .strip_prefix(':') + .with_context(|| format!("trailing '{trailer}' after ']'"))?; + return Ok((host, Some(parse_port(port)?))); } // An unbracketed colon separates the port only when it is the sole one; // a bare IPv6 literal has several and carries no port. match rest.split_once(':') { - Some((host, port)) if !port.contains(':') => Some((host, Some(port.parse().ok()?))), - Some(_) => Some((rest, None)), - None => Some((rest, None)), + Some((host, port)) if !port.contains(':') => Ok((host, Some(parse_port(port)?))), + _ => Ok((rest, None)), + } +} + +/// Parses a hop's port. Zero is rejected the way `ssh` rejects it — it can +/// never name a listening service. +fn parse_port(value: &str) -> anyhow::Result { + match value.parse::() { + Ok(0) | Err(_) => bail!("'{value}' is not a valid port"), + Ok(port) => Ok(port), } } @@ -219,12 +264,16 @@ mod tests { } } + fn names(chain: &[Host]) -> Vec<&str> { + chain.iter().map(|h| h.name.as_str()).collect() + } + // --- spec parsing ------------------------------------------------------ #[test] fn parses_a_bare_alias() { assert_eq!( - parse_jump_spec("bastion"), + parse_jump_spec("bastion").unwrap(), vec![JumpSpec { user: None, host: "bastion".into(), @@ -236,7 +285,7 @@ mod tests { #[test] fn parses_user_host_and_port() { assert_eq!( - parse_jump_spec("ops@jump.example.com:2222"), + parse_jump_spec("ops@jump.example.com:2222").unwrap(), vec![JumpSpec { user: Some("ops".into()), host: "jump.example.com".into(), @@ -247,7 +296,7 @@ mod tests { #[test] fn parses_a_multi_hop_value_in_order() { - let hops = parse_jump_spec("first, ops@second:2222"); + let hops = parse_jump_spec("first, ops@second:2222").unwrap(); assert_eq!(hops.len(), 2); assert_eq!(hops[0].host, "first"); assert_eq!(hops[1].host, "second"); @@ -256,21 +305,38 @@ mod tests { #[test] fn parses_ipv6_literals() { - let bare = parse_jump_spec("2001:db8::1"); + let bare = parse_jump_spec("2001:db8::1").unwrap(); assert_eq!(bare[0].host, "2001:db8::1"); assert_eq!(bare[0].port, None); - let bracketed = parse_jump_spec("ops@[2001:db8::1]:2222"); + let bracketed = parse_jump_spec("ops@[2001:db8::1]:2222").unwrap(); assert_eq!(bracketed[0].host, "2001:db8::1"); assert_eq!(bracketed[0].port, Some(2222)); assert_eq!(bracketed[0].user.as_deref(), Some("ops")); + + assert_eq!(parse_jump_spec("[2001:db8::1]").unwrap()[0].port, None); } #[test] - fn skips_unusable_hops() { - assert!(parse_jump_spec("").is_empty()); - assert!(parse_jump_spec(" , ").is_empty()); - assert!(parse_jump_spec("host:not-a-port").is_empty()); + fn rejects_unusable_hops() { + // Dropping any of these would silently shorten the route. + for value in [ + "", + " , ", + "host:not-a-port", + "host:", + "host:70000", + "host:0", + "gw: 2222", + "ops@", + "[2001:db8::1]junk:22", + "first,bad:port", + ] { + assert!( + parse_jump_spec(value).is_err(), + "expected '{value}' to be rejected" + ); + } } // --- chain resolution -------------------------------------------------- @@ -287,6 +353,14 @@ mod tests { assert!(resolve_chain(&target, &[]).unwrap().is_empty()); } + #[test] + fn a_malformed_value_fails_instead_of_connecting_direct() { + // The whole point of the module: never silently return an empty chain + // for a host that names a bastion. + let target = jumping("internal", "192.168.100.50", "public-proxy:22x"); + assert!(resolve_chain(&target, &[]).is_err()); + } + #[test] fn resolves_an_alias_against_the_known_hosts() { // The reported bug: `ProxyJump public-proxy` where `public-proxy` is @@ -331,6 +405,21 @@ Host internal assert_eq!(chain[0].port, 22); } + #[test] + fn a_renamed_bastion_is_still_found_under_its_original_alias() { + // Editing an imported host renames it and records the original; every + // other entry's ProxyJump still names the original. + let known = vec![Host { + original_ssh_host: Some("public-proxy".into()), + ..host("Prod Bastion", "proxy.example.com") + }]; + let target = jumping("internal", "10.0.0.2", "public-proxy"); + assert_eq!( + resolve_chain(&target, &known).unwrap()[0].hostname, + "proxy.example.com" + ); + } + #[test] fn unknown_alias_falls_back_to_a_literal_host() { let target = jumping("internal", "10.0.0.2", "jump.example.com"); @@ -373,8 +462,7 @@ Host internal let target = jumping("internal", "10.0.0.3", "first,second"); let chain = resolve_chain(&target, &known).unwrap(); - let names: Vec<&str> = chain.iter().map(|h| h.name.as_str()).collect(); - assert_eq!(names, ["first", "second"]); + assert_eq!(names(&chain), ["first", "second"]); } #[test] @@ -386,12 +474,57 @@ Host internal let target = jumping("internal", "10.0.0.3", "inner"); let chain = resolve_chain(&target, &known).unwrap(); - let names: Vec<&str> = chain.iter().map(|h| h.name.as_str()).collect(); - assert_eq!(names, ["outer", "inner"]); + assert_eq!(names(&chain), ["outer", "inner"]); // Flattened: the caller connects hop by hop and must not re-expand. assert!(chain.iter().all(|h| h.proxy_jump.is_none())); } + #[test] + fn only_the_first_hop_of_a_list_expands_its_own_bastion() { + // `ssh -J first,second` reaches `second` with the jump list on the + // command line, which outranks `second`'s configured ProxyJump. + let known = vec![ + host("edge", "10.0.0.1"), + host("first", "10.0.0.2"), + jumping("second", "10.0.0.3", "edge"), + ]; + let target = jumping("internal", "10.0.0.4", "first,second"); + assert_eq!( + names(&resolve_chain(&target, &known).unwrap()), + ["first", "second"] + ); + + // …but the first hop's own bastion still applies. + let target = jumping("internal", "10.0.0.4", "second,first"); + assert_eq!( + names(&resolve_chain(&target, &known).unwrap()), + ["edge", "second", "first"] + ); + } + + #[test] + fn a_hop_already_in_the_chain_is_not_a_cycle() { + // `edge` is both a hop of the list and `inner`'s own bastion. It is + // already connected by the time it comes round again — a duplicate to + // skip, not a loop to refuse. + let known = vec![ + host("edge", "10.0.0.1"), + jumping("inner", "10.0.0.2", "edge"), + ]; + + let target = jumping("internal", "10.0.0.3", "inner,edge"); + assert_eq!( + names(&resolve_chain(&target, &known).unwrap()), + ["edge", "inner"] + ); + + let target = jumping("internal", "10.0.0.3", "edge,inner"); + assert_eq!( + names(&resolve_chain(&target, &known).unwrap()), + ["edge", "inner"] + ); + } + #[test] fn a_self_referencing_jump_is_rejected() { let known = vec![jumping("loop", "10.0.0.1", "loop")]; @@ -402,7 +535,7 @@ Host internal #[test] fn two_aliases_for_one_bastion_are_rejected() { - // Same machine, different name: still a loop, just a less obvious one. + // Same machine, different name: reaching it would require reaching it. let known = vec![ jumping("proxy-a", "10.0.0.1", "proxy-b"), host("proxy-b", "10.0.0.1"), @@ -419,14 +552,40 @@ Host internal } #[test] - fn an_over_long_chain_is_rejected() { - let hops: Vec = (0..MAX_HOPS + 1).map(|i| format!("h{i}")).collect(); - let known: Vec = hops - .iter() - .enumerate() - .map(|(i, name)| host(name, &format!("10.0.0.{i}"))) + fn the_hop_limit_is_the_boundary_it_claims() { + let chain_of = |count: usize| { + let hops: Vec = (0..count).map(|i| format!("h{i}")).collect(); + let known: Vec = hops + .iter() + .enumerate() + .map(|(i, name)| host(name, &format!("10.0.0.{i}"))) + .collect(); + let target = jumping("internal", "10.1.0.1", &hops.join(",")); + resolve_chain(&target, &known) + }; + + assert_eq!(chain_of(MAX_HOPS).unwrap().len(), MAX_HOPS); + assert!(chain_of(MAX_HOPS + 1).is_err()); + } + + #[test] + fn a_deeply_nested_chain_is_rejected_before_it_recurses_away() { + // Each entry jumps through the next, so the walk descends without ever + // appending to the chain — the depth bound is what has to catch it. + let known: Vec = (0..MAX_HOPS * 4) + .map(|i| { + jumping( + &format!("h{i}"), + &format!("10.0.0.{i}"), + &format!("h{}", i + 1), + ) + }) .collect(); - let target = jumping("internal", "10.1.0.1", &hops.join(",")); - assert!(resolve_chain(&target, &known).is_err()); + let target = jumping("internal", "10.1.0.1", "h0"); + + // The depth bound has to be what stops it: the chain-length bound reads + // a chain that is still empty this far down. + let err = resolve_chain(&target, &known).unwrap_err().to_string(); + assert!(err.contains("nested deeper"), "unexpected error: {err}"); } } From 3102ee40323edf94a10d5c3ecf2724469cc3eba0 Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 11:33:02 +0400 Subject: [PATCH 3/6] fix(ssh): bound and fail closed on every ProxyJump hop --- crates/omnyssh-core/src/ssh/session.rs | 74 +++++++++++++++++++------- 1 file changed, 55 insertions(+), 19 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/session.rs b/crates/omnyssh-core/src/ssh/session.rs index 25dc131..fbec114 100644 --- a/crates/omnyssh-core/src/ssh/session.rs +++ b/crates/omnyssh-core/src/ssh/session.rs @@ -109,10 +109,12 @@ impl client::Handler for KnownHostsHandler { /// An authenticated russh connection to one host, plus the jump-host /// connections it is tunnelled through (empty for a direct connection). /// -/// The jump handles are held for the whole lifetime of the connection: each one -/// carries the `direct-tcpip` channel of the hop above it, so dropping a bastion -/// handle would tear down everything beyond it. Derefs to the target's -/// [`Handle`], so callers open channels on it exactly as before. +/// The bastion handles are owned for the whole lifetime of the connection so +/// the chain outlives nothing it carries. Teardown runs the other way: the +/// target's session task holds the `direct-tcpip` stream of the hop below it, +/// so dropping this struct closes the target first and cascades outward. +/// Derefs to the target's [`Handle`], so callers open channels on it exactly as +/// before. pub(crate) struct SshConnection { handle: Handle, /// Bastions, nearest-first. Never used directly — kept alive by ownership. @@ -278,15 +280,34 @@ pub(crate) async fn connect_and_auth(host: &Host) -> anyhow::Result connect_direct(&config, host).await?, - Some(via) => connect_tunnelled(&config, via, host).await?, - }; + None => connect_direct(&config, host).await, + Some(via) => connect_tunnelled(&config, via, host).await, + } + .with_context(|| match chain.last() { + None => format!("connecting to '{}' failed", host.name), + Some(last) => format!("connecting to '{}' via '{}' failed", host.name, last.name), + })?; + Ok(SshConnection { handle, _jumps: jumps, }) } +/// Wall-clock budget one [`SshSession::connect`] needs for `host`: the per-hop +/// connect timeout once for every bastion in its `ProxyJump` chain, plus the +/// target. +/// +/// Callers that wrap the connect in a timeout of their own must scale it by +/// this — a fixed budget trips on a bastion chain before the connection has had +/// the time [`connect_and_auth`] is entitled to. +pub(crate) async fn connect_budget(host: &Host) -> Duration { + // A chain that fails to resolve costs nothing to connect; the caller's own + // attempt reports why. + let hops = jump_chain(host).await.map_or(0, |chain| chain.len()); + CONNECT_TIMEOUT * (hops as u32 + 1) +} + /// The shared russh client configuration (timeouts + keepalives). fn client_config() -> Arc { Arc::new(client::Config { @@ -302,21 +323,29 @@ fn client_config() -> Arc { /// The jump aliases are looked up in the merged host list, so a bastion defined /// elsewhere in `~/.ssh/config` (or in `hosts.toml`) contributes its own /// HostName/User/Port/IdentityFile. Loading is skipped entirely for the common -/// no-`ProxyJump` case, and an unreadable host list degrades to treating the -/// alias as a literal hostname rather than failing the connection. +/// no-`ProxyJump` case. +/// +/// # Errors +/// An unreadable host list, or a chain that cannot be resolved. Both fail the +/// connection: resolving a bastion alias against nothing would fall back to +/// dialling the alias as a hostname, which is a different machine. async fn jump_chain(host: &Host) -> anyhow::Result> { - if host.proxy_jump.is_none() { + if crate::ssh::jump::jump_value(host).is_none() { return Ok(Vec::new()); } // load_all_hosts() is blocking file I/O — keep it off the async worker. let known = tokio::task::spawn_blocking(crate::config::load_all_hosts) .await .context("host list load panicked")? - .unwrap_or_else(|e| { - tracing::warn!(error = %e, "could not load hosts for ProxyJump resolution"); - Vec::new() - }); - crate::ssh::jump::resolve_chain(host, &known) + .context("could not load hosts for ProxyJump resolution")?; + + let chain = crate::ssh::jump::resolve_chain(host, &known)?; + tracing::debug!( + host = %host.name, + via = %chain.iter().map(|h| h.name.as_str()).collect::>().join(" -> "), + "resolved ProxyJump chain" + ); + Ok(chain) } /// Opens a TCP connection to `host` and authenticates. @@ -346,10 +375,17 @@ async fn connect_tunnelled( ) -> anyhow::Result> { // The originator address is informational; ssh(1) reports the loopback it // forwards from, and servers only log it. - let channel = via - .channel_open_direct_tcpip(host.hostname.clone(), host.port as u32, "127.0.0.1", 0) - .await - .with_context(|| format!("open tunnel to {}:{}", host.hostname, host.port))?; + // + // Timed out like the handshake it precedes: the bastion answers only once + // its own connect() to the target resolves, so a firewalled target would + // otherwise park the caller for the bastion's whole SYN budget. + let channel = time::timeout( + CONNECT_TIMEOUT, + via.channel_open_direct_tcpip(host.hostname.clone(), host.port as u32, "127.0.0.1", 0), + ) + .await + .map_err(|_| anyhow!("SSH connection timed out (10 s)"))? + .with_context(|| format!("open tunnel to {}:{}", host.hostname, host.port))?; let handle = time::timeout( CONNECT_TIMEOUT, From 59ab121e8ff47829abcfecf966fdb3229c0f525f Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 11:33:02 +0400 Subject: [PATCH 4/6] fix(key-setup): size the verification budget for a jump chain --- crates/omnyssh-core/src/ssh/key_setup.rs | 85 +++++++++++++++--------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/key_setup.rs b/crates/omnyssh-core/src/ssh/key_setup.rs index ae412d0..95dda30 100644 --- a/crates/omnyssh-core/src/ssh/key_setup.rs +++ b/crates/omnyssh-core/src/ssh/key_setup.rs @@ -19,7 +19,7 @@ use tokio::time; use tracing::{error, info, warn}; use crate::ssh::client::Host; -use crate::ssh::session::SshSession; +use crate::ssh::session::{self, SshSession}; // --------------------------------------------------------------------------- // Constants @@ -524,49 +524,69 @@ pub async fn setup_key_for_host( error_message: None, }; - // Wrap the entire process in a timeout. - match time::timeout( - TOTAL_TIMEOUT, - setup_key_internal(host, password_session, key_type, &mut machine, progress_tx), + // The two verification steps reconnect, so they walk the host's ProxyJump + // chain like every other connect. A fixed budget would trip on a bastion + // before the connection had the time the engine grants it per hop. + let verify_timeout = STEP_TIMEOUT.max(session::connect_budget(host).await); + let total_timeout = TOTAL_TIMEOUT + verify_timeout.saturating_sub(STEP_TIMEOUT) * 2; + + let error = match time::timeout( + total_timeout, + setup_key_internal( + host, + password_session, + key_type, + verify_timeout, + &mut machine, + progress_tx, + ), ) .await { Ok(Ok(key_path)) => { result.key_path = key_path; result.state = machine.state().clone(); - Ok(result) + return Ok(result); } Ok(Err(e)) => { error!("Key setup failed for {}: {}", host.name, e); - result.state = machine.state().clone(); - result.error_message = Some(format!("{:#}", e)); - - // Attempt rollback if needed. - if matches!(machine.state(), KeySetupState::NeedsRollback) { - if let Err(rollback_err) = emergency_rollback(password_session).await { - error!("Rollback failed: {}", rollback_err); - result.error_message = Some(format!( - "Setup failed AND rollback failed: {}\nRollback error: {}", - e, rollback_err - )); - } else { - machine.rollback_complete(); - result.state = KeySetupState::RolledBack; - } - } - - Err(e) + e } Err(_) => { - let err = anyhow!( + // Running out of time is only safe before the point of no return. + // Past it the server has password auth disabled, so the run needs + // the same rollback a failed final check would get. + let step = if machine.password_disabled { + KeySetupStep::FinalCheck + } else { + KeySetupStep::VerifyKeyAuth + }; + machine.step_result(step, Err(anyhow!("timed out"))); + anyhow!( "Key setup timed out after {} seconds", - TOTAL_TIMEOUT.as_secs() - ); - result.error_message = Some(err.to_string()); - result.state = KeySetupState::FailedSafe; - Err(err) + total_timeout.as_secs() + ) + } + }; + + result.state = machine.state().clone(); + result.error_message = Some(format!("{:#}", error)); + + // Attempt rollback if needed. + if matches!(machine.state(), KeySetupState::NeedsRollback) { + if let Err(rollback_err) = emergency_rollback(password_session).await { + error!("Rollback failed: {}", rollback_err); + result.error_message = Some(format!( + "Setup failed AND rollback failed: {}\nRollback error: {}", + error, rollback_err + )); + } else { + machine.rollback_complete(); + result.state = KeySetupState::RolledBack; } } + + Err(error) } /// Internal implementation of the key setup process. @@ -574,6 +594,7 @@ async fn setup_key_internal( host: &Host, password_session: &SshSession, key_type: KeyType, + verify_timeout: Duration, machine: &mut KeySetupMachine, progress_tx: Option>, ) -> Result { @@ -641,7 +662,7 @@ async fn setup_key_internal( test_host.identity_file = Some(private_key_path.to_string_lossy().to_string()); test_host.password = None; // Force key-only auth. - match time::timeout(STEP_TIMEOUT, SshSession::connect(&test_host)).await { + match time::timeout(verify_timeout, SshSession::connect(&test_host)).await { Ok(Ok(test_session)) => { info!("Key authentication verified successfully!"); test_session.disconnect().await; @@ -742,7 +763,7 @@ async fn setup_key_internal( if let Some(ref tx) = progress_tx { let _ = tx.send(KeySetupStep::FinalCheck).await; } - match time::timeout(STEP_TIMEOUT, SshSession::connect(&test_host)).await { + match time::timeout(verify_timeout, SshSession::connect(&test_host)).await { Ok(Ok(final_session)) => { info!("Final verification passed! Key setup complete."); final_session.disconnect().await; From fd91d21286684c022582fb392be2c80b14d19d3e Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 11:33:02 +0400 Subject: [PATCH 5/6] docs(changelog): note the corrected ProxyJump behaviour --- CHANGELOG.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1869afa..d0a4294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,11 @@ Versions follow [Semantic Versioning](https://semver.org/). ### Bug Fixes - **`ProxyJump` hosts now connect through their bastion.** A host with `ProxyJump` in `~/.ssh/config` was parsed but never routed: the terminal refused to open it, while metrics, SFTP, snippets and key setup quietly dialled the target address direct — which for an internal host meant every connection failed or, worse, landed somewhere else on that address. Every native SSH path now walks the jump chain the way `ssh -J` does, connecting and authenticating each bastion in turn and tunnelling the next hop over it. - The jump alias is resolved against your host list, so `ProxyJump public-proxy` picks up that entry's `HostName`, `User`, `Port` and `IdentityFile`. An alias that matches no entry is used as a literal hostname. - - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does. Chains that loop, or run past ten hops, are reported instead of hanging. - - Each hop's host key is checked against `known_hosts` under its own name, and each hop authenticates with the usual agent → identity file → default keys → password order. + - Multi-hop values (`ProxyJump first,second`), inline `user@host:port` overrides, IPv6 literals, bastions that are themselves behind a bastion, and the `ProxyJump none` opt-out all behave as OpenSSH does — including its precedence rule that only the first hop of a list contributes bastions of its own. Chains that loop, or run past ten hops, are reported instead of hanging. + - A `ProxyJump` that cannot be resolved — a malformed hop, an unreadable host list, a cycle — fails the connection. It never falls back to dialling the target address, which is the very thing that made the old behaviour dangerous. + - Each hop's host key is checked against `known_hosts` under its own name, and each hop authenticates with the usual agent → identity file → default keys → password order. Every hop, including the tunnel opened on a bastion, is bound by the same ten-second budget, so a firewalled target cannot leave a host stuck on "connecting". + - A bastion you renamed after importing it is still found by the alias other entries name it with. + - One-click SSH key setup works for hosts behind a bastion: its verification steps now get the time the longer connection needs, and running out of time after password authentication has been disabled rolls the server back instead of reporting a clean failure. - Editing an imported host in the TUI no longer drops its `ProxyJump`: the form has no field for it, so the saved copy used to lose the bastion. The GUI already preserved it. --- From 3d4a2c24c370bebabe5cf0fc288a5d60b4a2174d Mon Sep 17 00:00:00 2001 From: Tim Hartmann Date: Tue, 18 Aug 2026 11:55:22 +0400 Subject: [PATCH 6/6] fix(ssh): keep a repeated ProxyJump hop in its list position --- crates/omnyssh-core/src/ssh/jump.rs | 40 +++++++++++++++++++---------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/crates/omnyssh-core/src/ssh/jump.rs b/crates/omnyssh-core/src/ssh/jump.rs index bf9b6e0..d41dfea 100644 --- a/crates/omnyssh-core/src/ssh/jump.rs +++ b/crates/omnyssh-core/src/ssh/jump.rs @@ -97,12 +97,6 @@ impl Walk { if self.active.iter().any(|h| same_hop(h, &host)) { bail!("ProxyJump cycle detected at '{}'", host.name); } - // Already reached earlier in the chain: connecting it a second time - // would add a pointless hop, not close a loop. - if self.chain.iter().any(|h| same_hop(h, &host)) { - continue; - } - let nested = if index == 0 { jump_value(&host) } else { None }; if let Some(nested) = nested { if self.active.len() > MAX_HOPS { @@ -125,7 +119,8 @@ impl Walk { } /// Whether two hops are the same machine — the same alias, or the same -/// endpoint reached under a second name. +/// endpoint reached under a second name. Only ever asked of hops still being +/// expanded, so a match is a back-edge, not a repetition. fn same_hop(a: &Host, b: &Host) -> bool { a.name == b.name || (a.user == b.user && a.hostname == b.hostname && a.port == b.port) } @@ -137,9 +132,12 @@ fn same_hop(a: &Host, b: &Host) -> bool { /// `:port` in the spec always wins over the inherited value. fn resolve_hop(spec: &JumpSpec, known: &[Host]) -> Host { // A host imported from `~/.ssh/config` and then renamed keeps its original - // alias, which is still what every other entry's `ProxyJump` names. - let entry = known.iter().find(|h| { - h.name == spec.host || h.original_ssh_host.as_deref() == Some(spec.host.as_str()) + // alias, which is still what every other entry's `ProxyJump` names — but an + // entry that carries the alias as its own name comes first. + let entry = known.iter().find(|h| h.name == spec.host).or_else(|| { + known + .iter() + .find(|h| h.original_ssh_host.as_deref() == Some(spec.host.as_str())) }); let mut host = match entry { @@ -418,6 +416,19 @@ Host internal resolve_chain(&target, &known).unwrap()[0].hostname, "proxy.example.com" ); + + // An entry that owns the alias outright wins over one that used to. + let known = vec![ + Host { + original_ssh_host: Some("public-proxy".into()), + ..host("Prod Bastion", "renamed.example.com") + }, + host("public-proxy", "proxy.example.com"), + ]; + assert_eq!( + resolve_chain(&target, &known).unwrap()[0].hostname, + "proxy.example.com" + ); } #[test] @@ -503,10 +514,11 @@ Host internal } #[test] - fn a_hop_already_in_the_chain_is_not_a_cycle() { + fn a_bastion_named_twice_is_not_a_cycle() { // `edge` is both a hop of the list and `inner`'s own bastion. It is - // already connected by the time it comes round again — a duplicate to - // skip, not a loop to refuse. + // already connected by the time it comes round again — a repetition, + // not a loop to refuse. `ssh` reaches the last hop of a list last, so + // the second mention keeps its place rather than being dropped. let known = vec![ host("edge", "10.0.0.1"), jumping("inner", "10.0.0.2", "edge"), @@ -515,7 +527,7 @@ Host internal let target = jumping("internal", "10.0.0.3", "inner,edge"); assert_eq!( names(&resolve_chain(&target, &known).unwrap()), - ["edge", "inner"] + ["edge", "inner", "edge"] ); let target = jumping("internal", "10.0.0.3", "edge,inner");