From 3fa7b65182d032cc15decf0bbfe7818f1e74a386 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 12 Apr 2026 14:03:54 -0400 Subject: [PATCH] Split server runtime secrets from vault secrets --- Cargo.lock | 14 + apps/fabro-web/app/routes/setup-complete.tsx | 28 +- docs/administration/server-configuration.mdx | 15 +- docs/api-reference/fabro-api.yaml | 9 +- docs/integrations/github.mdx | 15 +- lib/crates/fabro-cli/Cargo.toml | 1 + lib/crates/fabro-cli/src/commands/install.rs | 95 +++++-- lib/crates/fabro-config/Cargo.toml | 1 + lib/crates/fabro-config/src/envfile.rs | 174 ++++++++++++ lib/crates/fabro-config/src/lib.rs | 1 + lib/crates/fabro-config/src/storage.rs | 9 + lib/crates/fabro-server/Cargo.toml | 1 + lib/crates/fabro-server/src/diagnostics.rs | 36 +-- lib/crates/fabro-server/src/error.rs | 9 +- lib/crates/fabro-server/src/jwt_auth.rs | 6 +- lib/crates/fabro-server/src/lib.rs | 2 +- lib/crates/fabro-server/src/serve.rs | 29 +- lib/crates/fabro-server/src/server.rs | 171 ++++++----- lib/crates/fabro-server/src/server_secrets.rs | 145 ++++++++++ lib/crates/fabro-server/src/web_auth.rs | 48 ++-- lib/crates/fabro-vault/Cargo.toml | 22 ++ .../src/lib.rs} | 268 +++--------------- 22 files changed, 708 insertions(+), 391 deletions(-) create mode 100644 lib/crates/fabro-config/src/envfile.rs create mode 100644 lib/crates/fabro-server/src/server_secrets.rs create mode 100644 lib/crates/fabro-vault/Cargo.toml rename lib/crates/{fabro-server/src/secret_store.rs => fabro-vault/src/lib.rs} (50%) diff --git a/Cargo.lock b/Cargo.lock index 11cd5bb62..98dda030c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1584,6 +1584,7 @@ dependencies = [ "fabro-types", "fabro-util", "fabro-validate", + "fabro-vault", "fabro-workflow", "futures", "git2", @@ -1636,6 +1637,7 @@ dependencies = [ "thiserror 2.0.18", "toml 0.8.23", "tracing", + "ulid", ] [[package]] @@ -1913,6 +1915,7 @@ dependencies = [ "fabro-types", "fabro-util", "fabro-validate", + "fabro-vault", "fabro-workflow", "futures-util", "hex", @@ -2121,6 +2124,17 @@ dependencies = [ "thiserror 2.0.18", ] +[[package]] +name = "fabro-vault" +version = "0.176.2" +dependencies = [ + "chrono", + "serde", + "serde_json", + "tempfile", + "ulid", +] + [[package]] name = "fabro-workflow" version = "0.176.2" diff --git a/apps/fabro-web/app/routes/setup-complete.tsx b/apps/fabro-web/app/routes/setup-complete.tsx index 58b3eebd2..566e43e3d 100644 --- a/apps/fabro-web/app/routes/setup-complete.tsx +++ b/apps/fabro-web/app/routes/setup-complete.tsx @@ -7,6 +7,7 @@ export default function SetupComplete() { const code = useMemo(() => new URLSearchParams(window.location.search).get("code"), []); const [state, setState] = useState(code ? "registering" : "done"); const [error, setError] = useState(null); + const [restartRequired, setRestartRequired] = useState(false); const registeredRef = useRef(false); useEffect(() => { @@ -33,6 +34,8 @@ export default function SetupComplete() { return; } + const payload = await response.json().catch(() => ({})); + setRestartRequired(payload.restart_required === true); setState("done"); window.history.replaceState({}, "", "/setup/complete"); } @@ -59,14 +62,25 @@ export default function SetupComplete() { Setup complete

- Your GitHub App has been registered and configured. + {restartRequired + ? "Your GitHub App is configured. Restart the Fabro server before attempting login." + : "Your GitHub App has been registered and configured."}

- - Continue to sign in - + {restartRequired ? ( + + Back to setup + + ) : ( + + Continue to sign in + + )} )} diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index cd29ffefb..ac931fb63 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -162,7 +162,7 @@ Customize the git author identity used for checkpoint commits. When not set, def ### `[server.integrations.github]` section -Configure GitHub integration auth. `strategy = "gh_cli"` is the default and uses a stored `GITHUB_CLI_TOKEN` from the server secret store. `strategy = "app"` enables the GitHub App flow, browser OAuth, and webhooks. +Configure GitHub integration auth. `strategy = "gh_cli"` is the default and uses a stored `GITHUB_CLI_TOKEN` from the vault. `strategy = "app"` enables the GitHub App flow, browser OAuth, and webhooks. ```toml title="settings.toml" [server.integrations.github] @@ -206,10 +206,17 @@ The same `[features]` section can be set in `.fabro/project.toml` (project-level ## Secrets and environment variables -Fabro stores server-managed credentials in `/secrets.json` and also honors relevant environment variables from the server process environment as a fallback. Fabro no longer auto-loads `.env` files. Provider API keys are required for the models you want to use; everything else is optional. +Fabro splits secrets into two scopes: + +- Server runtime secrets live in `/server.env` and resolve with precedence `process env -> server.env`. +- Workflow-visible secrets live in `/secrets.json` (the vault). Anything stored in the vault may be used by workflows. + +Fabro no longer auto-loads `.env` files. Provider API keys are required for the models you want to use; everything else is optional. ### LLM provider keys +Fabro's built-in provider access resolves these from `process env -> vault`. + | Variable | Provider | |---|---| | `ANTHROPIC_API_KEY` | Anthropic (Claude) | @@ -229,6 +236,8 @@ Fabro stores server-managed credentials in `/secrets.json` and also ho ### Server authentication +Fabro resolves these from `process env -> server.env`. + | Variable | Description | |---|---| | `FABRO_JWT_PRIVATE_KEY` | Ed25519 private key (base64-encoded PEM) for JWT signing | @@ -243,6 +252,8 @@ Fabro stores server-managed credentials in `/secrets.json` and also ho ### GitHub App extras (optional) +Fabro resolves these from `process env -> server.env`. + | Variable | Description | |---|---| | `GITHUB_APP_CLIENT_SECRET` | GitHub App client secret | diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml index a635bbc55..2cb28f1ea 100644 --- a/docs/api-reference/fabro-api.yaml +++ b/docs/api-reference/fabro-api.yaml @@ -1318,8 +1318,8 @@ paths: get: operationId: listSecrets tags: [Secrets] - summary: List stored secrets - description: Returns stored secret names and timestamps. Secret values are never exposed. + summary: List vault secrets + description: Returns workflow-visible vault secret names and timestamps. Secret values are never exposed. responses: "200": description: Secret metadata list @@ -1330,7 +1330,8 @@ paths: post: operationId: createSecret tags: [Secrets] - summary: Store or update a secret + summary: Store or update a vault secret + description: Stores a secret in the workflow-visible vault. Anything stored here may be used by workflows. requestBody: required: true content: @@ -1353,7 +1354,7 @@ paths: delete: operationId: deleteSecretByName tags: [Secrets] - summary: Delete a stored secret + summary: Delete a vault secret requestBody: required: true content: diff --git a/docs/integrations/github.mdx b/docs/integrations/github.mdx index 079ddb740..db44151c8 100644 --- a/docs/integrations/github.mdx +++ b/docs/integrations/github.mdx @@ -61,9 +61,8 @@ The rest of this page describes the `app` strategy, which is required for browse 4. GitHub redirects back to Fabro, which automatically: - Exchanges the temporary code for permanent app credentials - Writes `app_id`, `client_id`, and `slug` to `~/.fabro/settings.toml` - - Stores `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`, and `GITHUB_APP_PRIVATE_KEY` in the server secret store - - Generates a `SESSION_SECRET` for web app sessions - - Redirects you to the login page + - Stores `GITHUB_APP_CLIENT_SECRET`, `GITHUB_APP_WEBHOOK_SECRET`, and `GITHUB_APP_PRIVATE_KEY` in `/server.env` + - Marks the change as restart-bound so the server must be restarted before login 5. **Install the app** on your GitHub account or organization. Go to `https://github.com/settings/apps//installations` and install it on the repositories Fabro should access. @@ -81,9 +80,9 @@ The GitHub App check verifies five fields: |---|---| | `server.integrations.github.app_id` | `~/.fabro/settings.toml` | | `server.integrations.github.client_id` | `~/.fabro/settings.toml` | -| `GITHUB_APP_CLIENT_SECRET` | Server secret store | -| `GITHUB_APP_WEBHOOK_SECRET` | Server secret store | -| `GITHUB_APP_PRIVATE_KEY` | Server secret store | +| `GITHUB_APP_CLIENT_SECRET` | `/server.env` | +| `GITHUB_APP_WEBHOOK_SECRET` | `/server.env` | +| `GITHUB_APP_PRIVATE_KEY` | `/server.env` | If all five are set, the check passes. If none are set, it warns (GitHub integration is optional). If some are set but others are missing, it errors with the specific missing fields. @@ -106,9 +105,9 @@ slug = "fabro-a3f2" | `client_id` | OAuth Client ID for the app | | `slug` | App slug, used for linking to the GitHub App settings page | -### Server secret store +### `server.env` -Fabro stores the GitHub App secrets in the server secret store under these keys: +Fabro stores the GitHub App secrets in `/server.env` under these keys: - `GITHUB_APP_CLIENT_SECRET` - `GITHUB_APP_WEBHOOK_SECRET` diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index e716ae3d2..d476a3500 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -39,6 +39,7 @@ fabro-server = { path = "../fabro-server" } fabro-api = { path = "../fabro-api" } fabro-telemetry = { path = "../fabro-telemetry" } fabro-store = { path = "../fabro-store" } +fabro-vault = { path = "../fabro-vault" } fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fabro-http.workspace = true diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index c664a7b7d..b6127db56 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -15,9 +15,10 @@ use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; use fabro_config::user::SETTINGS_CONFIG_FILENAME; use fabro_config::{Storage, legacy_env}; use fabro_model::Provider; -use fabro_server::secret_store::{SecretStore, SecretType}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; +// Bootstrap-only direct vault writes for `fabro install` when no local server is running. +use fabro_vault::{SecretType, Vault}; use rand::Rng; use tokio::io::AsyncWriteExt; use tokio::net::TcpListener; @@ -690,7 +691,7 @@ async fn setup_github_app( Ok(env_pairs) } -async fn persist_install_secrets( +async fn persist_vault_secrets( storage_dir: &Path, secrets: &[(String, String)], server_was_running: bool, @@ -716,13 +717,35 @@ async fn persist_install_secrets( return Ok(()); } - let mut store = SecretStore::load(Storage::new(storage_dir).secrets_path())?; + let mut store = Vault::load(Storage::new(storage_dir).secrets_path())?; for (name, value) in secrets { store.set(name, value, SecretType::Environment, None)?; } Ok(()) } +fn persist_server_env_secrets(storage_dir: &Path, secrets: &[(String, String)]) -> Result<()> { + if secrets.is_empty() { + return Ok(()); + } + + fabro_config::envfile::merge_env_file( + &Storage::new(storage_dir).server_state().env_path(), + secrets.iter().cloned(), + )?; + Ok(()) +} + +async fn persist_install_outputs( + storage_dir: &Path, + server_env_secrets: &[(String, String)], + vault_secrets: &[(String, String)], + server_was_running: bool, +) -> Result<()> { + persist_server_env_secrets(storage_dir, server_env_secrets)?; + persist_vault_secrets(storage_dir, vault_secrets, server_was_running).await +} + pub(crate) async fn run_install( args: &InstallArgs, globals: &GlobalArgs, @@ -760,7 +783,7 @@ pub(crate) async fn run_install( if env_path.exists() { fabro_util::printerr!( printer, - " Warning: {} is no longer read by fabro server. This install will persist credentials in the server secret store instead.", + " Warning: {} is no longer read by fabro server. This install will persist runtime secrets in server.env and workflow-visible credentials in the vault instead.", env_path.display() ); fabro_util::printerr!(printer, ""); @@ -818,7 +841,8 @@ pub(crate) async fn run_install( fabro_util::printerr!(printer, " {}", s.dim.apply_to("──────────────────────")); fabro_util::printerr!(printer, ""); - let mut secret_pairs: Vec<(String, String)> = Vec::new(); + let mut vault_pairs: Vec<(String, String)> = Vec::new(); + let mut server_env_pairs: Vec<(String, String)> = Vec::new(); let mut configured_providers: Vec = Vec::new(); let codex_detected = detect_binary_on_path("codex").await; @@ -836,7 +860,7 @@ pub(crate) async fn run_install( if use_oauth { let pairs = run_openai_oauth_or_api_key(&s, printer).await?; - secret_pairs.extend(pairs); + vault_pairs.extend(pairs); configured_providers.push(Provider::OpenAi); openai_via_oauth = true; } @@ -859,7 +883,7 @@ pub(crate) async fn run_install( let first_provider = primary_providers[primary_idx]; { let (env_var, key) = prompt_and_validate_key(first_provider, &s, printer).await?; - secret_pairs.push((env_var, key)); + vault_pairs.push((env_var, key)); configured_providers.push(first_provider); } } @@ -893,7 +917,7 @@ pub(crate) async fn run_install( for idx in selected_indices { let provider = remaining_providers[idx]; let (env_var, key) = prompt_and_validate_key(provider, &s, printer).await?; - secret_pairs.push((env_var, key)); + vault_pairs.push((env_var, key)); } } fabro_util::printerr!(printer, ""); @@ -929,7 +953,7 @@ pub(crate) async fn run_install( write_github_cli_settings(&mut doc)?; std::fs::write(&user_toml_path, toml::to_string_pretty(&doc)?)?; fabro_util::printerr!(printer, " {} GitHub CLI configured", s.green.apply_to("✔")); - secret_pairs.push(("GITHUB_CLI_TOKEN".to_string(), token)); + vault_pairs.push(("GITHUB_CLI_TOKEN".to_string(), token)); } 1 => { let (owner, username) = prompt_github_app_owner(&s).await?; @@ -961,7 +985,7 @@ pub(crate) async fn run_install( s.green.apply_to("✔"), slug ); - secret_pairs.extend(github_env_pairs); + server_env_pairs.extend(github_env_pairs); } _ => unreachable!("prompt_select returned an out-of-range index"), } @@ -1044,12 +1068,12 @@ pub(crate) async fn run_install( 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 server_env_pairs = vec![ + let 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), ]; - secret_pairs.extend(server_env_pairs); + server_env_pairs.extend(generated_server_env_pairs); fabro_util::printerr!(printer, ""); fabro_util::printerr!(printer, " To start Fabro, run these commands:"); @@ -1058,18 +1082,34 @@ pub(crate) async fn run_install( fabro_util::printerr!(printer, ""); } - persist_install_secrets(&storage_dir, &secret_pairs, server_was_running).await?; + persist_install_outputs( + &storage_dir, + &server_env_pairs, + &vault_pairs, + server_was_running, + ) + .await?; fabro_util::printerr!( printer, - " {} Saved {} secrets to {}", + " {} Saved {} runtime secrets to {}", s.green.apply_to("✔"), - secret_pairs.len(), + server_env_pairs.len(), + Storage::new(&storage_dir) + .server_state() + .env_path() + .display() + ); + fabro_util::printerr!( + printer, + " {} Saved {} workflow-visible secrets to {}", + s.green.apply_to("✔"), + vault_pairs.len(), Storage::new(&storage_dir).secrets_path().display() ); if server_was_running { fabro_util::printerr!( printer, - " Warning: the local fabro server was already running. Restart it to pick up startup-time features that only initialize at boot." + " Warning: the local fabro server was already running. Restart it to pick up the new server.env values." ); } fabro_util::printerr!(printer, ""); @@ -1482,4 +1522,27 @@ client_id = "client-id" serde_json::json!("https://app.example.com/setup/callback"), ); } + + #[tokio::test] + async fn persist_install_outputs_offline_splits_server_env_and_vault() { + 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_pairs = vec![("OPENAI_API_KEY".to_string(), "openai-key".to_string())]; + + persist_install_outputs(dir.path(), &server_env_pairs, &vault_pairs, false) + .await + .unwrap(); + + let server_env = + std::fs::read_to_string(Storage::new(dir.path()).server_state().env_path()).unwrap(); + assert!(server_env.contains("SESSION_SECRET=session")); + assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=public-key")); + + let vault = Vault::load(Storage::new(dir.path()).secrets_path()).unwrap(); + assert_eq!(vault.get("OPENAI_API_KEY"), Some("openai-key")); + assert_eq!(vault.get("SESSION_SECRET"), None); + } } diff --git a/lib/crates/fabro-config/Cargo.toml b/lib/crates/fabro-config/Cargo.toml index f9882ecab..b2d701afc 100644 --- a/lib/crates/fabro-config/Cargo.toml +++ b/lib/crates/fabro-config/Cargo.toml @@ -29,6 +29,7 @@ strsim = "0.11" toml.workspace = true tracing.workspace = true thiserror.workspace = true +ulid.workspace = true [dev-dependencies] tempfile = "3" diff --git a/lib/crates/fabro-config/src/envfile.rs b/lib/crates/fabro-config/src/envfile.rs new file mode 100644 index 000000000..67a02bdf7 --- /dev/null +++ b/lib/crates/fabro-config/src/envfile.rs @@ -0,0 +1,174 @@ +use std::collections::HashMap; +use std::io; +use std::path::{Path, PathBuf}; + +pub fn read_env_file(path: &Path) -> io::Result> { + let contents = match std::fs::read_to_string(path) { + Ok(contents) => contents, + Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(HashMap::new()), + Err(err) => return Err(err), + }; + + let mut entries = HashMap::new(); + for (index, raw_line) in contents.lines().enumerate() { + let line = raw_line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + + let line = line.strip_prefix("export ").unwrap_or(line); + let Some((raw_key, raw_value)) = line.split_once('=') else { + return Err(invalid_data(format!( + "invalid env line {} in {}", + index + 1, + path.display() + ))); + }; + + let key = raw_key.trim(); + if key.is_empty() { + return Err(invalid_data(format!( + "empty env key on line {} in {}", + index + 1, + path.display() + ))); + } + + entries.insert(key.to_string(), decode_value(raw_value.trim())?); + } + + Ok(entries) +} + +pub fn merge_env_file(path: &Path, updates: I) -> io::Result> +where + I: IntoIterator, + K: Into, + V: Into, +{ + let mut entries = read_env_file(path)?; + for (key, value) in updates { + entries.insert(key.into(), value.into()); + } + write_env_file(path, &entries)?; + Ok(entries) +} + +pub fn write_env_file(path: &Path, entries: &HashMap) -> io::Result<()> { + let parent = path + .parent() + .map_or_else(|| PathBuf::from("."), Path::to_path_buf); + std::fs::create_dir_all(&parent)?; + + let file_name = path + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("server.env"); + let tmp_path = parent.join(format!(".{file_name}.tmp-{}", ulid::Ulid::new())); + + let mut data = entries.iter().collect::>(); + data.sort_by(|(left, _), (right, _)| left.cmp(right)); + let contents = data + .into_iter() + .map(|(key, value)| format!("{key}={}", encode_value(value))) + .collect::>() + .join("\n"); + + std::fs::write(&tmp_path, format!("{contents}\n"))?; + set_private_permissions(&tmp_path)?; + std::fs::rename(&tmp_path, path)?; + Ok(()) +} + +fn decode_value(raw: &str) -> io::Result { + if raw.len() >= 2 && raw.starts_with('"') && raw.ends_with('"') { + return serde_json::from_str(raw).map_err(|err| invalid_data(err.to_string())); + } + + if raw.len() >= 2 && raw.starts_with('\'') && raw.ends_with('\'') { + return Ok(raw[1..raw.len() - 1].to_string()); + } + + Ok(raw.to_string()) +} + +fn encode_value(value: &str) -> String { + if value + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '_' | '-' | '.' | '/' | '+' | '=')) + { + value.to_string() + } else { + serde_json::to_string(value).expect("serializing env value should not fail") + } +} + +fn invalid_data(message: impl Into) -> io::Error { + io::Error::new(io::ErrorKind::InvalidData, message.into()) +} + +#[cfg(unix)] +fn set_private_permissions(path: &Path) -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + Ok(()) +} + +#[cfg(not(unix))] +fn set_private_permissions(_path: &Path) -> io::Result<()> { + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn read_missing_env_file_returns_empty_map() { + let dir = tempfile::tempdir().unwrap(); + let entries = read_env_file(&dir.path().join("server.env")).unwrap(); + assert!(entries.is_empty()); + } + + #[test] + fn merge_env_file_preserves_existing_keys() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("server.env"); + std::fs::write(&path, "EXISTING=value\n").unwrap(); + + let entries = merge_env_file(&path, [ + ("SESSION_SECRET", "secret"), + ("FABRO_JWT_PUBLIC_KEY", "jwt"), + ]) + .unwrap(); + + assert_eq!(entries.get("EXISTING").map(String::as_str), Some("value")); + assert_eq!( + entries.get("SESSION_SECRET").map(String::as_str), + Some("secret") + ); + assert_eq!( + entries.get("FABRO_JWT_PUBLIC_KEY").map(String::as_str), + Some("jwt") + ); + } + + #[test] + fn write_env_file_round_trips_quoted_values() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("server.env"); + let entries = HashMap::from([ + ("SESSION_SECRET".to_string(), "abc 123".to_string()), + ( + "GITHUB_APP_PRIVATE_KEY".to_string(), + "-----BEGIN KEY-----\nabc\n-----END KEY-----".to_string(), + ), + ]); + + write_env_file(&path, &entries).unwrap(); + + let reloaded = read_env_file(&path).unwrap(); + assert_eq!(reloaded, entries); + } +} diff --git a/lib/crates/fabro-config/src/lib.rs b/lib/crates/fabro-config/src/lib.rs index 2e8f40e77..6431800c9 100644 --- a/lib/crates/fabro-config/src/lib.rs +++ b/lib/crates/fabro-config/src/lib.rs @@ -1,6 +1,7 @@ extern crate self as fabro_config; pub mod effective_settings; +pub mod envfile; pub mod error; pub mod home; pub mod legacy_env; diff --git a/lib/crates/fabro-config/src/storage.rs b/lib/crates/fabro-config/src/storage.rs index 8088a4561..7c999f8f6 100644 --- a/lib/crates/fabro-config/src/storage.rs +++ b/lib/crates/fabro-config/src/storage.rs @@ -85,6 +85,11 @@ impl ServerState { pub fn log_path(&self) -> PathBuf { self.root.join("logs").join("server.log") } + + #[must_use] + pub fn env_path(&self) -> PathBuf { + self.root.join("server.env") + } } impl RunScratch { @@ -170,6 +175,10 @@ mod tests { storage.server_state().log_path(), std::path::Path::new("/tmp/fabro-data/logs/server.log") ); + assert_eq!( + storage.server_state().env_path(), + std::path::Path::new("/tmp/fabro-data/server.env") + ); } #[test] diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index d45c14fcd..c006e2a1c 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -32,6 +32,7 @@ fabro-types = { path = "../fabro-types" } fabro-util = { path = "../fabro-util" } fabro-api = { path = "../fabro-api" } fabro-store = { path = "../fabro-store" } +fabro-vault = { path = "../fabro-vault" } fabro-http.workspace = true chrono.workspace = true futures-util.workspace = true diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 5cd1d35a9..f96fc8173 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -196,16 +196,16 @@ pub async fn run_all(state: &AppState) -> DiagnosticsReport { } async fn check_llm_providers(state: &AppState) -> CheckResult { - let configured: Vec = Provider::ALL - .iter() - .copied() - .filter(|provider| { - provider - .api_key_env_vars() - .iter() - .any(|name| state.secret_or_env(name).is_some()) - }) - .collect(); + let mut configured = Vec::new(); + for provider in Provider::ALL { + if state + .provider_credentials + .has_any(provider.api_key_env_vars()) + .await + { + configured.push(*provider); + } + } if configured.is_empty() { return CheckResult { @@ -411,10 +411,10 @@ async fn check_github_app(state: &AppState) -> CheckResult { .slug .as_ref() .map(InterpString::as_source); - let private_key_raw = state.secret_or_env("GITHUB_APP_PRIVATE_KEY"); + let private_key_raw = state.server_secret("GITHUB_APP_PRIVATE_KEY"); let client_id = settings.integrations.github.client_id.is_some(); - let client_secret = state.secret_or_env("GITHUB_APP_CLIENT_SECRET").is_some(); - let webhook_secret = state.secret_or_env("GITHUB_APP_WEBHOOK_SECRET").is_some(); + let client_secret = state.server_secret("GITHUB_APP_CLIENT_SECRET").is_some(); + let webhook_secret = state.server_secret("GITHUB_APP_WEBHOOK_SECRET").is_some(); if app_id.is_none() && private_key_raw.is_none() @@ -513,7 +513,7 @@ async fn check_github_app(state: &AppState) -> CheckResult { } fn check_sandbox(state: &AppState) -> CheckResult { - if state.secret_or_env("DAYTONA_API_KEY").is_some() { + if state.vault_or_env("DAYTONA_API_KEY").is_some() { CheckResult { name: "Sandbox".to_string(), status: CheckStatus::Pass, @@ -533,7 +533,7 @@ fn check_sandbox(state: &AppState) -> CheckResult { } async fn check_brave_search(state: &AppState) -> CheckResult { - let Some(api_key) = state.secret_or_env("BRAVE_SEARCH_API_KEY") else { + let Some(api_key) = state.vault_or_env("BRAVE_SEARCH_API_KEY") else { return CheckResult { name: "Brave Search".to_string(), status: CheckStatus::Warning, @@ -671,7 +671,7 @@ fn check_crypto(state: &AppState) -> CheckResult { } if has_jwt { - match state.secret_or_env("FABRO_JWT_PUBLIC_KEY") { + match state.server_secret("FABRO_JWT_PUBLIC_KEY") { Some(raw) => { if let Err(err) = decode_pem_value("FABRO_JWT_PUBLIC_KEY", &raw).and_then(|pem| { jsonwebtoken::DecodingKey::from_ed_pem(pem.as_bytes()) @@ -685,7 +685,7 @@ fn check_crypto(state: &AppState) -> CheckResult { } } - if let Some(raw) = state.secret_or_env("FABRO_JWT_PRIVATE_KEY") { + 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(|_| ()) @@ -695,7 +695,7 @@ fn check_crypto(state: &AppState) -> CheckResult { } } - if let Some(secret) = state.secret_or_env("SESSION_SECRET") { + if let Some(secret) = state.server_secret("SESSION_SECRET") { if let Err(err) = validate_session_secret(&secret) { errors.push(err); } diff --git a/lib/crates/fabro-server/src/error.rs b/lib/crates/fabro-server/src/error.rs index dd855713b..d2f849c07 100644 --- a/lib/crates/fabro-server/src/error.rs +++ b/lib/crates/fabro-server/src/error.rs @@ -1,10 +1,9 @@ use axum::Json; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; +use fabro_vault::Error as VaultError; use serde::Serialize; -use crate::secret_store::SecretStoreError; - #[derive(Debug, thiserror::Error)] pub enum Error { #[error(transparent)] @@ -23,7 +22,7 @@ pub enum Error { Config(#[from] fabro_config::Error), #[error(transparent)] - SecretStore(#[from] SecretStoreError), + Vault(#[from] VaultError), #[error("bad request: {0}")] BadRequest(String), @@ -113,9 +112,7 @@ impl From for ApiError { Error::Llm(err) => Self::new(StatusCode::BAD_GATEWAY, err.to_string()), Error::Store(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), Error::Config(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), - Error::SecretStore(err) => { - Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()) - } + Error::Vault(err) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()), Error::Internal(msg) => Self::new(StatusCode::INTERNAL_SERVER_ERROR, msg), } } diff --git a/lib/crates/fabro-server/src/jwt_auth.rs b/lib/crates/fabro-server/src/jwt_auth.rs index 1a1c983ab..e03241dc4 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -166,7 +166,7 @@ where anyhow!( "Fabro server refuses to start: [server.auth.api.jwt] is enabled but \ FABRO_JWT_PUBLIC_KEY is not set. Provide an Ed25519 public key in PEM format \ - (or base64-encoded PEM) for JWT authentication." + (or base64-encoded PEM) via process env or server.env for JWT authentication." ) })?; let pem = decode_pem_env("FABRO_JWT_PUBLIC_KEY", &raw)?; @@ -198,9 +198,9 @@ where "Fabro server refuses to start: no authentication strategies are configured.\n\ \n\ Configure at least one of the following in `[server.auth]`:\n\ - - `[server.auth.api.jwt]` (requires `FABRO_JWT_PUBLIC_KEY` env)\n\ + - `[server.auth.api.jwt]` (requires `FABRO_JWT_PUBLIC_KEY` in process env or server.env)\n\ - `[server.auth.api.mtls]` (requires `[server.listen.tls]` cert/key/ca)\n\ - - `SESSION_SECRET` env (enables cookie-based web auth)\n\ + - `SESSION_SECRET` in process env or server.env (enables cookie-based web auth)\n\ \n\ Or set `{FABRO_LOCAL_NO_AUTH_ENV}=1` to explicitly opt in to \ unauthenticated local daemon access." diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 8fb8fdf72..6a755feea 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -11,9 +11,9 @@ pub mod error; pub mod github_webhooks; pub mod jwt_auth; mod run_manifest; -pub mod secret_store; pub mod serve; pub mod server; +mod server_secrets; mod settings_view; pub mod static_files; pub mod tls; diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index ec2a3c7cc..6e718a0bd 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -14,6 +14,7 @@ use fabro_types::settings::{ ServerSettings as ResolvedServerSettings, SettingsLayer, }; use fabro_util::terminal::Styles; +use fabro_vault::Vault; use object_store::ObjectStore; use object_store::aws::AmazonS3Builder; use object_store::local::LocalFileSystem; @@ -26,11 +27,11 @@ use tracing::{error, info, warn}; use crate::bind::{self, Bind, BindRequest}; use crate::github_webhooks::WebhookManager; use crate::jwt_auth::{AuthMode, AuthStrategy, resolve_auth_mode_with_lookup}; -use crate::secret_store::SecretStore; use crate::server::{ RouterOptions, build_app_state_with_path, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, spawn_scheduler, }; +use crate::server_secrets::ServerSecrets; use crate::tls::{ClientAuth, build_rustls_config, serve_tls_with_shutdown}; const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; @@ -265,19 +266,19 @@ where None => resolve_interp_path(&disk_server_settings.storage.root)?, }; let storage = Storage::new(&data_dir); - let secret_store_path = storage.secrets_path(); - let secret_store = SecretStore::load(secret_store_path.clone())?; - let secret_snapshot = secret_store.snapshot(); + let vault_path = storage.secrets_path(); + let vault = Vault::load(vault_path.clone())?; + let vault_snapshot = vault.snapshot(); + let server_secrets = ServerSecrets::load(storage.server_state().env_path())?; // Resolve dry-run mode (same pattern as run.rs) let dry_run_mode = if args.dry_run { true } else { match LlmClient::from_lookup(|name| { - secret_snapshot - .get(name) - .cloned() - .or_else(|| std::env::var(name).ok()) + std::env::var(name) + .ok() + .or_else(|| vault_snapshot.get(name).cloned()) }) .await { @@ -306,10 +307,7 @@ where std::fs::create_dir_all(&data_dir)?; let (auth_mode, client_auth, max_concurrent_runs) = { let auth_mode = resolve_auth_mode_with_lookup(&resolved_server_settings, |name| { - secret_snapshot - .get(name) - .cloned() - .or_else(|| std::env::var(name).ok()) + server_secrets.get(name) })?; let tls_present = matches!( resolved_server_settings.listen, @@ -339,7 +337,7 @@ where max_concurrent_runs, store, artifact_store, - secret_store_path, + vault_path, active_config_path, matches!(&auth_mode, AuthMode::Disabled), )?; @@ -376,10 +374,7 @@ where .transpose()?; match webhook_app_id { Some(app_id) => { - let secret = secret_snapshot - .get("GITHUB_APP_WEBHOOK_SECRET") - .cloned() - .or_else(|| std::env::var("GITHUB_APP_WEBHOOK_SECRET").ok()); + let secret = server_secrets.get("GITHUB_APP_WEBHOOK_SECRET"); let github_app = state .github_credentials(&resolved_server_settings.integrations.github) .await diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 28b8fc5db..04c0837c3 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -73,6 +73,7 @@ use fabro_types::{ }; use fabro_util::redact::redact_jsonl_line; use fabro_util::version::FABRO_VERSION; +use fabro_vault::{Error as VaultError, SecretType, Vault}; use fabro_workflow::Error as WorkflowError; use fabro_workflow::artifact_upload::ArtifactSink; use fabro_workflow::event::{self as workflow_event, Emitter}; @@ -112,7 +113,7 @@ use crate::error::ApiError; use crate::jwt_auth::{ AuthMode, AuthenticatedService, AuthenticatedSubject, authenticate_service_parts, }; -use crate::secret_store::{SecretStore, SecretStoreError, SecretType as StoreSecretType}; +use crate::server_secrets::{ProviderCredentials, ServerSecrets}; use crate::{demo, diagnostics, run_manifest, settings_view, static_files, web_auth}; pub fn default_page_limit() -> u32 { @@ -516,15 +517,17 @@ pub struct AppState { scheduler_notify: Notify, global_event_tx: broadcast::Sender, - pub(crate) secret_store: AsyncRwLock, - pub(crate) settings: Arc>, - pub(crate) server_settings: RwLock>, - pub(crate) config_path: PathBuf, - pub(crate) local_daemon_mode: bool, - shutting_down: AtomicBool, - registry_factory_override: Option>, - slack_service: Option>, - slack_started: AtomicBool, + pub(crate) vault: Arc>, + pub(crate) server_secrets: ServerSecrets, + pub(crate) provider_credentials: ProviderCredentials, + pub(crate) settings: Arc>, + pub(crate) server_settings: RwLock>, + pub(crate) config_path: PathBuf, + pub(crate) local_daemon_mode: bool, + shutting_down: AtomicBool, + registry_factory_override: Option>, + slack_service: Option>, + slack_started: AtomicBool, } fn nonzero_i64(value: i64) -> Option { @@ -594,34 +597,24 @@ impl AppState { } pub(crate) async fn build_llm_client(&self) -> Result { - let snapshot = self.secret_store.read().await.snapshot(); - LlmClient::from_lookup(|name| { - snapshot - .get(name) - .cloned() - .or_else(|| std::env::var(name).ok()) - }) - .await - .map_err(|err| err.to_string()) + self.provider_credentials.build_llm_client().await } - pub(crate) fn secret_or_env(&self, name: &str) -> Option { - self.secret_store - .try_read() - .ok() - .and_then(|store| store.get(name).map(str::to_string)) - .or_else(|| std::env::var(name).ok()) + pub(crate) fn vault_or_env(&self, name: &str) -> Option { + std::env::var(name).ok().or_else(|| { + self.vault + .try_read() + .ok() + .and_then(|vault| vault.get(name).map(str::to_string)) + }) + } + + pub(crate) fn server_secret(&self, name: &str) -> Option { + self.server_secrets.get(name) } pub(crate) async fn session_key(&self) -> Option { - let secret = self - .secret_store - .read() - .await - .get("SESSION_SECRET") - .map(str::to_string); - secret - .or_else(|| std::env::var("SESSION_SECRET").ok()) + self.server_secret("SESSION_SECRET") .map(|value| Key::derive_from(value.as_bytes())) } @@ -634,13 +627,7 @@ impl AppState { let Some(app_id) = settings.app_id.as_ref().map(InterpString::as_source) else { return Ok(None); }; - let raw = self - .secret_store - .read() - .await - .get("GITHUB_APP_PRIVATE_KEY") - .map(str::to_string) - .or_else(|| std::env::var("GITHUB_APP_PRIVATE_KEY").ok()); + let raw = self.server_secret("GITHUB_APP_PRIVATE_KEY"); let Some(raw) = raw else { return Ok(None); }; @@ -654,10 +641,8 @@ impl AppState { } GithubIntegrationStrategy::GhCli => { let token = self - .secret_store - .read() - .await - .get("GITHUB_CLI_TOKEN") + .vault_or_env("GITHUB_CLI_TOKEN") + .as_deref() .map(str::trim) .filter(|token| !token.is_empty()) .map(str::to_string); @@ -1622,14 +1607,14 @@ where } async fn list_secrets(_auth: AuthenticatedService, State(state): State>) -> Response { - let data = state.secret_store.read().await.list(); + let data = state.vault.read().await.list(); (StatusCode::OK, Json(serde_json::json!({ "data": data }))).into_response() } -fn secret_type_from_api(secret_type: ApiSecretType) -> StoreSecretType { +fn secret_type_from_api(secret_type: ApiSecretType) -> SecretType { match secret_type { - ApiSecretType::Environment => StoreSecretType::Environment, - ApiSecretType::File => StoreSecretType::File, + ApiSecretType::Environment => SecretType::Environment, + ApiSecretType::File => SecretType::File, } } @@ -1644,23 +1629,23 @@ async fn create_secret( let description = body.description; let state_for_write = Arc::clone(&state); let result = spawn_blocking(move || { - let mut store = state_for_write.secret_store.blocking_write(); - store.set(&name, &value, secret_type, description.as_deref()) + let mut vault = state_for_write.vault.blocking_write(); + vault.set(&name, &value, secret_type, description.as_deref()) }) .await; match result { Ok(Ok(meta)) => (StatusCode::OK, Json(meta)).into_response(), - Ok(Err(SecretStoreError::InvalidName(_))) => { + Ok(Err(VaultError::InvalidName(_))) => { ApiError::bad_request("invalid secret name").into_response() } - Ok(Err(SecretStoreError::Io(err))) => { + Ok(Err(VaultError::Io(err))) => { ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() } - Ok(Err(SecretStoreError::Serde(err))) => { + Ok(Err(VaultError::Serde(err))) => { ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() } - Ok(Err(SecretStoreError::NotFound(_))) => ApiError::new( + Ok(Err(VaultError::NotFound(_))) => ApiError::new( StatusCode::INTERNAL_SERVER_ERROR, "secret unexpectedly missing", ) @@ -1681,24 +1666,24 @@ async fn delete_secret_by_name( let name = body.name; let state_for_write = Arc::clone(&state); let result = spawn_blocking(move || { - let mut store = state_for_write.secret_store.blocking_write(); - store.remove(&name) + let mut vault = state_for_write.vault.blocking_write(); + vault.remove(&name) }) .await; match result { Ok(Ok(())) => StatusCode::NO_CONTENT.into_response(), - Ok(Err(SecretStoreError::InvalidName(_))) => { + Ok(Err(VaultError::InvalidName(_))) => { ApiError::bad_request("invalid secret name").into_response() } - Ok(Err(SecretStoreError::NotFound(name))) => { + Ok(Err(VaultError::NotFound(name))) => { ApiError::new(StatusCode::NOT_FOUND, format!("secret not found: {name}")) .into_response() } - Ok(Err(SecretStoreError::Io(err))) => { + Ok(Err(VaultError::Io(err))) => { ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() } - Ok(Err(SecretStoreError::Serde(err))) => { + Ok(Err(VaultError::Serde(err))) => { ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() } Err(err) => ApiError::new( @@ -2199,11 +2184,17 @@ pub(crate) fn build_app_state_with_path( max_concurrent_runs: usize, store: Arc, artifact_store: ArtifactStore, - secret_store_path: PathBuf, + vault_path: PathBuf, config_path: PathBuf, local_daemon_mode: bool, ) -> anyhow::Result> { - let secret_store = SecretStore::load(secret_store_path)?; + let vault = Arc::new(AsyncRwLock::new(Vault::load(vault_path.clone())?)); + let server_env_path = vault_path.parent().map_or_else( + || PathBuf::from("server.env"), + |parent| parent.join("server.env"), + ); + let server_secrets = ServerSecrets::load(server_env_path)?; + let provider_credentials = ProviderCredentials::new(Arc::clone(&vault)); let (global_event_tx, _) = broadcast::channel(4096); let resolved_server_settings = { let settings = settings.read().expect("settings lock poisoned"); @@ -2251,7 +2242,9 @@ pub(crate) fn build_app_state_with_path( max_concurrent_runs, scheduler_notify: Notify::new(), global_event_tx, - secret_store: AsyncRwLock::new(secret_store), + vault, + server_secrets, + provider_credentials, settings, server_settings: RwLock::new(resolved_server_settings), config_path, @@ -6243,9 +6236,9 @@ mod tests { assert_eq!(body["type"], "file"); assert_eq!(body["description"], "Test certificate"); - let store = state.secret_store.read().await; - assert!(!store.snapshot().contains_key("/tmp/test.pem")); - assert_eq!(store.file_secrets(), vec![( + let vault = state.vault.read().await; + assert!(!vault.snapshot().contains_key("/tmp/test.pem")); + assert_eq!(vault.file_secrets(), vec![( "/tmp/test.pem".to_string(), "pem-data".to_string() )]); @@ -6286,7 +6279,51 @@ mod tests { let delete_response = app.oneshot(delete_req).await.unwrap(); assert_eq!(delete_response.status(), StatusCode::NO_CONTENT); - assert!(state.secret_store.read().await.list().is_empty()); + assert!(state.vault.read().await.list().is_empty()); + } + + #[test] + fn server_secrets_resolve_process_env_before_server_env() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("server.env"), + "SESSION_SECRET=file-value\nGITHUB_APP_CLIENT_SECRET=file-client\n", + ) + .unwrap(); + + let secrets = + ServerSecrets::with_env_lookup(dir.path().join("server.env"), |name| match name { + "SESSION_SECRET" => Some("env-value".to_string()), + _ => None, + }) + .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 provider_credentials_resolve_process_env_before_vault() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault + .set("OPENAI_API_KEY", "vault-key", SecretType::Environment, None) + .unwrap(); + + let provider_credentials = + ProviderCredentials::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), |name| { + match name { + "OPENAI_API_KEY" => Some("env-key".to_string()), + _ => None, + } + }); + + let runtime = tokio::runtime::Runtime::new().unwrap(); + let resolved = runtime.block_on(provider_credentials.get("OPENAI_API_KEY")); + assert_eq!(resolved.as_deref(), Some("env-key")); } #[tokio::test] diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs new file mode 100644 index 000000000..23b20c4e3 --- /dev/null +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -0,0 +1,145 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; + +use fabro_config::envfile; +use fabro_llm::client::Client as LlmClient; +use fabro_vault::Vault; +use tokio::sync::RwLock as AsyncRwLock; + +type EnvLookup = Arc Option + Send + Sync>; + +const PROVIDER_LOOKUP_NAMES: &[&str] = &[ + "ANTHROPIC_API_KEY", + "ANTHROPIC_BASE_URL", + "OPENAI_API_KEY", + "CHATGPT_ACCOUNT_ID", + "OPENAI_BASE_URL", + "OPENAI_ORG_ID", + "OPENAI_PROJECT_ID", + "GEMINI_API_KEY", + "GOOGLE_API_KEY", + "GEMINI_BASE_URL", + "KIMI_API_KEY", + "ZAI_API_KEY", + "MINIMAX_API_KEY", + "INCEPTION_API_KEY", +]; + +#[derive(Debug, thiserror::Error)] +pub(crate) enum Error { + #[error(transparent)] + Io(#[from] std::io::Error), +} + +pub(crate) struct ServerSecrets { + path: PathBuf, + 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, + { + Ok(Self { + 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()) + } + + pub(crate) fn persist_updates(&mut self, updates: I) -> Result<(), Error> + where + I: IntoIterator, + K: Into, + V: Into, + { + self.file_entries = envfile::merge_env_file(&self.path, updates)?; + Ok(()) + } +} + +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( + "file_entries", + &self.file_entries.keys().collect::>(), + ) + .finish() + } +} + +#[derive(Clone)] +pub(crate) struct ProviderCredentials { + vault: Arc>, + env_lookup: EnvLookup, +} + +impl ProviderCredentials { + pub(crate) fn new(vault: Arc>) -> Self { + Self::with_env_lookup(vault, |name| std::env::var(name).ok()) + } + + pub(crate) fn with_env_lookup(vault: Arc>, env_lookup: F) -> Self + where + F: Fn(&str) -> Option + Send + Sync + 'static, + { + Self { + vault, + env_lookup: Arc::new(env_lookup), + } + } + + pub(crate) async fn get(&self, name: &str) -> Option { + let env_value = (self.env_lookup)(name); + if env_value.is_some() { + return env_value; + } + + self.vault.read().await.get(name).map(str::to_string) + } + + pub(crate) async fn has_any(&self, names: &[&str]) -> bool { + for name in names { + if self.get(name).await.is_some() { + return true; + } + } + false + } + + pub(crate) async fn build_llm_client(&self) -> Result { + let vault_snapshot = self.vault.read().await.snapshot(); + let lookup = PROVIDER_LOOKUP_NAMES + .iter() + .filter_map(|name| { + (self.env_lookup)(name) + .or_else(|| vault_snapshot.get(*name).cloned()) + .map(|value| ((*name).to_string(), value)) + }) + .collect::>(); + + LlmClient::from_lookup(|name| lookup.get(name).cloned()) + .await + .map_err(|err| err.to_string()) + } +} + +impl std::fmt::Debug for ProviderCredentials { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ProviderCredentials") + .finish_non_exhaustive() + } +} diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 3695c9b9d..28a94e8bb 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -6,15 +6,18 @@ use axum::http::{HeaderMap, HeaderValue, StatusCode, header}; use axum::response::{IntoResponse, Redirect, Response}; use axum::routing::{get, post}; use axum::{Json, Router}; +use base64::Engine as _; +use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use cookie::time::Duration; use cookie::{Cookie, CookieJar, Expiration, Key, SameSite}; +use fabro_config::Storage; use fabro_types::settings::{InterpString, SettingsLayer}; use serde::{Deserialize, Serialize}; use serde_json::json; use tracing::{debug, error, info, warn}; -use crate::secret_store::SecretType; use crate::server::AppState; +use crate::server_secrets::ServerSecrets; pub const SESSION_COOKIE_NAME: &str = "__fabro_session"; const OAUTH_STATE_COOKIE_NAME: &str = "fabro_oauth_state"; @@ -269,7 +272,7 @@ async fn callback_github( ); } }; - let Some(client_secret) = state.secret_or_env("GITHUB_APP_CLIENT_SECRET") else { + let Some(client_secret) = state.server_secret("GITHUB_APP_CLIENT_SECRET") else { error!("OAuth callback failed: GITHUB_APP_CLIENT_SECRET not configured"); return json_response( StatusCode::CONFLICT, @@ -643,32 +646,41 @@ async fn setup_register( ); } - let session_secret = hex::encode(rand::random::<[u8; 32]>()); let mut secret_updates = vec![ - ("SESSION_SECRET", session_secret), ("GITHUB_APP_CLIENT_SECRET", data.client_secret.clone()), - ("GITHUB_APP_PRIVATE_KEY", data.pem.clone()), + ( + "GITHUB_APP_PRIVATE_KEY", + BASE64_STANDARD.encode(data.pem.as_bytes()), + ), ]; if let Some(ref webhook_secret) = data.webhook_secret { secret_updates.push(("GITHUB_APP_WEBHOOK_SECRET", webhook_secret.clone())); } - { - let mut store = state.secret_store.write().await; - for (name, value) in secret_updates { - if let Err(err) = store.set(name, &value, SecretType::Environment, None) { - error!(error = %err, secret = name, "Setup register failed: could not save secret"); - return json_response( - StatusCode::INTERNAL_SERVER_ERROR, - json!({"error": format!("Failed to save secret {name}: {err}")}), - ); - } + let server_env_path = Storage::new(state.server_storage_dir()) + .server_state() + .env_path(); + let mut server_secrets = match ServerSecrets::load(server_env_path.clone()) { + Ok(server_secrets) => server_secrets, + Err(err) => { + error!(error = %err, path = %server_env_path.display(), "Setup register failed: could not load server env"); + return json_response( + StatusCode::INTERNAL_SERVER_ERROR, + json!({"error": format!("Failed to load server env: {err}")}), + ); } + }; + if let Err(err) = server_secrets.persist_updates(secret_updates) { + error!(error = %err, path = %server_env_path.display(), "Setup register failed: could not write server env"); + return json_response( + StatusCode::INTERNAL_SERVER_ERROR, + json!({"error": format!("Failed to write server env: {err}")}), + ); } // Re-parse the freshly-written settings file and swap it into the - // in-memory state so subsequent OAuth requests see the new GitHub - // App credentials without a server restart. + // in-memory state so the non-secret GitHub App config becomes visible + // immediately. Secret material remains restart-bound through server.env. match state.reload_settings_from_disk() { Ok(()) => {} Err(err) => { @@ -681,7 +693,7 @@ async fn setup_register( } info!(slug = %data.slug, app_id = %data.id, "GitHub App registered successfully"); - Json(json!({"ok": true})).into_response() + Json(json!({"ok": true, "restart_required": true})).into_response() } /// Walk dotted `path` into `doc`, creating missing intermediate tables, diff --git a/lib/crates/fabro-vault/Cargo.toml b/lib/crates/fabro-vault/Cargo.toml new file mode 100644 index 000000000..29602442a --- /dev/null +++ b/lib/crates/fabro-vault/Cargo.toml @@ -0,0 +1,22 @@ +[package] +name = "fabro-vault" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Workflow-visible secret vault for Fabro" + +[lib] +doctest = false + +[lints] +workspace = true + +[dependencies] +chrono.workspace = true +serde.workspace = true +serde_json.workspace = true +ulid.workspace = true + +[dev-dependencies] +tempfile = "3" diff --git a/lib/crates/fabro-server/src/secret_store.rs b/lib/crates/fabro-vault/src/lib.rs similarity index 50% rename from lib/crates/fabro-server/src/secret_store.rs rename to lib/crates/fabro-vault/src/lib.rs index b599dac77..3c137c5ab 100644 --- a/lib/crates/fabro-server/src/secret_store.rs +++ b/lib/crates/fabro-vault/src/lib.rs @@ -33,14 +33,14 @@ pub struct SecretMetadata { } #[derive(Debug)] -pub enum SecretStoreError { +pub enum Error { InvalidName(String), NotFound(String), Io(std::io::Error), Serde(serde_json::Error), } -impl fmt::Display for SecretStoreError { +impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::InvalidName(name) => write!(f, "invalid secret name: {name}"), @@ -51,28 +51,28 @@ impl fmt::Display for SecretStoreError { } } -impl std::error::Error for SecretStoreError {} +impl std::error::Error for Error {} -impl From for SecretStoreError { +impl From for Error { fn from(value: std::io::Error) -> Self { Self::Io(value) } } -impl From for SecretStoreError { +impl From for Error { fn from(value: serde_json::Error) -> Self { Self::Serde(value) } } #[derive(Debug)] -pub struct SecretStore { +pub struct Vault { path: PathBuf, entries: HashMap, } -impl SecretStore { - pub fn load(path: PathBuf) -> Result { +impl Vault { + pub fn load(path: PathBuf) -> Result { let entries = match std::fs::read_to_string(&path) { Ok(contents) => serde_json::from_str(&contents)?, Err(err) if err.kind() == std::io::ErrorKind::NotFound => HashMap::new(), @@ -88,7 +88,7 @@ impl SecretStore { value: &str, secret_type: SecretType, description: Option<&str>, - ) -> Result { + ) -> Result { Self::validate_name(name, secret_type)?; let now = chrono::Utc::now().to_rfc3339(); @@ -122,9 +122,9 @@ impl SecretStore { }) } - pub fn remove(&mut self, name: &str) -> Result<(), SecretStoreError> { + pub fn remove(&mut self, name: &str) -> Result<(), Error> { if self.entries.remove(name).is_none() { - return Err(SecretStoreError::NotFound(name.to_string())); + return Err(Error::NotFound(name.to_string())); } self.write_atomic()?; Ok(()) @@ -169,48 +169,48 @@ impl SecretStore { data } - pub fn validate_name(name: &str, secret_type: SecretType) -> Result<(), SecretStoreError> { + pub fn validate_name(name: &str, secret_type: SecretType) -> Result<(), Error> { match secret_type { SecretType::Environment => Self::validate_env_name(name), SecretType::File => Self::validate_file_name(name), } } - fn validate_env_name(name: &str) -> Result<(), SecretStoreError> { + fn validate_env_name(name: &str) -> Result<(), Error> { let mut chars = name.chars(); match chars.next() { Some(first) if first.is_ascii_alphabetic() || first == '_' => {} - _ => return Err(SecretStoreError::InvalidName(name.to_string())), + _ => return Err(Error::InvalidName(name.to_string())), } if chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_') { Ok(()) } else { - Err(SecretStoreError::InvalidName(name.to_string())) + Err(Error::InvalidName(name.to_string())) } } - fn validate_file_name(name: &str) -> Result<(), SecretStoreError> { + fn validate_file_name(name: &str) -> Result<(), Error> { if !name.starts_with('/') || name.ends_with('/') || name.contains('\0') { - return Err(SecretStoreError::InvalidName(name.to_string())); + return Err(Error::InvalidName(name.to_string())); } let path = Path::new(name); if !path.is_absolute() { - return Err(SecretStoreError::InvalidName(name.to_string())); + return Err(Error::InvalidName(name.to_string())); } if path .components() .any(|component| matches!(component, Component::ParentDir)) { - return Err(SecretStoreError::InvalidName(name.to_string())); + return Err(Error::InvalidName(name.to_string())); } Ok(()) } - fn write_atomic(&self) -> Result<(), SecretStoreError> { + fn write_atomic(&self) -> Result<(), Error> { let parent = self .path .parent() @@ -232,7 +232,7 @@ impl SecretStore { } #[cfg(unix)] -fn set_private_permissions(path: &Path) -> Result<(), SecretStoreError> { +fn set_private_permissions(path: &Path) -> Result<(), Error> { use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; @@ -240,7 +240,7 @@ fn set_private_permissions(path: &Path) -> Result<(), SecretStoreError> { } #[cfg(not(unix))] -fn set_private_permissions(_path: &Path) -> Result<(), SecretStoreError> { +fn set_private_permissions(_path: &Path) -> Result<(), Error> { Ok(()) } @@ -251,7 +251,7 @@ mod tests { #[test] fn load_missing_file_returns_empty_store() { let dir = tempfile::tempdir().unwrap(); - let store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + let store = Vault::load(dir.path().join("secrets.json")).unwrap(); assert!(store.list().is_empty()); } @@ -259,7 +259,7 @@ mod tests { fn set_creates_entry_and_writes_file() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("secrets.json"); - let mut store = SecretStore::load(path.clone()).unwrap(); + let mut store = Vault::load(path.clone()).unwrap(); let meta = store .set("OPENAI_API_KEY", "secret", SecretType::Environment, None) @@ -267,33 +267,31 @@ mod tests { assert_eq!(meta.name, "OPENAI_API_KEY"); assert_eq!(meta.secret_type, SecretType::Environment); - assert_eq!(meta.description, None); assert_eq!(store.get("OPENAI_API_KEY"), Some("secret")); assert!(path.exists()); } #[test] - fn set_existing_key_preserves_created_at() { + fn set_updates_existing_entry() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("secrets.json"); - let mut store = SecretStore::load(path).unwrap(); + let mut store = Vault::load(path).unwrap(); - let first = store + store .set("OPENAI_API_KEY", "first", SecretType::Environment, None) .unwrap(); - let second = store + store .set("OPENAI_API_KEY", "second", SecretType::Environment, None) .unwrap(); - assert_eq!(first.created_at, second.created_at); assert_eq!(store.get("OPENAI_API_KEY"), Some("second")); } #[test] - fn remove_deletes_entry_and_writes_file() { + fn remove_deletes_entry() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("secrets.json"); - let mut store = SecretStore::load(path.clone()).unwrap(); + let mut store = Vault::load(path.clone()).unwrap(); store .set("OPENAI_API_KEY", "secret", SecretType::Environment, None) .unwrap(); @@ -301,215 +299,37 @@ mod tests { store.remove("OPENAI_API_KEY").unwrap(); assert_eq!(store.get("OPENAI_API_KEY"), None); - let written = std::fs::read_to_string(path).unwrap(); - assert_eq!(written.trim(), "{}"); } #[test] - fn remove_missing_key_returns_error() { + fn env_secret_snapshot_excludes_file_secrets() { let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - let error = store.remove("MISSING").unwrap_err(); - assert_eq!(error.to_string(), "secret not found: MISSING"); - } - - #[test] - fn list_returns_sorted_metadata_without_values() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + let mut store = Vault::load(dir.path().join("secrets.json")).unwrap(); store - .set("Z_KEY", "z", SecretType::Environment, None) + .set("OPENAI_API_KEY", "env", SecretType::Environment, None) .unwrap(); store - .set("A_KEY", "a", SecretType::Environment, None) + .set("/tmp/key.pem", "pem", SecretType::File, None) .unwrap(); - let listed = store.list(); - - assert_eq!( - listed - .iter() - .map(|item| item.name.as_str()) - .collect::>(), - vec!["A_KEY", "Z_KEY"] - ); + let snapshot = store.snapshot(); + assert_eq!(snapshot.get("OPENAI_API_KEY"), Some(&"env".to_string())); + assert!(!snapshot.contains_key("/tmp/key.pem")); } #[test] - fn invalid_names_are_rejected() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - let error = store - .set("NOT-VALID", "secret", SecretType::Environment, None) - .unwrap_err(); - assert_eq!(error.to_string(), "invalid secret name: NOT-VALID"); - } - - #[test] - fn set_file_secret_stores_type() { + fn file_secret_listing_survives_reload() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("secrets.json"); - let mut store = SecretStore::load(path.clone()).unwrap(); - - let meta = store - .set("/root/.ssh/id_rsa", "secret", SecretType::File, None) - .unwrap(); - - assert_eq!(meta.name, "/root/.ssh/id_rsa"); - assert_eq!(meta.secret_type, SecretType::File); - - let reloaded = SecretStore::load(path).unwrap(); - let listed = reloaded.list(); - assert_eq!(listed.len(), 1); - assert_eq!(listed[0].secret_type, SecretType::File); - } - - #[test] - fn set_file_secret_validates_absolute_path() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - - let error = store - .set("relative/path", "secret", SecretType::File, None) - .unwrap_err(); - - assert_eq!(error.to_string(), "invalid secret name: relative/path"); - } - - #[test] - fn set_file_secret_rejects_traversal() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - - let error = store - .set("/root/../id_rsa", "secret", SecretType::File, None) - .unwrap_err(); - - assert_eq!(error.to_string(), "invalid secret name: /root/../id_rsa"); - } - - #[test] - fn set_env_secret_rejects_path_names() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - - let error = store - .set("/foo/bar", "secret", SecretType::Environment, None) - .unwrap_err(); - - assert_eq!(error.to_string(), "invalid secret name: /foo/bar"); - } - - #[test] - fn snapshot_excludes_file_secrets() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); + let mut store = Vault::load(path.clone()).unwrap(); store - .set("OPENAI_API_KEY", "env", SecretType::Environment, None) - .unwrap(); - store - .set("/tmp/test.pem", "file", SecretType::File, None) + .set("/tmp/key.pem", "pem", SecretType::File, None) .unwrap(); - let snapshot = store.snapshot(); - - assert_eq!(snapshot.get("OPENAI_API_KEY"), Some(&"env".to_string())); - assert!(!snapshot.contains_key("/tmp/test.pem")); - } - - #[test] - fn snapshot_includes_only_env_secrets() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - store - .set("ANTHROPIC_API_KEY", "a", SecretType::Environment, None) - .unwrap(); - store - .set("OPENAI_API_KEY", "b", SecretType::Environment, None) - .unwrap(); - - let snapshot = store.snapshot(); - - assert_eq!(snapshot.len(), 2); - assert_eq!(snapshot.get("ANTHROPIC_API_KEY"), Some(&"a".to_string())); - assert_eq!(snapshot.get("OPENAI_API_KEY"), Some(&"b".to_string())); - } - - #[test] - fn file_secrets_returns_only_files() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - store - .set("OPENAI_API_KEY", "env", SecretType::Environment, None) - .unwrap(); - store - .set("/tmp/test.pem", "file", SecretType::File, None) - .unwrap(); - - let files = store.file_secrets(); - - assert_eq!(files, vec![( - "/tmp/test.pem".to_string(), - "file".to_string() + let reloaded = Vault::load(path).unwrap(); + assert_eq!(reloaded.file_secrets(), vec![( + "/tmp/key.pem".to_string(), + "pem".to_string() )]); } - - #[test] - fn description_round_trips() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("secrets.json"); - let mut store = SecretStore::load(path.clone()).unwrap(); - - let meta = store - .set( - "/tmp/test.pem", - "file", - SecretType::File, - Some("Test certificate"), - ) - .unwrap(); - - assert_eq!(meta.description.as_deref(), Some("Test certificate")); - - let reloaded = SecretStore::load(path).unwrap(); - let listed = reloaded.list(); - assert_eq!(listed[0].description.as_deref(), Some("Test certificate")); - } - - #[test] - fn legacy_json_defaults_to_environment() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("secrets.json"); - std::fs::write( - &path, - r#"{ - "OPENAI_API_KEY": { - "value": "secret", - "created_at": "2026-04-12T00:00:00Z", - "updated_at": "2026-04-12T00:00:00Z" - } -}"#, - ) - .unwrap(); - - let store = SecretStore::load(path).unwrap(); - let listed = store.list(); - - assert_eq!(listed.len(), 1); - assert_eq!(listed[0].secret_type, SecretType::Environment); - assert_eq!(listed[0].description, None); - } - - #[test] - fn remove_allows_file_path_names() { - let dir = tempfile::tempdir().unwrap(); - let mut store = SecretStore::load(dir.path().join("secrets.json")).unwrap(); - store - .set("/tmp/test.pem", "file", SecretType::File, None) - .unwrap(); - - store.remove("/tmp/test.pem").unwrap(); - - assert!(store.list().is_empty()); - } }