fix(gui-client): don't share log-directives via file system (#7445)

At present, the GUI client shares the current log-directives with the
IPC service via the file system. Supposedly, this has been done to allow
the IPC service to start back up with the same log filter as before.
This behaviour appears to be buggy though as we are receiving a fair
number of error reports where this file is not writable.

Instead of relying on the file system to communicate, we send the
current log-directives to the IPC service as soon as we start up. The
IPC service then uses the file system as a cache that log string and
re-apply it on the next startup. This way, no two programs need to read
/ write the same file. The IPC service runs with higher privileges, so
this should resolve the permission errors we are seeing in Sentry.

rust/gui-client/src-common/src/controller.rs

@@ -386,16 +386,19 @@ impl<'a, I: GuiIntegration> Controller<'a, I> {
     async fn handle_request(&mut self, req: ControllerRequest) -> Result<(), Error> {
         match req {
             Req::ApplySettings(settings) => {
-                let filter = firezone_logging::try_filter(&self.advanced_settings.log_filter)
+                let filter = firezone_logging::try_filter(&settings.log_filter)
                         .context("Couldn't parse new log filter directives")?;
                 self.advanced_settings = *settings;
+
                 self.log_filter_reloader
                     .reload(filter)
                     .context("Couldn't reload log filter")?;
-                self.ipc_client.send_msg(&IpcClientMsg::ReloadLogFilter).await?;
+                self.ipc_client.send_msg(&IpcClientMsg::ApplyLogFilter { directives: self.advanced_settings.log_filter.clone() }).await?;
+
                 tracing::debug!(
                     "Applied new settings. Log level will take effect immediately."
                 );
+
                 // Refresh the menu in case the favorites were reset.
                 self.refresh_system_tray_menu()?;
             }

rust/gui-client/src-common/src/settings.rs

@@ -2,12 +2,10 @@
 //! advanced settings and code for manipulating diagnostic logs.
 
 use anyhow::{Context as _, Result};
-use atomicwrites::{AtomicFile, OverwriteBehavior};
 use connlib_model::ResourceId;
 use firezone_headless_client::known_dirs;
-use firezone_logging::std_dyn_err;
 use serde::{Deserialize, Serialize};
-use std::{collections::HashSet, io::Write, path::PathBuf};
+use std::{collections::HashSet, path::PathBuf};
 use url::Url;
 
 #[derive(Clone, Deserialize, Serialize)]
@@ -65,21 +63,12 @@ pub async fn save(settings: &AdvancedSettings) -> Result<()> {
     let dir = path
         .parent()
         .context("settings path should have a parent")?;
+
     tokio::fs::create_dir_all(dir).await?;
     tokio::fs::write(&path, serde_json::to_string(settings)?).await?;
-    // Don't create the dir for the log filter file, that's the IPC service's job.
-    // If it isn't there for some reason yet, just log an error and move on.
-    let log_filter_path = known_dirs::ipc_log_filter().context("`ipc_log_filter` failed")?;
-    let f = AtomicFile::new(&log_filter_path, OverwriteBehavior::AllowOverwrite);
-    // Note: Blocking file write in async function
-    if let Err(error) = f.write(|f| f.write_all(settings.log_filter.as_bytes())) {
-        tracing::error!(
-            error = std_dyn_err(&error),
-            ?log_filter_path,
-            "Couldn't write log filter file for IPC service"
-        );
-    }
+
     tracing::debug!(?path, "Saved settings");
+
     Ok(())
 }
 

rust/headless-client/src/ipc_service.rs

@@ -3,13 +3,14 @@ use crate::{
     ConnlibMsg, LogFilterReloader,
 };
 use anyhow::{bail, Context as _, Result};
+use atomicwrites::{AtomicFile, OverwriteBehavior};
 use clap::Parser;
 use connlib_model::ResourceView;
 use firezone_bin_shared::{
     platform::{tcp_socket_factory, udp_socket_factory, DnsControlMethod},
     TunDeviceManager, TOKEN_ENV_KEY,
 };
-use firezone_logging::{anyhow_dyn_err, sentry_layer, telemetry_span};
+use firezone_logging::{anyhow_dyn_err, sentry_layer, std_dyn_err, telemetry_span};
 use firezone_telemetry::Telemetry;
 use futures::{
     future::poll_fn,
@@ -20,9 +21,15 @@ use phoenix_channel::LoginUrl;
 use secrecy::SecretString;
 use serde::{Deserialize, Serialize};
 use std::{
-    collections::BTreeSet, io, net::IpAddr, path::PathBuf, pin::pin, sync::Arc, time::Duration,
+    collections::BTreeSet,
+    io::{self, Write},
+    net::IpAddr,
+    path::PathBuf,
+    pin::pin,
+    sync::Arc,
+    time::Duration,
 };
-use tokio::{sync::mpsc, task::spawn_blocking, time::Instant};
+use tokio::{sync::mpsc, time::Instant};
 use tracing_subscriber::{layer::SubscriberExt, reload, EnvFilter, Layer, Registry};
 use url::Url;
 
@@ -83,7 +90,9 @@ pub enum ClientMsg {
         token: String,
     },
     Disconnect,
-    ReloadLogFilter,
+    ApplyLogFilter {
+        directives: String,
+    },
     Reset,
     SetDns(Vec<IpAddr>),
     SetDisabledResources(BTreeSet<ResourceId>),
@@ -498,9 +507,16 @@ impl<'a> Handler<'a> {
                     .await
                     .context("Failed to send `DisconnectedGracefully`")?;
             }
-            ClientMsg::ReloadLogFilter => {
-                let filter = spawn_blocking(get_log_filter).await??;
-                self.log_filter_reloader.reload(filter)?;
+            ClientMsg::ApplyLogFilter { directives } => {
+                self.log_filter_reloader.reload(directives.clone())?;
+
+                let path = known_dirs::ipc_log_filter()?;
+
+                if let Err(e) = AtomicFile::new(&path, OverwriteBehavior::AllowOverwrite)
+                    .write(|f| f.write_all(directives.as_bytes()))
+                {
+                    tracing::warn!(path = %path.display(), %directives, error = std_dyn_err(&e), "Failed to write new log directives");
+                }
             }
             ClientMsg::Reset => {
                 if self.last_connlib_start_instant.is_some() {
@@ -673,11 +689,8 @@ pub(crate) fn get_log_filter() -> Result<String> {
         return Ok(filter);
     }
 
-    if let Ok(filter) = std::fs::read_to_string(
-        known_dirs::ipc_log_filter()
-            .context("Failed to compute directory for log filter config file")?,
-    )
-    .map(|s| s.trim().to_string())
+    if let Ok(filter) =
+        std::fs::read_to_string(known_dirs::ipc_log_filter()?).map(|s| s.trim().to_string())
     {
         return Ok(filter);
     }

rust/headless-client/src/known_dirs.rs

@@ -7,9 +7,11 @@
 //!
 //! I wanted the ProgramData folder on Windows, which `dirs` alone doesn't provide.
 
-pub use platform::{ipc_service_config, ipc_service_logs, logs, runtime, session, settings};
+use anyhow::{Context as _, Result};
 use std::path::PathBuf;
 
+pub use platform::{ipc_service_config, ipc_service_logs, logs, runtime, session, settings};
+
 #[cfg(target_os = "linux")]
 #[path = "known_dirs/linux.rs"]
 pub mod platform;
@@ -22,8 +24,10 @@ pub mod platform;
 #[path = "known_dirs/windows.rs"]
 pub mod platform;
 
-pub fn ipc_log_filter() -> Option<PathBuf> {
-    Some(ipc_service_config()?.join("log-filter"))
+pub fn ipc_log_filter() -> Result<PathBuf> {
+    Ok(ipc_service_config()
+        .context("Failed to compute `ipc_service_config` directory")?
+        .join("log-filter"))
 }
 
 #[cfg(test)]

rust/headless-client/src/known_dirs/linux.rs

@@ -1,11 +1,9 @@
 use firezone_bin_shared::BUNDLE_ID;
 use std::path::PathBuf;
 
-/// Path for IPC service config that either the IPC service or GUI can write
+/// Path for IPC service config that the IPC service can write
 ///
-/// e.g. the device ID should only be written by the IPC service, and
-/// the log filter should only be written by the GUI. No file should be written
-/// by both programs. All writes should use `atomicwrites`.
+/// All writes should use `atomicwrites`.
 ///
 /// On Linux, `/var/lib/$BUNDLE_ID/config/firezone-id`
 ///

rust/headless-client/src/known_dirs/windows.rs

@@ -2,11 +2,9 @@ use firezone_bin_shared::{platform::app_local_data_dir, BUNDLE_ID};
 use known_folders::{get_known_folder_path, KnownFolder};
 use std::path::PathBuf;
 
-/// Path for IPC service config that either the IPC service or GUI can write
+/// Path for IPC service config that the IPC service can write
 ///
-/// e.g. the device ID should only be written by the IPC service, and
-/// the log filter should only be written by the GUI. No file should be written
-/// by both programs. All writes should use `atomicwrites`.
+/// All writes should use `atomicwrites`.
 ///
 /// On Windows, `C:/ProgramData/$BUNDLE_ID/config`
 pub fn ipc_service_config() -> Option<PathBuf> {