github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

chore(rust/gui-client): report account slug to Sentry (#7097)

Browse Source 
Closes #7087

<img width="375" alt="image"
src="https://github.com/user-attachments/assets/7fcf0f08-019c-4e48-9c1b-f038638ce930">
This commit is contained in:
Reactor Scram
2024-10-22 12:17:47 -05:00
committed by GitHub
parent 1f8530ec24
commit 4bfdf9b20b
3 changed files with 113 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

122
rust/gui-client/src-common/src/auth.rs
View File

@@ -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<SecretString, Error> {
pub(crate) fn handle_response(&mut self, resp: Response) -> Result<SecretString, Error> {
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<Option<SessionAndToken>, 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<PathBuf, Error> {
.join("actor_name.txt"))
}
fn session_data_path() -> Result<PathBuf, Error> {
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(

25
rust/gui-client/src-common/src/deep_link.rs
View File

@@ -36,7 +36,10 @@ pub enum Error {
pub use imp::{open, register, Server};
pub fn parse_auth_callback(url_secret: &SecretString) -> Result<auth::Response> {
/// 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<auth::Response> {
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<auth::Response>
_ => 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<auth::Response>
}
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());

10
rust/telemetry/src/lib.rs
View File

@@ -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::*;