diff options
| author | Can Celik <ogulcancelik@gmail.com> | 2026-09-22 19:12:29 +0300 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2026-09-22 19:12:29 +0300 |
| commit | 1c6397b1cddb288099ed212a5979732698bd72c2 (patch) | |
| tree | 0574b428903ea1ce106bc7ba36e213c0418aa9f4 /src | |
| parent | 7237703dc59102cf3097e833eb896b314007b548 (diff) | |
fix: reuse ssh authentication and add machine recovery commands (#4490)
* fix: reuse ssh authentication and add in-app mfa recovery
refs #3763
* refactor: move ssh recovery to machine cli
refs #3763
Diffstat (limited to 'src')
| -rw-r--r-- | src/cli/machine.rs | 113 | ||||
| -rw-r--r-- | src/cli/spec/machine.rs | 11 | ||||
| -rw-r--r-- | src/cli/target.rs | 2 | ||||
| -rw-r--r-- | src/client/endpoint/supervisor.rs | 25 | ||||
| -rw-r--r-- | src/client/mod.rs | 1 | ||||
| -rw-r--r-- | src/client/shell.rs | 1 | ||||
| -rw-r--r-- | src/client/shell/composition.rs | 11 | ||||
| -rw-r--r-- | src/client/shell/endpoint_sidebar.rs | 43 | ||||
| -rw-r--r-- | src/client/shell/endpoints.rs | 8 | ||||
| -rw-r--r-- | src/client/shell/input.rs | 3 | ||||
| -rw-r--r-- | src/client/shell/machine_diagnostics.rs | 130 | ||||
| -rw-r--r-- | src/client/shell/render.rs | 1 | ||||
| -rw-r--r-- | src/client/shell/state.rs | 2 | ||||
| -rw-r--r-- | src/client/shell/tests/endpoints.rs | 39 | ||||
| -rw-r--r-- | src/client/shell_runtime.rs | 1 | ||||
| -rw-r--r-- | src/platform/mod.rs | 15 | ||||
| -rw-r--r-- | src/platform/unix_common.rs | 136 | ||||
| -rw-r--r-- | src/remote/attach.rs | 340 |
18 files changed, 810 insertions, 72 deletions
diff --git a/src/cli/machine.rs b/src/cli/machine.rs index 53404c03..9328347e 100644 --- a/src/cli/machine.rs +++ b/src/cli/machine.rs @@ -4,6 +4,8 @@ use crate::client::endpoint::{EndpointCatalog, ProfileId}; const HELP: &str = "Usage: herdr machine list [--json] + herdr machine status [<label-or-id>] [--json] + herdr machine reconnect <label-or-id> herdr machine add <ssh-target> --label <label> [--remote-session <name>] herdr machine rename <profile-id> --label <label> herdr machine remove <profile-id> @@ -30,6 +32,8 @@ struct MachineListRow<'a> { pub(super) fn run_machine_command(args: &[String]) -> std::io::Result<i32> { match args.first().map(String::as_str) { Some("list") => list(&args[1..]), + Some("status") => status(&args[1..]), + Some("reconnect") => reconnect(&args[1..]), Some("add") => add(&args[1..]), Some("rename") => rename(&args[1..]), Some("remove") => remove(&args[1..]), @@ -89,6 +93,115 @@ fn list(args: &[String]) -> std::io::Result<i32> { Ok(0) } +#[derive(Serialize)] +struct MachineStatusRow<'a> { + id: &'a str, + label: &'a str, + status: &'static str, + error: Option<String>, +} + +fn status(args: &[String]) -> std::io::Result<i32> { + let mut json = false; + let mut selector = None; + for arg in args { + if arg == "--json" && !json { + json = true; + } else if !arg.starts_with('-') && selector.is_none() { + selector = Some(arg.as_str()); + } else { + eprintln!("usage: herdr machine status [<label-or-id>] [--json]"); + return Ok(2); + } + } + let catalog = load_catalog()?; + let profiles = match selector { + Some(selector) => match super::target::resolve_machine(&catalog.ssh, selector) { + Ok(profile) => vec![profile], + Err(error) => { + eprintln!("{error}"); + return Ok(2); + } + }, + None => catalog.ssh.iter().collect(), + }; + let rows = profiles + .into_iter() + .map(|profile| { + let (status, error) = if !profile.enabled { + ("disabled", None) + } else { + match crate::remote::check_saved_ssh(&profile.target, &profile.session) { + Ok(()) => ("reachable", None), + Err(error) => { + let message = error.to_string(); + let status = if crate::remote::ssh_error_requires_authentication(&message) { + "auth required" + } else { + "error" + }; + (status, Some(message)) + } + } + }; + MachineStatusRow { + id: profile.id.as_str(), + label: &profile.label, + status, + error, + } + }) + .collect::<Vec<_>>(); + if json { + println!( + "{}", + serde_json::to_string_pretty(&rows).map_err(std::io::Error::other)? + ); + } else { + for row in &rows { + println!("{}\t{}\t{}", row.id, row.label, row.status); + if let Some(error) = &row.error { + println!(" {}", error.escape_debug()); + } + } + if rows.is_empty() { + println!("No saved SSH machines."); + } + } + Ok(i32::from(rows.iter().any(|row| row.error.is_some()))) +} + +fn reconnect(args: &[String]) -> std::io::Result<i32> { + use std::io::IsTerminal; + let [selector] = args else { + eprintln!("usage: herdr machine reconnect <label-or-id>"); + return Ok(2); + }; + let catalog = load_catalog()?; + let profile = match super::target::resolve_machine(&catalog.ssh, selector) { + Ok(profile) => profile, + Err(error) => { + eprintln!("{error}"); + return Ok(2); + } + }; + if !std::io::stdin().is_terminal() { + eprintln!("reconnect requires an interactive terminal; use herdr machine status for noninteractive checks"); + return Ok(2); + } + let mut authentication = crate::remote::ssh_authentication_command(&profile.target)?; + if !authentication.command.status()?.success() { + eprintln!("SSH authentication failed; the saved machine was not changed."); + return Ok(1); + } + crate::remote::check_saved_ssh(&profile.target, &profile.session)?; + println!( + "Machine {} is reachable. Open Herdr clients retry within 30 seconds.", + profile.id + ); + Ok(0) +} + #[derive(Debug, PartialEq, Eq)] struct AddArgs { target: String, diff --git a/src/cli/spec/machine.rs b/src/cli/spec/machine.rs index 188e27b9..82bc4c5d 100644 --- a/src/cli/spec/machine.rs +++ b/src/cli/spec/machine.rs @@ -11,6 +11,17 @@ pub(super) fn command() -> Command { .arg(json_flag()), ) .subcommand( + Command::new("status") + .about("Check saved machines without prompting for authentication") + .arg(Arg::new("machine").value_name("LABEL_OR_ID")) + .arg(json_flag()), + ) + .subcommand( + Command::new("reconnect") + .about("Authenticate a saved machine in this terminal and verify connectivity") + .arg(Arg::new("machine").value_name("LABEL_OR_ID").required(true)), + ) + .subcommand( Command::new("add") .about("Prepare the remote Herdr server and save an SSH machine") .arg( diff --git a/src/cli/target.rs b/src/cli/target.rs index 220410f9..9c8b9c80 100644 --- a/src/cli/target.rs +++ b/src/cli/target.rs @@ -257,7 +257,7 @@ fn parse_machine_prefix(args: &[String]) -> Result<Option<(String, Vec<String>)> Ok(Some((machine, cleaned))) } -fn resolve_machine<'a>( +pub(super) fn resolve_machine<'a>( profiles: &'a [SavedSshEndpoint], selector: &str, ) -> Result<&'a SavedSshEndpoint, String> { diff --git a/src/client/endpoint/supervisor.rs b/src/client/endpoint/supervisor.rs index a5154156..ff86ac77 100644 --- a/src/client/endpoint/supervisor.rs +++ b/src/client/endpoint/supervisor.rs @@ -210,7 +210,13 @@ impl EndpointSupervisors { state.online_since.get_or_insert(now); state.next_attempt = None; } - ClientEndpointStatus::Attention | ClientEndpointStatus::Disabled => { + ClientEndpointStatus::Attention => { + state.online_since = None; + // Authentication or configuration may be repaired outside this client. + state.next_attempt = + (!endpoint_id.is_local()).then_some(now + Duration::from_secs(30)); + } + ClientEndpointStatus::Disabled => { state.online_since = None; state.next_attempt = None; } @@ -278,11 +284,11 @@ fn connect_once( (stream, Box::new(())) } ConnectTarget::Ssh(profile) => { - let connected = crate::remote::connect_saved_ssh(profile.id.as_str(), &profile.target, &profile.session).map_err(|error| { - if failure_needs_attention(&error) { - std::io::Error::new(error.kind(), format!("{error}. Run `{}` interactively to approve setup, then restart this client", crate::remote::saved_ssh_bootstrap_command(&profile.target, &profile.session))) - } else { error } - })?; + let connected = crate::remote::connect_saved_ssh( + profile.id.as_str(), + &profile.target, + &profile.session, + )?; (connected.stream, Box::new(connected.bridge)) } }; @@ -517,7 +523,7 @@ mod tests { } #[test] - fn ssh_recovery_rejects_stale_generations_and_stops_retries_for_attention() { + fn ssh_recovery_rejects_stale_generations_and_rechecks_attention() { let now = Instant::now(); let mut supervisors = EndpointSupervisors::new(&[profile()], now); let endpoint_id = ClientEndpointId::Ssh(profile().id); @@ -535,6 +541,9 @@ mod tests { Some(now + INITIAL_RETRY_DELAY) ); assert!(supervisors.record_status(&endpoint_id, 4, ClientEndpointStatus::Attention, now)); - assert!(supervisors.endpoints[&endpoint_id].next_attempt.is_none()); + assert_eq!( + supervisors.endpoints[&endpoint_id].next_attempt, + Some(now + Duration::from_secs(30)) + ); } } diff --git a/src/client/mod.rs b/src/client/mod.rs index 68e569f9..9ad51a36 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -1184,6 +1184,7 @@ async fn run_client_loop( } let unavailable = state.shell.as_mut().and_then(|shell| { shell.set_endpoint_status(&endpoint_id, status); + shell.set_machine_diagnostic(&endpoint_id, message.clone()); (status == endpoint::ClientEndpointStatus::Attention && shell.endpoint_is_active(&endpoint_id)) .then(|| format!("{}: {message}", shell.endpoint_label(&endpoint_id))) diff --git a/src/client/shell.rs b/src/client/shell.rs index 27d30df9..2d180443 100644 --- a/src/client/shell.rs +++ b/src/client/shell.rs @@ -3,6 +3,7 @@ use std::collections::{HashMap, HashSet, VecDeque}; mod actions; mod agent_sidebar; mod aggregate_navigation; +mod machine_diagnostics; mod workspace_navigation; use workspace_navigation::{PendingWorkspaceHighlight, WorkspaceNavigationTarget}; mod composition; diff --git a/src/client/shell/composition.rs b/src/client/shell/composition.rs index 4da21e77..9b710dd5 100644 --- a/src/client/shell/composition.rs +++ b/src/client/shell/composition.rs @@ -55,6 +55,7 @@ impl ClientShellState { == Some(ClientEndpointStatus::Online) }); let mut render_state = render::ShellRenderState { + machine_diagnostics: &self.machine_diagnostics, endpoints: &self.endpoints, active_endpoint_id: &self.active_endpoint_id, collapsed_endpoints: &self.collapsed_endpoints, @@ -134,6 +135,15 @@ impl ClientShellState { &self.config.keybinds, &self.config.palette, ); + if let Some(notice) = &self.visible_endpoint_notice { + self.hits.notification_toast = endpoint_notices::render_notice( + &mut buffer, + Rect::new(0, 0, cols, rows), + notice, + 1, + &self.config.palette, + ); + } FrameData::from_ratatui_buffer_with_hyperlinks(&buffer, None, &[]) } @@ -197,6 +207,7 @@ impl ClientShellState { snapshot, &self.config, render::ShellRenderState { + machine_diagnostics: &self.machine_diagnostics, endpoints: &self.endpoints, active_endpoint_id: &self.active_endpoint_id, collapsed_endpoints: &self.collapsed_endpoints, diff --git a/src/client/shell/endpoint_sidebar.rs b/src/client/shell/endpoint_sidebar.rs index c4f701c3..96982487 100644 --- a/src/client/shell/endpoint_sidebar.rs +++ b/src/client/shell/endpoint_sidebar.rs @@ -90,12 +90,26 @@ pub(super) fn render_collapsed( palette.overlay0 }), ); + let mut status_badge = Rect::default(); if !endpoint.endpoint_id.is_local() { let (glyph, _, color) = endpoint_status_presentation(endpoint.status, palette); - put_right_text(buffer, rect, rect.y, glyph, Style::default().fg(color)); + let width = display_width(glyph).min(rect.width); + status_badge = Rect::new(rect.right().saturating_sub(width), rect.y, width, 1); + put_right_text( + buffer, + rect, + rect.y, + glyph, + state.machine_diagnostics.badge_style( + endpoint, + palette, + Style::default().fg(color), + ), + ); } hits.machines.push(MachineHit { rect, + status_badge, collapse_toggle: Rect::new(rect.x, rect.y, u16::from(rect.width > 1), 1), endpoint_id: endpoint.endpoint_id.clone(), }); @@ -394,16 +408,18 @@ pub(super) fn render_expanded( let rect = Rect::new(body.x, y, content_width, 1); let collapsed = state.collapsed_endpoints.contains(&endpoint.endpoint_id); let marker = if collapsed { "▸" } else { "▾" }; - render_endpoint_row( + let status_badge = render_endpoint_row( buffer, rect, marker, endpoint, collapsed && &endpoint.endpoint_id == state.active_endpoint_id, + state.machine_diagnostics, palette, ); hits.machines.push(MachineHit { rect, + status_badge, collapse_toggle: Rect::new( rect.x.saturating_add(1), rect.y, @@ -577,8 +593,9 @@ fn render_endpoint_row( marker: &str, endpoint: &ClientShellEndpoint, highlighted: bool, + auth: &super::machine_diagnostics::MachineDiagnostics, palette: &Palette, -) { +) -> Rect { if highlighted { buffer.set_style(rect, Style::default().bg(palette.active_row_bg)); } @@ -588,7 +605,11 @@ fn render_endpoint_row( } else { state }; - let signal = if endpoint.endpoint_id.is_local() { + let signal = if auth.required_for(endpoint) { + "! auth".to_owned() + } else if endpoint.status == ClientEndpointStatus::Attention { + "! error".to_owned() + } else if endpoint.endpoint_id.is_local() { String::new() } else if state.is_empty() { glyph.to_owned() @@ -612,5 +633,17 @@ fn render_endpoint_row( ) .add_modifier(Modifier::BOLD), ); - put_right_text(buffer, rect, rect.y, &signal, Style::default().fg(color)); + put_right_text( + buffer, + rect, + rect.y, + &signal, + auth.badge_style(endpoint, palette, Style::default().fg(color)), + ); + Rect::new( + rect.right().saturating_sub(signal_width), + rect.y, + signal_width, + 1, + ) } diff --git a/src/client/shell/endpoints.rs b/src/client/shell/endpoints.rs index dd6fa3cb..fff28b71 100644 --- a/src/client/shell/endpoints.rs +++ b/src/client/shell/endpoints.rs @@ -26,6 +26,7 @@ pub(crate) struct ClientShellEndpoint { pub(super) struct MachineHit { pub(super) rect: Rect, + pub(super) status_badge: Rect, pub(super) collapse_toggle: Rect, pub(super) endpoint_id: ClientEndpointId, } @@ -108,6 +109,7 @@ impl ClientShellState { } pub(crate) fn retire_endpoint(&mut self, endpoint_id: &ClientEndpointId) { + self.clear_machine_diagnostic(endpoint_id); if endpoint_id == &self.active_endpoint_id { self.pending_workspace_highlight = None; } @@ -134,6 +136,12 @@ impl ClientShellState { endpoint_id: &ClientEndpointId, status: ClientEndpointStatus, ) { + if matches!( + status, + ClientEndpointStatus::Online | ClientEndpointStatus::Disabled + ) { + self.clear_machine_diagnostic(endpoint_id); + } if endpoint_id == &self.active_endpoint_id && status != ClientEndpointStatus::Online { self.pending_workspace_highlight = None; } diff --git a/src/client/shell/input.rs b/src/client/shell/input.rs index f1e7ea25..0e14e18c 100644 --- a/src/client/shell/input.rs +++ b/src/client/shell/input.rs @@ -163,6 +163,9 @@ impl ClientShellState { outcome.repaint = true; } for event in events { + if self.handle_machine_badge_event(&event, &mut outcome) { + continue; + } if let Some(update) = host_theme_update(&event) { push_host_theme_update(&mut outcome.requests, update); } diff --git a/src/client/shell/machine_diagnostics.rs b/src/client/shell/machine_diagnostics.rs new file mode 100644 index 00000000..67983024 --- /dev/null +++ b/src/client/shell/machine_diagnostics.rs @@ -0,0 +1,130 @@ +use super::*; +use crate::raw_input::RawInputEvent; +use crossterm::event::{MouseButton, MouseEventKind}; + +#[derive(Default)] +pub(super) struct MachineDiagnostics { + errors: HashMap<ClientEndpointId, String>, + hover: Option<ClientEndpointId>, +} + +impl MachineDiagnostics { + pub(super) fn required_for(&self, endpoint: &ClientShellEndpoint) -> bool { + self.errors + .get(&endpoint.endpoint_id) + .is_some_and(|message| crate::remote::ssh_error_requires_authentication(message)) + } + + pub(super) fn badge_style( + &self, + endpoint: &ClientShellEndpoint, + palette: &Palette, + style: Style, + ) -> Style { + if self.hover.as_ref() == Some(&endpoint.endpoint_id) + && self.errors.contains_key(&endpoint.endpoint_id) + { + style + .bg(palette.active_row_bg) + .add_modifier(Modifier::REVERSED) + } else { + style + } + } +} + +impl ClientShellState { + pub(crate) fn set_machine_diagnostic(&mut self, id: &ClientEndpointId, message: String) { + if !id.is_local() { + self.machine_diagnostics.errors.insert( + id.clone(), + message + .chars() + .filter(|c| !c.is_control() || *c == '\n') + .take(4096) + .collect(), + ); + } + } + + pub(super) fn clear_machine_diagnostic(&mut self, id: &ClientEndpointId) { + self.machine_diagnostics.errors.remove(id); + } + + pub(super) fn handle_machine_badge_event( + &mut self, + event: &RawInputEvent, + outcome: &mut ClientShellInput, + ) -> bool { + let RawInputEvent::Mouse(mouse) = event else { + return false; + }; + if self.overlay.is_some() + || self.popup_pending + || self.hits.popup.is_some() + || contains(self.hits.notification_toast, (mouse.column, mouse.row)) + { + return false; + } + let hit = self + .hits + .machines + .iter() + .find(|hit| { + contains(hit.status_badge, (mouse.column, mouse.row)) + && self + .machine_diagnostics + .errors + .contains_key(&hit.endpoint_id) + }) + .map(|hit| hit.endpoint_id.clone()); + if mouse.kind == MouseEventKind::Moved { + if self.machine_diagnostics.hover != hit { + self.machine_diagnostics.hover = hit; + outcome.repaint = true; + } + return false; + } + let Some(id) = hit else { + return false; + }; + if mouse.kind == MouseEventKind::Up(MouseButton::Left) { + return true; + } + if mouse.kind != MouseEventKind::Down(MouseButton::Left) { + return false; + } + let Some(error) = self.machine_diagnostics.errors.get(&id) else { + return true; + }; + let ClientEndpointId::Ssh(profile_id) = &id else { + return true; + }; + let command = if crate::remote::ssh_error_requires_authentication(error) { + format!("herdr machine reconnect {profile_id}") + } else { + format!("herdr machine status {profile_id}") + }; + let code = format!("machine-diagnostic:{}", profile_id); + // An explicit click can reopen its diagnostic, but must not replace another notice. + if self + .visible_endpoint_notice + .as_ref() + .is_some_and(|notice| notice.key.code != code) + { + return true; + } + self.visible_endpoint_notice = Some(ClientVisibleEndpointNotice { + key: ClientEndpointNoticeKey { + boot_id: "machine".into(), + kind: ClientEndpointNoticeKind::Unavailable, + code, + }, + title: format!("{}: {command}", self.endpoint_label(&id)), + body: error.clone(), + deadline: std::time::Instant::now() + std::time::Duration::from_secs(15), + }); + outcome.repaint = true; + true + } +} diff --git a/src/client/shell/render.rs b/src/client/shell/render.rs index 80387019..82cb2ac4 100644 --- a/src/client/shell/render.rs +++ b/src/client/shell/render.rs @@ -230,6 +230,7 @@ pub(super) fn render_mode_bar( } pub(super) struct ShellRenderState<'a> { + pub(super) machine_diagnostics: &'a super::machine_diagnostics::MachineDiagnostics, pub(super) endpoints: &'a [ClientShellEndpoint], pub(super) active_endpoint_id: &'a ClientEndpointId, pub(super) collapsed_endpoints: &'a HashSet<ClientEndpointId>, diff --git a/src/client/shell/state.rs b/src/client/shell/state.rs index 2eaf18bf..0d6937e0 100644 --- a/src/client/shell/state.rs +++ b/src/client/shell/state.rs @@ -844,6 +844,7 @@ pub(super) struct ClientCopyModeState { } pub(crate) struct ClientShellState { + pub(super) machine_diagnostics: super::machine_diagnostics::MachineDiagnostics, pub(super) config: ClientShellConfig, pub(super) snapshot: Option<Box<ClientShellSnapshot>>, pub(super) active_snapshot_generation: Option<u64>, @@ -1007,6 +1008,7 @@ impl ClientShellState { .extend(saved.collapsed_groups); } Self { + machine_diagnostics: Default::default(), config, snapshot: None, active_snapshot_generation: None, diff --git a/src/client/shell/tests/endpoints.rs b/src/client/shell/tests/endpoints.rs index 4434e119..f327f8c1 100644 --- a/src/client/shell/tests/endpoints.rs +++ b/src/client/shell/tests/endpoints.rs @@ -73,6 +73,45 @@ fn state_with_remote() -> (ClientShellState, ClientEndpointId) { (state, endpoint_id) } +#[test] +fn machine_diagnostic_badge_reopens_notice_without_collapsing_machine() { + let (mut state, id) = state_with_remote(); + state.set_endpoint_status(&id, ClientEndpointStatus::Attention); + state.set_machine_diagnostic(&id, "Permission denied (keyboard-interactive)".into()); + for _ in 0..2 { + state.compose(120, 40).unwrap(); + let hit = state + .hits + .machines + .iter() + .find(|hit| hit.endpoint_id == id) + .unwrap(); + let mouse = MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column: hit.status_badge.x, + row: hit.status_badge.y, + modifiers: KeyModifiers::NONE, + }; + let outcome = state.handle_raw_events(vec![RawInputEvent::Mouse(mouse)]); + assert!(outcome.repaint); + assert!(!state.collapsed_endpoints.contains(&id)); + let notice = state.visible_endpoint_notice.take().unwrap(); + assert!(notice.body.contains("Permission denied")); + assert!(notice + .title + .contains("herdr machine reconnect 0123456789abcdef0123456789abcdef")); + } + state.set_endpoint_status(&id, ClientEndpointStatus::Online); + state.compose(120, 40).unwrap(); + assert!(!state.machine_diagnostics.required_for( + state + .endpoints + .iter() + .find(|endpoint| endpoint.endpoint_id == id) + .unwrap() + )); +} + fn state_with_scrollable_agents() -> (ClientShellState, ClientEndpointId) { let (mut state, remote) = state_with_remote(); for endpoint_id in [ClientEndpointId::Local, remote.clone()] { diff --git a/src/client/shell_runtime.rs b/src/client/shell_runtime.rs index 2e8d1c72..3843654f 100644 --- a/src/client/shell_runtime.rs +++ b/src/client/shell_runtime.rs @@ -597,6 +597,7 @@ pub(super) fn handle_endpoint_attention( shell.cancel_endpoint_request(&request_id); } shell.set_endpoint_status(endpoint_id, endpoint::ClientEndpointStatus::Attention); + shell.set_machine_diagnostic(endpoint_id, message.clone()); endpoint_was_active.then(|| format!("{}: {message}", shell.endpoint_label(endpoint_id))) }); if let Some(message) = unavailable { diff --git a/src/platform/mod.rs b/src/platform/mod.rs index 864516da..88f433bf 100644 --- a/src/platform/mod.rs +++ b/src/platform/mod.rs @@ -81,7 +81,9 @@ impl ChildExitReason { } #[cfg(unix)] -pub(crate) use unix_common::{classify_child_exit, poll_fd_readable, read_fd}; +pub(crate) use unix_common::{ + classify_child_exit, poll_fd_readable, read_fd, shared_ssh_control_path, +}; #[cfg(not(any(unix, windows)))] pub(crate) fn classify_child_exit(_status: &portable_pty::ExitStatus) -> ChildExitReason { @@ -712,3 +714,14 @@ mod tests { ); } } + +#[cfg(not(unix))] +pub(crate) fn shared_ssh_control_path( + _namespace: &std::path::Path, + _target: &str, +) -> std::io::Result<std::path::PathBuf> { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "interactive SSH recovery requires Unix OpenSSH multiplexing", + )) +} diff --git a/src/platform/unix_common.rs b/src/platform/unix_common.rs index 6063edd9..8b76212c 100644 --- a/src/platform/unix_common.rs +++ b/src/platform/unix_common.rs @@ -498,3 +498,139 @@ mod tests { assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); } } + +/// Shared OpenSSH sockets outlive individual helpers. Never adopt a directory +/// belonging to another uid, a symlink, or a directory accessible by others. +pub(crate) fn shared_ssh_control_path(namespace: &Path, target: &str) -> std::io::Result<PathBuf> { + use sha2::{Digest, Sha256}; + use std::os::unix::{ + ffi::OsStrExt, + fs::{DirBuilderExt, MetadataExt}, + }; + + // Validate the resolved system temp directory, but retain the short /tmp + // spelling for sockets. On macOS /tmp resolves to /private/tmp; those extra + // bytes would consume the space OpenSSH needs for its staging suffix. + let base = Path::new("/tmp"); + let resolved_base = std::fs::canonicalize(base)?; + let metadata = std::fs::symlink_metadata(&resolved_base)?; + if !metadata.is_dir() || metadata.uid() != 0 || metadata.mode() & 0o1000 == 0 { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "unsafe SSH control directory parent", + )); + } + let dir = base.join(format!("hssh-{}", unsafe { libc::geteuid() })); + match std::fs::DirBuilder::new().mode(0o700).create(&dir) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => return Err(error), + } + validate_shared_ssh_dir(&dir)?; + let namespace = if namespace.is_absolute() { + namespace.to_owned() + } else { + std::env::current_dir()?.join(namespace) + }; + let mut hash = Sha256::new(); + hash.update(namespace.as_os_str().as_bytes()); + hash.update([0]); + hash.update(target.as_bytes()); + // %C additionally scopes the socket to OpenSSH's resolved destination, + // port and jump host, rather than merely the spelling of an alias. + // Keep 96 bits of namespace/target hash plus OpenSSH's 160-bit %C. + let hash = format!("{:x}", hash.finalize()); + let path = dir.join(format!("{}-%C", &hash[..24])); + // OpenSSH first binds ControlPath + '.' + 16 random characters, then + // renames it. Reserve those 17 bytes, not just the final socket's length. + let expanded = path.to_string_lossy().replace("%C", &"0".repeat(40)); + let staging = PathBuf::from(format!("{expanded}.{}", "0".repeat(16))); + if !fits_unix_socket_path(&staging) { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "SSH control socket staging path exceeds the Unix socket length limit", + )); + } + Ok(path) +} + +fn validate_shared_ssh_dir(dir: &Path) -> std::io::Result<()> { + use std::os::unix::fs::MetadataExt; + let metadata = std::fs::symlink_metadata(dir)?; + if !metadata.is_dir() + || metadata.uid() != unsafe { libc::geteuid() } + || metadata.mode() & 0o7777 != 0o700 + { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "SSH control directory must be owned by the current user, mode 0700, and not a symlink", + )); + } + Ok(()) +} + +#[cfg(test)] +mod shared_ssh_tests { + use super::*; + + #[test] + fn shared_ssh_control_path_is_stable_scoped_and_bounded() { + let path = shared_ssh_control_path(Path::new("/config/one"), "user@host").unwrap(); + assert_eq!( + path, + shared_ssh_control_path(Path::new("/config/one"), "user@host").unwrap() + ); + assert_ne!( + path, + shared_ssh_control_path(Path::new("/config/two"), "user@host").unwrap() + ); + assert_ne!( + path, + shared_ssh_control_path(Path::new("/config/one"), "other@host").unwrap() + ); + let expanded = path.to_string_lossy().replace("%C", &"f".repeat(40)); + assert!(fits_unix_socket_path(&PathBuf::from(&expanded))); + // OpenSSH binds this temporary socket before renaming it to ControlPath. + assert!(fits_unix_socket_path(&PathBuf::from(format!( + "{expanded}.QuuYe7ZFE2HYeAE4" + )))); + validate_shared_ssh_dir(path.parent().unwrap()).unwrap(); + } + + #[test] + fn shared_ssh_staging_path_fits_with_maximum_uid_width() { + let path = shared_ssh_control_path(Path::new("/config/one"), "user@host").unwrap(); + let directory = path.parent().unwrap(); + let name = directory.file_name().unwrap().to_string_lossy(); + let prefix = name.trim_end_matches(|ch: char| ch.is_ascii_digit()); + let maximum_uid_directory = directory + .parent() + .unwrap() + .join(format!("{prefix}{}", u32::MAX)); + let expanded = maximum_uid_directory + .join(path.file_name().unwrap()) + .to_string_lossy() + .replace("%C", &"f".repeat(40)); + assert!(fits_unix_socket_path(&PathBuf::from(format!( + "{expanded}.QuuYe7ZFE2HYeAE4" + )))); + } + + #[test] + fn shared_ssh_directory_rejects_symlinks_and_public_modes() { + use std::os::unix::fs::{symlink, PermissionsExt}; + let dir = create_remote_ssh_config_dir("ctl").unwrap(); + let link = dir.join("link"); + symlink(&dir, &link).unwrap(); + assert_eq!( + validate_shared_ssh_dir(&link).unwrap_err().kind(), + std::io::ErrorKind::PermissionDenied + ); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert_eq!( + validate_shared_ssh_dir(&dir).unwrap_err().kind(), + std::io::ErrorKind::PermissionDenied + ); + std::fs::remove_dir_all(dir).unwrap(); + } +} diff --git a/src/remote/attach.rs b/src/remote/attach.rs index 8e0950b9..a4a00dea 100644 --- a/src/remote/attach.rs +++ b/src/remote/attach.rs @@ -86,6 +86,39 @@ pub(crate) fn run_remote(remote: RemoteLaunch) -> io::Result<()> { run_client_process(&local_socket, &reattach_command, remote.keybindings) } +pub(crate) fn check_saved_ssh(target: &str, session: &str) -> io::Result<()> { + super::validate_remote_target(target) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + crate::session::validate_name(session) + .map_err(|error| io::Error::new(io::ErrorKind::InvalidInput, error))?; + let mut ssh = RemoteSsh::new_noninteractive(target.to_owned()); + ssh.session_name = session.to_owned(); + let remote = find_installed_remote_herdr(&ssh)?; + match remote_server_status(&ssh, &remote, false)? { + RemoteServerStatus::Running { + endpoint_protocol_generation, + surface_interest, + health_check, + detached_server_daemon, + .. + } if remote_server_restart_reason( + endpoint_protocol_generation, + detached_server_daemon, + true, + surface_interest, + health_check, + ) + .is_none() => + { + Ok(()) + } + _ => Err(io::Error::other(format!( + "remote Herdr server is stopped or incompatible; run `{}`", + super::saved_ssh_bootstrap_command(target, session), + ))), + } +} + pub(crate) fn prepare_saved_ssh( target: &str, session_name: &str, @@ -554,17 +587,92 @@ pub(super) struct PreparedRemoteHerdr { pub(super) struct ManagedSshOptions { config_path: PathBuf, control_path: Option<PathBuf>, + // Bridge workers may launch SSH after the helper that created this config + // has gone away. The last options owner removes only the temporary config. + _directory: Arc<ManagedSshConfigDirectory>, } struct ManagedSshConfig { options: ManagedSshOptions, } -impl Drop for ManagedSshConfig { +struct ManagedSshConfigDirectory(PathBuf); + +impl Drop for ManagedSshConfigDirectory { fn drop(&mut self) { - if let Some(dir) = self.options.config_path.parent() { - let _ = fs::remove_dir_all(dir); - } + let _ = fs::remove_dir_all(&self.0); + } +} + +/// Classify only SSH authentication diagnostics, not transport failures or +/// unknown/changed host keys. This does not imply permission to prompt. +pub(crate) fn ssh_error_requires_authentication(message: &str) -> bool { + let message = message.to_ascii_lowercase(); + if message.contains("host key verification failed") + || message.contains("remote host identification has changed") + { + return false; + } + (message.contains("permission denied") + && ["(publickey", "(keyboard-interactive", "(password"] + .iter() + .any(|method| message.contains(method))) + || (message.contains("signing failed") + && (message.contains("sign_and_send_pubkey") || message.contains("agent"))) +} + +/// Keep this owner alive until the child has exited: OpenSSH reads its temporary +/// config after spawn. Dropping it never stops the shared authenticated master. +pub(crate) struct SshAuthenticationCommand { + pub(crate) command: Command, + _config: ManagedSshConfig, +} + +pub(crate) fn ssh_authentication_command(target: &str) -> io::Result<SshAuthenticationCommand> { + if target.is_empty() || target.starts_with('-') || target.chars().any(char::is_control) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "invalid SSH target", + )); + } + if !crate::platform::remote_ssh_config_paths().multiplexing { + return Err(io::Error::new(io::ErrorKind::Unsupported, "interactive SSH recovery requires Unix OpenSSH multiplexing; authenticate outside Herdr on this platform")); + } + if !crate::config::Config::load() + .config + .remote + .manage_ssh_config + { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + "interactive SSH recovery requires remote.manage_ssh_config=true", + )); + } + let config = write_managed_ssh_config(target)?; + Ok(authentication_command_with_config(target, config)) +} + +fn authentication_command_with_config( + target: &str, + config: ManagedSshConfig, +) -> SshAuthenticationCommand { + let mut command = Command::new("ssh"); + apply_managed_ssh_options(&mut command, Some(&config.options)); + command + .env("SSH_ASKPASS_REQUIRE", "never") + .env_remove("SSH_ASKPASS") + .arg("-o") + .arg("BatchMode=no") + .arg("-o") + .arg("StrictHostKeyChecking=yes") + .arg("-o") + .arg("NumberOfPasswordPrompts=3") + .arg("-T") + .arg(target) + .arg("exit"); + SshAuthenticationCommand { + command, + _config: config, } } @@ -578,7 +686,7 @@ pub(super) struct RemoteSsh { impl RemoteSsh { fn new(target: String, manage_ssh_config: bool, session_name: String) -> Self { let managed_config = if manage_ssh_config { - write_managed_ssh_config() + write_managed_ssh_config(&target) .inspect_err(|err| { tracing::debug!(%err, "could not write managed ssh config; using plain ssh"); }) @@ -596,12 +704,14 @@ impl RemoteSsh { } pub(super) fn new_noninteractive(target: String) -> Self { - Self { - target, - session_name: crate::session::DEFAULT_SESSION_NAME.into(), - managed_config: None, - noninteractive: true, - } + let manage = crate::platform::remote_ssh_config_paths().multiplexing + && crate::config::Config::load() + .config + .remote + .manage_ssh_config; + let mut ssh = Self::new(target, manage, crate::session::DEFAULT_SESSION_NAME.into()); + ssh.noninteractive = true; + ssh } fn target(&self) -> &str { @@ -1001,31 +1111,6 @@ fn decode_windows_remote_path(encoded: &str) -> io::Result<String> { Ok(path) } -impl Drop for RemoteSsh { - fn drop(&mut self) { - let Some(_options) = self - .managed_config - .as_ref() - .map(|config| &config.options) - .filter(|options| options.control_path.is_some()) - else { - return; - }; - - let _ = self - .base_command() - .arg("-O") - .arg("exit") - .arg("-o") - .arg("BatchMode=yes") - .arg(&self.target) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status(); - } -} - fn apply_noninteractive_ssh_options(command: &mut Command) { command .arg("-o") @@ -1053,13 +1138,16 @@ fn apply_managed_ssh_options(command: &mut Command, options: Option<&ManagedSshO command.arg("-F").arg(&options.config_path); if let Some(control_path) = &options.control_path { + // User ControlPaths may be shared across isolated Herdr configs (or + // explicitly disabled). Managed auth must use our scoped transport; + // never stop or unlink a master belonging to the user's SSH setup. command .arg("-S") .arg(control_path) .arg("-o") .arg("ControlMaster=auto") .arg("-o") - .arg("ControlPersist=yes"); + .arg("ControlPersist=600"); } } @@ -1080,7 +1168,7 @@ fn apply_managed_scp_options(command: &mut Command, options: Option<&ManagedSshO .arg("-o") .arg("ControlMaster=auto") .arg("-o") - .arg("ControlPersist=yes"); + .arg("ControlPersist=600"); } } @@ -1947,7 +2035,7 @@ fn probe_remote_endpoint( remote_herdr.clone(), path.clone(), ssh.session_name.clone(), - None, + ssh.options(), true, )?; let mut stream = crate::ipc::connect_local_stream(&path)?; @@ -2723,14 +2811,19 @@ fn ssh_user_config_include(path: Option<&Path>) -> Option<String> { /// Builds a temporary ssh config that includes the user's settings first, so /// OpenSSH's first-value-wins behavior preserves explicit user keepalives. -fn write_managed_ssh_config() -> io::Result<ManagedSshConfig> { +fn write_managed_ssh_config(target: &str) -> io::Result<ManagedSshConfig> { let paths = crate::platform::remote_ssh_config_paths(); + let control_path = if paths.multiplexing { + Some(crate::platform::shared_ssh_control_path( + &crate::config::config_path(), + target, + )?) + } else { + None + }; + let dir = crate::platform::create_remote_ssh_config_dir(SSH_CONTROL_SOCKET_NAME)?; let path = dir.join("config"); - let control_path = paths - .multiplexing - .then(|| dir.join(SSH_CONTROL_SOCKET_NAME)); - let mut contents = String::new(); if let Some(include) = ssh_user_config_include(paths.user_config.as_deref()) { contents.push_str(&format!("Include {include}\n")); @@ -2757,6 +2850,7 @@ fn write_managed_ssh_config() -> io::Result<ManagedSshConfig> { options: ManagedSshOptions { config_path: path, control_path, + _directory: Arc::new(ManagedSshConfigDirectory(dir)), }, }) } @@ -3531,7 +3625,7 @@ mod tests { fn managed_ssh_config_includes_user_config_then_fallback() { use std::os::unix::fs::PermissionsExt; - let managed_config = write_managed_ssh_config().expect("write managed config"); + let managed_config = write_managed_ssh_config("example").expect("write managed config"); let path = managed_config.options.config_path.clone(); let control_path = managed_config .options @@ -3591,6 +3685,106 @@ mod tests { drop(managed_config); } + #[cfg(unix)] + #[test] + fn shared_ssh_transport_survives_helper_config_drop() { + let first = write_managed_ssh_config("example").unwrap(); + let second = write_managed_ssh_config("example").unwrap(); + let socket = first.options.control_path.clone().unwrap(); + assert_eq!(Some(&socket), second.options.control_path.as_ref()); + assert_ne!(socket.parent(), first.options.config_path.parent()); + let config_path = first.options.config_path.clone(); + drop(first); + assert!(!config_path.exists()); + assert!(socket.parent().unwrap().is_dir()); + } + + #[test] + fn ssh_authentication_diagnostics_are_narrow() { + for message in [ + "user@host: Permission denied (publickey).", + "Permission denied (keyboard-interactive,password).", + "Permission denied (password).", + "sign_and_send_pubkey: signing failed for ED25519 from agent: agent refused operation", + ] { + assert!(ssh_error_requires_authentication(message), "{message}"); + } + for message in [ + "Host key verification failed.", + "REMOTE HOST IDENTIFICATION HAS CHANGED!", + "Permission denied opening /tmp/file", + "Connection refused", + "agent disconnected", + "Permission denied (publickey). Host key verification failed.", + ] { + assert!(!ssh_error_requires_authentication(message), "{message}"); + } + } + + #[test] + fn bridge_options_keep_temporary_config_alive_after_helper_drop() { + let config = write_managed_ssh_config("example").unwrap(); + let path = config.options.config_path.clone(); + let worker_options = config.options.clone(); + drop(config); + assert!(path.is_file()); + drop(worker_options); + assert!(!path.exists()); + } + + #[cfg(unix)] + #[test] + fn authentication_command_uses_shared_transport_without_askpass_or_host_key_relaxation() { + let config = write_managed_ssh_config("example").unwrap(); + let setup = RemoteSsh::new("example".into(), true, "other-session".into()); + assert_eq!( + config.options.control_path, + setup.options().unwrap().control_path + ); + let authentication = authentication_command_with_config("example", config); + let command = &authentication.command; + assert_eq!(command.get_program(), "ssh"); + let args = command + .get_args() + .map(|arg| arg.to_string_lossy()) + .collect::<Vec<_>>(); + for required in [ + "ControlMaster=auto", + "ControlPersist=600", + "BatchMode=no", + "StrictHostKeyChecking=yes", + ] { + assert!(args.iter().any(|arg| arg == required), "missing {required}"); + } + assert_eq!(&args[args.len() - 3..], &["-T", "example", "exit"]); + let env = command.get_envs().collect::<Vec<_>>(); + assert!(env.iter().any( + |(key, value)| *key == std::ffi::OsStr::new("SSH_ASKPASS_REQUIRE") + && *value == Some(std::ffi::OsStr::new("never")) + )); + assert!(env + .iter() + .any(|(key, value)| *key == std::ffi::OsStr::new("SSH_ASKPASS") && value.is_none())); + } + + #[test] + fn authentication_command_rejects_option_injection() { + assert_eq!( + ssh_authentication_command("-oProxyCommand=bad") + .err() + .unwrap() + .kind(), + io::ErrorKind::InvalidInput + ); + } + + #[test] + fn unmanaged_ssh_setup_preserves_plain_transport() { + let ssh = RemoteSsh::new("example".into(), false, "main".into()); + assert!(ssh.options().is_none()); + assert!(!ssh.command().get_args().any(|arg| arg == "-F")); + } + #[test] fn ssh_config_quote_wraps_path_with_spaces() { assert_eq!( @@ -3602,14 +3796,9 @@ mod tests { #[cfg(unix)] #[test] fn remote_ssh_command_uses_managed_config_when_present() { - let mut managed_config = write_managed_ssh_config().expect("write managed config"); - managed_config.options.control_path = Some(PathBuf::from("/tmp/herdr test/control")); + let managed_config = write_managed_ssh_config("example").expect("write managed config"); let config_path = managed_config.options.config_path.clone(); - let control_path = managed_config - .options - .control_path - .clone() - .expect("Unix managed config has a control path"); + let control_path = managed_config.options.control_path.clone().unwrap(); let ssh = RemoteSsh { target: "example".to_string(), session_name: crate::session::DEFAULT_SESSION_NAME.into(), @@ -3634,7 +3823,7 @@ mod tests { "-o".to_string(), "ControlMaster=auto".to_string(), "-o".to_string(), - "ControlPersist=yes".to_string(), + "ControlPersist=600".to_string(), "-T".to_string(), "example".to_string(), ] @@ -3656,7 +3845,7 @@ mod tests { "-o".to_string(), "ControlMaster=auto".to_string(), "-o".to_string(), - "ControlPersist=yes".to_string(), + "ControlPersist=600".to_string(), ] ); } @@ -3664,7 +3853,7 @@ mod tests { #[cfg(windows)] #[test] fn windows_managed_ssh_config_uses_keepalives_without_control_socket() { - let managed_config = write_managed_ssh_config().expect("write managed config"); + let managed_config = write_managed_ssh_config("example").expect("write managed config"); let config_path = managed_config.options.config_path.clone(); assert!(managed_config.options.control_path.is_none()); let contents = std::fs::read_to_string(&config_path).expect("read managed config"); @@ -3741,6 +3930,44 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn endpoint_probe_preserves_setup_ssh_options() { + let managed_config = write_managed_ssh_config("example").expect("write managed config"); + let marker = managed_config + .options + .config_path + .with_file_name("probe-ran"); + fs::write( + &managed_config.options.config_path, + format!( + "Host *\n ProxyCommand /bin/sh -c {}\n", + shell_quote(&format!( + ": > {}; exit 1", + shell_quote(&marker.to_string_lossy()) + )) + ), + ) + .expect("write isolated probe config"); + let ssh = RemoteSsh { + target: "herdr-probe.invalid".into(), + session_name: "probe-options".into(), + managed_config: Some(managed_config), + noninteractive: false, + }; + let remote = RemoteHerdr::for_platform(RemotePlatform { + os: "linux", + arch: "x86_64", + }); + + let error = probe_remote_endpoint(&ssh, &remote).expect_err("proxy refuses connection"); + + assert!( + marker.exists(), + "endpoint probe discarded the authenticated setup's SSH options: {error}" + ); + } + #[test] fn noninteractive_ssh_stderr_capture_is_bounded() { let stderr = vec![b'x'; NONINTERACTIVE_SSH_STDERR_LIMIT + 4096]; @@ -3768,8 +3995,7 @@ mod tests { ] { assert!(args.iter().any(|arg| arg == required), "missing {required}"); } - assert!(!args.iter().any(|arg| arg == "-F")); - assert!(ssh.options().is_none()); + assert_eq!(args.iter().any(|arg| arg == "-F"), ssh.options().is_some()); } #[test] |
