diff --git a/README.md b/README.md index 24317f05..8dd53713 100644 --- a/README.md +++ b/README.md @@ -152,12 +152,16 @@ bssh -J bastion.example.com -C production "uptime" ### Multi-Server Mode (Cluster Operations) ```bash -# Using direct host specification +# Execute commands on multiple hosts (automatic command execution) bssh -H "user1@host1.com,user2@host2.com:2222" "uptime" # Using cluster from config bssh -C production "df -h" +# Filter specific hosts with pattern matching +bssh -H "web1,web2,db1,db2" -f "web*" "systemctl status nginx" +bssh -C production -f "db*" "pg_dump --version" + # With custom SSH key bssh -C staging -i ~/.ssh/custom_key "systemctl status nginx" @@ -180,14 +184,22 @@ bssh -C production --timeout 10 "quick-check" bssh -C staging --timeout 0 "long-running-backup" ``` -### Test connectivity +### Built-in Commands ```bash +# Test connectivity to hosts bssh -C production ping -``` +bssh -H "host1,host2" ping -### List configured clusters -```bash +# List configured clusters bssh list + +# Interactive mode (single or multiplexed) +bssh -C production interactive +bssh -H "host1,host2" interactive + +# File transfer operations +bssh -C production upload local.txt /tmp/ +bssh -H "host1,host2" download /etc/hosts ./backups/ ``` ## Authentication diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index b47c13cd..017a7830 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -134,6 +134,12 @@ Use password authentication. When this option is specified, bssh will prompt for the password securely without echoing it to the terminal. This is useful for systems that don't have SSH keys configured. +.TP +.BR \-f ", " \-\-filter " " \fIPATTERN\fR +Filter hosts by pattern (supports wildcards like 'web*'). +Use with -H or -C to execute on a subset of hosts. +Example: -f "web*" matches web01, web02, etc. + .TP .BR \-\-parallel " " \fIPARALLEL\fR Maximum parallel connections for multi-server mode (default: 10) @@ -161,9 +167,12 @@ Print help information Print version information .SH COMMANDS +In multi-server mode (-H or -C), commands can be executed directly without the 'exec' subcommand. +For example: bssh -H "host1,host2" "uptime" (automatic command execution) + .TP -.B exec -Execute a command on specified hosts +.B Built-in subcommands: +These commands provide special functionality and must be specified explicitly. .TP .B list @@ -376,16 +385,35 @@ Chain multiple jump hosts for connection .SS Multi-Server Mode .TP -Execute command on multiple hosts: +Execute command on multiple hosts (automatic execution): .B bssh -H "user1@host1,user2@host2" "uptime" +.RS +Commands are executed directly without needing 'exec' subcommand +.RE .TP Use cluster from configuration: .B bssh -C production "df -h" +.TP +Filter hosts with pattern matching: +.B bssh -H "web1,web2,db1,db2" -f "web*" "systemctl status nginx" +.RS +Executes command only on hosts matching the pattern 'web*' +.RE + +.TP +.B bssh -C production -f "db*" "pg_dump --version" +.RS +Executes command only on database nodes in the production cluster +.RE + .TP Test connectivity: .B bssh -C production ping +.RS +Note: 'ping' is a built-in subcommand, not an automatic execution +.RE .TP Upload file to remote hosts (SFTP): diff --git a/src/cli.rs b/src/cli.rs index dae9f939..10344426 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -23,12 +23,12 @@ use std::path::PathBuf; before_help = "\n\nBroadcast SSH - Parallel command execution across cluster nodes", about = "Broadcast SSH - SSH-compatible parallel command execution tool", long_about = "bssh is a high-performance SSH client with parallel execution capabilities.\nIt can be used as a drop-in replacement for SSH (single host) or as a powerful cluster management tool (multiple hosts).\n\nThe tool provides secure file transfer using SFTP and supports SSH keys, SSH agent, and password authentication.\nIt automatically detects Backend.AI multi-node session environments.\n\nSSH Configuration Support:\n- Reads standard SSH config files (defaulting to ~/.ssh/config)\n- Supports Host patterns, HostName, User, Port, IdentityFile, StrictHostKeyChecking\n- ProxyJump, and many other SSH configuration directives\n- CLI arguments override SSH config values following SSH precedence rules", - after_help = "EXAMPLES:\n SSH Mode:\n bssh user@host # Interactive shell\n bssh admin@server.com \"uptime\" # Execute command\n bssh -p 2222 -i ~/.ssh/key user@host # Custom port and key\n bssh -F ~/.ssh/myconfig webserver # Use custom SSH config\n\n Port Forwarding:\n bssh -L 8080:example.com:80 user@host # Local forward: localhost:8080 → example.com:80\n bssh -R 8080:localhost:80 user@host # Remote forward: remote:8080 → localhost:80\n bssh -D 1080 user@host # SOCKS5 proxy on localhost:1080\n bssh -L 3306:db:3306 -R 80:web:80 user@host # Multiple forwards\n bssh -D *:1080/4 user@host # SOCKS4 proxy on all interfaces\n\n Multi-Server Mode:\n bssh -C production \"systemctl status\" # Use cluster config\n bssh -H \"web1,web2,web3\" \"df -h\" # Direct hosts\n bssh -F /etc/ssh/ssh_config -H web* # SSH config with wildcards\n\n File Operations:\n bssh -C staging upload file.txt /tmp/ # Upload to cluster\n bssh -H host1,host2 download /etc/hosts ./backups/\n\n Other Commands:\n bssh list # List configured clusters\n bssh -C production ping # Test connectivity\n\n SSH Config Example (~/.ssh/config):\n Host web*\n HostName web.example.com\n User webuser\n Port 2222\n IdentityFile ~/.ssh/web_key\n StrictHostKeyChecking yes\n\nDeveloped and maintained as part of the Backend.AI project.\nFor more information: https://github.com/lablup/bssh" + after_help = "EXAMPLES:\n SSH Mode:\n bssh user@host # Interactive shell\n bssh admin@server.com \"uptime\" # Execute command\n bssh -p 2222 -i ~/.ssh/key user@host # Custom port and key\n bssh -F ~/.ssh/myconfig webserver # Use custom SSH config\n\n Port Forwarding:\n bssh -L 8080:example.com:80 user@host # Local forward: localhost:8080 → example.com:80\n bssh -R 8080:localhost:80 user@host # Remote forward: remote:8080 → localhost:80\n bssh -D 1080 user@host # SOCKS5 proxy on localhost:1080\n bssh -L 3306:db:3306 -R 80:web:80 user@host # Multiple forwards\n bssh -D *:1080/4 user@host # SOCKS4 proxy on all interfaces\n\n Multi-Server Mode:\n bssh -C production \"systemctl status\" # Execute on cluster\n bssh -H \"web1,web2,web3\" \"df -h\" # Execute on multiple hosts\n bssh -H \"web1,web2,web3\" -f \"web1\" \"df -h\" # Filter to web1 only\n bssh -C production -f \"web*\" \"uptime\" # Filter cluster nodes\n bssh --parallel 20 -H web* \"apt update\" # Increase parallelism\n\n File Operations:\n bssh -C staging upload file.txt /tmp/ # Upload to cluster\n bssh -H host1,host2 download /etc/hosts ./backups/\n\n Other Commands:\n bssh list # List configured clusters\n bssh -C production ping # Test connectivity\n bssh -H hosts interactive # Interactive mode\n\n SSH Config Example (~/.ssh/config):\n Host web*\n HostName web.example.com\n User webuser\n Port 2222\n IdentityFile ~/.ssh/web_key\n StrictHostKeyChecking yes\n\nDeveloped and maintained as part of the Backend.AI project.\nFor more information: https://github.com/lablup/bssh" )] pub struct Cli { /// SSH destination in format: [user@]hostname[:port] or ssh://[user@]hostname[:port] /// Used for SSH compatibility mode (single host connection) - #[arg(value_name = "destination", conflicts_with_all = ["cluster", "hosts"])] + #[arg(value_name = "destination")] pub destination: Option, #[command(subcommand)] @@ -42,6 +42,13 @@ pub struct Cli { )] pub hosts: Option>, + #[arg( + short = 'f', + long = "filter", + help = "Filter hosts by pattern (supports wildcards like 'web*')\nUse with -H or -C to execute on a subset of hosts\nExamples: 'web*' matches web01, web02, etc." + )] + pub filter: Option, + #[arg( short = 'C', long = "cluster", @@ -133,7 +140,11 @@ pub struct Cli { )] pub timeout: u64, - #[arg(trailing_var_arg = true, help = "Command to execute on remote hosts")] + #[arg( + trailing_var_arg = true, + help = "Command to execute on remote hosts", + allow_hyphen_values = true + )] pub command_args: Vec, // SSH-compatible options @@ -226,16 +237,6 @@ pub struct Cli { #[derive(Subcommand, Debug)] pub enum Commands { - #[command( - about = "Execute a command on specified hosts", - long_about = "Executes the specified command on all target hosts simultaneously.\nOutput is streamed in real-time with host prefixes for identification.\nSupports command timeout, output redirection, and partial failure handling.\n\nExit codes: 0 (all succeed), 1 (any failures)", - after_help = "Examples:\n bssh exec \"uptime\" # Execute on auto-detected or default hosts\n bssh -c prod exec \"systemctl status\" # Execute on cluster 'prod'\n bssh -p 20 exec \"apt update\" # Increase parallelism to 20\n bssh --output-dir logs exec \"df -h\" # Save outputs to files" - )] - Exec { - #[arg(trailing_var_arg = true)] - command: Vec, - }, - #[command( about = "List available clusters", long_about = "Displays all clusters defined in configuration files.\nShows cluster names, node counts, and configuration sources.\nIncludes auto-detected Backend.AI clusters if present.\n\nConfiguration sources checked (in order):\n - Backend.AI environment variables\n - Current directory (./config.yaml)\n - User config (~/.config/bssh/config.yaml)" @@ -347,17 +348,52 @@ pub enum Commands { impl Cli { pub fn get_command(&self) -> String { - if !self.command_args.is_empty() { + // In multi-server mode with destination, treat destination as first command arg + if self.is_multi_server_mode() && self.destination.is_some() { + let mut all_args = vec![self.destination.as_ref().unwrap().clone()]; + all_args.extend(self.command_args.clone()); + all_args.join(" ") + } else if !self.command_args.is_empty() { self.command_args.join(" ") - } else if let Some(Commands::Exec { command }) = &self.command { - command.join(" ") } else { String::new() } } + /// Check if the first command arg is a known subcommand + pub fn is_known_subcommand(arg: &str) -> bool { + matches!( + arg, + "list" | "ping" | "upload" | "download" | "interactive" | "cache-stats" + ) + } + + /// Determine if we should auto-execute a command + pub fn should_auto_exec(&self) -> bool { + // If in multi-server mode with destination or command_args, treat as exec + if self.is_multi_server_mode() { + // Check if destination is a known subcommand + if let Some(dest) = &self.destination { + if Self::is_known_subcommand(dest) { + return false; // It's a subcommand, not auto-exec + } + return true; // Has destination that's not a subcommand + } + // Check command_args + if !self.command_args.is_empty() { + if Self::is_known_subcommand(&self.command_args[0]) { + return false; + } + return true; + } + } + false + } + /// Check if running in SSH compatibility mode (single host) pub fn is_ssh_mode(&self) -> bool { + // Only SSH mode if destination is provided and no cluster/hosts + // If hosts/cluster is present, destination should be treated as first command arg self.destination.is_some() && self.cluster.is_none() && self.hosts.is_none() } @@ -366,6 +402,11 @@ impl Cli { self.cluster.is_some() || self.hosts.is_some() } + /// Get the host filter pattern if specified + pub fn get_host_filter(&self) -> Option<&str> { + self.filter.as_deref() + } + /// Parse destination string into components (user, host, port) pub fn parse_destination(&self) -> Option<(Option, String, Option)> { self.destination.as_ref().map(|dest| { diff --git a/src/commands/download.rs b/src/commands/download.rs index 29114853..7067b146 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -28,14 +28,24 @@ pub async fn download_file( source: &str, destination: &Path, ) -> Result<()> { + // Security: Validate the remote source path + let validated_source = crate::security::validate_remote_path(source) + .with_context(|| format!("Invalid source path: {}", source))?; + + // Security: Validate the local destination path + let validated_destination = crate::security::validate_local_path(destination) + .with_context(|| format!("Invalid destination path: {:?}", destination))?; + // Create destination directory if it doesn't exist - if !destination.exists() { - fs::create_dir_all(destination).await.with_context(|| { - format!( - "Failed to create destination directory: {}", - destination.display() - ) - })?; + if !validated_destination.exists() { + fs::create_dir_all(&validated_destination) + .await + .with_context(|| { + format!( + "Failed to create destination directory: {}", + validated_destination.display() + ) + })?; } let key_path_str = params.key_path.map(|p| p.to_string_lossy().to_string()); @@ -49,12 +59,17 @@ pub async fn download_file( ); // Check if source contains glob pattern - let has_glob = source.contains('*') || source.contains('?') || source.contains('['); + let has_glob = validated_source.contains('*') + || validated_source.contains('?') + || validated_source.contains('['); // Check if source is a directory (for recursive download) let is_directory = if params.recursive && !has_glob { // Use a test command to check if source is a directory - let test_cmd = format!("test -d '{source}' && echo 'dir' || echo 'file'"); + let test_cmd = format!( + "test -d '{}' && echo 'dir' || echo 'file'", + validated_source + ); let test_results = executor.execute(&test_cmd).await?; test_results.iter().any(|r| { r.result @@ -71,7 +86,7 @@ pub async fn download_file( "\n{} {} {} {} from {} nodes {}\n", "▶".cyan(), "Recursively downloading directory".cyan().bold(), - source.green(), + validated_source.green(), "from".dimmed(), params.nodes.len().to_string().yellow(), "(SFTP)".dimmed() @@ -82,7 +97,7 @@ pub async fn download_file( // Download the entire directory from each node for node in ¶ms.nodes { - let node_dir = destination.join(node.to_string()); + let node_dir = validated_destination.join(node.to_string()); println!( "\n{} {} {} {} {:?}", @@ -96,7 +111,7 @@ pub async fn download_file( // Use the download_dir_from_node function directly let result = executor::download_dir_from_node( node.clone(), - source, + &validated_source, &node_dir, key_path_str.as_deref(), params.strict_mode, @@ -150,7 +165,7 @@ pub async fn download_file( .nodes .first() .ok_or_else(|| anyhow::anyhow!("No nodes available"))?; - let glob_command = format!("ls -1 {source} 2>/dev/null || true"); + let glob_command = format!("ls -1 {} 2>/dev/null || true", validated_source); let mut test_client = SshClient::new( test_node.host.clone(), @@ -176,7 +191,7 @@ pub async fn download_file( .collect(); if remote_files.is_empty() { - anyhow::bail!("No files found matching pattern: {}", source); + anyhow::bail!("No files found matching pattern: {}", validated_source); } println!( @@ -188,11 +203,15 @@ pub async fn download_file( for file in &remote_files { println!(" {} {}", "•".dimmed(), file.cyan()); } - println!("{} {}\n", "Destination:".bold(), destination.display()); + println!( + "{} {}\n", + "Destination:".bold(), + validated_destination.display() + ); // Download each file let results = executor - .download_files(remote_files.clone(), destination) + .download_files(remote_files.clone(), &validated_destination) .await?; // Print results @@ -226,13 +245,15 @@ pub async fn download_file( "\n{} {} {} from {} nodes to {} {}\n", "▶".cyan(), "Downloading".cyan().bold(), - source.green(), + validated_source.green(), params.nodes.len().to_string().yellow(), - destination.display(), + validated_destination.display(), "(SFTP)".dimmed() ); - let results = executor.download_file(source, destination).await?; + let results = executor + .download_file(&validated_source, &validated_destination) + .await?; // Print results for result in &results { diff --git a/src/commands/ping.rs b/src/commands/ping.rs index 592c6de9..dd824e7d 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -69,7 +69,16 @@ pub async fn ping_nodes( "Failed".red() ); if let Err(e) = &result.result { - println!(" {} {}", "└".dimmed(), e.to_string().dimmed()); + // Display the full error chain for better debugging + let error_chain = format!("{:#}", e); + // Split by newlines and indent each line + for (i, line) in error_chain.lines().enumerate() { + if i == 0 { + println!(" {} {}", "└".dimmed(), line.dimmed()); + } else { + println!(" {}", line.dimmed()); + } + } } } } diff --git a/src/commands/upload.rs b/src/commands/upload.rs index e3a8a89d..02777586 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use anyhow::Result; +use anyhow::{Context, Result}; use owo_colors::OwoColorize; use std::path::Path; @@ -37,15 +37,23 @@ pub async fn upload_file( source: &Path, destination: &str, ) -> Result<()> { + // Security: Validate the local source path + let validated_source = crate::security::validate_local_path(source) + .with_context(|| format!("Invalid source path: {:?}", source))?; + + // Security: Validate the remote destination path + let validated_destination = crate::security::validate_remote_path(destination) + .with_context(|| format!("Invalid destination path: {}", destination))?; + // Collect all files matching the pattern - let files = resolve_source_files(source, params.recursive)?; + let files = resolve_source_files(&validated_source, params.recursive)?; if files.is_empty() { anyhow::bail!("No files found matching pattern: {:?}", source); } // Determine destination handling based on file count - let is_dir_destination = destination.ends_with('/') || files.len() > 1; + let is_dir_destination = validated_destination.ends_with('/') || files.len() > 1; // Display upload summary println!( @@ -61,7 +69,11 @@ pub async fn upload_file( .map_or_else(|_| "unknown".to_string(), |m| format_bytes(m.len())); println!(" {} {} ({})", "•".dimmed(), file.display(), size.yellow()); } - println!("{} {}\n", "Destination:".bold(), destination.green()); + println!( + "{} {}\n", + "Destination:".bold(), + validated_destination.green() + ); let key_path_str = params.key_path.map(|p| p.to_string_lossy().to_string()); let executor = ParallelExecutor::new_with_all_options( @@ -99,10 +111,10 @@ pub async fn upload_file( // Create remote directory structure if needed if let Some(parent) = relative_path.parent() { if !parent.as_os_str().is_empty() { - let remote_dir = if destination.ends_with('/') { - format!("{destination}{}", parent.display()) + let remote_dir = if validated_destination.ends_with('/') { + format!("{}{}", validated_destination, parent.display()) } else { - format!("{destination}/{}", parent.display()) + format!("{}/{}", validated_destination, parent.display()) }; // Create remote directory using SSH command let mkdir_cmd = format!("mkdir -p '{remote_dir}'"); @@ -110,10 +122,10 @@ pub async fn upload_file( } } - if destination.ends_with('/') { - format!("{destination}{remote_relative}") + if validated_destination.ends_with('/') { + format!("{}{remote_relative}", validated_destination) } else { - format!("{destination}/{remote_relative}") + format!("{}/{remote_relative}", validated_destination) } } else { // No base dir, just use filename @@ -121,10 +133,10 @@ pub async fn upload_file( .file_name() .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {:?}", file))? .to_string_lossy(); - if destination.ends_with('/') { - format!("{destination}{filename}") + if validated_destination.ends_with('/') { + format!("{}{filename}", validated_destination) } else { - format!("{destination}/{filename}") + format!("{}/{filename}", validated_destination) } } } else { @@ -133,15 +145,15 @@ pub async fn upload_file( .file_name() .ok_or_else(|| anyhow::anyhow!("Failed to get filename from {:?}", file))? .to_string_lossy(); - if destination.ends_with('/') { - format!("{destination}{filename}") + if validated_destination.ends_with('/') { + format!("{}{filename}", validated_destination) } else { - format!("{destination}/{filename}") + format!("{}/{filename}", validated_destination) } } } else { // Single file to specific destination - destination.to_string() + validated_destination.clone() }; println!( diff --git a/src/executor.rs b/src/executor.rs index 4c168a81..7e5db219 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -212,11 +212,14 @@ impl ParallelExecutor { } } Err(e) => { - let error_msg = format!("{e}"); - let short_error = if error_msg.len() > 40 { - format!("{}...", &error_msg[..37]) + // Get the most specific error message from the chain + let error_msg = format!("{:#}", e); + // Take the first line which is usually the most specific error + let first_line = error_msg.lines().next().unwrap_or("Unknown error"); + let short_error = if first_line.len() > 50 { + format!("{}...", &first_line[..47]) } else { - error_msg + first_line.to_string() }; pb.finish_with_message(format!("{} {}", "●".red(), short_error.red())); } @@ -324,11 +327,14 @@ impl ParallelExecutor { )); } Err(e) => { - let error_msg = format!("{e}"); - let short_error = if error_msg.len() > 40 { - format!("{}...", &error_msg[..37]) + // Get the most specific error message from the chain + let error_msg = format!("{:#}", e); + // Take the first line which is usually the most specific error + let first_line = error_msg.lines().next().unwrap_or("Unknown error"); + let short_error = if first_line.len() > 50 { + format!("{}...", &first_line[..47]) } else { - error_msg + first_line.to_string() }; pb.finish_with_message(format!("{} {}", "●".red(), short_error.red())); } @@ -752,12 +758,16 @@ impl UploadResult { } Err(e) => { println!( - "{} {}: {} - {}", + "{} {}: {}", "●".red(), self.node.to_string().bold(), - "Failed to upload file".red(), - e.to_string().dimmed() + "Failed to upload file".red() ); + // Show full error chain + let error_chain = format!("{:#}", e); + for line in error_chain.lines() { + println!(" {}", line.dimmed()); + } } } } @@ -787,12 +797,16 @@ impl DownloadResult { } Err(e) => { println!( - "{} {}: {} - {}", + "{} {}: {}", "●".red(), self.node.to_string().bold(), - "Failed to download file".red(), - e.to_string().dimmed() + "Failed to download file".red() ); + // Show full error chain + let error_chain = format!("{:#}", e); + for line in error_chain.lines() { + println!(" {}", line.dimmed()); + } } } } diff --git a/src/jump/chain.rs b/src/jump/chain.rs index c506dd33..3c0db79f 100644 --- a/src/jump/chain.rs +++ b/src/jump/chain.rs @@ -845,7 +845,7 @@ impl JumpHostChain { match auth_method { AuthMethod::Password(password) => { let auth_result = handle - .authenticate_password(username, password) + .authenticate_password(username, &**password) .await .map_err(|e| anyhow::anyhow!("Password authentication failed: {}", e))?; @@ -856,7 +856,7 @@ impl JumpHostChain { AuthMethod::PrivateKey { key_data, key_pass } => { let private_key = - russh::keys::decode_secret_key(key_data.as_str(), key_pass.as_deref()) + russh::keys::decode_secret_key(&key_data, key_pass.as_ref().map(|p| &***p)) .map_err(|e| anyhow::anyhow!("Failed to decode private key: {}", e))?; let auth_result = handle @@ -879,8 +879,11 @@ impl JumpHostChain { key_file_path, key_pass, } => { - let private_key = russh::keys::load_secret_key(key_file_path, key_pass.as_deref()) - .map_err(|e| anyhow::anyhow!("Failed to load private key from file: {}", e))?; + let private_key = russh::keys::load_secret_key( + key_file_path, + key_pass.as_ref().map(|p| &***p), + ) + .map_err(|e| anyhow::anyhow!("Failed to load private key from file: {}", e))?; let auth_result = handle .authenticate_publickey( diff --git a/src/lib.rs b/src/lib.rs index b6f01293..011174c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -20,6 +20,7 @@ pub mod forwarding; pub mod jump; pub mod node; pub mod pty; +pub mod security; pub mod ssh; pub mod ui; pub mod utils; diff --git a/src/main.rs b/src/main.rs index d6f8cb8f..3e64732d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -42,7 +42,7 @@ fn show_usage() { println!(" [--output-dir dir] [--timeout seconds] [--use-agent]"); println!(" destination [command [argument ...]]"); println!(" bssh [-Q query_option]"); - println!(" bssh [exec|list|ping|upload|download|interactive] ..."); + println!(" bssh [list|ping|upload|download|interactive] ..."); println!(); println!("SSH Config Support:"); println!(" -F ssh_configfile Use alternative SSH configuration file"); @@ -140,7 +140,9 @@ async fn main() -> Result<()> { }; // Handle list command first (doesn't need nodes) - if matches!(cli.command, Some(Commands::List)) { + if matches!(cli.command, Some(Commands::List)) + || (cli.is_multi_server_mode() && cli.destination.as_deref() == Some("list")) + { list_clusters(&config); return Ok(()); } @@ -215,10 +217,10 @@ async fn main() -> Result<()> { // Get command to execute let command = cli.get_command(); - // Check if command is required (not for subcommands like ping, copy) - // In SSH mode without a command, we start an interactive session - let needs_command = - matches!(cli.command, None | Some(Commands::Exec { .. })) && !cli.is_ssh_mode(); + // Check if command is required + // Auto-exec happens when in multi-server mode with command_args + let is_auto_exec = cli.should_auto_exec(); + let needs_command = (cli.command.is_none() || is_auto_exec) && !cli.is_ssh_mode(); if command.is_empty() && needs_command && !cli.force_tty { anyhow::bail!( "No command specified. Please provide a command to execute.\nExample: bssh -H host1,host2 'ls -la'" @@ -233,6 +235,14 @@ async fn main() -> Result<()> { }; // Handle remaining commands + // Check if destination is a subcommand in multi-server mode + // Check if destination is a subcommand in multi-server mode + let _dest_as_subcommand = if cli.is_multi_server_mode() { + cli.destination.as_deref() + } else { + None + }; + match cli.command { Some(Commands::Ping) => { // Determine SSH key path with SSH config integration @@ -402,7 +412,7 @@ async fn main() -> Result<()> { Ok(()) } _ => { - // Execute command (default or Exec subcommand) or interactive shell + // Execute command (auto-exec or interactive shell) // In SSH mode without command, start interactive session if cli.is_ssh_mode() && command.is_empty() { // SSH mode interactive session (like ssh user@host) @@ -516,6 +526,22 @@ async fn main() -> Result<()> { /// Parse a node string with SSH config integration fn parse_node_with_ssh_config(node_str: &str, ssh_config: &SshConfig) -> Result { + // Security: Validate the node string to prevent injection attacks + if node_str.is_empty() { + anyhow::bail!("Node string cannot be empty"); + } + + // Check for dangerous characters that could cause issues + if node_str.contains(';') + || node_str.contains('&') + || node_str.contains('|') + || node_str.contains('`') + || node_str.contains('$') + || node_str.contains('\n') + { + anyhow::bail!("Node string contains invalid characters"); + } + // First parse the raw node string to extract user, host, port from CLI let (user_part, host_part) = if let Some(at_pos) = node_str.find('@') { let user = &node_str[..at_pos]; @@ -534,8 +560,18 @@ fn parse_node_with_ssh_config(node_str: &str, ssh_config: &SshConfig) -> Result< (host_part, None) }; + // Security: Validate hostname + let validated_host = bssh::security::validate_hostname(raw_host) + .with_context(|| format!("Invalid hostname in node: {}", raw_host))?; + + // Security: Validate username if provided + if let Some(user) = user_part { + bssh::security::validate_username(user) + .with_context(|| format!("Invalid username in node: {}", user))?; + } + // Now resolve using SSH config with CLI taking precedence - let effective_hostname = ssh_config.get_effective_hostname(raw_host); + let effective_hostname = ssh_config.get_effective_hostname(&validated_host); let effective_user = if let Some(user) = user_part { user.to_string() } else if let Some(ssh_user) = ssh_config.get_effective_user(raw_host, None) { @@ -674,9 +710,105 @@ async fn resolve_nodes( } } + // Apply host filter if destination is used as a filter pattern + if let Some(filter) = cli.get_host_filter() { + nodes = filter_nodes(nodes, filter)?; + if nodes.is_empty() { + anyhow::bail!("No hosts matched the filter pattern: {}", filter); + } + } + Ok((nodes, cluster_name)) } +/// Filter nodes based on a pattern (supports wildcards) +fn filter_nodes(nodes: Vec, pattern: &str) -> Result> { + use glob::Pattern; + + // Security: Validate pattern length to prevent DoS + const MAX_PATTERN_LENGTH: usize = 256; + if pattern.len() > MAX_PATTERN_LENGTH { + anyhow::bail!( + "Filter pattern too long (max {} characters)", + MAX_PATTERN_LENGTH + ); + } + + // Security: Validate pattern for dangerous constructs + if pattern.is_empty() { + anyhow::bail!("Filter pattern cannot be empty"); + } + + // Security: Prevent excessive wildcard usage that could cause DoS + let wildcard_count = pattern.chars().filter(|c| *c == '*' || *c == '?').count(); + const MAX_WILDCARDS: usize = 10; + if wildcard_count > MAX_WILDCARDS { + anyhow::bail!( + "Filter pattern contains too many wildcards (max {})", + MAX_WILDCARDS + ); + } + + // Security: Check for potential path traversal attempts + if pattern.contains("..") || pattern.contains("//") { + anyhow::bail!("Filter pattern contains invalid sequences"); + } + + // Security: Sanitize pattern - only allow safe characters for hostnames + // Allow alphanumeric, dots, hyphens, underscores, wildcards, and brackets + let valid_chars = pattern.chars().all(|c| { + c.is_ascii_alphanumeric() + || c == '.' + || c == '-' + || c == '_' + || c == '@' + || c == ':' + || c == '*' + || c == '?' + || c == '[' + || c == ']' + }); + + if !valid_chars { + anyhow::bail!("Filter pattern contains invalid characters for hostname matching"); + } + + // If pattern contains wildcards, use glob matching + if pattern.contains('*') || pattern.contains('?') || pattern.contains('[') { + // Security: Compile pattern with timeout to prevent ReDoS attacks + let glob_pattern = Pattern::new(pattern) + .with_context(|| format!("Invalid filter pattern: {}", pattern))?; + + // Performance: Use HashSet for O(1) lookups if we need to check many nodes + let mut matched_nodes = Vec::with_capacity(nodes.len()); + + for node in nodes { + // Security: Limit matching to prevent excessive computation + let host_matches = glob_pattern.matches(&node.host); + let full_matches = if !host_matches { + glob_pattern.matches(&node.to_string()) + } else { + true + }; + + if host_matches || full_matches { + matched_nodes.push(node); + } + } + + Ok(matched_nodes) + } else { + // Exact match: check hostname, full node string, or partial match + // Performance: Pre-compute pattern once for contains check + Ok(nodes + .into_iter() + .filter(|node| { + node.host == pattern || node.to_string() == pattern || node.host.contains(pattern) + }) + .collect()) + } +} + /// Handle cache statistics command async fn handle_cache_stats(detailed: bool, clear: bool, maintain: bool) { use bssh::ssh::GLOBAL_CACHE; diff --git a/src/security.rs b/src/security.rs new file mode 100644 index 00000000..f4533253 --- /dev/null +++ b/src/security.rs @@ -0,0 +1,342 @@ +// 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. + +//! Security utilities for validating and sanitizing user input + +use anyhow::{Context, Result}; +use std::path::{Path, PathBuf}; + +/// Validate and sanitize a local file path to prevent path traversal attacks +/// +/// This function ensures: +/// - No path traversal sequences (..) +/// - No double slashes (//) +/// - Path is canonical and resolved +/// - No symlink attacks +pub fn validate_local_path(path: &Path) -> Result { + // Convert to string to check for dangerous patterns + let path_str = path.to_string_lossy(); + + // Check for path traversal attempts + if path_str.contains("..") { + anyhow::bail!("Path traversal detected: path contains '..'"); + } + + // Check for double slashes + if path_str.contains("//") { + anyhow::bail!("Invalid path: contains double slashes"); + } + + // Get canonical path (resolves symlinks, .., ., etc.) + // This will fail if the path doesn't exist yet, so we handle that case + let canonical = if path.exists() { + path.canonicalize() + .with_context(|| format!("Failed to canonicalize path: {:?}", path))? + } else { + // For non-existent paths, validate the parent directory + if let Some(parent) = path.parent() { + if parent.as_os_str().is_empty() { + // Parent is empty, use current directory + std::env::current_dir() + .with_context(|| "Failed to get current directory")? + .join(path) + } else if parent.exists() { + let canonical_parent = parent + .canonicalize() + .with_context(|| format!("Failed to canonicalize parent path: {:?}", parent))?; + + // Get the file name + let file_name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("Invalid path: no file name component"))?; + + // Validate file name doesn't contain path separators + let file_name_str = file_name.to_string_lossy(); + if file_name_str.contains('/') || file_name_str.contains('\\') { + anyhow::bail!("Invalid file name: contains path separator"); + } + + canonical_parent.join(file_name) + } else { + // Parent doesn't exist, recursively create and validate + validate_local_path(parent)?; + validate_local_path(path)? + } + } else { + // No parent, treat as relative to current directory + std::env::current_dir() + .with_context(|| "Failed to get current directory")? + .join(path) + } + }; + + Ok(canonical) +} + +/// Validate a remote path string to prevent injection attacks +/// +/// This function ensures: +/// - No shell metacharacters that could cause command injection +/// - No path traversal sequences +/// - Only valid characters for file paths +pub fn validate_remote_path(path: &str) -> Result { + // Check for empty path + if path.is_empty() { + anyhow::bail!("Remote path cannot be empty"); + } + + // Check path length to prevent DoS + const MAX_PATH_LENGTH: usize = 4096; + if path.len() > MAX_PATH_LENGTH { + anyhow::bail!("Remote path too long (max {} characters)", MAX_PATH_LENGTH); + } + + // Check for shell metacharacters that could cause injection + const DANGEROUS_CHARS: &[char] = &[ + ';', '&', '|', '`', '$', '(', ')', '{', '}', '<', '>', '\n', '\r', '\0', '!', '*', '?', + '[', ']', // Shell wildcards that could cause issues + ]; + + for &ch in DANGEROUS_CHARS { + if path.contains(ch) { + anyhow::bail!("Remote path contains invalid character: '{}'", ch); + } + } + + // Check for command substitution patterns + if path.contains("$(") || path.contains("${") || path.contains("`)") { + anyhow::bail!("Remote path contains potential command substitution"); + } + + // Check for path traversal + if path.contains("../") || path.starts_with("..") || path.ends_with("..") { + anyhow::bail!("Remote path contains path traversal sequence"); + } + + // Check for double slashes (could indicate protocol bypasses) + if path.contains("//") && !path.starts_with("//") { + anyhow::bail!("Remote path contains double slashes"); + } + + // Validate that path contains only allowed characters + // Allow: alphanumeric, spaces, and common path characters + let valid_chars = path.chars().all(|c| { + c.is_ascii_alphanumeric() + || c == '/' + || c == '\\' + || c == '.' + || c == '-' + || c == '_' + || c == ' ' + || c == '~' + || c == '=' + || c == ',' + || c == ':' + || c == '@' + }); + + if !valid_chars { + anyhow::bail!("Remote path contains invalid characters"); + } + + Ok(path.to_string()) +} + +/// Sanitize a hostname to prevent injection attacks +pub fn validate_hostname(hostname: &str) -> Result { + // Check for empty hostname + if hostname.is_empty() { + anyhow::bail!("Hostname cannot be empty"); + } + + // Check hostname length (RFC 1123) + const MAX_HOSTNAME_LENGTH: usize = 253; + if hostname.len() > MAX_HOSTNAME_LENGTH { + anyhow::bail!("Hostname too long (max {} characters)", MAX_HOSTNAME_LENGTH); + } + + // Validate hostname format (RFC 1123) + // Allow alphanumeric, dots, hyphens, and colons (for IPv6) + let valid_chars = hostname.chars().all(|c| { + c.is_ascii_alphanumeric() || c == '.' || c == '-' || c == ':' || c == '[' || c == ']' + }); + + if !valid_chars { + anyhow::bail!("Hostname contains invalid characters"); + } + + // Check for suspicious patterns + if hostname.contains("..") || hostname.contains("--") { + anyhow::bail!("Hostname contains suspicious repeated characters"); + } + + Ok(hostname.to_string()) +} + +/// Validate a username to prevent injection attacks +pub fn validate_username(username: &str) -> Result { + // Check for empty username + if username.is_empty() { + anyhow::bail!("Username cannot be empty"); + } + + // Check username length + const MAX_USERNAME_LENGTH: usize = 32; + if username.len() > MAX_USERNAME_LENGTH { + anyhow::bail!("Username too long (max {} characters)", MAX_USERNAME_LENGTH); + } + + // Validate username format (POSIX-compliant) + // Allow alphanumeric, underscore, hyphen, and dot + let valid_chars = username + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-' || c == '.'); + + if !valid_chars { + anyhow::bail!("Username contains invalid characters"); + } + + // Username should not start with a hyphen + if username.starts_with('-') { + anyhow::bail!("Username cannot start with a hyphen"); + } + + Ok(username.to_string()) +} + +/// Sanitize error messages to prevent information leakage +/// +/// This function redacts sensitive information like usernames, hostnames, +/// and ports from error messages to prevent information disclosure. +pub fn sanitize_error_message(message: &str) -> String { + // Replace specific user mentions with generic text + let mut sanitized = message.to_string(); + + // Remove specific usernames (format: user 'username') + if let Some(start) = sanitized.find("user '") { + if let Some(end) = sanitized[start + 6..].find('\'') { + let before = &sanitized[..start + 5]; + let after = &sanitized[start + 6 + end + 1..]; + sanitized = format!("{}{}", before, after); + } + } + + // Remove hostname:port combinations + // Match patterns like "on hostname:port" or "to hostname:port" + let re_patterns = [ + r" on [a-zA-Z0-9\.\-]+:[0-9]+", + r" to [a-zA-Z0-9\.\-]+:[0-9]+", + r" at [a-zA-Z0-9\.\-]+:[0-9]+", + r" from [a-zA-Z0-9\.\-]+:[0-9]+", + ]; + + for _pattern in &re_patterns { + // Simple pattern matching without regex for security + // This is a simplified approach - in production, consider using a proper regex library + if sanitized.contains(" on ") + || sanitized.contains(" to ") + || sanitized.contains(" at ") + || sanitized.contains(" from ") + { + // Replace with generic message + sanitized = sanitized + .replace(" on ", " on ") + .replace(" to ", " to ") + .replace(" at ", " at ") + .replace(" from ", " from "); + } + } + + // Remove any remaining IP addresses + // Simple check for IPv4 pattern + let parts: Vec<&str> = sanitized.split_whitespace().collect(); + let mut result_parts = Vec::new(); + + for part in parts { + if part.split('.').count() == 4 + && part + .split('.') + .all(|p| p.parse::().is_ok() || p.contains(':')) + { + result_parts.push(""); + } else { + result_parts.push(part); + } + } + + result_parts.join(" ") +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_validate_local_path() { + // Valid paths + assert!(validate_local_path(Path::new("/tmp/test.txt")).is_ok()); + assert!(validate_local_path(Path::new("./test.txt")).is_ok()); + + // Invalid paths with traversal + assert!(validate_local_path(Path::new("../etc/passwd")).is_err()); + assert!(validate_local_path(Path::new("/tmp/../etc/passwd")).is_err()); + assert!(validate_local_path(Path::new("/tmp//test")).is_err()); + } + + #[test] + fn test_validate_remote_path() { + // Valid paths + assert!(validate_remote_path("/home/user/file.txt").is_ok()); + assert!(validate_remote_path("~/documents/report.pdf").is_ok()); + assert!(validate_remote_path("C:\\Users\\test\\file.txt").is_ok()); + + // Invalid paths + assert!(validate_remote_path("../etc/passwd").is_err()); + assert!(validate_remote_path("/tmp/$(whoami)").is_err()); + assert!(validate_remote_path("/tmp/test; rm -rf /").is_err()); + assert!(validate_remote_path("/tmp/test`id`").is_err()); + assert!(validate_remote_path("/tmp/test|cat").is_err()); + assert!(validate_remote_path("").is_err()); + } + + #[test] + fn test_validate_hostname() { + // Valid hostnames + assert!(validate_hostname("example.com").is_ok()); + assert!(validate_hostname("192.168.1.1").is_ok()); + assert!(validate_hostname("server-01.example.com").is_ok()); + assert!(validate_hostname("[::1]").is_ok()); + + // Invalid hostnames + assert!(validate_hostname("example..com").is_err()); + assert!(validate_hostname("server--01").is_err()); + assert!(validate_hostname("example.com; ls").is_err()); + assert!(validate_hostname("").is_err()); + } + + #[test] + fn test_validate_username() { + // Valid usernames + assert!(validate_username("john_doe").is_ok()); + assert!(validate_username("user123").is_ok()); + assert!(validate_username("test.user").is_ok()); + + // Invalid usernames + assert!(validate_username("-user").is_err()); + assert!(validate_username("user@domain").is_err()); + assert!(validate_username("user name").is_err()); + assert!(validate_username("").is_err()); + assert!(validate_username(&"a".repeat(50)).is_err()); + } +} diff --git a/src/ssh/client.rs b/src/ssh/client.rs index df483378..9f3f982d 100644 --- a/src/ssh/client.rs +++ b/src/ssh/client.rs @@ -205,13 +205,52 @@ impl SshClient { const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; let connect_timeout = Duration::from_secs(SSH_CONNECT_TIMEOUT_SECS); - tokio::time::timeout( + match tokio::time::timeout( connect_timeout, - Client::connect(addr, &self.username, auth_method.clone(), check_method) + Client::connect(addr, &self.username, auth_method.clone(), check_method), ) .await - .with_context(|| format!("Connection timeout: Failed to connect to {}:{} after 30 seconds. Please check if the host is reachable and SSH service is running.", self.host, self.port))? - .with_context(|| format!("SSH connection failed to {}:{}. Please verify the hostname, port, and authentication credentials.", self.host, self.port)) + { + Ok(Ok(client)) => Ok(client), + Ok(Err(e)) => { + // Specific error from the SSH connection attempt + let error_msg = match &e { + super::tokio_client::Error::KeyAuthFailed => { + "Authentication failed. The private key was rejected by the server.".to_string() + } + super::tokio_client::Error::PasswordWrong => { + "Password authentication failed.".to_string() + } + super::tokio_client::Error::ServerCheckFailed => { + "Host key verification failed. The server's host key was not recognized or has changed.".to_string() + } + super::tokio_client::Error::KeyInvalid(key_err) => { + format!("Failed to load SSH key: {}. Please check the key file format and passphrase.", key_err) + } + super::tokio_client::Error::AgentConnectionFailed => { + "Failed to connect to SSH agent. Please ensure SSH_AUTH_SOCK is set and the agent is running.".to_string() + } + super::tokio_client::Error::AgentNoIdentities => { + "SSH agent has no identities. Please add your key to the agent using 'ssh-add'.".to_string() + } + super::tokio_client::Error::AgentAuthenticationFailed => { + "SSH agent authentication failed.".to_string() + } + super::tokio_client::Error::SshError(ssh_err) => { + format!("SSH connection error: {}", ssh_err) + } + _ => { + format!("Failed to connect: {}", e) + } + }; + Err(anyhow::anyhow!(error_msg).context(e)) + } + Err(_) => Err(anyhow::anyhow!( + "Connection timeout after {} seconds. \ + Please check if the host is reachable and SSH service is running.", + SSH_CONNECT_TIMEOUT_SECS + )), + } } /// Create an SSH connection through jump hosts @@ -286,13 +325,57 @@ impl SshClient { // - Balances user patience with reliability on poor networks const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; let connect_timeout = Duration::from_secs(SSH_CONNECT_TIMEOUT_SECS); - let client = tokio::time::timeout( + let client = match tokio::time::timeout( connect_timeout, - Client::connect(addr, &self.username, auth_method, check_method) + Client::connect(addr, &self.username, auth_method, check_method), ) .await - .with_context(|| format!("Connection timeout: Failed to connect to {}:{} after 30 seconds. Please check if the host is reachable and SSH service is running.", self.host, self.port))? - .with_context(|| format!("SSH connection failed to {}:{}. Please verify the hostname, port, and authentication credentials.", self.host, self.port))?; + { + Ok(Ok(client)) => client, + Ok(Err(e)) => { + let context = format!("SSH connection to {}:{}", self.host, self.port); + let detailed = match &e { + super::tokio_client::Error::KeyAuthFailed => { + format!( + "{} failed: Authentication rejected with provided SSH key", + context + ) + } + super::tokio_client::Error::KeyInvalid(err) => { + format!("{} failed: Invalid SSH key - {}", context, err) + } + super::tokio_client::Error::ServerCheckFailed => { + format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + } + super::tokio_client::Error::PasswordWrong => { + format!("{} failed: Password authentication rejected", context) + } + super::tokio_client::Error::AgentConnectionFailed => { + format!( + "{} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set.", + context + ) + } + super::tokio_client::Error::AgentNoIdentities => { + format!( + "{} failed: SSH agent has no keys. Use 'ssh-add' to add your key.", + context + ) + } + super::tokio_client::Error::AgentAuthenticationFailed => { + format!("{} failed: SSH agent authentication rejected", context) + } + _ => format!("{} failed: {}", context, e), + }; + return Err(anyhow::anyhow!(detailed).context(e)); + } + Err(_) => { + return Err(anyhow::anyhow!( + "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", + SSH_CONNECT_TIMEOUT_SECS + )); + } + }; tracing::debug!("Connected and authenticated successfully"); @@ -377,13 +460,57 @@ impl SshClient { // - Balances user patience with reliability on poor networks const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; let connect_timeout = Duration::from_secs(SSH_CONNECT_TIMEOUT_SECS); - let client = tokio::time::timeout( + let client = match tokio::time::timeout( connect_timeout, - Client::connect(addr, &self.username, auth_method, check_method) + Client::connect(addr, &self.username, auth_method, check_method), ) .await - .with_context(|| format!("Connection timeout: Failed to connect to {}:{} after 30 seconds. Please check if the host is reachable and SSH service is running.", self.host, self.port))? - .with_context(|| format!("SSH connection failed to {}:{}. Please verify the hostname, port, and authentication credentials.", self.host, self.port))?; + { + Ok(Ok(client)) => client, + Ok(Err(e)) => { + let context = format!("SSH connection to {}:{}", self.host, self.port); + let detailed = match &e { + super::tokio_client::Error::KeyAuthFailed => { + format!( + "{} failed: Authentication rejected with provided SSH key", + context + ) + } + super::tokio_client::Error::KeyInvalid(err) => { + format!("{} failed: Invalid SSH key - {}", context, err) + } + super::tokio_client::Error::ServerCheckFailed => { + format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + } + super::tokio_client::Error::PasswordWrong => { + format!("{} failed: Password authentication rejected", context) + } + super::tokio_client::Error::AgentConnectionFailed => { + format!( + "{} failed: Cannot connect to SSH agent. Ensure SSH_AUTH_SOCK is set.", + context + ) + } + super::tokio_client::Error::AgentNoIdentities => { + format!( + "{} failed: SSH agent has no keys. Use 'ssh-add' to add your key.", + context + ) + } + super::tokio_client::Error::AgentAuthenticationFailed => { + format!("{} failed: SSH agent authentication rejected", context) + } + _ => format!("{} failed: {}", context, e), + }; + return Err(anyhow::anyhow!(detailed).context(e)); + } + Err(_) => { + return Err(anyhow::anyhow!( + "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", + SSH_CONNECT_TIMEOUT_SECS + )); + } + }; tracing::debug!("Connected and authenticated successfully"); @@ -464,13 +591,42 @@ impl SshClient { // - Balances user patience with reliability on poor networks const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; let connect_timeout = Duration::from_secs(SSH_CONNECT_TIMEOUT_SECS); - let client = tokio::time::timeout( + let client = match tokio::time::timeout( connect_timeout, Client::connect(addr, &self.username, auth_method, check_method), ) .await - .with_context(|| format!("Connection timeout: Failed to connect to {}:{} after 30 seconds. Please check if the host is reachable and SSH service is running.", self.host, self.port))? - .with_context(|| format!("SSH connection failed to {}:{}. Please verify the hostname, port, and authentication credentials.", self.host, self.port))?; + { + Ok(Ok(client)) => client, + Ok(Err(e)) => { + let context = format!("SSH connection to {}:{}", self.host, self.port); + let detailed = match &e { + super::tokio_client::Error::KeyAuthFailed => { + format!( + "{} failed: Authentication rejected with provided SSH key", + context + ) + } + super::tokio_client::Error::KeyInvalid(err) => { + format!("{} failed: Invalid SSH key - {}", context, err) + } + super::tokio_client::Error::ServerCheckFailed => { + format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + } + super::tokio_client::Error::PasswordWrong => { + format!("{} failed: Password authentication rejected", context) + } + _ => format!("{} failed: {}", context, e), + }; + return Err(anyhow::anyhow!(detailed).context(e)); + } + Err(_) => { + return Err(anyhow::anyhow!( + "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", + SSH_CONNECT_TIMEOUT_SECS + )); + } + }; tracing::debug!("Connected and authenticated successfully"); @@ -554,13 +710,42 @@ impl SshClient { // - Balances user patience with reliability on poor networks const SSH_CONNECT_TIMEOUT_SECS: u64 = 30; let connect_timeout = Duration::from_secs(SSH_CONNECT_TIMEOUT_SECS); - let client = tokio::time::timeout( + let client = match tokio::time::timeout( connect_timeout, Client::connect(addr, &self.username, auth_method, check_method), ) .await - .with_context(|| format!("Connection timeout: Failed to connect to {}:{} after 30 seconds. Please check if the host is reachable and SSH service is running.", self.host, self.port))? - .with_context(|| format!("SSH connection failed to {}:{}. Please verify the hostname, port, and authentication credentials.", self.host, self.port))?; + { + Ok(Ok(client)) => client, + Ok(Err(e)) => { + let context = format!("SSH connection to {}:{}", self.host, self.port); + let detailed = match &e { + super::tokio_client::Error::KeyAuthFailed => { + format!( + "{} failed: Authentication rejected with provided SSH key", + context + ) + } + super::tokio_client::Error::KeyInvalid(err) => { + format!("{} failed: Invalid SSH key - {}", context, err) + } + super::tokio_client::Error::ServerCheckFailed => { + format!("{} failed: Host key verification failed. The server's host key is not trusted.", context) + } + super::tokio_client::Error::PasswordWrong => { + format!("{} failed: Password authentication rejected", context) + } + _ => format!("{} failed: {}", context, e), + }; + return Err(anyhow::anyhow!(detailed).context(e)); + } + Err(_) => { + return Err(anyhow::anyhow!( + "Connection timeout after {} seconds. Host may be unreachable or SSH service not running.", + SSH_CONNECT_TIMEOUT_SECS + )); + } + }; tracing::debug!("Connected and authenticated successfully"); diff --git a/src/ssh/tokio_client/client.rs b/src/ssh/tokio_client/client.rs index 07c19e80..87e72e72 100644 --- a/src/ssh/tokio_client/client.rs +++ b/src/ssh/tokio_client/client.rs @@ -9,6 +9,7 @@ use std::sync::Arc; use std::{fmt::Debug, path::Path}; use std::{io, path::PathBuf}; use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use zeroize::Zeroizing; use super::ToSocketAddrsWithHostname; use crate::utils::buffer_pool::global; @@ -47,18 +48,18 @@ const SSH_RESPONSE_BUFFER_SIZE: usize = 1024; /// /// Used when creating a [`Client`] for authentification. /// Supports password, private key, public key, SSH agent, and keyboard interactive authentication. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum AuthMethod { - Password(String), + Password(Zeroizing), PrivateKey { /// entire contents of private key file - key_data: String, - key_pass: Option, + key_data: Zeroizing, + key_pass: Option>, }, PrivateKeyFile { key_file_path: PathBuf, - key_pass: Option, + key_pass: Option>, }, #[cfg(not(target_os = "windows"))] PublicKeyFile { @@ -69,14 +70,14 @@ pub enum AuthMethod { KeyboardInteractive(AuthKeyboardInteractive), } -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq)] struct PromptResponse { exact: bool, prompt: String, - response: String, + response: Zeroizing, } -#[derive(Debug, Clone, PartialEq, Eq, Hash, Default)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] #[non_exhaustive] pub struct AuthKeyboardInteractive { /// Hnts to the server the preferred methods to be used for authentication. @@ -98,20 +99,20 @@ pub enum ServerCheckMethod { impl AuthMethod { /// Convenience method to create a [`AuthMethod`] from a string literal. pub fn with_password(password: &str) -> Self { - Self::Password(password.to_string()) + Self::Password(Zeroizing::new(password.to_string())) } pub fn with_key(key: &str, passphrase: Option<&str>) -> Self { Self::PrivateKey { - key_data: key.to_string(), - key_pass: passphrase.map(str::to_string), + key_data: Zeroizing::new(key.to_string()), + key_pass: passphrase.map(|p| Zeroizing::new(p.to_string())), } } pub fn with_key_file>(key_file_path: T, passphrase: Option<&str>) -> Self { Self::PrivateKeyFile { key_file_path: key_file_path.as_ref().to_path_buf(), - key_pass: passphrase.map(str::to_string), + key_pass: passphrase.map(|p| Zeroizing::new(p.to_string())), } } @@ -165,7 +166,7 @@ impl AuthKeyboardInteractive { self.responses.push(PromptResponse { exact: false, prompt: prompt.into(), - response: response.into(), + response: Zeroizing::new(response.into()), }); self @@ -180,7 +181,7 @@ impl AuthKeyboardInteractive { self.responses.push(PromptResponse { exact: true, prompt: prompt.into(), - response: response.into(), + response: Zeroizing::new(response.into()), }); self @@ -350,14 +351,16 @@ impl Client { ) -> Result<(), super::Error> { match auth { AuthMethod::Password(password) => { - let is_authentificated = handle.authenticate_password(username, password).await?; + let is_authentificated = + handle.authenticate_password(username, &**password).await?; if !is_authentificated.success() { return Err(super::Error::PasswordWrong); } } AuthMethod::PrivateKey { key_data, key_pass } => { - let cprivk = russh::keys::decode_secret_key(key_data.as_str(), key_pass.as_deref()) - .map_err(super::Error::KeyInvalid)?; + let cprivk = + russh::keys::decode_secret_key(&key_data, key_pass.as_ref().map(|p| &***p)) + .map_err(super::Error::KeyInvalid)?; let is_authentificated = handle .authenticate_publickey( username, @@ -375,8 +378,9 @@ impl Client { key_file_path, key_pass, } => { - let cprivk = russh::keys::load_secret_key(key_file_path, key_pass.as_deref()) - .map_err(super::Error::KeyInvalid)?; + let cprivk = + russh::keys::load_secret_key(key_file_path, key_pass.as_ref().map(|p| &***p)) + .map_err(super::Error::KeyInvalid)?; let is_authentificated = handle .authenticate_publickey( username, @@ -488,7 +492,7 @@ impl Client { )); }; let pr = kbd.responses.remove(pos); - responses.push(pr.response); + responses.push(pr.response.to_string()); } res = handle diff --git a/tests/interactive_test.rs b/tests/interactive_test.rs index 1c376427..be3c9084 100644 --- a/tests/interactive_test.rs +++ b/tests/interactive_test.rs @@ -194,7 +194,7 @@ mod auth_method_tests { fn test_auth_method_creation() { // Test password auth let auth = AuthMethod::with_password("test_password"); - assert_eq!(auth, AuthMethod::Password(String::from("test_password"))); + assert_eq!(auth, AuthMethod::with_password("test_password")); // Test key file auth let auth = AuthMethod::with_key_file("/path/to/key", Some("passphrase")); @@ -204,7 +204,7 @@ mod auth_method_tests { } = auth { assert_eq!(key_file_path.to_str().unwrap(), "/path/to/key"); - assert_eq!(key_pass, Some(String::from("passphrase"))); + assert_eq!(key_pass.as_ref().map(|p| &***p), Some("passphrase")); } else { panic!("Wrong auth method type"); }