diff --git a/Cargo.lock b/Cargo.lock index 3f795180f..c66de1be7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2343,6 +2343,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "toml 0.8.23", + "tracing", ] [[package]] @@ -2705,6 +2706,7 @@ dependencies = [ "fabro-vault", "ring", "tempfile", + "tokio", "toml 0.8.23", ] @@ -3296,12 +3298,18 @@ dependencies = [ name = "fabro-vault" version = "0.302.0-nightly.1" dependencies = [ + "anyhow", "chrono", + "fabro-db", "fabro-static", "fabro-types", "serde", "serde_json", + "sqlx", "tempfile", + "thiserror 2.0.18", + "tokio", + "tracing", "ulid", ] diff --git a/docs/plans/2026-07-11-secrets-sqlite-plan.md b/docs/plans/2026-07-11-secrets-sqlite-plan.md new file mode 100644 index 000000000..fdd0e8ce6 --- /dev/null +++ b/docs/plans/2026-07-11-secrets-sqlite-plan.md @@ -0,0 +1,145 @@ +# Secrets to SQLite sketch + +## Goal + +Move optional integration secrets from `vaults/default/secrets.json` to the shared SQL database. Preserve the REST contract and secret-type behavior. Make reads and OAuth refresh safe across nodes. Keep bootstrap secrets in process env or `server.env`. + +Encryption-at-rest is out of scope. This migration preserves the current plaintext-at-rest semantics; application-level encryption will be a separate PR. + +## Boundary + +- SQL: token, OAuth, and file secrets currently owned by `fabro-vault`. +- Not SQL: `SESSION_SECRET`, dev token, database/object-store bootstrap credentials, worker bearer tokens. +- `fabro-vault` remains the owning shared crate. Replace file persistence with `SecretStore { pool }`. +- No process-wide entry cache. Fetch on each operation; allow short-lived snapshots for one run/config-resolution operation. + +## Schema + +Schema: + +```sql +CREATE TABLE secrets ( + name TEXT PRIMARY KEY NOT NULL, + secret_type TEXT NOT NULL, + value TEXT NOT NULL, + description TEXT, + revision INTEGER NOT NULL DEFAULT 1, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + + CHECK (length(name) > 0), + CHECK (secret_type IN ('token', 'oauth', 'file')), + CHECK (revision > 0), + CHECK (length(created_at) > 0), + CHECK (length(updated_at) > 0), + CHECK ( + (secret_type IN ('token', 'oauth') + AND substr(name, 1, 1) GLOB '[A-Za-z_]' + AND name NOT GLOB '*[^A-Za-z0-9_]*') + OR + (secret_type = 'file' + AND substr(name, 1, 1) = '/' + AND substr(name, -1, 1) <> '/') + ) +); +``` + +Notes: + +- `revision` is internal optimistic concurrency. Ordinary API upserts increment it. OAuth refresh writes `WHERE name = ? AND revision = ?`; a loser reloads the winning credential. +- Keep timestamps as RFC 3339 text to match current SQLite tables. PostgreSQL migration can map them to `timestamptz` behind the store API. +- Keep full semantic validation in Rust and revalidate decoded database rows. SQLite checks provide defense in depth; PostgreSQL gets equivalent backend-specific constraints later. +- `value` deliberately preserves current plaintext-at-rest behavior. A follow-up encryption PR should migrate it to a versioned ciphertext envelope and introduce key provisioning/rotation without changing the caller-facing API. + +No secondary index: secret sets are small and every supported query is primary-key lookup or full sorted listing. + +## Store API + +```rust +pub struct SecretStore { /* pool */ } + +pub async fn get(&self, name: &str) -> Result, Error>; +pub async fn list(&self) -> Result, Error>; +pub async fn set( + &self, + name: &str, + value: &str, + secret_type: SecretType, + description: Option<&str>, +) -> Result; +pub async fn remove(&self, name: &str) -> Result<(), Error>; +pub async fn replace_if_revision( + &self, + expected_revision: i64, + entry: SecretReplacement<'_>, +) -> Result; +pub async fn snapshot(&self) -> Result; +``` + +- Preserve created time and existing description when an upsert omits description. +- `SecretEntry` carries internal revision; `SecretMetadata` and OpenAPI remain unchanged. +- Derive `EnumString` and `IntoStaticStr` for `SecretType`, aligned with serde/strum names. +- Typed errors: invalid name, not found, stale revision, invalid stored row, database failure, legacy import failure. Preserve sources; API handlers return curated messages. +- Never implement `Debug` for a value-bearing type by deriving it. Redact or omit values and OAuth JSON. + +## Read model and concurrency + +- Replace `Arc>` in `AppStores` with `Arc`. +- Convert `AppState::vault_secret` to async and fallible. Do not use `try_read()` or map database failure to “missing.” +- Refactor `VaultCredentialSource`/`CredentialResolver` to load an operation-scoped snapshot. Credential resolution stays internally synchronous over that snapshot; storage I/O is async at the boundary. +- OAuth refresh: load entry + revision, refresh remotely, CAS the new credential. On stale revision, reload and use the winner if valid; never overwrite a newer refresh token. +- Workflow/run creation and worker startup may take one snapshot for consistent interpolation and credential selection. A running worker does not observe secret rotation; the next request or worker sees later writes from any node. + +## Startup and legacy import + +New order: + +1. Load settings and bootstrap `ServerSecrets`. +2. Connect SQL and run schema migrations. +3. Normalize any supported legacy vault JSON shape. +4. Parse and validate the complete `secrets.json`; insert transactionally with `ON CONFLICT(name) DO NOTHING`. SQL wins. +5. Rename source to `secrets.json.imported-.bak`, preserving private permissions. +6. Move the temporary optional-`server.env` migration to target `SecretStore`; back up and clean `server.env` only after SQL contains the intended value. +7. Query required vault-only startup secrets, then resolve auth and integrations. + +Import is state-driven and retry-safe. Missing source is a no-op. Invalid JSON/name/type/OAuth payload leaves the source untouched. Logs contain paths, counts, and names only—never values or serialized rows. + +## Call-site migration + +1. Add schema, `SecretStore`, row validation, CAS, and store tests in `fabro-vault`. +2. Add legacy importer and reorder server startup before auth resolution. +3. Update secrets handlers and all server secret lookups to async/fallible access. +4. Refactor `fabro-auth` credential resolution and OAuth refresh around snapshots + CAS. +5. Update install persistence to write SQL. Make direct persistence async; preserve file rollback behavior around settings/`server.env` and use a SQL transaction for the secret batch. +6. Update CLI/local-run and worker startup to open the same database instead of loading JSON. Keep test helpers behind `test-support` boundaries. +7. Remove live JSON writes and `Storage::secrets_path()` production consumers. Retain only the legacy import path until its removal deadline. +8. Update operator docs: backup behavior, plaintext-at-rest scope, and SQLite/PostgreSQL boundary. Track application-level encryption as a separate follow-up. + +## Tests + +- Schema constraints and invalid stored-row decoding. +- CRUD parity: sorting, empty values, all types, description preservation, timestamps. +- Two independent pools observe writes immediately. +- Concurrent upsert and delete behavior. +- OAuth CAS: one winner; loser reloads without clobbering rotated refresh token. +- Redacted `Debug`/errors and no secret values in diagnostics. +- Import: absent, success, second-run no-op, SQL wins, invalid file unchanged, backup permissions, rename failure retry. +- Startup auth reads imported GitHub client secret before auth validation. +- Install, CLI local run, worker, API persistence, restart, and multi-pool integration coverage. +- Assert logs/errors/API bodies never contain fixture secret values. + +Verification: + +```text +cargo nextest run -p fabro-db -p fabro-vault -p fabro-auth +cargo nextest run -p fabro-server --features test-support secrets +cargo nextest run -p fabro-server --features test-support install +cargo nextest run -p fabro-cli --features test-support +cargo build --workspace +cargo +nightly-2026-04-14 fmt --check --all +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +``` + +## Unresolved questions + +None. diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 866c64733..b32ef10dd 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -9,7 +9,7 @@ use std::sync::{Arc, Mutex}; use anyhow::Context as _; use clap::{Args, Parser}; -use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_auth::{CredentialSource, SqlVaultCredentialSource}; use fabro_config::Storage; use fabro_config::user::default_storage_dir; use fabro_llm::Error as LlmError; @@ -23,10 +23,10 @@ use fabro_model::catalog::LlmCatalogSettings; use fabro_model::{AgentProfileKind, Catalog, ModelHandle, ProviderId}; use fabro_static::EnvVars; use fabro_util::terminal::Styles; -use fabro_vault::Vault; +use fabro_vault::SecretStore; use tokio::io::{AsyncWriteExt, stdout}; use tokio::signal; -use tokio::sync::{Mutex as AsyncMutex, RwLock as AsyncRwLock}; +use tokio::sync::Mutex as AsyncMutex; use crate::config::{ToolApprovalAdapter, ToolApprovalFn, ToolHookCallback, ToolSecrets}; use crate::error::InterruptReason; @@ -273,14 +273,12 @@ fn resolve_provider_id(catalog: &Catalog, args: &AgentArgs) -> anyhow::Result Arc { - let storage_dir = default_storage_dir(); - match Vault::load(Storage::new(storage_dir).secrets_path()) { - Ok(vault) => Arc::new(VaultCredentialSource::new(Arc::new(AsyncRwLock::new( - vault, - )))), - Err(_) => Arc::new(EnvCredentialSource::new()), - } +async fn standalone_llm_source() -> anyhow::Result> { + let storage = Storage::new(default_storage_dir()); + let store = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .context("opening the Fabro secret store")?; + Ok(Arc::new(SqlVaultCredentialSource::new(Arc::new(store)))) } fn profile_kind_for_provider( @@ -464,7 +462,7 @@ pub async fn run_with_args( args: AgentArgs, mcp_servers: Vec, ) -> anyhow::Result<()> { - let llm_source = standalone_llm_source(); + let llm_source = standalone_llm_source().await?; let catalog = Arc::new(Catalog::from_builtin().context("failed to build standalone agent LLM catalog")?); run_with_args_and_source_and_catalog(args, llm_source, mcp_servers, catalog).await diff --git a/lib/crates/fabro-auth/Cargo.toml b/lib/crates/fabro-auth/Cargo.toml index 992aa66a2..90ad9891e 100644 --- a/lib/crates/fabro-auth/Cargo.toml +++ b/lib/crates/fabro-auth/Cargo.toml @@ -25,6 +25,7 @@ serde.workspace = true serde_json.workspace = true thiserror.workspace = true tokio.workspace = true +tracing.workspace = true [dev-dependencies] httpmock = "0.8" diff --git a/lib/crates/fabro-auth/src/credential.rs b/lib/crates/fabro-auth/src/credential.rs index d4ec91262..95b195d61 100644 --- a/lib/crates/fabro-auth/src/credential.rs +++ b/lib/crates/fabro-auth/src/credential.rs @@ -1,47 +1,12 @@ use chrono::{DateTime, Duration, Utc}; use fabro_redact::redact_string; -use serde::{Deserialize, Serialize}; - -/// JSON shape stored in the vault when `secret_type == Oauth`. -/// -/// Provider context comes from the catalog and auth strategy at resolve time. -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] -pub struct OAuthCredential { - pub tokens: OAuthTokens, - pub config: OAuthConfig, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub account_id: Option, -} - -impl OAuthCredential { - #[must_use] - pub fn needs_refresh(&self) -> bool { - self.tokens.expires_at <= Utc::now() + Duration::minutes(5) - } -} - -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] -pub struct OAuthTokens { - pub access_token: String, - pub refresh_token: Option, - pub expires_at: DateTime, -} +pub use fabro_types::{OAuthConfig, OAuthCredential, OAuthTokens}; pub(crate) fn expires_at_from_now(expires_in: Option) -> DateTime { let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); Utc::now() + Duration::seconds(seconds) } -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] -pub struct OAuthConfig { - pub auth_url: String, - pub token_url: String, - pub client_id: String, - pub scopes: Vec, - pub redirect_uri: Option, - pub use_pkce: bool, -} - #[derive(Clone, PartialEq, Eq)] pub enum ApiKeyHeader { Bearer(String), diff --git a/lib/crates/fabro-auth/src/lib.rs b/lib/crates/fabro-auth/src/lib.rs index c46283c4e..1bc135149 100644 --- a/lib/crates/fabro-auth/src/lib.rs +++ b/lib/crates/fabro-auth/src/lib.rs @@ -4,6 +4,7 @@ mod credential_source; mod env_source; mod refresh; mod resolve; +mod sql_vault_source; mod strategy; mod vault_ext; mod vault_source; @@ -20,6 +21,7 @@ pub use resolve::{ ResolvedCredential, auth_issue_message, build_api_key_header, configured_providers_from_process_env, }; +pub use sql_vault_source::SqlVaultCredentialSource; pub use strategy::{ AuthMethod, AuthStrategy, CODEX_AUTH_URL, CODEX_CLIENT_ID, CODEX_TOKEN_URL, LoginResult, codex_oauth_config, strategy_for, diff --git a/lib/crates/fabro-auth/src/sql_vault_source.rs b/lib/crates/fabro-auth/src/sql_vault_source.rs new file mode 100644 index 000000000..ca581d5b6 --- /dev/null +++ b/lib/crates/fabro-auth/src/sql_vault_source.rs @@ -0,0 +1,131 @@ +use std::sync::Arc; + +use async_trait::async_trait; +use fabro_model::{Catalog, ProviderId}; +use fabro_types::SecretType; +use fabro_vault::{SecretSnapshot, SecretStore, SecretStoreError, Vault}; +use tokio::sync::RwLock; +use tracing::error; + +use crate::credential_source::{CredentialSource, ResolvedCredentials}; +use crate::{EnvLookup, VaultCredentialSource}; + +#[derive(Clone)] +pub struct SqlVaultCredentialSource { + store: Arc, + env_lookup: EnvLookup, +} + +impl SqlVaultCredentialSource { + #[must_use] + #[expect( + clippy::disallowed_methods, + reason = "SqlVaultCredentialSource::new owns the process-env fallback used after vault \ + lookup." + )] + pub fn new(store: Arc) -> Self { + Self::with_env_lookup(store, |name| std::env::var(name).ok()) + } + + #[must_use] + pub fn vault_only(store: Arc) -> Self { + Self::with_env_lookup(store, |_| None) + } + + #[must_use] + pub fn with_env_lookup(store: Arc, env_lookup: F) -> Self + where + F: Fn(&str) -> Option + Send + Sync + 'static, + { + Self { + store, + env_lookup: Arc::new(env_lookup), + } + } + + fn source_for_snapshot(&self, snapshot: SecretSnapshot) -> VaultCredentialSource { + let env_lookup = Arc::clone(&self.env_lookup); + VaultCredentialSource::with_env_lookup( + Arc::new(RwLock::new(snapshot.into_vault())), + move |name| env_lookup(name), + ) + } + + async fn persist_oauth_refreshes( + &self, + before: &Vault, + after: &Vault, + ) -> Result { + for (name, after_entry) in after.entries() { + if after_entry.secret_type != SecretType::Oauth { + continue; + } + let Some(before_entry) = before.get_entry(name) else { + continue; + }; + if before_entry.value == after_entry.value { + continue; + } + match self + .store + .replace_if_revision( + name, + before_entry.revision, + &after_entry.value, + SecretType::Oauth, + ) + .await + { + Ok(_) => {} + Err(SecretStoreError::StaleRevision { .. }) => return Ok(false), + Err(err) => return Err(err), + } + } + Ok(true) + } +} + +impl std::fmt::Debug for SqlVaultCredentialSource { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SqlVaultCredentialSource") + .finish_non_exhaustive() + } +} + +#[async_trait] +impl CredentialSource for SqlVaultCredentialSource { + async fn resolve(&self, catalog: &Catalog) -> anyhow::Result { + for _ in 0..2 { + let before = self.store.snapshot().await?; + let has_oauth = before + .entries() + .values() + .any(|entry| entry.secret_type == SecretType::Oauth); + if !has_oauth { + // Only OAuth resolution can write back (token refresh); with no + // OAuth secrets, skip the snapshot clones and CAS machinery. + return self.source_for_snapshot(before).resolve(catalog).await; + } + let source = self.source_for_snapshot(before.clone()); + let resolved = source.resolve(catalog).await?; + let after = source.snapshot().await; + if self.persist_oauth_refreshes(&before, &after).await? { + return Ok(resolved); + } + } + anyhow::bail!("OAuth credential changed concurrently during refresh") + } + + async fn configured_providers(&self, catalog: &Catalog) -> Vec { + let snapshot = match self.store.snapshot().await { + Ok(snapshot) => snapshot, + Err(err) => { + error!(error = ?err, "Failed to load configured providers from secret store"); + return Vec::new(); + } + }; + self.source_for_snapshot(snapshot) + .configured_providers(catalog) + .await + } +} diff --git a/lib/crates/fabro-auth/src/vault_source.rs b/lib/crates/fabro-auth/src/vault_source.rs index a78cf9428..aeba384a4 100644 --- a/lib/crates/fabro-auth/src/vault_source.rs +++ b/lib/crates/fabro-auth/src/vault_source.rs @@ -35,6 +35,10 @@ impl VaultCredentialSource { pub fn vault_only(vault: Arc>) -> Self { Self::with_env_lookup(vault, |_| None) } + + pub(crate) async fn snapshot(&self) -> Vault { + self.vault.read().await.clone() + } } impl std::fmt::Debug for VaultCredentialSource { diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index a30ec66f0..b3318ce15 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -2,7 +2,7 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, OnceLock}; use anyhow::{Context as _, Result, bail}; -use fabro_auth::{CredentialSource, EnvCredentialSource, VaultCredentialSource}; +use fabro_auth::{CredentialSource, SqlVaultCredentialSource}; use fabro_config::{CliLayer, Storage, load_llm_catalog_settings}; use fabro_model::Catalog; use fabro_types::UserSettings; @@ -10,8 +10,8 @@ use fabro_types::settings::RunNamespace; use fabro_types::settings::cli::{OutputFormat, OutputVerbosity}; use fabro_util::error::SharedError; use fabro_util::printer::Printer; -use fabro_vault::Vault; -use tokio::sync::{OnceCell, RwLock as AsyncRwLock}; +use fabro_vault::SecretStore; +use tokio::sync::OnceCell; use crate::args::{ ServerConnectionArgs, ServerTargetArgs, printer_from_verbosity, require_no_json_override, @@ -169,13 +169,12 @@ impl CommandContext { let source = self .llm_source .get_or_try_init(|| async move { + let storage = Storage::new(&storage_dir); + let store = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .context("opening the Fabro secret store")?; let source: Arc = - match Vault::load(Storage::new(&storage_dir).secrets_path()) { - Ok(vault) => Arc::new(VaultCredentialSource::new(Arc::new( - AsyncRwLock::new(vault), - ))), - Err(_) => Arc::new(EnvCredentialSource::new()), - }; + Arc::new(SqlVaultCredentialSource::new(Arc::new(store))); Ok::, anyhow::Error>(source) }) .await?; diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 6c74b31a4..7ea8885c0 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -29,7 +29,7 @@ use fabro_config::user::{SETTINGS_CONFIG_FILENAME, default_storage_dir}; use fabro_config::{Storage, UserSettingsBuilder, envfile}; use fabro_install::{ GITHUB_APP_VAULT_KEYS, GITHUB_INSTALL_SECRET_KEYS, InstallListenConfig, InstallPersistencePlan, - PendingDevTokenWrite, PendingSettingsWrite, VaultSecretWrite, + PendingDevTokenWrite, PendingSettingsWrite, SecretStoreWrite, merge_server_settings as merge_server_settings_impl, prepare_dev_token_write_for_install, restore_optional_file, rollback_dev_token_write, seed_environments_in_storage, write_github_app_settings, write_token_settings, @@ -1349,11 +1349,11 @@ struct PendingGitHubInstallWrite<'a> { settings_write: PendingSettingsWrite<'a>, server_env_set: Vec<(String, String)>, server_env_remove: Vec<&'static str>, - vault_set: Vec, + vault_set: Vec, vault_remove: Vec<&'static str>, } -fn persist_github_install_changes( +async fn persist_github_install_changes( storage_dir: &Path, writes: &PendingGitHubInstallWrite<'_>, ) -> Result<()> { @@ -1373,7 +1373,8 @@ fn persist_github_install_changes( .map(|key| (*key).to_string()) .collect(), } - .persist_direct()) + .persist_direct() + .await) { Ok(()) => Ok(()), Err(err) => { @@ -1422,7 +1423,8 @@ async fn persist_cli_install_outputs_with( vault_writes: Vec::new(), vault_removals: Vec::new(), } - .persist_direct()?; + .persist_direct() + .await?; let persist_result = persist_vault_secrets_with( storage_dir, @@ -1628,7 +1630,7 @@ async fn run_install_github_inner( match selection { GitHubInstallSelection::Token { token } => { write_token_settings(&mut doc)?; - vault_set.push(VaultSecretWrite { + vault_set.push(SecretStoreWrite { name: GITHUB_TOKEN_SECRET_KEY.to_string(), value: token, secret_type: VaultSecretType::Token, @@ -1666,7 +1668,7 @@ async fn run_install_github_inner( } else { VaultSecretType::Token }; - vault_set.push(VaultSecretWrite { + vault_set.push(SecretStoreWrite { name: key, value, secret_type, @@ -1698,7 +1700,8 @@ async fn run_install_github_inner( server_env_remove, vault_set, vault_remove, - })?; + }) + .await?; if let Some(restart_outcome) = maybe_restart_server_after_github_install(&storage_dir, &config_path, server_was_running) @@ -2069,7 +2072,7 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( " {} Saved {} workflow-visible secrets to {}", s.green.apply_to("✔"), vault_secrets.len(), - path::contract_tilde(&Storage::new(&storage_dir).secrets_path()).display() + path::contract_tilde(&Storage::new(&storage_dir).sqlite_path()).display() ); fabro_util::printerr!( printer, @@ -2170,6 +2173,16 @@ mod tests { use super::*; + async fn load_secret_snapshot(storage: &Storage) -> Vault { + fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .unwrap() + .snapshot() + .await + .unwrap() + .into_vault() + } + fn install_args(non_interactive: bool, scripted: InstallNonInteractiveArgs) -> InstallArgs { InstallArgs { storage_dir: crate::args::StorageDirArgs::default(), @@ -3227,8 +3240,8 @@ client_id = "client-id" ); } - #[test] - fn persist_github_install_changes_replaces_app_env_keys_with_token_secret() { + #[tokio::test] + async fn persist_github_install_changes_replaces_app_env_keys_with_token_secret() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let server_env_path = storage.runtime_directory().env_path(); @@ -3267,7 +3280,7 @@ client_id = "client-id" GITHUB_APP_CLIENT_SECRET_KEY, GITHUB_APP_WEBHOOK_SECRET_KEY, ], - vault_set: vec![VaultSecretWrite { + vault_set: vec![SecretStoreWrite { name: GITHUB_TOKEN_SECRET_KEY.to_string(), value: "token".to_string(), secret_type: VaultSecretType::Token, @@ -3275,6 +3288,7 @@ client_id = "client-id" }], vault_remove: Vec::new(), }) + .await .unwrap(); let server_env = envfile::read_env_file(&server_env_path).unwrap(); @@ -3283,7 +3297,7 @@ client_id = "client-id" assert!(!server_env.contains_key(GITHUB_APP_CLIENT_SECRET_KEY)); assert!(!server_env.contains_key(GITHUB_APP_WEBHOOK_SECRET_KEY)); - let vault = Vault::load(storage.secrets_path()).unwrap(); + let vault = load_secret_snapshot(&storage).await; assert_eq!(vault.get(GITHUB_TOKEN_SECRET_KEY), Some("token")); assert_eq!( vault @@ -3294,8 +3308,8 @@ client_id = "client-id" assert_eq!(std::fs::read_to_string(&settings_path).unwrap(), "after"); } - #[test] - fn persist_github_install_changes_replaces_token_secret_with_app_vault_keys() { + #[tokio::test] + async fn persist_github_install_changes_replaces_token_secret_with_app_vault_keys() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let server_env_path = storage.runtime_directory().env_path(); @@ -3332,19 +3346,19 @@ client_id = "client-id" GITHUB_APP_WEBHOOK_SECRET_KEY, ], vault_set: vec![ - VaultSecretWrite { + SecretStoreWrite { name: GITHUB_APP_PRIVATE_KEY_KEY.to_string(), value: "private".to_string(), secret_type: VaultSecretType::File, description: None, }, - VaultSecretWrite { + SecretStoreWrite { name: GITHUB_APP_CLIENT_SECRET_KEY.to_string(), value: "client".to_string(), secret_type: VaultSecretType::Token, description: None, }, - VaultSecretWrite { + SecretStoreWrite { name: GITHUB_APP_WEBHOOK_SECRET_KEY.to_string(), value: "webhook".to_string(), secret_type: VaultSecretType::Token, @@ -3353,6 +3367,7 @@ client_id = "client-id" ], vault_remove: vec![GITHUB_TOKEN_SECRET_KEY], }) + .await .unwrap(); let server_env = envfile::read_env_file(&server_env_path).unwrap(); @@ -3361,7 +3376,7 @@ client_id = "client-id" assert!(!server_env.contains_key(GITHUB_APP_CLIENT_SECRET_KEY)); assert!(!server_env.contains_key(GITHUB_APP_WEBHOOK_SECRET_KEY)); - let vault = Vault::load(storage.secrets_path()).unwrap(); + let vault = load_secret_snapshot(&storage).await; assert_eq!(vault.get(GITHUB_TOKEN_SECRET_KEY), None); assert_eq!(vault.get(GITHUB_APP_PRIVATE_KEY_KEY), Some("private")); assert_eq!(vault.get(GITHUB_APP_CLIENT_SECRET_KEY), Some("client")); @@ -3387,8 +3402,8 @@ client_id = "client-id" assert_eq!(std::fs::read_to_string(&settings_path).unwrap(), "after"); } - #[test] - fn persist_github_install_changes_restores_server_env_on_vault_failure() { + #[tokio::test] + async fn persist_github_install_changes_restores_server_env_on_vault_failure() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let server_env_path = storage.runtime_directory().env_path(); @@ -3419,14 +3434,15 @@ client_id = "client-id" }, server_env_set: Vec::new(), server_env_remove: vec![GITHUB_APP_PRIVATE_KEY_KEY, GITHUB_APP_CLIENT_SECRET_KEY], - vault_set: vec![VaultSecretWrite { + vault_set: vec![SecretStoreWrite { name: "bad-secret-name".to_string(), value: "token".to_string(), secret_type: VaultSecretType::Token, description: None, }], vault_remove: Vec::new(), - }); + }) + .await; assert!(result.is_err()); let server_env = envfile::read_env_file(&server_env_path).unwrap(); @@ -3445,9 +3461,7 @@ client_id = "client-id" assert_eq!(server_env.get("KEEP_ME").map(String::as_str), Some("1")); assert_eq!(std::fs::read_to_string(&settings_path).unwrap(), "before"); assert_eq!( - Vault::load(storage.secrets_path()) - .unwrap() - .get("bad-secret-name"), + load_secret_snapshot(&storage).await.get("bad-secret-name"), None ); } diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 9eba9a3d5..90283c353 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -24,7 +24,7 @@ use fabro_types::{ ArtifactUpload, EventBody, FailureReason, Principal, RunBlobId, RunEvent, RunId, WorkflowSettings, }; -use fabro_vault::Vault; +use fabro_vault::{SecretStore, Vault}; use fabro_workflow::artifact_upload::{ArtifactSink, StageArtifactUploader}; use fabro_workflow::event::{Emitter, RunEventSink}; use fabro_workflow::operations::{self, StartServices}; @@ -137,7 +137,7 @@ pub(crate) async fn execute( if let Some(control_manager) = &mut control_manager { control_manager.wait_for_first_connection().await?; } - let vault = load_worker_vault(storage_dir.as_deref())?; + let vault = load_worker_vault(storage_dir.as_deref()).await?; let github_app = { let vault_guard = match &vault { Some(arc) => Some(arc.read().await), @@ -272,18 +272,24 @@ impl fabro_tool::RunManifestBuilder for WorkerRunManifestBuilder { } } -fn load_worker_vault(storage_dir: Option<&Path>) -> Result>>> { +async fn load_worker_vault(storage_dir: Option<&Path>) -> Result>>> { let Some(storage_dir) = storage_dir else { return Ok(None); }; let storage = Storage::new(storage_dir); - let vault = Vault::load(storage.secrets_path()).with_context(|| { - format!( - "failed to load worker vault from {}", - storage.root().display() - ) - })?; + let vault = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .with_context(|| { + format!( + "failed to open worker secret store from {}", + storage.root().display() + ) + })? + .snapshot() + .await + .context("loading worker secrets snapshot")? + .into_vault(); Ok(Some(Arc::new(AsyncRwLock::new(vault)))) } @@ -1744,7 +1750,7 @@ mod tests { .set("ANTHROPIC_API_KEY", "vault-key", SecretType::Token, None) .unwrap(); - let loaded = load_worker_vault(Some(temp.path())).unwrap().unwrap(); + let loaded = load_worker_vault(Some(temp.path())).await.unwrap().unwrap(); let guard = loaded.read().await; let credential = guard.get("ANTHROPIC_API_KEY").unwrap(); diff --git a/lib/crates/fabro-cli/src/commands/server/start.rs b/lib/crates/fabro-cli/src/commands/server/start.rs index 6957360d3..a9d03c0a7 100644 --- a/lib/crates/fabro-cli/src/commands/server/start.rs +++ b/lib/crates/fabro-cli/src/commands/server/start.rs @@ -7,19 +7,20 @@ use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; 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::default_settings_path; +use fabro_config::{RuntimeDirectory, Storage}; use fabro_server::jwt_auth::auth_method_name; use fabro_server::serve::{DEFAULT_TCP_PORT, ServeArgs, resolve_runtime_server_settings_for_start}; use fabro_server::{ - load_startup_vault, process_env_snapshot, validate_startup, validate_startup_configuration, + migrate_startup_vault, process_env_snapshot, validate_startup, validate_startup_configuration, }; use fabro_static::EnvVars; use fabro_types::settings::{LogDestination, ServerAuthMethod}; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; +use fabro_vault::SecretStore; use tokio::process::Command as TokioCommand; use tokio::task::spawn_blocking; use tokio::time; @@ -289,7 +290,14 @@ async fn execute_daemon( ); } validate_startup_configuration(&resolved_settings)?; - let startup_vault = load_startup_vault(fabro_config::Storage::new(storage_dir).secrets_path())?; + let storage = Storage::new(storage_dir); + migrate_startup_vault(storage.secrets_path()); + let startup_vault = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .context("opening the Fabro secret store for startup validation")? + .snapshot() + .await + .context("loading secrets for startup validation")?; validate_startup( runtime_directory.env_path().as_path(), process_env_snapshot(), diff --git a/lib/crates/fabro-cli/tests/it/cmd/install.rs b/lib/crates/fabro-cli/tests/it/cmd/install.rs index 61e869c33..ee9cb4c93 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/install.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/install.rs @@ -13,6 +13,16 @@ use fabro_vault::{SecretType, Vault}; const INSTALL_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); +async fn load_secret_snapshot(storage: &Storage) -> Vault { + fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .expect("test secret store should open") + .snapshot() + .await + .expect("test secret snapshot should load") + .into_vault() +} + #[test] fn help() { let context = test_context!(); @@ -611,8 +621,8 @@ fn github_non_interactive_requires_strategy() { assert!(stderr.contains("install github --non-interactive requires --strategy")); } -#[test] -fn github_non_interactive_token_reconfigures_existing_app_install() { +#[tokio::test] +async fn github_non_interactive_token_reconfigures_existing_app_install() { let mut context = test_context!(); let storage_dir = context.home_dir.join("install-storage"); context.manage_storage_dir(&storage_dir); @@ -758,7 +768,7 @@ mode = "keep-me" assert!(!server_env.contains_key("GITHUB_APP_WEBHOOK_SECRET")); assert_eq!(server_env.get("KEEP_ME").map(String::as_str), Some("1")); - let vault = Vault::load(Storage::new(&storage_dir).secrets_path()).unwrap(); + let vault = load_secret_snapshot(&Storage::new(&storage_dir)).await; assert_eq!(vault.get("GITHUB_TOKEN"), Some("token-from-gh")); assert_eq!(vault.get("GITHUB_APP_PRIVATE_KEY"), None); assert_eq!(vault.get("GITHUB_APP_CLIENT_SECRET"), None); diff --git a/lib/crates/fabro-db/migrations/2026071101_secrets.sql b/lib/crates/fabro-db/migrations/2026071101_secrets.sql new file mode 100644 index 000000000..88c6d370d --- /dev/null +++ b/lib/crates/fabro-db/migrations/2026071101_secrets.sql @@ -0,0 +1,28 @@ +CREATE TABLE secrets ( + name TEXT PRIMARY KEY NOT NULL, + secret_type TEXT NOT NULL, + value TEXT NOT NULL, + description TEXT, + revision INTEGER NOT NULL DEFAULT 1, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL, + CHECK (length(name) > 0), + CHECK (secret_type IN ('token', 'oauth', 'file')), + CHECK (revision > 0), + CHECK (length(created_at) > 0), + CHECK (length(updated_at) > 0), + CHECK ( + (secret_type IN ('token', 'oauth') + AND substr(name, 1, 1) GLOB '[A-Za-z_]' + AND name NOT GLOB '*[^A-Za-z0-9_]*') + OR + (secret_type = 'file' + AND ( + name = 'GITHUB_APP_PRIVATE_KEY' + OR ( + substr(name, 1, 1) = '/' + AND substr(name, -1, 1) <> '/' + ) + )) + ) +); diff --git a/lib/crates/fabro-db/src/lib.rs b/lib/crates/fabro-db/src/lib.rs index f895ee07a..8765c82ea 100644 --- a/lib/crates/fabro-db/src/lib.rs +++ b/lib/crates/fabro-db/src/lib.rs @@ -6,6 +6,8 @@ use anyhow::Context as _; use sqlx::migrate::{Migrate as _, Migrator}; use sqlx::sqlite::{SqliteConnectOptions, SqliteJournalMode, SqlitePoolOptions, SqliteSynchronous}; use tokio::fs; +#[cfg(unix)] +use tokio::task::spawn_blocking; use tracing::info; pub type DbPool = sqlx::SqlitePool; @@ -26,6 +28,8 @@ impl Database { })?; } + prepare_private_database_file(path).await?; + let options = SqliteConnectOptions::new() .filename(path) .create_if_missing(true) @@ -203,3 +207,38 @@ async fn set_private_permissions(path: &Path) -> anyhow::Result<()> { async fn set_private_permissions(_path: &Path) -> anyhow::Result<()> { Ok(()) } + +#[cfg(unix)] +#[expect( + clippy::disallowed_methods, + reason = "SQLite file permissions must be established synchronously before opening the pool" +)] +async fn prepare_private_database_file(path: &Path) -> anyhow::Result<()> { + use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; + + let path = path.to_path_buf(); + spawn_blocking(move || { + std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .mode(0o600) + .open(&path) + .with_context(|| format!("creating private SQLite database {}", path.display()))?; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .with_context(|| format!("setting private SQLite permissions on {}", path.display())) + }) + .await + .context("joining SQLite permission setup task")? +} + +#[cfg(not(unix))] +async fn prepare_private_database_file(path: &Path) -> anyhow::Result<()> { + fs::OpenOptions::new() + .create(true) + .write(true) + .open(path) + .await + .with_context(|| format!("creating SQLite database {}", path.display()))?; + Ok(()) +} diff --git a/lib/crates/fabro-db/tests/sqlite.rs b/lib/crates/fabro-db/tests/sqlite.rs index 28af2aadd..53fc0d803 100644 --- a/lib/crates/fabro-db/tests/sqlite.rs +++ b/lib/crates/fabro-db/tests/sqlite.rs @@ -11,6 +11,23 @@ async fn connect_creates_parent_directory_and_migrate_is_idempotent() -> anyhow: database.health_check().await?; assert!(db_path.exists()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + + for path in [ + db_path.clone(), + db_path.with_extension("sqlite3-wal"), + db_path.with_extension("sqlite3-shm"), + ] { + assert_eq!( + std::fs::metadata(&path)?.permissions().mode() & 0o777, + 0o600, + "{} should be private", + path.display() + ); + } + } let variable_table_count: i64 = sqlx::query_scalar( "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'variables'", ) @@ -25,6 +42,13 @@ async fn connect_creates_parent_directory_and_migrate_is_idempotent() -> anyhow: .await?; assert_eq!(environments_table_count, 1); + let secrets_table_count: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'secrets'", + ) + .fetch_one(database.pool()) + .await?; + assert_eq!(secrets_table_count, 1); + let legacy_import_table_count: i64 = sqlx::query_scalar( "SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'legacy_imports'", ) diff --git a/lib/crates/fabro-install/Cargo.toml b/lib/crates/fabro-install/Cargo.toml index c7be60f2e..ccdd8b38c 100644 --- a/lib/crates/fabro-install/Cargo.toml +++ b/lib/crates/fabro-install/Cargo.toml @@ -14,6 +14,7 @@ anyhow.workspace = true base64.workspace = true ring = "0.17" toml.workspace = true +tokio.workspace = true fabro-config = { path = "../fabro-config" } fabro-db = { path = "../fabro-db" } fabro-environment.workspace = true diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index d26a0cf0e..0d465368f 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -10,7 +10,8 @@ use fabro_config::{Storage, envfile}; use fabro_static::EnvVars; use fabro_types::settings::run::EnvironmentProvider; use fabro_util::dev_token; -use fabro_vault::{SecretType as VaultSecretType, Vault}; +use fabro_vault::SecretStore; +pub use fabro_vault::SecretStoreWrite; #[derive(Debug, Clone, Copy)] pub struct PendingSettingsWrite<'a> { @@ -53,21 +54,13 @@ pub const GITHUB_APP_VAULT_KEYS: &[&str] = &[ EnvVars::GITHUB_APP_WEBHOOK_SECRET, ]; -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct VaultSecretWrite { - pub name: String, - pub value: String, - pub secret_type: VaultSecretType, - pub description: Option, -} - pub struct InstallPersistencePlan<'a> { pub storage_dir: &'a Path, pub settings_write: Option>, pub server_env_writes: Vec, pub server_env_removals: Vec, pub dev_token_write: Option, - pub vault_writes: Vec, + pub vault_writes: Vec, pub vault_removals: Vec, } @@ -582,33 +575,20 @@ fn persist_server_env_secrets( .with_context(|| format!("updating server env file {}", env_path.display())) } -fn persist_vault_secrets_direct( +async fn persist_vault_secrets_direct( storage_dir: &Path, - secrets: &[VaultSecretWrite], + secrets: &[SecretStoreWrite], removals: &[String], ) -> Result<()> { if secrets.is_empty() && removals.is_empty() { return Ok(()); } - let vault_path = Storage::new(storage_dir).secrets_path(); - let mut vault = Vault::load(vault_path).map_err(anyhow::Error::from)?; - for name in removals { - match vault.remove(name) { - Ok(()) | Err(fabro_vault::Error::NotFound(_)) => {} - Err(err) => return Err(err.into()), - } - } - for secret in secrets { - vault - .set( - &secret.name, - &secret.value, - secret.secret_type, - secret.description.as_deref(), - ) - .map_err(anyhow::Error::from)?; - } + let storage = Storage::new(storage_dir); + SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await? + .apply(removals, secrets) + .await?; Ok(()) } @@ -625,8 +605,6 @@ fn direct_persistence_error(err: anyhow::Error, rollback_failures: &[String]) -> fn rollback_direct_persistence( settings_write: Option<&PendingSettingsWrite<'_>>, - vault_path: &Path, - previous_vault: Option<&str>, dev_token_write: Option<&PendingDevTokenWrite>, ) -> Vec { let mut failures = Vec::new(); @@ -636,9 +614,6 @@ fn rollback_direct_persistence( failures.push(err.to_string()); } } - if let Err(err) = restore_optional_file(vault_path, previous_vault) { - failures.push(err.to_string()); - } if let Some(write) = dev_token_write { if let Err(err) = rollback_dev_token_write(write) { failures.push(err.to_string()); @@ -649,7 +624,7 @@ fn rollback_direct_persistence( } impl InstallPersistencePlan<'_> { - pub fn persist_direct(&self) -> std::result::Result<(), PersistInstallOutputsError> { + fn persist_files(&self) -> std::result::Result, PersistInstallOutputsError> { let server_env_report = persist_server_env_secrets( self.storage_dir, &self.server_env_writes, @@ -673,34 +648,10 @@ impl InstallPersistencePlan<'_> { })?; } - let vault_path = Storage::new(self.storage_dir).secrets_path(); - let previous_vault = std::fs::read_to_string(&vault_path).ok(); - - if let Err(err) = - persist_vault_secrets_direct(self.storage_dir, &self.vault_writes, &self.vault_removals) - { - let rollback_failures = rollback_direct_persistence( - self.settings_write.as_ref(), - &vault_path, - previous_vault.as_deref(), - self.dev_token_write.as_ref(), - ); - let error = direct_persistence_error(err, &rollback_failures); - return Err(PersistInstallOutputsError::new( - error, - true, - removed_env_keys, - )); - } - if let Some(write) = self.dev_token_write.as_ref() { if let Err(err) = write_pending_dev_token(write) { - let rollback_failures = rollback_direct_persistence( - self.settings_write.as_ref(), - &vault_path, - previous_vault.as_deref(), - Some(write), - ); + let rollback_failures = + rollback_direct_persistence(self.settings_write.as_ref(), Some(write)); let error = direct_persistence_error(err, &rollback_failures); return Err(PersistInstallOutputsError::new( error, @@ -710,15 +661,41 @@ impl InstallPersistencePlan<'_> { } } + Ok(removed_env_keys) + } + + fn secret_persistence_error( + &self, + err: anyhow::Error, + removed_env_keys: Vec, + ) -> PersistInstallOutputsError { + let rollback_failures = rollback_direct_persistence( + self.settings_write.as_ref(), + self.dev_token_write.as_ref(), + ); + let error = direct_persistence_error(err, &rollback_failures); + PersistInstallOutputsError::new(error, true, removed_env_keys) + } + + pub async fn persist_direct(&self) -> std::result::Result<(), PersistInstallOutputsError> { + let removed_env_keys = self.persist_files()?; + + if let Err(err) = + persist_vault_secrets_direct(self.storage_dir, &self.vault_writes, &self.vault_removals) + .await + { + return Err(self.secret_persistence_error(err, removed_env_keys)); + } + Ok(()) } } -pub fn persist_install_outputs_direct( +pub async fn persist_install_outputs_direct( storage_dir: &Path, server_env_writes: &[envfile::EnvFileUpdate], server_env_removals: &[envfile::EnvFileRemoval], - vault_secrets: &[VaultSecretWrite], + vault_secrets: &[SecretStoreWrite], settings_write: Option<&PendingSettingsWrite<'_>>, ) -> std::result::Result<(), PersistInstallOutputsError> { InstallPersistencePlan { @@ -731,6 +708,7 @@ pub fn persist_install_outputs_direct( vault_removals: Vec::new(), } .persist_direct() + .await } #[cfg(test)] @@ -744,13 +722,24 @@ mod tests { }; use fabro_vault::{SecretType as VaultSecretType, Vault}; + async fn load_secret_snapshot(storage: &Storage) -> Vault { + SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .unwrap() + .snapshot() + .await + .unwrap() + .into_vault() + } + use super::{ InstallListenConfig, InstallObjectStoreCredentialMode, InstallObjectStoreSelection, InstallPersistencePlan, InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_MANAGED_COMMENT, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, - VaultSecretWrite, default_web_url, merge_server_settings, persist_install_outputs_direct, - prepare_dev_token_write_for_install, set_cli_target_http, set_server_listen, - write_github_app_settings, write_object_store_settings, write_sandbox_settings, + SecretStore, SecretStoreWrite, default_web_url, merge_server_settings, + persist_install_outputs_direct, prepare_dev_token_write_for_install, set_cli_target_http, + set_server_listen, write_github_app_settings, write_object_store_settings, + write_sandbox_settings, }; fn format_config_toml() -> String { @@ -1009,8 +998,8 @@ stale = "remove-me" ); } - #[test] - fn persist_install_outputs_direct_restores_settings_and_vault_on_secret_failure() { + #[tokio::test] + async fn persist_install_outputs_direct_restores_settings_and_vault_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let settings_path = dir.path().join("settings.toml"); @@ -1029,7 +1018,7 @@ stale = "remove-me" comment: None, }], &[], - &[VaultSecretWrite { + &[SecretStoreWrite { name: "bad-secret-name".to_string(), value: "boom".to_string(), secret_type: VaultSecretType::Token, @@ -1040,7 +1029,8 @@ stale = "remove-me" contents: "_version = 1\n[server]\nfoo = \"bar\"\n", previous_contents: Some("_version = 1\n[server]\n"), }), - ); + ) + .await; assert!(result.is_err()); assert_eq!( @@ -1048,7 +1038,7 @@ stale = "remove-me" "_version = 1\n[server]\n" ); - let restored = Vault::load(vault_path).unwrap(); + let restored = load_secret_snapshot(&storage).await; assert_eq!(restored.get("EXISTING_SECRET"), Some("keep")); assert_eq!(restored.get("bad-secret-name"), None); @@ -1059,8 +1049,8 @@ stale = "remove-me" ); } - #[test] - fn install_persistence_plan_direct_writes_and_removes_vault_secrets() { + #[tokio::test] + async fn install_persistence_plan_direct_writes_and_removes_vault_secrets() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let mut vault = Vault::load(storage.secrets_path()).unwrap(); @@ -1077,7 +1067,7 @@ stale = "remove-me" server_env_writes: Vec::new(), server_env_removals: Vec::new(), dev_token_write: None, - vault_writes: vec![VaultSecretWrite { + vault_writes: vec![SecretStoreWrite { name: "NEW_SECRET".to_string(), value: "new".to_string(), secret_type: VaultSecretType::Token, @@ -1086,16 +1076,17 @@ stale = "remove-me" vault_removals: vec!["REMOVE_ME".to_string()], } .persist_direct() + .await .unwrap(); - let vault = Vault::load(storage.secrets_path()).unwrap(); + let vault = load_secret_snapshot(&storage).await; assert_eq!(vault.get("REMOVE_ME"), None); assert_eq!(vault.get("KEEP_ME"), Some("keep")); assert_eq!(vault.get("NEW_SECRET"), Some("new")); } - #[test] - fn install_persistence_plan_direct_restores_settings_and_vault_on_secret_failure() { + #[tokio::test] + async fn install_persistence_plan_direct_restores_settings_and_vault_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let settings_path = dir.path().join("settings.toml"); @@ -1120,7 +1111,7 @@ stale = "remove-me" }], server_env_removals: Vec::new(), dev_token_write: None, - vault_writes: vec![VaultSecretWrite { + vault_writes: vec![SecretStoreWrite { name: "bad-secret-name".to_string(), value: "boom".to_string(), secret_type: VaultSecretType::Token, @@ -1128,14 +1119,15 @@ stale = "remove-me" }], vault_removals: vec!["REMOVE_ME".to_string()], } - .persist_direct(); + .persist_direct() + .await; assert!(result.is_err()); assert_eq!( std::fs::read_to_string(&settings_path).unwrap(), "_version = 1\n[server]\n" ); - let vault = Vault::load(vault_path).unwrap(); + let vault = load_secret_snapshot(&storage).await; assert_eq!(vault.get("REMOVE_ME"), Some("old")); assert_eq!(vault.get("bad-secret-name"), None); let server_env = envfile::read_env_file(&storage.runtime_directory().env_path()).unwrap(); @@ -1197,8 +1189,8 @@ stale = "remove-me" assert!(err.to_string().contains("invalid dev token format")); } - #[test] - fn install_persistence_plan_direct_writes_staged_dev_token_on_success() { + #[tokio::test] + async fn install_persistence_plan_direct_writes_staged_dev_token_on_success() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let path = storage.runtime_directory().dev_token_path(); @@ -1215,6 +1207,7 @@ stale = "remove-me" vault_removals: Vec::new(), } .persist_direct() + .await .unwrap(); assert_eq!(read_dev_token_file(&path).as_deref(), Some(token.as_str())); @@ -1228,8 +1221,8 @@ stale = "remove-me" } } - #[test] - fn install_persistence_plan_direct_does_not_leave_staged_dev_token_on_vault_failure() { + #[tokio::test] + async fn install_persistence_plan_direct_does_not_leave_staged_dev_token_on_vault_failure() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let path = storage.runtime_directory().dev_token_path(); @@ -1241,7 +1234,7 @@ stale = "remove-me" server_env_writes: Vec::new(), server_env_removals: Vec::new(), dev_token_write: prepared.write, - vault_writes: vec![VaultSecretWrite { + vault_writes: vec![SecretStoreWrite { name: "bad-secret-name".to_string(), value: "boom".to_string(), secret_type: VaultSecretType::Token, @@ -1249,7 +1242,8 @@ stale = "remove-me" }], vault_removals: Vec::new(), } - .persist_direct(); + .persist_direct() + .await; assert!(result.is_err()); assert!( @@ -1506,8 +1500,8 @@ stale = "remove-me" .and_then(toml::Value::as_bool) } - #[test] - fn persist_install_outputs_direct_only_removes_marked_object_store_keys() { + #[tokio::test] + async fn persist_install_outputs_direct_only_removes_marked_object_store_keys() { let dir = tempfile::tempdir().unwrap(); let storage = Storage::new(dir.path()); let env_path = storage.runtime_directory().env_path(); @@ -1530,6 +1524,7 @@ stale = "remove-me" &[], None, ) + .await .expect("env-only persistence should succeed"); let server_env = envfile::read_env_file(&env_path).unwrap(); @@ -1549,8 +1544,8 @@ stale = "remove-me" } #[cfg(unix)] - #[test] - fn persist_install_outputs_direct_writes_private_server_env_permissions() { + #[tokio::test] + async fn persist_install_outputs_direct_writes_private_server_env_permissions() { use std::os::unix::fs::PermissionsExt; let dir = tempfile::tempdir().unwrap(); @@ -1568,6 +1563,7 @@ stale = "remove-me" &[], None, ) + .await .expect("initial env write should succeed"); let create_mode = std::fs::metadata(&env_path).unwrap().permissions().mode() & 0o777; assert_eq!(create_mode, 0o600); @@ -1583,6 +1579,7 @@ stale = "remove-me" &[], None, ) + .await .expect("rewrite env write should succeed"); let update_mode = std::fs::metadata(&env_path).unwrap().permissions().mode() & 0o777; assert_eq!(update_mode, 0o600); diff --git a/lib/crates/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs b/lib/crates/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs index c96563ff0..322ded40d 100644 --- a/lib/crates/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs +++ b/lib/crates/fabro-server/migrations/2026052501_optional_server_env_secrets_to_vault.rs @@ -15,7 +15,9 @@ use std::path::{Path, PathBuf}; use anyhow::Context as _; use fabro_config::envfile::{self, EnvFileRemoval}; use fabro_static::{EnvVars, optional_vault_secrets}; -use fabro_vault::{SecretType, Vault}; +#[cfg(test)] +use fabro_vault::Vault; +use fabro_vault::{SecretStore, SecretStoreWrite, SecretType}; pub(crate) const REMOVAL_DEADLINE: &str = "2026-08-18"; @@ -34,6 +36,7 @@ impl OptionalServerEnvSecretsMigrationReport { } } +#[cfg(test)] pub(crate) fn migrate( vault: &mut Vault, server_env_path: &Path, @@ -121,6 +124,92 @@ pub(crate) fn migrate( Ok(report) } +pub(crate) async fn migrate_to_store( + store: &SecretStore, + server_env_path: &Path, + env_entries: &HashMap, +) -> anyhow::Result { + let server_env_entries = envfile::read_env_file(server_env_path) + .with_context(|| format!("read server env file {}", server_env_path.display()))?; + let mut writes = Vec::new(); + let mut env_removals = Vec::new(); + let mut warnings = Vec::new(); + let mut preserved_env_entries = 0; + + for &name in optional_vault_secrets() { + let process_value = env_entries.get(name); + let file_value = server_env_entries.get(name); + if let Some(entry) = store.get(name).await? { + if let Some(file_value) = file_value { + if file_value == &entry.value { + env_removals.push(env_removal(name)); + } else { + preserved_env_entries += 1; + warnings.push(format!( + "Preserved {name} in server.env because the secret store already contains a different value" + )); + } + } + continue; + } + + let value = match (process_value, file_value) { + (Some(value), Some(file_value)) => { + if value == file_value { + env_removals.push(env_removal(name)); + } else { + preserved_env_entries += 1; + warnings.push(format!( + "Preserved {name} in server.env because process env takes precedence and the file value differs" + )); + } + Some(value) + } + (Some(value), None) => Some(value), + (None, Some(value)) => { + env_removals.push(env_removal(name)); + Some(value) + } + (None, None) => None, + }; + if let Some(value) = value { + writes.push(SecretStoreWrite { + name: name.to_string(), + value: value.clone(), + secret_type: secret_type_for(name), + description: None, + }); + } + } + + let mut report = OptionalServerEnvSecretsMigrationReport { + migrated_secrets: writes.len(), + removed_env_entries: 0, + preserved_env_entries, + backup_path: None, + warnings, + }; + if writes.is_empty() && env_removals.is_empty() { + return Ok(report); + } + + store.apply(&[], &writes).await?; + if !env_removals.is_empty() { + let backup_path = backup_server_env_file(server_env_path)?; + let update_report = + envfile::update_env_file_with_report(server_env_path, env_removals, Vec::new()) + .with_context(|| { + format!( + "remove migrated optional secrets from {}", + server_env_path.display() + ) + })?; + report.removed_env_entries = update_report.removed_keys.len(); + report.backup_path = Some(backup_path); + } + Ok(report) +} + fn secret_type_for(name: &str) -> SecretType { if name == EnvVars::GITHUB_APP_PRIVATE_KEY { SecretType::File diff --git a/lib/crates/fabro-server/src/automation_materializer.rs b/lib/crates/fabro-server/src/automation_materializer.rs index c63c8bb04..c7d808b33 100644 --- a/lib/crates/fabro-server/src/automation_materializer.rs +++ b/lib/crates/fabro-server/src/automation_materializer.rs @@ -41,6 +41,8 @@ pub(crate) enum RunMaterializeError { WorkflowNotFound(String), #[error("failed to build run manifest: {0}")] Manifest(String), + #[error("failed to load GitHub credentials: {0}")] + Credentials(String), } impl From for RunMaterializeError { diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 0c5d7b4b0..b88f93716 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -89,14 +89,14 @@ fn validate_session_secret(value: &str) -> Result<(), String> { } pub async fn run_all(state: &AppState) -> DiagnosticsReport { - let (llm, github, docker_sandbox, cloud_sandbox, brave) = tokio::join!( + let (llm, github, docker_sandbox, cloud_sandbox, brave, crypto) = tokio::join!( check_llm_providers(state), check_github_app(state), check_docker_sandbox(state), check_cloud_sandbox(state), check_brave_search(state), + check_crypto(state), ); - let crypto = check_crypto(state); DiagnosticsReport { version: FABRO_VERSION.to_string(), @@ -332,7 +332,10 @@ fn short_error_line(rendered: &str) -> String { async fn check_github_app(state: &AppState) -> CheckResult { let settings = state.server_settings(); if settings.server.integrations.github.strategy == GithubIntegrationStrategy::Token { - let token = match state.github_credentials(&settings.server.integrations.github) { + let token = match state + .github_credentials(&settings.server.integrations.github) + .await + { Ok(Some(fabro_github::GitHubCredentials::Pat(token))) => token, Ok(Some(fabro_github::GitHubCredentials::Installation(token))) => { match token.valid_token() { @@ -364,12 +367,13 @@ async fn check_github_app(state: &AppState) -> CheckResult { }; } Err(err) => { + let rendered = format!("{err:#}"); return CheckResult { name: "GitHub Token".to_string(), status: CheckStatus::Error, summary: "missing token".to_string(), - details: vec![CheckDetail::new(err.clone())], - remediation: Some(err), + details: vec![CheckDetail::new(rendered.clone())], + remediation: Some(rendered), }; } }; @@ -445,14 +449,22 @@ async fn check_github_app(state: &AppState) -> CheckResult { let app_id = settings.server.integrations.github.app_id.clone(); let slug = settings.server.integrations.github.slug.clone(); - let private_key_raw = state.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); + let private_key_raw = + match diagnostic_secret(state, "GitHub App", EnvVars::GITHUB_APP_PRIVATE_KEY).await { + Ok(value) => value, + Err(result) => return result, + }; let client_id = settings.server.integrations.github.client_id.is_some(); - let client_secret = state - .vault_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) - .is_some(); - let webhook_secret = state - .vault_secret(EnvVars::GITHUB_APP_WEBHOOK_SECRET) - .is_some(); + let client_secret = + match diagnostic_secret(state, "GitHub App", EnvVars::GITHUB_APP_CLIENT_SECRET).await { + Ok(value) => value.is_some(), + Err(result) => return result, + }; + let webhook_secret = + match diagnostic_secret(state, "GitHub App", EnvVars::GITHUB_APP_WEBHOOK_SECRET).await { + Ok(value) => value.is_some(), + Err(result) => return result, + }; if app_id.is_none() && private_key_raw.is_none() @@ -622,7 +634,11 @@ fn docker_sandbox_probe_check(probe: Result<(), String>) -> CheckResult { } async fn check_cloud_sandbox(state: &AppState) -> CheckResult { - let Some(api_key) = state.vault_secret(EnvVars::DAYTONA_API_KEY) else { + let api_key = match diagnostic_secret(state, "Cloud Sandbox", EnvVars::DAYTONA_API_KEY).await { + Ok(value) => value, + Err(result) => return result, + }; + let Some(api_key) = api_key else { return CheckResult { name: "Cloud Sandbox".to_string(), status: CheckStatus::Warning, @@ -707,7 +723,12 @@ fn check_storage_dir_path(path: &std::path::Path) -> CheckResult { } async fn check_brave_search(state: &AppState) -> CheckResult { - let Some(api_key) = state.vault_secret(EnvVars::BRAVE_SEARCH_API_KEY) else { + let api_key = + match diagnostic_secret(state, "Web Search (Brave)", EnvVars::BRAVE_SEARCH_API_KEY).await { + Ok(value) => value, + Err(result) => return result, + }; + let Some(api_key) = api_key else { return CheckResult { name: "Web Search (Brave)".to_string(), status: CheckStatus::Warning, @@ -767,7 +788,7 @@ async fn check_brave_search(state: &AppState) -> CheckResult { } } -fn check_crypto(state: &AppState) -> CheckResult { +async fn check_crypto(state: &AppState) -> CheckResult { let resolved_server_settings = state.server_settings(); let mut details = Vec::new(); @@ -802,10 +823,12 @@ fn check_crypto(state: &AppState) -> CheckResult { { errors.push("server.integrations.github.client_id is not configured".to_string()); } - if state - .vault_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) - .is_none() - { + let client_secret = + match diagnostic_secret(state, "Crypto", EnvVars::GITHUB_APP_CLIENT_SECRET).await { + Ok(value) => value, + Err(result) => return result, + }; + if client_secret.is_none() { errors.push("GITHUB_APP_CLIENT_SECRET not configured in vault".to_string()); } } @@ -832,6 +855,20 @@ fn check_crypto(state: &AppState) -> CheckResult { } } +async fn diagnostic_secret( + state: &AppState, + check_name: &str, + name: &str, +) -> Result, CheckResult> { + state.vault_secret(name).await.map_err(|err| CheckResult { + name: check_name.to_string(), + status: CheckStatus::Error, + summary: "secret store unavailable".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some("Check the Fabro database and retry".to_string()), + }) +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -888,14 +925,13 @@ mod tests { state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let result = check_llm_providers(&state).await; @@ -970,14 +1006,13 @@ mod tests { state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let result = check_llm_providers(&state).await; @@ -1132,8 +1167,8 @@ enabled = false ); } - #[test] - fn check_crypto_requires_github_client_secret_from_vault() { + #[tokio::test] + async fn check_crypto_requires_github_client_secret_from_vault() { let settings = fabro_config::ServerSettingsBuilder::from_toml( r#" _version = 1 @@ -1157,7 +1192,7 @@ client_id = "Iv1.test" )])) .build(); - let result = check_crypto(&state); + let result = check_crypto(&state).await; assert_eq!(result.status, CheckStatus::Error); assert!(result.details.iter().any(|detail| { diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 7afb7a8b2..a47b76557 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -19,7 +19,7 @@ use fabro_config::envfile::{EnvFileRemoval, EnvFileUpdate}; use fabro_install::{ GITHUB_APP_VAULT_KEYS, GITHUB_INSTALL_SECRET_KEYS, InstallListenConfig, InstallPersistencePlan, InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, - PendingSettingsWrite, VaultSecretWrite, merge_server_settings, + PendingSettingsWrite, SecretStoreWrite, merge_server_settings, prepare_dev_token_write_for_install, seed_default_environment_in_storage, write_github_app_settings, write_object_store_settings, write_sandbox_settings, write_token_settings, @@ -1573,7 +1573,7 @@ async fn post_install_finish( } let mut vault_secrets = Vec::new(); if let InstallSandboxProviderState::Daytona { api_key } = &sandbox.provider { - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name: EnvVars::DAYTONA_API_KEY.to_string(), value: api_key.expose_secret().to_string(), secret_type: VaultSecretType::Token, @@ -1585,7 +1585,7 @@ async fn post_install_finish( Ok(name) => name, Err(err) => return install_error_response(StatusCode::UNPROCESSABLE_ENTITY, err), }; - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name, value: provider.api_key, secret_type: VaultSecretType::Token, @@ -1612,7 +1612,7 @@ async fn post_install_finish( if let Err(err) = write_token_settings(&mut settings_doc) { return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); } - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name: EnvVars::GITHUB_TOKEN.to_string(), value: github.token, secret_type: VaultSecretType::Token, @@ -1649,20 +1649,20 @@ async fn post_install_finish( ) { return install_error_response(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()); } - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name: EnvVars::GITHUB_APP_PRIVATE_KEY.to_string(), value: BASE64_STANDARD.encode(github.pem.as_bytes()), secret_type: VaultSecretType::File, description: None, }); - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name: EnvVars::GITHUB_APP_CLIENT_SECRET.to_string(), value: github.client_secret, secret_type: VaultSecretType::Token, description: None, }); if let Some(secret) = github.webhook_secret { - vault_secrets.push(VaultSecretWrite { + vault_secrets.push(SecretStoreWrite { name: EnvVars::GITHUB_APP_WEBHOOK_SECRET.to_string(), value: secret, secret_type: VaultSecretType::Token, @@ -1713,7 +1713,7 @@ async fn post_install_finish( vault_writes: vault_secrets, vault_removals, }; - if let Err(err) = persistence_plan.persist_direct() { + if let Err(err) = persistence_plan.persist_direct().await { error!(error = %err, "install persistence failed"); let status = StatusCode::INTERNAL_SERVER_ERROR; let detail = err.to_string(); @@ -2502,7 +2502,12 @@ mod tests { assert!(!server_env.contains_key(EnvVars::GITHUB_APP_CLIENT_SECRET)); assert!(!server_env.contains_key(EnvVars::GITHUB_APP_WEBHOOK_SECRET)); - let vault = fabro_vault::Vault::load(storage.secrets_path()).unwrap(); + let vault = fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .unwrap() + .snapshot() + .await + .unwrap(); assert_eq!(vault.get(EnvVars::GITHUB_TOKEN), None); assert_eq!( vault.get(EnvVars::GITHUB_APP_CLIENT_SECRET), diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index 42c6fad15..5fc735ba0 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -57,4 +57,4 @@ mod worker_token; pub use error::{ApiError, Error, Result}; pub use run_manifest::workflow_bundle_from_manifest; pub use server_secrets::process_env_snapshot; -pub use startup::{load_startup_vault, validate_startup, validate_startup_configuration}; +pub use startup::{migrate_startup_vault, validate_startup, validate_startup_configuration}; diff --git a/lib/crates/fabro-server/src/migrations.rs b/lib/crates/fabro-server/src/migrations.rs index ba5745c03..986619184 100644 --- a/lib/crates/fabro-server/src/migrations.rs +++ b/lib/crates/fabro-server/src/migrations.rs @@ -1,6 +1,8 @@ use std::collections::HashMap; use std::path::Path; +use fabro_vault::SecretStore; +#[cfg(test)] use fabro_vault::Vault; #[path = "../migrations/2026051801_legacy_vault_entries.rs"] @@ -19,6 +21,7 @@ pub(crate) fn migrate_legacy_vault_file(path: &Path) -> anyhow::Result anyhow::Result { optional_server_env_secrets_to_vault::migrate(vault, server_env_path, env_entries) } + +pub(crate) async fn migrate_optional_server_env_secrets_to_store( + store: &SecretStore, + server_env_path: &Path, + env_entries: &HashMap, +) -> anyhow::Result { + optional_server_env_secrets_to_vault::migrate_to_store(store, server_env_path, env_entries) + .await +} diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 26fd4444a..ebe3f7f53 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -1217,7 +1217,10 @@ async fn reconnect_run_sandbox( .and_then(fabro_types::RunSandbox::instance) .cloned() .ok_or_else(|| ApiError::new(StatusCode::NOT_FOUND, "Run sandbox was not created."))?; - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = state + .vault_secret(EnvVars::DAYTONA_API_KEY) + .await + .map_err(|err| ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()))?; let sandbox = reconnect_for_run(&record, daytona_api_key, Some(*run_id)) .await .map_err(|err| ApiError::new(StatusCode::CONFLICT, err.to_string()))?; diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index d7794be48..93c6660c6 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -516,14 +516,22 @@ async fn build_preflight_report( let needs_github_credentials = sandbox_provider.is_clone_based() || resolved_run.integrations.github.is_token_requested(); let github_app = if needs_github_credentials { - state - .github_credentials(github_integration) - .unwrap_or_default() + match state.github_credentials(github_integration).await { + Ok(credentials) => credentials, + Err(err) + if err + .downcast_ref::() + .is_some() => + { + return Err(err); + } + Err(_) => None, + } } else { None }; - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY).await?; let sandbox_ok = run_sandbox_check( &mut checks, sandbox_provider, @@ -2326,14 +2334,13 @@ id = "daytona" state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "test-openai-key", fabro_vault::SecretType::Token, None, ) + .await .unwrap(); let mut manifest = minimal_manifest(); diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index d6d33f99c..642fa9435 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -39,8 +39,8 @@ use crate::server::{ spawn_automation_scheduler, spawn_scheduler, }; use crate::server_secrets::{ServerSecrets, process_env_snapshot}; -use crate::startup::{prepare_startup_vault, resolve_startup, validate_startup_configuration}; -use crate::static_files; +use crate::startup::{migrate_startup_vault, resolve_startup, validate_startup_configuration}; +use crate::{migrations, static_files}; pub const DEFAULT_TCP_PORT: u16 = 32276; type EnvLookup = Arc Option + Send + Sync>; @@ -256,7 +256,7 @@ enum WebhookPreconditions { Skip(String), } -fn resolve_webhook_preconditions( +async fn resolve_webhook_preconditions( github: &GithubIntegrationSettings, state: &Arc, webhook_secret_present: bool, @@ -272,7 +272,7 @@ fn resolve_webhook_preconditions( "server.integrations.github.app_id is not set".to_string(), ); }; - let github_app = match state.github_credentials(github) { + let github_app = match state.github_credentials(github).await { Ok(creds) => creds, Err(err) => { return WebhookPreconditions::Skip(format!("GitHub credentials are invalid: {err}")); @@ -312,7 +312,7 @@ async fn start_webhook_strategy( }; let (app_id, private_key_pem) = - match resolve_webhook_preconditions(github, state, webhook_secret_present) { + match resolve_webhook_preconditions(github, state, webhook_secret_present).await { WebhookPreconditions::Ready { app_id, private_key_pem, @@ -666,14 +666,7 @@ where let resolved_server_settings = resolved_app_settings.server_settings.server.clone(); validate_startup_configuration(&resolved_server_settings)?; let env_entries = process_env_snapshot(); - let startup_vault = prepare_startup_vault(&vault_path, &server_env_path, &env_entries)?; - let (auth_mode, server_secrets) = resolve_startup( - &server_env_path, - env_entries, - &resolved_server_settings, - &startup_vault, - )?; - let webhook_secret_present = startup_vault.get(WEBHOOK_SECRET_ENV).is_some(); + migrate_startup_vault(&vault_path); let bind_request = resolve_bind_request_from_server_settings( &resolved_app_settings.server_settings, args.bind.as_deref(), @@ -683,6 +676,45 @@ where .with_context(|| format!("creating data directory {}", data_dir.display()))?; let database = fabro_db::Database::connect(&sqlite_path).await?; database.migrate().await?; + fabro_vault::import_legacy_json_once(database.pool(), &vault_path) + .await + .with_context(|| format!("importing legacy secrets file {}", vault_path.display()))?; + let secret_store = fabro_vault::SecretStore::new(database.clone_pool()); + let optional_report = migrations::migrate_optional_server_env_secrets_to_store( + &secret_store, + &server_env_path, + &env_entries, + ) + .await + .context("migrate optional server env secrets into SQLite")?; + for warning in &optional_report.warnings { + warn!( + warning = %warning, + removal_deadline = migrations::OPTIONAL_SERVER_ENV_SECRETS_REMOVAL_DEADLINE, + "Optional server env secrets migration warning" + ); + } + if optional_report.changed() { + warn!( + migrated_secrets = optional_report.migrated_secrets, + removed_env_entries = optional_report.removed_env_entries, + preserved_env_entries = optional_report.preserved_env_entries, + backup_path = ?optional_report.backup_path, + removal_deadline = migrations::OPTIONAL_SERVER_ENV_SECRETS_REMOVAL_DEADLINE, + "Migrated optional server env secrets into SQLite" + ); + } + let startup_vault = secret_store + .snapshot() + .await + .context("loading startup secret snapshot")?; + let (auth_mode, server_secrets) = resolve_startup( + &server_env_path, + env_entries, + &resolved_server_settings, + &startup_vault, + )?; + let webhook_secret_present = startup_vault.get(WEBHOOK_SECRET_ENV).is_some(); fabro_variable::import_legacy_json_once(database.pool(), &variables_path) .await .with_context(|| { @@ -758,9 +790,8 @@ where max_concurrent_runs, store, artifact_store, - vault_path, db_pool, - preloaded_vault: Some(startup_vault), + preloaded_vault: startup_vault.into_vault(), server_secrets, env_lookup, github_api_base_url: None, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 8ba881fe1..cfba70bd7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -49,7 +49,7 @@ pub use fabro_api::types::{ TimelineEntryResponse, UpdateVariableRequest, VariableListResponse, VncPreviewResponse, WriteBlobResponse, }; -use fabro_auth::{CredentialSource, VaultCredentialSource, auth_issue_message}; +use fabro_auth::{CredentialSource, SqlVaultCredentialSource, auth_issue_message}; use fabro_automation::AutomationStore; use fabro_config::daemon::ServerDaemon; use fabro_config::{RunLayer, Storage, WorkflowSettingsBuilder}; @@ -106,7 +106,7 @@ use fabro_util::error::{ }; use fabro_util::version::FABRO_VERSION; use fabro_variable::{Error as VariableError, VariableStore}; -use fabro_vault::{Error as VaultError, SecretType, Vault}; +use fabro_vault::{SecretStore, SecretStoreError, SecretType, Vault}; use fabro_workflow::artifact_upload::ArtifactSink; #[cfg(test)] use fabro_workflow::command_log::command_log_path; @@ -163,7 +163,6 @@ use crate::request_id::{self, RequestId}; use crate::run_files::{FilesInFlight, new_files_in_flight}; use crate::server_secrets::{LlmClientResult, ServerSecrets}; use crate::spawn_env::apply_render_graph_env; -use crate::startup::load_startup_vault; use crate::worker_control::{LocalWorkerControlBus, WorkerControlBus, WorkerControlBusError}; use crate::worker_runtime::{ LocalWorkerRuntime, WorkerExit, WorkerLaunchSpec, WorkerRef, WorkerRuntime, @@ -1104,6 +1103,7 @@ pub struct AppState { registry_factory_override: Option>, slack_service: Option>, slack_started: AtomicBool, + github_webhook_secret: Option, } pub(crate) struct AppStores { @@ -1111,7 +1111,7 @@ pub(crate) struct AppStores { pub(crate) automations: Arc, pub(crate) environments: Arc, pub(crate) mcp_servers: Arc, - pub(crate) vault: Arc>, + pub(crate) vault: Arc, pub(crate) variables: Arc, } @@ -1142,8 +1142,8 @@ impl AppState { let settings = self.server_settings(); let credentials = self .github_credentials(&settings.server.integrations.github) - .ok() - .flatten(); + .await + .map_err(|err| RunMaterializeError::Credentials(err.to_string()))?; ProductionAutomationRunMaterializer::new( credentials, self.github_api_base_url.clone(), @@ -1250,9 +1250,8 @@ pub(crate) struct AppStateConfig { pub(crate) max_concurrent_runs: usize, pub(crate) store: Arc, pub(crate) artifact_store: ArtifactStore, - pub(crate) vault_path: PathBuf, pub(crate) db_pool: DbPool, - pub(crate) preloaded_vault: Option, + pub(crate) preloaded_vault: Vault, pub(crate) server_secrets: ServerSecrets, pub(crate) env_lookup: EnvLookup, pub(crate) github_api_base_url: Option, @@ -1440,12 +1439,15 @@ impl AppState { AskFabroReadiness { default_model } } - pub(crate) fn vault_secret(&self, name: &str) -> Option { + pub(crate) async fn vault_secret( + &self, + name: &str, + ) -> Result, SecretStoreError> { self.stores .vault - .try_read() - .ok() - .and_then(|vault| vault.get(name).map(str::to_string)) + .get(name) + .await + .map(|entry| entry.map(|entry| entry.value)) } pub(crate) fn config_env_lookup(&self, name: &str) -> Option { @@ -1522,20 +1524,24 @@ impl AppState { .and_then(|value| auth::derive_cookie_key(value.as_bytes()).ok()) } - pub(crate) fn github_credentials( + pub(crate) async fn github_credentials( &self, settings: &GithubIntegrationSettings, - ) -> Result, String> { + ) -> anyhow::Result> { match settings.strategy { GithubIntegrationStrategy::App => { let Some(app_id) = settings.app_id.clone() else { return Ok(None); }; - let raw = self.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY); + let raw = self + .vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY) + .await + .map_err(anyhow::Error::new)?; let Some(raw) = raw else { return Ok(None); }; - let private_key_pem = decode_secret_pem(EnvVars::GITHUB_APP_PRIVATE_KEY, &raw)?; + let private_key_pem = decode_secret_pem(EnvVars::GITHUB_APP_PRIVATE_KEY, &raw) + .map_err(anyhow::Error::msg)?; Ok(Some(fabro_github::GitHubCredentials::App( fabro_github::GitHubAppCredentials { app_id, @@ -1547,6 +1553,8 @@ impl AppState { GithubIntegrationStrategy::Token => { let token = self .vault_secret(EnvVars::GITHUB_TOKEN) + .await + .map_err(anyhow::Error::new)? .as_deref() .map(str::trim) .filter(|token| !token.is_empty()) @@ -1554,12 +1562,11 @@ impl AppState { match token { Some(token) => { fabro_github::validate_static_github_token(&token) - .map_err(|err| err.to_string())?; + .map_err(anyhow::Error::msg)?; Ok(Some(fabro_github::GitHubCredentials::Pat(token))) } - None => Err( + None => anyhow::bail!( "GITHUB_TOKEN not configured -- run fabro install or run fabro secret set GITHUB_TOKEN" - .to_string(), ), } } @@ -1758,7 +1765,7 @@ pub fn build_router_with_options( let state_for_canonical_host = Arc::clone(&state); let github_endpoints = github_endpoints.unwrap_or_else(|| Arc::new(GithubEndpoints::production_defaults())); - let webhook_secret = state.vault_secret(WEBHOOK_SECRET_ENV); + let webhook_secret = state.github_webhook_secret.clone(); let principal_layer = middleware::from_fn_with_state(Arc::clone(&state), principal_middleware); let api_common = if web_enabled { Router::new() @@ -2345,7 +2352,6 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result vault, - None => load_startup_vault(&vault_path)?, - }; + let variables = Arc::new(VariableStore::new(db_pool.clone())); + let secret_store = Arc::new(SecretStore::new(db_pool)); + let vault = preloaded_vault; // Read vault secrets needed for synchronous setup before we wrap the vault in // an async lock for the rest of AppState. let daytona_api_key = vault.get(EnvVars::DAYTONA_API_KEY).map(str::to_string); - let vault = Arc::new(AsyncRwLock::new(vault)); - let llm_source: Arc = - Arc::new(VaultCredentialSource::vault_only(Arc::clone(&vault))); + let llm_source: Arc = Arc::new(SqlVaultCredentialSource::vault_only( + Arc::clone(&secret_store), + )); let (global_event_tx, _) = broadcast::channel(4096); let current_server_settings = Arc::new(resolved_settings.server_settings); let current_effective_web_url = @@ -2423,11 +2427,8 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result { info!( @@ -2489,7 +2490,7 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result sandbox, Err(err) if force || delete_started => { @@ -3585,6 +3592,7 @@ fn worker_launch_spec( mode: RunExecutionMode, run_dir: &std::path::Path, agent_fabro_tools_enabled: bool, + github_app_private_key: Option, ) -> anyhow::Result { let current_exe = std::env::current_exe().context("reading current executable path")?; let executable = @@ -3622,7 +3630,7 @@ fn worker_launch_spec( log_destination, fabro_log, active_config_path: state.active_config_path().to_path_buf(), - github_app_private_key: state.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY), + github_app_private_key, }) } @@ -3990,9 +3998,9 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { let pull_request_can_use_github_credentials = settings.execution.mode != RunMode::DryRun && settings.pull_request.is_some(); if settings.integrations.github.is_token_requested() { - state.github_credentials(github_settings) + state.github_credentials(github_settings).await } else if clone_can_use_github_credentials || pull_request_can_use_github_credentials { - match state.github_credentials(github_settings) { + match state.github_credentials(github_settings).await { Ok(github_app) => Ok(github_app), Err(err) => { tracing::warn!( @@ -4032,6 +4040,20 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { .integrations .github .resolve_permissions(process_env_var); + let vault = match state.stores.vault.snapshot().await { + Ok(vault) => vault, + Err(err) => { + tracing::error!(run_id = %run_id, error = ?err, "Loading run secrets failed"); + fail_run_before_execution( + &state, + run_id, + FailureReason::WorkflowError, + "Loading run secrets failed".to_string(), + ) + .await; + return; + } + }; let services = operations::StartServices { run_id, cancel_token: cancel_token.clone(), @@ -4044,7 +4066,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { run_control: None, github_app, github_permissions, - vault: Some(Arc::clone(&state.stores.vault)), + vault: Some(Arc::new(AsyncRwLock::new(vault.into_vault()))), catalog: state.catalog(), on_node: None, registry_override, @@ -4214,6 +4236,20 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { return; } + let github_app_private_key = match state.vault_secret(EnvVars::GITHUB_APP_PRIVATE_KEY).await { + Ok(value) => value, + Err(err) => { + fail_run_before_execution( + &state, + run_id, + FailureReason::WorkflowError, + "Loading worker secrets failed".to_string(), + ) + .await; + tracing::error!(run_id = %run_id, error = ?err, "Loading worker secrets failed"); + return; + } + }; let state_for_build = Arc::clone(&state); let run_dir_for_build = run_dir.clone(); let start_result = spawn_blocking(move || { @@ -4223,6 +4259,7 @@ async fn execute_run_subprocess(state: Arc, run_id: RunId) { execution_mode, &run_dir_for_build, agent_fabro_tools_enabled, + github_app_private_key, ) }) .await diff --git a/lib/crates/fabro-server/src/server/handler/pull_requests.rs b/lib/crates/fabro-server/src/server/handler/pull_requests.rs index 069c8e730..b23af4d82 100644 --- a/lib/crates/fabro-server/src/server/handler/pull_requests.rs +++ b/lib/crates/fabro-server/src/server/handler/pull_requests.rs @@ -65,11 +65,14 @@ fn pull_request_record_from_link_request( }) } -fn load_server_github_credentials( +async fn load_server_github_credentials( state: &AppState, ) -> Result { let settings = state.server_settings(); - match state.github_credentials(&settings.server.integrations.github) { + match state + .github_credentials(&settings.server.integrations.github) + .await + { Ok(Some(creds)) => Ok(creds), Ok(None) => { warn!("GitHub integration unavailable on server: credentials not configured"); @@ -157,7 +160,7 @@ async fn load_pull_request_github_context( ) -> Result { let record = load_pull_request_record(state, id).await?; let (owner, repo, number) = github_coordinates_for_record(&record); - let creds = load_server_github_credentials(state.as_ref())?; + let creds = load_server_github_credentials(state.as_ref()).await?; Ok(PullRequestGithubContext { record, owner, @@ -305,7 +308,7 @@ async fn create_run_pull_request( Ok(inputs) => inputs, Err(err) => return err.into_response(), }; - let creds = match load_server_github_credentials(state.as_ref()) { + let creds = match load_server_github_credentials(state.as_ref()).await { Ok(creds) => creds, Err(err) => return err.into_response(), }; @@ -429,7 +432,7 @@ async fn get_run_pull_request( Err(err) => return err.into_response(), }; let (owner, repo, number) = github_coordinates_for_record(&record); - let creds = match load_server_github_credentials(state.as_ref()) { + let creds = match load_server_github_credentials(state.as_ref()).await { Ok(creds) => creds, Err(err) => { warn!(error = ?err, "Returning stored pull request without live GitHub details"); diff --git a/lib/crates/fabro-server/src/server/handler/sandbox.rs b/lib/crates/fabro-server/src/server/handler/sandbox.rs index f86041cb4..ab12a27a1 100644 --- a/lib/crates/fabro-server/src/server/handler/sandbox.rs +++ b/lib/crates/fabro-server/src/server/handler/sandbox.rs @@ -109,7 +109,10 @@ async fn retrieve_run_sandbox( Ok(record) => record, Err(response) => return response, }; - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = match load_daytona_api_key(&state).await { + Ok(value) => value, + Err(response) => return response, + }; let daytona_organization_id = state.config_env_lookup(EnvVars::DAYTONA_ORGANIZATION_ID); match sandbox_details(&record, daytona_api_key, daytona_organization_id, Some(id)).await { Ok(details) => Json::(details).into_response(), @@ -228,7 +231,19 @@ async fn terminal_websocket(mut socket: WebSocket, state: Arc, id: Run return; } }; - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = match load_daytona_api_key(&state).await { + Ok(value) => value, + Err(response) => { + let _ = socket + .send(terminal_server_text( + "error", + Some("Secret store unavailable."), + )) + .await; + tracing::error!(status = %response.status(), "Loading Daytona API key failed"); + return; + } + }; let daytona_organization_id = state.config_env_lookup(EnvVars::DAYTONA_ORGANIZATION_ID); let session = match open_terminal_for_run( &record, @@ -859,7 +874,7 @@ async fn reconnect_run_sandbox_instance( run_id: &RunId, record: &RunSandboxInstance, ) -> Result, Response> { - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = load_daytona_api_key(state).await?; let sandbox = reconnect_for_run(record, daytona_api_key, Some(*run_id)) .await .map_err(|err| { @@ -899,7 +914,7 @@ async fn reconnect_daytona_sandbox_instance( ) .into_response()); }; - let daytona_api_key = state.vault_secret(EnvVars::DAYTONA_API_KEY); + let daytona_api_key = load_daytona_api_key(state).await?; let sandbox = DaytonaSandbox::reconnect( &runtime.id, daytona_api_key, @@ -918,6 +933,20 @@ async fn reconnect_daytona_sandbox_instance( Ok(sandbox) } +async fn load_daytona_api_key(state: &AppState) -> Result, Response> { + state + .vault_secret(EnvVars::DAYTONA_API_KEY) + .await + .map_err(|err| { + tracing::error!(error = ?err, "Loading Daytona API key failed"); + ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "secret store operation failed", + ) + .into_response() + }) +} + async fn load_run_sandbox_instance( state: &Arc, run_id: &RunId, diff --git a/lib/crates/fabro-server/src/server/handler/secrets.rs b/lib/crates/fabro-server/src/server/handler/secrets.rs index 6ea1b3291..0336e6f7b 100644 --- a/lib/crates/fabro-server/src/server/handler/secrets.rs +++ b/lib/crates/fabro-server/src/server/handler/secrets.rs @@ -5,7 +5,7 @@ use fabro_static::EnvVars; use super::super::{ ApiError, AppState, CreateSecretRequest, DeleteSecretRequest, IntoResponse, Json, RequiredUser, - Response, Router, SecretType, State, StatusCode, VaultError, get, spawn_blocking, + Response, Router, SecretStoreError, SecretType, State, StatusCode, get, }; pub(super) fn routes() -> Router> { @@ -18,8 +18,10 @@ pub(super) fn routes() -> Router> { } async fn list_secrets(_auth: RequiredUser, State(state): State>) -> Response { - let data = state.stores.vault.read().await.list(); - (StatusCode::OK, Json(serde_json::json!({ "data": data }))).into_response() + match state.stores.vault.list().await { + Ok(data) => (StatusCode::OK, Json(serde_json::json!({ "data": data }))).into_response(), + Err(err) => secret_store_error(&err), + } } async fn create_secret( @@ -59,34 +61,20 @@ async fn create_secret( } } } - let state_for_write = Arc::clone(&state); - let result = spawn_blocking(move || { - let mut vault = state_for_write.stores.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(VaultError::InvalidName(_))) => { + match state + .stores + .vault + .set(&name, &value, secret_type, description.as_deref()) + .await + { + Ok(meta) => (StatusCode::OK, Json(meta)).into_response(), + Err(SecretStoreError::InvalidName(_)) => { ApiError::bad_request("invalid secret name").into_response() } - Ok(Err(VaultError::Io(err))) => { - ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() + Err(SecretStoreError::InvalidOauth { .. }) => { + ApiError::bad_request("invalid oauth credential JSON").into_response() } - Ok(Err(VaultError::Serde(err))) => { - ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() - } - Ok(Err(VaultError::NotFound(_))) => ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - "secret unexpectedly missing", - ) - .into_response(), - Err(err) => ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - format!("secret write task failed: {err}"), - ) - .into_response(), + Err(err) => secret_store_error(&err), } } @@ -96,32 +84,21 @@ async fn delete_secret_by_name( Json(body): Json, ) -> Response { let name = body.name; - let state_for_write = Arc::clone(&state); - let result = spawn_blocking(move || { - let mut vault = state_for_write.stores.vault.blocking_write(); - vault.remove(&name) - }) - .await; - - match result { - Ok(Ok(())) => StatusCode::NO_CONTENT.into_response(), - Ok(Err(VaultError::InvalidName(_))) => { - ApiError::bad_request("invalid secret name").into_response() - } - Ok(Err(VaultError::NotFound(name))) => { + match state.stores.vault.remove(&name).await { + Ok(()) => StatusCode::NO_CONTENT.into_response(), + Err(SecretStoreError::NotFound(name)) => { ApiError::new(StatusCode::NOT_FOUND, format!("secret not found: {name}")) .into_response() } - Ok(Err(VaultError::Io(err))) => { - ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() - } - Ok(Err(VaultError::Serde(err))) => { - ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.to_string()).into_response() - } - Err(err) => ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - format!("secret delete task failed: {err}"), - ) - .into_response(), + Err(err) => secret_store_error(&err), } } + +fn secret_store_error(err: &SecretStoreError) -> Response { + tracing::error!(error = ?err, "Secret store operation failed"); + ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "secret store operation failed", + ) + .into_response() +} diff --git a/lib/crates/fabro-server/src/server/handler/sessions.rs b/lib/crates/fabro-server/src/server/handler/sessions.rs index 3ac297f07..3e24a1f9d 100644 --- a/lib/crates/fabro-server/src/server/handler/sessions.rs +++ b/lib/crates/fabro-server/src/server/handler/sessions.rs @@ -705,13 +705,13 @@ async fn build_agent_session( let sandbox_instance = sandbox_record.instance().ok_or_else(|| { AskFabroBuildError::SandboxUnavailable(anyhow::anyhow!("run sandbox was not created")) })?; - let sandbox = reconnect_for_run( - sandbox_instance, - state.vault_secret(EnvVars::DAYTONA_API_KEY), - Some(run_id), - ) - .await - .map_err(AskFabroBuildError::SandboxUnavailable)?; + let daytona_api_key = state + .vault_secret(EnvVars::DAYTONA_API_KEY) + .await + .map_err(|err| AskFabroBuildError::Agent(anyhow::Error::new(err)))?; + let sandbox = reconnect_for_run(sandbox_instance, daytona_api_key, Some(run_id)) + .await + .map_err(AskFabroBuildError::SandboxUnavailable)?; let sandbox: Arc = Arc::from(sandbox); let mut profile = build_profile( provider_id, @@ -752,11 +752,15 @@ async fn build_agent_session( let profile: Arc = Arc::new(AskFabroProfile::new(profile, Arc::clone(&ask_fabro_policy))); + let brave_search_api_key = state + .vault_secret(EnvVars::BRAVE_SEARCH_API_KEY) + .await + .map_err(|err| AskFabroBuildError::Agent(anyhow::Error::new(err)))?; let config = SessionOptions { tool_access_policy: Some(ask_fabro_policy), tool_exposure_mode: ToolExposureMode::AutoApprovedOnly, tool_secrets: ToolSecrets { - brave_search_api_key: state.vault_secret(EnvVars::BRAVE_SEARCH_API_KEY), + brave_search_api_key, }, ..SessionOptions::default() }; diff --git a/lib/crates/fabro-server/src/server/handler/system.rs b/lib/crates/fabro-server/src/server/handler/system.rs index ec3ee62c9..ba387b1ab 100644 --- a/lib/crates/fabro-server/src/server/handler/system.rs +++ b/lib/crates/fabro-server/src/server/handler/system.rs @@ -8,6 +8,7 @@ use fabro_slack::config::{ }; use fabro_static::EnvVars; use fabro_types::settings::server::GithubIntegrationSettings; +use fabro_vault::Vault; use super::super::{ AggregateBilling, AggregateBillingTotals, ApiError, AppState, BilledTokenCounts, @@ -101,18 +102,21 @@ async fn get_system_integrations( State(state): State>, ) -> Response { let settings = state.server_settings(); + let vault = match state.stores.vault.snapshot().await { + Ok(vault) => vault, + Err(err) => return secret_store_failure(&err), + }; + let github = github_integration_status(&settings.server.integrations.github, &vault); + let slack = slack_integration_status(state.as_ref(), &vault); let response = SystemIntegrationsResponse { - data: vec![ - github_integration_status(state.as_ref(), &settings.server.integrations.github), - slack_integration_status(state.as_ref()), - ], + data: vec![github, slack], }; (StatusCode::OK, Json(response)).into_response() } fn github_integration_status( - state: &AppState, settings: &GithubIntegrationSettings, + vault: &Vault, ) -> SystemIntegrationStatus { let mut metadata = BTreeMap::new(); metadata.insert( @@ -144,7 +148,7 @@ fn github_integration_status( let mut missing = Vec::new(); match settings.strategy { GithubIntegrationStrategy::Token => { - if missing_vault_secret(state, EnvVars::GITHUB_TOKEN) { + if missing_vault_secret(vault, EnvVars::GITHUB_TOKEN) { missing.push(EnvVars::GITHUB_TOKEN.to_string()); } } @@ -155,10 +159,10 @@ fn github_integration_status( if settings.client_id.is_none() { missing.push("server.integrations.github.client_id".to_string()); } - if missing_vault_secret(state, EnvVars::GITHUB_APP_CLIENT_SECRET) { + if missing_vault_secret(vault, EnvVars::GITHUB_APP_CLIENT_SECRET) { missing.push(EnvVars::GITHUB_APP_CLIENT_SECRET.to_string()); } - if missing_vault_secret(state, EnvVars::GITHUB_APP_PRIVATE_KEY) { + if missing_vault_secret(vault, EnvVars::GITHUB_APP_PRIVATE_KEY) { missing.push(EnvVars::GITHUB_APP_PRIVATE_KEY.to_string()); } } @@ -180,7 +184,7 @@ fn github_integration_status( ) } -fn slack_integration_status(state: &AppState) -> SystemIntegrationStatus { +fn slack_integration_status(state: &AppState, vault: &Vault) -> SystemIntegrationStatus { let settings = &state.server_settings().server.integrations.slack; let mut metadata = BTreeMap::new(); if let Some(default_channel) = settings.default_channel.as_ref() { @@ -198,13 +202,14 @@ fn slack_integration_status(state: &AppState) -> SystemIntegrationStatus { ); } - let mut missing = - match resolve_slack_credentials_status_with_lookup(|name| state.vault_secret(name)) { - SlackCredentialResolution::Configured(_) => Vec::new(), - SlackCredentialResolution::Missing { env_vars } => { - env_vars.into_iter().map(str::to_string).collect() - } - }; + let mut missing = match resolve_slack_credentials_status_with_lookup(|name| { + vault.get(name).map(str::to_string) + }) { + SlackCredentialResolution::Configured(_) => Vec::new(), + SlackCredentialResolution::Missing { env_vars } => { + env_vars.into_iter().map(str::to_string).collect() + } + }; missing.sort(); if !missing.is_empty() { return integration_status( @@ -262,12 +267,17 @@ fn integration_status( } } -fn missing_vault_secret(state: &AppState, name: &str) -> bool { - state - .vault_secret(name) - .as_deref() - .map(str::trim) - .is_none_or(str::is_empty) +fn missing_vault_secret(vault: &Vault, name: &str) -> bool { + vault.get(name).map(str::trim).is_none_or(str::is_empty) +} + +fn secret_store_failure(err: &fabro_vault::SecretStoreError) -> Response { + tracing::error!(error = ?err, "Loading integration secrets failed"); + ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "secret store operation failed", + ) + .into_response() } async fn get_system_resources(_auth: RequiredUser, State(state): State>) -> Response { @@ -469,7 +479,7 @@ async fn get_github_repo( ) .into_response(); } - let creds = match state.github_credentials(github_settings) { + let creds = match state.github_credentials(github_settings).await { Ok(Some(fabro_github::GitHubCredentials::App(creds))) => creds, Ok(Some(_)) => unreachable!("app strategy should not return token credentials"), Ok(None) => { @@ -480,7 +490,12 @@ async fn get_github_repo( .into_response(); } Err(err) => { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + tracing::error!(error = ?err, "Loading GitHub credentials failed"); + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "GitHub credentials are unavailable", + ) + .into_response(); } }; @@ -553,7 +568,7 @@ async fn get_github_repo( } } GithubIntegrationStrategy::Token => { - let token = match state.github_credentials(github_settings) { + let token = match state.github_credentials(github_settings).await { Ok(Some(fabro_github::GitHubCredentials::Pat(token))) => token, Ok(Some(fabro_github::GitHubCredentials::Installation(token))) => { match token.valid_token() { @@ -573,7 +588,12 @@ async fn get_github_repo( .into_response(); } Err(err) => { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + tracing::error!(error = ?err, "Loading GitHub credentials failed"); + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "GitHub credentials are unavailable", + ) + .into_response(); } }; let client = match state.http_client() { diff --git a/lib/crates/fabro-server/src/server/tests.rs b/lib/crates/fabro-server/src/server/tests.rs index 45e26df15..9a863ed50 100644 --- a/lib/crates/fabro-server/src/server/tests.rs +++ b/lib/crates/fabro-server/src/server/tests.rs @@ -523,20 +523,10 @@ async fn http_log_records_webhook_principal_fields() { reason = "Test helper mirrors the public build_router convenience API." )] fn webhook_test_app(auth_mode: AuthMode) -> Router { - let secret = TEST_WEBHOOK_SECRET.to_string(); - let state = test_app_state_with_env_lookup( - default_test_server_settings(), - RunLayer::default(), - 5, - |_| None, - ); - state - .stores - .vault - .try_write() - .expect("test vault should not be locked") - .set(WEBHOOK_SECRET_ENV, &secret, SecretType::Token, None) - .unwrap(); + let state = TestAppStateBuilder::new() + .env_lookup(|_| None) + .vault_entries([(WEBHOOK_SECRET_ENV, TEST_WEBHOOK_SECRET)]) + .build(); build_router_with_options(state, &auth_mode, RouterOptions { web_enabled: false, ..RouterOptions::default() @@ -1383,7 +1373,7 @@ async fn create_secret_stores_file_secret_outside_token_lookups() { assert_eq!(body["type"], "file"); assert_eq!(body["description"], "Test certificate"); - let vault = state.stores.vault.read().await; + let vault = state.stores.vault.snapshot().await.unwrap(); assert_eq!( vault.get_entry("/tmp/test.pem").unwrap().secret_type, SecretType::File @@ -1427,7 +1417,7 @@ async fn create_secret_rejects_bootstrap_secret_names() { body["errors"][0]["detail"], format!("{name} is a bootstrap secret; configure it with process env or server.env") ); - assert!(state.stores.vault.read().await.get(name).is_none()); + assert!(state.stores.vault.get(name).await.unwrap().is_none()); } } @@ -1447,7 +1437,16 @@ async fn create_secret_allows_optional_vault_and_custom_secret_names() { .unwrap(); assert_status!(response, StatusCode::OK).await; - assert_eq!(state.stores.vault.read().await.get(name), Some(value)); + assert_eq!( + state + .stores + .vault + .get(name) + .await + .unwrap() + .map(|entry| entry.value), + Some(value.to_string()) + ); } } @@ -1540,7 +1539,7 @@ async fn create_secret_stores_valid_oauth_entries() { let response = app.oneshot(req).await.unwrap(); assert_status!(response, StatusCode::OK).await; - let listed = state.stores.vault.read().await.list(); + let listed = state.stores.vault.list().await.unwrap(); assert_eq!(listed.len(), 1); assert_eq!(listed[0].name, "OPENAI_CODEX"); assert_eq!(listed[0].secret_type, SecretType::Oauth); @@ -1548,9 +1547,9 @@ async fn create_secret_stores_valid_oauth_entries() { state .stores .vault - .read() - .await .get("OPENAI_CODEX") + .await + .unwrap() .is_some() ); } @@ -1578,14 +1577,13 @@ async fn create_secret_rejects_under_scoped_daytona_api_key_and_leaves_vault_unc state .stores .vault - .write() - .await .set( EnvVars::DAYTONA_API_KEY, "existing", SecretType::Token, None, ) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); @@ -1616,10 +1614,11 @@ async fn create_secret_rejects_under_scoped_daytona_api_key_and_leaves_vault_unc state .stores .vault - .read() + .get(EnvVars::DAYTONA_API_KEY) .await - .get(EnvVars::DAYTONA_API_KEY), - Some("existing") + .unwrap() + .map(|entry| entry.value), + Some("existing".to_string()) ); auth.assert_async().await; current_key.assert_async().await; @@ -1660,14 +1659,13 @@ enabled = false state .stores .vault - .write() - .await .set( EnvVars::DAYTONA_API_KEY, "dtn_test", SecretType::Token, None, ) + .await .unwrap(); let report = crate::diagnostics::run_all(&state).await; @@ -1710,14 +1708,13 @@ async fn resolve_llm_client_reads_openai_token_from_vault() { state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let llm_result = state.resolve_llm_client().await.unwrap(); @@ -1797,14 +1794,13 @@ async fn llm_source_configured_providers_reads_openai_token_from_vault() { state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let catalog = state.catalog(); @@ -1842,14 +1838,13 @@ async fn resolve_llm_client_uses_vault_key_without_env_lookup_openai_settings() state .stores .vault - .write() - .await .set( "OPENAI_API_KEY", "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let llm_result = state.resolve_llm_client().await.unwrap(); @@ -1881,17 +1876,17 @@ async fn resolve_llm_client_uses_vault_key_without_env_lookup_openai_settings() #[tokio::test] async fn list_secrets_includes_oauth_metadata() { let state = test_app_state(); - { - let mut vault = state.stores.vault.write().await; - vault - .set( - "OPENAI_CODEX", - &openai_oauth_credential_json(), - SecretType::Oauth, - Some("saved auth"), - ) - .unwrap(); - } + state + .stores + .vault + .set( + "OPENAI_CODEX", + &openai_oauth_credential_json(), + SecretType::Oauth, + Some("saved auth"), + ) + .await + .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let response = app @@ -1998,7 +1993,7 @@ async fn delete_secret_by_name_removes_file_secret() { let delete_response = app.oneshot(delete_req).await.unwrap(); assert_status!(delete_response, StatusCode::NO_CONTENT).await; - assert!(state.stores.vault.read().await.list().is_empty()); + assert!(state.stores.vault.list().await.unwrap().is_empty()); } #[test] @@ -2057,8 +2052,7 @@ fn slack_app_state_with_settings_and_secret_sources( store, artifact_store, db_pool: test_db_pool_for_vault_path(&vault_path).expect("test db pool should build"), - vault_path, - preloaded_vault: Some(vault), + preloaded_vault: vault, server_secrets: load_test_server_secrets(server_env_path, server_secret_env), env_lookup: default_env_lookup(), github_api_base_url: None, @@ -2216,8 +2210,7 @@ fn slack_service_respects_disabled_server_config_even_with_vault_tokens() { store, artifact_store, db_pool: test_db_pool_for_vault_path(&vault_path).expect("test db pool should build"), - vault_path, - preloaded_vault: Some(vault), + preloaded_vault: vault, server_secrets: load_test_server_secrets( tempfile::tempdir().unwrap().path().join("server.env"), HashMap::new(), @@ -2347,26 +2340,16 @@ fn worker_command_opt_in_token_includes_agent_run_tools_scope() { fn worker_command_forwards_github_app_private_key_from_vault() { let storage_dir = tempfile::tempdir().unwrap(); let state = worker_command_test_state(storage_dir.path(), &["dev-token"], Some(TEST_DEV_TOKEN)); - state - .stores - .vault - .try_write() - .expect("test vault should not be locked") - .set( - EnvVars::GITHUB_APP_PRIVATE_KEY, - "test-private-key", - SecretType::File, - None, - ) - .unwrap(); - let cmd = worker_command( + let spec = worker_launch_spec( state.as_ref(), RunId::new(), RunExecutionMode::Start, storage_dir.path(), false, + Some("test-private-key".to_string()), ) .unwrap(); + let cmd = LocalWorkerRuntime::command_for_spec(&spec); assert_eq!( command_env_value(&cmd, EnvVars::GITHUB_APP_PRIVATE_KEY), @@ -2570,6 +2553,9 @@ methods = ["dev-token"] let (store, artifact_store) = test_store_bundle(); let vault_path = test_secret_store_path(); let server_env_path = vault_path.with_file_name("server.env"); + let db_pool = test_db_pool_for_vault_path(&vault_path).expect("test db pool should build"); + let preloaded_vault = crate::test_support::test_secret_snapshot(db_pool.clone()) + .expect("test secret snapshot should build"); let Err(err) = build_app_state(AppStateConfig { resolved_settings: resolved_runtime_settings_for_tests( server_settings, @@ -2580,9 +2566,8 @@ methods = ["dev-token"] max_concurrent_runs: 5, store, artifact_store, - db_pool: test_db_pool_for_vault_path(&vault_path).expect("test db pool should build"), - vault_path, - preloaded_vault: None, + db_pool, + preloaded_vault, server_secrets: ServerSecrets::load(server_env_path, HashMap::new()).unwrap(), env_lookup: default_env_lookup(), github_api_base_url: None, @@ -2602,8 +2587,8 @@ methods = ["dev-token"] )); } -#[test] -fn build_app_state_migrates_legacy_vault_file_on_boot() { +#[tokio::test] +async fn build_app_state_migrates_legacy_vault_file_on_boot() { let vault_path = test_secret_store_path(); let timestamp = "2026-05-18T12:00:00Z"; let legacy_api_key = json!({ @@ -2664,11 +2649,7 @@ fn build_app_state_migrates_legacy_vault_file_on_boot() { let state = build_test_app_state_with_vault_path(&vault_path) .expect("legacy vault should not prevent server boot"); - let vault = state - .stores - .vault - .try_read() - .expect("test vault should not be locked"); + let vault = state.stores.vault.snapshot().await.unwrap(); let api_key_entry = vault .get_entry("ANTHROPIC_API_KEY") .expect("legacy provider credential should be migrated to token name"); @@ -2698,6 +2679,8 @@ fn build_app_state_migrates_legacy_vault_file_on_boot() { fn build_test_app_state_with_vault_path(vault_path: &Path) -> anyhow::Result> { let (store, artifact_store) = test_store_bundle(); + let db_pool = test_db_pool_for_vault_path(vault_path)?; + let preloaded_vault = crate::test_support::test_secret_snapshot(db_pool.clone())?; build_app_state(AppStateConfig { resolved_settings: resolved_runtime_settings_for_tests( default_test_server_settings(), @@ -2708,9 +2691,8 @@ fn build_test_app_state_with_vault_path(vault_path: &Path) -> anyhow::Result anyhow::Result { - let spec = worker_launch_spec(state, run_id, mode, run_dir, agent_fabro_tools_enabled)?; + let spec = worker_launch_spec( + state, + run_id, + mode, + run_dir, + agent_fabro_tools_enabled, + None, + )?; Ok(LocalWorkerRuntime::command_for_spec(&spec)) } @@ -3319,9 +3308,8 @@ async fn create_run_without_explicit_title_returns_deterministic_then_updates_ge state .stores .vault - .write() - .await .set("OPENAI_API_KEY", "openai-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); @@ -3345,9 +3333,8 @@ async fn create_run_with_explicit_title_skips_generated_title_work() { state .stores .vault - .write() - .await .set("OPENAI_API_KEY", "openai-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let mut manifest = minimal_manifest_json(MINIMAL_DOT); @@ -3413,9 +3400,8 @@ async fn generated_title_failure_leaves_deterministic_title_unchanged() { state .stores .vault - .write() - .await .set("OPENAI_API_KEY", "openai-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); @@ -3454,9 +3440,8 @@ async fn generated_title_does_not_overwrite_user_title_edit() { state .stores .vault - .write() - .await .set("OPENAI_API_KEY", "openai-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); @@ -6068,7 +6053,15 @@ fn create_github_token_app_state_with_env_lookup_and_llm_catalog_settings( let vault_path = test_secret_store_path(); let server_env_path = vault_path.with_file_name("server.env"); let active_config_path = vault_path.with_file_name("settings.toml"); + if let Some(token) = token { + Vault::load(vault_path.clone()) + .expect("test vault should load") + .set("GITHUB_TOKEN", token, SecretType::Token, None) + .expect("test github token should be writable"); + } let db_pool = test_db_pool_for_vault_path(&vault_path).expect("test db pool should build"); + let preloaded_vault = crate::test_support::test_secret_snapshot(db_pool.clone()) + .expect("test secret snapshot should build"); let config = AppStateConfig { resolved_settings: resolved_runtime_settings_for_tests( github_token_settings(), @@ -6080,8 +6073,7 @@ fn create_github_token_app_state_with_env_lookup_and_llm_catalog_settings( store, artifact_store, db_pool, - vault_path, - preloaded_vault: None, + preloaded_vault, server_secrets: load_test_server_secrets(server_env_path, HashMap::new()), env_lookup: Arc::new(env_lookup), github_api_base_url, @@ -6093,21 +6085,11 @@ fn create_github_token_app_state_with_env_lookup_and_llm_catalog_settings( worker_runtime: None, automation_materializer_override: None, }; - let state = build_app_state(config).expect("test app state should build"); - if let Some(token) = token { - state - .stores - .vault - .try_write() - .expect("test vault should not already be locked") - .set("GITHUB_TOKEN", token, SecretType::Token, None) - .expect("test github token should be writable"); - } - state + build_app_state(config).expect("test app state should build") } -#[test] -fn github_token_strategy_ignores_process_env_token() { +#[tokio::test] +async fn github_token_strategy_ignores_process_env_token() { let state = create_github_token_app_state_with_env_lookup(None, None, |name| match name { EnvVars::GITHUB_TOKEN => Some("ghu_from_env".to_string()), _ => None, @@ -6116,16 +6098,17 @@ fn github_token_strategy_ignores_process_env_token() { let err = state .github_credentials(&settings.server.integrations.github) + .await .expect_err("server runtime should ignore env-backed GitHub tokens"); assert_eq!( - err, + err.to_string(), "GITHUB_TOKEN not configured -- run fabro install or run fabro secret set GITHUB_TOKEN" ); } -#[test] -fn github_token_strategy_ignores_gh_token_alias() { +#[tokio::test] +async fn github_token_strategy_ignores_gh_token_alias() { let state = create_github_token_app_state_with_env_lookup(None, None, |name| match name { EnvVars::GH_TOKEN => Some("ghu_from_env_alias".to_string()), _ => None, @@ -6133,34 +6116,35 @@ fn github_token_strategy_ignores_gh_token_alias() { state .stores .vault - .try_write() - .expect("test vault should not already be locked") .set( EnvVars::GH_TOKEN, "ghu_from_vault_alias", SecretType::Token, None, ) + .await .unwrap(); let settings = state.server_settings(); let err = state .github_credentials(&settings.server.integrations.github) + .await .expect_err("server runtime should ignore GH_TOKEN in env and vault"); assert_eq!( - err, + err.to_string(), "GITHUB_TOKEN not configured -- run fabro install or run fabro secret set GITHUB_TOKEN" ); } -#[test] -fn github_token_strategy_reads_github_token_from_vault() { +#[tokio::test] +async fn github_token_strategy_reads_github_token_from_vault() { let state = create_github_token_app_state(Some("ghu_test"), None); let settings = state.server_settings(); let credentials = state .github_credentials(&settings.server.integrations.github) + .await .expect("vault GitHub token should resolve") .expect("vault GitHub token should produce credentials"); @@ -6521,14 +6505,13 @@ async fn list_models_marks_configured_true_when_provider_has_credential_material state .stores .vault - .write() - .await .set( EnvVars::ANTHROPIC_API_KEY, "test-key", SecretType::Token, None, ) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -6716,14 +6699,13 @@ async fn list_providers_marks_configured_per_provider_and_omits_secrets() { state .stores .vault - .write() - .await .set( EnvVars::ANTHROPIC_API_KEY, "test-key", SecretType::Token, None, ) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -6872,14 +6854,13 @@ async fn test_providers_successful_probe_returns_probe_model() { state .stores .vault - .write() - .await .set( EnvVars::OPENAI_API_KEY, "vault-openai-key", SecretType::Token, None, ) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -6927,14 +6908,13 @@ async fn test_providers_auth_issue_returns_error_without_upstream_call() { state .stores .vault - .write() - .await .set( "OPENAI_CODEX", &serde_json::to_string(&credential).unwrap(), SecretType::Oauth, None, ) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -7001,9 +6981,8 @@ reasoning = false state .stores .vault - .write() - .await .set("ACME_API_KEY", "acme-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -7117,15 +7096,18 @@ reasoning = false .max_concurrent_runs(5) .llm_catalog_settings(llm_catalog_settings) .build(); - { - let mut vault = state.stores.vault.write().await; - vault - .set("ALPHA_API_KEY", "alpha-key", SecretType::Token, None) - .unwrap(); - vault - .set("ZETA_API_KEY", "zeta-key", SecretType::Token, None) - .unwrap(); - } + state + .stores + .vault + .set("ALPHA_API_KEY", "alpha-key", SecretType::Token, None) + .await + .unwrap(); + state + .stores + .vault + .set("ZETA_API_KEY", "zeta-key", SecretType::Token, None) + .await + .unwrap(); let app = crate::test_support::build_test_router(state); let req = Request::builder() @@ -7180,9 +7162,8 @@ async fn test_providers_response_does_not_leak_api_keys() { state .stores .vault - .write() - .await .set(EnvVars::OPENAI_API_KEY, leaked_key, SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(state); @@ -8676,9 +8657,8 @@ async fn create_run_pull_request_creates_and_persists_record() { state .stores .vault - .write() - .await .set("OPENAI_API_KEY", "openai-key", SecretType::Token, None) + .await .unwrap(); let app = crate::test_support::build_test_router(Arc::clone(&state)); let run_id = fixtures::RUN_1; diff --git a/lib/crates/fabro-server/src/startup.rs b/lib/crates/fabro-server/src/startup.rs index 33dc33340..e0ef58fbc 100644 --- a/lib/crates/fabro-server/src/startup.rs +++ b/lib/crates/fabro-server/src/startup.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::path::Path; +#[cfg(test)] use anyhow::Context as _; use fabro_static::EnvVars; use fabro_types::settings::ServerNamespace; @@ -26,7 +27,7 @@ pub(crate) fn resolve_startup( Ok((auth_mode, server_secrets)) } -pub fn load_startup_vault(vault_path: impl AsRef) -> anyhow::Result { +pub fn migrate_startup_vault(vault_path: impl AsRef) { let vault_path = vault_path.as_ref(); match migrations::migrate_legacy_vault_file(vault_path) { Ok(report) if report.changed() => { @@ -51,10 +52,17 @@ pub fn load_startup_vault(vault_path: impl AsRef) -> anyhow::Result ); } } +} + +#[cfg(test)] +fn load_startup_vault(vault_path: impl AsRef) -> anyhow::Result { + let vault_path = vault_path.as_ref(); + migrate_startup_vault(vault_path); Vault::load(vault_path.to_path_buf()) .with_context(|| format!("load vault {}", vault_path.display())) } +#[cfg(test)] pub(crate) fn prepare_startup_vault( vault_path: impl AsRef, server_env_path: impl AsRef, diff --git a/lib/crates/fabro-server/src/test_support.rs b/lib/crates/fabro-server/src/test_support.rs index e7ad3722b..31f8aad5f 100644 --- a/lib/crates/fabro-server/src/test_support.rs +++ b/lib/crates/fabro-server/src/test_support.rs @@ -30,7 +30,6 @@ use tokio::runtime::Builder as TokioRuntimeBuilder; use tokio_util::sync::CancellationToken; use ulid::Ulid; -use crate::auth; use crate::automation_materializer::AutomationRunMaterializer; pub use crate::automation_materializer::TestAutomationRunMaterializer; use crate::interp::process_env_var; @@ -44,6 +43,7 @@ use crate::server::{ use crate::server_secrets::ServerSecrets; #[cfg(test)] use crate::worker_runtime::WorkerRuntime; +use crate::{auth, migrations}; pub const TEST_DEV_TOKEN: &str = "fabro_dev_abababababababababababababababababababababababababababababababab"; @@ -252,6 +252,7 @@ impl TestAppStateBuilder { &vault_path, self.default_environment_provider, )?; + let preloaded_vault = test_secret_snapshot(db_pool.clone())?; build_app_state(AppStateConfig { resolved_settings: resolved_runtime_settings_for_tests( self.server_settings, @@ -263,8 +264,7 @@ impl TestAppStateBuilder { store, artifact_store, db_pool, - vault_path, - preloaded_vault: None, + preloaded_vault, server_secrets: load_test_server_secrets(server_env_path, self.server_secret_env), env_lookup: self.env_lookup, github_api_base_url: None, @@ -283,6 +283,24 @@ impl TestAppStateBuilder { } } +#[expect( + clippy::disallowed_methods, + reason = "sync test builders may run inside Tokio; a dedicated thread avoids a nested runtime" +)] +pub(crate) fn test_secret_snapshot(pool: DbPool) -> anyhow::Result { + std::thread::spawn(move || { + let runtime = TokioRuntimeBuilder::new_current_thread() + .enable_all() + .build()?; + runtime + .block_on(fabro_vault::SecretStore::new(pool).snapshot()) + .map(fabro_vault::SecretSnapshot::into_vault) + .map_err(anyhow::Error::new) + }) + .join() + .expect("test secret snapshot thread should not panic") +} + pub fn llm_catalog_settings_with_provider_base_url( provider: impl Into, base_url: impl Into, @@ -512,6 +530,7 @@ pub(crate) fn test_db_pool_for_vault_path_with_default_environment( ) -> anyhow::Result { test_db_pool( sqlite_path_for_vault_path(vault_path), + vault_path.to_path_buf(), default_environment_provider, ) } @@ -540,15 +559,18 @@ pub async fn test_environment_from_storage_dir( )] fn test_db_pool( path: PathBuf, + vault_path: PathBuf, default_environment_provider: Option, ) -> anyhow::Result { std::thread::spawn(move || { + migrations::migrate_legacy_vault_file(&vault_path)?; let runtime = TokioRuntimeBuilder::new_current_thread() .enable_all() .build()?; runtime.block_on(async move { let database = fabro_db::Database::connect(&path).await?; database.migrate().await?; + fabro_vault::import_legacy_json_once(database.pool(), vault_path).await?; if let Some(provider) = default_environment_provider { fabro_environment::seed_default_environment(database.pool(), provider).await?; } diff --git a/lib/crates/fabro-server/src/web_auth.rs b/lib/crates/fabro-server/src/web_auth.rs index 8ff36f5a7..7954d8729 100644 --- a/lib/crates/fabro-server/src/web_auth.rs +++ b/lib/crates/fabro-server/src/web_auth.rs @@ -577,7 +577,17 @@ async fn callback_github( ); }; let client_id = client_id.clone(); - let Some(client_secret) = state.vault_secret(EnvVars::GITHUB_APP_CLIENT_SECRET) else { + let client_secret = match state.vault_secret(EnvVars::GITHUB_APP_CLIENT_SECRET).await { + Ok(value) => value, + Err(err) => { + error!(error = ?err, "OAuth callback failed: secret store unavailable"); + return json_response( + StatusCode::INTERNAL_SERVER_ERROR, + json!({"error": "secret store operation failed"}), + ); + } + }; + let Some(client_secret) = client_secret else { error!("OAuth callback failed: GITHUB_APP_CLIENT_SECRET not configured"); return json_response( StatusCode::CONFLICT, @@ -1659,14 +1669,13 @@ client_id = "github-client-id" state .stores .vault - .write() - .await .set( EnvVars::GITHUB_APP_CLIENT_SECRET, "vault-client-secret", SecretType::Token, None, ) + .await .unwrap(); let app = server::build_router_with_options(state, &github_auth_mode(), server::RouterOptions { diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index f7945fa38..937d12047 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -35,6 +35,17 @@ fn spa_fixture_root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/spa") } +async fn load_secret_snapshot(storage_dir: &std::path::Path) -> Vault { + let storage = Storage::new(storage_dir); + fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await + .expect("test secret store should open") + .snapshot() + .await + .expect("test secret snapshot should load") + .into_vault() +} + fn assert_sandbox_provider_policy( settings: &str, local_enabled: bool, @@ -994,7 +1005,7 @@ async fn token_install_finish_persists_settings_env_and_vault() { Some(finish_dev_token) ); - let vault = Vault::load(storage.secrets_path()).unwrap(); + let vault = load_secret_snapshot(temp_dir.path()).await; assert!(vault.get("ANTHROPIC_API_KEY").is_some()); assert_eq!(vault.get("GITHUB_TOKEN"), Some("ghp_test_token")); } @@ -1080,7 +1091,7 @@ async fn browser_install_finish_with_skipped_llm_persists_no_llm_credentials() { assert!(server_env.contains("SESSION_SECRET=")); assert!(server_env.contains("FABRO_DEV_TOKEN=")); - let vault = Vault::load(fabro_config::Storage::new(temp_dir.path()).secrets_path()).unwrap(); + let vault = load_secret_snapshot(temp_dir.path()).await; assert!( vault.get("OPENAI_API_KEY").is_none() && vault.get("OPENAI_CODEX").is_none(), "skipped LLM install should not write any OpenAI vault entries" @@ -2551,7 +2562,7 @@ async fn sandbox_daytona_resave_without_api_key_preserves_saved_key() { ) .await; - let vault = Vault::load(Storage::new(temp_dir.path()).secrets_path()).unwrap(); + let vault = load_secret_snapshot(temp_dir.path()).await; assert_eq!(vault.get("DAYTONA_API_KEY"), Some(api_key)); } @@ -2604,7 +2615,7 @@ async fn sandbox_switching_from_daytona_to_docker_drops_saved_key() { assert_no_legacy_environment_dir(&temp_dir); let default_environment = seeded_default_environment(&temp_dir).await; assert_eq!(default_environment.settings.provider.to_string(), "docker"); - let vault = Vault::load(Storage::new(temp_dir.path()).secrets_path()).unwrap(); + let vault = load_secret_snapshot(temp_dir.path()).await; assert_eq!(vault.get("DAYTONA_API_KEY"), None); } @@ -2730,7 +2741,7 @@ async fn daytona_install_finish_writes_settings_and_vault_secret() { if content.contains("buildpack-deps:noble") )); - let vault = Vault::load(Storage::new(temp_dir.path()).secrets_path()).unwrap(); + let vault = load_secret_snapshot(temp_dir.path()).await; assert_eq!(vault.get("DAYTONA_API_KEY"), Some(api_key)); } diff --git a/lib/crates/fabro-types/src/lib.rs b/lib/crates/fabro-types/src/lib.rs index 84567d913..e339f285d 100644 --- a/lib/crates/fabro-types/src/lib.rs +++ b/lib/crates/fabro-types/src/lib.rs @@ -145,7 +145,7 @@ pub use sandbox_services::{ SandboxService, SandboxServiceDiscoverySource, SandboxServiceListMeta, SandboxServiceListResponse, }; -pub use secret::{SecretMetadata, SecretType}; +pub use secret::{OAuthConfig, OAuthCredential, OAuthTokens, SecretMetadata, SecretType}; pub use session::{ PermissionLevel, SessionDetail, SessionId, SessionMessage, SessionRecord, SessionStatus, SessionSummary, SessionTurn, TurnId, diff --git a/lib/crates/fabro-types/src/secret.rs b/lib/crates/fabro-types/src/secret.rs index 9ba64eafe..d22d00d76 100644 --- a/lib/crates/fabro-types/src/secret.rs +++ b/lib/crates/fabro-types/src/secret.rs @@ -1,8 +1,20 @@ -use chrono::{DateTime, Utc}; +use chrono::{DateTime, Duration, Utc}; use serde::{Deserialize, Serialize}; -use strum::Display; +use strum::{Display, EnumString, IntoStaticStr}; -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Display, Serialize, Deserialize)] +#[derive( + Debug, + Clone, + Copy, + PartialEq, + Eq, + Default, + Display, + EnumString, + IntoStaticStr, + Serialize, + Deserialize, +)] #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] pub enum SecretType { @@ -15,6 +27,46 @@ pub enum SecretType { File, } +impl SecretType { + #[must_use] + pub fn as_str(self) -> &'static str { + self.into() + } +} + +/// JSON shape stored when [`SecretType::Oauth`] is used. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct OAuthCredential { + pub tokens: OAuthTokens, + pub config: OAuthConfig, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub account_id: Option, +} + +impl OAuthCredential { + #[must_use] + pub fn needs_refresh(&self) -> bool { + self.tokens.expires_at <= Utc::now() + Duration::minutes(5) + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct OAuthTokens { + pub access_token: String, + pub refresh_token: Option, + pub expires_at: DateTime, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct OAuthConfig { + pub auth_url: String, + pub token_url: String, + pub client_id: String, + pub scopes: Vec, + pub redirect_uri: Option, + pub use_pkce: bool, +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SecretMetadata { pub name: String, diff --git a/lib/crates/fabro-vault/Cargo.toml b/lib/crates/fabro-vault/Cargo.toml index 493d4f78d..c3be81a0f 100644 --- a/lib/crates/fabro-vault/Cargo.toml +++ b/lib/crates/fabro-vault/Cargo.toml @@ -13,11 +13,17 @@ doctest = false workspace = true [dependencies] +anyhow.workspace = true chrono.workspace = true +fabro-db = { path = "../fabro-db" } fabro-static = { path = "../fabro-static" } fabro-types = { path = "../fabro-types" } serde.workspace = true serde_json.workspace = true +sqlx.workspace = true +thiserror.workspace = true +tokio.workspace = true +tracing.workspace = true ulid.workspace = true [dev-dependencies] diff --git a/lib/crates/fabro-vault/src/lib.rs b/lib/crates/fabro-vault/src/lib.rs index c0aaccfb2..144ff7d23 100644 --- a/lib/crates/fabro-vault/src/lib.rs +++ b/lib/crates/fabro-vault/src/lib.rs @@ -3,6 +3,8 @@ reason = "fabro-vault: sync secret-file storage; not used on a Tokio hot path" )] +mod store; + use std::collections::HashMap; use std::path::{Component, Path, PathBuf}; use std::{fmt, io}; @@ -11,8 +13,12 @@ use chrono::{DateTime, Utc}; use fabro_static::EnvVars; pub use fabro_types::SecretType; use fabro_types::{SecretMetadata, is_env_style_name}; +pub use store::{ + ImportReport, SecretSnapshot, SecretStore, SecretStoreError, SecretStoreWrite, + import_legacy_json_once, +}; -#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] +#[derive(Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct SecretEntry { pub value: String, #[serde(rename = "type", default)] @@ -21,6 +27,25 @@ pub struct SecretEntry { pub description: Option, pub created_at: DateTime, pub updated_at: DateTime, + #[serde(skip, default = "default_revision")] + pub revision: i64, +} + +const fn default_revision() -> i64 { + 1 +} + +impl fmt::Debug for SecretEntry { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SecretEntry") + .field("value", &"[REDACTED]") + .field("secret_type", &self.secret_type) + .field("description", &self.description) + .field("created_at", &self.created_at) + .field("updated_at", &self.updated_at) + .field("revision", &self.revision) + .finish() + } } #[derive(Debug)] @@ -56,12 +81,21 @@ impl From for Error { } } -#[derive(Debug)] +#[derive(Clone)] pub struct Vault { - path: PathBuf, + path: Option, entries: HashMap, } +impl fmt::Debug for Vault { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Vault") + .field("path", &self.path) + .field("entry_count", &self.entries.len()) + .finish() + } +} + impl Vault { pub fn load(path: PathBuf) -> Result { let entries = match std::fs::read_to_string(&path) { @@ -70,7 +104,25 @@ impl Vault { Err(err) => return Err(io_context("read vault", &path, &err).into()), }; - Ok(Self { path, entries }) + Ok(Self { + path: Some(path), + entries, + }) + } + + /// Builds a detached in-memory vault with no backing file: mutations + /// update memory only and are never persisted to disk. + #[must_use] + pub fn from_entries(entries: HashMap) -> Self { + Self { + path: None, + entries, + } + } + + #[must_use] + pub fn entries(&self) -> &HashMap { + &self.entries } pub fn set( @@ -83,14 +135,15 @@ impl Vault { Self::validate_name(name, secret_type)?; let now = Utc::now(); - let (created_at, description) = self.entries.get(name).map_or_else( - || (now, description.map(str::to_string)), + let (created_at, description, revision) = self.entries.get(name).map_or_else( + || (now, description.map(str::to_string), 1), |entry| { ( entry.created_at, description .map(str::to_string) .or_else(|| entry.description.clone()), + entry.revision.saturating_add(1), ) }, ); @@ -100,6 +153,7 @@ impl Vault { description: description.clone(), created_at, updated_at: now, + revision, }; self.entries.insert(name.to_string(), entry); self.write_atomic()?; @@ -196,15 +250,16 @@ impl Vault { } fn write_atomic(&self) -> Result<(), Error> { - let parent = self - .path + let Some(path) = self.path.as_ref() else { + return Ok(()); + }; + let parent = path .parent() .map_or_else(|| PathBuf::from("."), Path::to_path_buf); std::fs::create_dir_all(&parent) .map_err(|err| io_context("create vault directory", &parent, &err))?; - let file_name = self - .path + let file_name = path .file_name() .and_then(|name| name.to_str()) .unwrap_or("secrets.json"); @@ -213,9 +268,9 @@ impl Vault { std::fs::write(&tmp_path, json) .map_err(|err| io_context("write vault temp file", &tmp_path, &err))?; set_private_permissions(&tmp_path)?; - std::fs::rename(&tmp_path, &self.path).map_err(|err| { + std::fs::rename(&tmp_path, path).map_err(|err| { io_context( - &format!("rename vault temp file to {}", self.path.display()), + &format!("rename vault temp file to {}", path.display()), &tmp_path, &err, ) diff --git a/lib/crates/fabro-vault/src/store.rs b/lib/crates/fabro-vault/src/store.rs new file mode 100644 index 000000000..d2a237e4e --- /dev/null +++ b/lib/crates/fabro-vault/src/store.rs @@ -0,0 +1,557 @@ +use std::collections::HashMap; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::str::FromStr as _; + +use chrono::{DateTime, Utc}; +use fabro_db::{Database, DbPool}; +use fabro_types::{OAuthCredential, SecretMetadata, SecretType}; +use sqlx::sqlite::SqliteRow; +use sqlx::{Row as _, Sqlite, Transaction}; +use tokio::fs; +use tracing::info; + +use crate::{SecretEntry, Vault}; + +#[derive(Debug, thiserror::Error)] +pub enum SecretStoreError { + #[error("invalid secret name: {0}")] + InvalidName(String), + + #[error("secret not found: {0}")] + NotFound(String), + + #[error("secret revision is stale for {name}: expected {expected}, actual {actual}")] + StaleRevision { + name: String, + expected: i64, + actual: i64, + }, + + #[error("secret {name} is not valid OAuth JSON")] + InvalidOauth { + name: String, + #[source] + source: serde_json::Error, + }, + + #[error("database error")] + Db(#[from] sqlx::Error), + + #[error("stored secret {name} has invalid type {value:?}")] + StoredType { name: String, value: String }, + + #[error("stored secret has invalid name {name:?}")] + StoredName { name: String }, + + #[error("stored secret {name} has invalid revision {revision}")] + StoredRevision { name: String, revision: i64 }, + + #[error("stored secret {name} is not a valid OAuth credential")] + StoredOauth { + name: String, + #[source] + source: serde_json::Error, + }, + + #[error("parsing secret timestamp for {name}.{column}")] + Timestamp { + name: String, + column: &'static str, + #[source] + source: chrono::ParseError, + }, + + #[error("reading legacy secrets file {path}")] + LegacyRead { + path: PathBuf, + #[source] + source: std::io::Error, + }, + + #[error("parsing legacy secrets file {path}")] + LegacyParse { + path: PathBuf, + #[source] + source: serde_json::Error, + }, + + #[error("legacy secrets file {path} contains invalid secret {name}")] + LegacyInvalid { path: PathBuf, name: String }, + + #[error("renaming legacy secrets file {source_path} to backup {backup_path}")] + LegacyBackup { + source_path: PathBuf, + backup_path: PathBuf, + #[source] + source: std::io::Error, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ImportReport { + pub source_path: PathBuf, + pub backup_path: PathBuf, + pub imported_rows: usize, + pub skipped_rows: usize, + pub secret_names: Vec, +} + +#[derive(Clone, PartialEq, Eq)] +pub struct SecretStoreWrite { + pub name: String, + pub value: String, + pub secret_type: SecretType, + pub description: Option, +} + +impl std::fmt::Debug for SecretStoreWrite { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SecretStoreWrite") + .field("name", &self.name) + .field("value", &"[REDACTED]") + .field("secret_type", &self.secret_type) + .field("description", &self.description) + .finish() + } +} + +#[derive(Clone)] +pub struct SecretStore { + pool: DbPool, +} + +#[derive(Clone)] +pub struct SecretSnapshot(Vault); + +impl SecretSnapshot { + #[must_use] + pub fn into_vault(self) -> Vault { + self.0 + } +} + +impl std::fmt::Debug for SecretSnapshot { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +impl std::ops::Deref for SecretSnapshot { + type Target = Vault; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::fmt::Debug for SecretStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("SecretStore").finish_non_exhaustive() + } +} + +impl SecretStore { + #[must_use] + pub fn new(pool: DbPool) -> Self { + Self { pool } + } + + /// Opens the Fabro database at `sqlite_path`, runs migrations, imports any + /// legacy secrets JSON at `legacy_secrets_path`, and returns the store. + pub async fn open( + sqlite_path: impl AsRef, + legacy_secrets_path: impl AsRef, + ) -> anyhow::Result { + let database = Database::connect(sqlite_path).await?; + database.migrate().await?; + import_legacy_json_once(database.pool(), legacy_secrets_path).await?; + Ok(Self::new(database.clone_pool())) + } + + pub async fn get(&self, name: &str) -> Result, SecretStoreError> { + let row = sqlx::query( + "SELECT name, secret_type, value, description, revision, created_at, updated_at \ + FROM secrets WHERE name = ?", + ) + .bind(name) + .fetch_optional(&self.pool) + .await?; + row.as_ref() + .map(entry_from_row) + .transpose() + .map(|entry| entry.map(|(_, entry)| entry)) + } + + pub async fn list(&self) -> Result, SecretStoreError> { + let rows = sqlx::query( + "SELECT name, secret_type, description, created_at, updated_at \ + FROM secrets ORDER BY name", + ) + .fetch_all(&self.pool) + .await?; + rows.iter().map(metadata_from_row).collect() + } + + pub async fn set( + &self, + name: &str, + value: &str, + secret_type: SecretType, + description: Option<&str>, + ) -> Result { + validate_write(name, value, secret_type)?; + let now = Utc::now().to_rfc3339(); + let mut transaction = self.pool.begin().await?; + let metadata = upsert_secret( + &mut transaction, + name, + value, + secret_type, + description, + &now, + ) + .await?; + transaction.commit().await?; + Ok(metadata) + } + + pub async fn remove(&self, name: &str) -> Result<(), SecretStoreError> { + let result = sqlx::query("DELETE FROM secrets WHERE name = ?") + .bind(name) + .execute(&self.pool) + .await?; + if result.rows_affected() == 0 { + return Err(SecretStoreError::NotFound(name.to_string())); + } + Ok(()) + } + + pub async fn apply( + &self, + removals: &[String], + writes: &[SecretStoreWrite], + ) -> Result<(), SecretStoreError> { + for write in writes { + validate_write(&write.name, &write.value, write.secret_type)?; + } + + let mut transaction = self.pool.begin().await?; + for name in removals { + sqlx::query("DELETE FROM secrets WHERE name = ?") + .bind(name) + .execute(&mut *transaction) + .await?; + } + let now = Utc::now().to_rfc3339(); + for write in writes { + upsert_secret( + &mut transaction, + &write.name, + &write.value, + write.secret_type, + write.description.as_deref(), + &now, + ) + .await?; + } + transaction.commit().await?; + Ok(()) + } + + pub async fn replace_if_revision( + &self, + name: &str, + expected_revision: i64, + value: &str, + secret_type: SecretType, + ) -> Result { + validate_write(name, value, secret_type)?; + let now = Utc::now().to_rfc3339(); + let row = sqlx::query( + r" + UPDATE secrets SET + secret_type = ?, + value = ?, + revision = revision + 1, + updated_at = ? + WHERE name = ? AND revision = ? + RETURNING name, secret_type, value, description, revision, created_at, updated_at + ", + ) + .bind(secret_type.as_str()) + .bind(value) + .bind(now) + .bind(name) + .bind(expected_revision) + .fetch_optional(&self.pool) + .await?; + + if let Some(row) = row { + return entry_from_row(&row).map(|(_, entry)| entry); + } + match self.get(name).await? { + Some(entry) => Err(SecretStoreError::StaleRevision { + name: name.to_string(), + expected: expected_revision, + actual: entry.revision, + }), + None => Err(SecretStoreError::NotFound(name.to_string())), + } + } + + pub async fn snapshot(&self) -> Result { + let rows = sqlx::query( + "SELECT name, secret_type, value, description, revision, created_at, updated_at \ + FROM secrets", + ) + .fetch_all(&self.pool) + .await?; + let entries = rows + .iter() + .map(entry_from_row) + .collect::, _>>()?; + Ok(SecretSnapshot(Vault::from_entries(entries))) + } +} + +async fn upsert_secret( + transaction: &mut Transaction<'_, Sqlite>, + name: &str, + value: &str, + secret_type: SecretType, + description: Option<&str>, + now: &str, +) -> Result { + let row = sqlx::query( + r" + INSERT INTO secrets ( + name, secret_type, value, description, revision, created_at, updated_at + ) VALUES (?, ?, ?, ?, 1, ?, ?) + ON CONFLICT(name) DO UPDATE SET + secret_type = excluded.secret_type, + value = excluded.value, + description = COALESCE(excluded.description, secrets.description), + revision = secrets.revision + 1, + updated_at = excluded.updated_at + RETURNING name, secret_type, description, created_at, updated_at + ", + ) + .bind(name) + .bind(secret_type.as_str()) + .bind(value) + .bind(description) + .bind(now) + .bind(now) + .fetch_one(&mut **transaction) + .await?; + metadata_from_row(&row) +} + +/// Removal deadline for this temporary legacy-import migration, per +/// docs/internal/migrations-strategy.md. +const IMPORT_REMOVAL_DEADLINE: &str = "2026-10-11"; + +pub async fn import_legacy_json_once( + pool: &DbPool, + source_path: impl AsRef, +) -> Result, SecretStoreError> { + let source_path = source_path.as_ref(); + let contents = match fs::read_to_string(source_path).await { + Ok(contents) => contents, + Err(source) if source.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(source) => { + return Err(SecretStoreError::LegacyRead { + path: source_path.to_path_buf(), + source, + }); + } + }; + let entries: HashMap = + serde_json::from_str(&contents).map_err(|source| SecretStoreError::LegacyParse { + path: source_path.to_path_buf(), + source, + })?; + let mut names = entries.keys().cloned().collect::>(); + names.sort(); + for name in &names { + let entry = &entries[name]; + if Vault::validate_name(name, entry.secret_type).is_err() + || (entry.secret_type == SecretType::Oauth + && validate_oauth_json(&entry.value).is_err()) + { + return Err(SecretStoreError::LegacyInvalid { + path: source_path.to_path_buf(), + name: name.clone(), + }); + } + } + + let mut transaction = pool.begin().await?; + let mut imported_names = Vec::new(); + let mut skipped_rows = 0usize; + for name in &names { + let entry = &entries[name]; + let inserted = insert_legacy_entry(&mut transaction, name, entry).await?; + if inserted { + imported_names.push(name.clone()); + } else { + skipped_rows += 1; + } + } + transaction.commit().await?; + let backup_path = rename_imported_legacy_file(source_path).await?; + let report = ImportReport { + source_path: source_path.to_path_buf(), + backup_path, + imported_rows: imported_names.len(), + skipped_rows, + secret_names: imported_names, + }; + info!( + source_path = %report.source_path.display(), + backup_path = %report.backup_path.display(), + imported_rows = report.imported_rows, + skipped_rows = report.skipped_rows, + secret_names = ?report.secret_names, + removal_deadline = IMPORT_REMOVAL_DEADLINE, + "Imported legacy secrets JSON into SQLite" + ); + Ok(Some(report)) +} + +async fn insert_legacy_entry( + transaction: &mut Transaction<'_, Sqlite>, + name: &str, + entry: &SecretEntry, +) -> Result { + let result = sqlx::query( + r" + INSERT INTO secrets ( + name, secret_type, value, description, revision, created_at, updated_at + ) VALUES (?, ?, ?, ?, 1, ?, ?) + ON CONFLICT(name) DO NOTHING + ", + ) + .bind(name) + .bind(entry.secret_type.as_str()) + .bind(&entry.value) + .bind(entry.description.as_deref()) + .bind(entry.created_at.to_rfc3339()) + .bind(entry.updated_at.to_rfc3339()) + .execute(&mut **transaction) + .await?; + Ok(result.rows_affected() == 1) +} + +fn entry_from_row(row: &SqliteRow) -> Result<(String, SecretEntry), SecretStoreError> { + let metadata = metadata_from_row(row)?; + let name = metadata.name; + let value = row.try_get::("value")?; + if metadata.secret_type == SecretType::Oauth { + validate_oauth_json(&value).map_err(|source| SecretStoreError::StoredOauth { + name: name.clone(), + source, + })?; + } + let revision = row.try_get::("revision")?; + if revision <= 0 { + return Err(SecretStoreError::StoredRevision { name, revision }); + } + let entry = SecretEntry { + value, + secret_type: metadata.secret_type, + description: metadata.description, + created_at: metadata.created_at, + updated_at: metadata.updated_at, + revision, + }; + Ok((name, entry)) +} + +fn metadata_from_row(row: &SqliteRow) -> Result { + let name = row.try_get::("name")?; + let type_value = row.try_get::("secret_type")?; + let secret_type = + SecretType::from_str(&type_value).map_err(|_| SecretStoreError::StoredType { + name: name.clone(), + value: type_value, + })?; + validate_stored_name(&name, secret_type)?; + Ok(SecretMetadata { + name: name.clone(), + secret_type, + description: row.try_get("description")?, + created_at: parse_timestamp( + &name, + "created_at", + &row.try_get::("created_at")?, + )?, + updated_at: parse_timestamp( + &name, + "updated_at", + &row.try_get::("updated_at")?, + )?, + }) +} + +fn parse_timestamp( + name: &str, + column: &'static str, + value: &str, +) -> Result, SecretStoreError> { + DateTime::parse_from_rfc3339(value) + .map(|timestamp| timestamp.with_timezone(&Utc)) + .map_err(|source| SecretStoreError::Timestamp { + name: name.to_string(), + column, + source, + }) +} + +fn validate_oauth_json(value: &str) -> Result<(), serde_json::Error> { + serde_json::from_str::(value).map(|_| ()) +} + +fn validate_write( + name: &str, + value: &str, + secret_type: SecretType, +) -> Result<(), SecretStoreError> { + Vault::validate_name(name, secret_type) + .map_err(|_| SecretStoreError::InvalidName(name.to_string()))?; + if secret_type == SecretType::Oauth { + validate_oauth_json(value).map_err(|source| SecretStoreError::InvalidOauth { + name: name.to_string(), + source, + })?; + } + Ok(()) +} + +fn validate_stored_name(name: &str, secret_type: SecretType) -> Result<(), SecretStoreError> { + Vault::validate_name(name, secret_type).map_err(|_| SecretStoreError::StoredName { + name: name.to_string(), + }) +} + +async fn rename_imported_legacy_file(source_path: &Path) -> Result { + let backup_path = legacy_backup_path(source_path, Utc::now()); + fs::rename(source_path, &backup_path) + .await + .map_err(|source| SecretStoreError::LegacyBackup { + source_path: source_path.to_path_buf(), + backup_path: backup_path.clone(), + source, + })?; + Ok(backup_path) +} + +fn legacy_backup_path(source_path: &Path, imported_at: DateTime) -> PathBuf { + let timestamp = imported_at.format("%Y%m%dT%H%M%S%fZ"); + let mut file_name = source_path + .file_name() + .map_or_else(|| OsString::from("secrets.json"), OsString::from); + file_name.push(format!(".imported-{timestamp}.bak")); + source_path.with_file_name(file_name) +} diff --git a/lib/crates/fabro-vault/tests/store.rs b/lib/crates/fabro-vault/tests/store.rs new file mode 100644 index 000000000..0f2d2afda --- /dev/null +++ b/lib/crates/fabro-vault/tests/store.rs @@ -0,0 +1,277 @@ +#![expect( + clippy::unwrap_used, + reason = "SQLite secret-store integration tests use panic-on-failure fixture setup" +)] + +use std::collections::HashMap; + +use chrono::{TimeZone as _, Utc}; +use fabro_db::Database; +use fabro_types::SecretType; +use fabro_vault::{SecretEntry, SecretStore, SecretStoreError, import_legacy_json_once}; +use tokio::fs; + +async fn test_database() -> (tempfile::TempDir, Database) { + let dir = tempfile::tempdir().unwrap(); + let database = Database::connect(dir.path().join("fabro.sqlite3")) + .await + .unwrap(); + database.migrate().await.unwrap(); + (dir, database) +} + +fn oauth_credential(access_token: &str) -> String { + serde_json::json!({ + "tokens": { + "access_token": access_token, + "refresh_token": "refresh", + "expires_at": "2026-07-11T13:00:00Z" + }, + "config": { + "auth_url": "https://auth.example.com", + "token_url": "https://auth.example.com/token", + "client_id": "client", + "scopes": ["openid"], + "redirect_uri": null, + "use_pkce": true + } + }) + .to_string() +} + +#[tokio::test] +async fn crud_preserves_metadata_and_description() { + let (_dir, database) = test_database().await; + let store = SecretStore::new(database.clone_pool()); + + let created = store + .set( + "OPENAI_API_KEY", + "first", + SecretType::Token, + Some("provider key"), + ) + .await + .unwrap(); + let updated = store + .set("OPENAI_API_KEY", "second", SecretType::Token, None) + .await + .unwrap(); + + assert_eq!(created.created_at, updated.created_at); + assert_eq!(updated.description.as_deref(), Some("provider key")); + let entry = store.get("OPENAI_API_KEY").await.unwrap().unwrap(); + assert_eq!(entry.value, "second"); + assert_eq!(entry.revision, 2); + let listed = store.list().await.unwrap(); + assert_eq!(listed.len(), 1); + assert_eq!(listed[0].name, updated.name); + assert_eq!(listed[0].description, updated.description); + + store.remove("OPENAI_API_KEY").await.unwrap(); + assert!(store.get("OPENAI_API_KEY").await.unwrap().is_none()); +} + +#[tokio::test] +async fn independent_stores_observe_writes() { + let (_dir, database) = test_database().await; + let first = SecretStore::new(database.clone_pool()); + let second = SecretStore::new(database.clone_pool()); + + first + .set("ANTHROPIC_API_KEY", "key", SecretType::Token, None) + .await + .unwrap(); + + assert_eq!( + second + .get("ANTHROPIC_API_KEY") + .await + .unwrap() + .unwrap() + .value, + "key" + ); +} + +#[tokio::test] +async fn replace_if_revision_rejects_stale_writer() { + let (_dir, database) = test_database().await; + let store = SecretStore::new(database.clone_pool()); + store + .set( + "OPENAI_CODEX", + &oauth_credential("initial"), + SecretType::Oauth, + None, + ) + .await + .unwrap(); + + let updated = store + .replace_if_revision( + "OPENAI_CODEX", + 1, + &oauth_credential("winner"), + SecretType::Oauth, + ) + .await + .unwrap(); + assert_eq!(updated.revision, 2); + + let err = store + .replace_if_revision( + "OPENAI_CODEX", + 1, + &oauth_credential("loser"), + SecretType::Oauth, + ) + .await + .unwrap_err(); + assert!(matches!(err, SecretStoreError::StaleRevision { + expected: 1, + actual: 2, + .. + })); +} + +#[tokio::test] +async fn imports_legacy_json_once_without_overwriting_sql() { + let (dir, database) = test_database().await; + let store = SecretStore::new(database.clone_pool()); + store + .set("EXISTING_KEY", "sql", SecretType::Token, None) + .await + .unwrap(); + let timestamp = Utc.with_ymd_and_hms(2026, 7, 11, 12, 0, 0).unwrap(); + let entries = HashMap::from([ + ("EXISTING_KEY".to_string(), SecretEntry { + value: "legacy".to_string(), + secret_type: SecretType::Token, + description: None, + created_at: timestamp, + updated_at: timestamp, + revision: 1, + }), + ("NEW_KEY".to_string(), SecretEntry { + value: "new".to_string(), + secret_type: SecretType::Token, + description: Some("imported".to_string()), + created_at: timestamp, + updated_at: timestamp, + revision: 1, + }), + ]); + let source = dir.path().join("secrets.json"); + fs::write(&source, serde_json::to_vec(&entries).unwrap()) + .await + .unwrap(); + + let report = import_legacy_json_once(database.pool(), &source) + .await + .unwrap() + .unwrap(); + + assert_eq!(report.imported_rows, 1); + assert_eq!(report.skipped_rows, 1); + assert!(!source.exists()); + assert!(report.backup_path.exists()); + assert_eq!( + store.get("EXISTING_KEY").await.unwrap().unwrap().value, + "sql" + ); + assert_eq!(store.get("NEW_KEY").await.unwrap().unwrap().value, "new"); + assert!( + import_legacy_json_once(database.pool(), &source) + .await + .unwrap() + .is_none() + ); +} + +#[tokio::test] +async fn malformed_legacy_json_does_not_import_or_rename() { + let (dir, database) = test_database().await; + let source = dir.path().join("secrets.json"); + fs::write(&source, br#"{"VALID_KEY":{"value":"secret"}}"#) + .await + .unwrap(); + + let err = import_legacy_json_once(database.pool(), &source) + .await + .unwrap_err(); + + assert!(matches!(err, SecretStoreError::LegacyParse { .. })); + assert!(source.exists()); + assert!( + SecretStore::new(database.clone_pool()) + .list() + .await + .unwrap() + .is_empty() + ); +} + +#[tokio::test] +async fn github_private_key_file_secret_satisfies_schema() { + let (_dir, database) = test_database().await; + let store = SecretStore::new(database.clone_pool()); + + store + .set("GITHUB_APP_PRIVATE_KEY", "pem", SecretType::File, None) + .await + .unwrap(); + store + .set("/run/secrets/key.pem", "pem", SecretType::File, None) + .await + .unwrap(); +} + +#[tokio::test] +async fn corrupted_stored_row_returns_typed_error() { + let (_dir, database) = test_database().await; + let mut connection = database.pool().acquire().await.unwrap(); + sqlx::query("PRAGMA ignore_check_constraints = ON") + .execute(&mut *connection) + .await + .unwrap(); + sqlx::query( + "INSERT INTO secrets (name, secret_type, value, revision, created_at, updated_at) \ + VALUES (?, ?, ?, ?, ?, ?)", + ) + .bind("VALID_NAME") + .bind("token") + .bind("value") + .bind(0_i64) + .bind("2026-07-11T12:00:00Z") + .bind("2026-07-11T12:00:00Z") + .execute(&mut *connection) + .await + .unwrap(); + + let err = SecretStore::new(database.clone_pool()) + .get("VALID_NAME") + .await + .unwrap_err(); + assert!(matches!(err, SecretStoreError::StoredRevision { + revision: 0, + .. + })); +} + +#[test] +fn debug_redacts_secret_value() { + let timestamp = Utc.with_ymd_and_hms(2026, 7, 11, 12, 0, 0).unwrap(); + let entry = SecretEntry { + value: "do-not-print".to_string(), + secret_type: SecretType::Token, + description: None, + created_at: timestamp, + updated_at: timestamp, + revision: 1, + }; + + let rendered = format!("{entry:?}"); + assert!(!rendered.contains("do-not-print")); + assert!(rendered.contains("[REDACTED]")); +}