mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-07 03:00:29 +00:00
refactor(auth): scrub FABRO_WORKER_TOKEN from worker env at startup
The worker subprocess is spawned with env_clear+allowlist by the server, so the only sensitive value in its env is FABRO_WORKER_TOKEN itself. Read the token and remove_var it from the process env in main() before Tokio starts worker threads, then thread it explicitly through runner::execute(&str). Every descendant (hooks, local sandbox, devcontainer initializeCommand, MCP stdio, etc.) now inherits a worker env with no bearer in it, so an unscrubbed spawn site cannot leak the token. This makes the prior denylist scrub in fabro-hooks and fabro-sandbox redundant — delete it and the shared WORKER_SECRET_ENV_DENYLIST constant. The sandbox keeps its _api_key/_secret/ _token/_password/_credential suffix heuristic for user-supplied env_vars hygiene. Extend the server-dispatched-worker env-leak integration test to also assert a Bash stage running in the worker does not observe FABRO_WORKER_TOKEN. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
fbb924b9c4
commit
077469d0c6
10 changed files with 53 additions and 65 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -1983,7 +1983,6 @@ dependencies = [
|
||||||
"fabro-github",
|
"fabro-github",
|
||||||
"fabro-proc",
|
"fabro-proc",
|
||||||
"fabro-types",
|
"fabro-types",
|
||||||
"fabro-util",
|
|
||||||
"futures",
|
"futures",
|
||||||
"git2",
|
"git2",
|
||||||
"glob",
|
"glob",
|
||||||
|
|
|
||||||
|
|
@ -47,6 +47,7 @@ There is no compatibility layer for removed secrets and no startup-time secret g
|
||||||
|
|
||||||
- Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables.
|
- Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables.
|
||||||
- Authority-bearing values are re-injected intentionally. For worker subprocesses this is `FABRO_WORKER_TOKEN`, not user auth state such as `FABRO_DEV_TOKEN` or `auth.json`.
|
- Authority-bearing values are re-injected intentionally. For worker subprocesses this is `FABRO_WORKER_TOKEN`, not user auth state such as `FABRO_DEV_TOKEN` or `auth.json`.
|
||||||
|
- The worker reads `FABRO_WORKER_TOKEN` from its env at startup (in `main()` before Tokio initializes) and immediately calls `std::env::remove_var` to scrub it. The token then flows through function arguments to `runner::execute`. Every descendant process (hooks, sandbox commands, devcontainer setup, MCP stdio, etc.) therefore inherits a worker env that no longer contains the bearer, so an unscrubbed spawn site cannot leak it.
|
||||||
- The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal).
|
- The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal).
|
||||||
|
|
||||||
## Tests
|
## Tests
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,4 @@
|
||||||
use anyhow::Result;
|
use anyhow::{Result, anyhow};
|
||||||
use fabro_util::terminal::Styles;
|
use fabro_util::terminal::Styles;
|
||||||
|
|
||||||
use crate::args::{AttachArgs, RunCommands, RunWorkerArgs, StartArgs};
|
use crate::args::{AttachArgs, RunCommands, RunWorkerArgs, StartArgs};
|
||||||
|
|
@ -23,7 +23,11 @@ pub(crate) mod ssh;
|
||||||
pub(crate) mod start;
|
pub(crate) mod start;
|
||||||
pub(crate) mod wait;
|
pub(crate) mod wait;
|
||||||
|
|
||||||
pub(crate) async fn dispatch(cmd: RunCommands, base_ctx: &CommandContext) -> Result<()> {
|
pub(crate) async fn dispatch(
|
||||||
|
cmd: RunCommands,
|
||||||
|
base_ctx: &CommandContext,
|
||||||
|
worker_token: Option<String>,
|
||||||
|
) -> Result<()> {
|
||||||
let printer = base_ctx.printer();
|
let printer = base_ctx.printer();
|
||||||
|
|
||||||
match cmd {
|
match cmd {
|
||||||
|
|
@ -76,7 +80,22 @@ pub(crate) async fn dispatch(cmd: RunCommands, base_ctx: &CommandContext) -> Res
|
||||||
run_dir,
|
run_dir,
|
||||||
run_id,
|
run_id,
|
||||||
mode,
|
mode,
|
||||||
}) => Box::pin(runner::execute(run_id, server, storage_dir, run_dir, mode)).await,
|
}) => {
|
||||||
|
let worker_token = worker_token
|
||||||
|
.filter(|token| !token.trim().is_empty())
|
||||||
|
.ok_or_else(|| {
|
||||||
|
anyhow!("FABRO_WORKER_TOKEN is required for worker subprocess auth")
|
||||||
|
})?;
|
||||||
|
Box::pin(runner::execute(
|
||||||
|
run_id,
|
||||||
|
server,
|
||||||
|
storage_dir,
|
||||||
|
run_dir,
|
||||||
|
mode,
|
||||||
|
&worker_token,
|
||||||
|
))
|
||||||
|
.await
|
||||||
|
}
|
||||||
RunCommands::Diff(args) => diff::run(args, base_ctx).await,
|
RunCommands::Diff(args) => diff::run(args, base_ctx).await,
|
||||||
RunCommands::Logs(args) => {
|
RunCommands::Logs(args) => {
|
||||||
let styles = Styles::detect_stdout();
|
let styles = Styles::detect_stdout();
|
||||||
|
|
|
||||||
|
|
@ -59,19 +59,13 @@ pub(crate) async fn execute(
|
||||||
storage_dir: Option<PathBuf>,
|
storage_dir: Option<PathBuf>,
|
||||||
run_dir: PathBuf,
|
run_dir: PathBuf,
|
||||||
mode: RunWorkerMode,
|
mode: RunWorkerMode,
|
||||||
|
worker_token: &str,
|
||||||
) -> Result<()> {
|
) -> Result<()> {
|
||||||
let _ = fabro_proc::title_init();
|
let _ = fabro_proc::title_init();
|
||||||
set_worker_title(&run_id, initial_worker_title_phase(mode));
|
set_worker_title(&run_id, initial_worker_title_phase(mode));
|
||||||
|
|
||||||
let worker_token = std::env::var("FABRO_WORKER_TOKEN")
|
|
||||||
.map_err(|_| anyhow!("FABRO_WORKER_TOKEN is required for worker subprocess auth"))?;
|
|
||||||
if worker_token.trim().is_empty() {
|
|
||||||
return Err(anyhow!(
|
|
||||||
"FABRO_WORKER_TOKEN is required for worker subprocess auth"
|
|
||||||
));
|
|
||||||
}
|
|
||||||
let target = server.parse::<fabro_client::ServerTarget>()?;
|
let target = server.parse::<fabro_client::ServerTarget>()?;
|
||||||
let client = server_client::connect_server_target_with_bearer(&target, &worker_token).await?;
|
let client = server_client::connect_server_target_with_bearer(&target, worker_token).await?;
|
||||||
let run_store = HttpRunStore::connect(run_id, client.clone_for_reuse()).await?;
|
let run_store = HttpRunStore::connect(run_id, client.clone_for_reuse()).await?;
|
||||||
let run_state = run_store
|
let run_state = run_store
|
||||||
.state()
|
.state()
|
||||||
|
|
@ -84,7 +78,7 @@ pub(crate) async fn execute(
|
||||||
let artifact_sink = Some(ArtifactSink::Uploader(build_artifact_uploader(
|
let artifact_sink = Some(ArtifactSink::Uploader(build_artifact_uploader(
|
||||||
run_id,
|
run_id,
|
||||||
client.clone_for_reuse(),
|
client.clone_for_reuse(),
|
||||||
worker_token,
|
worker_token.to_owned(),
|
||||||
)));
|
)));
|
||||||
let interviewer = Arc::new(ControlInterviewer::new());
|
let interviewer = Arc::new(ControlInterviewer::new());
|
||||||
let cancel_token = Arc::new(AtomicBool::new(false));
|
let cancel_token = Arc::new(AtomicBool::new(false));
|
||||||
|
|
|
||||||
|
|
@ -72,12 +72,33 @@ async fn main() {
|
||||||
std::process::exit(commands::render_graph::execute());
|
std::process::exit(commands::render_graph::execute());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Capture the worker bearer token immediately and scrub it from the process
|
||||||
|
// env before any subprocess can be spawned. Every descendant of the worker
|
||||||
|
// (hooks, sandbox commands, devcontainer setup, MCP stdio, etc.) therefore
|
||||||
|
// inherits a process env that no longer contains FABRO_WORKER_TOKEN, so an
|
||||||
|
// unscrubbed spawn site cannot leak it. The token flows to `runner::execute`
|
||||||
|
// through an explicit function argument instead of the environment.
|
||||||
|
let worker_token = if subcommand == Some("__run-worker") {
|
||||||
|
let token = std::env::var("FABRO_WORKER_TOKEN").ok();
|
||||||
|
#[expect(
|
||||||
|
clippy::disallowed_methods,
|
||||||
|
reason = "Scrub the worker bearer from this process's env before any \
|
||||||
|
child process is spawned, so no descendant can inherit it."
|
||||||
|
)]
|
||||||
|
{
|
||||||
|
std::env::remove_var("FABRO_WORKER_TOKEN");
|
||||||
|
}
|
||||||
|
token
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
};
|
||||||
|
|
||||||
tel_panic::install_panic_hook();
|
tel_panic::install_panic_hook();
|
||||||
fabro_telemetry::init_cli();
|
fabro_telemetry::init_cli();
|
||||||
|
|
||||||
let start = std::time::Instant::now();
|
let start = std::time::Instant::now();
|
||||||
|
|
||||||
let (command_name, result) = Box::pin(main_inner()).await;
|
let (command_name, result) = Box::pin(main_inner(worker_token)).await;
|
||||||
let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX);
|
let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX);
|
||||||
let exit_code = result.as_ref().err().map_or(0, exit::exit_code_for);
|
let exit_code = result.as_ref().err().map_or(0, exit::exit_code_for);
|
||||||
|
|
||||||
|
|
@ -145,7 +166,7 @@ async fn main() {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn main_inner() -> (String, Result<()>) {
|
async fn main_inner(worker_token: Option<String>) -> (String, Result<()>) {
|
||||||
let _ = default_provider().install_default();
|
let _ = default_provider().install_default();
|
||||||
|
|
||||||
let cli = Cli::parse();
|
let cli = Cli::parse();
|
||||||
|
|
@ -206,7 +227,7 @@ async fn main_inner() -> (String, Result<()>) {
|
||||||
commands::exec::execute(args, &base_ctx).await?;
|
commands::exec::execute(args, &base_ctx).await?;
|
||||||
}
|
}
|
||||||
Commands::RunCmd(cmd) => {
|
Commands::RunCmd(cmd) => {
|
||||||
Box::pin(commands::run::dispatch(cmd, &base_ctx)).await?;
|
Box::pin(commands::run::dispatch(cmd, &base_ctx, worker_token)).await?;
|
||||||
}
|
}
|
||||||
Commands::Preflight(args) => {
|
Commands::Preflight(args) => {
|
||||||
commands::preflight::execute(args, &base_ctx).await?;
|
commands::preflight::execute(args, &base_ctx).await?;
|
||||||
|
|
|
||||||
|
|
@ -184,6 +184,7 @@ fn assert_no_worker_env_leak(scope: &str, content: &str) {
|
||||||
for needle in [
|
for needle in [
|
||||||
"MY_API_TOKEN=",
|
"MY_API_TOKEN=",
|
||||||
"NEW_RELIC_LICENSE_KEY=",
|
"NEW_RELIC_LICENSE_KEY=",
|
||||||
|
"FABRO_WORKER_TOKEN=",
|
||||||
LEAKED_WORKER_PARENT_TOKEN,
|
LEAKED_WORKER_PARENT_TOKEN,
|
||||||
LEAKED_NEW_RELIC_LICENSE,
|
LEAKED_NEW_RELIC_LICENSE,
|
||||||
] {
|
] {
|
||||||
|
|
@ -522,7 +523,7 @@ methods = ["dev-token"]
|
||||||
graph [goal="Verify worker subprocess env isolation", default_max_retries=0]
|
graph [goal="Verify worker subprocess env isolation", default_max_retries=0]
|
||||||
start [shape=Mdiamond, label="Start"]
|
start [shape=Mdiamond, label="Start"]
|
||||||
exit [shape=Msquare, label="Exit"]
|
exit [shape=Msquare, label="Exit"]
|
||||||
probe [shape=parallelogram, label="Probe", script="echo probe-ran; for key in $(printf 'MY%s NEW%s' '_API_TOKEN' '_RELIC_LICENSE_KEY'); do value=$(printenv \"$key\" || true); if [ -n \"$value\" ]; then echo \"$key=$value\"; fi; done"]
|
probe [shape=parallelogram, label="Probe", script="echo probe-ran; for key in $(printf 'MY%s NEW%s FABRO%s' '_API_TOKEN' '_RELIC_LICENSE_KEY' '_WORKER_TOKEN'); do value=$(printenv \"$key\" || true); if [ -n \"$value\" ]; then echo \"$key=$value\"; fi; done"]
|
||||||
start -> probe -> exit
|
start -> probe -> exit
|
||||||
}
|
}
|
||||||
"#,
|
"#,
|
||||||
|
|
|
||||||
|
|
@ -13,7 +13,7 @@ use fabro_llm::generate::{GenerateParams, generate_object};
|
||||||
use fabro_llm::types::{Message, Request, ToolResult};
|
use fabro_llm::types::{Message, Request, ToolResult};
|
||||||
use fabro_template::{TemplateContext, render as render_template};
|
use fabro_template::{TemplateContext, render as render_template};
|
||||||
use fabro_types::settings::InterpString;
|
use fabro_types::settings::InterpString;
|
||||||
use fabro_util::env::{Env, SystemEnv, WORKER_SECRET_ENV_DENYLIST};
|
use fabro_util::env::{Env, SystemEnv};
|
||||||
use tokio::process::Command as TokioCommand;
|
use tokio::process::Command as TokioCommand;
|
||||||
use tokio::time::timeout as tokio_timeout;
|
use tokio::time::timeout as tokio_timeout;
|
||||||
use tokio_util::sync::CancellationToken;
|
use tokio_util::sync::CancellationToken;
|
||||||
|
|
@ -77,12 +77,6 @@ where
|
||||||
pub struct HookExecutorImpl;
|
pub struct HookExecutorImpl;
|
||||||
|
|
||||||
impl HookExecutorImpl {
|
impl HookExecutorImpl {
|
||||||
fn scrub_worker_secret_env(cmd: &mut TokioCommand) {
|
|
||||||
for key in WORKER_SECRET_ENV_DENYLIST {
|
|
||||||
cmd.env_remove(key);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
/// Parse a hook decision from JSON stdout and exit code.
|
/// Parse a hook decision from JSON stdout and exit code.
|
||||||
fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision {
|
fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision {
|
||||||
if exit_code == 0 {
|
if exit_code == 0 {
|
||||||
|
|
@ -195,7 +189,6 @@ impl HookExecutorImpl {
|
||||||
if let Some(wd) = work_dir {
|
if let Some(wd) = work_dir {
|
||||||
cmd.current_dir(wd);
|
cmd.current_dir(wd);
|
||||||
}
|
}
|
||||||
Self::scrub_worker_secret_env(&mut cmd);
|
|
||||||
for (k, v) in &env_vars {
|
for (k, v) in &env_vars {
|
||||||
cmd.env(k, v);
|
cmd.env(k, v);
|
||||||
}
|
}
|
||||||
|
|
@ -847,23 +840,6 @@ mod tests {
|
||||||
assert_eq!(result.decision, HookDecision::Proceed);
|
assert_eq!(result.decision, HookDecision::Proceed);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
|
||||||
fn host_command_scrubs_worker_secret_env() {
|
|
||||||
let mut cmd = TokioCommand::new("sh");
|
|
||||||
HookExecutorImpl::scrub_worker_secret_env(&mut cmd);
|
|
||||||
|
|
||||||
let removed = cmd
|
|
||||||
.as_std()
|
|
||||||
.get_envs()
|
|
||||||
.filter(|(_, value)| value.is_none())
|
|
||||||
.map(|(name, _)| name.to_string_lossy().into_owned())
|
|
||||||
.collect::<Vec<_>>();
|
|
||||||
|
|
||||||
for key in WORKER_SECRET_ENV_DENYLIST {
|
|
||||||
assert!(removed.iter().any(|name| name == key));
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn no_hook_type_blocks() {
|
async fn no_hook_type_blocks() {
|
||||||
let executor = HookExecutorImpl;
|
let executor = HookExecutorImpl;
|
||||||
|
|
|
||||||
|
|
@ -30,7 +30,6 @@ strum.workspace = true
|
||||||
tracing.workspace = true
|
tracing.workspace = true
|
||||||
base64.workspace = true
|
base64.workspace = true
|
||||||
fabro-proc = { path = "../fabro-proc" }
|
fabro-proc = { path = "../fabro-proc" }
|
||||||
fabro-util = { path = "../fabro-util" }
|
|
||||||
shlex = "1"
|
shlex = "1"
|
||||||
|
|
||||||
# local
|
# local
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,6 @@ use std::path::{Path, PathBuf};
|
||||||
use std::time::Instant;
|
use std::time::Instant;
|
||||||
|
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
use fabro_util::env::WORKER_SECRET_ENV_DENYLIST;
|
|
||||||
use tokio::io::AsyncReadExt;
|
use tokio::io::AsyncReadExt;
|
||||||
use tokio::process::{Child, Command};
|
use tokio::process::{Child, Command};
|
||||||
use tokio::task::spawn_blocking;
|
use tokio::task::spawn_blocking;
|
||||||
|
|
@ -58,9 +57,6 @@ impl LocalSandbox {
|
||||||
if Self::ENV_SAFELIST.contains(&key) {
|
if Self::ENV_SAFELIST.contains(&key) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
if WORKER_SECRET_ENV_DENYLIST.contains(&key) {
|
|
||||||
return true;
|
|
||||||
}
|
|
||||||
let lower = key.to_lowercase();
|
let lower = key.to_lowercase();
|
||||||
lower.ends_with("_api_key")
|
lower.ends_with("_api_key")
|
||||||
|| lower.ends_with("_secret")
|
|| lower.ends_with("_secret")
|
||||||
|
|
@ -740,9 +736,6 @@ mod tests {
|
||||||
assert!(LocalSandbox::should_filter_env_var("MY_CREDENTIAL"));
|
assert!(LocalSandbox::should_filter_env_var("MY_CREDENTIAL"));
|
||||||
assert!(LocalSandbox::should_filter_env_var("FABRO_WORKER_TOKEN"));
|
assert!(LocalSandbox::should_filter_env_var("FABRO_WORKER_TOKEN"));
|
||||||
assert!(LocalSandbox::should_filter_env_var("SESSION_SECRET"));
|
assert!(LocalSandbox::should_filter_env_var("SESSION_SECRET"));
|
||||||
assert!(LocalSandbox::should_filter_env_var(
|
|
||||||
"GITHUB_APP_PRIVATE_KEY"
|
|
||||||
));
|
|
||||||
// Case insensitive
|
// Case insensitive
|
||||||
assert!(LocalSandbox::should_filter_env_var("my_api_key"));
|
assert!(LocalSandbox::should_filter_env_var("my_api_key"));
|
||||||
assert!(LocalSandbox::should_filter_env_var("Some_Secret"));
|
assert!(LocalSandbox::should_filter_env_var("Some_Secret"));
|
||||||
|
|
|
||||||
|
|
@ -1,18 +1,3 @@
|
||||||
/// Server-managed secret env vars that must never leak into subprocesses
|
|
||||||
/// (hook executors, local sandbox). A suffix filter (`_secret`, `_token`,
|
|
||||||
/// `_api_key`, `_password`, `_credential`) catches many secrets by convention,
|
|
||||||
/// but these names don't match those suffixes and are explicitly named to
|
|
||||||
/// eliminate ambiguity when the list is inspected.
|
|
||||||
pub const WORKER_SECRET_ENV_DENYLIST: &[&str] = &[
|
|
||||||
"FABRO_WORKER_TOKEN",
|
|
||||||
"SESSION_SECRET",
|
|
||||||
"FABRO_JWT_PRIVATE_KEY",
|
|
||||||
"FABRO_JWT_PUBLIC_KEY",
|
|
||||||
"GITHUB_APP_PRIVATE_KEY",
|
|
||||||
"GITHUB_APP_CLIENT_SECRET",
|
|
||||||
"GITHUB_APP_WEBHOOK_SECRET",
|
|
||||||
];
|
|
||||||
|
|
||||||
/// Abstraction over environment variable lookup.
|
/// Abstraction over environment variable lookup.
|
||||||
///
|
///
|
||||||
/// Production code uses [`SystemEnv`] which delegates to [`std::env::var`].
|
/// Production code uses [`SystemEnv`] which delegates to [`std::env::var`].
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue