From f2cbc016eeccd31fce6ffe657fcbd11e369986c7 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 11 Jul 2026 13:41:31 -0400 Subject: [PATCH] Clean up SQLite secrets migration and fix CLI env credential regression Review pass over the secrets-to-SQLite migration: - Add SecretStore::open() consolidating the connect/migrate/import-legacy sequence repeated at five call sites; fabro-agent and fabro-cli drop their fabro-db dependency - Restore process-env LLM credential lookup in the standalone CLI/agent sources via SqlVaultCredentialSource::new (regression: vault_only dropped the env fallback that VaultCredentialSource::new provided) - Fix five install tests that still asserted against the legacy secrets.json, which the importer renames to .bak - Make AppStateConfig.preloaded_vault required, deleting the fallback that re-read the already-renamed legacy file; drop the now-unused vault_path field and demote load_startup_vault to test-only - Skip the snapshot clones and CAS retry in resolve() when the vault holds no OAuth secrets (per-request hot path) - Remove dead persist_with_secret_store, the VaultSecretWrite alias, the secret_type_string one-liner (now SecretType::as_str), the impossible RowCountOverflow error, and duplicated row parsing - Run check_crypto concurrently with the other diagnostics checks Co-Authored-By: Claude Fable 5 --- Cargo.lock | 3 +- lib/crates/fabro-agent/Cargo.toml | 1 - lib/crates/fabro-agent/src/cli.rs | 20 ++---- lib/crates/fabro-auth/src/sql_vault_source.rs | 19 +++++ lib/crates/fabro-cli/Cargo.toml | 1 - lib/crates/fabro-cli/src/command_context.rs | 17 ++--- lib/crates/fabro-cli/src/commands/install.rs | 24 +++---- .../fabro-cli/src/commands/run/runner.rs | 20 +++--- .../fabro-cli/src/commands/server/start.rs | 15 +--- lib/crates/fabro-cli/tests/it/cmd/install.rs | 16 ++++- lib/crates/fabro-install/src/lib.rs | 46 ++++--------- lib/crates/fabro-server/src/diagnostics.rs | 4 +- lib/crates/fabro-server/src/install.rs | 19 +++-- lib/crates/fabro-server/src/lib.rs | 4 +- lib/crates/fabro-server/src/serve.rs | 3 +- lib/crates/fabro-server/src/server.rs | 12 ++-- lib/crates/fabro-server/src/server/tests.rs | 26 +++---- lib/crates/fabro-server/src/startup.rs | 4 +- lib/crates/fabro-server/src/test_support.rs | 5 +- .../fabro-server/tests/it/api/install.rs | 21 ++++-- lib/crates/fabro-types/src/secret.rs | 7 ++ lib/crates/fabro-vault/Cargo.toml | 1 + lib/crates/fabro-vault/src/lib.rs | 2 + lib/crates/fabro-vault/src/store.rs | 69 ++++++++----------- 24 files changed, 167 insertions(+), 192 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3972d2e15..c66de1be7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2267,7 +2267,6 @@ dependencies = [ "dirs", "fabro-auth", "fabro-config", - "fabro-db", "fabro-http", "fabro-llm", "fabro-macros", @@ -2410,7 +2409,6 @@ dependencies = [ "fabro-checkpoint", "fabro-client", "fabro-config", - "fabro-db", "fabro-dump", "fabro-environment", "fabro-github", @@ -3300,6 +3298,7 @@ dependencies = [ name = "fabro-vault" version = "0.302.0-nightly.1" dependencies = [ + "anyhow", "chrono", "fabro-db", "fabro-static", diff --git a/lib/crates/fabro-agent/Cargo.toml b/lib/crates/fabro-agent/Cargo.toml index ffc2a8906..89f836440 100644 --- a/lib/crates/fabro-agent/Cargo.toml +++ b/lib/crates/fabro-agent/Cargo.toml @@ -25,7 +25,6 @@ workspace = true clap.workspace = true anyhow.workspace = true fabro-auth = { path = "../fabro-auth" } -fabro-db = { path = "../fabro-db" } fabro-config = { path = "../fabro-config", features = ["clap"] } fabro-types = { path = "../fabro-types", features = ["clap"] } fabro-llm = { path = "../fabro-llm" } diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index aed586325..b32ef10dd 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -12,7 +12,6 @@ use clap::{Args, Parser}; use fabro_auth::{CredentialSource, SqlVaultCredentialSource}; use fabro_config::Storage; use fabro_config::user::default_storage_dir; -use fabro_db::Database; use fabro_llm::Error as LlmError; use fabro_llm::client::Client; use fabro_llm::middleware::{Middleware, NextFn, NextStreamFn}; @@ -24,7 +23,7 @@ use fabro_model::catalog::LlmCatalogSettings; use fabro_model::{AgentProfileKind, Catalog, ModelHandle, ProviderId}; use fabro_static::EnvVars; use fabro_util::terminal::Styles; -use fabro_vault::{SecretStore, import_legacy_json_once}; +use fabro_vault::SecretStore; use tokio::io::{AsyncWriteExt, stdout}; use tokio::signal; use tokio::sync::Mutex as AsyncMutex; @@ -275,20 +274,11 @@ fn resolve_provider_id(catalog: &Catalog, args: &AgentArgs) -> anyhow::Result anyhow::Result> { - let storage_dir = default_storage_dir(); - let storage = Storage::new(storage_dir); - let database = Database::connect(storage.sqlite_path()) + let storage = Storage::new(default_storage_dir()); + let store = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) .await - .context("opening the Fabro database for secrets")?; - database - .migrate() - .await - .context("migrating the Fabro database for secrets")?; - import_legacy_json_once(database.pool(), storage.secrets_path()) - .await - .context("importing legacy secrets into SQLite")?; - let store = Arc::new(SecretStore::new(database.clone_pool())); - Ok(Arc::new(SqlVaultCredentialSource::vault_only(store))) + .context("opening the Fabro secret store")?; + Ok(Arc::new(SqlVaultCredentialSource::new(Arc::new(store)))) } fn profile_kind_for_provider( diff --git a/lib/crates/fabro-auth/src/sql_vault_source.rs b/lib/crates/fabro-auth/src/sql_vault_source.rs index 2e0fda962..ca581d5b6 100644 --- a/lib/crates/fabro-auth/src/sql_vault_source.rs +++ b/lib/crates/fabro-auth/src/sql_vault_source.rs @@ -17,6 +17,16 @@ pub struct SqlVaultCredentialSource { } 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) @@ -87,6 +97,15 @@ 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; diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index d3e8be35b..6f14e385e 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -19,7 +19,6 @@ workspace = true [dependencies] fabro-auth = { path = "../fabro-auth" } -fabro-db = { path = "../fabro-db" } fabro-config = { path = "../fabro-config" } fabro-environment = { path = "../fabro-environment" } fabro-llm = { path = "../fabro-llm" } diff --git a/lib/crates/fabro-cli/src/command_context.rs b/lib/crates/fabro-cli/src/command_context.rs index 405b82482..b3318ce15 100644 --- a/lib/crates/fabro-cli/src/command_context.rs +++ b/lib/crates/fabro-cli/src/command_context.rs @@ -4,14 +4,13 @@ use std::sync::{Arc, OnceLock}; use anyhow::{Context as _, Result, bail}; use fabro_auth::{CredentialSource, SqlVaultCredentialSource}; use fabro_config::{CliLayer, Storage, load_llm_catalog_settings}; -use fabro_db::Database; use fabro_model::Catalog; use fabro_types::UserSettings; 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::{SecretStore, import_legacy_json_once}; +use fabro_vault::SecretStore; use tokio::sync::OnceCell; use crate::args::{ @@ -171,19 +170,11 @@ impl CommandContext { .llm_source .get_or_try_init(|| async move { let storage = Storage::new(&storage_dir); - let database = Database::connect(storage.sqlite_path()) + let store = SecretStore::open(storage.sqlite_path(), storage.secrets_path()) .await - .context("opening the Fabro database for secrets")?; - database - .migrate() - .await - .context("migrating the Fabro database for secrets")?; - import_legacy_json_once(database.pool(), storage.secrets_path()) - .await - .context("importing legacy secrets into SQLite")?; - let store = Arc::new(SecretStore::new(database.clone_pool())); + .context("opening the Fabro secret store")?; let source: Arc = - Arc::new(SqlVaultCredentialSource::vault_only(store)); + 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 f9202c60f..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,7 +1349,7 @@ 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>, } @@ -1630,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, @@ -1668,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, @@ -2174,11 +2174,9 @@ mod tests { use super::*; async fn load_secret_snapshot(storage: &Storage) -> Vault { - let database = fabro_db::Database::connect(storage.sqlite_path()) + fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) .await - .unwrap(); - database.migrate().await.unwrap(); - fabro_vault::SecretStore::new(database.clone_pool()) + .unwrap() .snapshot() .await .unwrap() @@ -3282,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, @@ -3348,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, @@ -3436,7 +3434,7 @@ 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, diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index b6f504764..90283c353 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -9,7 +9,6 @@ use fabro_api::types::RunManifest; use fabro_client::ServerTarget; use fabro_config::user::active_settings_path; use fabro_config::{ServerSettingsBuilder, Storage, load_llm_catalog_settings}; -use fabro_db::Database; use fabro_interview::{ AnswerSubmission, ControlInterviewer, WORKER_CONTROL_INVALID_CURSOR_REASON, WORKER_CONTROL_PONG_TIMEOUT_REASON, WORKER_CONTROL_WS_LIVENESS_TIMEOUT, @@ -25,7 +24,7 @@ use fabro_types::{ ArtifactUpload, EventBody, FailureReason, Principal, RunBlobId, RunEvent, RunId, WorkflowSettings, }; -use fabro_vault::{SecretStore, Vault, import_legacy_json_once}; +use fabro_vault::{SecretStore, Vault}; use fabro_workflow::artifact_upload::{ArtifactSink, StageArtifactUploader}; use fabro_workflow::event::{Emitter, RunEventSink}; use fabro_workflow::operations::{self, StartServices}; @@ -279,17 +278,14 @@ async fn load_worker_vault(storage_dir: Option<&Path>) -> Result 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-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index 31ae4ca2f..0d465368f 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -10,8 +10,8 @@ use fabro_config::{Storage, envfile}; use fabro_static::EnvVars; use fabro_types::settings::run::EnvironmentProvider; use fabro_util::dev_token; -pub use fabro_vault::SecretStoreWrite as VaultSecretWrite; -use fabro_vault::{SecretStore, import_legacy_json_once}; +use fabro_vault::SecretStore; +pub use fabro_vault::SecretStoreWrite; #[derive(Debug, Clone, Copy)] pub struct PendingSettingsWrite<'a> { @@ -60,7 +60,7 @@ pub struct InstallPersistencePlan<'a> { 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, } @@ -577,7 +577,7 @@ fn persist_server_env_secrets( async fn persist_vault_secrets_direct( storage_dir: &Path, - secrets: &[VaultSecretWrite], + secrets: &[SecretStoreWrite], removals: &[String], ) -> Result<()> { if secrets.is_empty() && removals.is_empty() { @@ -585,9 +585,8 @@ async fn persist_vault_secrets_direct( } let storage = Storage::new(storage_dir); - let database = open_migrated_database(storage_dir).await?; - import_legacy_json_once(database.pool(), storage.secrets_path()).await?; - SecretStore::new(database.clone_pool()) + SecretStore::open(storage.sqlite_path(), storage.secrets_path()) + .await? .apply(removals, secrets) .await?; Ok(()) @@ -690,28 +689,13 @@ impl InstallPersistencePlan<'_> { Ok(()) } - - pub async fn persist_with_secret_store( - &self, - store: &SecretStore, - ) -> std::result::Result<(), PersistInstallOutputsError> { - let removed_env_keys = self.persist_files()?; - if let Err(err) = store - .apply(&self.vault_removals, &self.vault_writes) - .await - .map_err(anyhow::Error::new) - { - return Err(self.secret_persistence_error(err, removed_env_keys)); - } - Ok(()) - } } 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 { @@ -739,11 +723,9 @@ mod tests { use fabro_vault::{SecretType as VaultSecretType, Vault}; async fn load_secret_snapshot(storage: &Storage) -> Vault { - let database = fabro_db::Database::connect(storage.sqlite_path()) + SecretStore::open(storage.sqlite_path(), storage.secrets_path()) .await - .unwrap(); - database.migrate().await.unwrap(); - SecretStore::new(database.clone_pool()) + .unwrap() .snapshot() .await .unwrap() @@ -754,7 +736,7 @@ mod tests { InstallListenConfig, InstallObjectStoreCredentialMode, InstallObjectStoreSelection, InstallPersistencePlan, InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_MANAGED_COMMENT, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, - SecretStore, VaultSecretWrite, default_web_url, merge_server_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, @@ -1036,7 +1018,7 @@ stale = "remove-me" comment: None, }], &[], - &[VaultSecretWrite { + &[SecretStoreWrite { name: "bad-secret-name".to_string(), value: "boom".to_string(), secret_type: VaultSecretType::Token, @@ -1085,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, @@ -1129,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, @@ -1252,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, diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 81a66e138..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).await; DiagnosticsReport { version: FABRO_VERSION.to_string(), diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index eceade948..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, @@ -2502,10 +2502,9 @@ mod tests { assert!(!server_env.contains_key(EnvVars::GITHUB_APP_CLIENT_SECRET)); assert!(!server_env.contains_key(EnvVars::GITHUB_APP_WEBHOOK_SECRET)); - let database = fabro_db::Database::connect(storage.sqlite_path()) + let vault = fabro_vault::SecretStore::open(storage.sqlite_path(), storage.secrets_path()) .await - .unwrap(); - let vault = fabro_vault::SecretStore::new(database.clone_pool()) + .unwrap() .snapshot() .await .unwrap(); diff --git a/lib/crates/fabro-server/src/lib.rs b/lib/crates/fabro-server/src/lib.rs index e36c6db4f..5fc735ba0 100644 --- a/lib/crates/fabro-server/src/lib.rs +++ b/lib/crates/fabro-server/src/lib.rs @@ -57,6 +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, migrate_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/serve.rs b/lib/crates/fabro-server/src/serve.rs index 89904f54b..642fa9435 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -790,9 +790,8 @@ where max_concurrent_runs, store, artifact_store, - vault_path, db_pool, - preloaded_vault: Some(startup_vault.into_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 16abe31ed..cfba70bd7 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -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, @@ -1251,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, @@ -2354,7 +2352,6 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result anyhow::Result vault, - None => load_startup_vault(&vault_path)?, - }; + 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); @@ -2533,6 +2527,8 @@ pub(crate) fn build_app_state(config: AppStateConfig) -> anyhow::Result 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(), @@ -2689,9 +2691,8 @@ fn build_test_app_state_with_vault_path(vault_path: &Path) -> anyhow::Result) { } } -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()) diff --git a/lib/crates/fabro-server/src/test_support.rs b/lib/crates/fabro-server/src/test_support.rs index 2144b3c15..31f8aad5f 100644 --- a/lib/crates/fabro-server/src/test_support.rs +++ b/lib/crates/fabro-server/src/test_support.rs @@ -264,8 +264,7 @@ impl TestAppStateBuilder { store, artifact_store, db_pool, - vault_path, - preloaded_vault: Some(preloaded_vault), + 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, @@ -288,7 +287,7 @@ impl TestAppStateBuilder { clippy::disallowed_methods, reason = "sync test builders may run inside Tokio; a dedicated thread avoids a nested runtime" )] -fn test_secret_snapshot(pool: DbPool) -> anyhow::Result { +pub(crate) fn test_secret_snapshot(pool: DbPool) -> anyhow::Result { std::thread::spawn(move || { let runtime = TokioRuntimeBuilder::new_current_thread() .enable_all() 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/secret.rs b/lib/crates/fabro-types/src/secret.rs index ff23c6afe..d22d00d76 100644 --- a/lib/crates/fabro-types/src/secret.rs +++ b/lib/crates/fabro-types/src/secret.rs @@ -27,6 +27,13 @@ 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 { diff --git a/lib/crates/fabro-vault/Cargo.toml b/lib/crates/fabro-vault/Cargo.toml index 1b5bf635c..c3be81a0f 100644 --- a/lib/crates/fabro-vault/Cargo.toml +++ b/lib/crates/fabro-vault/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +anyhow.workspace = true chrono.workspace = true fabro-db = { path = "../fabro-db" } fabro-static = { path = "../fabro-static" } diff --git a/lib/crates/fabro-vault/src/lib.rs b/lib/crates/fabro-vault/src/lib.rs index 80e720b86..144ff7d23 100644 --- a/lib/crates/fabro-vault/src/lib.rs +++ b/lib/crates/fabro-vault/src/lib.rs @@ -110,6 +110,8 @@ impl Vault { }) } + /// 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 { diff --git a/lib/crates/fabro-vault/src/store.rs b/lib/crates/fabro-vault/src/store.rs index d40e59284..704eab6b8 100644 --- a/lib/crates/fabro-vault/src/store.rs +++ b/lib/crates/fabro-vault/src/store.rs @@ -4,7 +4,7 @@ use std::path::{Path, PathBuf}; use std::str::FromStr as _; use chrono::{DateTime, Utc}; -use fabro_db::DbPool; +use fabro_db::{Database, DbPool}; use fabro_types::{OAuthCredential, SecretMetadata, SecretType}; use sqlx::sqlite::SqliteRow; use sqlx::{Row as _, Sqlite, Transaction}; @@ -86,17 +86,14 @@ pub enum SecretStoreError { #[source] source: std::io::Error, }, - - #[error("secret row count {count} exceeds SQLite integer range")] - RowCountOverflow { count: usize }, } #[derive(Debug, Clone, PartialEq, Eq)] pub struct ImportReport { pub source_path: PathBuf, pub backup_path: PathBuf, - pub imported_rows: i64, - pub skipped_rows: i64, + pub imported_rows: usize, + pub skipped_rows: usize, pub secret_names: Vec, } @@ -160,6 +157,18 @@ impl SecretStore { 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 \ @@ -270,7 +279,7 @@ impl SecretStore { RETURNING name, secret_type, value, description, revision, created_at, updated_at ", ) - .bind(secret_type_string(secret_type)) + .bind(secret_type.as_str()) .bind(value) .bind(now) .bind(name) @@ -329,7 +338,7 @@ async fn upsert_secret( ", ) .bind(name) - .bind(secret_type_string(secret_type)) + .bind(secret_type.as_str()) .bind(value) .bind(description) .bind(now) @@ -391,8 +400,8 @@ pub async fn import_legacy_json_once( let report = ImportReport { source_path: source_path.to_path_buf(), backup_path, - imported_rows: row_count(imported_names.len())?, - skipped_rows: row_count(skipped_rows)?, + imported_rows: imported_names.len(), + skipped_rows, secret_names: imported_names, }; info!( @@ -420,7 +429,7 @@ async fn insert_legacy_entry( ", ) .bind(name) - .bind(secret_type_string(entry.secret_type)) + .bind(entry.secret_type.as_str()) .bind(&entry.value) .bind(entry.description.as_deref()) .bind(entry.created_at.to_rfc3339()) @@ -431,26 +440,10 @@ async fn insert_legacy_entry( } fn entry_from_row(row: &SqliteRow) -> Result<(String, SecretEntry), SecretStoreError> { - 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)?; - let created_at = parse_timestamp( - &name, - "created_at", - &row.try_get::("created_at")?, - )?; - let updated_at = parse_timestamp( - &name, - "updated_at", - &row.try_get::("updated_at")?, - )?; + let metadata = metadata_from_row(row)?; + let name = metadata.name; let value = row.try_get::("value")?; - if secret_type == SecretType::Oauth { + if metadata.secret_type == SecretType::Oauth { validate_oauth_json(&value).map_err(|source| SecretStoreError::StoredOauth { name: name.clone(), source, @@ -462,10 +455,10 @@ fn entry_from_row(row: &SqliteRow) -> Result<(String, SecretEntry), SecretStoreE } let entry = SecretEntry { value, - secret_type, - description: row.try_get("description")?, - created_at, - updated_at, + secret_type: metadata.secret_type, + description: metadata.description, + created_at: metadata.created_at, + updated_at: metadata.updated_at, revision, }; Ok((name, entry)) @@ -497,10 +490,6 @@ fn metadata_from_row(row: &SqliteRow) -> Result &'static str { - secret_type.into() -} - fn parse_timestamp( name: &str, column: &'static str, @@ -561,7 +550,3 @@ fn legacy_backup_path(source_path: &Path, imported_at: DateTime) -> PathBuf file_name.push(format!(".imported-{timestamp}.bak")); source_path.with_file_name(file_name) } - -fn row_count(count: usize) -> Result { - i64::try_from(count).map_err(|_| SecretStoreError::RowCountOverflow { count }) -}