mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +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-proc",
|
||||
"fabro-types",
|
||||
"fabro-util",
|
||||
"futures",
|
||||
"git2",
|
||||
"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.
|
||||
- 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).
|
||||
|
||||
## Tests
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
use anyhow::Result;
|
||||
use anyhow::{Result, anyhow};
|
||||
use fabro_util::terminal::Styles;
|
||||
|
||||
use crate::args::{AttachArgs, RunCommands, RunWorkerArgs, StartArgs};
|
||||
|
|
@ -23,7 +23,11 @@ pub(crate) mod ssh;
|
|||
pub(crate) mod start;
|
||||
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();
|
||||
|
||||
match cmd {
|
||||
|
|
@ -76,7 +80,22 @@ pub(crate) async fn dispatch(cmd: RunCommands, base_ctx: &CommandContext) -> Res
|
|||
run_dir,
|
||||
run_id,
|
||||
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::Logs(args) => {
|
||||
let styles = Styles::detect_stdout();
|
||||
|
|
|
|||
|
|
@ -59,19 +59,13 @@ pub(crate) async fn execute(
|
|||
storage_dir: Option<PathBuf>,
|
||||
run_dir: PathBuf,
|
||||
mode: RunWorkerMode,
|
||||
worker_token: &str,
|
||||
) -> Result<()> {
|
||||
let _ = fabro_proc::title_init();
|
||||
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 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_state = run_store
|
||||
.state()
|
||||
|
|
@ -84,7 +78,7 @@ pub(crate) async fn execute(
|
|||
let artifact_sink = Some(ArtifactSink::Uploader(build_artifact_uploader(
|
||||
run_id,
|
||||
client.clone_for_reuse(),
|
||||
worker_token,
|
||||
worker_token.to_owned(),
|
||||
)));
|
||||
let interviewer = Arc::new(ControlInterviewer::new());
|
||||
let cancel_token = Arc::new(AtomicBool::new(false));
|
||||
|
|
|
|||
|
|
@ -72,12 +72,33 @@ async fn main() {
|
|||
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();
|
||||
fabro_telemetry::init_cli();
|
||||
|
||||
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 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 cli = Cli::parse();
|
||||
|
|
@ -206,7 +227,7 @@ async fn main_inner() -> (String, Result<()>) {
|
|||
commands::exec::execute(args, &base_ctx).await?;
|
||||
}
|
||||
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::execute(args, &base_ctx).await?;
|
||||
|
|
|
|||
|
|
@ -184,6 +184,7 @@ fn assert_no_worker_env_leak(scope: &str, content: &str) {
|
|||
for needle in [
|
||||
"MY_API_TOKEN=",
|
||||
"NEW_RELIC_LICENSE_KEY=",
|
||||
"FABRO_WORKER_TOKEN=",
|
||||
LEAKED_WORKER_PARENT_TOKEN,
|
||||
LEAKED_NEW_RELIC_LICENSE,
|
||||
] {
|
||||
|
|
@ -522,7 +523,7 @@ methods = ["dev-token"]
|
|||
graph [goal="Verify worker subprocess env isolation", default_max_retries=0]
|
||||
start [shape=Mdiamond, label="Start"]
|
||||
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
|
||||
}
|
||||
"#,
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ use fabro_llm::generate::{GenerateParams, generate_object};
|
|||
use fabro_llm::types::{Message, Request, ToolResult};
|
||||
use fabro_template::{TemplateContext, render as render_template};
|
||||
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::time::timeout as tokio_timeout;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
|
|
@ -77,12 +77,6 @@ where
|
|||
pub struct 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.
|
||||
fn parse_decision(exit_code: i32, stdout: &str) -> HookDecision {
|
||||
if exit_code == 0 {
|
||||
|
|
@ -195,7 +189,6 @@ impl HookExecutorImpl {
|
|||
if let Some(wd) = work_dir {
|
||||
cmd.current_dir(wd);
|
||||
}
|
||||
Self::scrub_worker_secret_env(&mut cmd);
|
||||
for (k, v) in &env_vars {
|
||||
cmd.env(k, v);
|
||||
}
|
||||
|
|
@ -847,23 +840,6 @@ mod tests {
|
|||
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]
|
||||
async fn no_hook_type_blocks() {
|
||||
let executor = HookExecutorImpl;
|
||||
|
|
|
|||
|
|
@ -30,7 +30,6 @@ strum.workspace = true
|
|||
tracing.workspace = true
|
||||
base64.workspace = true
|
||||
fabro-proc = { path = "../fabro-proc" }
|
||||
fabro-util = { path = "../fabro-util" }
|
||||
shlex = "1"
|
||||
|
||||
# local
|
||||
|
|
|
|||
|
|
@ -2,7 +2,6 @@ use std::path::{Path, PathBuf};
|
|||
use std::time::Instant;
|
||||
|
||||
use async_trait::async_trait;
|
||||
use fabro_util::env::WORKER_SECRET_ENV_DENYLIST;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio::process::{Child, Command};
|
||||
use tokio::task::spawn_blocking;
|
||||
|
|
@ -58,9 +57,6 @@ impl LocalSandbox {
|
|||
if Self::ENV_SAFELIST.contains(&key) {
|
||||
return false;
|
||||
}
|
||||
if WORKER_SECRET_ENV_DENYLIST.contains(&key) {
|
||||
return true;
|
||||
}
|
||||
let lower = key.to_lowercase();
|
||||
lower.ends_with("_api_key")
|
||||
|| lower.ends_with("_secret")
|
||||
|
|
@ -740,9 +736,6 @@ mod tests {
|
|||
assert!(LocalSandbox::should_filter_env_var("MY_CREDENTIAL"));
|
||||
assert!(LocalSandbox::should_filter_env_var("FABRO_WORKER_TOKEN"));
|
||||
assert!(LocalSandbox::should_filter_env_var("SESSION_SECRET"));
|
||||
assert!(LocalSandbox::should_filter_env_var(
|
||||
"GITHUB_APP_PRIVATE_KEY"
|
||||
));
|
||||
// Case insensitive
|
||||
assert!(LocalSandbox::should_filter_env_var("my_api_key"));
|
||||
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.
|
||||
///
|
||||
/// Production code uses [`SystemEnv`] which delegates to [`std::env::var`].
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue