From 8ee5e8bd7d7468d589ae08b8d8328ee4bc557351 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 15 May 2026 15:34:42 +0900 Subject: [PATCH 1/4] fix: collect --password once up-front and honor -S across subcommands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two defects in credential collection that surfaced together in `bssh -C orin -l lablup --password -S ping` (issue #200). Defect 1 — `--password` is collected lazily per-connection. The previous implementation prompted via `rpassword::prompt_password()` inside each per-node authentication task (`ssh/auth.rs::password_auth()`). The executor fans out N parallel connection attempts, so multiple tasks raced for stdin: the prompt could be missed, repeated per node, or interleaved with the indicatif progress UI rendering. The `-S` (sudo-password) path already did this correctly — collect once in the dispatcher, wrap in `Arc`, share across tasks — and now `--password` follows the same pattern. Defect 2 — `-S` was silently dropped by `ping`, `upload`, and `download`. The dispatcher only read `cli.sudo_password` in the `exec` and interactive branches. Users could pass `-S` to `ping`/`upload`/`download` without any error, but the flag had no effect. Changes: - Add `src/security/password.rs` mirroring the `SudoPassword` design: a `Password` wrapper backed by `secrecy::SecretString` (auto-zeroized on drop), `prompt_password()` with a non-host-specific prompt ("Enter SSH password (used for all hosts): "), `get_password_from_env()` reading `BSSH_PASSWORD`, and `get_password(warn_env)` combining both. Wrapped in `Arc` for sharing across per-node tasks without duplicating secret material. - Hoist `--password` collection to `dispatch_command` in `src/app/dispatcher.rs`. Prompt runs once, before any `ParallelExecutor` is built and before any indicatif `MultiProgress` is initialized, so the terminal is in a clean state when reading the password. The resulting `Arc` is threaded through `ping_nodes`, `FileTransferParams` (upload/download), `ExecuteCommandParams`, `InteractiveCommand`, and the SSH-mode interactive path. - Extend `AuthContext` in `src/ssh/auth.rs` with a `password: Option>` field and a `with_pre_collected_password()` builder. `password_auth()` now consumes the pre-collected value when available; the in-task `rpassword::prompt_password()` call remains only as the fallback for the OpenSSH-style "all key methods failed, prompt for password" path (which has no dispatcher-side collection because it is opportunistic). - Thread `ssh_password: Option>` through the executor (`ParallelExecutor::with_ssh_password`), `ExecutionConfig`, `ConnectionConfig`, and the SFTP `*_with_jump_hosts` helpers. Every per-node task clones the `Arc` so all tasks observe the same secret without copying it. - For `-S`: emit `tracing::warn!` in the dispatcher when the flag is passed to a subcommand where it has no effect (`ping`, `upload`, `download`, `list`, `cache-stats`). Chose a warning over a hard reject to match typical CLI UX — existing scripts that happen to pass `-S` to non-applicable subcommands keep working but the user gets visible feedback. `exec` and interactive paths continue to honor `-S` as before. Tests: - 9 unit tests in `src/security/password.rs` mirror the structure of `src/security/sudo.rs`: creation, empty rejection, debug redaction, clone independence, `Arc` sharing, env var handling (empty/valid/unset), and a dispatcher-collection-pattern test that verifies a single `get_password()` call yields an `Arc` observably shared across three simulated per-node tasks. - 2 new tests in `src/ssh/auth.rs`: `test_auth_context_with_pre_collected_password` checks the builder and Arc sharing semantics; `test_password_auth_uses_pre_collected_password` verifies `password_auth()` returns immediately without prompting when the pre-collected value is set (critical: it never blocks on stdin in non-interactive test environments). - All existing tests (1233 lib tests + integration suites) continue to pass. `cargo build --release`, `cargo clippy --all-targets --all-features -- -D warnings`, and `cargo fmt --check` all pass. Closes #200 --- examples/interactive_demo.rs | 1 + src/app/dispatcher.rs | 82 ++++++- src/commands/download.rs | 4 +- src/commands/exec.rs | 7 +- src/commands/interactive/connection.rs | 3 +- src/commands/interactive/types.rs | 5 + src/commands/interactive/utils.rs | 2 + src/commands/ping.rs | 6 +- src/commands/upload.rs | 9 +- src/executor/connection_manager.rs | 14 +- src/executor/execution_strategy.rs | 5 + src/executor/parallel.rs | 33 ++- src/security/mod.rs | 4 + src/security/password.rs | 292 +++++++++++++++++++++++++ src/ssh/auth.rs | 87 +++++++- src/ssh/client/command.rs | 4 + src/ssh/client/config.rs | 10 + src/ssh/client/connection.rs | 20 +- src/ssh/client/file_transfer.rs | 19 ++ tests/interactive_integration_test.rs | 9 + tests/interactive_test.rs | 2 + tests/ssh_keepalive_test.rs | 3 + 22 files changed, 605 insertions(+), 16 deletions(-) create mode 100644 src/security/password.rs diff --git a/examples/interactive_demo.rs b/examples/interactive_demo.rs index 81ec33db..f7b3378d 100644 --- a/examples/interactive_demo.rs +++ b/examples/interactive_demo.rs @@ -54,6 +54,7 @@ async fn main() -> anyhow::Result<()> { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, diff --git a/src/app/dispatcher.rs b/src/app/dispatcher.rs index 1c1d14a6..c5f3e339 100644 --- a/src/app/dispatcher.rs +++ b/src/app/dispatcher.rs @@ -27,7 +27,7 @@ use bssh::{ }, config::InteractiveMode, pty::PtyConfig, - security::get_sudo_password, + security::{Password, get_password, get_sudo_password}, ssh::tokio_client::{DEFAULT_KEEPALIVE_INTERVAL, DEFAULT_KEEPALIVE_MAX, SshConnectionConfig}, }; use std::path::{Path, PathBuf}; @@ -83,6 +83,38 @@ fn build_ssh_connection_config( ssh_connection_config } +/// Decide whether `-S` (sudo-password) is meaningful for the given subcommand. +/// +/// `exec` and the interactive paths legitimately need a sudo password; the +/// other subcommands run either no command (`ping` just runs `true`) or operate +/// over SFTP (`upload`, `download`, `list`), so sudo is never relevant. +fn sudo_password_is_applicable(command: &Option) -> bool { + match command { + Some(Commands::Ping) + | Some(Commands::Upload { .. }) + | Some(Commands::Download { .. }) + | Some(Commands::List) + | Some(Commands::CacheStats { .. }) => false, + // exec (None), Interactive, and unknown future subcommands default to + // "applicable" — better to over-accept than silently strip a flag the + // user explicitly passed. + _ => true, + } +} + +/// Human-readable name of the currently-dispatched subcommand for diagnostics. +fn subcommand_name(command: &Option) -> &'static str { + match command { + Some(Commands::List) => "list", + Some(Commands::Ping) => "ping", + Some(Commands::Upload { .. }) => "upload", + Some(Commands::Download { .. }) => "download", + Some(Commands::Interactive { .. }) => "interactive", + Some(Commands::CacheStats { .. }) => "cache-stats", + None => "exec", + } +} + /// Dispatch commands to their appropriate handlers pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { // Get command to execute @@ -100,6 +132,36 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { ); } + // Warn if -S is passed to subcommands where it has no effect. We choose + // a warning (not a hard reject) to match typical CLI UX: existing scripts + // that happen to pass -S to `ping` or `upload` keep working, but the user + // gets visible feedback that the flag is being ignored. + if cli.sudo_password && !sudo_password_is_applicable(&cli.command) { + tracing::warn!( + "--sudo-password (-S) has no effect for the `{}` subcommand and will be ignored", + subcommand_name(&cli.command) + ); + } + + // Hoist `--password` collection: prompt ONCE here, before any per-node + // SSH connection task is spawned and before the indicatif progress UI is + // rendered. Previously this prompt was issued inside each parallel auth + // task (see `ssh/auth.rs::password_auth()`), which interleaved with the + // progress bar and could fan out N concurrent stdin reads. Collecting + // here mirrors what `-S` (`SudoPassword`) does and is the entire point + // of issue #200. + // + // The returned value is wrapped in `Arc` so cloning across + // per-node tasks does not duplicate the underlying secret; on drop, the + // memory is zeroized by `secrecy::SecretString`. + let ssh_password: Option> = if cli.password { + Some(Arc::new(get_password(true).map_err(|e| { + anyhow::anyhow!("Failed to collect SSH password: {e}") + })?)) + } else { + None + }; + // Calculate hostname for SSH config integration let hostname_for_ssh_config = if cli.is_ssh_mode() { cli.parse_destination().map(|(_, host, _)| host) @@ -143,6 +205,7 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { cli.timeout, Some(cli.connect_timeout), jump_hosts, + ssh_password.clone(), ) .await } @@ -172,6 +235,7 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { strict_mode: ctx.strict_mode, use_agent: cli.use_agent, use_password: cli.password, + ssh_password: ssh_password.clone(), recursive: *recursive, ssh_config: Some(&ctx.ssh_config), jump_hosts, @@ -204,6 +268,7 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { strict_mode: ctx.strict_mode, use_agent: cli.use_agent, use_password: cli.password, + ssh_password: ssh_password.clone(), recursive: *recursive, ssh_config: Some(&ctx.ssh_config), jump_hosts, @@ -225,6 +290,7 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { prompt_format, history_file, work_dir.as_deref(), + ssh_password.clone(), ) .await } @@ -234,12 +300,13 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { } None => { // Execute command (auto-exec or interactive shell) - handle_exec_command(cli, ctx, &command).await + handle_exec_command(cli, ctx, &command, ssh_password.clone()).await } } } /// Handle interactive command execution +#[allow(clippy::too_many_arguments)] async fn handle_interactive_command( cli: &Cli, ctx: &AppContext, @@ -248,6 +315,7 @@ async fn handle_interactive_command( prompt_format: &str, history_file: &Path, work_dir: Option<&str>, + ssh_password: Option>, ) -> Result<()> { // Get interactive config from configuration file (with cluster-specific overrides) let cluster_name = cli.cluster.as_deref(); @@ -340,6 +408,7 @@ async fn handle_interactive_command( key_path, use_agent: cli.use_agent, use_password: cli.password, + ssh_password, #[cfg(target_os = "macos")] use_keychain, strict_mode: ctx.strict_mode, @@ -358,7 +427,12 @@ async fn handle_interactive_command( } /// Handle exec command or SSH mode interactive session -async fn handle_exec_command(cli: &Cli, ctx: &AppContext, command: &str) -> Result<()> { +async fn handle_exec_command( + cli: &Cli, + ctx: &AppContext, + command: &str, + ssh_password: Option>, +) -> Result<()> { // In SSH mode without command, start interactive session if cli.is_ssh_mode() && command.is_empty() { // SSH mode interactive session (like ssh user@host) @@ -414,6 +488,7 @@ async fn handle_exec_command(cli: &Cli, ctx: &AppContext, command: &str) -> Resu key_path, use_agent: cli.use_agent, use_password: cli.password, + ssh_password, #[cfg(target_os = "macos")] use_keychain, strict_mode: ctx.strict_mode, @@ -504,6 +579,7 @@ async fn handle_exec_command(cli: &Cli, ctx: &AppContext, command: &str) -> Resu strict_mode: ctx.strict_mode, use_agent: cli.use_agent, use_password: cli.password, + ssh_password, #[cfg(target_os = "macos")] use_keychain, output_dir: cli.output_dir.as_deref(), diff --git a/src/commands/download.rs b/src/commands/download.rs index 86f7b7f0..a5f17b8d 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -57,7 +57,8 @@ pub async fn download_file( params.use_agent, params.use_password, ) - .with_jump_hosts(params.jump_hosts.clone()); + .with_jump_hosts(params.jump_hosts.clone()) + .with_ssh_password(params.ssh_password.clone()); if let Some(ssh_config) = params.ssh_config { executor = executor.with_ssh_config(Some(ssh_config.clone())); } @@ -121,6 +122,7 @@ pub async fn download_file( params.jump_hosts.as_deref(), // Pass jump hosts from params None, // Use default connect timeout params.ssh_config, // Pass ssh_config for ProxyJump resolution + params.ssh_password.clone(), ) .await; diff --git a/src/commands/exec.rs b/src/commands/exec.rs index c9204b93..08c37640 100644 --- a/src/commands/exec.rs +++ b/src/commands/exec.rs @@ -19,7 +19,7 @@ use std::sync::Arc; use crate::executor::{ExitCodeStrategy, OutputMode, ParallelExecutor, RankDetector}; use crate::forwarding::ForwardingType; use crate::node::Node; -use crate::security::SudoPassword; +use crate::security::{Password, SudoPassword}; use crate::ssh::SshConfig; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::SshConnectionConfig; @@ -35,6 +35,10 @@ pub struct ExecuteCommandParams<'a> { pub strict_mode: StrictHostKeyChecking, pub use_agent: bool, pub use_password: bool, + /// Pre-collected SSH password collected once by the dispatcher and shared + /// (via `Arc::clone`) with every per-node SSH connection task. When + /// `use_password` is `true`, this should be `Some(_)`. + pub ssh_password: Option>, #[cfg(target_os = "macos")] pub use_keychain: bool, pub output_dir: Option<&'a Path>, @@ -218,6 +222,7 @@ async fn execute_command_without_forwarding(params: ExecuteCommandParams<'_>) -> .with_connect_timeout(params.connect_timeout) .with_jump_hosts(params.jump_hosts.map(|s| s.to_string())) .with_sudo_password(params.sudo_password) + .with_ssh_password(params.ssh_password) .with_batch_mode(params.batch) .with_fail_fast(params.fail_fast) .with_ssh_config(params.ssh_config.cloned()) diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index 61fd1064..98493eed 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -161,7 +161,8 @@ impl InteractiveCommand { auth_ctx = auth_ctx .with_agent(self.use_agent) .with_password(self.use_password) - .with_password_fallback(!self.use_password); // Enable fallback only if not using explicit password + .with_password_fallback(!self.use_password) // Enable fallback only if not using explicit password + .with_pre_collected_password(self.ssh_password.clone()); // Set macOS Keychain integration if available #[cfg(target_os = "macos")] diff --git a/src/commands/interactive/types.rs b/src/commands/interactive/types.rs index 5631422f..3960c120 100644 --- a/src/commands/interactive/types.rs +++ b/src/commands/interactive/types.rs @@ -18,11 +18,13 @@ use anyhow::Result; use russh::Channel; use russh::client::Msg; use std::path::PathBuf; +use std::sync::Arc; use tokio::time::Duration; use crate::config::{Config, InteractiveConfig}; use crate::node::Node; use crate::pty::PtyConfig; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::{Client, SshConnectionConfig}; @@ -53,6 +55,9 @@ pub struct InteractiveCommand { pub key_path: Option, pub use_agent: bool, pub use_password: bool, + /// Pre-collected SSH password (collected once by the dispatcher and shared + /// with each node's interactive session). Mirrors the exec/ping path. + pub ssh_password: Option>, #[cfg(target_os = "macos")] pub use_keychain: bool, pub strict_mode: StrictHostKeyChecking, diff --git a/src/commands/interactive/utils.rs b/src/commands/interactive/utils.rs index 3748bdc7..0772613e 100644 --- a/src/commands/interactive/utils.rs +++ b/src/commands/interactive/utils.rs @@ -87,6 +87,7 @@ mod tests { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -121,6 +122,7 @@ mod tests { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, diff --git a/src/commands/ping.rs b/src/commands/ping.rs index ed53cd92..7bf06e96 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -15,9 +15,11 @@ use anyhow::Result; use owo_colors::OwoColorize; use std::path::Path; +use std::sync::Arc; use crate::executor::ParallelExecutor; use crate::node::Node; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ui::OutputFormatter; @@ -33,6 +35,7 @@ pub async fn ping_nodes( timeout: Option, connect_timeout: Option, jump_hosts: Option, + ssh_password: Option>, ) -> Result<()> { println!( "{}", @@ -55,7 +58,8 @@ pub async fn ping_nodes( ) .with_timeout(ping_timeout) .with_connect_timeout(connect_timeout) - .with_jump_hosts(jump_hosts); + .with_jump_hosts(jump_hosts) + .with_ssh_password(ssh_password); #[cfg(target_os = "macos")] let executor = executor.with_keychain(use_keychain); diff --git a/src/commands/upload.rs b/src/commands/upload.rs index bf35e10f..c181347e 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -15,9 +15,11 @@ use anyhow::{Context, Result}; use owo_colors::OwoColorize; use std::path::Path; +use std::sync::Arc; use crate::executor::ParallelExecutor; use crate::node::Node; +use crate::security::Password; use crate::ssh::SshConfig; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ui::OutputFormatter; @@ -30,6 +32,10 @@ pub struct FileTransferParams<'a> { pub strict_mode: StrictHostKeyChecking, pub use_agent: bool, pub use_password: bool, + /// Pre-collected SSH password (collected once by the dispatcher and shared + /// with every per-node SFTP task). Honors the same single-prompt invariant + /// as the `exec` path. + pub ssh_password: Option>, pub recursive: bool, pub ssh_config: Option<&'a SshConfig>, /// Jump hosts specification for connections. @@ -88,7 +94,8 @@ pub async fn upload_file( params.use_agent, params.use_password, ) - .with_jump_hosts(params.jump_hosts.clone()); + .with_jump_hosts(params.jump_hosts.clone()) + .with_ssh_password(params.ssh_password.clone()); if let Some(ssh_config) = params.ssh_config { executor = executor.with_ssh_config(Some(ssh_config.clone())); } diff --git a/src/executor/connection_manager.rs b/src/executor/connection_manager.rs index 900bcd77..539a967e 100644 --- a/src/executor/connection_manager.rs +++ b/src/executor/connection_manager.rs @@ -19,7 +19,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use crate::node::Node; -use crate::security::SudoPassword; +use crate::security::{Password, SudoPassword}; use crate::ssh::{ SshClient, SshConfig, client::{CommandResult, ConnectionConfig}, @@ -40,6 +40,10 @@ pub(crate) struct ExecutionConfig<'a> { pub connect_timeout: Option, pub jump_hosts: Option<&'a str>, pub sudo_password: Option>, + /// Pre-collected SSH password (collected once by the dispatcher and shared + /// across every per-node task). When `use_password` is true, this MUST be + /// `Some(_)`; auth.rs consumes it instead of prompting per node. + pub ssh_password: Option>, pub ssh_config: Option<&'a SshConfig>, /// SSH connection configuration (keepalive settings). /// Threaded through to `Client::connect_with_ssh_config` so user-configured @@ -82,6 +86,7 @@ pub(crate) async fn execute_on_node_with_jump_hosts( connect_timeout_seconds: config.connect_timeout, jump_hosts_spec: effective_jump_hosts, ssh_connection_config: config.ssh_connection_config, + ssh_password: config.ssh_password.clone(), }; // If sudo password is provided, use streaming execution to handle prompts @@ -134,6 +139,7 @@ pub(crate) async fn upload_to_node( jump_hosts: Option<&str>, connect_timeout_seconds: Option, ssh_config: Option<&SshConfig>, + pre_collected_password: Option>, ) -> Result<()> { let mut client = SshClient::new(node.host.clone(), node.port, node.username.clone()); @@ -161,6 +167,7 @@ pub(crate) async fn upload_to_node( use_password, effective_jump_hosts, connect_timeout_seconds, + pre_collected_password, ) .await } else { @@ -174,6 +181,7 @@ pub(crate) async fn upload_to_node( use_password, effective_jump_hosts, connect_timeout_seconds, + pre_collected_password, ) .await } @@ -192,6 +200,7 @@ pub(crate) async fn download_from_node( jump_hosts: Option<&str>, connect_timeout_seconds: Option, ssh_config: Option<&SshConfig>, + pre_collected_password: Option>, ) -> Result { let mut client = SshClient::new(node.host.clone(), node.port, node.username.clone()); @@ -219,6 +228,7 @@ pub(crate) async fn download_from_node( use_password, effective_jump_hosts, connect_timeout_seconds, + pre_collected_password, ) .await?; @@ -238,6 +248,7 @@ pub async fn download_dir_from_node( jump_hosts: Option<&str>, connect_timeout_seconds: Option, ssh_config: Option<&SshConfig>, + pre_collected_password: Option>, ) -> Result { let mut client = SshClient::new(node.host.clone(), node.port, node.username.clone()); @@ -263,6 +274,7 @@ pub async fn download_dir_from_node( use_password, effective_jump_hosts, connect_timeout_seconds, + pre_collected_password, ) .await?; diff --git a/src/executor/execution_strategy.rs b/src/executor/execution_strategy.rs index 0c666e0e..e60d6bf2 100644 --- a/src/executor/execution_strategy.rs +++ b/src/executor/execution_strategy.rs @@ -22,6 +22,7 @@ use std::sync::Arc; use tokio::sync::Semaphore; use crate::node::Node; +use crate::security::Password; use super::connection_manager::{ ExecutionConfig, download_from_node, execute_on_node_with_jump_hosts, upload_to_node, @@ -117,6 +118,7 @@ pub(crate) async fn upload_file_task( jump_hosts: Option, connect_timeout: Option, ssh_config: Option, + ssh_password: Option>, semaphore: Arc, pb: ProgressBar, ) -> UploadResult { @@ -144,6 +146,7 @@ pub(crate) async fn upload_file_task( jump_hosts.as_deref(), connect_timeout, ssh_config.as_ref(), + ssh_password, ) .await; @@ -179,6 +182,7 @@ pub(crate) async fn download_file_task( jump_hosts: Option, connect_timeout: Option, ssh_config: Option, + ssh_password: Option>, semaphore: Arc, pb: ProgressBar, ) -> DownloadResult { @@ -218,6 +222,7 @@ pub(crate) async fn download_file_task( jump_hosts.as_deref(), connect_timeout, ssh_config.as_ref(), + ssh_password, ) .await; diff --git a/src/executor/parallel.rs b/src/executor/parallel.rs index 6193b775..55675cdd 100644 --- a/src/executor/parallel.rs +++ b/src/executor/parallel.rs @@ -22,7 +22,7 @@ use std::sync::Arc; use tokio::sync::Semaphore; use crate::node::Node; -use crate::security::SudoPassword; +use crate::security::{Password, SudoPassword}; use crate::ssh::SshConfig; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::SshConnectionConfig; @@ -49,6 +49,11 @@ pub struct ParallelExecutor { pub(crate) connect_timeout: Option, pub(crate) jump_hosts: Option, pub(crate) sudo_password: Option>, + /// SSH password collected once up-front by the dispatcher. + /// + /// When set, this is shared (via `Arc::clone`) with every per-node SSH + /// connection task so a single prompt is reused across the whole cluster. + pub(crate) ssh_password: Option>, pub(crate) batch: bool, pub(crate) fail_fast: bool, pub(crate) ssh_config: Option, @@ -87,6 +92,7 @@ impl ParallelExecutor { connect_timeout: None, jump_hosts: None, sudo_password: None, + ssh_password: None, batch: false, fail_fast: false, ssh_config: None, @@ -115,6 +121,7 @@ impl ParallelExecutor { connect_timeout: None, jump_hosts: None, sudo_password: None, + ssh_password: None, batch: false, fail_fast: false, ssh_config: None, @@ -144,6 +151,7 @@ impl ParallelExecutor { connect_timeout: None, jump_hosts: None, sudo_password: None, + ssh_password: None, batch: false, fail_fast: false, ssh_config: None, @@ -191,6 +199,17 @@ impl ParallelExecutor { self } + /// Set the pre-collected SSH password. + /// + /// The dispatcher prompts for the password once (before any progress UI is + /// initialized) and threads the result through here. Every per-node SSH + /// connection task then shares this same `Arc` instead of + /// prompting on its own — matching how `with_sudo_password` works. + pub fn with_ssh_password(mut self, ssh_password: Option>) -> Self { + self.ssh_password = ssh_password; + self + } + /// Set batch mode for signal handling. /// /// When set to true, a single Ctrl+C will immediately terminate all jobs. @@ -268,6 +287,7 @@ impl ParallelExecutor { let connect_timeout = self.connect_timeout; let jump_hosts = self.jump_hosts.clone(); let sudo_password = self.sudo_password.clone(); + let ssh_password = self.ssh_password.clone(); let semaphore = Arc::clone(&semaphore); let pb = setup_progress_bar(&multi_progress, &node, style.clone(), "Connecting..."); @@ -286,6 +306,7 @@ impl ParallelExecutor { connect_timeout, jump_hosts: jump_hosts.as_deref(), sudo_password: sudo_password.clone(), + ssh_password: ssh_password.clone(), ssh_config: ssh_config_ref.as_ref(), ssh_connection_config: Some(&ssh_connection_config), }; @@ -396,6 +417,7 @@ impl ParallelExecutor { let connect_timeout = self.connect_timeout; let jump_hosts = self.jump_hosts.clone(); let sudo_password = self.sudo_password.clone(); + let ssh_password = self.ssh_password.clone(); let semaphore = Arc::clone(&semaphore); let pb = setup_progress_bar(&multi_progress, &node, style.clone(), "Connecting..."); let mut cancel_rx = cancel_rx.clone(); @@ -463,6 +485,7 @@ impl ParallelExecutor { connect_timeout, jump_hosts: jump_hosts.as_deref(), sudo_password: sudo_password.clone(), + ssh_password: ssh_password.clone(), ssh_config: ssh_config_ref.as_ref(), ssh_connection_config: Some(&ssh_connection_config_ref), }; @@ -632,6 +655,7 @@ impl ParallelExecutor { let use_password = self.use_password; let jump_hosts = self.jump_hosts.clone(); let connect_timeout = self.connect_timeout; + let ssh_password = self.ssh_password.clone(); let semaphore = Arc::clone(&semaphore); let pb = setup_progress_bar(&multi_progress, &node, style.clone(), "Connecting..."); @@ -648,6 +672,7 @@ impl ParallelExecutor { jump_hosts, connect_timeout, ssh_config_ref, + ssh_password, semaphore, pb, )) @@ -745,6 +770,7 @@ impl ParallelExecutor { let use_password = self.use_password; let jump_hosts = self.jump_hosts.clone(); let connect_timeout = self.connect_timeout; + let ssh_password = self.ssh_password.clone(); let semaphore = Arc::clone(&semaphore); let pb = setup_progress_bar(&multi_progress, &node, style.clone(), "Connecting..."); let ssh_config_ref = self.ssh_config.clone(); @@ -760,6 +786,7 @@ impl ParallelExecutor { jump_hosts, connect_timeout, ssh_config_ref, + ssh_password, semaphore, pb, )) @@ -881,6 +908,7 @@ impl ParallelExecutor { let jump_hosts = self.jump_hosts.clone(); let connect_timeout = self.connect_timeout; let ssh_config_ref = self.ssh_config.clone(); + let ssh_password = self.ssh_password.clone(); tokio::spawn(async move { let _permit = match semaphore.acquire().await { @@ -907,6 +935,7 @@ impl ParallelExecutor { jump_hosts.as_deref(), connect_timeout, ssh_config_ref.as_ref(), + ssh_password, ) .await; @@ -1131,6 +1160,7 @@ impl ParallelExecutor { let connect_timeout = self.connect_timeout; let jump_hosts = self.jump_hosts.clone(); let sudo_password = self.sudo_password.clone(); + let ssh_password = self.ssh_password.clone(); let semaphore = Arc::clone(&semaphore); let ssh_connection_config = self.ssh_connection_config.clone(); @@ -1179,6 +1209,7 @@ impl ParallelExecutor { connect_timeout_seconds: connect_timeout, jump_hosts_spec: jump_hosts.as_deref(), ssh_connection_config: Some(&ssh_connection_config), + ssh_password: ssh_password.clone(), }; // Execute with or without sudo password support diff --git a/src/security/mod.rs b/src/security/mod.rs index 6cb140bc..d3dd8032 100644 --- a/src/security/mod.rs +++ b/src/security/mod.rs @@ -21,6 +21,7 @@ //! backward compatibility. New code should prefer importing directly from //! `crate::shared::validation`. +mod password; mod sudo; // Keep the validation module for backward compatibility but it now re-exports @@ -33,6 +34,9 @@ pub use crate::shared::validation::{ validate_username, }; +// Re-export SSH password handling (collected once up-front by the dispatcher) +pub use password::{Password, get_password, get_password_from_env, prompt_password}; + // Re-export sudo password handling pub use sudo::{ SUDO_FAILURE_PATTERNS, SUDO_PROMPT_PATTERNS, SudoPassword, contains_sudo_failure, diff --git a/src/security/password.rs b/src/security/password.rs new file mode 100644 index 00000000..89da1298 --- /dev/null +++ b/src/security/password.rs @@ -0,0 +1,292 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Secure SSH password handling with automatic memory clearing. +//! +//! This module provides: +//! - `Password`: A secure wrapper for SSH authentication passwords with automatic +//! zeroization (mirrors the `SudoPassword` design). +//! - Helpers for collecting the password once, up-front, from either the +//! `BSSH_PASSWORD` environment variable (discouraged) or an interactive prompt. +//! +//! # Security Considerations +//! - Passwords are automatically cleared from memory when dropped. +//! - Debug output never reveals the password. +//! - Empty passwords are rejected at construction time. +//! - Cloning produces an independent copy that is also zeroized on drop. +//! +//! # Why hoist the prompt to the dispatcher? +//! Prompting per-connection (inside each parallel SSH task) races multiple +//! tasks for stdin and interleaves with the progress UI. By collecting the +//! password once before the executor / `indicatif` UI is initialized and +//! sharing an `Arc` across every per-node auth task, the prompt is +//! shown exactly once and reused for all nodes in the cluster. + +use anyhow::Result; +use secrecy::{ExposeSecret, SecretString}; +use std::fmt; + +/// A secure wrapper for SSH authentication passwords. +/// +/// Automatically clears its contents from memory when dropped. Designed to be +/// wrapped in `Arc` and shared across all per-node SSH connection +/// tasks so that the password is collected exactly once. +/// +/// # Security +/// - Password is automatically zeroized when dropped. +/// - Debug output does not reveal the password. +/// - Clone creates a new copy that is also zeroized independently. +/// - Safe to share across tasks (via `Arc`). +#[derive(Clone)] +pub struct Password { + /// The actual password stored securely. + inner: SecretString, +} + +impl Password { + /// Create a new `Password` from a string. + /// + /// The password will be automatically cleared from memory when dropped. + /// + /// # Arguments + /// * `password` - The password string (will be moved into a secure container). + /// + /// # Returns + /// * `Ok(Password)` if the password is non-empty. + /// * `Err` if the password is empty. + pub fn new(password: String) -> Result { + if password.is_empty() { + anyhow::bail!("Password cannot be empty"); + } + Ok(Self { + inner: SecretString::new(password.into_boxed_str()), + }) + } + + /// Get the password as a string slice. + /// + /// # Security Note + /// The returned slice should be used immediately and not stored. + /// Consumers (e.g., `AuthMethod::with_password`) typically copy this + /// into their own `Zeroizing` container. + pub fn as_str(&self) -> &str { + self.inner.expose_secret() + } + + /// Check if the password is empty. + /// + /// Note: this always returns `false` since empty passwords are rejected + /// during construction; kept for API symmetry with `SudoPassword`. + pub fn is_empty(&self) -> bool { + self.inner.expose_secret().is_empty() + } +} + +impl fmt::Debug for Password { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Password") + .field("password", &"[REDACTED]") + .finish() + } +} + +/// Prompt the user for an SSH password securely. +/// +/// Uses `rpassword` to read the password without echoing it to the terminal. +/// The prompt is intentionally non-host-specific because the same password is +/// reused for every node in the cluster. +/// +/// # Security Note +/// - The password is never printed to stdout/stderr. +/// - The password is stored in a `Password` (backed by `SecretString`). +/// - Empty passwords are rejected with a clear error message. +/// +/// # Important: call this BEFORE initializing any progress UI (`indicatif`). +/// Prompting after `MultiProgress` is rendered will interleave the prompt +/// with progress bar output and corrupt the terminal state. +pub fn prompt_password() -> Result { + let password = rpassword::prompt_password("Enter SSH password (used for all hosts): ") + .map_err(|e| anyhow::anyhow!("Failed to read password: {}", e))?; + + if password.is_empty() { + anyhow::bail!("Empty password not allowed. Please enter a valid SSH password."); + } + + Password::new(password) +} + +/// Get SSH password from environment variable (if set). +/// +/// # Security Warning +/// Using environment variables for passwords is NOT recommended in production +/// as they may be visible in process listings and shell history. Provided for +/// automation scenarios where the security trade-off is acceptable. +/// +/// # Returns +/// * `Some(Password)` if `BSSH_PASSWORD` is set and non-empty. +/// * `None` if the environment variable is not set. +/// * `Err` if `BSSH_PASSWORD` is set but empty (empty passwords are rejected). +pub fn get_password_from_env() -> Result> { + match std::env::var("BSSH_PASSWORD") { + Ok(password) if !password.is_empty() => Ok(Some(Password::new(password)?)), + Ok(_) => { + anyhow::bail!("BSSH_PASSWORD is set but empty. Empty passwords are not allowed."); + } + Err(_) => Ok(None), + } +} + +/// Get the SSH password from either environment or interactive prompt. +/// +/// First checks `BSSH_PASSWORD`; if unset, prompts the user interactively. +/// +/// # Arguments +/// * `warn_env` - If `true`, print a warning when using the environment variable. +/// +/// # Returns +/// A `Password` containing the entered password. +/// +/// # Important +/// Call this BEFORE any progress UI is initialized so the prompt is shown +/// cleanly. The dispatcher invokes this once per command and threads the +/// resulting `Arc` through every per-node connection task. +pub fn get_password(warn_env: bool) -> Result { + match get_password_from_env()? { + Some(password) => { + if warn_env { + eprintln!( + "Warning: Using SSH password from BSSH_PASSWORD environment variable. \ + This is not recommended for security reasons." + ); + } + Ok(password) + } + None => prompt_password(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::test_helpers::EnvGuard; + use serial_test::serial; + + #[test] + fn test_password_creation() { + let password = Password::new("test123".to_string()).unwrap(); + assert_eq!(password.as_str(), "test123"); + assert!(!password.is_empty()); + } + + #[test] + fn test_password_empty_rejection() { + let result = Password::new(String::new()); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("cannot be empty")); + } + + #[test] + fn test_password_debug_redaction() { + let password = Password::new("secret".to_string()).unwrap(); + let debug_output = format!("{password:?}"); + assert!(!debug_output.contains("secret")); + assert!(debug_output.contains("[REDACTED]")); + } + + #[test] + fn test_clone_independence() { + let p1 = Password::new("original".to_string()).unwrap(); + let p2 = p1.clone(); + + assert_eq!(p1.as_str(), "original"); + assert_eq!(p2.as_str(), "original"); + // Each clone is zeroized independently on drop. + } + + #[test] + fn test_arc_sharing() { + use std::sync::Arc; + let p = Arc::new(Password::new("shared".to_string()).unwrap()); + let c1 = Arc::clone(&p); + let c2 = Arc::clone(&p); + + // All three references see the same value (the same underlying secret). + assert_eq!(p.as_str(), "shared"); + assert_eq!(c1.as_str(), "shared"); + assert_eq!(c2.as_str(), "shared"); + assert_eq!(Arc::strong_count(&p), 3); + } + + #[test] + #[serial] + fn test_get_password_from_env_empty() { + // Empty BSSH_PASSWORD should error; guard restores prior value on drop. + let _guard = EnvGuard::set("BSSH_PASSWORD", ""); + let result = get_password_from_env(); + + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("empty")); + } + + #[test] + #[serial] + fn test_get_password_from_env_valid() { + let _guard = EnvGuard::set("BSSH_PASSWORD", "test_password"); + let result = get_password_from_env(); + + assert!(result.is_ok()); + let password = result.unwrap(); + assert!(password.is_some()); + assert_eq!(password.unwrap().as_str(), "test_password"); + } + + #[test] + #[serial] + fn test_get_password_from_env_not_set() { + let _guard = EnvGuard::remove("BSSH_PASSWORD"); + let result = get_password_from_env(); + + assert!(result.is_ok()); + assert!(result.unwrap().is_none()); + } + + /// Verify that the env-driven helper used by the dispatcher returns the + /// same password every caller would observe — modeling the dispatcher-level + /// collection pattern (collect once, share to every node). + #[test] + #[serial] + fn test_get_password_dispatcher_collection_pattern() { + use std::sync::Arc; + + let _guard = EnvGuard::set("BSSH_PASSWORD", "shared_password"); + + // Dispatcher path: collect once, no warning here (warn behavior covered separately). + let password = get_password(false).expect("env password should succeed"); + + // Wrap in Arc (this is exactly what the dispatcher does before fanning out to + // per-node connection tasks). + let shared = Arc::new(password); + + // Simulate three parallel per-node auth tasks pulling the same password. + let n1 = Arc::clone(&shared); + let n2 = Arc::clone(&shared); + let n3 = Arc::clone(&shared); + + assert_eq!(n1.as_str(), "shared_password"); + assert_eq!(n2.as_str(), "shared_password"); + assert_eq!(n3.as_str(), "shared_password"); + // All three nodes observed an identical password (single prompt invariant). + assert_eq!(Arc::strong_count(&shared), 4); + } +} diff --git a/src/ssh/auth.rs b/src/ssh/auth.rs index 467a9ff5..a211c4b0 100644 --- a/src/ssh/auth.rs +++ b/src/ssh/auth.rs @@ -27,11 +27,13 @@ use anyhow::{Context, Result}; use std::io::IsTerminal; use std::path::{Path, PathBuf}; +use std::sync::Arc; use std::time::Duration; use tokio::time::timeout; use zeroize::Zeroizing; use super::tokio_client::AuthMethod; +use crate::security::Password; /// Maximum time to wait for password/passphrase input const AUTH_PROMPT_TIMEOUT: Duration = Duration::from_secs(30); @@ -112,6 +114,14 @@ pub struct AuthContext { pub username: String, /// Host for authentication prompts (validated) pub host: String, + /// Pre-collected SSH password shared across all per-node connection tasks. + /// + /// When set, `password_auth()` consumes this value instead of prompting, + /// so the user is asked exactly once per command (in the dispatcher) and + /// the same password is reused for every node and for the password + /// fallback path. Wrapped in `Arc` so cloning the context across tasks + /// does not duplicate the underlying secret. + pub password: Option>, } impl AuthContext { @@ -151,6 +161,7 @@ impl AuthContext { use_keychain: false, username, host, + password: None, }) } @@ -208,6 +219,19 @@ impl AuthContext { self } + /// Provide a pre-collected SSH password. + /// + /// Used by callers (the dispatcher) that prompt for the password once, + /// up-front, before fanning out parallel SSH connections. When set, + /// `password_auth()` consumes this value instead of prompting interactively. + /// + /// The password is shared via `Arc`, so cloning this context (e.g., + /// per-node) does not duplicate the underlying secret material. + pub fn with_pre_collected_password(mut self, password: Option>) -> Self { + self.password = password; + self + } + /// Determine the appropriate authentication method based on the context. /// /// This method implements the standard authentication priority with security hardening: @@ -408,16 +432,35 @@ impl AuthContext { .context("Consent prompt task failed")? } - /// Attempt password authentication with timeout. + /// Build an `AuthMethod` from the pre-collected password. + /// + /// `AuthMethod::with_password` internally wraps the value in `Zeroizing`, + /// so the borrowed slice from `Password::as_str()` is only used to build + /// a securely-stored copy; no plaintext is retained outside the secrets + /// machinery. async fn password_auth(&self) -> Result { tracing::debug!("Using password authentication"); - // Run password prompt with timeout to prevent hanging + // Priority 1: Use the pre-collected password if the dispatcher provided one. + // This is the expected path for `--password`: the dispatcher prompts once + // before any per-node connection task starts (and before the indicatif + // progress UI is initialized), then shares the same value with every node. + if let Some(ref password) = self.password { + return Ok(AuthMethod::with_password(password.as_str())); + } + + // Priority 2: Fallback prompt (only reached when no pre-collected password + // is available — e.g., the OpenSSH-style "all key-based methods failed, + // prompt for password" path triggered by `determine_method` itself, never + // by an explicit `--password` flag if the dispatcher behaves correctly). + // + // This branch keeps the historical behavior so the password-fallback + // path (interactive only) still works; it is the only remaining caller + // that legitimately needs an in-task prompt. let prompt_future = tokio::task::spawn_blocking({ let username = self.username.clone(); let host = self.host.clone(); move || -> Result> { - // Use Zeroizing to ensure password is cleared from memory when dropped let password = Zeroizing::new( rpassword::prompt_password(format!("Enter password for {username}@{host}: ")) .with_context(|| "Failed to read password")?, @@ -750,6 +793,44 @@ mod tests { assert_eq!(ctx.key_path, None); assert!(!ctx.use_agent); assert!(!ctx.use_password); + assert!(ctx.password.is_none()); + } + + #[tokio::test] + async fn test_auth_context_with_pre_collected_password() { + let password = Arc::new(Password::new("test123".to_string()).unwrap()); + let ctx = AuthContext::new("user".to_string(), "host".to_string()) + .unwrap() + .with_password(true) + .with_pre_collected_password(Some(Arc::clone(&password))); + + assert!(ctx.use_password); + assert!(ctx.password.is_some()); + assert_eq!(ctx.password.as_ref().unwrap().as_str(), "test123"); + // Both the test holder and the AuthContext share the same Arc. + assert_eq!(Arc::strong_count(&password), 2); + } + + #[tokio::test] + async fn test_password_auth_uses_pre_collected_password() { + // Verifies that when a pre-collected password is provided, password_auth() + // returns immediately with AuthMethod::Password(...) instead of prompting. + // Critically, this means it never blocks on stdin — even in a non-interactive + // test environment. + let password = Arc::new(Password::new("pre_collected_secret".to_string()).unwrap()); + let ctx = AuthContext::new("user".to_string(), "host".to_string()) + .unwrap() + .with_password(true) + .with_pre_collected_password(Some(password)); + + let auth = ctx.password_auth().await.expect("should not prompt"); + + match auth { + AuthMethod::Password(stored) => { + assert_eq!(stored.as_str(), "pre_collected_secret"); + } + other => panic!("Expected AuthMethod::Password, got {other:?}"), + } } #[tokio::test] diff --git a/src/ssh/client/command.rs b/src/ssh/client/command.rs index 8ab10d87..f7aadd8e 100644 --- a/src/ssh/client/command.rs +++ b/src/ssh/client/command.rs @@ -63,6 +63,7 @@ impl SshClient { connect_timeout_seconds: None, // Use default jump_hosts_spec: None, // No jump hosts ssh_connection_config: None, + ssh_password: None, // Legacy API path: no pre-collected password }; self.connect_and_execute_with_jump_hosts(command, &config) @@ -85,6 +86,7 @@ impl SshClient { config.use_password, #[cfg(target_os = "macos")] config.use_keychain, + config.ssh_password.clone(), ) .await?; @@ -196,6 +198,7 @@ impl SshClient { config.use_password, #[cfg(target_os = "macos")] config.use_keychain, + config.ssh_password.clone(), ) .await?; @@ -308,6 +311,7 @@ impl SshClient { config.use_password, #[cfg(target_os = "macos")] config.use_keychain, + config.ssh_password.clone(), ) .await?; diff --git a/src/ssh/client/config.rs b/src/ssh/client/config.rs index 1bd7f45c..4b81405d 100644 --- a/src/ssh/client/config.rs +++ b/src/ssh/client/config.rs @@ -12,9 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::SshConnectionConfig; use std::path::Path; +use std::sync::Arc; /// Configuration for SSH connection and command execution #[derive(Clone)] @@ -30,4 +32,12 @@ pub struct ConnectionConfig<'a> { pub jump_hosts_spec: Option<&'a str>, /// SSH keepalive / inactivity settings. `None` falls back to defaults. pub ssh_connection_config: Option<&'a SshConnectionConfig>, + /// Pre-collected SSH password shared with every per-node auth task. + /// + /// When `use_password` is `true`, this MUST be `Some(_)`; the dispatcher + /// is responsible for prompting once up-front and threading the value + /// through. `None` means "no pre-collected password" — `auth.rs` then + /// falls back to its in-task prompt (used for the OpenSSH-style + /// password-fallback path only). + pub ssh_password: Option>, } diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index d4675a5c..422142fe 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -14,10 +14,12 @@ use super::core::SshClient; use crate::jump::{JumpHostChain, parse_jump_hosts}; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::{AuthMethod, Client, SshConnectionConfig}; use anyhow::{Context, Result}; use std::path::Path; +use std::sync::Arc; use std::time::Duration; // SSH connection timeout design: @@ -27,13 +29,19 @@ use std::time::Duration; const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; impl SshClient { - /// Determine the authentication method based on provided parameters + /// Determine the authentication method based on provided parameters. + /// + /// The `pre_collected_password` argument carries the password the dispatcher + /// collected once up-front (via `--password`). When `use_password` is `true` + /// and a pre-collected value is provided, `AuthContext::password_auth()` + /// consumes it directly instead of prompting in the per-node task. pub(super) async fn determine_auth_method( &self, key_path: Option<&Path>, use_agent: bool, use_password: bool, #[cfg(target_os = "macos")] use_keychain: bool, + pre_collected_password: Option>, ) -> Result { // Use centralized authentication logic from auth module let mut auth_ctx = @@ -49,7 +57,10 @@ impl SshClient { .with_context(|| format!("Invalid SSH key path: {path:?}"))?; } - auth_ctx = auth_ctx.with_agent(use_agent).with_password(use_password); + auth_ctx = auth_ctx + .with_agent(use_agent) + .with_password(use_password) + .with_pre_collected_password(pre_collected_password); #[cfg(target_os = "macos")] { @@ -292,6 +303,7 @@ mod tests { false, #[cfg(target_os = "macos")] false, + None, ) .await .unwrap(); @@ -338,6 +350,7 @@ mod tests { false, #[cfg(target_os = "macos")] false, + None, ) .await .unwrap(); @@ -382,7 +395,7 @@ mod tests { let client = SshClient::new("test.com".to_string(), 22, "user".to_string()); let auth = client - .determine_auth_method(None, true, false) + .determine_auth_method(None, true, false, None) .await .unwrap(); @@ -430,6 +443,7 @@ mod tests { false, #[cfg(target_os = "macos")] false, + None, ) .await .unwrap(); diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index 900b31fe..07eb1da7 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -13,10 +13,12 @@ // limitations under the License. use super::core::SshClient; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::Client; use anyhow::{Context, Result}; use std::path::Path; +use std::sync::Arc; use std::time::Duration; // File upload timeout design: @@ -72,6 +74,7 @@ impl SshClient { use_password, "file copy", connect_timeout_seconds, + None, ) .await?; @@ -140,6 +143,7 @@ impl SshClient { use_password, "file download", connect_timeout_seconds, + None, ) .await?; @@ -204,6 +208,7 @@ impl SshClient { use_password, "directory upload", connect_timeout_seconds, + None, ) .await?; @@ -270,6 +275,7 @@ impl SshClient { use_password, "directory download", connect_timeout_seconds, + None, ) .await?; @@ -326,6 +332,7 @@ impl SshClient { use_password: bool, jump_hosts_spec: Option<&str>, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result<()> { tracing::debug!( "Uploading file to {}:{} (jump hosts: {:?})", @@ -342,6 +349,7 @@ impl SshClient { use_password, jump_hosts_spec, connect_timeout_seconds, + pre_collected_password, ) .await?; @@ -402,6 +410,7 @@ impl SshClient { use_password: bool, jump_hosts_spec: Option<&str>, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result<()> { tracing::debug!( "Downloading file from {}:{} (jump hosts: {:?})", @@ -418,6 +427,7 @@ impl SshClient { use_password, jump_hosts_spec, connect_timeout_seconds, + pre_collected_password, ) .await?; @@ -474,6 +484,7 @@ impl SshClient { use_password: bool, jump_hosts_spec: Option<&str>, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result<()> { tracing::debug!( "Uploading directory to {}:{} (jump hosts: {:?})", @@ -490,6 +501,7 @@ impl SshClient { use_password, jump_hosts_spec, connect_timeout_seconds, + pre_collected_password, ) .await?; @@ -548,6 +560,7 @@ impl SshClient { use_password: bool, jump_hosts_spec: Option<&str>, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result<()> { tracing::debug!( "Downloading directory from {}:{} (jump hosts: {:?})", @@ -564,6 +577,7 @@ impl SshClient { use_password, jump_hosts_spec, connect_timeout_seconds, + pre_collected_password, ) .await?; @@ -609,6 +623,7 @@ impl SshClient { } /// Helper function to connect for file transfer operations (without jump hosts) + #[allow(clippy::too_many_arguments)] async fn connect_for_file_transfer( &self, key_path: Option<&Path>, @@ -617,6 +632,7 @@ impl SshClient { use_password: bool, operation_desc: &str, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result { let addr = (self.host.as_str(), self.port); tracing::debug!( @@ -635,6 +651,7 @@ impl SshClient { use_password, #[cfg(target_os = "macos")] false, + pre_collected_password, ) .await?; @@ -676,6 +693,7 @@ impl SshClient { use_password: bool, jump_hosts_spec: Option<&str>, connect_timeout_seconds: Option, + pre_collected_password: Option>, ) -> Result { // Determine authentication method // Note: use_keychain is set to false for file transfers to avoid prompts @@ -686,6 +704,7 @@ impl SshClient { use_password, #[cfg(target_os = "macos")] false, + pre_collected_password, ) .await?; diff --git a/tests/interactive_integration_test.rs b/tests/interactive_integration_test.rs index fe6c72db..b0c96479 100644 --- a/tests/interactive_integration_test.rs +++ b/tests/interactive_integration_test.rs @@ -47,6 +47,7 @@ fn test_interactive_command_builder() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -82,6 +83,7 @@ fn test_history_file_handling() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -180,6 +182,7 @@ async fn test_interactive_with_unreachable_nodes() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -215,6 +218,7 @@ async fn test_interactive_with_no_nodes() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -260,6 +264,7 @@ fn test_mode_configuration() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -286,6 +291,7 @@ fn test_mode_configuration() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -315,6 +321,7 @@ fn test_working_directory_config() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -339,6 +346,7 @@ fn test_working_directory_config() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -375,6 +383,7 @@ fn test_prompt_format() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, diff --git a/tests/interactive_test.rs b/tests/interactive_test.rs index 77080441..5b3c7a2d 100644 --- a/tests/interactive_test.rs +++ b/tests/interactive_test.rs @@ -37,6 +37,7 @@ async fn test_interactive_command_creation() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -66,6 +67,7 @@ async fn test_interactive_with_no_nodes() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, diff --git a/tests/ssh_keepalive_test.rs b/tests/ssh_keepalive_test.rs index 376a462f..12436048 100644 --- a/tests/ssh_keepalive_test.rs +++ b/tests/ssh_keepalive_test.rs @@ -667,6 +667,7 @@ fn test_interactive_mode_ssh_connection_config_default() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -714,6 +715,7 @@ fn test_interactive_mode_ssh_connection_config_custom() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, @@ -759,6 +761,7 @@ fn test_interactive_mode_ssh_connection_config_disabled_keepalive() { key_path: None, use_agent: false, use_password: false, + ssh_password: None, #[cfg(target_os = "macos")] use_keychain: false, strict_mode: StrictHostKeyChecking::AcceptNew, From 48846898472c390608ac23a4b250da0f0e366901 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 15 May 2026 15:59:01 +0900 Subject: [PATCH 2/4] fix: thread pre-collected password through jump-host and legacy command paths Addresses pr-reviewer findings on PR #201: - jump::chain::auth::determine_auth_method now consumes Arc instead of prompting per-call (issue #200 C1) - ssh::client::SshClient::connect_and_execute_with_host_check accepts Arc so the download glob-resolution step reuses the dispatcher's single prompt (issue #200 C2) - commands::exec::execute_command_with_forwarding now builds AuthMethod::with_password from the pre-collected secret instead of erroring after the user has already entered a password (issue #200 M1) --- src/commands/download.rs | 4 ++ src/commands/exec.rs | 19 ++++++++-- src/commands/interactive/connection.rs | 14 +++++-- src/jump/chain.rs | 19 ++++++++++ src/jump/chain/auth.rs | 52 ++++++++++++++++++-------- src/jump/chain/tunnel.rs | 3 ++ src/ssh/client/command.rs | 26 ++++++++++--- src/ssh/client/connection.rs | 6 ++- src/ssh/client/file_transfer.rs | 8 +++- 9 files changed, 119 insertions(+), 32 deletions(-) diff --git a/src/commands/download.rs b/src/commands/download.rs index a5f17b8d..e2fda5fc 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -187,6 +187,10 @@ pub async fn download_file( params.use_agent, params.use_password, None, // Use default timeout for ls command + // Issue #200 (C2): forward the dispatcher's pre-collected + // password so this glob-resolution step does NOT re-prompt + // after the dispatcher already asked once. + params.ssh_password.clone(), ) .await?; diff --git a/src/commands/exec.rs b/src/commands/exec.rs index 08c37640..6e7b49a3 100644 --- a/src/commands/exec.rs +++ b/src/commands/exec.rs @@ -113,10 +113,21 @@ async fn execute_command_with_forwarding(params: ExecuteCommandParams<'_>) -> Re return Err(anyhow::anyhow!("SSH agent not supported on Windows")); } } else if params.use_password { - // For password auth, we'd need to prompt - for now return error - return Err(anyhow::anyhow!( - "Password authentication not yet supported with port forwarding" - )); + // Issue #200 (M1): consume the dispatcher's pre-collected password + // instead of erroring out. Previously this branch returned + // "Password authentication not yet supported with port forwarding" + // even though the dispatcher had already prompted the user — a + // confusing UX regression. The dispatcher collects unconditionally + // for `exec`, so when `use_password` is true we always have one. + let Some(password) = params.ssh_password.as_ref() else { + anyhow::bail!( + "--password was requested for port-forwarding exec but no \ + password was collected up-front (programmer error in dispatcher: \ + the `exec` arm must populate `ExecuteCommandParams::ssh_password` \ + when `use_password` is true)." + ); + }; + AuthMethod::with_password(password.as_str()) } else { // Use default key file authentication let key_path = params diff --git a/src/commands/interactive/connection.rs b/src/commands/interactive/connection.rs index 98493eed..d732d6b5 100644 --- a/src/commands/interactive/connection.rs +++ b/src/commands/interactive/connection.rs @@ -263,11 +263,14 @@ impl InteractiveCommand { .min(MAX_TIMEOUT_SECS), ); - // Pass SSH connection config to jump host chain for keepalive settings + // Pass SSH connection config to jump host chain for keepalive settings. + // Also pass the dispatcher's pre-collected password so jump-host + // authentication consumes it instead of re-prompting per call. See #200. let chain = JumpHostChain::new(jump_hosts) .with_connect_timeout(adjusted_timeout) .with_command_timeout(Duration::from_secs(300)) - .with_ssh_connection_config(self.ssh_connection_config.clone()); + .with_ssh_connection_config(self.ssh_connection_config.clone()) + .with_ssh_password(self.ssh_password.clone()); // Connect through the chain let connection = timeout( @@ -407,11 +410,14 @@ impl InteractiveCommand { .min(MAX_TIMEOUT_SECS), ); - // Pass SSH connection config to jump host chain for keepalive settings + // Pass SSH connection config to jump host chain for keepalive settings. + // Also pass the dispatcher's pre-collected password so jump-host + // authentication consumes it instead of re-prompting per call. See #200. let chain = JumpHostChain::new(jump_hosts) .with_connect_timeout(adjusted_timeout) .with_command_timeout(Duration::from_secs(300)) - .with_ssh_connection_config(self.ssh_connection_config.clone()); + .with_ssh_connection_config(self.ssh_connection_config.clone()) + .with_ssh_password(self.ssh_password.clone()); // Connect through the chain let connection = timeout( diff --git a/src/jump/chain.rs b/src/jump/chain.rs index 4d960b81..809540a3 100644 --- a/src/jump/chain.rs +++ b/src/jump/chain.rs @@ -24,6 +24,7 @@ pub use types::{JumpConnection, JumpInfo}; use super::connection::JumpHostConnection; use super::parser::{JumpHost, get_max_jump_hosts}; use super::rate_limiter::ConnectionRateLimiter; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::{AuthMethod, SshConnectionConfig}; use anyhow::{Context, Result}; @@ -68,6 +69,12 @@ pub struct JumpHostChain { auth_mutex: Arc>, /// SSH connection configuration (keepalive settings) ssh_connection_config: SshConnectionConfig, + /// Pre-collected SSH password (from the dispatcher's single up-front prompt). + /// When `use_password` is set on a per-call basis, this is consumed by every + /// jump-host auth step instead of prompting per-call, which would otherwise + /// race N parallel auth tasks (serialized by `auth_mutex`) into N separate + /// prompts. See issue #200. + ssh_password: Option>, } impl JumpHostChain { @@ -104,9 +111,19 @@ impl JumpHostChain { max_connection_age: Duration::from_secs(1800), // 30 minutes auth_mutex: Arc::new(Mutex::new(())), ssh_connection_config: SshConnectionConfig::default(), + ssh_password: None, } } + /// Provide the pre-collected SSH password (collected once up-front by the + /// dispatcher). When `--password` is used together with `-J `, every + /// jump-host auth step consumes this shared secret instead of prompting + /// per-call. See issue #200. + pub fn with_ssh_password(mut self, password: Option>) -> Self { + self.ssh_password = password; + self + } + /// Set SSH connection configuration (keepalive settings) /// /// Configures keepalive interval and maximum attempts to prevent @@ -276,6 +293,7 @@ impl JumpHostChain { dest_key_path, dest_use_agent, dest_use_password, + self.ssh_password.clone(), dest_strict_mode.unwrap_or(StrictHostKeyChecking::AcceptNew), self.connect_timeout, &self.rate_limiter, @@ -381,6 +399,7 @@ impl JumpHostChain { key_path, use_agent, use_password, + self.ssh_password.clone(), &self.auth_mutex, ) .await?; diff --git a/src/jump/chain/auth.rs b/src/jump/chain/auth.rs index f7c4f227..6a1a41e4 100644 --- a/src/jump/chain/auth.rs +++ b/src/jump/chain/auth.rs @@ -13,9 +13,11 @@ // limitations under the License. use crate::jump::parser::JumpHost; +use crate::security::Password; use crate::ssh::tokio_client::{AuthMethod, ClientHandler}; use anyhow::{Context, Result}; use std::path::Path; +use std::sync::Arc; use tokio::sync::Mutex; use tracing::{debug, warn}; use zeroize::Zeroizing; @@ -72,11 +74,18 @@ async fn agent_has_identities() -> bool { /// 2. Cluster/defaults `key_path` (fallback, passed as parameter) /// 3. SSH agent (if use_agent=true and agent has keys) /// 4. Default key files (~/.ssh/id_*) +/// +/// When `use_password` is `true`, the `pre_collected_password` argument MUST +/// carry the password the dispatcher collected once up-front via +/// `prompt_password()` (issue #200). Per-call password prompts here would race +/// across parallel jump-host auth tasks and produce N prompts for N nodes — +/// the very bug `--password` was supposed to fix. pub(super) async fn determine_auth_method( jump_host: &JumpHost, key_path: Option<&Path>, use_agent: bool, use_password: bool, + pre_collected_password: Option>, auth_mutex: &Mutex<()>, ) -> Result { // Priority 1: Use jump host's own ssh_key if provided @@ -121,22 +130,27 @@ pub(super) async fn determine_auth_method( let agent_available = false; if use_password { - // SECURITY: Acquire mutex to serialize password prompts - // This prevents multiple simultaneous prompts that could confuse users - let _guard = auth_mutex.lock().await; - - // Display which jump host we're authenticating to - let prompt = format!( - "Enter password for jump host {} ({}@{}): ", - jump_host.to_connection_string(), - jump_host.effective_user(), - jump_host.host - ); - - let password = Zeroizing::new( - rpassword::prompt_password(prompt).with_context(|| "Failed to read password")?, - ); - return Ok(AuthMethod::with_password(&password)); + // Issue #200: consume the dispatcher's single up-front password instead + // of prompting per-call. Per-call prompts here race across parallel + // jump-host auth tasks (even with auth_mutex, they serialize into N + // sequential prompts for N nodes) and contradict the entire point of + // `--password`. The dispatcher MUST have collected this value before + // any per-node task was spawned; if it didn't, fail loudly rather than + // silently fall back to prompting. + let Some(password) = pre_collected_password.as_ref() else { + anyhow::bail!( + "SSH password authentication was requested for jump host {} but no \ + password was collected up-front. This is a programmer error: the \ + dispatcher must call `prompt_password()` once before spawning \ + per-node connection tasks and thread the resulting `Arc` \ + through to this jump-host authentication context.", + jump_host.to_connection_string(), + ); + }; + // Note: `auth_mutex` is intentionally NOT acquired here — no prompt is + // issued, so there is no I/O to serialize. The mutex remains needed + // for the key passphrase prompts below. + return Ok(AuthMethod::with_password(password.as_str())); } if use_agent && agent_available { @@ -536,6 +550,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= Some(key_path.as_path()), true, // use_agent false, // use_password + None, // pre_collected_password &auth_mutex, ) .await; @@ -587,6 +602,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= Some(key_path.as_path()), true, // use_agent false, // use_password + None, // pre_collected_password &auth_mutex, ) .await; @@ -642,6 +658,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= None, // No key_path false, // use_agent false, // use_password + None, // pre_collected_password &auth_mutex, ) .await; @@ -696,6 +713,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= None, // No key_path false, // use_agent=false means don't try agent first false, // use_password + None, // pre_collected_password &auth_mutex, ) .await; @@ -800,6 +818,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= Some(cluster_key_path.as_path()), // Cluster key false, // use_agent false, // use_password + None, // pre_collected_password &auth_mutex, ) .await; @@ -850,6 +869,7 @@ dGVzdHMgLSBub3QgcmVhbAECAwQ= Some(cluster_key_path.as_path()), false, false, + None, // pre_collected_password &auth_mutex, ) .await; diff --git a/src/jump/chain/tunnel.rs b/src/jump/chain/tunnel.rs index 3c5c5e6d..1e9b4914 100644 --- a/src/jump/chain/tunnel.rs +++ b/src/jump/chain/tunnel.rs @@ -15,6 +15,7 @@ use super::auth::authenticate_connection; use crate::jump::parser::JumpHost; use crate::jump::rate_limiter::ConnectionRateLimiter; +use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::{AuthMethod, Client, ClientHandler, SshConnectionConfig}; use anyhow::{Context, Result}; @@ -31,6 +32,7 @@ pub(super) async fn connect_through_tunnel( key_path: Option<&Path>, use_agent: bool, use_password: bool, + pre_collected_password: Option>, strict_mode: StrictHostKeyChecking, connect_timeout: std::time::Duration, rate_limiter: &ConnectionRateLimiter, @@ -82,6 +84,7 @@ pub(super) async fn connect_through_tunnel( key_path, use_agent, use_password, + pre_collected_password, auth_mutex, ) .await?; diff --git a/src/ssh/client/command.rs b/src/ssh/client/command.rs index f7aadd8e..f34bd8cb 100644 --- a/src/ssh/client/command.rs +++ b/src/ssh/client/command.rs @@ -15,11 +15,12 @@ use super::config::ConnectionConfig; use super::core::SshClient; use super::result::CommandResult; -use crate::security::SudoPassword; +use crate::security::{Password, SudoPassword}; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::CommandOutput; use anyhow::{Context, Result}; use std::path::Path; +use std::sync::Arc; use std::time::Duration; use tokio::sync::mpsc::Sender; @@ -38,11 +39,22 @@ impl SshClient { key_path: Option<&Path>, use_agent: bool, ) -> Result { - self.connect_and_execute_with_host_check(command, key_path, None, use_agent, false, None) - .await + // Trivial-call shim used only by the default-key path; no `--password` + // flag is in scope here so the pre-collected password is `None`. + self.connect_and_execute_with_host_check( + command, key_path, None, use_agent, false, None, None, + ) + .await } - /// Execute a command with host key checking configuration + /// Execute a command with host key checking configuration. + /// + /// The `ssh_password` argument carries the dispatcher's single up-front + /// password (when `--password` is used). Callers in the `download` + /// glob-resolution path MUST forward `FileTransferParams::ssh_password` + /// here — otherwise the user is prompted twice (once by the dispatcher, + /// once by this connection). See issue #200. + #[allow(clippy::too_many_arguments)] pub async fn connect_and_execute_with_host_check( &mut self, command: &str, @@ -51,6 +63,7 @@ impl SshClient { use_agent: bool, use_password: bool, timeout_seconds: Option, + ssh_password: Option>, ) -> Result { let config = ConnectionConfig { key_path, @@ -63,7 +76,7 @@ impl SshClient { connect_timeout_seconds: None, // Use default jump_hosts_spec: None, // No jump hosts ssh_connection_config: None, - ssh_password: None, // Legacy API path: no pre-collected password + ssh_password, }; self.connect_and_execute_with_jump_hosts(command, &config) @@ -105,6 +118,7 @@ impl SshClient { config.use_password, config.connect_timeout_seconds, config.ssh_connection_config, + config.ssh_password.clone(), ) .await?; @@ -217,6 +231,7 @@ impl SshClient { config.use_password, config.connect_timeout_seconds, config.ssh_connection_config, + config.ssh_password.clone(), ) .await?; @@ -330,6 +345,7 @@ impl SshClient { config.use_password, config.connect_timeout_seconds, config.ssh_connection_config, + config.ssh_password.clone(), ) .await?; diff --git a/src/ssh/client/connection.rs b/src/ssh/client/connection.rs index 422142fe..b147cd3b 100644 --- a/src/ssh/client/connection.rs +++ b/src/ssh/client/connection.rs @@ -176,13 +176,15 @@ impl SshClient { use_password: bool, connect_timeout_seconds: Option, ssh_connection_config: Option<&SshConnectionConfig>, + pre_collected_password: Option>, ) -> Result { // Create jump host chain with user-specified or default connect timeout let connect_timeout = Duration::from_secs(connect_timeout_seconds.unwrap_or(SSH_CONNECT_TIMEOUT_SECS)); let mut chain = JumpHostChain::new(jump_hosts.to_vec()) .with_connect_timeout(connect_timeout) - .with_command_timeout(Duration::from_secs(300)); + .with_command_timeout(Duration::from_secs(300)) + .with_ssh_password(pre_collected_password); if let Some(cfg) = ssh_connection_config { chain = chain.with_ssh_connection_config(cfg.clone()); } @@ -227,6 +229,7 @@ impl SshClient { use_password: bool, connect_timeout_seconds: Option, ssh_connection_config: Option<&SshConnectionConfig>, + pre_collected_password: Option>, ) -> Result { if let Some(jump_spec) = jump_hosts_spec { // Parse jump hosts @@ -265,6 +268,7 @@ impl SshClient { use_password, connect_timeout_seconds, ssh_connection_config, + pre_collected_password, ) .await } diff --git a/src/ssh/client/file_transfer.rs b/src/ssh/client/file_transfer.rs index 07eb1da7..c7441732 100644 --- a/src/ssh/client/file_transfer.rs +++ b/src/ssh/client/file_transfer.rs @@ -704,13 +704,16 @@ impl SshClient { use_password, #[cfg(target_os = "macos")] false, - pre_collected_password, + pre_collected_password.clone(), ) .await?; let strict_mode = strict_mode.unwrap_or(StrictHostKeyChecking::AcceptNew); - // Create client connection - either direct or through jump hosts + // Create client connection - either direct or through jump hosts. + // Threading `pre_collected_password` here ensures jump-host auth + // (when `--password` is combined with `-J`) consumes the dispatcher's + // single up-front prompt instead of re-prompting per jump. See #200. self.establish_connection( &auth_method, strict_mode, @@ -720,6 +723,7 @@ impl SshClient { use_password, connect_timeout_seconds, None, + pre_collected_password, ) .await } From b6d882067838b25906540cdd1fdce89ea613cd84 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 15 May 2026 16:12:43 +0900 Subject: [PATCH 3/4] fix: route -S ignored warning to stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tracing::warn!` lands on stdout under the default tracing subscriber, so the `-S has no effect` warning was being mixed into stdout — breaking `bssh ... ping | grep ...` style pipelines and contradicting the PR description's contract. Switch to `eprintln!` to match the existing pattern used for the `BSSH_PASSWORD` env warning in `src/security/password.rs:168-171`. --- src/app/dispatcher.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/dispatcher.rs b/src/app/dispatcher.rs index c5f3e339..419e7fc5 100644 --- a/src/app/dispatcher.rs +++ b/src/app/dispatcher.rs @@ -137,8 +137,8 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { // that happen to pass -S to `ping` or `upload` keep working, but the user // gets visible feedback that the flag is being ignored. if cli.sudo_password && !sudo_password_is_applicable(&cli.command) { - tracing::warn!( - "--sudo-password (-S) has no effect for the `{}` subcommand and will be ignored", + eprintln!( + "Warning: --sudo-password (-S) has no effect for the `{}` subcommand and will be ignored", subcommand_name(&cli.command) ); } From aeca1f45b5ce74635264f5bd0897e04bdba9c220 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 15 May 2026 16:16:58 +0900 Subject: [PATCH 4/4] docs: document BSSH_PASSWORD env var and single-prompt behavior Add CHANGELOG entry for #201 (collect --password once up-front, -S warning to stderr). Document the new BSSH_PASSWORD environment variable in the README environment-variables section and in the bssh.1 man page. Update the --password flag description in the man page to describe the single-prompt / Arc-shared behavior introduced by #201. --- CHANGELOG.md | 6 ++++++ README.md | 13 ++++++++++++- docs/man/bssh.1 | 27 +++++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50dbfb4a..f000043b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.1.5] - Unreleased + +### Fixed +- **`--password` prompt is now collected once up-front and shared across all parallel connection tasks** (#201, closes #200). Previously each per-node SSH task prompted for the password independently, racing each other for stdin and interleaving with the progress UI. The dispatcher now collects the password before any executor or `indicatif` UI is initialized and threads an `Arc` to every per-node auth task — matching the existing `-S` (`SudoPassword`) pattern. The `BSSH_PASSWORD` environment variable is supported for automation scenarios (discouraged; see security notes below). +- **`-S` / `--sudo-password` warning now routes to stderr** when passed to subcommands where it has no effect (`ping`, `upload`, `download`, `list`), keeping stdout clean for scripts that consume bssh output (#201). + ## [2.1.4] - 2026-05-10 ### Performance diff --git a/README.md b/README.md index 38553997..36722281 100644 --- a/README.md +++ b/README.md @@ -574,8 +574,10 @@ bssh supports multiple authentication methods: - **Explicit**: Use `-A` flag to force SSH agent authentication ### Password Authentication -- Use `-P` flag to enable password authentication +- Use `-P` / `--password` flag to enable password authentication +- The password is prompted **once up-front**, before any parallel connection tasks start, and is shared securely across all nodes — the prompt appears exactly once regardless of how many hosts are targeted - Password is prompted securely without echo +- For automation, set `BSSH_PASSWORD` in the environment (not recommended; see security notes in the Sudo Password section) ### Examples ```bash @@ -656,6 +658,15 @@ bssh supports configuration via environment variables: - **`SSH_AUTH_SOCK`**: SSH agent socket path (Unix-like systems) +### SSH Password Variable + +- **`BSSH_PASSWORD`**: SSH password for automated password authentication + - Used when `--password` / `-P` is set and `BSSH_PASSWORD` is non-empty; skips the interactive prompt + - **WARNING**: Not recommended for security reasons + - Environment variables may be visible in process listings and shell history + - Use the interactive `-P` prompt instead for security-sensitive operations + - Example: `BSSH_PASSWORD=secret bssh -P -H "user@host" "uptime"` + ### Sudo Password Variable - **`BSSH_SUDO_PASSWORD`**: Sudo password for automated sudo authentication diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 4c51371e..e48cc57b 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -155,8 +155,14 @@ is not available or authentication fails. .TP .BR \-\-password -Use password authentication. When this option is specified, bssh will -prompt for the password securely without echoing it to the terminal. +Use password authentication. The password is collected once up-front, +before any parallel connection tasks are started, and is shared securely +across all target nodes. The prompt appears exactly once regardless of +how many hosts are targeted. If the +.B BSSH_PASSWORD +environment variable is set and non-empty, it is used instead of +prompting interactively (not recommended; see +.BR ENVIRONMENT ). This is useful for systems that don't have SSH keys configured. .TP @@ -1796,6 +1802,23 @@ attacks while allowing flexible jump host configurations. .br Example: BSSH_MAX_JUMP_HOSTS=20 bssh -J host1,host2,...,host20 target +.TP +.B BSSH_PASSWORD +SSH password for automated password authentication. When set along with the +.B \-\-password +flag and the variable is non-empty, bssh uses this value instead of +prompting interactively. The password is still collected once up-front and +shared across all parallel connection tasks. +.br +.B WARNING: +Using environment variables for passwords is not recommended for production +use as they may be visible in process listings, shell history, or logs. +Prefer the interactive +.B \-\-password +prompt for security-sensitive operations. +.br +Example: BSSH_PASSWORD=mysecret bssh --password -H "user@host" "uptime" + .TP .B BSSH_SUDO_PASSWORD Sudo password for automated sudo authentication. When set along with the