From 079546cfbf0e87e2c0cd0192fcf97bf3ee79412d Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Mon, 22 Jul 2024 15:50:11 -0500 Subject: [PATCH] refactor(gui-client): remove abstraction layer over `keyring` dep (#5961) This abstraction existed early in the days of the Linux GUI, before the auth module was fully set up. It's not needed now, both Linux and Windows just sit atop `keyring-rs` ```[tasklist] - [x] Smoke test in Linux VM - [x] Smoke test in Windows VM ``` --- rust/gui-client/src-tauri/src/client/auth.rs | 74 ++++++++++++------- .../src/client/auth/token_storage_keyring.rs | 73 ------------------ rust/gui-client/src-tauri/src/client/gui.rs | 2 +- 3 files changed, 50 insertions(+), 99 deletions(-) delete mode 100644 rust/gui-client/src-tauri/src/client/auth/token_storage_keyring.rs diff --git a/rust/gui-client/src-tauri/src/client/auth.rs b/rust/gui-client/src-tauri/src/client/auth.rs index 6fc33b469..d4d94ee8a 100644 --- a/rust/gui-client/src-tauri/src/client/auth.rs +++ b/rust/gui-client/src-tauri/src/client/auth.rs @@ -1,5 +1,6 @@ //! Fulfills +use anyhow::Result; use firezone_headless_client::known_dirs; use rand::{thread_rng, RngCore}; use secrecy::{ExposeSecret, SecretString}; @@ -7,10 +8,6 @@ use std::path::PathBuf; use subtle::ConstantTimeEq; use url::Url; -mod token_storage_keyring; - -use token_storage_keyring::TokenStorage; - const NONCE_LENGTH: usize = 32; #[derive(thiserror::Error, Debug)] @@ -35,11 +32,9 @@ pub(crate) enum Error { WriteActorName(std::io::Error), } -type Result = std::result::Result; - pub(crate) struct Auth { /// Implementation details in case we need to disable `keyring-rs` - token_store: TokenStorage, + token_store: keyring::Entry, state: State, } @@ -85,15 +80,16 @@ impl Auth { /// Creates a new Auth struct using the "dev.firezone.client/token" keyring key. If the token is stored on disk, the struct is automatically signed in. /// /// Performs I/O. - pub fn new() -> Self { + pub fn new() -> Result { Self::new_with_key("dev.firezone.client/token") } /// Creates a new Auth struct with a custom keyring key for testing. /// /// `new` also just wraps this. - fn new_with_key(keyring_key: &'static str) -> Self { - let token_store = TokenStorage::new(keyring_key); + fn new_with_key(keyring_key: &'static str) -> Result { + // The 2nd and 3rd args are ignored on some platforms, so don't use them + let token_store = keyring::Entry::new_with_target(keyring_key, "", "")?; let mut this = Self { token_store, state: State::SignedOut, @@ -111,7 +107,7 @@ impl Auth { Ok(None) => tracing::info!("No token on disk, starting in signed-out state."), } - this + Ok(this) } /// Returns the session iff we are signed in. @@ -125,8 +121,8 @@ impl Auth { /// Mark the session as signed out, or cancel an ongoing sign-in flow /// /// Performs I/O. - pub fn sign_out(&mut self) -> Result<()> { - if let Err(error) = self.token_store.delete() { + pub fn sign_out(&mut self) -> Result<(), Error> { + if let Err(error) = self.token_store.delete_password() { tracing::warn!(?error, "Couldn't delete token while signing out"); } if let Err(error) = std::fs::remove_file(actor_name_path()?) { @@ -143,7 +139,7 @@ impl Auth { /// /// Returns parameters used to make a URL for the web browser to open /// May return Ok(None) if we're already signed in - pub fn start_sign_in(&mut self) -> Result> { + pub fn start_sign_in(&mut self) -> Result, Error> { self.sign_out()?; self.state = State::NeedResponse(Request { nonce: generate_nonce(), @@ -158,7 +154,7 @@ impl Auth { /// Performs I/O. /// /// Errors if we don't have any ongoing flow, or if the response is invalid - pub fn handle_response(&mut self, resp: Response) -> Result { + pub fn handle_response(&mut self, resp: Response) -> Result { let req = self.ongoing_request()?; if !secure_equality(&resp.state, &req.state) { @@ -175,7 +171,7 @@ impl Auth { // This MUST be the only place the GUI can call `set_password`, since // the actor name is also saved here. - self.token_store.set(token.clone())?; + self.token_store.set_password(token.expose_secret())?; let path = actor_name_path()?; std::fs::create_dir_all(path.parent().ok_or(Error::ActorNamePathWrong)?) .map_err(Error::CreateDirAll)?; @@ -189,7 +185,7 @@ impl Auth { /// Returns the token if we are signed in /// /// This will always make syscalls, but it should be fast enough for normal use. - pub fn token(&self) -> Result> { + pub fn token(&self) -> Result, Error> { match self.state { State::SignedIn(_) => {} State::NeedResponse(_) | State::SignedOut => return Ok(None), @@ -203,7 +199,7 @@ impl Auth { /// Retrieves the token from disk regardless of in-memory state /// /// Performs I/O - fn get_token_from_disk(&self) -> Result> { + fn get_token_from_disk(&self) -> Result, Error> { let actor_name = match std::fs::read_to_string(actor_name_path()?) { Ok(x) => x, // It can happen with dev systems that actor_name.txt doesn't exist @@ -215,9 +211,10 @@ impl Auth { // This MUST be the only place the GUI can call `get_password`, since the // actor name is also loaded here. - let Some(token) = self.token_store.get()? else { + let Ok(token) = self.token_store.get_password() else { return Ok(None); }; + let token = SecretString::from(token); Ok(Some(SessionAndToken { session: Session { actor_name }, @@ -225,7 +222,7 @@ impl Auth { })) } - pub fn ongoing_request(&self) -> Result<&Request> { + pub fn ongoing_request(&self) -> Result<&Request, Error> { match &self.state { State::NeedResponse(x) => Ok(x), State::SignedIn(_) | State::SignedOut => Err(Error::NoInflightRequest), @@ -236,7 +233,7 @@ impl Auth { /// Returns a path to a file where we can save the actor name /// /// Hopefully we don't need to save anything else, or there will be a migration step -fn actor_name_path() -> Result { +fn actor_name_path() -> Result { Ok(known_dirs::session() .ok_or(Error::CantFindKnownDir)? .join("actor_name.txt")) @@ -295,6 +292,32 @@ mod tests { states_dont_match(); } + #[test] + fn keyring_rs() { + // We used this test to find that `service` is not used on Windows - We have to namespace on our own. + + let name_1 = "dev.firezone.client/test_1/token"; + let name_2 = "dev.firezone.client/test_2/token"; + + // Accessing the same keys from different `Entry` instances doesn't work well, + // even within the same thread + let entry = keyring::Entry::new_with_target(name_1, "", "").unwrap(); + entry.set_password("test_password_1").unwrap(); + + { + // In the middle of accessing one token, access another to make sure they don't interfere much + let entry = keyring::Entry::new_with_target(name_2, "", "").unwrap(); + entry.set_password("test_password_2").unwrap(); + assert_eq!(entry.get_password().unwrap(), "test_password_2"); + entry.delete_password().unwrap(); + assert!(entry.get_password().is_err()); + } + + assert_eq!(entry.get_password().unwrap(), "test_password_1"); + entry.delete_password().unwrap(); + assert!(entry.get_password().is_err()); + } + fn utils() { // This doesn't test for constant-time properties, it just makes sure the function // gives the right result @@ -326,7 +349,7 @@ mod tests { { // Start the program - let mut state = Auth::new_with_key(key); + let mut state = Auth::new_with_key(key).unwrap(); // Delete any token on disk from previous test runs state.sign_out().unwrap(); @@ -351,7 +374,7 @@ mod tests { // Recreate the state to simulate closing and re-opening the app { - let mut state = Auth::new_with_key(key); + let mut state = Auth::new_with_key(key).unwrap(); // Make sure we automatically got the token and actor_name back assert!(state.token().unwrap().is_some()); @@ -372,7 +395,7 @@ mod tests { fn no_inflight_request() { // Start the program let mut state = - Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_invalid_response/token"); + Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_invalid_response/token").unwrap(); // Delete any token on disk from previous test runs state.sign_out().unwrap(); @@ -397,7 +420,8 @@ mod tests { fn states_dont_match() { // Start the program let mut state = - Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_states_dont_match/token"); + Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_states_dont_match/token") + .unwrap(); // Delete any token on disk from previous test runs state.sign_out().unwrap(); diff --git a/rust/gui-client/src-tauri/src/client/auth/token_storage_keyring.rs b/rust/gui-client/src-tauri/src/client/auth/token_storage_keyring.rs deleted file mode 100644 index d77722b74..000000000 --- a/rust/gui-client/src-tauri/src/client/auth/token_storage_keyring.rs +++ /dev/null @@ -1,73 +0,0 @@ -//! Implements credential storage with `keyring-rs`. -//! -//! `keyring-rs` is cross-platform but it's hard to get it working in headless Linux -//! environments like Github's CI. - -use super::Error; -use secrecy::{ExposeSecret, SecretString}; - -pub(crate) struct TokenStorage { - /// Key for secure keyrings, e.g. "dev.firezone.client/token" for releases - /// and something else for automated tests of the auth module. - keyring_key: &'static str, -} - -impl TokenStorage { - pub(crate) fn new(keyring_key: &'static str) -> Self { - Self { keyring_key } - } - - // `&mut` is probably not needed here, but it feels like it should be - pub(crate) fn delete(&mut self) -> Result<(), Error> { - match self.keyring_entry()?.delete_password() { - Ok(_) | Err(keyring::Error::NoEntry) => Ok(()), - Err(e) => Err(e)?, - } - } - - pub(crate) fn get(&self) -> Result, Error> { - match self.keyring_entry()?.get_password() { - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) => Err(e.into()), - Ok(token) => Ok(Some(SecretString::from(token))), - } - } - - pub(crate) fn set(&mut self, token: SecretString) -> Result<(), Error> { - self.keyring_entry()?.set_password(token.expose_secret())?; - Ok(()) - } - - /// Returns an Entry into the OS' credential manager - /// - /// Anything you do in there is technically blocking I/O. - fn keyring_entry(&self) -> Result { - Ok(keyring::Entry::new_with_target(self.keyring_key, "", "")?) - } -} - -#[cfg(test)] -mod tests { - #[cfg(target_os = "windows")] - #[test] - fn test_keyring() -> anyhow::Result<()> { - // I used this test to find that `service` is not used on Windows - We have to namespace on our own. - - let name_1 = "dev.firezone.client/test_1/token"; - let name_2 = "dev.firezone.client/test_2/token"; - - keyring::Entry::new_with_target(name_1, "", "")?.set_password("test_password_1")?; - - keyring::Entry::new_with_target(name_2, "", "")?.set_password("test_password_2")?; - - let actual = keyring::Entry::new_with_target(name_1, "", "")?.get_password()?; - let expected = "test_password_1"; - - assert_eq!(actual, expected); - - keyring::Entry::new_with_target(name_1, "", "")?.delete_password()?; - keyring::Entry::new_with_target(name_2, "", "")?.delete_password()?; - - Ok(()) - } -} diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 16bde19d9..36c28c259 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -769,7 +769,7 @@ async fn run_controller( let mut controller = Controller { advanced_settings, app: app.clone(), - auth: client::auth::Auth::new(), + auth: client::auth::Auth::new()?, ctlr_tx, ipc_client, log_filter_reloader,