From e6e091fe8e9eb9bf0878df3e6e160e594bfe42b8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 07:15:27 -0400 Subject: [PATCH] refactor(server): lock down server secrets --- AGENTS.md | 1 + clippy.toml | 2 + docs-internal/server-secrets-strategy.md | 68 ++++++++ docs/administration/security.mdx | 2 +- docs/administration/server-configuration.mdx | 3 - .../specs/2026-04-18-web-install-design.md | 2 +- lib/crates/fabro-cli/src/commands/install.rs | 158 ++++++------------ .../fabro-cli/src/commands/server/start.rs | 55 ++---- lib/crates/fabro-cli/src/local_server.rs | 9 +- lib/crates/fabro-cli/src/server_client.rs | 25 ++- lib/crates/fabro-cli/src/user_config.rs | 14 +- lib/crates/fabro-cli/tests/it/cmd/ps.rs | 9 +- .../fabro-cli/tests/it/cmd/server_start.rs | 9 +- .../tests/it/scenario/server_lifecycle.rs | 14 +- .../tests/it/support/auth_harness.rs | 26 ++- lib/crates/fabro-config/src/envfile.rs | 6 +- lib/crates/fabro-install/src/lib.rs | 50 ------ lib/crates/fabro-server/src/diagnostics.rs | 24 --- lib/crates/fabro-server/src/install.rs | 31 +--- lib/crates/fabro-server/src/lib.rs | 3 + lib/crates/fabro-server/src/serve.rs | 32 ++-- lib/crates/fabro-server/src/server.rs | 80 ++++++--- lib/crates/fabro-server/src/server_secrets.rs | 88 ++++++++-- lib/crates/fabro-server/src/spawn_env.rs | 137 +++++++++++++++ lib/crates/fabro-server/src/startup.rs | 118 +++++++++++++ lib/crates/fabro-server/tests/it/api/docs.rs | 8 +- .../fabro-server/tests/it/api/install.rs | 2 - .../tests/it/openapi_conformance.rs | 11 +- lib/crates/fabro-telemetry/src/spawn.rs | 2 +- lib/crates/fabro-test/src/lib.rs | 7 +- test/twin/openai/src/config.rs | 13 +- test/twin/openai/tests/config_contract.rs | 28 +--- 32 files changed, 653 insertions(+), 384 deletions(-) create mode 100644 docs-internal/server-secrets-strategy.md create mode 100644 lib/crates/fabro-server/src/spawn_env.rs create mode 100644 lib/crates/fabro-server/src/startup.rs diff --git a/AGENTS.md b/AGENTS.md index 2df3d2b6e..adc9b5d79 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,7 @@ When working on Rust crates, read the relevant strategy doc **before** making ch - **`docs-internal/logging-strategy.md`** — read when adding `tracing` calls (`info!`, `debug!`, `warn!`, `error!`), working on error handling paths, or adding new operations that should be observable - **`docs-internal/events-strategy.md`** — read when adding or modifying `Event` variants, touching `Emitter`/`emit()`, changing `progress.jsonl` output, or adding new workflow stage types - **`files-internal/testing-strategy.md`** — read when adding or reorganizing tests, choosing between unit vs `tests/it`, deciding whether a test belongs in `cmd` vs `workflow` vs `scenario`, or deciding how to structure snapshots and fixtures +- **`docs-internal/server-secrets-strategy.md`** — read when adding or changing server-level secrets, startup validation, install-time secret persistence, or subprocess env inheritance/scrubbing ## Shell quoting in sandbox code diff --git a/clippy.toml b/clippy.toml index 0a19186f3..033174d94 100644 --- a/clippy.toml +++ b/clippy.toml @@ -20,6 +20,8 @@ disallowed-methods = [ { path = "std::fs::File::create", reason = "Blocking open; prefer tokio::fs::File::create on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, { path = "std::fs::File::create_new", reason = "Blocking open; prefer tokio::fs::File::create_new on Tokio paths. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, { path = "std::fs::OpenOptions::open", reason = "Blocking open; prefer tokio::fs::OpenOptions::open on Tokio paths. OS file-lock semantics may require spawn_blocking instead. Document intentional sync I/O with #[expect(clippy::disallowed_methods, reason = \"...\")]" }, + { path = "std::env::set_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, + { path = "std::env::remove_var", reason = "Server/process env must be injected at construction or child-process spawn time, not mutated globally. See docs-internal/server-secrets-strategy.md" }, { path = "reqwest::Client::new", reason = "Use fabro_http::http_client() or fabro_http::test_http_client()", allow-invalid = true }, { path = "reqwest::Client::builder", reason = "Use fabro_http::HttpClientBuilder::new()", allow-invalid = true }, { path = "reqwest::blocking::Client::new", reason = "Use fabro_http::blocking_http_client() or fabro_http::blocking_test_http_client()", allow-invalid = true }, diff --git a/docs-internal/server-secrets-strategy.md b/docs-internal/server-secrets-strategy.md new file mode 100644 index 000000000..dd0df73f5 --- /dev/null +++ b/docs-internal/server-secrets-strategy.md @@ -0,0 +1,68 @@ +# Server Secrets Strategy + +This document defines how Fabro handles server-level secrets. + +## Core Rules + +- `ServerSecrets` is the canonical server-secret reader. +- It reads from `process env` and `/server.env`. +- Resolution is snapshot-based: env and file are read once at construction, then treated as immutable for the life of the process. +- `process env` wins over `server.env` on conflicts. +- `fabro server start` never generates secrets. Missing required secrets are a startup error. +- `std::env::set_var` and `std::env::remove_var` are banned workspace-wide. Tests are not exempt. + +## Active Server Secrets + +These values belong to the server runtime and are read via `state.server_secret(...)`: + +| Secret | Used by | +|---|---| +| `SESSION_SECRET` | Cookie encryption and JWT signing derivation | +| `FABRO_DEV_TOKEN` | Dev-token auth for worker/server interactions | +| `GITHUB_APP_PRIVATE_KEY` | GitHub App credentials | +| `GITHUB_APP_WEBHOOK_SECRET` | GitHub webhook verification | +| `GITHUB_APP_CLIENT_SECRET` | GitHub OAuth login | + +`FABRO_JWT_PRIVATE_KEY` and `FABRO_JWT_PUBLIC_KEY` are removed. `SESSION_SECRET` is the single auth root. + +## Startup + +- Foreground and daemon startup use the same validation path. +- Required-at-startup secrets are: + - `SESSION_SECRET` + - `FABRO_DEV_TOKEN` when dev-token auth is enabled + - `GITHUB_APP_CLIENT_SECRET` when GitHub auth is enabled +- Other server secrets remain lazy/feature-specific rather than universal boot blockers. + +## Provisioning + +Server secrets come from one of two sources: + +- Platform env for 12-factor deployments +- `server.env` written by install flows + +There is no compatibility layer for removed secrets and no startup-time secret generation. + +## Subprocess Boundaries + +- Worker and render-graph subprocesses start from `env_clear()` and re-add only explicit allowlisted variables. +- Authority-bearing values are re-injected intentionally. +- The daemon child inherits the parent env unchanged except for output-format hygiene (`FABRO_JSON` removal). + +## Tests + +- In-process tests must inject server secrets with construction-time stubs (`EnvSource`, `StubEnv`) or by writing `server.env`. +- Subprocess tests must set child env with `Command::env`. +- Tests must not mutate the process-wide environment. + +## Rotation + +- Secret rotation requires restart. +- Live rotation is intentionally unsupported. + +## Adding A New Server Secret + +1. Provision it through platform env or install-written `server.env`. +2. Read it through `state.server_secret(...)`. +3. Decide explicitly whether startup should fail when it is absent. +4. If a worker or render subprocess needs it, re-inject it explicitly rather than broadening inheritance casually. diff --git a/docs/administration/security.mdx b/docs/administration/security.mdx index 2a3feec24..437c2b5ac 100644 --- a/docs/administration/security.mdx +++ b/docs/administration/security.mdx @@ -30,7 +30,7 @@ Fabro is single-tenant software designed for small, trusted teams. The following - **Enable authentication.** Fabro supports `dev-token` and GitHub OAuth. Do not disable auth outside of local development or controlled demos. - **Configure a username allowlist for GitHub OAuth.** `[server.auth.github].allowed_usernames` should contain the exact GitHub users allowed to log in. An empty list rejects everyone. -- **Configure the session secret used by the web flow.** `SESSION_SECRET` should be provisioned with a strong value on long-lived deployments. If you also provision `FABRO_JWT_PRIVATE_KEY` and `FABRO_JWT_PUBLIC_KEY`, treat them as server runtime secrets, but they are not what currently gates browser auth. +- **Configure the session secret used by the web flow.** `SESSION_SECRET` should be provisioned with a strong value on long-lived deployments. - **Terminate HTTPS or mTLS upstream when needed.** Fabro's listener is plain HTTP/Unix only. If CI, scripts, or a browser must connect over HTTPS, terminate TLS at a reverse proxy or load balancer and keep the Fabro listener on a private network. ### Secrets diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index dce6b3ae7..1d96421d8 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -299,7 +299,6 @@ For the auth model above, the main server runtime secrets are: - `SESSION_SECRET` when the web UI is enabled - `FABRO_DEV_TOKEN` when `"dev-token"` auth is enabled - `GITHUB_APP_CLIENT_SECRET` when `"github"` auth is enabled -- `FABRO_JWT_PRIVATE_KEY` / `FABRO_JWT_PUBLIC_KEY`, provisioned during install for future CLI login flows - `AWS_ACCESS_KEY_ID` / `AWS_SECRET_ACCESS_KEY` when the install wizard or a manual config uses static S3 object-store credentials @@ -332,8 +331,6 @@ Fabro resolves these from `process env -> server.env`. | Variable | Description | |---|---| -| `FABRO_JWT_PRIVATE_KEY` | Ed25519 private key (base64-encoded PEM) for JWT signing | -| `FABRO_JWT_PUBLIC_KEY` | Ed25519 public key (base64-encoded PEM) for JWT verification | | `SESSION_SECRET` | Session encryption secret (64-character hex string) | ### Object store runtime secrets (optional) diff --git a/docs/superpowers/specs/2026-04-18-web-install-design.md b/docs/superpowers/specs/2026-04-18-web-install-design.md index ce5f96ae0..05b4b7466 100644 --- a/docs/superpowers/specs/2026-04-18-web-install-design.md +++ b/docs/superpowers/specs/2026-04-18-web-install-design.md @@ -260,7 +260,7 @@ The web wizard produces **the same on-disk state** as the CLI install. The TOML- Files written: - `~/.fabro/settings.toml` — server config, auth methods, GitHub integration strategy. -- `/server.env` — `FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, `SESSION_SECRET`, `FABRO_DEV_TOKEN`, plus GitHub App env pairs (`GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`) if the App strategy was chosen. +- `/server.env` — `SESSION_SECRET`, `FABRO_DEV_TOKEN`, plus GitHub App env pairs (`GITHUB_APP_PRIVATE_KEY`, `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`) if the App strategy was chosen. - `/vaults/default/secrets.json` — vault entries for LLM API key credentials and (if Token strategy) `GITHUB_TOKEN`. Path matches `Storage::secrets_path()` at `lib/crates/fabro-config/src/storage.rs:38`. - `/server.dev-token` — the per-storage dev token, written via `Storage::server_state().dev_token_path()` at `storage.rs:103`. The CLI install also writes a home-level mirror at `Home::from_env().dev_token_path()` (`install.rs:1994-1999`); the web flow does the same to keep parity, since the home-level file is what tooling outside the storage dir expects to find. - Artifact store metadata stamped with `FABRO_VERSION` via `write_artifact_store_metadata` (`install.rs:1458`). diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index d7f2742ed..3197f4c79 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -26,8 +26,8 @@ use fabro_config::daemon::ServerDaemon; use fabro_config::user::{SETTINGS_CONFIG_FILENAME, default_storage_dir}; use fabro_config::{ResolveError, Storage, envfile}; use fabro_install::{ - InstallListenConfig, generate_jwt_keypair, merge_server_settings as merge_server_settings_impl, - write_github_app_settings, write_token_settings, + InstallListenConfig, PendingSettingsWrite, merge_server_settings as merge_server_settings_impl, + persist_install_outputs_direct, write_github_app_settings, write_token_settings, }; use fabro_model::Provider; use fabro_server::serve; @@ -883,13 +883,6 @@ enum PendingGitHubSettings { }, } -#[derive(Clone, Copy)] -struct PendingSettingsWrite<'a> { - path: &'a Path, - contents: &'a str, - previous_contents: Option<&'a str>, -} - async fn setup_github_app( s: &Styles, web_url: &str, @@ -1148,15 +1141,24 @@ fn credential_secret_request(credential: &AuthCredential) -> Result Result<()> { - if secrets.is_empty() { - return Ok(()); - } +fn server_env_updates(secrets: &[(String, String)]) -> Vec { + secrets + .iter() + .map(|(key, value)| envfile::EnvFileUpdate { + key: key.clone(), + value: value.clone(), + comment: None, + }) + .collect() +} - let env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&env_path, secrets.iter().cloned()) - .with_context(|| format!("merging server env secrets into {}", env_path.display()))?; - Ok(()) +fn server_env_removals(keys: &[&'static str]) -> Vec { + keys.iter() + .map(|key| envfile::EnvFileRemoval { + key: (*key).to_string(), + comment: None, + }) + .collect() } async fn persist_install_outputs( @@ -1221,20 +1223,15 @@ fn persist_github_install_changes( let previous_vault = std::fs::read_to_string(&vault_path).ok(); let result = (|| -> Result<()> { - let mut server_env = envfile::read_env_file(&server_env_path) - .with_context(|| format!("reading env file {}", server_env_path.display()))?; - for key in &writes.server_env_remove { - server_env.remove(*key); - } - for (key, value) in &writes.server_env_set { - server_env.insert(key.clone(), value.clone()); - } - if server_env.is_empty() { - restore_optional_file(&server_env_path, None)?; - } else { - envfile::write_env_file(&server_env_path, &server_env) - .with_context(|| format!("writing env file {}", server_env_path.display()))?; - } + let server_env_writes = server_env_updates(&writes.server_env_set); + let server_env_removals = server_env_removals(&writes.server_env_remove); + persist_install_outputs_direct( + storage_dir, + &server_env_writes, + &server_env_removals, + &[], + Some(&writes.settings_write), + )?; let mut vault = Vault::load(vault_path.clone()).map_err(anyhow::Error::from)?; for key in &writes.vault_remove { @@ -1249,14 +1246,6 @@ fn persist_github_install_changes( .map_err(anyhow::Error::from)?; } - std::fs::write(writes.settings_write.path, writes.settings_write.contents).with_context( - || { - format!( - "writing settings file {}", - writes.settings_write.path.display() - ) - }, - )?; Ok(()) })(); @@ -1305,12 +1294,16 @@ async fn persist_install_outputs_with_settings( connect_server: impl for<'a> Fn(&'a Path) -> BoxFuture<'a, Result>, stop_server: impl for<'a> Fn(&'a Path, Duration) -> BoxFuture<'a, bool>, ) -> Result<()> { - persist_server_env_secrets(storage_dir, server_env_secrets)?; - - if let Some(write) = settings_write { - std::fs::write(write.path, write.contents) - .with_context(|| format!("writing settings file {}", write.path.display()))?; - } + let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); + let previous_server_env = std::fs::read_to_string(&server_env_path).ok(); + let settings_write_ref = settings_write.as_ref(); + persist_install_outputs_direct( + storage_dir, + &server_env_updates(server_env_secrets), + &[], + &[], + settings_write_ref, + )?; let persist_result = persist_vault_secrets_with( storage_dir, @@ -1322,6 +1315,7 @@ async fn persist_install_outputs_with_settings( .await; if let Err(err) = persist_result { + restore_optional_file(&server_env_path, previous_server_env.as_deref())?; if let Some(write) = settings_write { match write.previous_contents { Some(previous) => std::fs::write(write.path, previous) @@ -1820,13 +1814,6 @@ async fn run_install_inner( s.green.apply_to("✔") ); - let (jwt_private_pem, jwt_public_pem) = generate_jwt_keypair()?; - fabro_util::printerr!( - printer, - " {} Ed25519 JWT keypair generated", - s.green.apply_to("✔") - ); - let dev_token = if fabro_config::dev_token_auth_enabled(&install_settings) { let token = dev_token::read_or_mint_dev_token_for_install( &fabro_util::Home::from_env().dev_token_path(), @@ -1847,14 +1834,7 @@ async fn run_install_inner( None }; - let jwt_private_b64 = BASE64_STANDARD.encode(jwt_private_pem.as_bytes()); - let jwt_public_b64 = BASE64_STANDARD.encode(jwt_public_pem.as_bytes()); - - let mut generated_server_env_pairs = vec![ - ("FABRO_JWT_PRIVATE_KEY".to_string(), jwt_private_b64), - ("FABRO_JWT_PUBLIC_KEY".to_string(), jwt_public_b64), - ("SESSION_SECRET".to_string(), session_secret), - ]; + let mut generated_server_env_pairs = vec![("SESSION_SECRET".to_string(), session_secret)]; if let Some(token) = dev_token { generated_server_env_pairs.push(("FABRO_DEV_TOKEN".to_string(), token)); } @@ -2039,39 +2019,6 @@ mod tests { assert!(secret.chars().all(|c| !c.is_ascii_uppercase())); } - // -- JWT keypair -- - - #[tokio::test] - async fn jwt_keypair_private_pem_header() { - let (private, _) = generate_jwt_keypair().unwrap(); - assert!( - private.starts_with("-----BEGIN PRIVATE KEY-----"), - "private PEM: {private}" - ); - } - - #[tokio::test] - async fn jwt_keypair_public_pem_header() { - let (_, public) = generate_jwt_keypair().unwrap(); - assert!( - public.starts_with("-----BEGIN PUBLIC KEY-----"), - "public PEM: {public}" - ); - } - - #[tokio::test] - async fn jwt_keypair_public_parses() { - let (_, public) = generate_jwt_keypair().unwrap(); - jsonwebtoken::DecodingKey::from_ed_pem(public.as_bytes()).expect("public key should parse"); - } - - #[tokio::test] - async fn jwt_keypair_private_parses() { - let (private, _) = generate_jwt_keypair().unwrap(); - jsonwebtoken::EncodingKey::from_ed_pem(private.as_bytes()) - .expect("private key should parse"); - } - // -- Config TOML generation -- #[test] @@ -2524,11 +2471,8 @@ client_id = "client-id" #[tokio::test] async fn persist_install_outputs_persists_vault_secrets_via_server_when_autostarting() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![ - ("SESSION_SECRET".to_string(), "session".to_string()), - ("FABRO_JWT_PUBLIC_KEY".to_string(), "public-key".to_string()), - ]; - let vault_secrets = vec![ + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [ CreateSecretRequest { name: "GITHUB_TOKEN".to_string(), value: "gh-token".to_string(), @@ -2562,7 +2506,8 @@ client_id = "client-id" .await; let stop_called = Arc::new(AtomicBool::new(false)); - persist_server_env_secrets(dir.path(), &server_env_pairs).unwrap(); + let env_path = Storage::new(dir.path()).runtime_directory().env_path(); + envfile::merge_env_file(&env_path, server_env_pairs.iter().cloned()).unwrap(); persist_vault_secrets_with( dir.path(), &vault_secrets, @@ -2589,7 +2534,6 @@ client_id = "client-id" std::fs::read_to_string(Storage::new(dir.path()).runtime_directory().env_path()) .unwrap(); assert!(server_env.contains("SESSION_SECRET=session")); - assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=public-key")); assert_eq!(created.calls_async().await, 2); assert!(stop_called.load(Ordering::SeqCst)); assert!(!Storage::new(dir.path()).secrets_path().exists()); @@ -2598,7 +2542,7 @@ client_id = "client-id" #[tokio::test] async fn persist_vault_secrets_with_leaves_running_server_up() { let dir = tempfile::tempdir().unwrap(); - let vault_secrets = vec![CreateSecretRequest { + let vault_secrets = [CreateSecretRequest { name: "GITHUB_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, @@ -2806,10 +2750,10 @@ client_id = "client-id" } #[tokio::test] - async fn persist_install_outputs_with_settings_does_not_write_settings_on_secret_failure() { + async fn persist_install_outputs_with_settings_rolls_back_new_files_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![("SESSION_SECRET".to_string(), "session".to_string())]; - let vault_secrets = vec![CreateSecretRequest { + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [CreateSecretRequest { name: "GITHUB_CLI_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, @@ -2844,7 +2788,7 @@ client_id = "client-id" assert!(result.is_err()); assert!( - Storage::new(dir.path()) + !Storage::new(dir.path()) .runtime_directory() .env_path() .exists() @@ -2856,8 +2800,8 @@ client_id = "client-id" #[tokio::test] async fn persist_install_outputs_with_settings_restores_previous_contents_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); - let server_env_pairs = vec![("SESSION_SECRET".to_string(), "session".to_string())]; - let vault_secrets = vec![CreateSecretRequest { + let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; + let vault_secrets = [CreateSecretRequest { name: "GITHUB_CLI_TOKEN".to_string(), value: "gh-token".to_string(), type_: ApiSecretType::Environment, diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 9e87dcc50..f2b47ab2b 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -7,15 +7,15 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::{Context, Result, anyhow, bail}; +use fabro_config::RuntimeDirectory; use fabro_config::bind::{Bind, BindRequest}; use fabro_config::daemon::ServerDaemon; use fabro_config::user::{FABRO_CONFIG_ENV, default_settings_path, load_settings_config}; -use fabro_config::{RuntimeDirectory, envfile}; use fabro_server::jwt_auth::auth_method_name; -use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs}; +use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs, resolve_runtime_server_settings_for_start}; +use fabro_server::{ProcessEnv, validate_startup}; use fabro_types::settings::ServerAuthMethod; use fabro_util::printer::Printer; -use fabro_util::session_secret; use fabro_util::terminal::Styles; use tokio::net::{TcpStream, UnixStream}; use tokio::process::Command as TokioCommand; @@ -222,33 +222,6 @@ fn configured_auth_methods(config_path: Option<&Path>) -> Vec .unwrap_or_default() } -fn valid_session_secret(secret: &str) -> bool { - session_secret::validate_session_secret(secret).is_ok() -} - -fn load_or_create_local_session_secret(runtime_directory: &RuntimeDirectory) -> Result { - if let Some(secret) = std::env::var("SESSION_SECRET") - .ok() - .filter(|secret| valid_session_secret(secret)) - { - return Ok(secret); - } - - let server_env_path = runtime_directory.env_path(); - if let Some(secret) = envfile::read_env_file(&server_env_path) - .ok() - .and_then(|entries| entries.get("SESSION_SECRET").cloned()) - .filter(|secret| valid_session_secret(secret)) - { - return Ok(secret); - } - - let secret = session_secret::generate_session_secret(); - envfile::merge_env_file(&server_env_path, [("SESSION_SECRET", secret.as_str())]) - .with_context(|| format!("merging session secret into {}", server_env_path.display()))?; - Ok(secret) -} - // --------------------------------------------------------------------------- // Foreground mode // --------------------------------------------------------------------------- @@ -261,18 +234,6 @@ async fn execute_foreground( styles: &'static Styles, _printer: Printer, ) -> Result<()> { - let session_secret = load_or_create_local_session_secret(&RuntimeDirectory::new(&storage_dir))?; - let prior_session_secret = std::env::var_os("SESSION_SECRET"); - std::env::set_var("SESSION_SECRET", &session_secret); - let _env_guard = - scopeguard::guard( - prior_session_secret, - |prior_session_secret| match prior_session_secret { - Some(value) => std::env::set_var("SESSION_SECRET", value), - None => std::env::remove_var("SESSION_SECRET"), - }, - ); - super::foreground::serve_with_daemon_record(serve_args, bind, storage_dir, styles).await } @@ -303,6 +264,14 @@ async fn execute_daemon( return Ok(()); } + let resolved_settings = resolve_runtime_server_settings_for_start(serve_args, storage_dir)?; + validate_startup( + runtime_directory.env_path().as_path(), + &ProcessEnv, + &resolved_settings, + ) + .map_err(anyhow::Error::from)?; + let log_path = runtime_directory.log_path(); if let Some(parent) = log_path.parent() { std::fs::create_dir_all(parent) @@ -347,9 +316,7 @@ async fn execute_daemon( cmd.arg("--watch-web"); } - let session_secret = load_or_create_local_session_secret(&runtime_directory)?; cmd.arg("--storage-dir").arg(storage_dir); - cmd.env("SESSION_SECRET", &session_secret); cmd.env_remove("FABRO_JSON"); cmd.stdout(stdout_log) diff --git a/lib/crates/fabro-cli/src/local_server.rs b/lib/crates/fabro-cli/src/local_server.rs index 6ba1a01aa..687360db2 100644 --- a/lib/crates/fabro-cli/src/local_server.rs +++ b/lib/crates/fabro-cli/src/local_server.rs @@ -12,9 +12,16 @@ use fabro_server::serve::resolve_bind_request_from_settings; use fabro_types::settings::{ServerAuthMethod, SettingsLayer}; pub(crate) fn storage_dir(settings: &SettingsLayer) -> Result { + storage_dir_with_lookup(settings, &|name| std::env::var(name).ok()) +} + +pub(crate) fn storage_dir_with_lookup( + settings: &SettingsLayer, + lookup: &dyn Fn(&str) -> Option, +) -> Result { let storage_root = fabro_config::resolve_storage_root(settings); let resolved_root = storage_root - .resolve(|name| std::env::var(name).ok()) + .resolve(lookup) .map_err(|err| anyhow::anyhow!("failed to resolve {}: {err}", storage_root.as_source()))?; Ok(PathBuf::from(resolved_root.value)) } diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 60ef4c21e..fc82ede62 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -500,21 +500,20 @@ mod tests { #[test] fn resolve_local_tcp_credential_does_not_fallback_to_home_dev_token() { let temp_home = tempfile::tempdir().unwrap(); - std::fs::write( - temp_home.path().join("dev-token"), - "fabro_dev_abababababababababababababababababababababababababababababababab", - ) - .unwrap(); - let original_home = std::env::var_os("FABRO_HOME"); - std::env::set_var("FABRO_HOME", temp_home.path()); - let _guard = scopeguard::guard(original_home, |original_home| match original_home { - Some(value) => std::env::set_var("FABRO_HOME", value), - None => std::env::remove_var("FABRO_HOME"), - }); - + let token = "fabro_dev_abababababababababababababababababababababababababababababababab"; + std::fs::write(temp_home.path().join("dev-token"), token).unwrap(); let target = ServerTarget::http_url("http://127.0.0.1:32276").unwrap(); + let store = AuthStore::new(temp_home.path().join("auth.json")); + assert_eq!( + load_cli_dev_token_from_sources(None, &Home::new(temp_home.path())).as_deref(), + Some(token) + ); - assert!(resolve_local_tcp_credential(&target).unwrap().is_none()); + assert!( + resolve_local_tcp_credential_with_store(&target, None, &store, Utc::now()) + .unwrap() + .is_none() + ); } #[test] diff --git a/lib/crates/fabro-cli/src/user_config.rs b/lib/crates/fabro-cli/src/user_config.rs index 76fbc9507..0dcf265e7 100644 --- a/lib/crates/fabro-cli/src/user_config.rs +++ b/lib/crates/fabro-cli/src/user_config.rs @@ -281,13 +281,13 @@ root = "{{ env.FABRO_STORAGE_ROOT }}" "#, ); let temp = tempfile::tempdir().unwrap(); - let original = std::env::var_os("FABRO_STORAGE_ROOT"); - std::env::set_var("FABRO_STORAGE_ROOT", temp.path()); - let _guard = scopeguard::guard(original, |original| match original { - Some(value) => std::env::set_var("FABRO_STORAGE_ROOT", value), - None => std::env::remove_var("FABRO_STORAGE_ROOT"), - }); - assert_eq!(storage_dir(&settings).unwrap(), temp.path()); + assert_eq!( + local_server::storage_dir_with_lookup(&settings, &|name| { + (name == "FABRO_STORAGE_ROOT").then(|| temp.path().display().to_string()) + }) + .unwrap(), + temp.path() + ); } } diff --git a/lib/crates/fabro-cli/tests/it/cmd/ps.rs b/lib/crates/fabro-cli/tests/it/cmd/ps.rs index a6d021a3b..4642010d1 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/ps.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/ps.rs @@ -9,12 +9,17 @@ use crate::support::{fatal_error_line, unique_run_id}; const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; +const TEST_SESSION_SECRET: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; fn provision_local_server_auth(context: &fabro_test::TestContext, storage_dir: &std::path::Path) { context.ensure_home_server_auth_methods(); let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .expect("merging FABRO_DEV_TOKEN into server.env"); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .expect("merging server auth into server.env"); dev_token::write_dev_token( &context.home_dir.join(".fabro").join("dev-token"), TEST_DEV_TOKEN, diff --git a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs index 13996832b..7c8df950b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/server_start.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/server_start.rs @@ -21,6 +21,8 @@ use fabro_util::dev_token; const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; +const TEST_SESSION_SECRET: &str = + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; fn write_dev_token_server_settings(config_path: &std::path::Path, rest: &str) { std::fs::write( @@ -32,8 +34,11 @@ fn write_dev_token_server_settings(config_path: &std::path::Path, rest: &str) { fn provision_dev_token_auth(home_dir: &std::path::Path, storage_dir: &std::path::Path) { let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .expect("merging FABRO_DEV_TOKEN into server.env"); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .expect("merging server auth into server.env"); dev_token::write_dev_token(&home_dir.join(".fabro").join("dev-token"), TEST_DEV_TOKEN) .expect("writing home dev-token"); } diff --git a/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs b/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs index 5abc94115..5906cc6da 100644 --- a/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs +++ b/lib/crates/fabro-cli/tests/it/scenario/server_lifecycle.rs @@ -13,10 +13,16 @@ fn start_status_stop_lifecycle() { let server_env_path = fabro_config::Storage::new(&storage_dir) .runtime_directory() .env_path(); - fabro_config::envfile::merge_env_file(&server_env_path, [( - "FABRO_DEV_TOKEN", - "fabro_dev_abababababababababababababababababababababababababababababababab", - )]) + fabro_config::envfile::merge_env_file(&server_env_path, [ + ( + "FABRO_DEV_TOKEN", + "fabro_dev_abababababababababababababababababababababababababababababababab", + ), + ( + "SESSION_SECRET", + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + ), + ]) .unwrap(); fabro_util::dev_token::write_dev_token( &context.home_dir.join(".fabro").join("dev-token"), diff --git a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs index a8a81b40b..68c4bd83e 100644 --- a/lib/crates/fabro-cli/tests/it/support/auth_harness.rs +++ b/lib/crates/fabro-cli/tests/it/support/auth_harness.rs @@ -22,7 +22,8 @@ use fabro_server::auth::GithubEndpoints; use fabro_server::ip_allowlist::IpAllowlistConfig; use fabro_server::jwt_auth::resolve_auth_mode_with_lookup; use fabro_server::server::{ - RouterOptions, build_router_with_options, create_app_state_with_env_lookup, + RouterOptions, build_router_with_options, + create_app_state_with_env_lookup_and_server_secret_env, }; use fabro_test::{GitHubAppState, TestContext, apply_test_isolation}; use fabro_types::RunAuthMethod; @@ -79,12 +80,23 @@ impl RealAuthHarness { _ => None, }) .expect("auth mode should resolve"); - let state = create_app_state_with_env_lookup(settings, 5, move |name| match name { - "SESSION_SECRET" => Some(TEST_SESSION_SECRET.to_string()), - "GITHUB_APP_CLIENT_SECRET" => Some(github_client_secret.clone()), - "FABRO_DEV_TOKEN" => dev_token.clone(), - _ => None, - }); + let state = + create_app_state_with_env_lookup_and_server_secret_env(settings, 5, |_| None, { + let mut secrets = std::collections::HashMap::from([ + ( + "SESSION_SECRET".to_string(), + TEST_SESSION_SECRET.to_string(), + ), + ( + "GITHUB_APP_CLIENT_SECRET".to_string(), + github_client_secret.clone(), + ), + ]); + if let Some(token) = dev_token.clone() { + secrets.insert("FABRO_DEV_TOKEN".to_string(), token); + } + secrets + }); let github_base = github_base_url(&twin.base_url); let router = build_router_with_options( state, diff --git a/lib/crates/fabro-config/src/envfile.rs b/lib/crates/fabro-config/src/envfile.rs index 403cf5595..576877827 100644 --- a/lib/crates/fabro-config/src/envfile.rs +++ b/lib/crates/fabro-config/src/envfile.rs @@ -358,7 +358,7 @@ mod tests { let entries = merge_env_file(&path, [ ("SESSION_SECRET", "secret"), - ("FABRO_JWT_PUBLIC_KEY", "jwt"), + ("FABRO_DEV_TOKEN", "token"), ]) .unwrap(); @@ -368,8 +368,8 @@ mod tests { Some("secret") ); assert_eq!( - entries.get("FABRO_JWT_PUBLIC_KEY").map(String::as_str), - Some("jwt") + entries.get("FABRO_DEV_TOKEN").map(String::as_str), + Some("token") ); } diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index 7cf5c222f..29d03911a 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -6,17 +6,8 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; -use base64::Engine as _; -use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use fabro_config::{Storage, envfile}; use fabro_vault::{SecretType as VaultSecretType, Vault}; -use ring::rand::SystemRandom; -use ring::signature::{Ed25519KeyPair, KeyPair as _}; - -const ED25519_SPKI_PREFIX: [u8; 12] = [ - 0x30, 0x2A, 0x30, 0x05, 0x06, 0x03, 0x2B, 0x65, 0x70, 0x03, 0x21, 0x00, -]; -const ED25519_PUBLIC_KEY_LEN: usize = 32; pub struct PendingSettingsWrite<'a> { pub path: &'a Path, @@ -95,47 +86,6 @@ impl std::error::Error for PersistInstallOutputsError { } } -fn pem_encode(label: &str, bytes: &[u8]) -> String { - let body = BASE64_STANDARD.encode(bytes); - let mut pem = String::new(); - pem.push_str("-----BEGIN "); - pem.push_str(label); - pem.push_str("-----\n"); - for chunk in body.as_bytes().chunks(64) { - pem.push_str(std::str::from_utf8(chunk).expect("base64 output should be valid UTF-8")); - pem.push('\n'); - } - pem.push_str("-----END "); - pem.push_str(label); - pem.push_str("-----\n"); - pem -} - -fn ed25519_public_key_spki(public_key: &[u8]) -> Result> { - anyhow::ensure!( - public_key.len() == ED25519_PUBLIC_KEY_LEN, - "generated Ed25519 public key had unexpected length" - ); - - let mut spki = Vec::with_capacity(ED25519_SPKI_PREFIX.len() + public_key.len()); - spki.extend_from_slice(&ED25519_SPKI_PREFIX); - spki.extend_from_slice(public_key); - Ok(spki) -} - -pub fn generate_jwt_keypair() -> Result<(String, String)> { - let pkcs8 = Ed25519KeyPair::generate_pkcs8(&SystemRandom::new()) - .map_err(|_| anyhow::anyhow!("failed to generate Ed25519 keypair"))?; - let keypair = Ed25519KeyPair::from_pkcs8(pkcs8.as_ref()) - .map_err(|_| anyhow::anyhow!("failed to parse generated Ed25519 keypair"))?; - let public_der = ed25519_public_key_spki(keypair.public_key().as_ref())?; - - Ok(( - pem_encode("PRIVATE KEY", pkcs8.as_ref()), - pem_encode("PUBLIC KEY", &public_der), - )) -} - pub fn default_web_url() -> String { "http://127.0.0.1:32276".to_string() } diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 582816a76..4b1d6e82d 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -537,30 +537,6 @@ fn check_crypto(state: &AppState) -> CheckResult { } } - if let Some(raw) = state.server_secret("FABRO_JWT_PUBLIC_KEY") { - if let Err(err) = decode_pem_value("FABRO_JWT_PUBLIC_KEY", &raw).and_then(|pem| { - jsonwebtoken::DecodingKey::from_ed_pem(pem.as_bytes()) - .map(|_| ()) - .map_err(|e| format!("invalid JWT public key: {e}")) - }) { - errors.push(err); - } else { - details.push(CheckDetail::new("FABRO_JWT_PUBLIC_KEY valid".to_string())); - } - } - - if let Some(raw) = state.server_secret("FABRO_JWT_PRIVATE_KEY") { - if let Err(err) = decode_pem_value("FABRO_JWT_PRIVATE_KEY", &raw).and_then(|pem| { - jsonwebtoken::EncodingKey::from_ed_pem(pem.as_bytes()) - .map(|_| ()) - .map_err(|e| format!("invalid JWT private key: {e}")) - }) { - errors.push(err); - } else { - details.push(CheckDetail::new("FABRO_JWT_PRIVATE_KEY valid".to_string())); - } - } - if errors.is_empty() { CheckResult { name: "Crypto".to_string(), diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 65deb4c5c..276c99e9c 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -18,9 +18,8 @@ use fabro_config::envfile::EnvFileUpdate; use fabro_config::{Storage, resolve_server_from_file}; use fabro_install::{ InstallListenConfig, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, - PendingSettingsWrite, VaultSecretWrite, generate_jwt_keypair, merge_server_settings, - persist_install_outputs_direct, write_github_app_settings, write_object_store_settings, - write_token_settings, + PendingSettingsWrite, VaultSecretWrite, merge_server_settings, persist_install_outputs_direct, + write_github_app_settings, write_object_store_settings, write_token_settings, }; use fabro_model::Provider; use fabro_store::ArtifactStore; @@ -43,7 +42,7 @@ use zeroize::Zeroizing; use crate::error::ApiError; use crate::serve::{self, DEFAULT_TCP_PORT}; -use crate::server_secrets::ServerSecrets; +use crate::server_secrets::{ProcessEnv, ServerSecrets}; use crate::{security_headers, static_files}; #[derive(Clone)] @@ -962,7 +961,8 @@ async fn validate_install_object_store_selection( let server_env_path = Storage::new(state.storage_dir.as_ref()) .runtime_directory() .env_path(); - let server_secrets = ServerSecrets::load(server_env_path).map_err(|err| err.to_string())?; + let server_secrets = + ServerSecrets::load(server_env_path, &ProcessEnv).map_err(|err| err.to_string())?; let build_options = serve::ObjectStoreBuildOptions { client_options, retry_config: RetryConfig { @@ -1362,23 +1362,7 @@ async fn post_install_finish( }; let session_secret = session_secret::generate_session_secret(); - let (jwt_private_pem, jwt_public_pem) = match generate_jwt_keypair() { - Ok(value) => value, - Err(err) => { - return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); - } - }; - server_env_writes.extend([ - make_env_write( - "FABRO_JWT_PRIVATE_KEY", - BASE64_STANDARD.encode(jwt_private_pem.as_bytes()), - ), - make_env_write( - "FABRO_JWT_PUBLIC_KEY", - BASE64_STANDARD.encode(jwt_public_pem.as_bytes()), - ), - make_env_write("SESSION_SECRET", session_secret), - ]); + server_env_writes.push(make_env_write("SESSION_SECRET", session_secret)); if let Some(token) = dev_token.as_ref() { server_env_writes.push(make_env_write("FABRO_DEV_TOKEN", token.clone())); } @@ -1981,6 +1965,7 @@ mod tests { classify_object_store_validation_error, detect_canonical_url, install_object_store_lookup, lock_unpoisoned, resolve_install_object_store_state, token_is_valid, }; + use crate::server_secrets::StubEnv; #[test] fn token_validation_accepts_any_matching_source() { @@ -2105,7 +2090,7 @@ AWS_SESSION_TOKEN=ambient-session\n\ AWS_WEB_IDENTITY_TOKEN_FILE=/tmp/fabro-web-identity-token\n", ) .unwrap(); - let server_secrets = ServerSecrets::with_env_lookup(env_path.clone(), |_| None).unwrap(); + let server_secrets = ServerSecrets::load(env_path.clone(), &StubEnv::default()).unwrap(); let manual_credentials = InstallAwsCredentialPair::new("submitted-access", "submitted-secret"); diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 92c3d936d..582fcaca7 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -32,7 +32,10 @@ pub mod serve; pub mod server; mod server_secrets; mod settings_view; +mod spawn_env; +mod startup; pub mod static_files; pub mod web_auth; pub use error::{ApiError, Error, Result}; +pub use startup::{EnvSource, ProcessEnv, StartupValidationError, validate_startup}; diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 048f18bf3..ecd0686af 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -31,12 +31,12 @@ use tracing::{error, info, warn}; use crate::canonical_origin::resolve_canonical_origin; use crate::github_webhooks::{TailscaleFunnelManager, WEBHOOK_ROUTE, WEBHOOK_SECRET_ENV}; use crate::ip_allowlist::{GitHubMetaResolver, IpAllowlistConfig, resolve_ip_allowlist_config}; -use crate::jwt_auth::resolve_auth_mode_with_lookup; use crate::server::{ AppState, AppStateConfig, RouterOptions, build_app_state, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, spawn_scheduler, }; -use crate::server_secrets::ServerSecrets; +use crate::server_secrets::{ProcessEnv, ServerSecrets}; +use crate::startup::resolve_startup; const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; const AWS_SESSION_TOKEN_ENV: &str = "AWS_SESSION_TOKEN"; @@ -449,6 +449,15 @@ fn resolve_server_settings(file: &SettingsLayer) -> anyhow::Result anyhow::Result { + let disk_settings = load_settings(args.config.as_deref())?; + let effective_settings = apply_runtime_settings(&disk_settings, args, data_dir); + resolve_server_settings(&effective_settings) +} + pub fn resolve_bind_request_from_settings( settings: &SettingsLayer, explicit_bind: Option<&str>, @@ -509,7 +518,7 @@ fn load_server_secrets_for_settings( ) -> anyhow::Result { let storage_root = resolve_interp_path(&settings.storage.root)?; let server_env_path = Storage::new(&storage_root).runtime_directory().env_path(); - ServerSecrets::load(server_env_path).map_err(anyhow::Error::from) + ServerSecrets::load(server_env_path, &ProcessEnv).map_err(anyhow::Error::from) } pub(crate) fn build_artifact_object_store_with_server_secrets( @@ -591,24 +600,19 @@ where let storage = Storage::new(&data_dir); let vault_path = storage.secrets_path(); let server_env_path = storage.runtime_directory().env_path(); - let server_secrets = ServerSecrets::load(server_env_path.clone())?; - let webhook_secret_present = server_secrets.get(WEBHOOK_SECRET_ENV).is_some(); - // Shared config for live reloading let effective_settings = apply_runtime_settings(&disk_settings, &args, &data_dir); let resolved_server_settings = resolve_server_settings(&effective_settings)?; + let startup = resolve_startup(&server_env_path, &ProcessEnv, &resolved_server_settings)?; + let auth_mode = startup.auth_mode; + let server_secrets = startup.server_secrets; + let webhook_secret_present = server_secrets.get(WEBHOOK_SECRET_ENV).is_some(); let bind_request = resolve_bind_request_from_settings(&effective_settings, args.bind.as_deref())?; let shared_settings = Arc::new(RwLock::new(effective_settings)); std::fs::create_dir_all(&data_dir) .with_context(|| format!("creating data directory {}", data_dir.display()))?; - let (auth_mode, max_concurrent_runs) = { - let auth_mode = resolve_auth_mode_with_lookup(&resolved_server_settings, |name| { - server_secrets.get(name) - })?; - let max_concurrent_runs = resolved_server_settings.scheduler.max_concurrent_runs; - (auth_mode, max_concurrent_runs) - }; + let max_concurrent_runs = resolved_server_settings.scheduler.max_concurrent_runs; let web_enabled = router_web_enabled(&resolved_server_settings); let github_meta_resolver = GitHubMetaResolver::from_cache_dir(&storage.cache_dir())?; @@ -641,7 +645,7 @@ where store, artifact_store, vault_path, - server_env_path, + server_secrets, local_daemon_mode: true, env_lookup, http_client: None, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 32ed01eb1..cff4ddde1 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -125,8 +125,9 @@ use crate::jwt_auth::{ use crate::run_files::{FilesInFlight, list_run_files, new_files_in_flight}; use crate::run_selector::{ResolveRunError, resolve_run_by_selector}; use crate::server_secrets::{ - LlmClientResult, ProviderCredentials, ServerSecrets, auth_issue_message, + LlmClientResult, ProviderCredentials, ServerSecrets, StubEnv, auth_issue_message, }; +use crate::spawn_env::{apply_render_graph_env, apply_worker_env}; use crate::{ demo, diagnostics, run_manifest, security_headers, settings_view, static_files, web_auth, }; @@ -576,7 +577,7 @@ pub struct AppState { pub(crate) files_in_flight: FilesInFlight, pub(crate) vault: Arc>, - pub(crate) server_secrets: ServerSecrets, + pub(super) server_secrets: ServerSecrets, pub(crate) provider_credentials: ProviderCredentials, pub(crate) settings: Arc>, pub(crate) server_settings: RwLock>, @@ -596,7 +597,7 @@ pub(crate) struct AppStateConfig { pub(crate) store: Arc, pub(crate) artifact_store: ArtifactStore, pub(crate) vault_path: PathBuf, - pub(crate) server_env_path: PathBuf, + pub(crate) server_secrets: ServerSecrets, pub(crate) local_daemon_mode: bool, pub(crate) env_lookup: EnvLookup, pub(crate) http_client: Option, @@ -2489,6 +2490,21 @@ pub fn create_app_state_with_env_lookup( settings: SettingsLayer, max_concurrent_runs: usize, env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, +) -> Arc { + create_app_state_with_env_lookup_and_server_secret_env( + settings, + max_concurrent_runs, + env_lookup, + HashMap::new(), + ) +} + +#[doc(hidden)] +pub fn create_app_state_with_env_lookup_and_server_secret_env( + settings: SettingsLayer, + max_concurrent_runs: usize, + env_lookup: impl Fn(&str) -> Option + Send + Sync + 'static, + server_secret_env: HashMap, ) -> Arc { let (store, artifact_store) = test_store_bundle(); let env_lookup: EnvLookup = Arc::new(env_lookup); @@ -2499,6 +2515,8 @@ pub fn create_app_state_with_env_lookup( ); config.store = store; config.artifact_store = artifact_store; + let server_env_path = config.vault_path.with_file_name("server.env"); + config.server_secrets = load_test_server_secrets(server_env_path, server_secret_env); build_app_state(config).expect("test app state should build") } @@ -2535,7 +2553,7 @@ pub(crate) fn create_test_app_state_with_session_key( store, artifact_store, vault_path, - server_env_path, + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), local_daemon_mode, env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), @@ -2571,7 +2589,7 @@ fn default_test_app_state_config( store, artifact_store, vault_path, - server_env_path, + server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), local_daemon_mode: false, env_lookup, http_client: Some(fabro_http::test_http_client().expect("test HTTP client should build")), @@ -2628,6 +2646,10 @@ fn default_env_lookup() -> EnvLookup { Arc::new(|name| std::env::var(name).ok()) } +fn load_test_server_secrets(path: PathBuf, env: HashMap) -> ServerSecrets { + ServerSecrets::load(path, &StubEnv(env)).expect("test server secrets should load") +} + pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result> { let AppStateConfig { settings, @@ -2636,17 +2658,13 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result Router { let secret = TEST_WEBHOOK_SECRET.to_string(); - let state = create_app_state_with_env_lookup(SettingsLayer::default(), 5, move |name| { - (name == WEBHOOK_SECRET_ENV).then(|| secret.clone()) - }); + let state = create_app_state_with_env_lookup_and_server_secret_env( + SettingsLayer::default(), + 5, + |_| None, + HashMap::from([(WEBHOOK_SECRET_ENV.to_string(), secret)]), + ); build_router_with_options( state, &auth_mode, @@ -7870,12 +7890,14 @@ type = "http" ) .unwrap(); - let secrets = - ServerSecrets::with_env_lookup(dir.path().join("server.env"), |name| match name { - "SESSION_SECRET" => Some("env-value".to_string()), - _ => None, - }) - .unwrap(); + let secrets = ServerSecrets::load( + dir.path().join("server.env"), + &StubEnv(HashMap::from([( + "SESSION_SECRET".to_string(), + "env-value".to_string(), + )])), + ) + .unwrap(); assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("env-value")); assert_eq!( @@ -7899,7 +7921,7 @@ type = "http" .unwrap(); assert_eq!( command_env_value(&github_cmd, "FABRO_DEV_TOKEN"), - EnvOverride::Removed + EnvOverride::Unchanged ); let dev_token = tempfile::tempdir().unwrap(); @@ -7955,10 +7977,14 @@ allowed_usernames = ["octocat"] .write(&runtime_directory) .unwrap(); - create_app_state_with_env_lookup(settings, 5, move |name| match name { - "FABRO_DEV_TOKEN" => dev_token.clone(), - _ => None, - }) + create_app_state_with_env_lookup_and_server_secret_env( + settings, + 5, + |_| None, + dev_token + .map(|token| HashMap::from([("FABRO_DEV_TOKEN".to_string(), token)])) + .unwrap_or_default(), + ) } #[cfg(unix)] diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index d296f0f0b..a27a2acde 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -11,6 +11,27 @@ use tokio::sync::RwLock as AsyncRwLock; type EnvLookup = Arc Option + Send + Sync>; +pub trait EnvSource { + fn snapshot(&self) -> HashMap; +} + +pub struct ProcessEnv; + +impl EnvSource for ProcessEnv { + fn snapshot(&self) -> HashMap { + std::env::vars().collect() + } +} + +#[derive(Clone, Debug, Default)] +pub(crate) struct StubEnv(pub(crate) HashMap); + +impl EnvSource for StubEnv { + fn snapshot(&self) -> HashMap { + self.0.clone() + } +} + #[derive(Debug, thiserror::Error)] pub(crate) enum Error { #[error(transparent)] @@ -19,28 +40,24 @@ pub(crate) enum Error { pub(crate) struct ServerSecrets { path: PathBuf, + env_entries: HashMap, file_entries: HashMap, - env_lookup: EnvLookup, } impl ServerSecrets { - pub(crate) fn load(path: PathBuf) -> Result { - Self::with_env_lookup(path, |name| std::env::var(name).ok()) - } - - pub(crate) fn with_env_lookup(path: PathBuf, env_lookup: F) -> Result - where - F: Fn(&str) -> Option + Send + Sync + 'static, - { + pub(crate) fn load(path: PathBuf, env: &dyn EnvSource) -> Result { Ok(Self { + env_entries: env.snapshot(), file_entries: envfile::read_env_file(&path)?, path, - env_lookup: Arc::new(env_lookup), }) } pub(crate) fn get(&self, name: &str) -> Option { - (self.env_lookup)(name).or_else(|| self.file_entries.get(name).cloned()) + self.env_entries + .get(name) + .cloned() + .or_else(|| self.file_entries.get(name).cloned()) } } @@ -48,6 +65,7 @@ impl std::fmt::Debug for ServerSecrets { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("ServerSecrets") .field("path", &self.path) + .field("env_entries", &self.env_entries.keys().collect::>()) .field( "file_entries", &self.file_entries.keys().collect::>(), @@ -149,13 +167,15 @@ impl std::fmt::Debug for ProviderCredentials { #[cfg(test)] mod tests { + use std::collections::HashMap; use std::sync::Arc; use fabro_auth::{AuthCredential, AuthDetails}; + use fabro_config::envfile; use fabro_vault::{SecretType, Vault}; use tokio::sync::RwLock as AsyncRwLock; - use super::ProviderCredentials; + use super::{ProviderCredentials, ServerSecrets, StubEnv}; use crate::server_secrets::Provider; #[tokio::test] @@ -198,4 +218,48 @@ mod tests { Provider::Anthropic ]); } + + #[test] + fn server_secrets_snapshot_prefers_env_over_file() { + let dir = tempfile::tempdir().unwrap(); + let env_path = dir.path().join("server.env"); + envfile::write_env_file( + &env_path, + &HashMap::from([ + ("SESSION_SECRET".to_string(), "file-value".to_string()), + ( + "GITHUB_APP_CLIENT_SECRET".to_string(), + "file-client".to_string(), + ), + ]), + ) + .unwrap(); + + let secrets = ServerSecrets::load( + env_path, + &StubEnv(HashMap::from([( + "SESSION_SECRET".to_string(), + "env-value".to_string(), + )])), + ) + .unwrap(); + + assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("env-value")); + assert_eq!( + secrets.get("GITHUB_APP_CLIENT_SECRET").as_deref(), + Some("file-client") + ); + } + + #[test] + fn server_secrets_snapshot_is_owned_after_load() { + let dir = tempfile::tempdir().unwrap(); + let env_path = dir.path().join("server.env"); + let mut env = HashMap::from([("SESSION_SECRET".to_string(), "before".to_string())]); + + let secrets = ServerSecrets::load(env_path, &StubEnv(env.clone())).unwrap(); + env.insert("SESSION_SECRET".to_string(), "after".to_string()); + + assert_eq!(secrets.get("SESSION_SECRET").as_deref(), Some("before")); + } } diff --git a/lib/crates/fabro-server/src/spawn_env.rs b/lib/crates/fabro-server/src/spawn_env.rs new file mode 100644 index 000000000..d9a6029cc --- /dev/null +++ b/lib/crates/fabro-server/src/spawn_env.rs @@ -0,0 +1,137 @@ +use std::ffi::OsString; + +use tokio::process::Command; + +const WORKER_ENV_ALLOWLIST: &[&str] = &[ + "PATH", // process essentials + "HOME", // process essentials + "TMPDIR", // temp file staging + "USER", // process identity + "RUST_LOG", // diagnostics + "RUST_BACKTRACE", // diagnostics + "FABRO_HOME", // worker state lookup + "FABRO_STORAGE_ROOT", // worker state lookup +]; + +const RENDER_GRAPH_ENV_ALLOWLIST: &[&str] = &[ + "PATH", // executable lookup + "HOME", // graphviz/font resolution + "TMPDIR", // temp file staging +]; + +pub(crate) fn apply_worker_env(cmd: &mut Command) { + apply_worker_env_with_lookup(cmd, &|name| std::env::var_os(name)); +} + +pub(crate) fn apply_render_graph_env(cmd: &mut Command) { + apply_render_graph_env_with_lookup(cmd, &|name| std::env::var_os(name)); +} + +fn apply_worker_env_with_lookup(cmd: &mut Command, lookup: &dyn Fn(&str) -> Option) { + apply_allowlist(cmd, WORKER_ENV_ALLOWLIST, lookup); +} + +fn apply_render_graph_env_with_lookup( + cmd: &mut Command, + lookup: &dyn Fn(&str) -> Option, +) { + apply_allowlist(cmd, RENDER_GRAPH_ENV_ALLOWLIST, lookup); +} + +fn apply_allowlist(cmd: &mut Command, keys: &[&str], lookup: &dyn Fn(&str) -> Option) { + cmd.env_clear(); + for key in keys { + if let Some(value) = lookup(key) { + cmd.env(key, value); + } + } +} + +#[cfg(all(test, unix))] +mod tests { + use std::collections::HashMap; + use std::ffi::OsString; + use std::path::Path; + + use super::{apply_render_graph_env_with_lookup, apply_worker_env_with_lookup}; + + fn env_command() -> tokio::process::Command { + assert!(Path::new("/usr/bin/env").exists()); + tokio::process::Command::new("/usr/bin/env") + } + + fn env_output(mut cmd: tokio::process::Command) -> HashMap { + let runtime = tokio::runtime::Runtime::new().expect("creating test Tokio runtime"); + runtime.block_on(async move { + let output = cmd.output().await.expect("running env subprocess"); + assert!(output.status.success()); + String::from_utf8(output.stdout) + .expect("parsing env subprocess output as UTF-8") + .lines() + .filter_map(|line| { + let (key, value) = line.split_once('=')?; + Some((key.to_string(), value.to_string())) + }) + .collect() + }) + } + + #[test] + fn worker_allowlist_is_fail_closed() { + let env = HashMap::from([ + ("PATH".to_string(), "/bin".to_string()), + ("HOME".to_string(), "/tmp/home".to_string()), + ("TMPDIR".to_string(), "/tmp".to_string()), + ("USER".to_string(), "alice".to_string()), + ("RUST_LOG".to_string(), "debug".to_string()), + ("FABRO_HOME".to_string(), "/tmp/fabro-home".to_string()), + ( + "FABRO_STORAGE_ROOT".to_string(), + "/tmp/fabro-storage".to_string(), + ), + ("SESSION_SECRET".to_string(), "leak".to_string()), + ("FABRO_DEV_TOKEN".to_string(), "garbage".to_string()), + ("MY_API_KEY".to_string(), "blocked".to_string()), + ]); + let mut cmd = env_command(); + apply_worker_env_with_lookup(&mut cmd, &|name| env.get(name).map(OsString::from)); + cmd.env( + "FABRO_DEV_TOKEN", + "fabro_dev_abababababababababababababababababababababababababababababababab", + ); + + let actual = env_output(cmd); + + assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); + assert_eq!(actual.get("HOME").map(String::as_str), Some("/tmp/home")); + assert_eq!( + actual.get("FABRO_DEV_TOKEN").map(String::as_str), + Some("fabro_dev_abababababababababababababababababababababababababababababababab") + ); + assert!(!actual.contains_key("SESSION_SECRET")); + assert!(!actual.contains_key("MY_API_KEY")); + } + + #[test] + fn render_graph_allowlist_is_fail_closed() { + let env = HashMap::from([ + ("PATH".to_string(), "/bin".to_string()), + ("HOME".to_string(), "/tmp/home".to_string()), + ("TMPDIR".to_string(), "/tmp".to_string()), + ("FABRO_TELEMETRY".to_string(), "on".to_string()), + ("SESSION_SECRET".to_string(), "leak".to_string()), + ]); + let mut cmd = env_command(); + apply_render_graph_env_with_lookup(&mut cmd, &|name| env.get(name).map(OsString::from)); + cmd.env("FABRO_TELEMETRY", "off"); + + let actual = env_output(cmd); + + assert_eq!(actual.get("PATH").map(String::as_str), Some("/bin")); + assert_eq!( + actual.get("FABRO_TELEMETRY").map(String::as_str), + Some("off") + ); + assert!(!actual.contains_key("SESSION_SECRET")); + } +} diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs new file mode 100644 index 000000000..32e6ce606 --- /dev/null +++ b/lib/crates/fabro-server/src/startup.rs @@ -0,0 +1,118 @@ +use std::path::Path; + +use fabro_types::settings::ServerSettings as ResolvedServerSettings; + +use crate::jwt_auth::{AuthMode, resolve_auth_mode_with_lookup}; +pub use crate::server_secrets::{EnvSource, ProcessEnv}; +use crate::server_secrets::{Error as ServerSecretsError, ServerSecrets}; + +#[derive(Debug)] +pub(crate) struct StartupResolution { + pub(crate) auth_mode: AuthMode, + pub(crate) server_secrets: ServerSecrets, +} + +#[derive(Debug, thiserror::Error)] +pub enum StartupValidationError { + #[error("{0}")] + Message(String), +} + +impl From for StartupValidationError { + fn from(err: ServerSecretsError) -> Self { + Self::Message(err.to_string()) + } +} + +impl From for StartupValidationError { + fn from(err: anyhow::Error) -> Self { + Self::Message(err.to_string()) + } +} + +pub(crate) fn resolve_startup( + env_path: &Path, + env: &dyn EnvSource, + settings: &ResolvedServerSettings, +) -> std::result::Result { + let server_secrets = ServerSecrets::load(env_path.to_path_buf(), env)?; + let auth_mode = resolve_auth_mode_with_lookup(settings, |name| server_secrets.get(name))?; + Ok(StartupResolution { + auth_mode, + server_secrets, + }) +} + +pub fn validate_startup( + env_path: &Path, + env: &dyn EnvSource, + settings: &ResolvedServerSettings, +) -> std::result::Result<(), StartupValidationError> { + resolve_startup(env_path, env, settings).map(|_| ()) +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use fabro_config::parse_settings_layer; + use fabro_types::settings::ServerSettings as ResolvedServerSettings; + + use super::{resolve_startup, validate_startup}; + use crate::server_secrets::StubEnv; + + fn resolved_settings(auth_methods: &[&str]) -> ResolvedServerSettings { + let settings = parse_settings_layer(&format!( + r" +_version = 1 + +[server.auth] +methods = [{}] +", + auth_methods + .iter() + .map(|method| format!("\"{method}\"")) + .collect::>() + .join(", ") + )) + .unwrap(); + fabro_config::resolve_server_from_file(&settings).unwrap() + } + + #[test] + fn validate_startup_matches_resolve_startup() { + let dir = tempfile::tempdir().unwrap(); + let env = StubEnv(HashMap::from([ + ( + "SESSION_SECRET".to_string(), + "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef".to_string(), + ), + ( + "FABRO_DEV_TOKEN".to_string(), + "fabro_dev_abababababababababababababababababababababababababababababababab" + .to_string(), + ), + ])); + let settings = resolved_settings(&["dev-token"]); + + assert!(validate_startup(dir.path().join("server.env").as_path(), &env, &settings).is_ok()); + assert!(resolve_startup(dir.path().join("server.env").as_path(), &env, &settings).is_ok()); + } + + #[test] + fn validate_startup_and_resolve_startup_share_errors() { + let dir = tempfile::tempdir().unwrap(); + let env = StubEnv::default(); + let settings = resolved_settings(&["dev-token"]); + + let validate_err = + validate_startup(dir.path().join("server.env").as_path(), &env, &settings) + .unwrap_err() + .to_string(); + let resolve_err = resolve_startup(dir.path().join("server.env").as_path(), &env, &settings) + .unwrap_err() + .to_string(); + + assert_eq!(validate_err, resolve_err); + } +} diff --git a/lib/crates/fabro-server/tests/it/api/docs.rs b/lib/crates/fabro-server/tests/it/api/docs.rs index e20617f90..006fe67db 100644 --- a/lib/crates/fabro-server/tests/it/api/docs.rs +++ b/lib/crates/fabro-server/tests/it/api/docs.rs @@ -23,8 +23,12 @@ fn security_doc_does_not_require_jwt_keys_for_the_current_web_flow() { "security doc should still mention the session secret" ); assert!( - !security.contains("`FABRO_JWT_PRIVATE_KEY`, `FABRO_JWT_PUBLIC_KEY`, and `SESSION_SECRET`"), - "security doc should not describe JWT keys as required for the current web flow" + !security.contains("FABRO_JWT_PRIVATE_KEY"), + "security doc should not mention removed JWT key settings" + ); + assert!( + !security.contains("FABRO_JWT_PUBLIC_KEY"), + "security doc should not mention removed JWT key settings" ); } diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index c5bc618d0..95bc8d4d6 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -764,8 +764,6 @@ async fn token_install_finish_persists_settings_env_and_vault() { .env_path(), ) .unwrap(); - assert!(server_env.contains("FABRO_JWT_PRIVATE_KEY=")); - assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=")); assert!(server_env.contains("SESSION_SECRET=")); assert!(server_env.contains("FABRO_DEV_TOKEN=")); assert!(!server_env.contains("AWS_ACCESS_KEY_ID=")); diff --git a/lib/crates/fabro-server/tests/it/openapi_conformance.rs b/lib/crates/fabro-server/tests/it/openapi_conformance.rs index 898d6fc2a..30589c696 100644 --- a/lib/crates/fabro-server/tests/it/openapi_conformance.rs +++ b/lib/crates/fabro-server/tests/it/openapi_conformance.rs @@ -12,7 +12,7 @@ use axum::body::Body; use axum::http::{Method, Request, StatusCode}; use fabro_server::install::{InstallAppState, build_install_router}; use fabro_server::jwt_auth::AuthMode; -use fabro_server::server::{build_router, create_app_state_with_env_lookup}; +use fabro_server::server::{build_router, create_app_state_with_env_lookup_and_server_secret_env}; use serde_yaml::Value; use tower::ServiceExt; @@ -146,9 +146,12 @@ fn github_webhook_spec_and_sdk_describe_a_json_body() { async fn github_webhook_spec_route_is_routable_when_webhook_secret_is_present() { let secret = "test-webhook-secret".to_string(); let app = build_router( - create_app_state_with_env_lookup(test_settings(), 5, move |name| { - (name == "GITHUB_APP_WEBHOOK_SECRET").then(|| secret.clone()) - }), + create_app_state_with_env_lookup_and_server_secret_env( + test_settings(), + 5, + |_| None, + std::collections::HashMap::from([("GITHUB_APP_WEBHOOK_SECRET".to_string(), secret)]), + ), AuthMode::Disabled, ); diff --git a/lib/crates/fabro-telemetry/src/spawn.rs b/lib/crates/fabro-telemetry/src/spawn.rs index 741ea8748..08e4abe05 100644 --- a/lib/crates/fabro-telemetry/src/spawn.rs +++ b/lib/crates/fabro-telemetry/src/spawn.rs @@ -37,7 +37,7 @@ pub fn spawn_detached(args: &[&str], env: &[(&str, &str)], env_remove: &[&str]) #[expect( clippy::disallowed_types, clippy::disallowed_methods, - reason = "Detaching must flush stdio synchronously before the double-fork." + reason = "Detaching must flush stdio synchronously before the double-fork; post-fork pre-exec env mutation is the one allowed exception to the workspace env-mutation ban." )] fn spawn_detached_unix(args: &[&str], env: &[(&str, &str)], env_remove: &[&str]) { // Flush stdout/stderr before forking so the child process doesn't inherit diff --git a/lib/crates/fabro-test/src/lib.rs b/lib/crates/fabro-test/src/lib.rs index 05b5cca44..4c9f690bf 100644 --- a/lib/crates/fabro-test/src/lib.rs +++ b/lib/crates/fabro-test/src/lib.rs @@ -631,8 +631,11 @@ fn write_settings_file(path: &Path, storage_dir: &Path, rest: &str) { fn write_test_server_dev_token(storage_dir: &Path) { let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); - envfile::merge_env_file(&server_env_path, [("FABRO_DEV_TOKEN", TEST_DEV_TOKEN)]) - .unwrap_or_else(|err| panic!("failed to write {}: {err}", server_env_path.display())); + envfile::merge_env_file(&server_env_path, [ + ("FABRO_DEV_TOKEN", TEST_DEV_TOKEN), + ("SESSION_SECRET", TEST_SESSION_SECRET), + ]) + .unwrap_or_else(|err| panic!("failed to write {}: {err}", server_env_path.display())); } fn write_test_home_dev_token(settings_path: &Path) { diff --git a/test/twin/openai/src/config.rs b/test/twin/openai/src/config.rs index 1bb77929a..f7cd31dbd 100644 --- a/test/twin/openai/src/config.rs +++ b/test/twin/openai/src/config.rs @@ -11,20 +11,21 @@ pub struct Config { impl Config { pub fn from_env() -> Result { - let bind_addr = std::env::var("TWIN_OPENAI_BIND_ADDR") - .ok() + Self::from_lookup(&|name| std::env::var(name).ok()) + } + + pub fn from_lookup(lookup: &dyn Fn(&str) -> Option) -> Result { + let bind_addr = lookup("TWIN_OPENAI_BIND_ADDR") .map(|value| value.parse().context("invalid TWIN_OPENAI_BIND_ADDR")) .transpose()? .unwrap_or_else(|| SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3000)); - let require_auth = std::env::var("TWIN_OPENAI_REQUIRE_AUTH") - .ok() + let require_auth = lookup("TWIN_OPENAI_REQUIRE_AUTH") .map(|value| parse_bool_env(&value, "TWIN_OPENAI_REQUIRE_AUTH")) .transpose()? .unwrap_or(true); - let enable_admin = std::env::var("TWIN_OPENAI_ENABLE_ADMIN") - .ok() + let enable_admin = lookup("TWIN_OPENAI_ENABLE_ADMIN") .map(|value| parse_bool_env(&value, "TWIN_OPENAI_ENABLE_ADMIN")) .transpose()? .unwrap_or(true); diff --git a/test/twin/openai/tests/config_contract.rs b/test/twin/openai/tests/config_contract.rs index e49564332..283c14538 100644 --- a/test/twin/openai/tests/config_contract.rs +++ b/test/twin/openai/tests/config_contract.rs @@ -2,30 +2,14 @@ use twin_openai::config::Config; #[test] fn config_loads_from_environment() { - let prior_bind = std::env::var("TWIN_OPENAI_BIND_ADDR").ok(); - let prior_auth = std::env::var("TWIN_OPENAI_REQUIRE_AUTH").ok(); - let prior_admin = std::env::var("TWIN_OPENAI_ENABLE_ADMIN").ok(); - - std::env::set_var("TWIN_OPENAI_BIND_ADDR", "127.0.0.1:4100"); - std::env::set_var("TWIN_OPENAI_REQUIRE_AUTH", "false"); - std::env::set_var("TWIN_OPENAI_ENABLE_ADMIN", "false"); - - let config = Config::from_env().expect("config should load"); + let config = Config::from_lookup(&|name| match name { + "TWIN_OPENAI_BIND_ADDR" => Some("127.0.0.1:4100".to_string()), + "TWIN_OPENAI_REQUIRE_AUTH" | "TWIN_OPENAI_ENABLE_ADMIN" => Some("false".to_string()), + _ => None, + }) + .expect("config should load"); assert_eq!(config.bind_addr.to_string(), "127.0.0.1:4100"); assert!(!config.require_auth); assert!(!config.enable_admin); - - match prior_bind { - Some(value) => std::env::set_var("TWIN_OPENAI_BIND_ADDR", value), - None => std::env::remove_var("TWIN_OPENAI_BIND_ADDR"), - } - match prior_auth { - Some(value) => std::env::set_var("TWIN_OPENAI_REQUIRE_AUTH", value), - None => std::env::remove_var("TWIN_OPENAI_REQUIRE_AUTH"), - } - match prior_admin { - Some(value) => std::env::set_var("TWIN_OPENAI_ENABLE_ADMIN", value), - None => std::env::remove_var("TWIN_OPENAI_ENABLE_ADMIN"), - } }