diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f761bb..d0a4294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ 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 — 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. + +--- + ## 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..d41dfea --- /dev/null +++ b/crates/omnyssh-core/src/ssh/jump.rs @@ -0,0 +1,603 @@ +//! `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. +//! +//! 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 anyhow::{bail, Context}; + +use crate::ssh::client::Host; + +/// 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]`. +#[derive(Debug, PartialEq)] +struct JumpSpec { + user: Option, + host: String, + 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 +/// 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 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 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". +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 + } else { + Some(value) + } +} + +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); + } + 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); + } + Ok(()) + } +} + +/// Whether two hops are the same machine — the same alias, or the same +/// 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) +} + +/// 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 { + // A host imported from `~/.ssh/config` and then renamed keeps its original + // 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 { + Some(h) => h.clone(), + 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(); + } + 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 +} + +/// Splits a `ProxyJump` value into its comma-separated hops, nearest first. +/// +/// # 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) -> anyhow::Result { + let hop = hop.trim(); + if hop.is_empty() { + bail!("empty ProxyJump hop"); + } + + // 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).with_context(|| format!("unusable ProxyJump hop '{hop}'"))?; + if host.is_empty() { + bail!("ProxyJump hop '{hop}' has no host"); + } + Ok(JumpSpec { + user, + host: host.to_string(), + port, + }) +} + +/// Splits `host`, `host:port`, `[v6]` or `[v6]:port` into its two parts. +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]; + 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(':') => 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), + } +} + +// --------------------------------------------------------------------------- +// 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) + } + } + + 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").unwrap(), + 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").unwrap(), + 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").unwrap(); + 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").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").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 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 -------------------------------------------------- + + #[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 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 + // 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 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" + ); + + // 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] + 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(); + assert_eq!(names(&chain), ["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(); + 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_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 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"), + ]; + + let target = jumping("internal", "10.0.0.3", "inner,edge"); + assert_eq!( + names(&resolve_chain(&target, &known).unwrap()), + ["edge", "inner", "edge"] + ); + + 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")]; + 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: 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"), + ]; + 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 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", "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}"); + } +} 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; 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..fbec114 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,33 @@ 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 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. + _jumps: Vec>, +} + +impl std::ops::Deref for SshConnection { + type Target = Handle; + + fn deref(&self) -> &Self::Target { + &self.handle + } +} + // --------------------------------------------------------------------------- // SshSession // --------------------------------------------------------------------------- @@ -110,7 +141,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 +153,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 +250,173 @@ 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, + } + .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 { 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. +/// +/// # 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 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")? + .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. +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. + // + // 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, + 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();