From 94cb494e0a151ba4b4136b3507e471123978a350 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Fri, 31 May 2024 13:21:57 -0500 Subject: [PATCH] refactor(gui-client): finish refactors from #4978 (#5158) ```[tasklist] ### Before opening for review - [ ] ~~Wait for some other refactors to merge~~ - [x] Test Windows - [x] Test Linux ``` --- rust/gui-client/src-tauri/src/client.rs | 2 +- .../src-tauri/src/client/elevation.rs | 8 +-- rust/headless-client/src/lib.rs | 8 +-- rust/headless-client/src/linux.rs | 3 +- rust/headless-client/src/windows.rs | 63 +++++++------------ 5 files changed, 33 insertions(+), 51 deletions(-) diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index 1414f4e01..cb65100b4 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -87,7 +87,7 @@ pub(crate) fn run() -> Result<()> { } Some(Cmd::SmokeTest) => { if !elevation::is_normal_user()? { - anyhow::bail!("`smoke-test` failed its elevation check"); + anyhow::bail!("`smoke-test` must run as a normal user"); } let result = gui::run(cli); diff --git a/rust/gui-client/src-tauri/src/client/elevation.rs b/rust/gui-client/src-tauri/src/client/elevation.rs index c67bbc095..a9b2c22d2 100644 --- a/rust/gui-client/src-tauri/src/client/elevation.rs +++ b/rust/gui-client/src-tauri/src/client/elevation.rs @@ -7,13 +7,13 @@ mod imp { /// Returns true if we're running without root privileges /// - /// On Linux we've already switched to IPC, so the process must NOT be elevated - #[allow(clippy::print_stderr)] + /// Everything that needs root / admin powers happens in the IPC services, + /// so for security and practicality reasons the GUIs must be non-root. + /// (In Linux by default a root GUI app barely works at all) pub(crate) fn is_normal_user() -> anyhow::Result { // Must use `eprintln` here because `tracing` won't be initialized yet. let user = std::env::var("USER").context("USER env var should be set")?; if user == "root" { - eprintln!("Firezone must run as a normal user, not with `sudo` or as root"); return Ok(false); } @@ -42,7 +42,7 @@ mod imp { // Returns true on Windows /// - /// On Windows we are switching to IPC, and checking for elevation is complicated, + /// On Windows, checking for elevation is complicated, /// so it just always returns true. The Windows GUI does work correctly even if /// elevated, so we should warn users that it doesn't need elevation, but it's /// not a show-stopper if they accidentally "Run as admin". diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index fb8ea1c38..3e34723d7 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -252,6 +252,8 @@ pub fn run_only_headless_client() -> Result<()> { return Ok(()); } + // TODO: Can theoretically be removed the same way the Windows IPC service works, + // using an abort handle let (on_disconnect_tx, mut on_disconnect_rx) = mpsc::channel(1); let callback_handler = CallbackHandler { on_disconnect_tx }; @@ -333,9 +335,7 @@ pub(crate) fn run_debug_ipc_service() -> Result<()> { tracing::info!("Caught Interrupt signal"); Ok(()) } - future::Either::Right((Ok(()), _)) => { - bail!("Impossible, ipc_listen can't return Ok"); - } + future::Either::Right((Ok(impossible), _)) => match impossible {}, future::Either::Right((Err(error), _)) => Err(error).context("ipc_listen failed"), } }) @@ -383,7 +383,7 @@ impl Callbacks for CallbackHandlerIpc { } } -async fn ipc_listen() -> Result<()> { +async fn ipc_listen() -> Result { let mut server = platform::IpcServer::new().await?; loop { connlib_shared::deactivate_dns_control()?; diff --git a/rust/headless-client/src/linux.rs b/rust/headless-client/src/linux.rs index d50109aa1..b43fe1226 100644 --- a/rust/headless-client/src/linux.rs +++ b/rust/headless-client/src/linux.rs @@ -167,7 +167,8 @@ pub(crate) fn run_ipc_service(cli: CliCommon) -> Result<()> { anyhow::bail!("This is the IPC service binary, it's not meant to run interactively."); } let rt = tokio::runtime::Runtime::new()?; - rt.block_on(async { crate::ipc_listen().await }) + rt.block_on(crate::ipc_listen())?; + Ok(()) } pub fn firezone_group() -> Result { diff --git a/rust/headless-client/src/windows.rs b/rust/headless-client/src/windows.rs index 00b40f2bb..ed5a6a691 100644 --- a/rust/headless-client/src/windows.rs +++ b/rust/headless-client/src/windows.rs @@ -5,20 +5,17 @@ //! We must tell Windows explicitly when our service is stopping. use crate::{CliCommon, SignalKind}; -use anyhow::{anyhow, Context as _, Result}; +use anyhow::{Context as _, Result}; use connlib_client_shared::file_logger; use connlib_shared::BUNDLE_ID; use std::{ ffi::{c_void, OsString}, - future::Future, net::IpAddr, path::{Path, PathBuf}, - pin::pin, str::FromStr, - task::Poll, time::Duration, }; -use tokio::{net::windows::named_pipe, sync::mpsc}; +use tokio::net::windows::named_pipe; use tracing::subscriber::set_global_default; use tracing_subscriber::{layer::SubscriberExt as _, EnvFilter, Layer, Registry}; use windows::Win32::Security as WinSec; @@ -102,7 +99,13 @@ fn fallible_windows_service_run(arguments: Vec) -> Result<()> { tracing::info!(?arguments, "fallible_windows_service_run"); let rt = tokio::runtime::Runtime::new()?; - let (shutdown_tx, mut shutdown_rx) = mpsc::channel(1); + + // Fixes , + // DNS rules persisting after reboot + connlib_shared::deactivate_dns_control().ok(); + + let ipc_task = rt.spawn(super::ipc_listen()); + let ipc_task_ah = ipc_task.abort_handle(); let event_handler = move |control_event| -> ServiceControlHandlerResult { tracing::debug!(?control_event); @@ -111,7 +114,7 @@ fn fallible_windows_service_run(arguments: Vec) -> Result<()> { ServiceControl::Interrogate => ServiceControlHandlerResult::NoError, ServiceControl::Stop => { tracing::info!("Got stop signal from service controller"); - shutdown_tx.blocking_send(()).unwrap(); + ipc_task_ah.abort(); ServiceControlHandlerResult::NoError } ServiceControl::UserEvent(_) => ServiceControlHandlerResult::NoError, @@ -133,10 +136,6 @@ fn fallible_windows_service_run(arguments: Vec) -> Result<()> { } }; - // Fixes , - // DNS rules persisting after reboot - connlib_shared::deactivate_dns_control().ok(); - // Tell Windows that we're running (equivalent to sd_notify in systemd) let status_handle = service_control_handler::register(SERVICE_NAME, event_handler)?; status_handle.set_service_status(ServiceStatus { @@ -149,36 +148,18 @@ fn fallible_windows_service_run(arguments: Vec) -> Result<()> { process_id: None, })?; - let mut ipc_service = pin!(super::ipc_listen()); - let result = rt.block_on(async { - std::future::poll_fn(|cx| { - match shutdown_rx.poll_recv(cx) { - Poll::Ready(Some(())) => { - tracing::info!("Got shutdown signal"); - return Poll::Ready(Ok(())); - } - Poll::Ready(None) => { - return Poll::Ready(Err(anyhow!( - "shutdown channel unexpectedly dropped, shutting down" - ))) - } - Poll::Pending => {} - } - - match ipc_service.as_mut().poll(cx) { - Poll::Ready(Ok(())) => { - return Poll::Ready(Err(anyhow!("Impossible, ipc_listen can't return Ok"))) - } - Poll::Ready(Err(error)) => { - return Poll::Ready(Err(error.context("ipc_listen failed"))) - } - Poll::Pending => {} - } - - Poll::Pending - }) - .await - }); + let result = match rt.block_on(ipc_task) { + Err(join_error) if join_error.is_cancelled() => { + // We cancelled because Windows asked us to shut down. + Ok(()) + } + Err(join_error) => { + // The IPC task may have panicked + Err(join_error.into()) + } + Ok(Err(error)) => Err(error.context("ipc_listen failed")), + Ok(Ok(impossible)) => match impossible {}, + }; // Tell Windows that we're stopping status_handle.set_service_status(ServiceStatus {