github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(gui-client): refuse to ever be elevated on Linux (#4232)

Browse Source 
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
This commit is contained in:
Reactor Scram
2024-03-21 09:42:48 -05:00
committed by GitHub
parent b0904e382a
commit 7fece80006
6 changed files with 117 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
rust/connlib/shared/src/device_id.rs
View File

@@ -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<DeviceId> {
{
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<DeviceId> {
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)]

6
rust/gui-client/src-tauri/src/client.rs
View File

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

12
rust/gui-client/src-tauri/src/client/elevation.rs
View File

@@ -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.");
}
}

130
rust/gui-client/src-tauri/src/client/gui.rs
View File

@@ -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<Session>,
/// 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<Notify>,
@@ -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,

22
rust/gui-client/src-tauri/src/client/gui/os_linux.rs
View File

@@ -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 <https://github.com/firezone/firezone/issues/3713> 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(())
}

70
scripts/tests/smoke-test-gui-linux.sh
View File

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