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: _ <ReactorScram@users.noreply.github.com>
This commit is contained in:
Thomas Eizinger
2024-10-10 07:04:24 +11:00
committed by GitHub
parent f5362ce009
commit 0825055ff2
4 changed files with 47 additions and 18 deletions

View File

@@ -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<I: GuiIntegration> Builder<I> {
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()?,

View File

@@ -55,13 +55,13 @@ pub fn device_info() -> phoenix_channel::DeviceInfo {
/// Returns the device ID without generating it
pub fn get() -> Result<DeviceId> {
let path = path()?;
let Some(j) = fs::read_to_string(&path)
.ok()
.and_then(|s| serde_json::from_str::<DeviceIdJson>(&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::<DeviceIdJson>(&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<DeviceId> {
// 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<DeviceId> {
{
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<DeviceId> {
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(())
}

View File

@@ -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

View File

@@ -14,7 +14,11 @@ export default function GUI({ title }: { title: string }) {
return (
<Entries href={href} arches={arches} title={title}>
{/* 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. */}
<Unreleased></Unreleased>
<Unreleased>
<ChangeItem enable={title === "Linux GUI"} pull="6987">
Fixes a crash on startup caused by incorrect permissions on the ID file.
</ChangeItem>
</Unreleased>
<Entry version="1.3.8" date={new Date("2024-10-08")}>
<ChangeItem pull="6874">
Fixes the GUI shutting down slowly.