From 044f2436f882ae40ef45ba9d01d2079c4749d3eb Mon Sep 17 00:00:00 2001 From: Christian Visintin Date: Sat, 13 May 2023 16:09:37 +0200 Subject: [PATCH] Don't prompt for password if a ssh key is set for that host (#186) * feat: don't ask password if a ssh key is set for that host * fix: User from ssh config was not used as it should * fix: forgot a file --- CHANGELOG.md | 2 + src/activity_manager.rs | 62 +++++++++++++++++++++----- src/filetransfer/builder.rs | 22 ++++++++- src/filetransfer/params.rs | 1 - src/system/sshkey_storage.rs | 18 ++------ src/ui/activities/filetransfer/view.rs | 21 ++++----- src/utils/mod.rs | 1 + src/utils/parser.rs | 21 +++------ src/utils/ssh.rs | 45 +++++++++++++++++++ 9 files changed, 139 insertions(+), 54 deletions(-) create mode 100644 src/utils/ssh.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 269f13f..7c29277 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ Released on ?? - [Issue 153](https://github.com/veeso/termscp/issues/153): show a loading message when loading directory's content - [Issue 176](https://github.com/veeso/termscp/issues/176): debug log is now written to CACHE_DIR - [Issue 173](https://github.com/veeso/termscp/issues/173): allow unknown fields in ssh2 configuration file +- [Issue 175](https://github.com/veeso/termscp/issues/175): don't prompt for password if a ssh key is set for that host +- Fixed an issue that didn't use the `User` specified in ssh2-config ## 0.11.3 diff --git a/src/activity_manager.rs b/src/activity_manager.rs index 2e7c1e5..cb04f2a 100644 --- a/src/activity_manager.rs +++ b/src/activity_manager.rs @@ -7,11 +7,14 @@ use std::path::{Path, PathBuf}; use std::time::Duration; -use crate::filetransfer::FileTransferParams; +use remotefs_ssh::SshKeyStorage as SshKeyStorageTrait; + +use crate::filetransfer::{FileTransferParams, FileTransferProtocol}; use crate::host::{HostError, Localhost}; use crate::system::bookmarks_client::BookmarksClient; use crate::system::config_client::ConfigClient; use crate::system::environment; +use crate::system::sshkey_storage::SshKeyStorage; use crate::system::theme_provider::ThemeProvider; use crate::ui::activities::auth::AuthActivity; use crate::ui::activities::filetransfer::FileTransferActivity; @@ -71,18 +74,37 @@ impl ActivityManager { if params.password_missing() { if let Some(password) = password { params.set_default_secret(password.to_string()); - } else { - match tty::read_secret_from_tty("Password: ") { - Err(err) => return Err(format!("Could not read password: {err}")), - Ok(Some(secret)) => { - debug!( - "Read password from tty: {}", - fmt::shadow_password(secret.as_str()) - ); - params.set_default_secret(secret); - } - Ok(None) => {} + } else if matches!( + params.protocol, + FileTransferProtocol::Scp | FileTransferProtocol::Sftp, + ) && params.params.generic_params().is_some() + { + // * if protocol is SCP or SFTP check whether a SSH key is registered for this remote, in case not ask password + let storage = SshKeyStorage::from(self.context.as_ref().unwrap().config()); + let generic_params = params.params.generic_params().unwrap(); + if storage + .resolve( + &generic_params.address, + &generic_params + .username + .clone() + .unwrap_or(whoami::username()), + ) + .is_none() + { + debug!( + "storage could not find any suitable key for {}... prompting for password", + generic_params.address + ); + self.prompt_password(&mut params)?; + } else { + debug!( + "a key is already set for {}; password is not required", + generic_params.address + ); } + } else { + self.prompt_password(&mut params)?; } } // Put params into the context @@ -90,6 +112,22 @@ impl ActivityManager { Ok(()) } + /// Prompt user for password to set into params. + fn prompt_password(&self, params: &mut FileTransferParams) -> Result<(), String> { + match tty::read_secret_from_tty("Password: ") { + Err(err) => Err(format!("Could not read password: {err}")), + Ok(Some(secret)) => { + debug!( + "Read password from tty: {}", + fmt::shadow_password(secret.as_str()) + ); + params.set_default_secret(secret); + Ok(()) + } + Ok(None) => Ok(()), + } + } + /// Resolve provided bookmark name and set it as file transfer params. /// Returns error if bookmark is not found pub fn resolve_bookmark_name( diff --git a/src/filetransfer/builder.rs b/src/filetransfer/builder.rs index 84a4de0..aef366d 100644 --- a/src/filetransfer/builder.rs +++ b/src/filetransfer/builder.rs @@ -20,6 +20,7 @@ use super::params::{AwsS3Params, GenericProtocolParams, SmbParams}; use super::{FileTransferProtocol, ProtocolParams}; use crate::system::config_client::ConfigClient; use crate::system::sshkey_storage::SshKeyStorage; +use crate::utils::ssh as ssh_utils; /// Remotefs builder pub struct Builder; @@ -155,11 +156,30 @@ impl Builder { /// Build ssh options from generic protocol params and client configuration fn build_ssh_opts(params: GenericProtocolParams, config_client: &ConfigClient) -> SshOpts { - let mut opts = SshOpts::new(params.address) + let mut opts = SshOpts::new(params.address.clone()) .key_storage(Box::new(Self::make_ssh_storage(config_client))) .port(params.port); + //* get username. Case 1 provided in params if let Some(username) = params.username { opts = opts.username(username); + } else if let Some(ssh_config) = config_client.get_ssh_config().and_then(|x| { + //* case 2: found in ssh2 config + debug!("reading ssh config at {}", x); + ssh_utils::parse_ssh2_config(x).ok() + }) { + debug!("no username was provided, checking whether a user is set for this host"); + if let Some(username) = ssh_config.query(¶ms.address).user { + debug!("found username from config: {username}"); + opts = opts.username(username); + } else { + //* case 3: use system username; can't be None + debug!("no username was provided, using current username"); + opts = opts.username(whoami::username()); + } + } else { + //* case 3: use system username; can't be None + debug!("no username was provided, using current username"); + opts = opts.username(whoami::username()); } if let Some(password) = params.password { opts = opts.password(password); diff --git a/src/filetransfer/params.rs b/src/filetransfer/params.rs index ec0544d..76234c6 100644 --- a/src/filetransfer/params.rs +++ b/src/filetransfer/params.rs @@ -107,7 +107,6 @@ impl Default for ProtocolParams { } impl ProtocolParams { - #[cfg(test)] /// Retrieve generic parameters from protocol params if any pub fn generic_params(&self) -> Option<&GenericProtocolParams> { match self { diff --git a/src/system/sshkey_storage.rs b/src/system/sshkey_storage.rs index 6f3d31b..60cf63d 100644 --- a/src/system/sshkey_storage.rs +++ b/src/system/sshkey_storage.rs @@ -7,10 +7,11 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; // Ext -use remotefs_ssh::{SshConfigParseRule, SshKeyStorage as SshKeyStorageTrait}; +use remotefs_ssh::SshKeyStorage as SshKeyStorageTrait; use ssh2_config::SshConfig; use super::config_client::ConfigClient; +use crate::utils::ssh as ssh_utils; #[derive(Default)] pub struct SshKeyStorage { @@ -34,19 +35,6 @@ impl SshKeyStorage { self.hosts.insert(key, p); } - /// Parse ssh2 config - fn parse_ssh2_config(path: &str) -> Result { - use std::fs::File; - use std::io::BufReader; - - let mut reader = File::open(path) - .map_err(|e| format!("failed to open {path}: {e}")) - .map(BufReader::new)?; - SshConfig::default() - .parse(&mut reader, SshConfigParseRule::ALLOW_UNKNOWN_FIELDS) - .map_err(|e| format!("Failed to parse ssh2 config: {e}")) - } - /// Resolve host via termscp ssh keys storage fn resolve_host_in_termscp_storage(&self, host: &str, username: &str) -> Option<&Path> { let key: String = Self::make_mapkey(host, username); @@ -89,7 +77,7 @@ impl From<&ConfigClient> for SshKeyStorage { // read ssh2 config let ssh_config = cfg_client.get_ssh_config().and_then(|x| { debug!("reading ssh config at {}", x); - Self::parse_ssh2_config(x).ok() + ssh_utils::parse_ssh2_config(x).ok() }); let mut hosts: HashMap = HashMap::with_capacity(cfg_client.iter_ssh_keys().count()); diff --git a/src/ui/activities/filetransfer/view.rs b/src/ui/activities/filetransfer/view.rs index c504fde..5658880 100644 --- a/src/ui/activities/filetransfer/view.rs +++ b/src/ui/activities/filetransfer/view.rs @@ -153,7 +153,16 @@ impl FileTransferActivity { self.app.view(&Id::StatusBarLocal, f, status_bar_chunks[0]); self.app.view(&Id::StatusBarRemote, f, status_bar_chunks[1]); // @! Draw popups - if self.app.mounted(&Id::CopyPopup) { + if self.app.mounted(&Id::FatalPopup) { + let popup = Popup( + Size::Percentage(50), + self.calc_popup_height(Id::FatalPopup, f.size().width, f.size().height), + ) + .draw_in(f.size()); + f.render_widget(Clear, popup); + // make popup + self.app.view(&Id::FatalPopup, f, popup); + } else if self.app.mounted(&Id::CopyPopup) { let popup = Popup(Size::Percentage(40), Size::Unit(3)).draw_in(f.size()); f.render_widget(Clear, popup); // make popup @@ -292,15 +301,6 @@ impl FileTransferActivity { f.render_widget(Clear, popup); // make popup self.app.view(&Id::ErrorPopup, f, popup); - } else if self.app.mounted(&Id::FatalPopup) { - let popup = Popup( - Size::Percentage(50), - self.calc_popup_height(Id::FatalPopup, f.size().width, f.size().height), - ) - .draw_in(f.size()); - f.render_widget(Clear, popup); - // make popup - self.app.view(&Id::FatalPopup, f, popup); } else if self.app.mounted(&Id::WaitPopup) { let popup = Popup(Size::Percentage(50), Size::Unit(3)).draw_in(f.size()); f.render_widget(Clear, popup); @@ -360,6 +360,7 @@ impl FileTransferActivity { } pub(super) fn mount_fatal>(&mut self, text: S) { + self.umount_wait(); // Mount let error_color = self.theme().misc_error_dialog; assert!(self diff --git a/src/utils/mod.rs b/src/utils/mod.rs index f3c7661..7fb9c9e 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -9,6 +9,7 @@ pub mod fmt; pub mod parser; pub mod path; pub mod random; +pub mod ssh; pub mod string; pub mod tty; pub mod ui; diff --git a/src/utils/parser.rs b/src/utils/parser.rs index bedbc67..410f8ed 100644 --- a/src/utils/parser.rs +++ b/src/utils/parser.rs @@ -196,16 +196,7 @@ fn parse_generic_remote_opt( match REMOTE_GENERIC_OPT_REGEX.captures(s) { Some(groups) => { // Match user - let username: Option = match groups.get(1) { - Some(group) => Some(group.as_str().to_string()), - None => match protocol { - // If group is empty, set to current user - FileTransferProtocol::Scp | FileTransferProtocol::Sftp => { - Some(whoami::username()) - } - _ => None, - }, - }; + let username = groups.get(1).map(|x| x.as_str().to_string()); // Get address let address: String = match groups.get(2) { Some(group) => group.as_str().to_string(), @@ -437,7 +428,7 @@ mod tests { assert_eq!(result.protocol, FileTransferProtocol::Sftp); assert_eq!(params.address, String::from("172.26.104.1")); assert_eq!(params.port, 22); - assert!(params.username.is_some()); + assert!(params.username.is_none()); // User case let result: FileTransferParams = parse_remote_opt(&String::from("root@172.26.104.1")) .ok() @@ -472,7 +463,7 @@ mod tests { assert_eq!(result.protocol, FileTransferProtocol::Sftp); assert_eq!(params.address, String::from("172.26.104.1")); assert_eq!(params.port, 4022); - assert!(params.username.is_some()); + assert!(params.username.is_none()); assert!(result.entry_directory.is_none()); // Protocol let result: FileTransferParams = parse_remote_opt(&String::from("ftp://172.26.104.1")) @@ -492,7 +483,7 @@ mod tests { assert_eq!(result.protocol, FileTransferProtocol::Sftp); assert_eq!(params.address, String::from("172.26.104.1")); assert_eq!(params.port, 22); // Fallback to sftp default - assert!(params.username.is_some()); // Doesn't fall back + assert!(params.username.is_none()); // Doesn't fall back assert!(result.entry_directory.is_none()); let result: FileTransferParams = parse_remote_opt(&String::from("scp://172.26.104.1")) .ok() @@ -501,7 +492,7 @@ mod tests { assert_eq!(result.protocol, FileTransferProtocol::Scp); assert_eq!(params.address, String::from("172.26.104.1")); assert_eq!(params.port, 22); // Fallback to scp default - assert!(params.username.is_some()); // Doesn't fall back + assert!(params.username.is_none()); // Doesn't fall back assert!(result.entry_directory.is_none()); // Protocol + user let result: FileTransferParams = @@ -539,7 +530,7 @@ mod tests { assert_eq!(result.protocol, FileTransferProtocol::Sftp); assert_eq!(params.address, String::from("172.26.104.1")); assert_eq!(params.port, 22); - assert!(params.username.is_some()); + assert!(params.username.is_none()); assert_eq!(result.entry_directory.unwrap(), PathBuf::from("home")); // All together now let result: FileTransferParams = diff --git a/src/utils/ssh.rs b/src/utils/ssh.rs new file mode 100644 index 0000000..7da56c7 --- /dev/null +++ b/src/utils/ssh.rs @@ -0,0 +1,45 @@ +use ssh2_config::{ParseRule, SshConfig}; + +pub fn parse_ssh2_config(path: &str) -> Result { + use std::fs::File; + use std::io::BufReader; + + let mut reader = File::open(path) + .map_err(|e| format!("failed to open {path}: {e}")) + .map(BufReader::new)?; + SshConfig::default() + .parse(&mut reader, ParseRule::ALLOW_UNKNOWN_FIELDS) + .map_err(|e| format!("Failed to parse ssh2 config: {e}")) +} + +#[cfg(test)] +mod test { + + use crate::utils::{ssh::parse_ssh2_config, test_helpers}; + + #[test] + fn should_parse_ssh2_config() { + let rsa_key = test_helpers::create_sample_file_with_content("ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDErJhQxEI0+VvhlXVUyh+vMCm7aXfCA/g633AG8ezD/5EylwchtAr2JCoBWnxn4zV8nI9dMqOgm0jO4IsXpKOjQojv+0VOH7I+cDlBg0tk4hFlvyyS6YviDAfDDln3jYUM+5QNDfQLaZlH2WvcJ3mkDxLVlI9MBX1BAeSmChLxwAvxALp2ncImNQLzDO9eHcig3dtMrEKkzXQowRW5Y7eUzg2+vvVq4H2DOjWwUndvB5sJkhEfTUVE7ID8ZdGJo60kUb/02dZYj+IbkAnMCsqktk0cg/4XFX82hEfRYFeb1arkysFisPU1DOb6QielL/axeTebVplaouYcXY0pFdJt root@8c50fd4c345a"); + let ssh_config_file = test_helpers::create_sample_file_with_content(format!( + r#" +Host test + HostName 127.0.0.1 + Port 2222 + User test + IdentityFile {} + StrictHostKeyChecking no + UserKnownHostsFile /dev/null +"#, + rsa_key.path().display() + )); + + assert!(parse_ssh2_config( + ssh_config_file + .path() + .to_string_lossy() + .to_string() + .as_str() + ) + .is_ok()); + } +}