diff --git a/rust/gui-client/src-common/src/auth.rs b/rust/gui-client/src-common/src/auth.rs index 6813d3d2f..df386fe77 100644 --- a/rust/gui-client/src-common/src/auth.rs +++ b/rust/gui-client/src-common/src/auth.rs @@ -4,7 +4,8 @@ use anyhow::Result; use firezone_headless_client::known_dirs; use rand::{thread_rng, RngCore}; use secrecy::{ExposeSecret, SecretString}; -use std::path::PathBuf; +use serde::{Deserialize, Serialize}; +use std::path::{Path, PathBuf}; use subtle::ConstantTimeEq; use url::Url; @@ -12,24 +13,26 @@ const NONCE_LENGTH: usize = 32; #[derive(thiserror::Error, Debug)] pub enum Error { - #[error("`actor_name_path` has no parent, this should be impossible")] - ActorNamePathWrong, #[error("`known_dirs` failed")] CantFindKnownDir, #[error("`create_dir_all` failed while writing `actor_name_path`")] CreateDirAll(std::io::Error), - #[error("Couldn't delete actor_name from disk: {0}")] - DeleteActorName(std::io::Error), + #[error("Couldn't delete session file from disk: {0}")] + DeleteFile(std::io::Error), #[error(transparent)] Keyring(#[from] keyring::Error), #[error("No in-flight request")] NoInflightRequest, - #[error("Couldn't read actor_name from disk: {0}")] - ReadActorName(std::io::Error), + #[error("session file path has no parent, this should be impossible")] + PathWrong, + #[error("Couldn't read session file: {0}")] + ReadFile(std::io::Error), + #[error("Could not (de)serialize session data")] + Serde, #[error("State in server response doesn't match state in client request")] StatesDontMatch, - #[error("Couldn't write actor_name to disk: {0}")] - WriteActorName(std::io::Error), + #[error("Couldn't write session file: {0}")] + WriteFile(std::io::Error), } pub struct Auth { @@ -61,14 +64,17 @@ impl Request { } } -pub struct Response { - pub actor_name: String, - pub fragment: SecretString, - pub state: SecretString, +pub(crate) struct Response { + pub(crate) account_slug: String, + pub(crate) actor_name: String, + pub(crate) fragment: SecretString, + pub(crate) state: SecretString, } -pub struct Session { - pub actor_name: String, +#[derive(Default, Deserialize, Serialize)] +pub(crate) struct Session { + pub(crate) account_slug: String, + pub(crate) actor_name: String, } struct SessionAndToken { @@ -111,7 +117,7 @@ impl Auth { } /// Returns the session iff we are signed in. - pub fn session(&self) -> Option<&Session> { + pub(crate) fn session(&self) -> Option<&Session> { match &self.state { State::SignedIn(x) => Some(x), State::NeedResponse(_) | State::SignedOut => None, @@ -125,12 +131,8 @@ impl Auth { if let Err(error) = self.token_store.delete_credential() { tracing::warn!(?error, "Couldn't delete token while signing out"); } - if let Err(error) = std::fs::remove_file(actor_name_path()?) { - // Ignore NotFound, since the file is gone anyway - if error.kind() != std::io::ErrorKind::NotFound { - return Err(Error::DeleteActorName(error)); - } - } + delete_if_exists(&actor_name_path()?)?; + delete_if_exists(&session_data_path()?)?; self.state = State::SignedOut; Ok(()) } @@ -154,7 +156,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(crate) fn handle_response(&mut self, resp: Response) -> Result { let req = self.ongoing_request()?; if !secure_equality(&resp.state, &req.state) { @@ -169,21 +171,27 @@ impl Auth { ); let token = SecretString::from(token); - self.save_session(&resp.actor_name, &token)?; - self.state = State::SignedIn(Session { + let session = Session { + account_slug: resp.account_slug, actor_name: resp.actor_name, - }); + }; + + self.save_session(&session, &token)?; + self.state = State::SignedIn(session); Ok(SecretString::from(token)) } - fn save_session(&self, actor_name: &str, token: &SecretString) -> Result<(), Error> { + fn save_session(&self, session: &Session, token: &SecretString) -> Result<(), Error> { // This MUST be the only place the GUI can call `set_password`, since // the actor name is also saved here. 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)?; - std::fs::write(path, actor_name.as_bytes()).map_err(Error::WriteActorName)?; + save_file(&actor_name_path()?, session.actor_name.as_bytes())?; + save_file( + &session_data_path()?, + serde_json::to_string(session) + .map_err(|_| Error::Serde)? + .as_bytes(), + )?; Ok(()) } @@ -205,14 +213,25 @@ impl Auth { /// /// Performs I/O fn get_token_from_disk(&self) -> Result, Error> { - let actor_name = match std::fs::read_to_string(actor_name_path()?) { - Ok(x) => x, + // Read the actor_name file, then let the session file override it if present. + + let mut session = Session::default(); + match std::fs::read_to_string(actor_name_path()?) { + Ok(x) => session.actor_name = x, // It can happen with dev systems that actor_name.txt doesn't exist // even though the token is in the cred manager. // In that case we just say the app is signed out Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), - Err(e) => return Err(Error::ReadActorName(e)), + Err(e) => return Err(Error::ReadFile(e)), }; + match std::fs::read_to_string(session_data_path()?) { + Ok(x) => { + session = serde_json::from_str(&x).map_err(|_| Error::Serde)?; + firezone_telemetry::set_account_slug(session.account_slug.to_string()); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(Error::ReadFile(e)), + } // This MUST be the only place the GUI can call `get_password`, since the // actor name is also loaded here. @@ -221,10 +240,7 @@ impl Auth { }; let token = SecretString::from(token); - Ok(Some(SessionAndToken { - session: Session { actor_name }, - token, - })) + Ok(Some(SessionAndToken { session, token })) } pub fn ongoing_request(&self) -> Result<&Request, Error> { @@ -235,6 +251,22 @@ impl Auth { } } +fn delete_if_exists(path: &Path) -> Result<(), Error> { + if let Err(error) = std::fs::remove_file(path) { + // Ignore NotFound, since the file is gone anyway + if error.kind() != std::io::ErrorKind::NotFound { + return Err(Error::DeleteFile(error)); + } + } + Ok(()) +} + +fn save_file(path: &Path, content: &[u8]) -> Result<(), Error> { + std::fs::create_dir_all(path.parent().ok_or(Error::PathWrong)?).map_err(Error::CreateDirAll)?; + std::fs::write(path, content).map_err(Error::WriteFile)?; + Ok(()) +} + /// 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 @@ -244,6 +276,12 @@ fn actor_name_path() -> Result { .join("actor_name.txt")) } +fn session_data_path() -> Result { + Ok(known_dirs::session() + .ok_or(Error::CantFindKnownDir)? + .join("session_data.json")) +} + /// Generates a random nonce using a CSPRNG, then returns it as hexadecimal fn generate_nonce() -> SecretString { let mut buf = [0u8; NONCE_LENGTH]; @@ -268,7 +306,10 @@ pub fn replicate_6791() -> Result<()> { tracing::warn!("Debugging issue #6791, pretending to be signed in with a bad token"); let this = Auth::new()?; this.save_session( - "Jane Doe", + &Session { + account_slug: "firezone".to_string(), + actor_name: "Jane Doe".to_string(), + }, &SecretString::from("obviously invalid token for testing #6791".to_string()), )?; Ok(()) @@ -391,6 +432,7 @@ mod tests { // User clicks "Sign In", build a fake server response let req = state.start_sign_in().unwrap().unwrap(); let resp = Response { + account_slug: "firezone".into(), actor_name: actor_name.into(), fragment: bogus_secret("fragment"), state: req.state.clone(), @@ -437,6 +479,7 @@ mod tests { // If we get a deep link with no in-flight request, it's invalid let r = state.handle_response(Response { + account_slug: "firezone".into(), actor_name: "Jane Doe".into(), fragment: bogus_secret("fragment"), state: bogus_secret("state"), @@ -465,6 +508,7 @@ mod tests { // User clicks "Sign In", build a fake server response state.start_sign_in().unwrap(); let resp = Response { + account_slug: "firezone".into(), actor_name: "Jane Doe".into(), fragment: bogus_secret("fragment"), state: SecretString::from( diff --git a/rust/gui-client/src-common/src/deep_link.rs b/rust/gui-client/src-common/src/deep_link.rs index c80da81f2..43930849f 100644 --- a/rust/gui-client/src-common/src/deep_link.rs +++ b/rust/gui-client/src-common/src/deep_link.rs @@ -36,7 +36,10 @@ pub enum Error { pub use imp::{open, register, Server}; -pub fn parse_auth_callback(url_secret: &SecretString) -> Result { +/// Parses a deep-link URL into a struct. +/// +/// e.g. `firezone-fd0020211111://handle_client_sign_in_callback/?state=secret&fragment=secret&account_name=Firezone&account_slug=firezone&actor_name=Jane+Doe&identity_provider_identifier=secret` +pub(crate) fn parse_auth_callback(url_secret: &SecretString) -> Result { let url = Url::parse(url_secret.expose_secret())?; if Some(url::Host::Domain("handle_client_sign_in_callback")) != url.host() { bail!("URL host should be `handle_client_sign_in_callback`"); @@ -48,12 +51,20 @@ pub fn parse_auth_callback(url_secret: &SecretString) -> Result _ => bail!("URL path should be `/` or empty"), } + let mut account_slug = None; let mut actor_name = None; let mut fragment = None; let mut state = None; + // There's probably a way to get serde to do this for (key, value) in url.query_pairs() { match key.as_ref() { + "account_slug" => { + if account_slug.is_some() { + bail!("`account_slug` should appear exactly once"); + } + account_slug = Some(value.to_string()); + } "actor_name" => { if actor_name.is_some() { bail!("`actor_name` should appear exactly once"); @@ -77,6 +88,7 @@ pub fn parse_auth_callback(url_secret: &SecretString) -> Result } Ok(auth::Response { + account_slug: account_slug.context("URL should have `account_slug`")?, actor_name: actor_name.context("URL should have `actor_name`")?, fragment: fragment.context("URL should have `fragment`")?, state: state.context("URL should have `state`")?, @@ -92,9 +104,10 @@ mod tests { #[test] fn parse_auth_callback() -> Result<()> { // Positive cases - let input = "firezone://handle_client_sign_in_callback/?actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345"; + let input = "firezone://handle_client_sign_in_callback/?account_slug=firezone&actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345"; let actual = parse_callback_wrapper(input)?; + assert_eq!(actual.account_slug, "firezone"); assert_eq!(actual.actor_name, "Reactor Scram"); assert_eq!(actual.fragment.expose_secret(), "a_very_secret_string"); assert_eq!(actual.state.expose_secret(), "a_less_secret_string"); @@ -102,14 +115,16 @@ mod tests { let input = "firezone-fd0020211111://handle_client_sign_in_callback?account_name=Firezone&account_slug=firezone&actor_name=Reactor+Scram&fragment=a_very_secret_string&identity_provider_identifier=1234&state=a_less_secret_string"; let actual = parse_callback_wrapper(input)?; + assert_eq!(actual.account_slug, "firezone"); assert_eq!(actual.actor_name, "Reactor Scram"); assert_eq!(actual.fragment.expose_secret(), "a_very_secret_string"); assert_eq!(actual.state.expose_secret(), "a_less_secret_string"); // Empty string "" `actor_name` is fine - let input = "firezone://handle_client_sign_in_callback/?actor_name=&fragment=&state=&identity_provider_identifier=12345"; + let input = "firezone://handle_client_sign_in_callback/?account_slug=firezone&actor_name=&fragment=&state=&identity_provider_identifier=12345"; let actual = parse_callback_wrapper(input)?; + assert_eq!(actual.account_slug, "firezone"); assert_eq!(actual.actor_name, ""); assert_eq!(actual.fragment.expose_secret(), ""); assert_eq!(actual.state.expose_secret(), ""); @@ -117,12 +132,12 @@ mod tests { // Negative cases // URL host is wrong - let input = "firezone://not_handle_client_sign_in_callback/?actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345"; + let input = "firezone://not_handle_client_sign_in_callback/?account_slug=firezone&actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345"; let actual = parse_callback_wrapper(input); assert!(actual.is_err()); // `actor_name` is not just blank but totally missing - let input = "firezone://handle_client_sign_in_callback/?fragment=&state=&identity_provider_identifier=12345"; + let input = "firezone://handle_client_sign_in_callback/?account_slug=firezone&fragment=&state=&identity_provider_identifier=12345"; let actual = parse_callback_wrapper(input); assert!(actual.is_err()); diff --git a/rust/telemetry/src/lib.rs b/rust/telemetry/src/lib.rs index 7b4a9cbdf..bede25af2 100644 --- a/rust/telemetry/src/lib.rs +++ b/rust/telemetry/src/lib.rs @@ -101,6 +101,16 @@ impl Telemetry { } } +/// Sets the Firezone account slug on the Sentry hub +/// +/// This sets the entire set of "user" info, so it will conflict if we set any other user ID later. +pub fn set_account_slug(account_slug: String) { + let mut user = sentry::User::default(); + user.other + .insert("account_slug".to_string(), account_slug.into()); + sentry::Hub::main().configure_scope(|scope| scope.set_user(Some(user))); +} + #[cfg(test)] mod tests { use super::*;