From 31f016cc5fa202d4ec0b531fbe2ed5eac604078d Mon Sep 17 00:00:00 2001 From: Dmitry Pankratov Date: Sun, 16 Feb 2025 17:18:59 +0100 Subject: [PATCH] Added `password-factor=N` option to determine which authentication factor is the password. This is helpful when the password is read from the config file or from the keychain. Fixes #78 --- snx-rs-gui/src/settings.rs | 9 +++++++++ snx-rs/src/cmdline.rs | 11 +++++++++++ snx-rs/src/main.rs | 11 ++++++----- snxcore/src/controller.rs | 23 +++++++++++------------ snxcore/src/model.rs | 16 +--------------- snxcore/src/model/params.rs | 4 ++++ snxcore/src/server_info.rs | 1 - 7 files changed, 42 insertions(+), 33 deletions(-) diff --git a/snx-rs-gui/src/settings.rs b/snx-rs-gui/src/settings.rs index 4a2223d..f13f5d3 100644 --- a/snx-rs-gui/src/settings.rs +++ b/snx-rs-gui/src/settings.rs @@ -52,6 +52,7 @@ struct MyWidgets { tunnel_type: gtk::ComboBoxText, user_name: gtk::Entry, password: gtk::Entry, + password_factor: gtk::Entry, no_dns: gtk::CheckButton, search_domains: gtk::Entry, ignored_domains: gtk::Entry, @@ -116,6 +117,7 @@ impl MyWidgets { self.ike_lifetime.text().parse::()?; self.esp_lifetime.text().parse::()?; self.ike_port.text().parse::()?; + self.password_factor.text().parse::()?; let dns_servers = self.dns_servers.text(); if !dns_servers.is_empty() { @@ -175,6 +177,7 @@ impl SettingsDialog { let tunnel_type = gtk::ComboBoxText::builder().build(); let user_name = gtk::Entry::builder().text(¶ms.user_name).build(); let password = gtk::Entry::builder().text(¶ms.password).visibility(false).build(); + let password_factor = gtk::Entry::builder().text(params.password_factor.to_string()).build(); let no_dns = gtk::CheckButton::builder().active(params.no_dns).build(); @@ -399,6 +402,7 @@ impl SettingsDialog { tunnel_type, user_name, password, + password_factor, no_dns, search_domains, ignored_domains, @@ -473,6 +477,7 @@ impl SettingsDialog { }; params.user_name = self.widgets.user_name.text().into(); params.password = self.widgets.password.text().into(); + params.password_factor = self.widgets.password_factor.text().parse()?; params.no_dns = self.widgets.no_dns.is_active(); params.search_domains = self .widgets @@ -746,6 +751,10 @@ impl SettingsDialog { mfa_prompts.pack_start(&self.widgets.mfa_prompts, false, true, 0); misc_box.pack_start(&mfa_prompts, false, true, 6); + let password_factor = self.form_box("Index of password factor, 1..N"); + password_factor.pack_start(&self.widgets.password_factor, false, true, 0); + misc_box.pack_start(&password_factor, false, true, 6); + let no_keychain = self.form_box("Do not store passwords in the keychain"); no_keychain.pack_start(&self.widgets.no_keychain, false, true, 0); misc_box.pack_start(&no_keychain, false, true, 6); diff --git a/snx-rs/src/cmdline.rs b/snx-rs/src/cmdline.rs index 075ff30..c7e505f 100644 --- a/snx-rs/src/cmdline.rs +++ b/snx-rs/src/cmdline.rs @@ -26,6 +26,13 @@ pub struct CmdlineParams { #[clap(long = "password", short = 'p', help = "Password in base64-encoded form")] pub password: Option, + #[clap( + long = "password-factor", + short = 'Y', + help = "Numerical index of the password factor, 1..N [default: 1]" + )] + pub password_factor: Option, + #[clap(long = "config-file", short = 'c', help = "Read parameters from config file")] pub config_file: Option, @@ -225,6 +232,10 @@ impl CmdlineParams { let _ = other.decode_password(); } + if let Some(password_factor) = self.password_factor { + other.password_factor = password_factor; + } + if let Some(log_level) = self.log_level { other.log_level = log_level.to_string(); } diff --git a/snx-rs/src/main.rs b/snx-rs/src/main.rs index 078915b..442d4d8 100644 --- a/snx-rs/src/main.rs +++ b/snx-rs/src/main.rs @@ -139,17 +139,18 @@ async fn main_standalone(params: TunnelParams) -> anyhow::Result<()> { connector.authenticate().await? }; - let mut first_mfa = true; + let mut mfa_index = 0; while let SessionState::PendingChallenge(challenge) = session.state.clone() { match challenge.mfa_type { MfaType::PasswordInput => { + mfa_index += 1; + let prompt = mfa_prompts .pop_front() - .unwrap_or_else(|| AuthPrompt::new_password(&challenge.prompt)); + .unwrap_or_else(|| AuthPrompt::new("", &challenge.prompt)); - let input = if !params.password.is_empty() && first_mfa && prompt.is_password() { - first_mfa = false; + let input = if !params.password.is_empty() && mfa_index == params.password_factor { Ok(params.password.clone()) } else { TtyPrompt.get_secure_input(&prompt) @@ -172,7 +173,7 @@ async fn main_standalone(params: TunnelParams) -> anyhow::Result<()> { session = connector.challenge_code(session, &otp).await?; } MfaType::UserNameInput => { - let prompt = AuthPrompt::new("", "username", &challenge.prompt); + let prompt = AuthPrompt::new("Username is required for authentication", &challenge.prompt); let input = TtyPrompt.get_plain_input(&prompt)?; session = connector.challenge_code(session, &input).await?; } diff --git a/snxcore/src/controller.rs b/snxcore/src/controller.rs index 04b58cf..430f333 100644 --- a/snxcore/src/controller.rs +++ b/snxcore/src/controller.rs @@ -48,7 +48,7 @@ pub struct ServiceController { mfa_prompts: Option>, password_from_keychain: String, username: String, - first_mfa: bool, + mfa_index: usize, browser_controller: B, } @@ -64,7 +64,7 @@ where mfa_prompts: None, password_from_keychain: String::new(), username: String::new(), - first_mfa: true, + mfa_index: 0, browser_controller, } } @@ -105,14 +105,12 @@ where } async fn process_mfa_request(&mut self, mfa: &MfaChallenge) -> anyhow::Result { - let first_mfa = self.first_mfa; - match self.get_mfa_input(mfa).await { Ok(input) => { let result = self.do_challenge_code(input.clone()).await; if result.is_ok() && mfa.mfa_type == MfaType::PasswordInput - && first_mfa + && self.mfa_index == self.params.password_factor && !self.params.no_keychain && !input.is_empty() { @@ -130,23 +128,23 @@ where async fn get_mfa_input(&mut self, mfa: &MfaChallenge) -> anyhow::Result { match mfa.mfa_type { MfaType::PasswordInput => { + self.mfa_index += 1; + let prompt = self .mfa_prompts .as_mut() .and_then(|p| p.pop_front()) - .unwrap_or_else(|| AuthPrompt::new_password(&mfa.prompt)); + .unwrap_or_else(|| AuthPrompt::new("", &mfa.prompt)); - if !self.params.password.is_empty() && self.first_mfa && prompt.is_password() { - self.first_mfa = false; + if !self.params.password.is_empty() && self.mfa_index == self.params.password_factor { Ok(self.params.password.clone()) - } else if !self.password_from_keychain.is_empty() && self.first_mfa && prompt.is_password() { - self.first_mfa = false; + } else if !self.password_from_keychain.is_empty() && self.mfa_index == self.params.password_factor { Ok(self.password_from_keychain.clone()) } else { let prompt = if self.params.server_prompt { prompt } else { - AuthPrompt::new_password(&mfa.prompt) + AuthPrompt::new("", &mfa.prompt) }; let input = self.prompt.get_secure_input(&prompt)?; Ok(input) @@ -169,7 +167,7 @@ where } } MfaType::UserNameInput => { - let prompt = AuthPrompt::new("", "username", &mfa.prompt); + let prompt = AuthPrompt::new("Username is required for authentication", &mfa.prompt); let input = self.prompt.get_plain_input(&prompt)?; self.username = input.clone(); @@ -257,6 +255,7 @@ where } async fn fill_mfa_prompts(&mut self) { + self.mfa_index = 0; self.mfa_prompts .replace(server_info::get_mfa_prompts(&self.params).await.unwrap_or_default()); } diff --git a/snxcore/src/model.rs b/snxcore/src/model.rs index bfd2544..50633f5 100644 --- a/snxcore/src/model.rs +++ b/snxcore/src/model.rs @@ -136,32 +136,18 @@ pub enum TunnelServiceResponse { #[derive(Debug, Clone, PartialEq)] pub struct AuthPrompt { pub header: String, - pub factor_type: String, pub prompt: String, } impl AuthPrompt { - pub fn new(header: H, factor_type: F, prompt: S) -> Self + pub fn new(header: H, prompt: S) -> Self where H: AsRef, - F: AsRef, S: AsRef, { Self { header: header.as_ref().to_owned(), - factor_type: factor_type.as_ref().to_owned(), prompt: prompt.as_ref().to_owned(), } } - - pub fn new_password>(prompt: S) -> Self { - Self { - header: String::new(), - factor_type: "password".to_owned(), - prompt: prompt.as_ref().to_owned(), - } - } - pub fn is_password(&self) -> bool { - self.factor_type == "password" || self.factor_type == "user_defined" - } } diff --git a/snxcore/src/model/params.rs b/snxcore/src/model/params.rs index 5dfa95d..b1eef3a 100644 --- a/snxcore/src/model/params.rs +++ b/snxcore/src/model/params.rs @@ -258,6 +258,7 @@ pub struct TunnelParams { pub server_name: String, pub user_name: String, pub password: String, + pub password_factor: usize, pub log_level: String, pub search_domains: Vec, pub ignore_search_domains: Vec, @@ -299,6 +300,7 @@ impl Default for TunnelParams { server_name: String::new(), user_name: String::new(), password: String::new(), + password_factor: 1, log_level: "off".to_owned(), search_domains: Vec::new(), ignore_search_domains: Vec::new(), @@ -351,6 +353,7 @@ impl TunnelParams { "server-name" => params.server_name = v, "user-name" => params.user_name = v, "password" => params.password = v, + "password-factor" => params.password_factor = v.parse().unwrap_or(1), "log-level" => params.log_level = v, "search-domains" => params.search_domains = v.split(',').map(|s| s.trim().to_owned()).collect(), "ignore-search-domains" => { @@ -413,6 +416,7 @@ impl TunnelParams { "password={}", base64::engine::general_purpose::STANDARD.encode(&self.password) )?; + writeln!(buf, "password-factor={}", self.password_factor)?; writeln!(buf, "search-domains={}", self.search_domains.join(","))?; writeln!(buf, "ignore-search-domains={}", self.ignore_search_domains.join(","))?; writeln!( diff --git a/snxcore/src/server_info.rs b/snxcore/src/server_info.rs index 53e4189..17c9b4e 100644 --- a/snxcore/src/server_info.rs +++ b/snxcore/src/server_info.rs @@ -32,7 +32,6 @@ pub async fn get_mfa_prompts(params: &TunnelParams) -> anyhow::Result map.get("password").map(|label| { AuthPrompt::new( map.get("header").map(ToOwned::to_owned).unwrap_or_default(), - factor.factor_type, format!("{}: ", label), ) }),