diff --git a/rust/Cargo.lock b/rust/Cargo.lock index c57065608..141d01dba 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -1972,7 +1972,6 @@ version = "1.0.0" dependencies = [ "anyhow", "firezone-headless-client", - "tokio", ] [[package]] diff --git a/rust/gui-client/src-tauri/src/bin/firezone-client-tunnel.rs b/rust/gui-client/src-tauri/src/bin/firezone-client-tunnel.rs index d55e6595c..53b338c45 100644 --- a/rust/gui-client/src-tauri/src/bin/firezone-client-tunnel.rs +++ b/rust/gui-client/src-tauri/src/bin/firezone-client-tunnel.rs @@ -1,4 +1,3 @@ -#[tokio::main] -async fn main() -> anyhow::Result<()> { - firezone_headless_client::run().await +fn main() -> anyhow::Result<()> { + firezone_headless_client::run() } diff --git a/rust/headless-client/Cargo.toml b/rust/headless-client/Cargo.toml index 8140b4a10..7504a9c20 100644 --- a/rust/headless-client/Cargo.toml +++ b/rust/headless-client/Cargo.toml @@ -13,9 +13,6 @@ clap = { version = "4.5", features = ["derive", "env"] } git-version = "0.3.9" humantime = "2.1" serde = { version = "1.0.197", features = ["derive"] } -# This actually relies on many other features in Tokio, so this will probably -# fail to build outside the workspace. -tokio = { version = "1.36.0", features = ["macros", "signal"] } tracing = { workspace = true } url = { version = "2.3.1", default-features = false } @@ -30,6 +27,9 @@ resolv-conf = "0.7.0" sd-notify = "0.4.1" # This is a pure Rust re-implementation, so it isn't vulnerable to CVE-2024-3094 serde_json = "1.0.115" secrecy = { workspace = true } +# This actually relies on many other features in Tokio, so this will probably +# fail to build outside the workspace. +tokio = { version = "1.36.0", features = ["macros", "signal"] } tokio-util = { version = "0.7.10", features = ["codec"] } [lints] diff --git a/rust/headless-client/src/imp_linux.rs b/rust/headless-client/src/imp_linux.rs index 21a97b11c..ad0ec3918 100644 --- a/rust/headless-client/src/imp_linux.rs +++ b/rust/headless-client/src/imp_linux.rs @@ -1,6 +1,6 @@ //! Implementation, Linux-specific -use super::{Cli, Cmd}; +use super::{Cli, Cmd, TOKEN_ENV_KEY}; use anyhow::{bail, Context, Result}; use clap::Parser; use connlib_client_shared::{file_logger, Callbacks, Session, Sockets}; @@ -37,8 +37,27 @@ pub fn default_token_path() -> PathBuf { .join("token") } -pub async fn run() -> Result<()> { - let cli = Cli::parse(); +pub fn run() -> Result<()> { + let mut cli = Cli::parse(); + + // Modifying the environment of a running process is unsafe. If any other + // thread is reading or writing the environment, something bad can happen. + // So `run` must take over as early as possible during startup, and + // take the token env var before any other threads spawn. + + let token_env_var = cli.token.take().map(SecretString::from); + let cli = cli; + + // Docs indicate that `remove_var` should actually be marked unsafe + // SAFETY: We haven't spawned any other threads, this code should be the first + // thing to run after entering `main`. So nobody else is reading the environment. + #[allow(unused_unsafe)] + unsafe { + // This removes the token from the environment per . We run as root so it may not do anything besides defense-in-depth. + std::env::remove_var(TOKEN_ENV_KEY); + } + assert!(std::env::var(TOKEN_ENV_KEY).is_err()); + let (layer, _handle) = cli.log_dir.as_deref().map(file_logger::layer).unzip(); setup_global_subscriber(layer); @@ -46,47 +65,65 @@ pub async fn run() -> Result<()> { match cli.command() { Cmd::Auto => { - if let Some(token) = token(&cli)? { - run_standalone(cli, &token).await + if let Some(token) = get_token(token_env_var, &cli)? { + run_standalone(cli, &token) } else { - run_ipc_service(cli).await + run_ipc_service(cli) } } - Cmd::IpcService => run_ipc_service(cli).await, + Cmd::IpcService => run_ipc_service(cli), Cmd::Standalone => { - let token = token(&cli)?.with_context(|| { + let token = get_token(token_env_var, &cli)?.with_context(|| { format!( - "Can't find the Firezone token in $FIREZONE_TOKEN or in `{}`", + "Can't find the Firezone token in ${TOKEN_ENV_KEY} or in `{}`", cli.token_path ) })?; - run_standalone(cli, &token).await + run_standalone(cli, &token) } - Cmd::StubIpcClient => run_debug_ipc_client(cli).await, + Cmd::StubIpcClient => run_debug_ipc_client(cli), } } -/// Try to retrieve the token from CLI arg, env var, or disk +/// Read the token from disk if it was not in the environment +/// +/// # Returns +/// - `Ok(None)` if there is no token to be found +/// - `Ok(Some(_))` if we found the token +/// - `Err(_)` if we found the token on disk but failed to read it +fn get_token(token_env_var: Option, cli: &Cli) -> Result> { + // This is very simple but I don't want to write it twice + if let Some(token) = token_env_var { + return Ok(Some(token)); + } + read_token_file(cli) +} + +/// Try to retrieve the token from disk /// /// Sync because we do blocking file I/O -fn token(cli: &Cli) -> Result> { +fn read_token_file(cli: &Cli) -> Result> { let path = PathBuf::from(&cli.token_path); - if let Some(token) = &cli.token { - // Token was provided in CLI args or env var - // Not very secure, but we do get the token + if let Ok(token) = std::env::var(TOKEN_ENV_KEY) { + std::env::remove_var(TOKEN_ENV_KEY); + + let token = SecretString::from(token); + // Token was provided in env var tracing::info!( ?path, - "Found token in environment or CLI args, ignoring any token that may be on disk." + ?TOKEN_ENV_KEY, + "Found token in env var, ignoring any token that may be on disk." ); - return Ok(Some(token.clone().into())); + return Ok(Some(token)); } let Ok(stat) = nix::sys::stat::fstatat(None, &path, nix::fcntl::AtFlags::empty()) else { // File doesn't exist or can't be read tracing::info!( ?path, - "No token found in CLI args, in environment, or on disk" + ?TOKEN_ENV_KEY, + "No token found in env var or on disk" ); return Ok(None); }; @@ -115,19 +152,21 @@ fn token(cli: &Cli) -> Result> { tracing::info!(?path, "Token file existed but now is unreadable"); return Ok(None); }; - let s = String::from_utf8(bytes)?; - let token = s.trim().to_string(); + let token = String::from_utf8(bytes)?.trim().to_string(); + let token = SecretString::from(token); tracing::info!(?path, "Loaded token from disk"); - Ok(Some(token.into())) + Ok(Some(token)) } -async fn run_standalone(cli: Cli, token: &SecretString) -> Result<()> { +fn run_standalone(cli: Cli, token: &SecretString) -> Result<()> { tracing::info!("Running in standalone mode"); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build()?; + let _guard = rt.enter(); let max_partition_time = cli.max_partition_time.map(|d| d.into()); - let callbacks = CallbackHandler; - // AKA "Device ID", not the Firezone slug let firezone_id = match cli.firezone_id { Some(id) => id, @@ -147,9 +186,9 @@ async fn run_standalone(cli: Cli, token: &SecretString) -> Result<()> { Sockets::new(), private_key, None, - callbacks.clone(), + CallbackHandler, max_partition_time, - tokio::runtime::Handle::current(), + rt.handle().clone(), ); // TODO: this should be added dynamically session.set_dns(system_resolvers(get_dns_control_from_env()).unwrap_or_default()); @@ -157,27 +196,29 @@ async fn run_standalone(cli: Cli, token: &SecretString) -> Result<()> { let mut sigint = tokio::signal::unix::signal(SignalKind::interrupt())?; let mut sighup = tokio::signal::unix::signal(SignalKind::hangup())?; - future::poll_fn(|cx| loop { - if sigint.poll_recv(cx).is_ready() { - tracing::debug!("Received SIGINT"); + let result = rt.block_on(async { + future::poll_fn(|cx| loop { + if sigint.poll_recv(cx).is_ready() { + tracing::debug!("Received SIGINT"); - return Poll::Ready(std::io::Result::Ok(())); - } + return Poll::Ready(std::io::Result::Ok(())); + } - if sighup.poll_recv(cx).is_ready() { - tracing::debug!("Received SIGHUP"); + if sighup.poll_recv(cx).is_ready() { + tracing::debug!("Received SIGHUP"); - session.reconnect(); - continue; - } + session.reconnect(); + continue; + } - return Poll::Pending; - }) - .await?; + return Poll::Pending; + }) + .await + }); session.disconnect(); - Ok(()) + Ok(result?) } fn system_resolvers(dns_control_method: Option) -> Result> { @@ -258,20 +299,25 @@ fn parse_resolvectl_output(s: &str) -> Vec { .collect() } -async fn run_debug_ipc_client(_cli: Cli) -> Result<()> { - tracing::info!(pid = std::process::id(), "run_debug_ipc_client"); - let stream = UnixStream::connect(SOCK_PATH) - .await - .with_context(|| format!("couldn't connect to UDS at {SOCK_PATH}"))?; - let mut stream = IpcStream::new(stream, LengthDelimitedCodec::new()); +fn run_debug_ipc_client(_cli: Cli) -> Result<()> { + let rt = tokio::runtime::Runtime::new()?; + rt.block_on(async { + tracing::info!(pid = std::process::id(), "run_debug_ipc_client"); + let stream = UnixStream::connect(SOCK_PATH) + .await + .with_context(|| format!("couldn't connect to UDS at {SOCK_PATH}"))?; + let mut stream = IpcStream::new(stream, LengthDelimitedCodec::new()); - stream.send(serde_json::to_string("Hello")?.into()).await?; + stream.send(serde_json::to_string("Hello")?.into()).await?; + Ok::<_, anyhow::Error>(()) + })?; Ok(()) } -async fn run_ipc_service(_cli: Cli) -> Result<()> { +fn run_ipc_service(_cli: Cli) -> Result<()> { + let rt = tokio::runtime::Runtime::new()?; tracing::info!("run_daemon"); - ipc_listen().await + rt.block_on(async { ipc_listen().await }) } async fn ipc_listen() -> Result<()> { diff --git a/rust/headless-client/src/lib.rs b/rust/headless-client/src/lib.rs index c38243a93..234e66eea 100644 --- a/rust/headless-client/src/lib.rs +++ b/rust/headless-client/src/lib.rs @@ -25,11 +25,14 @@ mod imp_windows { todo!() } - pub async fn run() -> anyhow::Result<()> { + pub fn run() -> anyhow::Result<()> { let cli = super::Cli::parse(); let _cmd = cli.command(); tracing::info!(git_version = crate::GIT_VERSION); - Ok(()) + // Clippy will complain that the `Result` type is pointless if we can't + // possibly throw an error, because it doesn't see that the Linux impl does + // throw errors + anyhow::bail!("`headless-client` is not implemented for Windows yet"); } } #[cfg(target_os = "windows")] @@ -48,6 +51,8 @@ pub const GIT_VERSION: &str = git_version::git_version!( fallback = "unknown" ); +const TOKEN_ENV_KEY: &str = "FIREZONE_TOKEN"; + #[derive(clap::Parser)] #[command(author, version, about, long_about = None)] struct Cli { @@ -71,14 +76,17 @@ struct Cli { check: bool, /// Token generated by the portal to authorize websocket connection. - - // TODO: It isn't good for security to pass the token as a CLI arg. - // If we pass it as an env var, we should remove it immediately so that - // child processes don't inherit it. Reading it from a file is probably safest. - #[arg(env = "FIREZONE_TOKEN", hide = true, long)] - pub token: Option, + // systemd recommends against passing secrets through env vars: + // + #[arg(env = TOKEN_ENV_KEY, hide = true)] + token: Option, /// A filesystem path where the token can be found + + // Apparently passing secrets through stdin is the most secure method, but + // until anyone asks for it, env vars are okay and files on disk are slightly better. + // (Since we run as root and the env var on a headless system is probably stored + // on disk somewhere anyway.) #[arg(default_value_t = default_token_path().display().to_string(), env = "FIREZONE_TOKEN_PATH", long)] token_path: String, diff --git a/rust/headless-client/src/main.rs b/rust/headless-client/src/main.rs index d55e6595c..53b338c45 100644 --- a/rust/headless-client/src/main.rs +++ b/rust/headless-client/src/main.rs @@ -1,4 +1,3 @@ -#[tokio::main] -async fn main() -> anyhow::Result<()> { - firezone_headless_client::run().await +fn main() -> anyhow::Result<()> { + firezone_headless_client::run() } diff --git a/rust/linux-client/Cargo.toml b/rust/linux-client/Cargo.toml index 49d52d142..f0b4fc6f2 100644 --- a/rust/linux-client/Cargo.toml +++ b/rust/linux-client/Cargo.toml @@ -10,7 +10,6 @@ authors = ["Firezone, Inc."] [dependencies] anyhow = { version = "1.0" } firezone-headless-client = { path = "../headless-client" } -tokio = { version = "1.36.0", features = ["macros", "signal"] } [lints] workspace = true diff --git a/rust/linux-client/src/main.rs b/rust/linux-client/src/main.rs index d55e6595c..53b338c45 100644 --- a/rust/linux-client/src/main.rs +++ b/rust/linux-client/src/main.rs @@ -1,4 +1,3 @@ -#[tokio::main] -async fn main() -> anyhow::Result<()> { - firezone_headless_client::run().await +fn main() -> anyhow::Result<()> { + firezone_headless_client::run() } diff --git a/scripts/tests/token-path.sh b/scripts/tests/token-path.sh index bc21a4a23..674947c59 100755 --- a/scripts/tests/token-path.sh +++ b/scripts/tests/token-path.sh @@ -3,29 +3,36 @@ source "./scripts/tests/lib.sh" BINARY_NAME=firezone-linux-client +TOKEN="n.SFMyNTY.g2gDaANtAAAAJGM4OWJjYzhjLTkzOTItNGRhZS1hNDBkLTg4OGFlZjZkMjhlMG0AAAAkN2RhN2QxY2QtMTExYy00NGE3LWI1YWMtNDAyN2I5ZDIzMGU1bQAAACtBaUl5XzZwQmstV0xlUkFQenprQ0ZYTnFJWktXQnMyRGR3XzJ2Z0lRdkZnbgYAGUmu74wBYgABUYA.UN3vSLLcAMkHeEh5VHumPOutkuue8JA6wlxM9JxJEPE" TOKEN_PATH="token" sudo cp "rust/target/debug/firezone-headless-client" "/usr/bin/$BINARY_NAME" -# Check should fail because there's no token yet -sudo "$BINARY_NAME" standalone --check && exit 1 +# Fails because there's no token yet +sudo "$BINARY_NAME" --check standalone && exit 1 + +# Pass if we use the env var +sudo FIREZONE_TOKEN="$TOKEN" "$BINARY_NAME" --check standalone + +# Fails because passing tokens as CLI args is not allowed anymore +sudo "$BINARY_NAME" --check --token "$TOKEN" standalone && exit 1 touch "$TOKEN_PATH" chmod 600 "$TOKEN_PATH" sudo chown root:root "$TOKEN_PATH" -echo "n.SFMyNTY.g2gDaANtAAAAJGM4OWJjYzhjLTkzOTItNGRhZS1hNDBkLTg4OGFlZjZkMjhlMG0AAAAkN2RhN2QxY2QtMTExYy00NGE3LWI1YWMtNDAyN2I5ZDIzMGU1bQAAACtBaUl5XzZwQmstV0xlUkFQenprQ0ZYTnFJWktXQnMyRGR3XzJ2Z0lRdkZnbgYAGUmu74wBYgABUYA.UN3vSLLcAMkHeEh5VHumPOutkuue8JA6wlxM9JxJEPE" | sudo tee "$TOKEN_PATH" > /dev/null +echo "$TOKEN" | sudo tee "$TOKEN_PATH" > /dev/null -# Check should fail because the token is not in the default path +# Fails because the token is not in the default path sudo "$BINARY_NAME" --check standalone && exit 1 -# Check should pass if we tell it where to look +# Passes if we tell it where to look sudo "$BINARY_NAME" --check --token-path "$TOKEN_PATH" standalone # Move the token to the default path sudo mkdir /etc/dev.firezone.client sudo mv "$TOKEN_PATH" /etc/dev.firezone.client/token -# Check should now pass with the default path +# Now passes with the default path sudo "$BINARY_NAME" --check standalone # Redundant, but helps if the last command has an `&& exit 1` diff --git a/website/src/app/kb/user-guides/linux-client/readme.mdx b/website/src/app/kb/user-guides/linux-client/readme.mdx index ef8d33868..cf990299d 100644 --- a/website/src/app/kb/user-guides/linux-client/readme.mdx +++ b/website/src/app/kb/user-guides/linux-client/readme.mdx @@ -64,8 +64,6 @@ Commands: help Print this message or the help of the given subcommand(s) Options: - --token - Token generated by the portal to authorize websocket connection [env: FIREZONE_TOKEN=] --token-path A filesystem path where the token can be found [env: FIREZONE_TOKEN_PATH=] [default: /etc/dev.firezone.client/token] -i, --firezone-id