From 7fece80006a429539bf73049f5fe7b9add2e54a8 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Thu, 21 Mar 2024 09:42:48 -0500 Subject: [PATCH] refactor(gui-client): refuse to ever be elevated on Linux (#4232) Running as sudo / root causes a lot of problems for GUI programs, so we're unwinding that. In this case we can go back to using Tauri's "open URL" function, which is great. Closes #4103 Refs #3713 Affects #3972 - I was finally able to debug it because it came up constantly during this PR --- rust/connlib/shared/src/device_id.rs | 12 +- rust/gui-client/src-tauri/src/client.rs | 6 +- .../src-tauri/src/client/elevation.rs | 12 +- rust/gui-client/src-tauri/src/client/gui.rs | 130 +++++++++--------- .../src-tauri/src/client/gui/os_linux.rs | 22 +-- scripts/tests/smoke-test-gui-linux.sh | 70 +++++----- 6 files changed, 117 insertions(+), 135 deletions(-) diff --git a/rust/connlib/shared/src/device_id.rs b/rust/connlib/shared/src/device_id.rs index 9116cf23e..fcd6e9d1f 100644 --- a/rust/connlib/shared/src/device_id.rs +++ b/rust/connlib/shared/src/device_id.rs @@ -3,8 +3,6 @@ use std::fs; use std::io::Write; pub struct DeviceId { - /// True iff the device ID was not found on disk and we had to generate it, meaning this is the app's first run since installing. - pub is_first_time: bool, pub id: String, } @@ -27,10 +25,7 @@ pub fn get() -> Result { { let id = j.device_id(); tracing::debug!(?id, "Loaded device ID from disk"); - return Ok(DeviceId { - is_first_time: false, - id, - }); + return Ok(DeviceId { id }); } // Couldn't read, it's missing or invalid, generate a new one and save it. @@ -50,10 +45,7 @@ pub fn get() -> Result { let id = j.device_id(); tracing::debug!(?id, "Saved device ID to disk"); - Ok(DeviceId { - is_first_time: true, - id, - }) + Ok(DeviceId { id }) } #[derive(serde::Deserialize, serde::Serialize)] diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index 404a56e4e..50e2f686c 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -87,10 +87,10 @@ pub(crate) fn run() -> Result<()> { Some(Cmd::SmokeTest) => { // Check for elevation. This also ensures wintun.dll is installed. if !elevation::check()? { - anyhow::bail!("`smoke-test` must be run with elevated permissions"); + anyhow::bail!("`smoke-test` failed its elevation check"); } - let result = gui::run(&cli); + let result = gui::run(cli); if let Err(error) = &result { // In smoke-test mode, don't show the dialog, since it might be running // unattended in CI and the dialog would hang forever @@ -108,7 +108,7 @@ pub(crate) fn run() -> Result<()> { /// /// Automatically logs or shows error dialogs for important user-actionable errors fn run_gui(cli: Cli) -> Result<()> { - let result = gui::run(&cli); + let result = gui::run(cli); // Make sure errors get logged, at least to stderr if let Err(error) = &result { diff --git a/rust/gui-client/src-tauri/src/client/elevation.rs b/rust/gui-client/src-tauri/src/client/elevation.rs index 4c6d6e3b7..17678f645 100644 --- a/rust/gui-client/src-tauri/src/client/elevation.rs +++ b/rust/gui-client/src-tauri/src/client/elevation.rs @@ -8,21 +8,15 @@ mod imp { // 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 with root permissions to set up DNS. Re-run it with `sudo --preserve-env`"); - return Ok(false); - } - let home = std::env::var("HOME").context("HOME env var should be set")?; - if home == "/root" { - eprintln!("If Firezone is run with `$HOME == /root`, deep links will not work. Re-run it with `sudo --preserve-env`"); - // If we don't bail out here, this message will probably never be read. + if user == "root" { + eprintln!("Firezone must run as a normal user, not with `sudo` or as root"); return Ok(false); } Ok(true) } pub(crate) fn elevate() -> Result<()> { - anyhow::bail!("Firezone does not self-elevate on Linux."); + anyhow::bail!("Firezone must not elevate on Linux."); } } diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 1c9cb9f31..418a0c5d0 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -84,7 +84,7 @@ pub(crate) enum Error { /// Runs the Tauri GUI and returns on exit or unrecoverable error /// /// Still uses `thiserror` so we can catch the deep_link `CantListen` error -pub(crate) fn run(cli: &client::Cli) -> Result<(), Error> { +pub(crate) fn run(cli: client::Cli) -> Result<(), Error> { let advanced_settings = settings::load_advanced_settings().unwrap_or_default(); // If the log filter is unparsable, show an error and use the default @@ -133,39 +133,6 @@ pub(crate) fn run(cli: &client::Cli) -> Result<(), Error> { let (ctlr_tx, ctlr_rx) = mpsc::channel(5); - // Check for updates - let ctlr_tx_clone = ctlr_tx.clone(); - let always_show_update_notification = cli.always_show_update_notification; - tokio::spawn(async move { - if let Err(error) = check_for_updates(ctlr_tx_clone, always_show_update_notification).await - { - tracing::error!(?error, "Error in check_for_updates"); - } - }); - - // Make sure we're single-instance - // We register our deep links to call the `open-deep-link` subcommand, - // so if we're at this point, we know we've been launched manually - let server = deep_link::Server::new()?; - - if let Some(client::Cmd::SmokeTest) = &cli.command { - let ctlr_tx = ctlr_tx.clone(); - tokio::spawn(async move { - if let Err(error) = smoke_test(ctlr_tx).await { - tracing::error!(?error, "Error during smoke test"); - std::process::exit(1); - } - }); - } - - tracing::debug!(cli.no_deep_links); - if !cli.no_deep_links { - // The single-instance check is done, so register our exe - // to handle deep links - deep_link::register().context("Failed to register deep link handler")?; - tokio::spawn(accept_deep_links(server, ctlr_tx.clone())); - } - let managed = Managed { ctlr_tx: ctlr_tx.clone(), inject_faults: cli.inject_faults, @@ -173,20 +140,6 @@ pub(crate) fn run(cli: &client::Cli) -> Result<(), Error> { let tray = SystemTray::new().with_menu(system_tray_menu::signed_out()); - if let Some(failure) = cli.fail_on_purpose() { - let ctlr_tx = ctlr_tx.clone(); - tokio::spawn(async move { - let delay = 5; - tracing::info!( - "Will crash / error / panic on purpose in {delay} seconds to test error handling." - ); - tokio::time::sleep(Duration::from_secs(delay)).await; - tracing::info!("Crashing / erroring / panicking on purpose"); - ctlr_tx.send(ControllerRequest::Fail(failure)).await?; - Ok::<_, anyhow::Error>(()) - }); - } - tracing::debug!("Setting up Tauri app instance..."); let app = tauri::Builder::default() .manage(managed) @@ -230,6 +183,55 @@ pub(crate) fn run(cli: &client::Cli) -> Result<(), Error> { }) .setup(move |app| { tracing::debug!("Entered Tauri's `setup`"); + + // Check for updates + let ctlr_tx_clone = ctlr_tx.clone(); + let always_show_update_notification = cli.always_show_update_notification; + tokio::spawn(async move { + if let Err(error) = check_for_updates(ctlr_tx_clone, always_show_update_notification).await + { + tracing::error!(?error, "Error in check_for_updates"); + } + }); + + // Make sure we're single-instance + // We register our deep links to call the `open-deep-link` subcommand, + // so if we're at this point, we know we've been launched manually + let server = deep_link::Server::new()?; + + if let Some(client::Cmd::SmokeTest) = &cli.command { + let ctlr_tx = ctlr_tx.clone(); + tokio::spawn(async move { + if let Err(error) = smoke_test(ctlr_tx).await { + tracing::error!(?error, "Error during smoke test"); + tracing::error!("Crashing on purpose so a dev can see our stacktraces"); + unsafe { sadness_generator::raise_segfault() } + } + }); + } + + tracing::debug!(cli.no_deep_links); + if !cli.no_deep_links { + // The single-instance check is done, so register our exe + // to handle deep links + deep_link::register().context("Failed to register deep link handler")?; + tokio::spawn(accept_deep_links(server, ctlr_tx.clone())); + } + + if let Some(failure) = cli.fail_on_purpose() { + let ctlr_tx = ctlr_tx.clone(); + tokio::spawn(async move { + let delay = 5; + tracing::info!( + "Will crash / error / panic on purpose in {delay} seconds to test error handling." + ); + tokio::time::sleep(Duration::from_secs(delay)).await; + tracing::info!("Crashing / erroring / panicking on purpose"); + ctlr_tx.send(ControllerRequest::Fail(failure)).await?; + Ok::<_, anyhow::Error>(()) + }); + } + assert_eq!( BUNDLE_ID, app.handle().config().tauri.bundle.identifier, @@ -312,9 +314,6 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { tracing::info!("Will quit on purpose in {delay} seconds as part of the smoke test."); let quit_time = tokio::time::Instant::now() + Duration::from_secs(delay); - // Write the settings so we can check the path for those - settings::save(&settings::AdvancedSettings::default()).await?; - // Test log exporting let path = PathBuf::from("smoke_test_log_export.zip"); @@ -338,6 +337,9 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { // Give the app some time to export the zip and reach steady state tokio::time::sleep_until(quit_time).await; + // Write the settings so we can check the path for those + settings::save(&settings::AdvancedSettings::default()).await?; + // Check results of tests let zip_len = tokio::fs::metadata(&path) .await @@ -487,9 +489,6 @@ struct Controller { ctlr_tx: CtlrTx, /// connlib session for the currently signed-in user, if there is one session: Option, - /// The UUIDv4 device ID persisted to disk - /// Sent verbatim to Session::connect - device_id: String, logging_handles: client::logging::Handles, /// Tells us when to wake up and look for a new resource list. Tokio docs say that memory reads and writes are synchronized when notifying, so we don't need an extra mutex on the resources. notify_controller: Arc, @@ -523,11 +522,13 @@ impl Controller { api_url = api_url.to_string(), "Calling connlib Session::connect" ); + let device_id = + connlib_shared::device_id::get().context("Failed to read / create device ID")?; let (private_key, public_key) = keypair(); let login = LoginUrl::client( api_url.as_str(), &token, - self.device_id.clone(), + device_id.id, None, public_key.to_bytes(), )?; @@ -771,17 +772,23 @@ async fn run_controller( logging_handles: client::logging::Handles, advanced_settings: AdvancedSettings, ) -> Result<()> { - tracing::debug!("Reading / generating device ID..."); - let device_id = - connlib_shared::device_id::get().context("Failed to read / create device ID")?; - - if device_id.is_first_time { + let session_dir = crate::client::known_dirs::session().context("Couldn't find session dir")?; + let ran_before_path = session_dir.join("ran_before.txt"); + if !tokio::fs::try_exists(&ran_before_path).await? { let win = app .get_window("welcome") .context("Couldn't get handle to Welcome window")?; win.show()?; + tokio::fs::create_dir_all(&session_dir).await?; + tokio::fs::write(&ran_before_path, &[]).await?; } - let device_id = device_id.id; + + // TODO: This is temporary until process separation is done + // Try to create the device ID and ignore errors if we fail. + // This allows Linux to run with the "Generate device ID lazily" behavior, + // but Windows, which runs elevated, will write the device ID, and the smoke + // tests can cover it. + connlib_shared::device_id::get().ok(); let mut controller = Controller { advanced_settings, @@ -789,7 +796,6 @@ async fn run_controller( auth: client::auth::Auth::new().context("Failed to set up auth module")?, ctlr_tx, session: None, - device_id, logging_handles, notify_controller: Arc::new(Notify::new()), // TODO: Fix cancel-safety tunnel_ready: false, diff --git a/rust/gui-client/src-tauri/src/client/gui/os_linux.rs b/rust/gui-client/src-tauri/src/client/gui/os_linux.rs index 08aaf21db..26cf64354 100644 --- a/rust/gui-client/src-tauri/src/client/gui/os_linux.rs +++ b/rust/gui-client/src-tauri/src/client/gui/os_linux.rs @@ -1,27 +1,11 @@ use super::{ControllerRequest, CtlrTx, Error}; use anyhow::Result; use secrecy::{ExposeSecret, SecretString}; +use tauri::Manager; /// Open a URL in the user's default browser -pub(crate) fn open_url(_app: &tauri::AppHandle, url: &SecretString) -> Result<()> { - // This is silly but it might work. - // Once closes, - // just go back to using Tauri's shell open like we do on Windows. - // - // Figure out who we were before `sudo` - let username = std::env::var("SUDO_USER")?; - std::process::Command::new("sudo") - // Make sure `XDG_RUNTIME_DIR` is preserved so we can find the Unix domain socket again - .arg("--preserve-env") - // Sudo back to our original username - .arg("--user") - .arg(username) - // Use the XDG wrapper for the user's default web browser (this will leak an `xdg-open` process if the user's first browser tab is the Firezone login. That process will end when the browser closes - .arg("xdg-open") - .arg(url.expose_secret()) - // Detach, since web browsers, and therefore xdg-open, typically block if there are no windows open and you're opening the first tab from CLI - .spawn()?; - +pub(crate) fn open_url(app: &tauri::AppHandle, url: &SecretString) -> Result<()> { + tauri::api::shell::open(&app.shell_scope(), url.expose_secret(), None)?; Ok(()) } diff --git a/scripts/tests/smoke-test-gui-linux.sh b/scripts/tests/smoke-test-gui-linux.sh index e80f1ab30..a2c2b6829 100755 --- a/scripts/tests/smoke-test-gui-linux.sh +++ b/scripts/tests/smoke-test-gui-linux.sh @@ -4,70 +4,79 @@ set -euo pipefail BUNDLE_ID="dev.firezone.client" -DEVICE_ID_PATH="/var/lib/$BUNDLE_ID/config/firezone-id.json" +#DEVICE_ID_PATH="/var/lib/$BUNDLE_ID/config/firezone-id.json" LOGS_PATH="$HOME/.cache/$BUNDLE_ID/data/logs" DUMP_PATH="$LOGS_PATH/last_crash.dmp" SETTINGS_PATH="$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" +RAN_BEFORE_PATH="$HOME/.local/share/$BUNDLE_ID/data/ran_before.txt" +SYMS_PATH="../target/debug/firezone-gui-client.syms" PACKAGE=firezone-gui-client export RUST_LOG=firezone_gui_client=debug,warn export WEBKIT_DISABLE_COMPOSITING_MODE=1 cargo build -p "$PACKAGE" +cargo install --quiet --locked dump_syms minidump-stackwalk +# The dwp doesn't actually do anything if the exe already has all the debug info +# Getting this to coordinate between Linux and Windows is tricky +dump_syms ../target/debug/firezone-gui-client --output "$SYMS_PATH" +ls -lash ../target/debug function smoke_test() { # Make sure the files we want to check don't exist on the system yet - sudo stat "$LOGS_PATH" && exit 1 - sudo stat "$SETTINGS_PATH" && exit 1 - sudo stat "$DEVICE_ID_PATH" && exit 1 + stat "$LOGS_PATH" && exit 1 + stat "$SETTINGS_PATH" && exit 1 + # TODO: The device ID will be written by the tunnel, not the GUI, so we can't check that. + # stat "$DEVICE_ID_PATH" && exit 1 + stat "$RAN_BEFORE_PATH" && exit 1 # Run the smoke test normally - sudo --preserve-env xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --no-deep-links smoke-test + if ! xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --no-deep-links smoke-test + then + minidump-stackwalk --symbols-path "$SYMS_PATH" "$DUMP_PATH" + exit 1 + fi # Note the device ID - DEVICE_ID_1=$(cat "$DEVICE_ID_PATH") + # DEVICE_ID_1=$(cat "$DEVICE_ID_PATH") # Make sure the files were written in the right paths # TODO: Inject some bogus sign-in sequence to test the actor_name file # https://stackoverflow.com/questions/41321092 - sudo bash -c "stat \"${LOGS_PATH}/\"connlib*log" - sudo stat "$SETTINGS_PATH" - sudo stat "$DEVICE_ID_PATH" + bash -c "stat \"${LOGS_PATH}/\"connlib*log" + stat "$SETTINGS_PATH" + # stat "$DEVICE_ID_PATH" + stat "$RAN_BEFORE_PATH" # Run the test again and make sure the device ID is not changed - sudo --preserve-env xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --no-deep-links smoke-test - DEVICE_ID_2=$(cat "$DEVICE_ID_PATH") + xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --no-deep-links smoke-test + # DEVICE_ID_2=$(cat "$DEVICE_ID_PATH") - if [ "$DEVICE_ID_1" != "$DEVICE_ID_2" ] - then - echo "The device ID should not change if the file is intact between runs" - exit 1 - fi + #if [ "$DEVICE_ID_1" != "$DEVICE_ID_2" ] + #then + # echo "The device ID should not change if the file is intact between runs" + # exit 1 + #fi # Clean up the files but not the folders - sudo rm -rf "$LOGS_PATH" - sudo rm "$SETTINGS_PATH" - sudo rm "$DEVICE_ID_PATH" + rm -rf "$LOGS_PATH" + rm "$SETTINGS_PATH" + # rm "$DEVICE_ID_PATH" + rm "$RAN_BEFORE_PATH" } function crash_test() { # Delete the crash file if present - sudo rm -f "$DUMP_PATH" + rm -f "$DUMP_PATH" # Fail if it returns success, this is supposed to crash - sudo --preserve-env xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --crash --no-deep-links && exit 1 + xvfb-run --auto-servernum ../target/debug/"$PACKAGE" --crash --no-deep-links && exit 1 # Fail if the crash file wasn't written - sudo stat "$DUMP_PATH" + stat "$DUMP_PATH" } function get_stacktrace() { - SYMS_PATH="../target/debug/firezone-gui-client.syms" - cargo install --quiet --locked dump_syms minidump-stackwalk - # The dwp doesn't actually do anything if the exe already has all the debug info - # Getting this to coordinate between Linux and Windows is tricky - dump_syms ../target/debug/firezone-gui-client --output "$SYMS_PATH" - ls -lash ../target/debug minidump-stackwalk --symbols-path "$SYMS_PATH" "$DUMP_PATH" } @@ -79,7 +88,4 @@ crash_test get_stacktrace # Clean up -sudo rm "$DUMP_PATH" - -# I'm not sure if the last command is handled specially, so explicitly exit with 0 -exit 0 +rm "$DUMP_PATH"