From 01d60fa2815062ce5b2c9dd7241b920fe3f0ba16 Mon Sep 17 00:00:00 2001 From: italic-jinxin <106428113+italic-jinxin@users.noreply.github.com> Date: Wed, 25 Mar 2026 18:08:09 +0800 Subject: [PATCH] fix(security): store LLM API keys in encrypted secrets store instead of plaintext --- src/app.rs | 23 +- src/channels/web/handlers/settings.rs | 485 +++++++++++++++++++++- src/channels/web/mod.rs | 12 + src/channels/web/server.rs | 29 +- src/channels/web/static/app.js | 41 +- src/channels/web/test_helpers.rs | 1 + src/channels/web/tests/multi_tenant.rs | 1 + src/channels/web/ws.rs | 1 + src/config/mod.rs | 180 +++++++- src/main.rs | 3 + tests/multi_tenant_integration.rs | 2 + tests/openai_compat_integration.rs | 2 + tests/support/gateway_workflow_harness.rs | 1 + tests/ws_gateway_integration.rs | 1 + 14 files changed, 762 insertions(+), 20 deletions(-) diff --git a/src/app.rs b/src/app.rs index edd547d3..81390f93 100644 --- a/src/app.rs +++ b/src/app.rs @@ -229,18 +229,35 @@ impl AppBuilder { let store = crate::secrets::create_secrets_store(crypto, handles); if let Some(ref secrets) = store { + // Migrate any plaintext API keys from the settings table to the + // encrypted secrets store. Idempotent — safe to run on every startup. + if let Some(ref db) = self.db { + crate::config::migrate_plaintext_llm_keys( + db.as_ref(), + secrets.as_ref(), + &self.config.owner_id, + ) + .await; + } + // Inject LLM API keys from encrypted storage crate::config::inject_llm_keys_from_secrets(secrets.as_ref(), &self.config.owner_id) .await; - // Re-resolve only the LLM config with newly available keys. - let store: Option<&(dyn crate::db::SettingsStore + Sync)> = + // Re-resolve only the LLM config with newly available keys, + // including keys hydrated from the secrets store. + let settings_store: Option<&(dyn crate::db::SettingsStore + Sync)> = self.db.as_ref().map(|db| db.as_ref() as _); let toml_path = self.toml_path.as_deref(); let owner_id = self.config.owner_id.clone(); if let Err(e) = self .config - .re_resolve_llm(store, &owner_id, toml_path) + .re_resolve_llm_with_secrets( + settings_store, + &owner_id, + toml_path, + Some(secrets.as_ref()), + ) .await { tracing::warn!("Failed to re-resolve LLM config after secret injection: {e}"); diff --git a/src/channels/web/handlers/settings.rs b/src/channels/web/handlers/settings.rs index 6d980b25..ade5fe13 100644 --- a/src/channels/web/handlers/settings.rs +++ b/src/channels/web/handlers/settings.rs @@ -7,10 +7,15 @@ use axum::{ extract::{Path, State}, http::StatusCode, }; +use secrecy::SecretString; use crate::channels::web::auth::AuthenticatedUser; use crate::channels::web::server::GatewayState; use crate::channels::web::types::*; +use crate::secrets::{CreateSecretParams, SecretsStore}; + +/// Sentinel value the frontend sends to mean "key is unchanged, don't touch it". +const API_KEY_UNCHANGED: &str = "••••••••"; pub async fn settings_list_handler( State(state): State>, @@ -79,8 +84,20 @@ pub async fn settings_set_handler( validate_custom_providers_adapters(&body.value)?; } + // Extract API keys from LLM settings and vault them in the secrets store. + // The sanitized value has api_key fields removed (stored encrypted instead). + let sanitized_value = match key.as_str() { + "llm_builtin_overrides" => { + extract_builtin_override_keys(&state, &user.user_id, &body.value).await? + } + "llm_custom_providers" => { + extract_custom_provider_keys(&state, &user.user_id, &body.value).await? + } + _ => body.value.clone(), + }; + store - .set_setting(&user.user_id, &key, &body.value) + .set_setting(&user.user_id, &key, &sanitized_value) .await .map_err(|e| { tracing::error!("Failed to set setting '{}': {}", key, e); @@ -202,11 +219,16 @@ pub async fn settings_export_handler( .store .as_ref() .ok_or(StatusCode::SERVICE_UNAVAILABLE)?; - let settings = store.get_all_settings(&user.user_id).await.map_err(|e| { + let mut settings = store.get_all_settings(&user.user_id).await.map_err(|e| { tracing::error!("Failed to export settings: {}", e); StatusCode::INTERNAL_SERVER_ERROR })?; + // Indicate key presence from secrets store without exposing values. + annotate_secret_key_presence(&state, &user.user_id, &mut settings).await; + + mask_settings_api_keys(&mut settings); + Ok(Json(SettingsExportResponse { settings })) } @@ -229,3 +251,462 @@ pub async fn settings_import_handler( Ok(StatusCode::NO_CONTENT) } + +// --------------------------------------------------------------------------- +// LLM API key vaulting helpers +// --------------------------------------------------------------------------- + +/// Canonical secret name for a built-in provider's API key. +fn builtin_secret_name(provider_id: &str) -> String { + format!("llm_builtin_{}_api_key", provider_id) +} + +/// Canonical secret name for a custom provider's API key. +fn custom_secret_name(provider_id: &str) -> String { + format!("llm_custom_{}_api_key", provider_id) +} + +/// Extract API keys from builtin overrides, store in secrets, return sanitized JSON. +async fn extract_builtin_override_keys( + state: &GatewayState, + user_id: &str, + value: &serde_json::Value, +) -> Result { + let secrets = match state.secrets_store.as_ref() { + Some(s) => s, + None => return Ok(value.clone()), + }; + + let obj = match value.as_object() { + Some(o) => o, + None => return Ok(value.clone()), + }; + + let mut sanitized = obj.clone(); + + for (provider_id, override_val) in obj { + if let Some(api_key) = override_val.get("api_key").and_then(|v| v.as_str()) { + if api_key == API_KEY_UNCHANGED || api_key.is_empty() { + // Unchanged or empty — remove from settings, keep existing secret. + if let Some(o) = sanitized + .get_mut(provider_id) + .and_then(|v| v.as_object_mut()) + { + o.remove("api_key"); + } + continue; + } + vault_secret( + secrets.as_ref(), + user_id, + &builtin_secret_name(provider_id), + api_key, + provider_id, + ) + .await?; + if let Some(o) = sanitized + .get_mut(provider_id) + .and_then(|v| v.as_object_mut()) + { + o.remove("api_key"); + } + } + } + + Ok(serde_json::Value::Object(sanitized)) +} + +/// Extract API keys from custom providers, store in secrets, return sanitized JSON. +async fn extract_custom_provider_keys( + state: &GatewayState, + user_id: &str, + value: &serde_json::Value, +) -> Result { + let secrets = match state.secrets_store.as_ref() { + Some(s) => s, + None => return Ok(value.clone()), + }; + + let arr = match value.as_array() { + Some(a) => a, + None => return Ok(value.clone()), + }; + + let mut sanitized = arr.clone(); + + for (idx, provider_val) in arr.iter().enumerate() { + let provider_id = provider_val + .get("id") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if provider_id.is_empty() { + continue; + } + + if let Some(api_key) = provider_val.get("api_key").and_then(|v| v.as_str()) { + if api_key == API_KEY_UNCHANGED || api_key.is_empty() { + if let Some(o) = sanitized[idx].as_object_mut() { + o.remove("api_key"); + } + continue; + } + vault_secret( + secrets.as_ref(), + user_id, + &custom_secret_name(provider_id), + api_key, + provider_id, + ) + .await?; + if let Some(o) = sanitized[idx].as_object_mut() { + o.remove("api_key"); + } + } + } + + Ok(serde_json::Value::Array(sanitized)) +} + +/// Encrypt and store an API key in the secrets store. +async fn vault_secret( + secrets: &(dyn SecretsStore + Send + Sync), + user_id: &str, + secret_name: &str, + api_key: &str, + provider_id: &str, +) -> Result<(), StatusCode> { + secrets + .create( + user_id, + CreateSecretParams { + name: secret_name.to_string(), + value: SecretString::from(api_key.to_string()), + provider: Some(provider_id.to_string()), + expires_at: None, + }, + ) + .await + .map_err(|e| { + tracing::error!( + "Failed to store secret '{}' for provider '{}': {}", + secret_name, + provider_id, + e + ); + StatusCode::INTERNAL_SERVER_ERROR + })?; + Ok(()) +} + +/// Mask plaintext API keys in settings values before returning to the frontend. +/// +/// Any `api_key` field still present in the settings JSON (legacy plaintext) +/// is replaced with the sentinel so the frontend shows "key configured". +fn mask_settings_api_keys(settings: &mut std::collections::HashMap) { + if let Some(obj) = settings + .get_mut("llm_builtin_overrides") + .and_then(|v| v.as_object_mut()) + { + for override_val in obj.values_mut() { + if let Some(o) = override_val.as_object_mut() + && o.contains_key("api_key") + { + o.insert( + "api_key".to_string(), + serde_json::Value::String(API_KEY_UNCHANGED.to_string()), + ); + } + } + } + + if let Some(arr) = settings + .get_mut("llm_custom_providers") + .and_then(|v| v.as_array_mut()) + { + for provider_val in arr.iter_mut() { + if let Some(o) = provider_val.as_object_mut() + && o.contains_key("api_key") + { + o.insert( + "api_key".to_string(), + serde_json::Value::String(API_KEY_UNCHANGED.to_string()), + ); + } + } + } +} + +/// Check the secrets store for vaulted API keys and annotate the settings map. +/// +/// For builtin overrides and custom providers whose API key was stripped from +/// settings (stored in secrets), this adds `api_key: "••••••••"` so the +/// frontend knows a key is configured without seeing the actual value. +async fn annotate_secret_key_presence( + state: &GatewayState, + user_id: &str, + settings: &mut std::collections::HashMap, +) { + let secrets = match state.secrets_store.as_ref() { + Some(s) => s, + None => return, + }; + + // Annotate builtin overrides + if let Some(obj) = settings + .get_mut("llm_builtin_overrides") + .and_then(|v| v.as_object_mut()) + { + let provider_ids: Vec = obj.keys().cloned().collect(); + for provider_id in provider_ids { + let has_key_in_settings = obj + .get(&provider_id) + .and_then(|v| v.get("api_key")) + .is_some(); + if has_key_in_settings { + continue; // Will be masked by mask_settings_api_keys + } + let secret_name = builtin_secret_name(&provider_id); + if secrets.exists(user_id, &secret_name).await.unwrap_or(false) + && let Some(o) = obj.get_mut(&provider_id).and_then(|v| v.as_object_mut()) + { + o.insert( + "api_key".to_string(), + serde_json::Value::String(API_KEY_UNCHANGED.to_string()), + ); + } + } + } + + // Annotate custom providers + if let Some(arr) = settings + .get_mut("llm_custom_providers") + .and_then(|v| v.as_array_mut()) + { + for provider_val in arr.iter_mut() { + let provider_id = provider_val + .get("id") + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string(); + if provider_id.is_empty() { + continue; + } + let has_key_in_settings = provider_val.get("api_key").is_some(); + if has_key_in_settings { + continue; + } + let secret_name = custom_secret_name(&provider_id); + if secrets.exists(user_id, &secret_name).await.unwrap_or(false) + && let Some(o) = provider_val.as_object_mut() + { + o.insert( + "api_key".to_string(), + serde_json::Value::String(API_KEY_UNCHANGED.to_string()), + ); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::collections::HashMap; + + #[test] + fn test_mask_settings_api_keys_builtin_overrides() { + let mut settings = HashMap::new(); + settings.insert( + "llm_builtin_overrides".to_string(), + serde_json::json!({ + "openai": { "api_key": "sk-secret-123", "model": "gpt-4" }, + "anthropic": { "model": "claude-3" } + }), + ); + + mask_settings_api_keys(&mut settings); + + let overrides = settings["llm_builtin_overrides"].as_object().unwrap(); + assert_eq!( + overrides["openai"]["api_key"].as_str().unwrap(), + API_KEY_UNCHANGED, + ); + assert_eq!(overrides["openai"]["model"].as_str().unwrap(), "gpt-4"); + assert!(overrides["anthropic"].get("api_key").is_none()); + } + + #[test] + fn test_mask_settings_api_keys_custom_providers() { + let mut settings = HashMap::new(); + settings.insert( + "llm_custom_providers".to_string(), + serde_json::json!([ + { "id": "my-llm", "api_key": "secret-key", "adapter": "open_ai_completions" }, + { "id": "no-key", "adapter": "ollama" } + ]), + ); + + mask_settings_api_keys(&mut settings); + + let providers = settings["llm_custom_providers"].as_array().unwrap(); + assert_eq!(providers[0]["api_key"].as_str().unwrap(), API_KEY_UNCHANGED,); + assert!(providers[1].get("api_key").is_none()); + } + + #[test] + fn test_mask_settings_no_llm_keys_is_noop() { + let mut settings = HashMap::new(); + settings.insert("some_other_setting".to_string(), serde_json::json!("value")); + + mask_settings_api_keys(&mut settings); + + assert_eq!(settings["some_other_setting"].as_str().unwrap(), "value"); + } + + #[test] + fn test_builtin_secret_name_format() { + assert_eq!(builtin_secret_name("openai"), "llm_builtin_openai_api_key"); + } + + #[test] + fn test_custom_secret_name_format() { + assert_eq!(custom_secret_name("my-groq"), "llm_custom_my-groq_api_key"); + } + + fn test_secrets_store() -> Arc { + let crypto = Arc::new( + crate::secrets::SecretsCrypto::new(secrecy::SecretString::from( + crate::secrets::keychain::generate_master_key_hex(), + )) + .unwrap(), + ); + Arc::new(crate::secrets::InMemorySecretsStore::new(crypto)) + } + + fn test_gateway_state(secrets: Arc) -> GatewayState { + GatewayState { + msg_tx: tokio::sync::RwLock::new(None), + sse: Arc::new(crate::channels::web::sse::SseManager::new()), + workspace: None, + workspace_pool: None, + session_manager: None, + log_broadcaster: None, + log_level_handle: None, + extension_manager: None, + tool_registry: None, + store: None, + job_manager: None, + prompt_queue: None, + scheduler: None, + default_user_id: "test".to_string(), + shutdown_tx: tokio::sync::RwLock::new(None), + ws_tracker: None, + llm_provider: None, + skill_registry: None, + skill_catalog: None, + chat_rate_limiter: crate::channels::web::server::PerUserRateLimiter::new(30, 60), + oauth_rate_limiter: crate::channels::web::server::RateLimiter::new(10, 60), + webhook_rate_limiter: crate::channels::web::server::RateLimiter::new(10, 60), + registry_entries: Vec::new(), + cost_guard: None, + routine_engine: Arc::new(tokio::sync::RwLock::new(None)), + startup_time: std::time::Instant::now(), + active_config: crate::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: Some(secrets), + } + } + + #[tokio::test] + async fn test_extract_builtin_keys_vaults_and_strips() { + let secrets = test_secrets_store(); + let state = test_gateway_state(Arc::clone(&secrets)); + + let input = serde_json::json!({ + "openai": { "api_key": "sk-test-key", "model": "gpt-4" }, + "anthropic": { "model": "claude-3" } + }); + + let result = extract_builtin_override_keys(&state, "test", &input) + .await + .unwrap(); + + let obj = result.as_object().unwrap(); + assert!( + obj["openai"].get("api_key").is_none(), + "api_key should be stripped" + ); + assert_eq!(obj["openai"]["model"].as_str().unwrap(), "gpt-4"); + assert_eq!(obj["anthropic"]["model"].as_str().unwrap(), "claude-3"); + + let decrypted = secrets + .get_decrypted("test", "llm_builtin_openai_api_key") + .await + .unwrap(); + assert_eq!(decrypted.expose(), "sk-test-key"); + } + + #[tokio::test] + async fn test_extract_custom_keys_vaults_and_strips() { + let secrets = test_secrets_store(); + let state = test_gateway_state(Arc::clone(&secrets)); + + let input = serde_json::json!([ + { "id": "my-llm", "api_key": "gsk-custom-key", "adapter": "open_ai_completions" }, + { "id": "local", "adapter": "ollama" } + ]); + + let result = extract_custom_provider_keys(&state, "test", &input) + .await + .unwrap(); + + let arr = result.as_array().unwrap(); + assert!( + arr[0].get("api_key").is_none(), + "api_key should be stripped" + ); + assert_eq!(arr[0]["id"].as_str().unwrap(), "my-llm"); + assert!(arr[1].get("api_key").is_none()); + + let decrypted = secrets + .get_decrypted("test", "llm_custom_my-llm_api_key") + .await + .unwrap(); + assert_eq!(decrypted.expose(), "gsk-custom-key"); + } + + #[tokio::test] + async fn test_unchanged_sentinel_preserves_existing_secret() { + let secrets = test_secrets_store(); + + secrets + .create( + "test", + CreateSecretParams { + name: "llm_builtin_openai_api_key".to_string(), + value: SecretString::from("sk-original".to_string()), + provider: Some("openai".to_string()), + expires_at: None, + }, + ) + .await + .unwrap(); + + let state = test_gateway_state(Arc::clone(&secrets)); + + let input = serde_json::json!({ + "openai": { "api_key": "••••••••", "model": "gpt-4" } + }); + + let result = extract_builtin_override_keys(&state, "test", &input) + .await + .unwrap(); + + assert!(result["openai"].get("api_key").is_none()); + + let decrypted = secrets + .get_decrypted("test", "llm_builtin_openai_api_key") + .await + .unwrap(); + assert_eq!(decrypted.expose(), "sk-original"); + } +} diff --git a/src/channels/web/mod.rs b/src/channels/web/mod.rs index b26a7829..a1a89611 100644 --- a/src/channels/web/mod.rs +++ b/src/channels/web/mod.rs @@ -112,6 +112,7 @@ impl GatewayChannel { routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: server::ActiveConfigSnapshot::default(), + secrets_store: None, }); Self { @@ -151,6 +152,7 @@ impl GatewayChannel { startup_time: std::time::Instant::now(), webhook_rate_limiter: server::RateLimiter::new(10, 60), active_config: server::ActiveConfigSnapshot::default(), + secrets_store: None, }); Self { @@ -191,6 +193,7 @@ impl GatewayChannel { routine_engine: Arc::clone(&self.state.routine_engine), startup_time: self.state.startup_time, active_config: self.state.active_config.clone(), + secrets_store: self.state.secrets_store.clone(), }; mutate(&mut new_state); self.state = Arc::new(new_state); @@ -308,6 +311,15 @@ impl GatewayChannel { self } + /// Inject the secrets store for encrypting LLM API keys in settings handlers. + pub fn with_secrets_store( + mut self, + ss: Arc, + ) -> Self { + self.rebuild_state(|s| s.secrets_store = Some(ss)); + self + } + /// Inject the per-user workspace pool for multi-user mode. pub fn with_workspace_pool(mut self, pool: Arc) -> Self { self.rebuild_state(|s| s.workspace_pool = Some(pool)); diff --git a/src/channels/web/server.rs b/src/channels/web/server.rs index 2cd98a09..16eb14da 100644 --- a/src/channels/web/server.rs +++ b/src/channels/web/server.rs @@ -381,6 +381,8 @@ pub struct GatewayState { pub startup_time: std::time::Instant, /// Snapshot of active (resolved) configuration for the frontend. pub active_config: ActiveConfigSnapshot, + /// Secrets store for encrypting LLM API keys (and future sensitive settings). + pub secrets_store: Option>, } /// Start the gateway HTTP server. @@ -2955,9 +2957,11 @@ async fn llm_env_defaults_handler() -> Json { // NEAR AI is a special case (not in the registry) { let mut entry = serde_json::Map::new(); - if let Some(key) = read_env("NEARAI_API_KEY") { - entry.insert("api_key".to_string(), serde_json::Value::String(key)); - } + // Only expose presence of API key, never the value itself. + entry.insert( + "has_api_key".to_string(), + serde_json::Value::Bool(read_env("NEARAI_API_KEY").is_some()), + ); if let Some(model) = read_env("NEARAI_MODEL") { entry.insert("model".to_string(), serde_json::Value::String(model)); } @@ -2971,10 +2975,11 @@ async fn llm_env_defaults_handler() -> Json { for def in registry.all() { let mut entry = serde_json::Map::new(); - if let Some(ref api_key_env) = def.api_key_env - && let Some(key) = read_env(api_key_env) - { - entry.insert("api_key".to_string(), serde_json::Value::String(key)); + if let Some(ref api_key_env) = def.api_key_env { + entry.insert( + "has_api_key".to_string(), + serde_json::Value::Bool(read_env(api_key_env).is_some()), + ); } if let Some(model) = read_env(&def.model_env) { @@ -3257,9 +3262,14 @@ mod tests { .get("nearai") .and_then(|v| v.as_object()) .expect("nearai entry"); + // API key should NOT be exposed — only has_api_key presence flag. assert_eq!( - nearai.get("api_key").and_then(|v| v.as_str()), - Some("test-key-123") + nearai.get("has_api_key").and_then(|v| v.as_bool()), + Some(true) + ); + assert!( + nearai.get("api_key").is_none(), + "raw api_key must never be returned" ); assert_eq!( nearai.get("model").and_then(|v| v.as_str()), @@ -3326,6 +3336,7 @@ mod tests { routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: ActiveConfigSnapshot::default(), + secrets_store: None, }) } diff --git a/src/channels/web/static/app.js b/src/channels/web/static/app.js index 2fcacd50..9dac47bb 100644 --- a/src/channels/web/static/app.js +++ b/src/channels/web/static/app.js @@ -6374,7 +6374,14 @@ function editCustomProvider(id) { idField.style.opacity = '0.6'; document.getElementById('provider-adapter').value = p.adapter || 'open_ai_completions'; document.getElementById('provider-base-url').value = p.base_url || ''; - document.getElementById('provider-api-key').value = p.api_key || ''; + const editApiKeyInput = document.getElementById('provider-api-key'); + if (p.api_key === '••••••••') { + editApiKeyInput.value = ''; + editApiKeyInput.placeholder = 'Key configured (leave blank to keep)'; + } else { + editApiKeyInput.value = ''; + editApiKeyInput.placeholder = 'Enter API key'; + } document.getElementById('provider-model').value = p.default_model || ''; openProviderDialog(true); document.getElementById('provider-name').focus(); @@ -6404,8 +6411,16 @@ function configureBuiltinProvider(id) { document.getElementById('provider-api-key-row').style.display = p.api_key_required !== false ? '' : 'none'; document.getElementById('fetch-models-btn').style.display = p.can_list_models ? '' : 'none'; const apiKeyInput = document.getElementById('provider-api-key'); - apiKeyInput.value = override.api_key || envDef.api_key || ''; - apiKeyInput.placeholder = ''; + const hasDbKey = override.api_key === '••••••••'; + const hasEnvKey = envDef.has_api_key === true; + apiKeyInput.value = ''; + if (hasDbKey) { + apiKeyInput.placeholder = 'Key configured (leave blank to keep)'; + } else if (hasEnvKey) { + apiKeyInput.placeholder = 'Key set via environment variable'; + } else { + apiKeyInput.placeholder = 'Enter API key'; + } document.getElementById('provider-model').value = override.model || envDef.model || p.default_model || ''; openProviderDialog(true); document.getElementById('provider-model').focus(); @@ -6495,8 +6510,15 @@ document.getElementById('save-provider-btn').addEventListener('click', () => { const model = document.getElementById('provider-model').value.trim(); const baseUrl = document.getElementById('provider-base-url').value.trim(); const id = _configuringBuiltinId; + const prevOverride = _builtinOverrides[id] || {}; + const hadKey = prevOverride.api_key === '••••••••'; const override = {}; - if (apiKey) override.api_key = apiKey; + if (apiKey) { + override.api_key = apiKey; // New key entered — backend will encrypt it + } else if (hadKey) { + override.api_key = '••••••••'; // Sentinel: keep existing encrypted key + } + // If neither — key is cleared (no key configured) if (model) override.model = model; if (baseUrl) override.base_url = baseUrl; const prev = _builtinOverrides[id]; @@ -6543,7 +6565,16 @@ document.getElementById('save-provider-btn').addEventListener('click', () => { const idx = _customProviders.findIndex((p) => p.id === _editingProviderId); if (idx === -1) return; const original = _customProviders[idx]; - _customProviders[idx] = { ...original, name, adapter, base_url: baseUrl, default_model: model || undefined, api_key: apiKey || undefined }; + const hadCustomKey = original.api_key === '••••••••'; + let effectiveApiKey; + if (apiKey) { + effectiveApiKey = apiKey; // New key — backend will encrypt it + } else if (hadCustomKey) { + effectiveApiKey = '••••••••'; // Sentinel: keep existing encrypted key + } else { + effectiveApiKey = undefined; // No key + } + _customProviders[idx] = { ...original, name, adapter, base_url: baseUrl, default_model: model || undefined, api_key: effectiveApiKey }; const isActive = _editingProviderId === _activeLlmBackend; const modelUpdate = () => { if (!isActive) return Promise.resolve(); diff --git a/src/channels/web/test_helpers.rs b/src/channels/web/test_helpers.rs index 802512a6..07a05573 100644 --- a/src/channels/web/test_helpers.rs +++ b/src/channels/web/test_helpers.rs @@ -91,6 +91,7 @@ impl TestGatewayBuilder { routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: crate::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, }) } diff --git a/src/channels/web/tests/multi_tenant.rs b/src/channels/web/tests/multi_tenant.rs index 55010831..6b0f533e 100644 --- a/src/channels/web/tests/multi_tenant.rs +++ b/src/channels/web/tests/multi_tenant.rs @@ -79,6 +79,7 @@ fn build_state( routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: ActiveConfigSnapshot::default(), + secrets_store: None, }) } diff --git a/src/channels/web/ws.rs b/src/channels/web/ws.rs index 3a601679..9e75bafb 100644 --- a/src/channels/web/ws.rs +++ b/src/channels/web/ws.rs @@ -534,6 +534,7 @@ mod tests { routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: crate::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, } } } diff --git a/src/config/mod.rs b/src/config/mod.rs index cce4dd03..f01e0902 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -298,7 +298,19 @@ impl Config { user_id: &str, toml_path: Option<&std::path::Path>, ) -> Result<(), ConfigError> { - let settings = if let Some(store) = store { + self.re_resolve_llm_with_secrets(store, user_id, toml_path, None) + .await + } + + /// Re-resolve LLM config, hydrating API keys from the secrets store. + pub async fn re_resolve_llm_with_secrets( + &mut self, + store: Option<&(dyn crate::db::SettingsStore + Sync)>, + user_id: &str, + toml_path: Option<&std::path::Path>, + secrets: Option<&(dyn crate::secrets::SecretsStore + Send + Sync)>, + ) -> Result<(), ConfigError> { + let mut settings = if let Some(store) = store { // TOML as base, then DB on top (DB wins). let mut s = Settings::default(); Self::apply_toml_overlay(&mut s, toml_path)?; @@ -310,6 +322,14 @@ impl Config { } else { Settings::default() }; + + // Hydrate API keys from encrypted secrets store into the settings + // struct so that LlmConfig::resolve() sees them without any changes + // to its synchronous resolution logic. + if let Some(secrets) = secrets { + hydrate_llm_keys_from_secrets(&mut settings, secrets, user_id).await; + } + self.llm = LlmConfig::resolve(&settings)?; Ok(()) } @@ -512,3 +532,161 @@ fn inject_os_credential_store_tokens(injected: &mut HashMap) { tracing::debug!("Refreshed ANTHROPIC_OAUTH_TOKEN from OS credential store"); } } + +/// Hydrate LLM API keys from the secrets store into the settings struct. +/// +/// Called after loading settings from DB but before `LlmConfig::resolve()`. +/// Populates `api_key` fields that were stripped from settings during the +/// write path and stored encrypted in the secrets store instead. +pub async fn hydrate_llm_keys_from_secrets( + settings: &mut Settings, + secrets: &(dyn crate::secrets::SecretsStore + Send + Sync), + user_id: &str, +) { + // Hydrate builtin overrides + for (provider_id, override_val) in settings.llm_builtin_overrides.iter_mut() { + if override_val.api_key.is_some() { + continue; // Already has a key (legacy plaintext or TOML) + } + let secret_name = format!("llm_builtin_{}_api_key", provider_id); + if let Ok(decrypted) = secrets.get_decrypted(user_id, &secret_name).await { + override_val.api_key = Some(decrypted.expose().to_string()); + } + } + + // Hydrate custom providers + for provider in settings.llm_custom_providers.iter_mut() { + if provider.api_key.is_some() { + continue; + } + let secret_name = format!("llm_custom_{}_api_key", provider.id); + if let Ok(decrypted) = secrets.get_decrypted(user_id, &secret_name).await { + provider.api_key = Some(decrypted.expose().to_string()); + } + } +} + +/// Migrate plaintext API keys from the settings table to the encrypted secrets store. +/// +/// Idempotent: skips keys that are already in the secrets store. +/// After migration, strips plaintext keys from the settings table. +pub async fn migrate_plaintext_llm_keys( + settings_store: &(dyn crate::db::SettingsStore + Sync), + secrets: &(dyn crate::secrets::SecretsStore + Send + Sync), + user_id: &str, +) { + let settings_map = match settings_store.get_all_settings(user_id).await { + Ok(m) => m, + Err(_) => return, + }; + + let mut migrated = 0u32; + + // Migrate builtin overrides + if let Some(obj) = settings_map + .get("llm_builtin_overrides") + .and_then(|v| v.as_object()) + { + let mut sanitized = obj.clone(); + for (provider_id, override_val) in obj { + if let Some(api_key) = override_val.get("api_key").and_then(|v| v.as_str()) { + if api_key.is_empty() { + continue; + } + let secret_name = format!("llm_builtin_{}_api_key", provider_id); + if !secrets.exists(user_id, &secret_name).await.unwrap_or(false) + && let Err(e) = secrets + .create( + user_id, + crate::secrets::CreateSecretParams { + name: secret_name.clone(), + value: secrecy::SecretString::from(api_key.to_string()), + provider: Some(provider_id.clone()), + expires_at: None, + }, + ) + .await + { + tracing::warn!("Failed to migrate key for builtin '{}': {}", provider_id, e); + continue; + } + if let Some(o) = sanitized + .get_mut(provider_id) + .and_then(|v| v.as_object_mut()) + { + o.remove("api_key"); + } + migrated += 1; + } + } + if migrated > 0 { + let _ = settings_store + .set_setting( + user_id, + "llm_builtin_overrides", + &serde_json::Value::Object(sanitized), + ) + .await; + } + } + + // Migrate custom providers + let before = migrated; + if let Some(arr) = settings_map + .get("llm_custom_providers") + .and_then(|v| v.as_array()) + { + let mut sanitized = arr.clone(); + for (idx, provider_val) in arr.iter().enumerate() { + let provider_id = provider_val + .get("id") + .and_then(|v| v.as_str()) + .unwrap_or(""); + if provider_id.is_empty() { + continue; + } + if let Some(api_key) = provider_val.get("api_key").and_then(|v| v.as_str()) { + if api_key.is_empty() { + continue; + } + let secret_name = format!("llm_custom_{}_api_key", provider_id); + if !secrets.exists(user_id, &secret_name).await.unwrap_or(false) + && let Err(e) = secrets + .create( + user_id, + crate::secrets::CreateSecretParams { + name: secret_name.clone(), + value: secrecy::SecretString::from(api_key.to_string()), + provider: Some(provider_id.to_string()), + expires_at: None, + }, + ) + .await + { + tracing::warn!("Failed to migrate key for custom '{}': {}", provider_id, e); + continue; + } + if let Some(o) = sanitized[idx].as_object_mut() { + o.remove("api_key"); + } + migrated += 1; + } + } + if migrated > before { + let _ = settings_store + .set_setting( + user_id, + "llm_custom_providers", + &serde_json::Value::Array(sanitized), + ) + .await; + } + } + + if migrated > 0 { + tracing::info!( + "Migrated {} plaintext LLM API key(s) to encrypted secrets store", + migrated + ); + } +} diff --git a/src/main.rs b/src/main.rs index eab01264..899141f8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -650,6 +650,9 @@ async fn async_main() -> anyhow::Result<()> { if let Some(ref d) = components.db { gw = gw.with_store(Arc::clone(d)); } + if let Some(ref ss) = components.secrets_store { + gw = gw.with_secrets_store(Arc::clone(ss)); + } if let Some(ref jm) = container_job_manager { gw = gw.with_job_manager(Arc::clone(jm)); } diff --git a/tests/multi_tenant_integration.rs b/tests/multi_tenant_integration.rs index 02eb60e8..bf7c5b35 100644 --- a/tests/multi_tenant_integration.rs +++ b/tests/multi_tenant_integration.rs @@ -551,6 +551,7 @@ fn gateway_state_has_multi_tenant_fields() { startup_time: std::time::Instant::now(), webhook_rate_limiter: RateLimiter::new(10, 60), active_config: Default::default(), + secrets_store: None, }; assert_eq!(state.default_user_id, "fallback"); @@ -902,6 +903,7 @@ async fn start_multi_user_server_with_db() -> ( startup_time: std::time::Instant::now(), webhook_rate_limiter: RateLimiter::new(10, 60), active_config: Default::default(), + secrets_store: None, }); let addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); diff --git a/tests/openai_compat_integration.rs b/tests/openai_compat_integration.rs index 16568246..1ade2838 100644 --- a/tests/openai_compat_integration.rs +++ b/tests/openai_compat_integration.rs @@ -217,6 +217,7 @@ async fn start_test_server_with_provider( routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: ironclaw::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, }); let auth = ironclaw::channels::web::auth::MultiAuthState::single( @@ -715,6 +716,7 @@ async fn test_no_llm_provider_returns_503() { routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: ironclaw::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, }); let auth = ironclaw::channels::web::auth::MultiAuthState::single( diff --git a/tests/support/gateway_workflow_harness.rs b/tests/support/gateway_workflow_harness.rs index e4620f70..0615494e 100644 --- a/tests/support/gateway_workflow_harness.rs +++ b/tests/support/gateway_workflow_harness.rs @@ -240,6 +240,7 @@ impl GatewayWorkflowHarness { routine_engine: Arc::clone(&routine_slot), startup_time: Instant::now(), active_config: ironclaw::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, }); let mut agent = Agent::new( diff --git a/tests/ws_gateway_integration.rs b/tests/ws_gateway_integration.rs index 43277389..f96b8382 100644 --- a/tests/ws_gateway_integration.rs +++ b/tests/ws_gateway_integration.rs @@ -65,6 +65,7 @@ async fn start_test_server() -> ( routine_engine: Arc::new(tokio::sync::RwLock::new(None)), startup_time: std::time::Instant::now(), active_config: ironclaw::channels::web::server::ActiveConfigSnapshot::default(), + secrets_store: None, }); let auth = ironclaw::channels::web::auth::MultiAuthState::single(