From 0825055ff29c634e082c42eb470bc18ce1bab7dd Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 10 Oct 2024 07:04:24 +1100 Subject: [PATCH] fix(rust/gui-client): allow GUI process to read the `firezone-id` file from disk (#6987) Closes #6989 - The tunnel daemon (IPC service) now explicitly sets the ID file's perms to 0o640, even if the file already exists. - The GUI error is now non-fatal. If the file can't be read, we just won't get the device ID in Sentry. - More specific error message when the GUI fails to read the ID file We attempted to set the tunnel daemon's umask, but this caused the smoke tests to fail. Fixing the regression is more urgent than getting the smoke tests to match local debugging. --------- Co-authored-by: _ --- rust/gui-client/src-common/src/controller.rs | 17 +++++--- rust/headless-client/src/device_id.rs | 41 ++++++++++++++------ rust/tests/gui-smoke-test/src/main.rs | 1 + website/src/components/Changelog/GUI.tsx | 6 ++- 4 files changed, 47 insertions(+), 18 deletions(-) diff --git a/rust/gui-client/src-common/src/controller.rs b/rust/gui-client/src-common/src/controller.rs index 85ad28c91..bee570ce9 100644 --- a/rust/gui-client/src-common/src/controller.rs +++ b/rust/gui-client/src-common/src/controller.rs @@ -13,7 +13,7 @@ use firezone_headless_client::{ IpcClientMsg::{self, SetDisabledResources}, IpcServerMsg, IpcServiceError, LogFilterReloader, }; -use firezone_telemetry::Telemetry; +use firezone_telemetry::{self as telemetry, Telemetry}; use secrecy::{ExposeSecret as _, SecretString}; use std::{collections::BTreeSet, ops::ControlFlow, path::PathBuf, time::Instant}; use tokio::sync::{mpsc, oneshot}; @@ -70,11 +70,16 @@ impl Builder { let (ipc_tx, ipc_rx) = mpsc::channel(1); let ipc_client = ipc::Client::new(ipc_tx).await?; // Get the device ID after connecting to the IPC service, this creates a happens-before relationship where we know the IPC service has written a device ID to disk. - let firezone_id = firezone_headless_client::device_id::get() - .context("Failed to read / create device ID")? - .id; - firezone_telemetry::Hub::main() - .configure_scope(|scope| scope.set_tag("firezone_id", &firezone_id)); + match firezone_headless_client::device_id::get() { + Ok(id) => { + telemetry::Hub::main() + .configure_scope(|scope| scope.set_tag("firezone_id", &id.id)); + } + Err(error) => { + telemetry::capture_anyhow(&error.context("Failed to read device ID")); + } + } + Ok(Controller { advanced_settings, auth: auth::Auth::new()?, diff --git a/rust/headless-client/src/device_id.rs b/rust/headless-client/src/device_id.rs index 3f17a8b8d..ff1851cc4 100644 --- a/rust/headless-client/src/device_id.rs +++ b/rust/headless-client/src/device_id.rs @@ -55,13 +55,13 @@ pub fn device_info() -> phoenix_channel::DeviceInfo { /// Returns the device ID without generating it pub fn get() -> Result { let path = path()?; - let Some(j) = fs::read_to_string(&path) - .ok() - .and_then(|s| serde_json::from_str::(&s).ok()) - else { - anyhow::bail!("Couldn't read device ID from disk"); - }; - Ok(DeviceId { id: j.device_id() }) + let content = fs::read_to_string(&path).context("Failed to read file")?; + let device_id_json = serde_json::from_str::(&content) + .context("Failed to deserialize content as JSON")?; + + Ok(DeviceId { + id: device_id_json.device_id(), + }) } /// Returns the device ID, generating it and saving it to disk if needed. @@ -80,7 +80,7 @@ pub fn get_or_create() -> Result { // Make sure the dir exists, and fix its permissions so the GUI can write the // log filter file fs::create_dir_all(dir).context("Failed to create dir for firezone-id")?; - set_permissions(dir).with_context(|| { + set_dir_permissions(dir).with_context(|| { format!( "Couldn't set permissions on IPC service config dir `{}`", dir.display() @@ -94,6 +94,8 @@ pub fn get_or_create() -> Result { { let id = j.device_id(); tracing::debug!(?id, "Loaded device ID from disk"); + // Correct permissions for #6989 + set_id_permissions(&path).context("Couldn't set permissions on Firezone ID file")?; return Ok(DeviceId { id }); } @@ -110,13 +112,14 @@ pub fn get_or_create() -> Result { let id = j.device_id(); tracing::debug!(?id, "Saved device ID to disk"); + set_id_permissions(&path).context("Couldn't set permissions on Firezone ID file")?; Ok(DeviceId { id }) } #[cfg(target_os = "linux")] -fn set_permissions(dir: &Path) -> Result<()> { +fn set_dir_permissions(dir: &Path) -> Result<()> { use std::os::unix::fs::PermissionsExt; - // user read/write, group read-write, others nothing + // user read/write, group read/write, others nothing // directories need `+x` to work of course let perms = fs::Permissions::from_mode(0o770); std::fs::set_permissions(dir, perms)?; @@ -126,7 +129,23 @@ fn set_permissions(dir: &Path) -> Result<()> { /// Does nothing on non-Linux systems #[cfg(not(target_os = "linux"))] #[expect(clippy::unnecessary_wraps)] -fn set_permissions(_: &Path) -> Result<()> { +fn set_dir_permissions(_: &Path) -> Result<()> { + Ok(()) +} + +#[cfg(target_os = "linux")] +fn set_id_permissions(path: &Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + // user read/write, group read, others nothing + let perms = fs::Permissions::from_mode(0o640); + std::fs::set_permissions(path, perms)?; + Ok(()) +} + +/// Does nothing on non-Linux systems +#[cfg(not(target_os = "linux"))] +#[expect(clippy::unnecessary_wraps)] +fn set_id_permissions(_: &Path) -> Result<()> { Ok(()) } diff --git a/rust/tests/gui-smoke-test/src/main.rs b/rust/tests/gui-smoke-test/src/main.rs index 1d19ec9b4..135b90851 100644 --- a/rust/tests/gui-smoke-test/src/main.rs +++ b/rust/tests/gui-smoke-test/src/main.rs @@ -46,6 +46,7 @@ fn main() -> Result<()> { .popen()?; gui.wait()?.fz_exit_ok().context("GUI process")?; + ipc_service.wait()?.fz_exit_ok().context("IPC service")?; // Force the GUI to crash diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index f45caeb46..dd9134d27 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -14,7 +14,11 @@ export default function GUI({ title }: { title: string }) { return ( {/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */} - + + + Fixes a crash on startup caused by incorrect permissions on the ID file. + + Fixes the GUI shutting down slowly.