fix(linux-client): forbid passing the token as a CLI arg (#4683)

Closes #4682 
Closes #4691 

```[tasklist]
# Before merging
- [x] Wait for `linux-group` test to go green on `main` (#4692)
- [ ] Wait for those browsers tests to get fixed
- [ ] *All* compatibility tests must pass on this branch
```

---------

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

rust/Cargo.lock

@@ -1972,7 +1972,6 @@ version = "1.0.0"
 dependencies = [
  "anyhow",
  "firezone-headless-client",
- "tokio",
 ]
 
 [[package]]

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()
 }

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. <https://github.com/firezone/firezone/pull/4328#discussion_r1540342142>
-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. <https://github.com/firezone/firezone/pull/4328#discussion_r1540342142>
+tokio = { version = "1.36.0", features = ["macros", "signal"] }
 tokio-util = { version = "0.7.10", features = ["codec"] }
 
 [lints]

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 <https://security.stackexchange.com/a/271285>. 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<SecretString>, cli: &Cli) -> Result<Option<SecretString>> {
+    // 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<Option<SecretString>> {
+fn read_token_file(cli: &Cli) -> Result<Option<SecretString>> {
     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<Option<SecretString>> {
         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<DnsControlMethod>) -> Result<Vec<IpAddr>> {
@@ -258,20 +299,25 @@ fn parse_resolvectl_output(s: &str) -> Vec<IpAddr> {
         .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<()> {

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<String>,
+    // systemd recommends against passing secrets through env vars:
+    // <https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#Environment=>
+    #[arg(env = TOKEN_ENV_KEY, hide = true)]
+    token: Option<String>,
 
     /// 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,
 

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()
 }

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

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()
 }

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`

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>
-          Token generated by the portal to authorize websocket connection [env: FIREZONE_TOKEN=]
       --token-path <TOKEN_PATH>
           A filesystem path where the token can be found [env: FIREZONE_TOKEN_PATH=] [default: /etc/dev.firezone.client/token]
   -i, --firezone-id <FIREZONE_ID>