diff --git a/Cargo.lock b/Cargo.lock index 98dda030c..19754876c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1525,6 +1525,27 @@ dependencies = [ "uuid", ] +[[package]] +name = "fabro-auth" +version = "0.176.2" +dependencies = [ + "anyhow", + "async-trait", + "base64", + "chrono", + "fabro-http", + "fabro-model", + "fabro-oauth", + "fabro-vault", + "httpmock", + "serde", + "serde_json", + "shlex", + "tempfile", + "thiserror 2.0.18", + "tokio", +] + [[package]] name = "fabro-checkpoint" version = "0.176.2" @@ -1561,6 +1582,7 @@ dependencies = [ "dotenvy", "fabro-agent", "fabro-api", + "fabro-auth", "fabro-checkpoint", "fabro-config", "fabro-devcontainer", @@ -1753,6 +1775,7 @@ dependencies = [ "async-trait", "base64", "bytes", + "fabro-auth", "fabro-http", "fabro-macros", "fabro-model", @@ -1898,6 +1921,7 @@ dependencies = [ "dirs", "fabro-agent", "fabro-api", + "fabro-auth", "fabro-config", "fabro-github", "fabro-graphviz", @@ -2147,6 +2171,7 @@ dependencies = [ "chrono", "dirs", "fabro-agent", + "fabro-auth", "fabro-checkpoint", "fabro-config", "fabro-core", @@ -2168,6 +2193,7 @@ dependencies = [ "fabro-types", "fabro-util", "fabro-validate", + "fabro-vault", "futures", "git2", "hex", diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml index 1751ccc8c..1c6a9817a 100644 --- a/docs/api-reference/fabro-api.yaml +++ b/docs/api-reference/fabro-api.yaml @@ -4353,6 +4353,7 @@ components: enum: - environment - file + - credential CreateSecretRequest: description: Request to store or update a secret. diff --git a/lib/crates/fabro-auth/Cargo.toml b/lib/crates/fabro-auth/Cargo.toml new file mode 100644 index 000000000..747e943ba --- /dev/null +++ b/lib/crates/fabro-auth/Cargo.toml @@ -0,0 +1,30 @@ +[package] +name = "fabro-auth" +edition.workspace = true +version.workspace = true +publish = false +license.workspace = true +description = "Typed provider credential storage and resolution for Fabro" + +[lints] +workspace = true + +[dependencies] +anyhow.workspace = true +async-trait.workspace = true +base64.workspace = true +chrono = { workspace = true, features = ["serde"] } +fabro-http.workspace = true +fabro-model = { path = "../fabro-model" } +fabro-oauth = { path = "../fabro-oauth" } +fabro-vault = { path = "../fabro-vault" } +serde.workspace = true +serde_json.workspace = true +shlex = "1" +thiserror.workspace = true +tokio.workspace = true + +[dev-dependencies] +httpmock = "0.8" +tempfile = "3" +tokio = { workspace = true, features = ["macros", "test-util"] } diff --git a/lib/crates/fabro-auth/src/context.rs b/lib/crates/fabro-auth/src/context.rs new file mode 100644 index 000000000..93bf14c40 --- /dev/null +++ b/lib/crates/fabro-auth/src/context.rs @@ -0,0 +1,20 @@ +use fabro_model::Provider; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AuthContextRequest { + ApiKey { + provider: Provider, + env_var_names: Vec, + }, + DeviceCode { + user_code: String, + verification_uri: String, + expires_in: u64, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AuthContextResponse { + ApiKey { key: String }, + DeviceCodeConfirmed, +} diff --git a/lib/crates/fabro-auth/src/credential.rs b/lib/crates/fabro-auth/src/credential.rs new file mode 100644 index 000000000..67d95d297 --- /dev/null +++ b/lib/crates/fabro-auth/src/credential.rs @@ -0,0 +1,158 @@ +use chrono::{DateTime, Duration, Utc}; +use fabro_model::Provider; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct AuthCredential { + pub provider: Provider, + #[serde(flatten)] + pub details: AuthDetails, +} + +impl AuthCredential { + #[must_use] + pub fn needs_refresh(&self) -> bool { + match &self.details { + AuthDetails::ApiKey { .. } => false, + AuthDetails::CodexOAuth { tokens, .. } => { + tokens.expires_at <= Utc::now() + Duration::minutes(5) + } + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum AuthDetails { + ApiKey { + key: String, + }, + CodexOAuth { + tokens: OAuthTokens, + config: OAuthConfig, + #[serde(default, skip_serializing_if = "Option::is_none")] + account_id: Option, + }, +} + +#[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, PartialEq, Eq)] +pub enum ApiKeyHeader { + Bearer(String), + Custom { name: String, value: String }, +} + +pub fn credential_id_for(credential: &AuthCredential) -> Result { + match (&credential.provider, &credential.details) { + (Provider::OpenAi, AuthDetails::ApiKey { .. }) => Ok("openai".to_string()), + (Provider::OpenAi, AuthDetails::CodexOAuth { .. }) => Ok("openai_codex".to_string()), + (_, AuthDetails::CodexOAuth { .. }) => Err(format!( + "codex_oauth credentials are only valid for OpenAI, got {}", + credential.provider + )), + (provider, AuthDetails::ApiKey { .. }) => Ok(provider.as_str().to_string()), + } +} + +pub fn parse_credential_secret(name: &str, value: &str) -> Result { + let credential: AuthCredential = + serde_json::from_str(value).map_err(|err| format!("invalid credential JSON: {err}"))?; + let expected_name = credential_id_for(&credential)?; + if name != expected_name { + return Err(format!( + "credential ID must be '{expected_name}' for this credential, got '{name}'" + )); + } + Ok(credential) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn oauth_credential(expires_at: DateTime) -> AuthCredential { + AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: "access".to_string(), + refresh_token: Some("refresh".to_string()), + expires_at, + }, + config: OAuthConfig { + auth_url: "https://auth.openai.com".to_string(), + token_url: "https://auth.openai.com/oauth/token".to_string(), + client_id: "client".to_string(), + scopes: vec!["openid".to_string()], + redirect_uri: Some("https://auth.openai.com/deviceauth/callback".to_string()), + use_pkce: true, + }, + account_id: Some("acct_123".to_string()), + }, + } + } + + #[test] + fn auth_credential_round_trips_through_json() { + let credential = oauth_credential(Utc::now() + Duration::hours(1)); + let json = serde_json::to_string(&credential).unwrap(); + let parsed: AuthCredential = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed, credential); + } + + #[test] + fn needs_refresh_uses_five_minute_buffer() { + assert!(oauth_credential(Utc::now() + Duration::minutes(4)).needs_refresh()); + assert!(!oauth_credential(Utc::now() + Duration::minutes(6)).needs_refresh()); + } + + #[test] + fn credential_id_for_openai_codex_oauth() { + let credential = oauth_credential(Utc::now() + Duration::hours(1)); + assert_eq!(credential_id_for(&credential).unwrap(), "openai_codex"); + } + + #[test] + fn credential_id_for_openai_api_key() { + let credential = AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::ApiKey { + key: "sk-test".to_string(), + }, + }; + assert_eq!(credential_id_for(&credential).unwrap(), "openai"); + } + + #[test] + fn credential_id_for_non_openai_codex_oauth_errors() { + let mut credential = oauth_credential(Utc::now() + Duration::hours(1)); + credential.provider = Provider::Anthropic; + assert!(credential_id_for(&credential).is_err()); + } + + #[test] + fn parse_credential_secret_validates_name_and_json() { + let credential = oauth_credential(Utc::now() + Duration::hours(1)); + let json = serde_json::to_string(&credential).unwrap(); + + assert!(parse_credential_secret("openai_codex", &json).is_ok()); + assert!(parse_credential_secret("openai", &json).is_err()); + assert!(parse_credential_secret("openai_codex", "{").is_err()); + } +} diff --git a/lib/crates/fabro-auth/src/lib.rs b/lib/crates/fabro-auth/src/lib.rs new file mode 100644 index 000000000..1f99057ce --- /dev/null +++ b/lib/crates/fabro-auth/src/lib.rs @@ -0,0 +1,24 @@ +mod context; +mod credential; +mod refresh; +mod resolve; +mod strategy; +mod vault_ext; + +pub mod strategies; + +pub use context::{AuthContextRequest, AuthContextResponse}; +pub use credential::{ + ApiKeyHeader, AuthCredential, AuthDetails, OAuthConfig, OAuthTokens, credential_id_for, + parse_credential_secret, +}; +pub use refresh::refresh_oauth_credential; +pub use resolve::{ + ApiCredential, CliAgentKind, CliCredential, CredentialResolver, CredentialUsage, EnvLookup, + ResolveError, ResolvedCredential, +}; +pub use strategy::{ + AuthMethod, AuthStrategy, CODEX_AUTH_URL, CODEX_CLIENT_ID, CODEX_TOKEN_URL, codex_oauth_config, + strategy_for, +}; +pub use vault_ext::{vault_credentials_for_provider, vault_get_credential, vault_set_credential}; diff --git a/lib/crates/fabro-auth/src/refresh.rs b/lib/crates/fabro-auth/src/refresh.rs new file mode 100644 index 000000000..a1d240d55 --- /dev/null +++ b/lib/crates/fabro-auth/src/refresh.rs @@ -0,0 +1,49 @@ +use chrono::{Duration, Utc}; + +use crate::credential::{AuthCredential, AuthDetails, OAuthTokens}; + +fn expires_at_from_now(expires_in: Option) -> chrono::DateTime { + let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); + Utc::now() + Duration::seconds(seconds) +} + +pub async fn refresh_oauth_credential( + credential: &AuthCredential, +) -> anyhow::Result { + match &credential.details { + AuthDetails::ApiKey { .. } => Ok(credential.clone()), + AuthDetails::CodexOAuth { + tokens, + config, + account_id, + } => { + let refresh_token = tokens + .refresh_token + .as_deref() + .ok_or_else(|| anyhow::anyhow!("refresh token missing"))?; + let response = fabro_oauth::refresh_token( + fabro_oauth::OAuthEndpoint { + token_url: &config.token_url, + client_id: &config.client_id, + }, + refresh_token, + ) + .await + .map_err(anyhow::Error::msg)?; + Ok(AuthCredential { + provider: credential.provider, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: response.access_token, + refresh_token: response + .refresh_token + .or_else(|| tokens.refresh_token.clone()), + expires_at: expires_at_from_now(response.expires_in), + }, + config: config.clone(), + account_id: account_id.clone(), + }, + }) + } + } +} diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs new file mode 100644 index 000000000..a89178788 --- /dev/null +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -0,0 +1,608 @@ +use std::collections::HashMap; +use std::sync::Arc; + +use fabro_model::Provider; +use fabro_vault::Vault; +use shlex::try_quote; +use tokio::sync::RwLock as AsyncRwLock; + +use crate::credential::{ApiKeyHeader, AuthCredential, AuthDetails, credential_id_for}; +use crate::refresh::refresh_oauth_credential; +use crate::vault_ext::{vault_get_credential, vault_set_credential}; + +pub type EnvLookup = Arc Option + Send + Sync>; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CliAgentKind { + Claude, + Codex, + Gemini, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CredentialUsage { + ApiRequest, + CliAgent(CliAgentKind), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ApiCredential { + pub provider: Provider, + pub auth_header: ApiKeyHeader, + pub extra_headers: HashMap, + pub base_url: Option, + pub codex_mode: bool, + pub org_id: Option, + pub project_id: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CliCredential { + pub env_vars: HashMap, + pub login_command: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ResolvedCredential { + Api(ApiCredential), + Cli(CliCredential), +} + +#[derive(Debug, thiserror::Error)] +pub enum ResolveError { + #[error("{0} is not configured")] + NotConfigured(Provider), + #[error("{provider} requires re-authentication: {source}")] + RefreshFailed { + provider: Provider, + #[source] + source: anyhow::Error, + }, + #[error("{0} requires re-authentication: missing refresh token")] + RefreshTokenMissing(Provider), +} + +#[derive(Clone)] +pub struct CredentialResolver { + vault: Arc>, + env_lookup: EnvLookup, +} + +impl CredentialResolver { + #[must_use] + pub fn new(vault: Arc>) -> Self { + Self::with_env_lookup(vault, Arc::new(|name| std::env::var(name).ok())) + } + + #[must_use] + pub fn with_env_lookup(vault: Arc>, env_lookup: EnvLookup) -> Self { + Self { vault, env_lookup } + } + + pub async fn resolve( + &self, + provider: Provider, + usage: CredentialUsage, + ) -> Result { + let initial_credential = { + let vault = self.vault.read().await; + self.find_credential(&vault, provider, usage)? + }; + + let credential = if initial_credential.needs_refresh() { + let AuthDetails::CodexOAuth { tokens, .. } = &initial_credential.details else { + unreachable!("only OAuth credentials can need refresh"); + }; + if tokens.refresh_token.is_none() { + return Err(ResolveError::RefreshTokenMissing(provider)); + } + + let refreshed = refresh_oauth_credential(&initial_credential) + .await + .map_err(|source| ResolveError::RefreshFailed { provider, source })?; + let credential_id = + credential_id_for(&refreshed).map_err(|message| ResolveError::RefreshFailed { + provider, + source: anyhow::anyhow!(message), + })?; + let refreshed_for_store = refreshed.clone(); + let vault = Arc::clone(&self.vault); + tokio::task::spawn_blocking(move || { + let mut vault = vault.blocking_write(); + vault_set_credential(&mut vault, &credential_id, &refreshed_for_store) + .map(|_| ()) + .map_err(anyhow::Error::from) + }) + .await + .map_err(|join_err| ResolveError::RefreshFailed { + provider, + source: anyhow::Error::from(join_err), + })? + .map_err(|source| ResolveError::RefreshFailed { provider, source })?; + refreshed + } else { + initial_credential + }; + + let vault = self.vault.read().await; + match usage { + CredentialUsage::ApiRequest => Ok(ResolvedCredential::Api( + self.to_api_credential(&vault, &credential), + )), + CredentialUsage::CliAgent(kind) => Ok(ResolvedCredential::Cli( + self.to_cli_credential(&credential, kind), + )), + } + } + + fn find_credential( + &self, + vault: &Vault, + provider: Provider, + usage: CredentialUsage, + ) -> Result { + for credential_id in credential_ids_for(provider, usage) { + if let Some(credential) = vault_get_credential(vault, credential_id) { + return Ok(credential); + } + } + + for env_var in env_vars_for(provider) { + if let Some(value) = self.lookup_env_or_vault(vault, env_var) { + return Ok(AuthCredential { + provider, + details: AuthDetails::ApiKey { key: value }, + }); + } + } + + Err(ResolveError::NotConfigured(provider)) + } + + fn lookup_env_or_vault(&self, vault: &Vault, name: &str) -> Option { + (self.env_lookup)(name).or_else(|| vault.get(name).map(str::to_string)) + } + + fn to_api_credential(&self, vault: &Vault, credential: &AuthCredential) -> ApiCredential { + let mut extra_headers = HashMap::new(); + let base_url = match credential.provider { + Provider::Anthropic => self.lookup_env_or_vault(vault, "ANTHROPIC_BASE_URL"), + Provider::OpenAi => self.lookup_env_or_vault(vault, "OPENAI_BASE_URL"), + Provider::Gemini => self.lookup_env_or_vault(vault, "GEMINI_BASE_URL"), + Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => None, + Provider::OpenAiCompatible => None, + }; + let mut api_credential = match &credential.details { + AuthDetails::ApiKey { key } => ApiCredential { + provider: credential.provider, + auth_header: match credential.provider { + Provider::Anthropic => ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: key.clone(), + }, + _ => ApiKeyHeader::Bearer(key.clone()), + }, + extra_headers, + base_url, + codex_mode: false, + org_id: if credential.provider == Provider::OpenAi { + self.lookup_env_or_vault(vault, "OPENAI_ORG_ID") + } else { + None + }, + project_id: if credential.provider == Provider::OpenAi { + self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID") + } else { + None + }, + }, + AuthDetails::CodexOAuth { + tokens, account_id, .. + } => { + if let Some(account_id) = account_id { + extra_headers.insert("ChatGPT-Account-Id".to_string(), account_id.clone()); + extra_headers.insert("originator".to_string(), "fabro".to_string()); + } + ApiCredential { + provider: credential.provider, + auth_header: ApiKeyHeader::Bearer(tokens.access_token.clone()), + extra_headers, + base_url: Some("https://chatgpt.com/backend-api/codex".to_string()), + codex_mode: true, + org_id: self.lookup_env_or_vault(vault, "OPENAI_ORG_ID"), + project_id: self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID"), + } + } + }; + if api_credential.provider == Provider::OpenAi && api_credential.codex_mode { + api_credential.base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); + } + api_credential + } + + fn to_cli_credential(&self, credential: &AuthCredential, kind: CliAgentKind) -> CliCredential { + let mut env_vars = HashMap::new(); + let login_command = match (&credential.provider, &credential.details, kind) { + (Provider::OpenAi, AuthDetails::ApiKey { key }, CliAgentKind::Codex) => { + env_vars.insert("OPENAI_API_KEY".to_string(), key.clone()); + Some(codex_login_command(key)) + } + ( + Provider::OpenAi, + AuthDetails::CodexOAuth { + tokens, account_id, .. + }, + CliAgentKind::Codex, + ) => { + env_vars.insert("OPENAI_API_KEY".to_string(), tokens.access_token.clone()); + if let Some(account_id) = account_id { + env_vars.insert("CHATGPT_ACCOUNT_ID".to_string(), account_id.clone()); + } + Some(codex_login_command(&tokens.access_token)) + } + (_, AuthDetails::ApiKey { key }, _) => { + if let Some(name) = env_vars_for(credential.provider).first() { + env_vars.insert((*name).to_string(), key.clone()); + } + None + } + (_, AuthDetails::CodexOAuth { tokens, .. }, _) => { + env_vars.insert("OPENAI_API_KEY".to_string(), tokens.access_token.clone()); + None + } + }; + + CliCredential { + env_vars, + login_command, + } + } +} + +fn codex_login_command(api_key: &str) -> String { + let quoted = try_quote(api_key) + .map(std::borrow::Cow::into_owned) + .unwrap_or_else(|_| api_key.to_string()); + format!("PATH=\"$HOME/.local/bin:$PATH\" echo {quoted} | codex login --with-api-key") +} + +fn env_vars_for(provider: Provider) -> &'static [&'static str] { + provider.api_key_env_vars() +} + +fn credential_ids_for(provider: Provider, usage: CredentialUsage) -> &'static [&'static str] { + match (provider, usage) { + (Provider::OpenAi, CredentialUsage::ApiRequest) => &["openai"], + (Provider::OpenAi, CredentialUsage::CliAgent(CliAgentKind::Codex)) => { + &["openai_codex", "openai"] + } + (Provider::Anthropic, _) => &["anthropic"], + (Provider::Gemini, _) => &["gemini"], + (Provider::Kimi, _) => &["kimi"], + (Provider::Zai, _) => &["zai"], + (Provider::Minimax, _) => &["minimax"], + (Provider::Inception, _) => &["inception"], + (Provider::OpenAiCompatible, _) => &[], + (Provider::OpenAi, _) => &["openai"], + } +} + +#[cfg(test)] +mod tests { + use chrono::{Duration, Utc}; + use httpmock::Method::POST; + use httpmock::MockServer; + + use super::*; + use crate::credential::{OAuthConfig, OAuthTokens}; + use crate::vault_ext::vault_get_credential; + + fn api_key_credential(provider: Provider, key: &str) -> AuthCredential { + AuthCredential { + provider, + details: AuthDetails::ApiKey { + key: key.to_string(), + }, + } + } + + fn oauth_credential(token_url: String, expires_at: chrono::DateTime) -> AuthCredential { + AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: "expired-access".to_string(), + refresh_token: Some("refresh-token".to_string()), + expires_at, + }, + config: OAuthConfig { + auth_url: "https://auth.openai.com".to_string(), + token_url, + client_id: "test-client".to_string(), + scopes: vec!["openid".to_string()], + redirect_uri: Some("https://auth.openai.com/deviceauth/callback".to_string()), + use_pkce: true, + }, + account_id: Some("acct_123".to_string()), + }, + } + } + + fn test_resolver(vault: Vault, env_lookup: EnvLookup) -> CredentialResolver { + CredentialResolver::with_env_lookup(Arc::new(AsyncRwLock::new(vault)), env_lookup) + } + + #[tokio::test] + async fn resolve_openai_api_request_prefers_typed_credential() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai", + &api_key_credential(Provider::OpenAi, "vault-key"), + ) + .unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| Some("env-key".to_string()))); + + let resolved = resolver + .resolve(Provider::OpenAi, CredentialUsage::ApiRequest) + .await + .unwrap(); + + let ResolvedCredential::Api(api) = resolved else { + panic!("expected api credential"); + }; + assert_eq!( + api.auth_header, + ApiKeyHeader::Bearer("vault-key".to_string()) + ); + } + + #[tokio::test] + async fn resolve_returns_not_configured_for_missing_provider() { + let dir = tempfile::tempdir().unwrap(); + let vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let err = resolver + .resolve(Provider::Anthropic, CredentialUsage::ApiRequest) + .await + .unwrap_err(); + + assert!(matches!( + err, + ResolveError::NotConfigured(Provider::Anthropic) + )); + } + + #[tokio::test] + async fn anthropic_api_credentials_use_x_api_key_header() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "anthropic", + &api_key_credential(Provider::Anthropic, "anthropic-key"), + ) + .unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let ResolvedCredential::Api(api) = resolver + .resolve(Provider::Anthropic, CredentialUsage::ApiRequest) + .await + .unwrap() + else { + panic!("expected api credential"); + }; + + assert_eq!(api.auth_header, ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: "anthropic-key".to_string(), + }); + } + + #[tokio::test] + async fn openai_codex_cli_credential_includes_login_command_and_account_id() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai_codex", + &oauth_credential( + "https://auth.openai.com/oauth/token".to_string(), + Utc::now() + Duration::hours(1), + ), + ) + .unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let ResolvedCredential::Cli(cli) = resolver + .resolve( + Provider::OpenAi, + CredentialUsage::CliAgent(CliAgentKind::Codex), + ) + .await + .unwrap() + else { + panic!("expected cli credential"); + }; + + assert_eq!( + cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), + Some("expired-access") + ); + assert_eq!( + cli.env_vars.get("CHATGPT_ACCOUNT_ID").map(String::as_str), + Some("acct_123") + ); + assert!( + cli.login_command + .as_deref() + .is_some_and(|command| command.contains("codex login --with-api-key")) + ); + } + + #[tokio::test] + async fn openai_api_key_cli_fallback_has_no_account_id() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai", + &api_key_credential(Provider::OpenAi, "openai-key"), + ) + .unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let ResolvedCredential::Cli(cli) = resolver + .resolve( + Provider::OpenAi, + CredentialUsage::CliAgent(CliAgentKind::Codex), + ) + .await + .unwrap() + else { + panic!("expected cli credential"); + }; + + assert_eq!( + cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), + Some("openai-key") + ); + assert!(!cli.env_vars.contains_key("CHATGPT_ACCOUNT_ID")); + assert!(cli.login_command.is_some()); + } + + #[tokio::test] + async fn with_env_lookup_overrides_vault_settings() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai", + &api_key_credential(Provider::OpenAi, "vault-key"), + ) + .unwrap(); + vault + .set( + "OPENAI_ORG_ID", + "vault-org", + fabro_vault::SecretType::Environment, + None, + ) + .unwrap(); + let resolver = test_resolver( + vault, + Arc::new(|name| (name == "OPENAI_ORG_ID").then(|| "env-org".to_string())), + ); + + let ResolvedCredential::Api(api) = resolver + .resolve(Provider::OpenAi, CredentialUsage::ApiRequest) + .await + .unwrap() + else { + panic!("expected api credential"); + }; + + assert_eq!(api.org_id.as_deref(), Some("env-org")); + } + + #[tokio::test] + async fn resolve_refreshes_expired_oauth_credentials_and_persists_them() { + let server = MockServer::start_async().await; + let refresh_mock = server + .mock_async(|when, then| { + when.method(POST) + .path("/oauth/token") + .header("content-type", "application/x-www-form-urlencoded") + .form_urlencoded_tuple("grant_type", "refresh_token") + .form_urlencoded_tuple("client_id", "test-client") + .form_urlencoded_tuple("refresh_token", "refresh-token"); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "access_token": "new-access", + "refresh_token": "new-refresh", + "expires_in": 3600 + }) + .to_string(), + ); + }) + .await; + + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential( + &mut vault, + "openai_codex", + &oauth_credential( + server.url("/oauth/token"), + Utc::now() - Duration::minutes(1), + ), + ) + .unwrap(); + let vault = Arc::new(AsyncRwLock::new(vault)); + let resolver = CredentialResolver::with_env_lookup(Arc::clone(&vault), Arc::new(|_| None)); + + let ResolvedCredential::Cli(cli) = resolver + .resolve( + Provider::OpenAi, + CredentialUsage::CliAgent(CliAgentKind::Codex), + ) + .await + .unwrap() + else { + panic!("expected cli credential"); + }; + + assert_eq!( + cli.env_vars.get("OPENAI_API_KEY").map(String::as_str), + Some("new-access") + ); + + let stored = { + let vault = vault.read().await; + vault_get_credential(&vault, "openai_codex").unwrap() + }; + let AuthDetails::CodexOAuth { + tokens, account_id, .. + } = stored.details + else { + panic!("expected codex oauth credential"); + }; + assert_eq!(tokens.access_token, "new-access"); + assert_eq!(tokens.refresh_token.as_deref(), Some("new-refresh")); + assert_eq!(account_id.as_deref(), Some("acct_123")); + refresh_mock.assert_async().await; + } + + #[tokio::test] + async fn resolve_returns_refresh_token_missing_when_expired_oauth_has_no_refresh_token() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + let mut credential = oauth_credential( + "https://auth.openai.com/oauth/token".to_string(), + Utc::now() - Duration::minutes(1), + ); + let AuthDetails::CodexOAuth { tokens, .. } = &mut credential.details else { + unreachable!(); + }; + tokens.refresh_token = None; + vault_set_credential(&mut vault, "openai_codex", &credential).unwrap(); + let resolver = test_resolver(vault, Arc::new(|_| None)); + + let err = resolver + .resolve( + Provider::OpenAi, + CredentialUsage::CliAgent(CliAgentKind::Codex), + ) + .await + .unwrap_err(); + + assert!(matches!( + err, + ResolveError::RefreshTokenMissing(Provider::OpenAi) + )); + } +} diff --git a/lib/crates/fabro-auth/src/strategies/api_key.rs b/lib/crates/fabro-auth/src/strategies/api_key.rs new file mode 100644 index 000000000..5de78ea6b --- /dev/null +++ b/lib/crates/fabro-auth/src/strategies/api_key.rs @@ -0,0 +1,44 @@ +use async_trait::async_trait; +use fabro_model::Provider; + +use crate::context::{AuthContextRequest, AuthContextResponse}; +use crate::credential::{AuthCredential, AuthDetails}; +use crate::strategy::AuthStrategy; + +pub struct ApiKeyStrategy { + provider: Provider, +} + +impl ApiKeyStrategy { + #[must_use] + pub fn new(provider: Provider) -> Self { + Self { provider } + } +} + +#[async_trait] +impl AuthStrategy for ApiKeyStrategy { + async fn init(&mut self) -> anyhow::Result { + Ok(AuthContextRequest::ApiKey { + provider: self.provider, + env_var_names: self + .provider + .api_key_env_vars() + .iter() + .map(|name| (*name).to_string()) + .collect(), + }) + } + + async fn complete(&mut self, response: AuthContextResponse) -> anyhow::Result { + match response { + AuthContextResponse::ApiKey { key } => Ok(AuthCredential { + provider: self.provider, + details: AuthDetails::ApiKey { key }, + }), + AuthContextResponse::DeviceCodeConfirmed => { + Err(anyhow::anyhow!("expected API key response")) + } + } + } +} diff --git a/lib/crates/fabro-auth/src/strategies/codex_device.rs b/lib/crates/fabro-auth/src/strategies/codex_device.rs new file mode 100644 index 000000000..60bc22493 --- /dev/null +++ b/lib/crates/fabro-auth/src/strategies/codex_device.rs @@ -0,0 +1,286 @@ +use std::time::{Duration, Instant}; + +use async_trait::async_trait; +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use fabro_http::HttpClient; +use serde::{Deserialize, Serialize}; +use serde_json::json; +use tokio::time::sleep; + +use crate::context::{AuthContextRequest, AuthContextResponse}; +use crate::credential::{AuthCredential, AuthDetails, OAuthConfig, OAuthTokens}; +use crate::strategy::AuthStrategy; + +const DEVICE_AUTH_TIMEOUT: Duration = Duration::from_secs(15 * 60); +const DEVICE_AUTH_POLL_INTERVAL: Duration = Duration::from_secs(2); + +fn http_client() -> anyhow::Result { + #[cfg(test)] + { + fabro_http::test_http_client().map_err(anyhow::Error::from) + } + #[cfg(not(test))] + { + fabro_http::http_client().map_err(anyhow::Error::from) + } +} + +fn join_url(base: &str, path: &str) -> String { + format!("{}{}", base.trim_end_matches('/'), path) +} + +fn expires_at_from_now(expires_in: Option) -> chrono::DateTime { + let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); + chrono::Utc::now() + chrono::Duration::seconds(seconds) +} + +#[derive(Debug, Deserialize)] +struct JwtPayload { + #[serde(default)] + chatgpt_account_id: Option, + #[serde(default, rename = "https://api.openai.com/auth")] + auth_claim: Option, + #[serde(default)] + organizations: Option>, +} + +#[derive(Debug, Deserialize)] +struct AuthClaim { + #[serde(default)] + chatgpt_account_id: Option, +} + +#[derive(Debug, Deserialize)] +struct Organization { + #[serde(default)] + id: Option, +} + +fn parse_jwt_payload(token: &str) -> Option { + let parts: Vec<&str> = token.split('.').collect(); + if parts.len() != 3 { + return None; + } + let payload_bytes = URL_SAFE_NO_PAD.decode(parts[1]).ok()?; + serde_json::from_slice(&payload_bytes).ok() +} + +pub fn extract_chatgpt_account_id(id_token: &str) -> Option { + let payload = parse_jwt_payload(id_token)?; + payload + .chatgpt_account_id + .or_else(|| { + payload + .auth_claim + .and_then(|claim| claim.chatgpt_account_id) + }) + .or_else(|| { + payload + .organizations + .and_then(|orgs| orgs.into_iter().next()) + .and_then(|org| org.id) + }) +} + +#[derive(Debug, Deserialize)] +struct DeviceCodeInitResponse { + device_auth_id: String, + user_code: String, + #[serde(alias = "verificationUrl", alias = "verification_uri")] + verification_uri: String, + #[serde(default)] + expires_in: Option, +} + +#[derive(Debug, Deserialize)] +struct DeviceCodePollResponse { + #[serde(default)] + status: Option, + #[serde(default)] + authorization_code: Option, +} + +#[derive(Debug, Serialize)] +struct DeviceCodeInitRequest<'a> { + client_id: &'a str, + #[serde(skip_serializing_if = "Option::is_none")] + code_challenge: Option<&'a str>, + #[serde(skip_serializing_if = "Option::is_none")] + scope: Option, +} + +pub struct CodexDeviceStrategy { + config: OAuthConfig, + device_auth_id: Option, + code_verifier: Option, +} + +impl CodexDeviceStrategy { + #[must_use] + pub fn new(config: OAuthConfig) -> Self { + Self { + config, + device_auth_id: None, + code_verifier: None, + } + } + + async fn poll_codex_device(&self, device_auth_id: &str) -> anyhow::Result { + let client = http_client()?; + let deadline = Instant::now() + DEVICE_AUTH_TIMEOUT; + let url = join_url(&self.config.auth_url, "/api/accounts/deviceauth/token"); + + loop { + if Instant::now() >= deadline { + return Err(anyhow::anyhow!("device auth timed out after 15 minutes")); + } + + let response = client + .post(&url) + .json(&json!({ "device_auth_id": device_auth_id })) + .send() + .await?; + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + return Err(anyhow::anyhow!( + "device auth failed with status {status}: {body}" + )); + } + + let payload: DeviceCodePollResponse = response.json().await?; + if let Some(code) = payload.authorization_code { + return Ok(code); + } + + match payload.status.as_deref() { + Some("pending") | Some("running") | None => { + sleep(DEVICE_AUTH_POLL_INTERVAL).await; + } + Some(other) => { + return Err(anyhow::anyhow!("device code exchange failed: {other}")); + } + } + } + } +} + +#[async_trait] +impl AuthStrategy for CodexDeviceStrategy { + async fn init(&mut self) -> anyhow::Result { + let pkce = self.config.use_pkce.then(fabro_oauth::generate_pkce); + self.code_verifier = pkce.as_ref().map(|codes| codes.verifier.clone()); + + let client = http_client()?; + let url = join_url(&self.config.auth_url, "/api/accounts/deviceauth/usercode"); + let response = client + .post(&url) + .json(&DeviceCodeInitRequest { + client_id: &self.config.client_id, + code_challenge: pkce.as_ref().map(|codes| codes.challenge.as_str()), + scope: (!self.config.scopes.is_empty()) + .then(|| self.config.scopes.join(" ")), + }) + .send() + .await?; + let status = response.status(); + if !status.is_success() { + let body = response.text().await.unwrap_or_default(); + return Err(anyhow::anyhow!( + "device code request failed with status {status}: {body}" + )); + } + + let payload: DeviceCodeInitResponse = response.json().await?; + self.device_auth_id = Some(payload.device_auth_id); + + Ok(AuthContextRequest::DeviceCode { + user_code: payload.user_code, + verification_uri: payload.verification_uri, + expires_in: payload.expires_in.unwrap_or(900), + }) + } + + async fn complete(&mut self, response: AuthContextResponse) -> anyhow::Result { + match response { + AuthContextResponse::ApiKey { .. } => Err(anyhow::anyhow!( + "expected device code confirmation response" + )), + AuthContextResponse::DeviceCodeConfirmed => { + let device_auth_id = self + .device_auth_id + .take() + .ok_or_else(|| anyhow::anyhow!("device auth flow was not initialized"))?; + let authorization_code = self.poll_codex_device(&device_auth_id).await?; + let token_response = fabro_oauth::exchange_code( + fabro_oauth::OAuthEndpoint { + token_url: &self.config.token_url, + client_id: &self.config.client_id, + }, + &authorization_code, + self.config.redirect_uri.as_deref(), + self.code_verifier.as_deref(), + ) + .await + .map_err(anyhow::Error::msg)?; + + Ok(AuthCredential { + provider: fabro_model::Provider::OpenAi, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: token_response.access_token, + refresh_token: token_response.refresh_token, + expires_at: expires_at_from_now(token_response.expires_in), + }, + config: self.config.clone(), + account_id: token_response + .id_token + .as_deref() + .and_then(extract_chatgpt_account_id), + }, + }) + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_test_jwt(claims: &serde_json::Value) -> String { + let header = URL_SAFE_NO_PAD.encode(r#"{"alg":"RS256"}"#); + let payload = URL_SAFE_NO_PAD.encode(serde_json::to_string(claims).unwrap()); + format!("{header}.{payload}.signature") + } + + #[test] + fn extract_chatgpt_account_id_prefers_top_level_claim() { + let jwt = make_test_jwt(&json!({ + "chatgpt_account_id": "top_level", + "https://api.openai.com/auth": { "chatgpt_account_id": "nested" }, + "organizations": [{ "id": "org_123" }] + })); + assert_eq!( + extract_chatgpt_account_id(&jwt).as_deref(), + Some("top_level") + ); + } + + #[test] + fn extract_chatgpt_account_id_falls_back_to_nested_claim() { + let jwt = make_test_jwt(&json!({ + "https://api.openai.com/auth": { "chatgpt_account_id": "nested" } + })); + assert_eq!(extract_chatgpt_account_id(&jwt).as_deref(), Some("nested")); + } + + #[test] + fn extract_chatgpt_account_id_falls_back_to_organization() { + let jwt = make_test_jwt(&json!({ + "organizations": [{ "id": "org_123" }] + })); + assert_eq!(extract_chatgpt_account_id(&jwt).as_deref(), Some("org_123")); + } +} diff --git a/lib/crates/fabro-auth/src/strategies/mod.rs b/lib/crates/fabro-auth/src/strategies/mod.rs new file mode 100644 index 000000000..27f9303fe --- /dev/null +++ b/lib/crates/fabro-auth/src/strategies/mod.rs @@ -0,0 +1,2 @@ +pub mod api_key; +pub mod codex_device; diff --git a/lib/crates/fabro-auth/src/strategy.rs b/lib/crates/fabro-auth/src/strategy.rs new file mode 100644 index 000000000..2ade66ad1 --- /dev/null +++ b/lib/crates/fabro-auth/src/strategy.rs @@ -0,0 +1,81 @@ +use async_trait::async_trait; +use fabro_model::Provider; + +use crate::context::{AuthContextRequest, AuthContextResponse}; +use crate::credential::{AuthCredential, OAuthConfig}; +use crate::strategies::api_key::ApiKeyStrategy; +use crate::strategies::codex_device::CodexDeviceStrategy; + +pub const CODEX_CLIENT_ID: &str = "app_EMoamEEZ73f0CkXaXp7hrann"; +pub const CODEX_AUTH_URL: &str = "https://auth.openai.com"; +pub const CODEX_TOKEN_URL: &str = "https://auth.openai.com/oauth/token"; + +#[async_trait] +pub trait AuthStrategy: Send { + async fn init(&mut self) -> anyhow::Result; + async fn complete(&mut self, response: AuthContextResponse) -> anyhow::Result; +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AuthMethod { + ApiKey, + CodexDevice(OAuthConfig), +} + +#[must_use] +pub fn codex_oauth_config() -> OAuthConfig { + OAuthConfig { + auth_url: CODEX_AUTH_URL.to_string(), + token_url: CODEX_TOKEN_URL.to_string(), + client_id: CODEX_CLIENT_ID.to_string(), + scopes: vec![ + "openid".to_string(), + "profile".to_string(), + "email".to_string(), + "offline_access".to_string(), + ], + redirect_uri: Some(format!("{CODEX_AUTH_URL}/deviceauth/callback")), + use_pkce: true, + } +} + +#[must_use] +pub fn strategy_for(provider: Provider, method: AuthMethod) -> Box { + match method { + AuthMethod::ApiKey => Box::new(ApiKeyStrategy::new(provider)), + AuthMethod::CodexDevice(config) => { + assert_eq!( + provider, + Provider::OpenAi, + "Codex device auth is only supported for OpenAI" + ); + Box::new(CodexDeviceStrategy::new(config)) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::context::AuthContextRequest; + + #[test] + fn codex_oauth_config_has_expected_defaults() { + let config = codex_oauth_config(); + assert_eq!(config.auth_url, CODEX_AUTH_URL); + assert_eq!(config.token_url, CODEX_TOKEN_URL); + assert_eq!(config.client_id, CODEX_CLIENT_ID); + assert!(config.use_pkce); + assert!(config.scopes.contains(&"offline_access".to_string())); + } + + #[tokio::test] + async fn api_key_strategy_uses_provider_env_names() { + let mut strategy = ApiKeyStrategy::new(Provider::Anthropic); + let request = strategy.init().await.unwrap(); + assert_eq!(request, AuthContextRequest::ApiKey { + provider: Provider::Anthropic, + env_var_names: vec!["ANTHROPIC_API_KEY".to_string()], + }); + } +} diff --git a/lib/crates/fabro-auth/src/vault_ext.rs b/lib/crates/fabro-auth/src/vault_ext.rs new file mode 100644 index 000000000..99d1c5306 --- /dev/null +++ b/lib/crates/fabro-auth/src/vault_ext.rs @@ -0,0 +1,102 @@ +use fabro_model::Provider; +use fabro_vault::{SecretMetadata, SecretType, Vault}; + +use crate::credential::AuthCredential; + +pub fn vault_set_credential( + vault: &mut Vault, + id: &str, + credential: &AuthCredential, +) -> Result { + let json = serde_json::to_string(credential)?; + vault.set(id, &json, SecretType::Credential, None) +} + +#[must_use] +pub fn vault_get_credential(vault: &Vault, id: &str) -> Option { + let entry = vault.get_entry(id)?; + if entry.secret_type != SecretType::Credential { + return None; + } + serde_json::from_str(&entry.value).ok() +} + +#[must_use] +pub fn vault_credentials_for_provider( + vault: &Vault, + provider: Provider, +) -> Vec<(String, AuthCredential)> { + vault + .credential_entries() + .into_iter() + .filter_map(|(name, entry)| { + serde_json::from_str::(&entry.value) + .ok() + .filter(|credential| credential.provider == provider) + .map(|credential| (name.to_string(), credential)) + }) + .collect() +} + +#[cfg(test)] +mod tests { + use chrono::{Duration, Utc}; + + use super::*; + use crate::credential::{AuthDetails, OAuthConfig, OAuthTokens}; + + fn oauth_credential() -> AuthCredential { + AuthCredential { + provider: Provider::OpenAi, + details: AuthDetails::CodexOAuth { + tokens: OAuthTokens { + access_token: "access".to_string(), + refresh_token: Some("refresh".to_string()), + expires_at: Utc::now() + Duration::hours(1), + }, + config: OAuthConfig { + auth_url: "https://auth.openai.com".to_string(), + token_url: "https://auth.openai.com/oauth/token".to_string(), + client_id: "client".to_string(), + scopes: vec!["openid".to_string()], + redirect_uri: Some("https://auth.openai.com/deviceauth/callback".to_string()), + use_pkce: true, + }, + account_id: Some("acct_123".to_string()), + }, + } + } + + #[test] + fn vault_credential_round_trip() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + let credential = oauth_credential(); + + vault_set_credential(&mut vault, "openai_codex", &credential).unwrap(); + + assert_eq!( + vault_get_credential(&vault, "openai_codex").unwrap(), + credential + ); + } + + #[test] + fn vault_credentials_for_provider_filters_by_provider() { + let dir = tempfile::tempdir().unwrap(); + let mut vault = Vault::load(dir.path().join("secrets.json")).unwrap(); + vault_set_credential(&mut vault, "openai_codex", &oauth_credential()).unwrap(); + vault_set_credential(&mut vault, "anthropic", &AuthCredential { + provider: Provider::Anthropic, + details: AuthDetails::ApiKey { + key: "anthropic-key".to_string(), + }, + }) + .unwrap(); + + let credentials = vault_credentials_for_provider(&vault, Provider::OpenAi); + + assert_eq!(credentials.len(), 1); + assert_eq!(credentials[0].0, "openai_codex"); + } +} diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index d476a3500..6f6de98b0 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -18,6 +18,7 @@ sleep_inhibitor = ["dep:core-foundation"] workspace = true [dependencies] +fabro-auth = { path = "../fabro-auth" } fabro-config = { path = "../fabro-config" } fabro-llm = { path = "../fabro-llm" } fabro-model = { path = "../fabro-model" } diff --git a/lib/crates/fabro-cli/src/commands/doctor.rs b/lib/crates/fabro-cli/src/commands/doctor.rs index 1c0870dcf..f20cc1f0c 100644 --- a/lib/crates/fabro-cli/src/commands/doctor.rs +++ b/lib/crates/fabro-cli/src/commands/doctor.rs @@ -217,10 +217,7 @@ fn check_legacy_env(path: Option) -> CheckResult { "{} is no longer read by fabro", path.display() ))], - remediation: Some( - "Re-enter credentials with `fabro provider login` or `fabro secret set`." - .to_string(), - ), + remediation: Some("Re-enter credentials with `fabro provider login`.".to_string()), }, None => CheckResult { name: "Legacy .env".to_string(), diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 3b152dabc..58d965e24 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -12,6 +12,9 @@ use dialoguer::console::Term; use dialoguer::theme::ColorfulTheme; use dialoguer::{MultiSelect, Select}; use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; +use fabro_auth::{ + AuthCredential, AuthMethod, codex_oauth_config, credential_id_for, parse_credential_secret, +}; use fabro_config::user::SETTINGS_CONFIG_FILENAME; use fabro_config::{Storage, envfile, legacy_env}; use fabro_model::Provider; @@ -31,7 +34,7 @@ use crate::args::{DoctorArgs, GlobalArgs, InstallArgs, ServerTargetArgs}; use crate::commands::server::record; use crate::gh::GhCli; use crate::shared::provider_auth::{ - prompt_and_validate_key, prompt_confirm, provider_display_name, run_openai_oauth_or_api_key, + authenticate_provider, authenticate_provider_with_method, prompt_confirm, provider_display_name, }; use crate::{server_client, user_config}; @@ -693,7 +696,7 @@ async fn setup_github_app( async fn persist_vault_secrets( storage_dir: &Path, - secrets: &[(String, String)], + secrets: &[CreateSecretRequest], server_was_running: bool, ) -> Result<()> { if secrets.is_empty() { @@ -702,14 +705,14 @@ async fn persist_vault_secrets( if server_was_running { let client = server_client::connect_api_client(storage_dir).await?; - for (name, value) in secrets { + for secret in secrets { client .create_secret() .body(CreateSecretRequest { - name: name.clone(), - value: value.clone(), - type_: ApiSecretType::Environment, - description: None, + name: secret.name.clone(), + value: secret.value.clone(), + type_: secret.type_.clone(), + description: secret.description.clone(), }) .send() .await?; @@ -718,12 +721,45 @@ async fn persist_vault_secrets( } let mut store = Vault::load(Storage::new(storage_dir).secrets_path())?; - for (name, value) in secrets { - store.set(name, value, SecretType::Environment, None)?; + for secret in secrets { + validate_vault_secret(secret)?; + store.set( + &secret.name, + &secret.value, + local_secret_type(&secret.type_), + secret.description.as_deref(), + )?; } Ok(()) } +fn local_secret_type(secret_type: &ApiSecretType) -> SecretType { + match secret_type { + ApiSecretType::Environment => SecretType::Environment, + ApiSecretType::File => SecretType::File, + ApiSecretType::Credential => SecretType::Credential, + } +} + +fn validate_vault_secret(secret: &CreateSecretRequest) -> Result<()> { + if secret.type_ != ApiSecretType::Credential { + return Ok(()); + } + + parse_credential_secret(&secret.name, &secret.value) + .map(|_| ()) + .map_err(anyhow::Error::msg) +} + +fn credential_secret_request(credential: &AuthCredential) -> Result { + Ok(CreateSecretRequest { + name: credential_id_for(credential).map_err(anyhow::Error::msg)?, + value: serde_json::to_string(credential)?, + type_: ApiSecretType::Credential, + description: None, + }) +} + fn persist_server_env_secrets(storage_dir: &Path, secrets: &[(String, String)]) -> Result<()> { if secrets.is_empty() { return Ok(()); @@ -739,7 +775,7 @@ fn persist_server_env_secrets(storage_dir: &Path, secrets: &[(String, String)]) async fn persist_install_outputs( storage_dir: &Path, server_env_secrets: &[(String, String)], - vault_secrets: &[(String, String)], + vault_secrets: &[CreateSecretRequest], server_was_running: bool, ) -> Result<()> { persist_server_env_secrets(storage_dir, server_env_secrets)?; @@ -841,32 +877,38 @@ pub(crate) async fn run_install( fabro_util::printerr!(printer, " {}", s.dim.apply_to("──────────────────────")); fabro_util::printerr!(printer, ""); - let mut vault_pairs: Vec<(String, String)> = Vec::new(); + let mut vault_secrets: Vec = Vec::new(); let mut server_env_pairs: Vec<(String, String)> = Vec::new(); let mut configured_providers: Vec = Vec::new(); let codex_detected = detect_binary_on_path("codex").await; - let mut openai_via_oauth = false; + let mut openai_configured = false; if codex_detected { tracing::debug!("Codex binary detected on PATH"); - let use_oauth = spawn_blocking(|| { + let use_device_auth = spawn_blocking(|| { prompt_confirm( - "OpenAI (Codex) detected. Set up OpenAI via browser login?", + "OpenAI (Codex) detected. Set up OpenAI with device code login?", true, ) }) .await??; - if use_oauth { - let pairs = run_openai_oauth_or_api_key(&s, printer).await?; - vault_pairs.extend(pairs); + if use_device_auth { + let credential = authenticate_provider_with_method( + Provider::OpenAi, + AuthMethod::CodexDevice(codex_oauth_config()), + &s, + printer, + ) + .await?; + vault_secrets.push(credential_secret_request(&credential)?); configured_providers.push(Provider::OpenAi); - openai_via_oauth = true; + openai_configured = true; } } - if !openai_via_oauth { + if !openai_configured { // First provider — single choice from the top 3 let primary_providers = [Provider::Anthropic, Provider::OpenAi, Provider::Gemini]; let primary_labels: Vec = primary_providers @@ -882,8 +924,8 @@ pub(crate) async fn run_install( let first_provider = primary_providers[primary_idx]; { - let (env_var, key) = prompt_and_validate_key(first_provider, &s, printer).await?; - vault_pairs.push((env_var, key)); + let credential = authenticate_provider(first_provider, &s, printer).await?; + vault_secrets.push(credential_secret_request(&credential)?); configured_providers.push(first_provider); } } @@ -916,8 +958,8 @@ pub(crate) async fn run_install( for idx in selected_indices { let provider = remaining_providers[idx]; - let (env_var, key) = prompt_and_validate_key(provider, &s, printer).await?; - vault_pairs.push((env_var, key)); + let credential = authenticate_provider(provider, &s, printer).await?; + vault_secrets.push(credential_secret_request(&credential)?); } } fabro_util::printerr!(printer, ""); @@ -953,7 +995,12 @@ pub(crate) async fn run_install( write_github_cli_settings(&mut doc)?; std::fs::write(&user_toml_path, toml::to_string_pretty(&doc)?)?; fabro_util::printerr!(printer, " {} GitHub CLI configured", s.green.apply_to("✔")); - vault_pairs.push(("GITHUB_CLI_TOKEN".to_string(), token)); + vault_secrets.push(CreateSecretRequest { + name: "GITHUB_CLI_TOKEN".to_string(), + value: token, + type_: ApiSecretType::Environment, + description: None, + }); } 1 => { let (owner, username) = prompt_github_app_owner(&s).await?; @@ -1085,7 +1132,7 @@ pub(crate) async fn run_install( persist_install_outputs( &storage_dir, &server_env_pairs, - &vault_pairs, + &vault_secrets, server_was_running, ) .await?; @@ -1103,7 +1150,7 @@ pub(crate) async fn run_install( printer, " {} Saved {} workflow-visible secrets to {}", s.green.apply_to("✔"), - vault_pairs.len(), + vault_secrets.len(), Storage::new(&storage_dir).secrets_path().display() ); if server_was_running { @@ -1530,9 +1577,23 @@ client_id = "client-id" ("SESSION_SECRET".to_string(), "session".to_string()), ("FABRO_JWT_PUBLIC_KEY".to_string(), "public-key".to_string()), ]; - let vault_pairs = vec![("OPENAI_API_KEY".to_string(), "openai-key".to_string())]; + let vault_secrets = vec![ + CreateSecretRequest { + name: "GITHUB_CLI_TOKEN".to_string(), + value: "gh-token".to_string(), + type_: ApiSecretType::Environment, + description: None, + }, + credential_secret_request(&AuthCredential { + provider: Provider::Anthropic, + details: fabro_auth::AuthDetails::ApiKey { + key: "anthropic-key".to_string(), + }, + }) + .unwrap(), + ]; - persist_install_outputs(dir.path(), &server_env_pairs, &vault_pairs, false) + persist_install_outputs(dir.path(), &server_env_pairs, &vault_secrets, false) .await .unwrap(); @@ -1542,7 +1603,9 @@ client_id = "client-id" assert!(server_env.contains("FABRO_JWT_PUBLIC_KEY=public-key")); let vault = Vault::load(Storage::new(dir.path()).secrets_path()).unwrap(); - assert_eq!(vault.get("OPENAI_API_KEY"), Some("openai-key")); + assert_eq!(vault.get("GITHUB_CLI_TOKEN"), Some("gh-token")); + let anthropic = vault.get_entry("anthropic").unwrap(); + assert_eq!(anthropic.secret_type, SecretType::Credential); assert_eq!(vault.get("SESSION_SECRET"), None); } } diff --git a/lib/crates/fabro-cli/src/commands/provider/login.rs b/lib/crates/fabro-cli/src/commands/provider/login.rs index 49189bd94..8af8d4cf6 100644 --- a/lib/crates/fabro-cli/src/commands/provider/login.rs +++ b/lib/crates/fabro-cli/src/commands/provider/login.rs @@ -1,10 +1,9 @@ use anyhow::Result; use fabro_api::types; +use fabro_auth::credential_id_for; use fabro_config::legacy_env; -use fabro_model::Provider; use fabro_util::printer::Printer; use fabro_util::terminal::Styles; -use tokio::task::spawn_blocking; use crate::args::{GlobalArgs, ProviderLoginArgs}; use crate::command_context::CommandContext; @@ -19,43 +18,37 @@ pub(super) async fn login_command( let s = Styles::detect_stderr(); let ctx = CommandContext::for_target(&args.target, printer)?; let server = ctx.server().await?; - - let use_oauth = args.provider == Provider::OpenAi - && spawn_blocking(|| provider_auth::prompt_confirm("Log in via browser (OAuth)?", true)) - .await??; - - let env_pairs = if use_oauth { - provider_auth::run_openai_oauth_or_api_key(&s, printer).await? - } else { - let (env_var, key) = - provider_auth::prompt_and_validate_key(args.provider, &s, printer).await?; - vec![(env_var, key)] - }; + let credential = provider_auth::authenticate_provider(args.provider, &s, printer).await?; + let credential_id = credential_id_for(&credential).map_err(anyhow::Error::msg)?; + let value = serde_json::to_string(&credential)?; { let path = legacy_env::legacy_env_file_path(); if path.exists() { fabro_util::printerr!( printer, - " Warning: {} is no longer read by fabro server. Re-enter credentials with `fabro provider login` or `fabro secret set`.", + " Warning: {} is no longer read by fabro server. Re-enter credentials with `fabro provider login`.", path.display() ); } } - for (name, value) in env_pairs { - server - .api() - .create_secret() - .body(types::CreateSecretRequest { - name: name.clone(), - value, - type_: types::SecretType::Environment, - description: None, - }) - .send() - .await?; - fabro_util::printerr!(printer, " {} Saved {}", s.green.apply_to("✔"), name); - } + server + .api() + .create_secret() + .body(types::CreateSecretRequest { + name: credential_id.clone(), + value, + type_: types::SecretType::Credential, + description: None, + }) + .send() + .await?; + fabro_util::printerr!( + printer, + " {} Saved {}", + s.green.apply_to("✔"), + credential_id + ); Ok(()) } diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 6e3a01b48..92c7ef6d7 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -92,6 +92,7 @@ pub(crate) async fn execute( artifact_sink, run_control: Some(run_control), github_app, + vault: None, on_node: None, registry_override: None, }; diff --git a/lib/crates/fabro-cli/src/shared/mod.rs b/lib/crates/fabro-cli/src/shared/mod.rs index 89c85fde6..25e76c1ce 100644 --- a/lib/crates/fabro-cli/src/shared/mod.rs +++ b/lib/crates/fabro-cli/src/shared/mod.rs @@ -1,5 +1,4 @@ pub(crate) mod github; -pub(crate) mod openai_jwt; pub(crate) mod provider_auth; pub(crate) mod repo; mod utilities; diff --git a/lib/crates/fabro-cli/src/shared/openai_jwt.rs b/lib/crates/fabro-cli/src/shared/openai_jwt.rs deleted file mode 100644 index e15665550..000000000 --- a/lib/crates/fabro-cli/src/shared/openai_jwt.rs +++ /dev/null @@ -1,138 +0,0 @@ -use base64::Engine; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; -use serde::Deserialize; - -pub(crate) const DEFAULT_CLIENT_ID: &str = "app_EMoamEEZ73f0CkXaXp7hrann"; -pub(crate) const DEFAULT_ISSUER: &str = "https://auth.openai.com"; -pub(crate) const OAUTH_PORT: u16 = 1455; - -#[derive(Deserialize)] -struct JwtPayload { - #[serde(default)] - chatgpt_account_id: Option, - #[serde(default, rename = "https://api.openai.com/auth")] - auth_claim: Option, - #[serde(default)] - organizations: Option>, -} - -#[derive(Deserialize)] -struct AuthClaim { - #[serde(default)] - chatgpt_account_id: Option, -} - -#[derive(Deserialize)] -struct Organization { - #[serde(default)] - id: Option, -} - -fn parse_jwt_payload(token: &str) -> Option { - let parts: Vec<&str> = token.split('.').collect(); - if parts.len() != 3 { - return None; - } - let payload_bytes = URL_SAFE_NO_PAD.decode(parts[1]).ok()?; - serde_json::from_slice(&payload_bytes).ok() -} - -pub(crate) fn extract_account_id(id_token: &str) -> Option { - let payload = parse_jwt_payload(id_token)?; - payload - .chatgpt_account_id - .or_else(|| { - payload - .auth_claim - .and_then(|claim| claim.chatgpt_account_id) - }) - .or_else(|| { - payload - .organizations - .and_then(|orgs| orgs.into_iter().next()) - .and_then(|org| org.id) - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn make_test_jwt(claims: &serde_json::Value) -> String { - let header = URL_SAFE_NO_PAD.encode(r#"{"alg":"RS256"}"#); - let payload = URL_SAFE_NO_PAD.encode(serde_json::to_string(claims).unwrap()); - format!("{header}.{payload}.signature") - } - - #[test] - fn parse_jwt_with_chatgpt_account_id() { - let jwt = make_test_jwt(&serde_json::json!({ - "chatgpt_account_id": "acct_123" - })); - let payload = parse_jwt_payload(&jwt).unwrap(); - assert_eq!(payload.chatgpt_account_id.as_deref(), Some("acct_123")); - } - - #[test] - fn parse_jwt_with_nested_auth_claim() { - let jwt = make_test_jwt(&serde_json::json!({ - "https://api.openai.com/auth": { - "chatgpt_account_id": "acct_nested" - } - })); - let payload = parse_jwt_payload(&jwt).unwrap(); - assert_eq!( - payload - .auth_claim - .and_then(|claim| claim.chatgpt_account_id) - .as_deref(), - Some("acct_nested") - ); - } - - #[test] - fn parse_jwt_invalid_format() { - assert!(parse_jwt_payload("not-a-jwt").is_none()); - } - - #[test] - fn parse_jwt_invalid_base64() { - assert!(parse_jwt_payload("header.!!!invalid!!!.sig").is_none()); - } - - #[test] - fn extract_account_id_prefers_top_level() { - let jwt = make_test_jwt(&serde_json::json!({ - "chatgpt_account_id": "top_level", - "https://api.openai.com/auth": { - "chatgpt_account_id": "nested" - }, - "organizations": [{"id": "org"}] - })); - assert_eq!(extract_account_id(&jwt).as_deref(), Some("top_level")); - } - - #[test] - fn extract_account_id_falls_back_to_nested() { - let jwt = make_test_jwt(&serde_json::json!({ - "https://api.openai.com/auth": { - "chatgpt_account_id": "nested" - } - })); - assert_eq!(extract_account_id(&jwt).as_deref(), Some("nested")); - } - - #[test] - fn extract_account_id_falls_back_to_first_organization() { - let jwt = make_test_jwt(&serde_json::json!({ - "organizations": [{"id": "org_456"}] - })); - assert_eq!(extract_account_id(&jwt).as_deref(), Some("org_456")); - } - - #[test] - fn extract_account_id_none_when_missing() { - let jwt = make_test_jwt(&serde_json::json!({})); - assert!(extract_account_id(&jwt).is_none()); - } -} diff --git a/lib/crates/fabro-cli/src/shared/provider_auth.rs b/lib/crates/fabro-cli/src/shared/provider_auth.rs index 524ae2cff..d8fc83411 100644 --- a/lib/crates/fabro-cli/src/shared/provider_auth.rs +++ b/lib/crates/fabro-cli/src/shared/provider_auth.rs @@ -4,6 +4,10 @@ use anyhow::Result; use dialoguer::console::Term; use dialoguer::theme::ColorfulTheme; use dialoguer::{Confirm, Password}; +use fabro_auth::{ + ApiCredential, ApiKeyHeader, AuthContextRequest, AuthContextResponse, AuthCredential, + AuthMethod, codex_oauth_config, strategy_for, +}; use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate}; use fabro_model::{Catalog, Provider}; @@ -12,8 +16,6 @@ use fabro_util::terminal::Styles; use tokio::task::spawn_blocking; use tokio::time::timeout; -use super::openai_jwt; - // --------------------------------------------------------------------------- // Provider key URLs // --------------------------------------------------------------------------- @@ -46,86 +48,6 @@ pub(crate) fn provider_display_name(provider: Provider) -> &'static str { } } -// --------------------------------------------------------------------------- -// OpenAI OAuth helpers -// --------------------------------------------------------------------------- - -/// Convert OAuth tokens to secret name/value pairs. -pub(crate) fn openai_oauth_env_pairs( - access_token: &str, - refresh_token: &str, - account_id: Option<&str>, -) -> Vec<(String, String)> { - let mut pairs = vec![ - ("OPENAI_API_KEY".to_string(), access_token.to_string()), - ( - "OPENAI_REFRESH_TOKEN".to_string(), - refresh_token.to_string(), - ), - ]; - if let Some(id) = account_id { - pairs.push(("CHATGPT_ACCOUNT_ID".to_string(), id.to_string())); - } - pairs -} - -// --------------------------------------------------------------------------- -// OpenAI OAuth browser flow with API-key fallback -// --------------------------------------------------------------------------- - -/// Run the OpenAI OAuth browser flow, falling back to manual API key entry on -/// failure. Returns the env-var pairs to persist. -pub(crate) async fn run_openai_oauth_or_api_key( - s: &Styles, - printer: Printer, -) -> Result> { - fabro_util::printerr!( - printer, - " {}", - s.dim.apply_to("Opening browser for OpenAI login...") - ); - match fabro_oauth::run_browser_flow( - openai_jwt::DEFAULT_ISSUER, - openai_jwt::DEFAULT_CLIENT_ID, - "openid profile email offline_access", - openai_jwt::OAUTH_PORT, - "/auth/callback", - ) - .await - { - Ok(tokens) => { - tracing::info!("OpenAI OAuth browser flow completed"); - let account_id = tokens - .id_token - .as_deref() - .and_then(openai_jwt::extract_account_id); - let refresh_token = tokens - .refresh_token - .as_deref() - .ok_or_else(|| anyhow::anyhow!("OpenAI did not return a refresh token"))?; - let pairs = - openai_oauth_env_pairs(&tokens.access_token, refresh_token, account_id.as_deref()); - fabro_util::printerr!( - printer, - " {} OpenAI configured via browser login", - s.green.apply_to("✔") - ); - Ok(pairs) - } - Err(e) => { - tracing::warn!(error = %e, "OpenAI OAuth browser flow failed"); - fabro_util::printerr!(printer, " Browser login failed: {e}"); - fabro_util::printerr!( - printer, - " {}", - s.dim.apply_to("Falling back to manual API key entry.") - ); - let (env_var, key) = prompt_and_validate_key(Provider::OpenAi, s, printer).await?; - Ok(vec![(env_var, key)]) - } - } -} - // --------------------------------------------------------------------------- // Interactive prompts // --------------------------------------------------------------------------- @@ -148,14 +70,23 @@ pub(crate) fn prompt_password(prompt: &str) -> Result { // --------------------------------------------------------------------------- pub(crate) async fn validate_api_key(provider: Provider, api_key: &str) -> Result<(), String> { - let env_var = provider.api_key_env_vars()[0]; - let client = LlmClient::from_lookup(|name| { - if name == env_var { - Some(api_key.to_string()) - } else { - None + let auth_header = if provider == Provider::Anthropic { + ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: api_key.to_string(), } - }) + } else { + ApiKeyHeader::Bearer(api_key.to_string()) + }; + let client = LlmClient::from_credentials(vec![ApiCredential { + provider, + auth_header, + extra_headers: std::collections::HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }]) .await .map_err(|e| e.to_string())?; @@ -213,6 +144,112 @@ pub(crate) async fn prompt_and_validate_key( } } +pub(crate) async fn pick_auth_method(provider: Provider) -> Result { + if provider != Provider::OpenAi { + return Ok(AuthMethod::ApiKey); + } + + let use_device_auth = + spawn_blocking(|| prompt_confirm("Log in with OpenAI account (device code)?", true)) + .await??; + if use_device_auth { + Ok(AuthMethod::CodexDevice(codex_oauth_config())) + } else { + Ok(AuthMethod::ApiKey) + } +} + +pub(crate) async fn authenticate_provider( + provider: Provider, + s: &Styles, + printer: Printer, +) -> Result { + let method = pick_auth_method(provider).await?; + authenticate_provider_with_method(provider, method, s, printer).await +} + +pub(crate) async fn authenticate_provider_with_method( + provider: Provider, + method: AuthMethod, + s: &Styles, + printer: Printer, +) -> Result { + let mut strategy = strategy_for(provider, method); + let request = strategy.init().await?; + present_to_user(&request, s, printer)?; + let response = await_user_response(&request, s, printer).await?; + strategy.complete(response).await +} + +pub(crate) fn present_to_user( + request: &AuthContextRequest, + s: &Styles, + printer: Printer, +) -> Result<()> { + match request { + AuthContextRequest::ApiKey { + provider, + env_var_names, + } => { + let env_var = env_var_names + .first() + .map(String::as_str) + .unwrap_or("API_KEY"); + let url = provider_key_url(*provider); + fabro_util::printerr!( + printer, + " {}", + s.dim.apply_to(format!("Get your API key at: {url}")) + ); + fabro_util::printerr!( + printer, + " {}", + s.dim.apply_to(format!("Expected variable name: {env_var}")) + ); + } + AuthContextRequest::DeviceCode { + user_code, + verification_uri, + expires_in, + } => { + fabro_util::printerr!(printer, " Open this URL in your browser:"); + fabro_util::printerr!(printer, " {verification_uri}"); + fabro_util::printerr!(printer, " Enter this one-time code:"); + fabro_util::printerr!(printer, " {}", s.bold.apply_to(user_code)); + fabro_util::printerr!( + printer, + " {}", + s.dim + .apply_to(format!("Code expires in {} minutes", expires_in / 60)) + ); + } + } + Ok(()) +} + +pub(crate) async fn await_user_response( + request: &AuthContextRequest, + s: &Styles, + printer: Printer, +) -> Result { + match request { + AuthContextRequest::ApiKey { provider, .. } => { + let (_, key) = prompt_and_validate_key(*provider, s, printer).await?; + Ok(AuthContextResponse::ApiKey { key }) + } + AuthContextRequest::DeviceCode { .. } => { + let ready = spawn_blocking(|| { + prompt_confirm("Continue after completing sign-in in the browser?", true) + }) + .await??; + if !ready { + return Err(anyhow::anyhow!("device code login cancelled")); + } + Ok(AuthContextResponse::DeviceCodeConfirmed) + } + } +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- @@ -221,33 +258,6 @@ pub(crate) async fn prompt_and_validate_key( mod tests { use super::*; - // -- OpenAI OAuth env pairs -- - - #[test] - fn openai_oauth_env_pairs_sets_api_key() { - let pairs = openai_oauth_env_pairs("tok", "ref", None); - assert!(pairs.contains(&("OPENAI_API_KEY".to_string(), "tok".to_string()))); - } - - #[test] - fn openai_oauth_env_pairs_sets_refresh_token() { - let pairs = openai_oauth_env_pairs("tok", "ref", None); - assert!(pairs.contains(&("OPENAI_REFRESH_TOKEN".to_string(), "ref".to_string()))); - } - - #[test] - fn openai_oauth_env_pairs_count() { - let pairs = openai_oauth_env_pairs("tok", "ref", None); - assert_eq!(pairs.len(), 2); - } - - #[test] - fn openai_oauth_env_pairs_with_account_id() { - let pairs = openai_oauth_env_pairs("tok", "ref", Some("acct_123")); - assert!(pairs.contains(&("CHATGPT_ACCOUNT_ID".to_string(), "acct_123".to_string()))); - assert_eq!(pairs.len(), 3); - } - // -- Provider key URLs -- #[test] diff --git a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs b/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs index 8a1ce48d2..33783477f 100644 --- a/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs +++ b/lib/crates/fabro-cli/tests/it/workflow/real_cli.rs @@ -14,7 +14,7 @@ async fn run_real_cli_test(provider: Provider, model: &str) { let env: Arc = Arc::new(fabro_agent::LocalSandbox::new( workspace.path().to_path_buf(), )); - let backend = AgentCliBackend::new(model.to_string(), provider) + let backend = AgentCliBackend::new_from_env(model.to_string(), provider) .with_poll_interval(Duration::from_millis(10)); let mut node = Node::new("real_cli_test"); diff --git a/lib/crates/fabro-llm/Cargo.toml b/lib/crates/fabro-llm/Cargo.toml index 676715b7e..a382decf6 100644 --- a/lib/crates/fabro-llm/Cargo.toml +++ b/lib/crates/fabro-llm/Cargo.toml @@ -32,6 +32,7 @@ bytes.workspace = true tokio-util.workspace = true tracing.workspace = true fabro-http.workspace = true +fabro-auth = { path = "../fabro-auth" } fabro-model = { path = "../fabro-model" } fabro-util = { path = "../fabro-util" } diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index 7dea66ab5..3b2db727b 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::sync::Arc; +use fabro_auth::{ApiCredential, ApiKeyHeader}; use tracing::debug; use crate::error::Error; @@ -9,6 +10,11 @@ use crate::provider::{ProviderAdapter, StreamEventStream}; use crate::providers; use crate::types::{Request, Response}; +const KIMI_BASE_URL: &str = "https://api.moonshot.ai/v1"; +const ZAI_BASE_URL: &str = "https://api.z.ai/api/coding/paas/v4"; +const MINIMAX_BASE_URL: &str = "https://api.minimax.io/v1"; +const INCEPTION_BASE_URL: &str = "https://api.inceptionlabs.ai/v1"; + /// The core client that routes requests to provider adapters (Section 2.2, 3). #[derive(Clone)] pub struct Client { @@ -40,90 +46,221 @@ impl Client { /// /// Returns `Error` if any provider adapter fails to initialize. pub async fn from_env() -> Result { - Self::from_lookup(|name| std::env::var(name).ok()).await + let mut credentials = Vec::new(); + if let Ok(key) = std::env::var("ANTHROPIC_API_KEY") { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Anthropic, + auth_header: ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: key, + }, + extra_headers: HashMap::new(), + base_url: std::env::var("ANTHROPIC_BASE_URL").ok(), + codex_mode: false, + org_id: None, + project_id: None, + }); + } + if let Ok(key) = std::env::var("OPENAI_API_KEY") { + let mut extra_headers = HashMap::new(); + let mut base_url = std::env::var("OPENAI_BASE_URL").ok(); + let mut codex_mode = false; + if let Ok(account_id) = std::env::var("CHATGPT_ACCOUNT_ID") { + base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); + codex_mode = true; + extra_headers.insert("ChatGPT-Account-Id".to_string(), account_id); + extra_headers.insert("originator".to_string(), "fabro".to_string()); + } + credentials.push(ApiCredential { + provider: fabro_model::Provider::OpenAi, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers, + base_url, + codex_mode, + org_id: std::env::var("OPENAI_ORG_ID").ok(), + project_id: std::env::var("OPENAI_PROJECT_ID").ok(), + }); + } + if let Ok(key) = + std::env::var("GEMINI_API_KEY").or_else(|_| std::env::var("GOOGLE_API_KEY")) + { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Gemini, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers: HashMap::new(), + base_url: std::env::var("GEMINI_BASE_URL").ok(), + codex_mode: false, + org_id: None, + project_id: None, + }); + } + if let Ok(key) = std::env::var("KIMI_API_KEY") { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Kimi, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }); + } + if let Ok(key) = std::env::var("ZAI_API_KEY") { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Zai, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }); + } + if let Ok(key) = std::env::var("MINIMAX_API_KEY") { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Minimax, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }); + } + if let Ok(key) = std::env::var("INCEPTION_API_KEY") { + credentials.push(ApiCredential { + provider: fabro_model::Provider::Inception, + auth_header: ApiKeyHeader::Bearer(key), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }); + } + Self::from_credentials(credentials).await } - /// Create a Client from a custom variable lookup. + /// Create a Client from typed provider credentials. /// - /// This is useful when credentials come from a source other than process - /// environment variables, while still preserving the env-style provider - /// configuration surface. - pub async fn from_lookup(lookup: F) -> Result - where - F: Fn(&str) -> Option, - { + /// # Errors + /// + /// Returns `Error` if any provider adapter fails to initialize. + pub async fn from_credentials(credentials: Vec) -> Result { let mut client = Self { providers: HashMap::new(), default_provider: None, middleware: Vec::new(), }; - // Register providers whose API keys are present in the environment. - // Order determines which becomes the default provider. - if let Some(key) = lookup("ANTHROPIC_API_KEY") { - let mut adapter = providers::AnthropicAdapter::new(key); - if let Some(base_url) = lookup("ANTHROPIC_BASE_URL") { - adapter = adapter.with_base_url(base_url); - } - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("OPENAI_API_KEY") { - let mut adapter = providers::OpenAiAdapter::new(key); - if let Some(account_id) = lookup("CHATGPT_ACCOUNT_ID") { - // Codex OAuth: route through chatgpt.com backend with required headers - adapter = adapter - .with_base_url("https://chatgpt.com/backend-api/codex") - .with_codex_mode(); - let mut headers = std::collections::HashMap::new(); - headers.insert("ChatGPT-Account-Id".to_string(), account_id); - headers.insert("originator".to_string(), "fabro".to_string()); - adapter = adapter.with_default_headers(headers); - } else if let Some(base_url) = lookup("OPENAI_BASE_URL") { - adapter = adapter.with_base_url(base_url); - } - if let Some(org_id) = lookup("OPENAI_ORG_ID") { - adapter = adapter.with_org_id(org_id); - } - if let Some(project_id) = lookup("OPENAI_PROJECT_ID") { - adapter = adapter.with_project_id(project_id); - } - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("GEMINI_API_KEY").or_else(|| lookup("GOOGLE_API_KEY")) { - let mut adapter = providers::GeminiAdapter::new(key); - if let Some(base_url) = lookup("GEMINI_BASE_URL") { - adapter = adapter.with_base_url(base_url); - } - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("KIMI_API_KEY") { - let adapter = - providers::OpenAiCompatibleAdapter::new(key, "https://api.moonshot.ai/v1") + for credential in credentials { + let auth_value = auth_value(&credential.auth_header); + match credential.provider { + fabro_model::Provider::Anthropic => { + let mut adapter = providers::AnthropicAdapter::new(auth_value); + if let Some(base_url) = credential.base_url { + adapter = adapter.with_base_url(base_url); + } + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::OpenAi => { + let mut adapter = providers::OpenAiAdapter::new(auth_value); + if let Some(base_url) = credential.base_url { + adapter = adapter.with_base_url(base_url); + } + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + if credential.codex_mode { + adapter = adapter.with_codex_mode(); + } + if let Some(org_id) = credential.org_id { + adapter = adapter.with_org_id(org_id); + } + if let Some(project_id) = credential.project_id { + adapter = adapter.with_project_id(project_id); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::Gemini => { + let mut adapter = providers::GeminiAdapter::new(auth_value); + if let Some(base_url) = credential.base_url { + adapter = adapter.with_base_url(base_url); + } + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::Kimi => { + let mut adapter = providers::OpenAiCompatibleAdapter::new( + auth_value, + credential + .base_url + .unwrap_or_else(|| KIMI_BASE_URL.to_string()), + ) .with_name("kimi"); - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("ZAI_API_KEY") { - let adapter = - providers::OpenAiCompatibleAdapter::new(key, "https://api.z.ai/api/coding/paas/v4") + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::Zai => { + let mut adapter = providers::OpenAiCompatibleAdapter::new( + auth_value, + credential + .base_url + .unwrap_or_else(|| ZAI_BASE_URL.to_string()), + ) .with_name("zai"); - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("MINIMAX_API_KEY") { - let adapter = providers::OpenAiCompatibleAdapter::new(key, "https://api.minimax.io/v1") - .with_name("minimax"); - client.register_provider(Arc::new(adapter)).await?; - } - if let Some(key) = lookup("INCEPTION_API_KEY") { - let adapter = - providers::OpenAiCompatibleAdapter::new(key, "https://api.inceptionlabs.ai/v1") + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::Minimax => { + let mut adapter = providers::OpenAiCompatibleAdapter::new( + auth_value, + credential + .base_url + .unwrap_or_else(|| MINIMAX_BASE_URL.to_string()), + ) + .with_name("minimax"); + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::Inception => { + let mut adapter = providers::OpenAiCompatibleAdapter::new( + auth_value, + credential + .base_url + .unwrap_or_else(|| INCEPTION_BASE_URL.to_string()), + ) .with_name("inception"); - client.register_provider(Arc::new(adapter)).await?; + if !credential.extra_headers.is_empty() { + adapter = adapter.with_default_headers(credential.extra_headers); + } + client.register_provider(Arc::new(adapter)).await?; + } + fabro_model::Provider::OpenAiCompatible => { + return Err(Error::Configuration { + message: "Provider::OpenAiCompatible is not supported by from_credentials" + .to_string(), + source: None, + }); + } + } } debug!( providers = ?client.provider_names(), default = ?client.default_provider(), - "LLM client initialized from environment" + "LLM client initialized from typed credentials" ); Ok(client) @@ -273,6 +410,13 @@ impl Client { } } +fn auth_value(auth_header: &ApiKeyHeader) -> String { + match auth_header { + ApiKeyHeader::Bearer(value) => value.clone(), + ApiKeyHeader::Custom { value, .. } => value.clone(), + } +} + #[cfg(test)] mod tests { use futures::stream; @@ -417,6 +561,58 @@ mod tests { assert!(matches!(result.unwrap_err(), Error::Configuration { .. })); } + #[tokio::test] + async fn from_credentials_registers_multiple_providers() { + let client = Client::from_credentials(vec![ + ApiCredential { + provider: fabro_model::Provider::Anthropic, + auth_header: ApiKeyHeader::Custom { + name: "x-api-key".to_string(), + value: "anthropic-key".to_string(), + }, + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }, + ApiCredential { + provider: fabro_model::Provider::OpenAi, + auth_header: ApiKeyHeader::Bearer("openai-key".to_string()), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }, + ]) + .await + .unwrap(); + + let mut providers = client.provider_names(); + providers.sort_unstable(); + assert_eq!(providers, vec!["anthropic", "openai"]); + assert_eq!(client.default_provider(), Some("anthropic")); + } + + #[tokio::test] + async fn from_credentials_supports_openai_compatible_provider_constants() { + let client = Client::from_credentials(vec![ApiCredential { + provider: fabro_model::Provider::Kimi, + auth_header: ApiKeyHeader::Bearer("kimi-key".to_string()), + extra_headers: HashMap::new(), + base_url: None, + codex_mode: false, + org_id: None, + project_id: None, + }]) + .await + .unwrap(); + + assert_eq!(client.provider_names(), vec!["kimi"]); + assert_eq!(client.default_provider(), Some("kimi")); + } + #[tokio::test] async fn register_sets_first_as_default() { let mut client = Client::new(HashMap::new(), None, vec![]); diff --git a/lib/crates/fabro-oauth/src/lib.rs b/lib/crates/fabro-oauth/src/lib.rs index e1315293d..08dbd5741 100644 --- a/lib/crates/fabro-oauth/src/lib.rs +++ b/lib/crates/fabro-oauth/src/lib.rs @@ -94,6 +94,11 @@ pub fn build_authorize_url( // Token types // --------------------------------------------------------------------------- +pub struct OAuthEndpoint<'a> { + pub token_url: &'a str, + pub client_id: &'a str, +} + #[derive(Debug, Deserialize)] pub struct TokenResponse { pub id_token: Option, @@ -106,27 +111,33 @@ pub struct TokenResponse { // Token exchange // --------------------------------------------------------------------------- -pub async fn exchange_code_for_tokens( - client: &fabro_http::HttpClient, - issuer: &str, - client_id: &str, +pub async fn exchange_code( + endpoint: OAuthEndpoint<'_>, code: &str, - redirect_uri: &str, - code_verifier: &str, + redirect_uri: Option<&str>, + verifier: Option<&str>, ) -> Result { - tracing::debug!(issuer, "Exchanging authorization code"); + tracing::debug!( + token_url = endpoint.token_url, + "Exchanging authorization code" + ); - let body = encode_form(&[ + let mut params = vec![ ("grant_type", "authorization_code"), - ("client_id", client_id), + ("client_id", endpoint.client_id), ("code", code), - ("redirect_uri", redirect_uri), - ("code_verifier", code_verifier), - ]); + ]; + if let Some(redirect_uri) = redirect_uri { + params.push(("redirect_uri", redirect_uri)); + } + if let Some(verifier) = verifier { + params.push(("code_verifier", verifier)); + } - let url = format!("{issuer}/oauth/token"); + let body = encode_form(¶ms); + let client = fabro_http::http_client().map_err(|e| e.to_string())?; let resp = client - .post(&url) + .post(endpoint.token_url) .header("content-type", "application/x-www-form-urlencoded") .body(body) .send() @@ -153,23 +164,21 @@ pub async fn exchange_code_for_tokens( // Token refresh // --------------------------------------------------------------------------- -pub async fn refresh_access_token( - client: &fabro_http::HttpClient, - issuer: &str, - client_id: &str, +pub async fn refresh_token( + endpoint: OAuthEndpoint<'_>, refresh_token: &str, ) -> Result { - tracing::debug!(issuer, "Refreshing access token"); + tracing::debug!(token_url = endpoint.token_url, "Refreshing access token"); let body = encode_form(&[ ("grant_type", "refresh_token"), - ("client_id", client_id), + ("client_id", endpoint.client_id), ("refresh_token", refresh_token), ]); - let url = format!("{issuer}/oauth/token"); + let client = fabro_http::http_client().map_err(|e| e.to_string())?; let resp = client - .post(&url) + .post(endpoint.token_url) .header("content-type", "application/x-www-form-urlencoded") .body(body) .send() @@ -394,14 +403,15 @@ pub async fn run_browser_flow( .map_err(|_| "Did not receive authorization code".to_string())? .map_err(|e| format!("Authorization failed: {e}"))?; - let client = fabro_http::http_client().map_err(|e| e.to_string())?; - exchange_code_for_tokens( - &client, - issuer, - client_id, + let token_url = format!("{issuer}/oauth/token"); + exchange_code( + OAuthEndpoint { + token_url: &token_url, + client_id, + }, &code, - &redirect_uri, - &pkce.verifier, + Some(&redirect_uri), + Some(&pkce.verifier), ) .await } @@ -599,14 +609,14 @@ mod tests { }) .await; - let client = test_http_client(); - let tokens = exchange_code_for_tokens( - &client, - &server.url(""), - "test-client", + let tokens = exchange_code( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, "test-code", - "http://localhost/cb", - "test-verifier", + Some("http://localhost/cb"), + Some("test-verifier"), ) .await .unwrap(); @@ -637,14 +647,14 @@ mod tests { }) .await; - let client = test_http_client(); - let tokens = exchange_code_for_tokens( - &client, - &server.url(""), - "test-client", + let tokens = exchange_code( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, "test-code", - "http://localhost/cb", - "test-verifier", + Some("http://localhost/cb"), + Some("test-verifier"), ) .await .unwrap(); @@ -666,14 +676,14 @@ mod tests { }) .await; - let client = test_http_client(); - let err = exchange_code_for_tokens( - &client, - &server.url(""), - "test-client", + let err = exchange_code( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, "bad-code", - "http://localhost/cb", - "verifier", + Some("http://localhost/cb"), + Some("verifier"), ) .await .unwrap_err(); @@ -681,6 +691,45 @@ mod tests { assert!(err.contains("400"), "error should contain status: {err}"); } + #[tokio::test] + async fn exchange_code_skips_optional_params_when_absent() { + let server = httpmock::MockServer::start_async().await; + + let mock = server + .mock_async(|when, then| { + when.method("POST") + .path("/oauth/token") + .header("content-type", "application/x-www-form-urlencoded") + .form_urlencoded_tuple("grant_type", "authorization_code") + .form_urlencoded_tuple("client_id", "test-client") + .form_urlencoded_tuple("code", "test-code"); + then.status(200) + .header("content-type", "application/json") + .body( + serde_json::json!({ + "access_token": "access-tok" + }) + .to_string(), + ); + }) + .await; + + let tokens = exchange_code( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, + "test-code", + None, + None, + ) + .await + .unwrap(); + + assert_eq!(tokens.access_token, "access-tok"); + mock.assert_async().await; + } + // ----------------------------------------------------------------------- // Phase 5: Token refresh // ----------------------------------------------------------------------- @@ -710,11 +759,15 @@ mod tests { }) .await; - let client = test_http_client(); - let tokens = - refresh_access_token(&client, &server.url(""), "test-client", "old-refresh-tok") - .await - .unwrap(); + let tokens = refresh_token( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, + "old-refresh-tok", + ) + .await + .unwrap(); assert_eq!(tokens.id_token.as_deref(), Some("new-id")); assert_eq!(tokens.access_token, "new-access"); @@ -735,10 +788,15 @@ mod tests { }) .await; - let client = test_http_client(); - let err = refresh_access_token(&client, &server.url(""), "test-client", "expired-tok") - .await - .unwrap_err(); + let err = refresh_token( + OAuthEndpoint { + token_url: &server.url("/oauth/token"), + client_id: "test-client", + }, + "expired-tok", + ) + .await + .unwrap_err(); assert!(err.contains("401"), "error should contain status: {err}"); } diff --git a/lib/crates/fabro-server/Cargo.toml b/lib/crates/fabro-server/Cargo.toml index c006e2a1c..db59de395 100644 --- a/lib/crates/fabro-server/Cargo.toml +++ b/lib/crates/fabro-server/Cargo.toml @@ -13,6 +13,7 @@ doctest = false workspace = true [dependencies] +fabro-auth = { path = "../fabro-auth" } fabro-spa = { path = "../fabro-spa" } fabro-config = { path = "../fabro-config" } fabro-graphviz = { path = "../fabro-graphviz" } diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index e28cfdd26..6160d502c 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -18,6 +18,7 @@ use tokio::process::Command; use tokio::time::timeout; use crate::server::AppState; +use crate::server_secrets::auth_issue_message; fn http_client_or_check( name: &str, @@ -196,29 +197,8 @@ pub async fn run_all(state: &AppState) -> DiagnosticsReport { } async fn check_llm_providers(state: &AppState) -> CheckResult { - let mut configured = Vec::new(); - for provider in Provider::ALL { - if state - .provider_credentials - .has_any(provider.api_key_env_vars()) - .await - { - configured.push(*provider); - } - } - - if configured.is_empty() { - return CheckResult { - name: "LLM Providers".to_string(), - status: CheckStatus::Error, - summary: "none configured".to_string(), - details: Vec::new(), - remediation: Some("Set at least one provider API key".to_string()), - }; - } - - let client = match state.build_llm_client().await { - Ok(client) => client, + let result = match state.build_llm_client().await { + Ok(result) => result, Err(err) => { return CheckResult { name: "LLM Providers".to_string(), @@ -229,13 +209,29 @@ async fn check_llm_providers(state: &AppState) -> CheckResult { }; } }; + if result.client.provider_names().is_empty() && result.auth_issues.is_empty() { + return CheckResult { + name: "LLM Providers".to_string(), + status: CheckStatus::Error, + summary: "none configured".to_string(), + details: Vec::new(), + remediation: Some("Set at least one provider API key".to_string()), + }; + } let mut details = Vec::new(); let mut failed = Vec::new(); - for provider in configured { + for (provider, issue) in &result.auth_issues { + failed.push(provider.to_string()); + details.push(CheckDetail::new(auth_issue_message(*provider, issue))); + } + for provider_name in result.client.provider_names() { + let Ok(provider) = provider_name.parse::() else { + continue; + }; let result = timeout( Duration::from_secs(30), - probe_llm_provider(&client, provider), + probe_llm_provider(&result.client, provider), ) .await; match result { @@ -257,7 +253,7 @@ async fn check_llm_providers(state: &AppState) -> CheckResult { CheckResult { name: "LLM Providers".to_string(), status: CheckStatus::Pass, - summary: format!("{} configured", details.len()), + summary: format!("{} configured", result.client.provider_names().len()), details, remediation: None, } @@ -265,7 +261,7 @@ async fn check_llm_providers(state: &AppState) -> CheckResult { CheckResult { name: "LLM Providers".to_string(), status: CheckStatus::Warning, - summary: "connectivity issues".to_string(), + summary: "some providers require attention".to_string(), details, remediation: Some(format!("Connectivity issues with: {}", failed.join(", "))), } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 1b6f81679..c223f5b0a 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -36,6 +36,7 @@ use fabro_workflow::run_materialization::materialize_run; use fabro_workflow::workflow_bundle::{BundledWorkflow, WorkflowBundle}; use crate::server::AppState; +use crate::server_secrets::auth_issue_message; #[derive(Clone)] pub(crate) struct PreparedManifest { @@ -548,12 +549,14 @@ async fn run_llm_check( let default_provider = provider.as_deref().unwrap_or("anthropic"); match state.build_llm_client().await { - Ok(client) => { - let configured = client + Ok(result) => { + let configured = result + .client .provider_names() .iter() .map(std::string::ToString::to_string) .collect::>(); + let auth_issues = result.auth_issues; let mut model_providers = std::collections::BTreeSet::new(); for node in graph.nodes.values() { @@ -589,19 +592,28 @@ async fn run_llm_check( let mut all_ok = true; for (model_id, provider_name) in &model_providers { match provider_name.parse::() { - Ok(_) => { + Ok(provider) => { let mut status = CheckStatus::Pass; - if !configured.iter().any(|name| name == provider_name) { + let remediation = if let Some((_, issue)) = auth_issues + .iter() + .find(|(candidate, _)| *candidate == provider) + { status = CheckStatus::Warning; all_ok = false; - } + Some(auth_issue_message(provider, issue)) + } else if !configured.iter().any(|name| name == provider_name) { + status = CheckStatus::Warning; + all_ok = false; + Some(format!("Provider \"{provider_name}\" is not configured")) + } else { + None + }; checks.push(CheckResult { name: "LLM".into(), status, summary: model_id.clone(), details: vec![CheckDetail::new(format!("Provider: {provider_name}"))], - remediation: (status == CheckStatus::Warning) - .then(|| format!("Provider \"{provider_name}\" is not configured")), + remediation, }); } Err(err) => { diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 1dbaed0b4..008e05aa6 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -6,7 +6,6 @@ use anyhow::Context; use clap::Args; use fabro_config::user::{active_settings_path, load_settings_config}; use fabro_config::{Storage, resolve_server_from_file}; -use fabro_llm::client::Client as LlmClient; use fabro_sandbox::SandboxProvider; use fabro_types::settings::server::GithubIntegrationStrategy; use fabro_types::settings::{ @@ -20,7 +19,7 @@ use object_store::aws::AmazonS3Builder; use object_store::local::LocalFileSystem; use object_store::memory::InMemory; use tokio::net::{TcpListener, UnixListener}; -use tokio::sync::watch; +use tokio::sync::{RwLock as AsyncRwLock, watch}; use tokio::time::interval; use tracing::{error, info, warn}; @@ -31,7 +30,7 @@ use crate::server::{ RouterOptions, build_app_state_with_path, build_router_with_options, reconcile_incomplete_runs_on_startup, shutdown_active_workers, spawn_scheduler, }; -use crate::server_secrets::ServerSecrets; +use crate::server_secrets::{ProviderCredentials, ServerSecrets}; use crate::tls::{ClientAuth, build_rustls_config, serve_tls_with_shutdown}; const TEST_IN_MEMORY_STORE_ENV: &str = "FABRO_TEST_IN_MEMORY_STORE"; @@ -267,22 +266,20 @@ where }; let storage = Storage::new(&data_dir); let vault_path = storage.secrets_path(); - let vault = Vault::load(vault_path.clone())?; - let vault_snapshot = vault.snapshot(); + let vault = Arc::new(AsyncRwLock::new(Vault::load(vault_path.clone())?)); let server_secrets = ServerSecrets::load(storage.server_state().env_path())?; // Resolve dry-run mode (same pattern as run.rs) let dry_run_mode = if args.dry_run { true } else { - match LlmClient::from_lookup(|name| { - std::env::var(name) - .ok() - .or_else(|| vault_snapshot.get(name).cloned()) - }) - .await + match ProviderCredentials::new(Arc::clone(&vault)) + .build_llm_client() + .await { - Ok(c) if c.provider_names().is_empty() => { + Ok(result) + if result.client.provider_names().is_empty() && result.auth_issues.is_empty() => + { eprintln!( "{} No LLM providers configured. Running in dry-run mode.", styles.yellow.apply_to("Warning:"), diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index babe221e7..b8702f8bd 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -37,12 +37,12 @@ pub use fabro_api::types::{ SshAccessRequest, SshAccessResponse, StartRunRequest, StatusReason as ApiStatusReason, SubmitAnswerRequest, SystemInfoResponse, SystemRunCounts, WriteBlobResponse, }; +use fabro_auth::parse_credential_secret; use fabro_config::{Storage, resolve_server_from_file}; use fabro_graphviz::render::GraphFormat; use fabro_interview::{ Answer, ControlInterviewer, Interviewer, Question, QuestionType, WorkerControlEnvelope, }; -use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate_object}; use fabro_llm::model_test::{ModelTestMode, run_model_test_with_client}; use fabro_llm::types::{ @@ -105,7 +105,7 @@ use tokio_stream::StreamExt; use tokio_stream::wrappers::{BroadcastStream, UnboundedReceiverStream}; use tower::{ServiceExt, service_fn}; use tower_http::trace::TraceLayer; -use tracing::{debug, error, info}; +use tracing::{debug, error, info, warn}; use ulid::Ulid; use crate::bind::Bind; @@ -113,7 +113,9 @@ use crate::error::ApiError; use crate::jwt_auth::{ AuthMode, AuthenticatedService, AuthenticatedSubject, authenticate_service_parts, }; -use crate::server_secrets::{ProviderCredentials, ServerSecrets}; +use crate::server_secrets::{ + LlmClientResult, ProviderCredentials, ServerSecrets, auth_issue_message, +}; use crate::{demo, diagnostics, run_manifest, settings_view, static_files, web_auth}; pub fn default_page_limit() -> u32 { @@ -596,7 +598,7 @@ impl AppState { .unwrap_or(false) } - pub(crate) async fn build_llm_client(&self) -> Result { + pub(crate) async fn build_llm_client(&self) -> Result { self.provider_credentials.build_llm_client().await } @@ -1633,6 +1635,7 @@ fn secret_type_from_api(secret_type: ApiSecretType) -> SecretType { match secret_type { ApiSecretType::Environment => SecretType::Environment, ApiSecretType::File => SecretType::File, + ApiSecretType::Credential => SecretType::Credential, } } @@ -1645,6 +1648,11 @@ async fn create_secret( let name = body.name; let value = body.value; let description = body.description; + if secret_type == SecretType::Credential { + if let Err(err) = parse_credential_secret(&name, &value) { + return ApiError::bad_request(err).into_response(); + } + } let state_for_write = Arc::clone(&state); let result = spawn_blocking(move || { let mut vault = state_for_write.vault.blocking_write(); @@ -3873,6 +3881,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { artifact_sink: Some(ArtifactSink::Store(state.artifact_store.clone())), run_control: None, github_app, + vault: Some(Arc::clone(&state.vault)), on_node: None, registry_override, }; @@ -5811,8 +5820,8 @@ async fn test_model( .into_response(); } - let client = match state.build_llm_client().await { - Ok(client) => Arc::new(client), + let llm_result = match state.build_llm_client().await { + Ok(result) => result, Err(err) => { return ApiError::new( StatusCode::INTERNAL_SERVER_ERROR, @@ -5821,6 +5830,14 @@ async fn test_model( .into_response(); } }; + if let Some((_, issue)) = llm_result + .auth_issues + .iter() + .find(|(provider, _)| *provider == info.provider) + { + return ApiError::bad_request(auth_issue_message(info.provider, issue)).into_response(); + } + let client = Arc::new(llm_result.client); let outcome = run_model_test_with_client(info, mode, client).await; Json(serde_json::json!({ @@ -6013,8 +6030,8 @@ async fn create_completion( } // Get or create LLM client (cached in AppState) - let client = match state.build_llm_client().await { - Ok(client) => client, + let llm_result = match state.build_llm_client().await { + Ok(result) => result, Err(err) => { return ApiError::new( StatusCode::INTERNAL_SERVER_ERROR, @@ -6023,6 +6040,10 @@ async fn create_completion( .into_response(); } }; + for (provider, issue) in &llm_result.auth_issues { + warn!(provider = %provider, error = %issue, "LLM provider unavailable due to auth issue"); + } + let client = llm_result.client; if use_stream { // Streaming path: forward all StreamEvents as SSE @@ -6181,6 +6202,7 @@ mod tests { use axum::body::Body; use axum::http::Request; use fabro_interview::{AnswerValue, ControlInterviewer, Interviewer, Question, QuestionType}; + use fabro_model::Provider; use fabro_types::{InterviewQuestionRecord, InterviewQuestionType, RunBlobId, RunId, fixtures}; use tower::ServiceExt; @@ -6290,6 +6312,115 @@ type = "http" )]); } + #[tokio::test] + async fn create_secret_stores_valid_credential_entries() { + let state = create_app_state(); + let app = build_router(Arc::clone(&state), AuthMode::Disabled); + let credential = fabro_auth::AuthCredential { + provider: Provider::OpenAi, + details: fabro_auth::AuthDetails::CodexOAuth { + tokens: fabro_auth::OAuthTokens { + access_token: "access".to_string(), + refresh_token: Some("refresh".to_string()), + expires_at: chrono::DateTime::parse_from_rfc3339("2030-01-01T00:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc), + }, + config: fabro_auth::OAuthConfig { + auth_url: "https://auth.openai.com".to_string(), + token_url: "https://auth.openai.com/oauth/token".to_string(), + client_id: "client".to_string(), + scopes: vec!["openid".to_string()], + redirect_uri: Some("https://auth.openai.com/deviceauth/callback".to_string()), + use_pkce: true, + }, + account_id: Some("acct_123".to_string()), + }, + }; + + let req = Request::builder() + .method("POST") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "openai_codex", + "value": serde_json::to_string(&credential).unwrap(), + "type": "credential" + })) + .unwrap(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + assert!(state.vault.read().await.list().is_empty()); + assert!(state.vault.read().await.get("openai_codex").is_some()); + } + + #[tokio::test] + async fn create_secret_rejects_invalid_credential_json() { + let state = create_app_state(); + let app = build_router(state, AuthMode::Disabled); + + let req = Request::builder() + .method("POST") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "openai_codex", + "value": "{not-json", + "type": "credential" + })) + .unwrap(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + } + + #[tokio::test] + async fn create_secret_rejects_wrong_credential_name() { + let state = create_app_state(); + let app = build_router(state, AuthMode::Disabled); + + let req = Request::builder() + .method("POST") + .uri(api("/secrets")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({ + "name": "openai", + "value": serde_json::to_string(&serde_json::json!({ + "provider": "openai", + "type": "codex_oauth", + "tokens": { + "access_token": "access", + "refresh_token": "refresh", + "expires_at": "2030-01-01T00:00:00Z" + }, + "config": { + "auth_url": "https://auth.openai.com", + "token_url": "https://auth.openai.com/oauth/token", + "client_id": "client", + "scopes": ["openid"], + "redirect_uri": "https://auth.openai.com/deviceauth/callback", + "use_pkce": true + } + })) + .unwrap(), + "type": "credential" + })) + .unwrap(), + )) + .unwrap(); + + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + } + #[tokio::test] async fn delete_secret_by_name_removes_file_secret() { let state = create_app_state(); diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index 911ad16de..aa9b9ff5b 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -2,30 +2,15 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use fabro_auth::{CredentialResolver, CredentialUsage, ResolveError, ResolvedCredential}; use fabro_config::envfile; use fabro_llm::client::Client as LlmClient; +use fabro_model::Provider; use fabro_vault::Vault; use tokio::sync::RwLock as AsyncRwLock; type EnvLookup = Arc Option + Send + Sync>; -const PROVIDER_LOOKUP_NAMES: &[&str] = &[ - "ANTHROPIC_API_KEY", - "ANTHROPIC_BASE_URL", - "OPENAI_API_KEY", - "CHATGPT_ACCOUNT_ID", - "OPENAI_BASE_URL", - "OPENAI_ORG_ID", - "OPENAI_PROJECT_ID", - "GEMINI_API_KEY", - "GOOGLE_API_KEY", - "GEMINI_BASE_URL", - "KIMI_API_KEY", - "ZAI_API_KEY", - "MINIMAX_API_KEY", - "INCEPTION_API_KEY", -]; - #[derive(Debug, thiserror::Error)] pub(crate) enum Error { #[error(transparent)] @@ -102,6 +87,7 @@ impl ProviderCredentials { } } + #[cfg(test)] pub(crate) async fn get(&self, name: &str) -> Option { let env_value = (self.env_lookup)(name); if env_value.is_some() { @@ -111,29 +97,67 @@ impl ProviderCredentials { self.vault.read().await.get(name).map(str::to_string) } - pub(crate) async fn has_any(&self, names: &[&str]) -> bool { - for name in names { - if self.get(name).await.is_some() { - return true; + pub(crate) async fn build_llm_client(&self) -> Result { + let resolver = + CredentialResolver::with_env_lookup(Arc::clone(&self.vault), self.env_lookup.clone()); + let mut api_credentials = Vec::new(); + let mut auth_issues = Vec::new(); + + for provider in Provider::ALL { + match resolver + .resolve(*provider, CredentialUsage::ApiRequest) + .await + { + Ok(ResolvedCredential::Api(credential)) => api_credentials.push(credential), + Ok(ResolvedCredential::Cli(_)) => {} + Err(ResolveError::NotConfigured(_)) => {} + Err(err) => auth_issues.push((*provider, err)), } } - false - } - pub(crate) async fn build_llm_client(&self) -> Result { - let vault_snapshot = self.vault.read().await.snapshot(); - let lookup = PROVIDER_LOOKUP_NAMES - .iter() - .filter_map(|name| { - (self.env_lookup)(name) - .or_else(|| vault_snapshot.get(*name).cloned()) - .map(|value| ((*name).to_string(), value)) - }) - .collect::>(); - - LlmClient::from_lookup(|name| lookup.get(name).cloned()) + let client = LlmClient::from_credentials(api_credentials) .await - .map_err(|err| err.to_string()) + .map_err(|err| err.to_string())?; + + Ok(LlmClientResult { + client, + auth_issues, + }) + } +} + +pub(crate) struct LlmClientResult { + pub client: LlmClient, + pub auth_issues: Vec<(Provider, ResolveError)>, +} + +pub(crate) fn provider_display_name(provider: Provider) -> &'static str { + match provider { + Provider::Anthropic => "Anthropic", + Provider::OpenAi => "OpenAI", + Provider::Gemini => "Gemini", + Provider::Kimi => "Kimi", + Provider::Zai => "Zai", + Provider::Minimax => "Minimax", + Provider::Inception => "Inception", + Provider::OpenAiCompatible => "OpenAI Compatible", + } +} + +pub(crate) fn auth_issue_message(provider: Provider, err: &ResolveError) -> String { + match err { + ResolveError::NotConfigured(_) => { + format!("{} is not configured", provider_display_name(provider)) + } + ResolveError::RefreshFailed { source, .. } => format!( + "{} requires re-authentication: {}", + provider_display_name(provider), + source + ), + ResolveError::RefreshTokenMissing(_) => format!( + "{} requires re-authentication: refresh token missing", + provider_display_name(provider) + ), } } diff --git a/lib/crates/fabro-vault/src/lib.rs b/lib/crates/fabro-vault/src/lib.rs index 3c137c5ab..c612bad2d 100644 --- a/lib/crates/fabro-vault/src/lib.rs +++ b/lib/crates/fabro-vault/src/lib.rs @@ -8,6 +8,7 @@ pub enum SecretType { #[default] Environment, File, + Credential, } #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] @@ -134,6 +135,7 @@ impl Vault { let mut data = self .entries .iter() + .filter(|(_, entry)| entry.secret_type != SecretType::Credential) .map(|(name, entry)| SecretMetadata { name: name.clone(), secret_type: entry.secret_type, @@ -150,6 +152,10 @@ impl Vault { self.entries.get(name).map(|entry| entry.value.as_str()) } + pub fn get_entry(&self, name: &str) -> Option<&SecretEntry> { + self.entries.get(name) + } + pub fn snapshot(&self) -> HashMap { self.entries .iter() @@ -158,6 +164,17 @@ impl Vault { .collect() } + pub fn credential_entries(&self) -> Vec<(&str, &SecretEntry)> { + let mut data = self + .entries + .iter() + .filter(|(_, entry)| entry.secret_type == SecretType::Credential) + .map(|(name, entry)| (name.as_str(), entry)) + .collect::>(); + data.sort_by(|a, b| a.0.cmp(b.0)); + data + } + pub fn file_secrets(&self) -> Vec<(String, String)> { let mut data = self .entries @@ -171,7 +188,7 @@ impl Vault { pub fn validate_name(name: &str, secret_type: SecretType) -> Result<(), Error> { match secret_type { - SecretType::Environment => Self::validate_env_name(name), + SecretType::Environment | SecretType::Credential => Self::validate_env_name(name), SecretType::File => Self::validate_file_name(name), } } @@ -332,4 +349,76 @@ mod tests { "pem".to_string() )]); } + + #[test] + fn list_hides_credential_entries_loaded_from_disk() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secrets.json"); + std::fs::write( + &path, + serde_json::json!({ + "OPENAI_API_KEY": { + "value": "env", + "type": "environment", + "created_at": "2026-04-12T00:00:00Z", + "updated_at": "2026-04-12T00:00:00Z" + }, + "openai_codex": { + "value": "{\"provider\":\"openai\"}", + "type": "credential", + "created_at": "2026-04-12T00:00:00Z", + "updated_at": "2026-04-12T00:00:00Z" + } + }) + .to_string(), + ) + .unwrap(); + + let store = Vault::load(path).unwrap(); + + assert_eq!(store.list().len(), 1); + assert_eq!(store.list()[0].name, "OPENAI_API_KEY"); + assert_eq!(store.get("openai_codex"), Some("{\"provider\":\"openai\"}")); + } + + #[test] + fn get_entry_returns_full_secret_entry() { + let dir = tempfile::tempdir().unwrap(); + let mut store = Vault::load(dir.path().join("secrets.json")).unwrap(); + store + .set( + "openai_codex", + "credential-json", + SecretType::Credential, + Some("saved auth"), + ) + .unwrap(); + + let entry = store.get_entry("openai_codex").unwrap(); + + assert_eq!(entry.value, "credential-json"); + assert_eq!(entry.secret_type, SecretType::Credential); + assert_eq!(entry.description.as_deref(), Some("saved auth")); + } + + #[test] + fn credential_entries_only_returns_credentials() { + let dir = tempfile::tempdir().unwrap(); + let mut store = Vault::load(dir.path().join("secrets.json")).unwrap(); + store + .set("OPENAI_API_KEY", "env", SecretType::Environment, None) + .unwrap(); + store + .set( + "openai_codex", + "credential-json", + SecretType::Credential, + None, + ) + .unwrap(); + + assert_eq!(store.credential_entries().len(), 1); + assert_eq!(store.credential_entries()[0].0, "openai_codex"); + assert_eq!(store.credential_entries()[0].1.value, "credential-json"); + } } diff --git a/lib/crates/fabro-workflow/Cargo.toml b/lib/crates/fabro-workflow/Cargo.toml index f0714b65c..11bdfc4a9 100644 --- a/lib/crates/fabro-workflow/Cargo.toml +++ b/lib/crates/fabro-workflow/Cargo.toml @@ -18,6 +18,7 @@ workspace = true [dependencies] anyhow.workspace = true +fabro-auth = { path = "../fabro-auth" } fabro-agent = { path = "../fabro-agent" } fabro-config = { path = "../fabro-config" } fabro-graphviz = { path = "../fabro-graphviz" } @@ -64,6 +65,7 @@ tracing.workspace = true walkdir.workspace = true tempfile = "3" toml.workspace = true +fabro-vault = { path = "../fabro-vault" } [dev-dependencies] base64.workspace = true fabro-mcp = { path = "../fabro-mcp" } diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index 42b2915eb..a34c15d28 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use async_trait::async_trait; use fabro_agent::Sandbox; use fabro_agent::sandbox::ExecResult; +use fabro_auth::{CliAgentKind, CredentialResolver, CredentialUsage, ResolvedCredential}; use fabro_graphviz::graph::Node; use fabro_llm::types::TokenCounts; use fabro_model::Provider; @@ -369,8 +370,11 @@ pub fn parse_cli_response(provider: Provider, output: &str) -> Option String { - val.replace('\'', "'\\''") +fn shell_quote(val: &str) -> String { + shlex::try_quote(val).map_or_else( + |_| format!("'{}'", val.replace('\'', "'\\''")), + std::borrow::Cow::into_owned, + ) } /// CLI backend that invokes external CLI tools (claude, codex, gemini) via @@ -380,16 +384,29 @@ pub struct AgentCliBackend { provider: Provider, env: HashMap, poll_interval: std::time::Duration, + resolver: Option, } impl AgentCliBackend { #[must_use] - pub fn new(model: String, provider: Provider) -> Self { + pub fn new(model: String, provider: Provider, resolver: CredentialResolver) -> Self { Self { model, provider, env: HashMap::new(), poll_interval: std::time::Duration::from_secs(5), + resolver: Some(resolver), + } + } + + #[must_use] + pub fn new_from_env(model: String, provider: Provider) -> Self { + Self { + model, + provider, + env: HashMap::new(), + poll_interval: std::time::Duration::from_secs(5), + resolver: None, } } @@ -516,26 +533,22 @@ impl CodergenBackend for AgentCliBackend { // base64-encoded command, avoiding filesystem-to-process race // conditions that can occur when writing an env file via the fs API and // sourcing it via the process API. - let mut launch_env: HashMap = HashMap::new(); - for name in provider.api_key_env_vars() { - if let Ok(val) = std::env::var(name) { - launch_env.insert((*name).to_string(), val); - } - } - for (name, val) in &self.env { - launch_env.insert(name.clone(), val.clone()); - } - - // Codex CLI requires `codex login --with-api-key` to store credentials; - // it does not read OPENAI_API_KEY from the environment at runtime. - if cli == AgentCli::Codex { - if let Some(api_key) = launch_env.get("OPENAI_API_KEY") { - let login_cmd = format!( - "PATH=\"$HOME/.local/bin:$PATH\" echo '{}' | codex login --with-api-key", - shell_escape(api_key) - ); + let cli_agent = match cli { + AgentCli::Claude => CliAgentKind::Claude, + AgentCli::Codex => CliAgentKind::Codex, + AgentCli::Gemini => CliAgentKind::Gemini, + }; + let mut launch_env = if let Some(resolver) = &self.resolver { + let resolved = resolver + .resolve(provider, CredentialUsage::CliAgent(cli_agent)) + .await + .map_err(|e| Error::handler(format!("Failed to resolve CLI credential: {e}")))?; + let ResolvedCredential::Cli(cli_credential) = resolved else { + return Err(Error::handler("Expected CLI credential".to_string())); + }; + if let Some(login_cmd) = &cli_credential.login_command { let login_result = sandbox - .exec_command(&login_cmd, 30_000, None, None, None) + .exec_command(login_cmd, 30_000, None, None, None) .await .map_err(|e| Error::handler(format!("codex login failed: {e}")))?; if login_result.exit_code != 0 { @@ -546,6 +559,18 @@ impl CodergenBackend for AgentCliBackend { ); } } + cli_credential.env_vars + } else { + let mut env = HashMap::new(); + for name in provider.api_key_env_vars() { + if let Ok(val) = std::env::var(name) { + env.insert((*name).to_string(), val); + } + } + env + }; + for (name, val) in &self.env { + launch_env.insert(name.clone(), val.clone()); } // Also write env file as fallback for commands that source it (e.g. ensure_cli @@ -554,7 +579,7 @@ impl CodergenBackend for AgentCliBackend { env_lines.extend( launch_env .iter() - .map(|(k, v)| format!("export {k}='{}'", shell_escape(v))), + .map(|(k, v)| format!("export {k}={}", shell_quote(v))), ); { sandbox @@ -1161,7 +1186,7 @@ mod tests { node.attrs .insert("backend".to_string(), AttrValue::String("cli".to_string())); - let cli_backend = AgentCliBackend::new("model".into(), Provider::Anthropic); + let cli_backend = AgentCliBackend::new_from_env("model".into(), Provider::Anthropic); let router = BackendRouter::new(Box::new(StubBackend), cli_backend); assert!(router.should_use_cli(&node)); } @@ -1170,7 +1195,7 @@ mod tests { fn router_uses_api_by_default() { let node = Node::new("test"); - let cli_backend = AgentCliBackend::new("model".into(), Provider::Anthropic); + let cli_backend = AgentCliBackend::new_from_env("model".into(), Provider::Anthropic); let router = BackendRouter::new(Box::new(StubBackend), cli_backend); assert!(!router.should_use_cli(&node)); } @@ -1183,7 +1208,7 @@ mod tests { AttrValue::String("claude-opus-4-6".to_string()), ); - let cli_backend = AgentCliBackend::new("model".into(), Provider::Anthropic); + let cli_backend = AgentCliBackend::new_from_env("model".into(), Provider::Anthropic); let router = BackendRouter::new(Box::new(StubBackend), cli_backend); assert!(!router.should_use_cli(&node)); } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 7dcb9f984..7965cd1a2 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -25,7 +25,9 @@ use fabro_types::settings::run::{ PullRequestSettings, RunMode, RunModelSettings as ResolvedRunModelSettings, RunSettings as ResolvedRunSettings, TlsMode as ResolvedTlsMode, }; +use fabro_vault::Vault; use tokio::runtime::Handle; +use tokio::sync::RwLock as AsyncRwLock; use crate::artifact_upload::ArtifactSink; use crate::context::Context; @@ -76,6 +78,7 @@ struct RunSession { workflow_path: Option, workflow_bundle: Option>, run_control: Option>, + vault: Option>>, } pub struct StartServices { @@ -88,6 +91,7 @@ pub struct StartServices { pub artifact_sink: Option, pub run_control: Option>, pub github_app: Option, + pub vault: Option>>, pub on_node: crate::OnNodeCallback, pub registry_override: Option>, } @@ -428,6 +432,7 @@ impl RunSession { pr_model: model, workflow_path, workflow_bundle, + vault: services.vault, }) } } @@ -701,6 +706,7 @@ impl RunSession { workflow_bundle: self.workflow_bundle, hooks: self.hooks, sandbox_env: self.sandbox_env, + vault: self.vault, devcontainer: self.devcontainer, git: self.git, worktree_mode: self.worktree_mode, @@ -1048,6 +1054,7 @@ mod tests { artifact_sink: None, run_control: None, github_app: None, + vault: None, on_node: None, registry_override: Some(registry), } diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index 2d995fe04..5712a1356 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -218,6 +218,7 @@ async fn execute_test_run_with_options( github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: git_options, worktree_mode: None, @@ -275,6 +276,7 @@ async fn execute_runs_start_to_exit_and_returns_final_context() { github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, @@ -342,6 +344,7 @@ async fn run_with_lifecycle( github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 9de5e25c4..40e8a6151 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::time::Instant; use fabro_agent::Sandbox; +use fabro_auth::CredentialResolver; use fabro_config::RunScratch; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookRunner}; @@ -12,9 +13,11 @@ use fabro_sandbox::{ ReadBeforeWriteSandbox, SandboxEventCallback, SandboxSpec, WorkdirStrategy, WorktreeOptions, WorktreeSandbox, }; +use fabro_vault::Vault; use shlex::try_quote; use tokio::process::Command as TokioCommand; use tokio::runtime::Handle; +use tokio::sync::RwLock as AsyncRwLock; use tokio::task::spawn_blocking; use tokio::time::timeout as tokio_timeout; @@ -279,6 +282,7 @@ async fn build_registry( interviewer: Arc, sandbox_env: &HashMap, graph: &graph::Graph, + vault: Option>>, ) -> Result<(Arc, Option, bool), Error> { let build_no_backend = || Arc::new(default_registry(Arc::clone(&interviewer), || None)); @@ -306,11 +310,18 @@ async fn build_registry( let provider = spec.provider; let fallback_chain = spec.fallback_chain.clone(); let mcp_servers = spec.mcp_servers.clone(); + let resolver = vault.map(CredentialResolver::new); let registry = Arc::new(default_registry(interviewer, move || { let api = AgentApiBackend::new(model.clone(), provider, fallback_chain.clone()) .with_env(env.clone()) .with_mcp_servers(mcp_servers.clone()); - let cli = AgentCliBackend::new(model.clone(), provider).with_env(env.clone()); + let cli = resolver + .clone() + .map_or_else( + || AgentCliBackend::new_from_env(model.clone(), provider), + |resolver| AgentCliBackend::new(model.clone(), provider, resolver), + ) + .with_env(env.clone()); Some(Box::new(BackendRouter::new(Box::new(api), cli))) })); Ok((registry, Some(client), false)) @@ -545,7 +556,14 @@ pub async fn initialize( // A caller-supplied registry owns execution behavior for its handlers. (registry, None, options.dry_run) } else { - build_registry(&options.llm, Arc::clone(&options.interviewer), &env, &graph).await? + build_registry( + &options.llm, + Arc::clone(&options.interviewer), + &env, + &graph, + options.vault.clone(), + ) + .await? }; if effective_dry_run { use fabro_types::settings::run::{RunExecutionLayer, RunLayer, RunMode}; @@ -838,6 +856,7 @@ mod tests { github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, @@ -912,6 +931,7 @@ mod tests { github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, @@ -980,6 +1000,7 @@ mod tests { github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, @@ -1042,6 +1063,7 @@ mod tests { github_permissions: None, origin_url: None, }, + vault: None, devcontainer: None, git: None, worktree_mode: None, diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index ff87e0e20..1c69b3fe3 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -16,6 +16,8 @@ use fabro_sandbox::config::WorktreeMode; use fabro_types::RunId; use fabro_types::settings::run::PullRequestSettings; use fabro_validate::{Diagnostic, Severity}; +use fabro_vault::Vault; +use tokio::sync::RwLock as AsyncRwLock; use crate::artifact_upload::ArtifactSink; use crate::context::Context; @@ -245,6 +247,7 @@ pub struct InitOptions { pub workflow_bundle: Option>, pub hooks: fabro_hooks::HookSettings, pub sandbox_env: SandboxEnvSpec, + pub vault: Option>>, pub devcontainer: Option, pub git: Option, pub worktree_mode: Option, diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 82f3392e5..7f3ee4a19 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -1043,7 +1043,7 @@ async fn run_daytona_cli_test(provider: Provider, model: &str, install_command: install_result.exit_code, install_result.stdout ); - let backend = AgentCliBackend::new(model.to_string(), provider); + let backend = AgentCliBackend::new_from_env(model.to_string(), provider); let node = Node::new("daytona_cli_test"); let context = Context::new(); let emitter = Arc::new(Emitter::default()); diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 89da5396e..618684676 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -9360,7 +9360,7 @@ async fn cli_backend_run_writes_prompt_and_calls_exec() { let claude_output = r#"{"type":"result","result":"I fixed the bug.","usage":{"input_tokens":500,"output_tokens":200}}"#; let test_env = Arc::new(CliTestEnv::new(claude_output)); let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("fix_code"); @@ -9432,7 +9432,7 @@ async fn cli_backend_run_detects_changed_files() { let claude_output = r#"{"type":"result","result":"Created new file.","usage":{"input_tokens":100,"output_tokens":50}}"#; let env: Arc = Arc::new(CliTestEnv::new(claude_output).with_git_diff_after("src/main.rs\nsrc/lib.rs\n")); - let backend = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("implement"); @@ -9465,7 +9465,7 @@ async fn cli_backend_run_with_codex_provider() { let codex_output = "{\"type\":\"item.completed\",\"item\":{\"id\":\"item_0\",\"type\":\"agent_message\",\"text\":\"Implemented the feature.\"}}\n{\"type\":\"turn.completed\",\"usage\":{\"input_tokens\":300,\"output_tokens\":150}}"; let test_env = Arc::new(CliTestEnv::new(codex_output)); let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new("gpt-5.3-codex".into(), Provider::OpenAi) + let backend = AgentCliBackend::new_from_env("gpt-5.3-codex".into(), Provider::OpenAi) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("implement"); @@ -9622,7 +9622,7 @@ async fn cli_backend_run_fails_on_nonzero_exit() { } let failing_env: Arc = Arc::new(FailingCliEnv); - let backend = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("step"); let context = Context::new(); @@ -9660,7 +9660,7 @@ async fn cli_backend_run_fails_on_nonzero_exit() { #[tokio::test] async fn cli_backend_run_fails_on_unparseable_output() { let env: Arc = Arc::new(CliTestEnv::new("this is not json at all")); - let backend = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("step"); @@ -9688,7 +9688,7 @@ async fn cli_backend_run_uses_node_model_override() { r#"{"type":"result","result":"ok","usage":{"input_tokens":10,"output_tokens":5}}"#; let test_env = Arc::new(CliTestEnv::new(claude_output)); let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new("default-model".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("default-model".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let mut node = Node::new("step"); @@ -9725,7 +9725,7 @@ async fn cli_backend_run_uses_node_provider_override() { let codex_output = "{\"type\":\"item.completed\",\"item\":{\"id\":\"item_0\",\"type\":\"agent_message\",\"text\":\"ok\"}}\n{\"type\":\"turn.completed\",\"usage\":{\"input_tokens\":10,\"output_tokens\":5}}"; let test_env = Arc::new(CliTestEnv::new(codex_output)); let env: Arc = test_env.clone(); - let backend = AgentCliBackend::new("default-model".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("default-model".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let mut node = Node::new("step"); @@ -9759,7 +9759,7 @@ async fn cli_backend_run_returns_text_and_usage() { let claude_output = r#"{"type":"result","result":"done","usage":{"input_tokens":10,"output_tokens":5}}"#; let env: Arc = Arc::new(CliTestEnv::new(claude_output)); - let backend = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let backend = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let node = Node::new("step"); @@ -9791,7 +9791,7 @@ async fn backend_router_delegates_to_cli_for_cli_node() { let env: Arc = Arc::new(CliTestEnv::new(claude_output)); let api_backend = Box::new(MockCodergenBackend); // would return "Response for ..." - let cli = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let router = BackendRouter::new(api_backend, cli); @@ -9827,7 +9827,7 @@ async fn backend_router_delegates_to_api_for_normal_node() { let env = local_env(); let api_backend = Box::new(MockCodergenBackend); - let cli = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let router = BackendRouter::new(api_backend, cli); @@ -9862,7 +9862,7 @@ async fn backend_router_delegates_to_cli_for_backend_attr() { let env: Arc = Arc::new(CliTestEnv::new(codex_output)); let api_backend = Box::new(MockCodergenBackend); - let cli = AgentCliBackend::new("gpt-5.3-codex".into(), Provider::OpenAi) + let cli = AgentCliBackend::new_from_env("gpt-5.3-codex".into(), Provider::OpenAi) .with_poll_interval(Duration::from_millis(10)); let router = BackendRouter::new(api_backend, cli); @@ -9947,7 +9947,7 @@ async fn full_pipeline_with_cli_backend_node() { // Build engine with BackendRouter let api = MockCodergenBackend; - let cli = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let router = BackendRouter::new(Box::new(api), cli); let codergen_handler = AgentHandler::new(Some(Box::new(router))); @@ -9960,7 +9960,7 @@ async fn full_pipeline_with_cli_backend_node() { Box::new(AgentHandler::new(Some(Box::new({ // Second BackendRouter for the "agent" handler let api2 = MockCodergenBackend; - let cli2 = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli2 = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); BackendRouter::new(Box::new(api2), cli2) })))), @@ -10068,7 +10068,7 @@ async fn stylesheet_backend_property_routes_to_cli() { // Run the pipeline let api = MockCodergenBackend; - let cli = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let router = BackendRouter::new(Box::new(api), cli); @@ -10076,7 +10076,7 @@ async fn stylesheet_backend_property_routes_to_cli() { registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); let api2 = MockCodergenBackend; - let cli2 = AgentCliBackend::new("claude-opus-4-6".into(), Provider::Anthropic) + let cli2 = AgentCliBackend::new_from_env("claude-opus-4-6".into(), Provider::Anthropic) .with_poll_interval(Duration::from_millis(10)); let router2 = BackendRouter::new(Box::new(api2), cli2); registry.register( diff --git a/lib/packages/fabro-api-client/src/models/secret-type.ts b/lib/packages/fabro-api-client/src/models/secret-type.ts index caa89fdf4..b85844891 100644 --- a/lib/packages/fabro-api-client/src/models/secret-type.ts +++ b/lib/packages/fabro-api-client/src/models/secret-type.ts @@ -20,7 +20,8 @@ export const SecretType = { ENVIRONMENT: 'environment', - FILE: 'file' + FILE: 'file', + CREDENTIAL: 'credential' } as const; export type SecretType = typeof SecretType[keyof typeof SecretType];