From d82d167f07418bd0920d9e4967b1f7b59d72e58f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 9 Apr 2026 18:38:22 -0400 Subject: [PATCH] refactor(settings): stage 6.6g rewrite auth resolver for v2 Replaces the `build_legacy_api_settings` + `resolve_auth_mode_with_lookup(&ApiSettings, &[String], lookup)` shim path with a direct `resolve_auth_mode_with_lookup(&SettingsFile, lookup)` that walks the v2 `server.auth.api.{jwt,mtls}` and `server.auth.web.allowed_usernames` subtrees directly: - Each strategy subtree is considered enabled when present unless `enabled = false` is explicit (R52). - `allowed_usernames` is read from `server.auth.web.allowed_usernames` instead of a separate caller-supplied `&[String]` slice. - The FABRO_LOCAL_NO_AUTH escape hatch and "no strategies configured; rejecting everything" warnings are preserved. Deletes the `ApiAuthStrategy` and `ApiSettings` transitional shim types from `fabro-server/src/jwt_auth.rs`. `TlsSettings` survives (it's the resolved `(cert, key, ca)` triple that `tls.rs`'s rustls builder still consumes), with a new `TlsSettings::from_settings(&SettingsFile)` constructor that projects `server.listen.tls` into the runtime shape. `serve.rs` drops its `build_legacy_api_settings` helper entirely (~60 LOC). The serve bootstrap now calls `resolve_auth_mode_with_lookup(&cfg_file, ...)` directly and uses `TlsSettings::from_settings(&cfg_file)` for the TCP-vs-Unix branch. The `build_legacy_api_settings` TODO-2 from handoff-2 is resolved. TlsSettings uses `is_some_and` instead of `map_or(false, ...)` to satisfy the clippy `unnecessary_map_or` lint. All 3,758 workspace tests pass. `cargo fmt --check --all` and `cargo clippy --workspace -- -D warnings` are clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-server/src/jwt_auth.rs | 173 +++++++++++++++--------- lib/crates/fabro-server/src/serve.rs | 80 +---------- 2 files changed, 111 insertions(+), 142 deletions(-) diff --git a/lib/crates/fabro-server/src/jwt_auth.rs b/lib/crates/fabro-server/src/jwt_auth.rs index f1589f311..a4f12e266 100644 --- a/lib/crates/fabro-server/src/jwt_auth.rs +++ b/lib/crates/fabro-server/src/jwt_auth.rs @@ -6,42 +6,45 @@ use axum::http::request::Parts; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use jsonwebtoken::{Algorithm, DecodingKey, Validation}; use rustls_pki_types::CertificateDer; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use tracing::warn; use crate::error::ApiError; use crate::web_auth::SessionCookie; use fabro_types::RunAuthMethod; +use fabro_types::settings::SettingsFile; +use fabro_types::settings::interp::InterpString; +use fabro_types::settings::server::ServerListenLayer; -/// Authentication strategy flag consumed by `resolve_auth_mode_with_lookup`. -/// -/// Projected out of the v2 `server.auth.api.{jwt,mtls}` subtree by -/// `serve::build_legacy_api_settings`. Stage 6.6g will delete this shim -/// and walk the v2 tree directly in the auth resolver. -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] -#[serde(rename_all = "snake_case")] -pub enum ApiAuthStrategy { - Jwt, - Mtls, -} - -/// mTLS material loaded from `[server.listen.tls]`. Consumed by the -/// `tls.rs` rustls config builder. -#[derive(Debug, Clone, Deserialize, PartialEq, Serialize)] +/// Resolved TLS material used by the rustls config builder in `tls.rs` +/// when the server is listening on TCP with `[server.listen.tls]` set. +#[derive(Debug, Clone, PartialEq)] pub struct TlsSettings { pub cert: PathBuf, pub key: PathBuf, pub ca: PathBuf, } -/// Shim `ApiSettings` that `serve::build_legacy_api_settings` projects out -/// of the v2 tree so the pre-v2 [`resolve_auth_mode_with_lookup`] signature -/// keeps working until Stage 6.6g rewrites it. -#[derive(Debug, Clone, Default, Deserialize, PartialEq, Serialize)] -pub struct ApiSettings { - #[serde(default)] - pub authentication_strategies: Vec, - pub tls: Option, +impl TlsSettings { + /// Extract the `[server.listen.tls]` subtree out of a `SettingsFile`. + /// Returns `None` when the server is on Unix sockets, TLS is unset, or + /// any of the three fields is missing. + #[must_use] + pub fn from_settings(file: &SettingsFile) -> Option { + let listen = file.server.as_ref()?.listen.as_ref()?; + let tls = match listen { + ServerListenLayer::Tcp { tls, .. } => tls.as_ref()?, + ServerListenLayer::Unix { .. } => return None, + }; + let cert = tls.cert.as_ref().map(InterpString::as_source)?; + let key = tls.key.as_ref().map(InterpString::as_source)?; + let ca = tls.ca.as_ref().map(InterpString::as_source)?; + Some(Self { + cert: cert.into(), + key: key.into(), + ca: ca.into(), + }) + } } /// JWT claims for service-to-service authentication. @@ -99,34 +102,74 @@ pub fn decode_pem_env(name: &str, value: &str) -> String { .unwrap_or_else(|e| panic!("{name} base64 decoded to invalid UTF-8: {e}")) } -/// Resolve the authentication mode from the API config section. +/// Resolve the authentication mode from a [`SettingsFile`]. /// /// Call this once at startup before serving requests. Panics if the -/// configuration is invalid (JWT strategy but no public key, or mTLS without TLS config). -pub fn resolve_auth_mode(api_settings: &ApiSettings, allowed_usernames: &[String]) -> AuthMode { - resolve_auth_mode_with_lookup(api_settings, allowed_usernames, |name| { - std::env::var(name).ok() - }) +/// configuration is invalid (JWT strategy but no public key, or mTLS without +/// TLS config). Walks the v2 `server.auth.api.{jwt,mtls}` subtree and +/// `server.auth.web.allowed_usernames`. +pub fn resolve_auth_mode(settings: &SettingsFile) -> AuthMode { + resolve_auth_mode_with_lookup(settings, |name| std::env::var(name).ok()) } -pub fn resolve_auth_mode_with_lookup( - api_settings: &ApiSettings, - allowed_usernames: &[String], - lookup: F, -) -> AuthMode +/// Describes which API auth strategies are enabled in a `SettingsFile`. +struct ResolvedAuthStrategies { + jwt_enabled: bool, + mtls_enabled: bool, + tls_present: bool, + allowed_usernames: Vec, +} + +fn resolve_auth_strategies(settings: &SettingsFile) -> ResolvedAuthStrategies { + let server = settings.server.as_ref(); + let auth = server.and_then(|s| s.auth.as_ref()); + let auth_api = auth.and_then(|a| a.api.as_ref()); + + // Strategies: a subtree with `enabled = false` is explicitly off. + // Presence of the subtree with `enabled` unset counts as on. + let jwt_enabled = auth_api + .and_then(|api| api.jwt.as_ref()) + .is_some_and(|jwt| jwt.enabled.unwrap_or(true)); + let mtls_enabled = auth_api + .and_then(|api| api.mtls.as_ref()) + .is_some_and(|mtls| mtls.enabled.unwrap_or(true)); + + let tls_present = TlsSettings::from_settings(settings).is_some(); + + let allowed_usernames = auth + .and_then(|a| a.web.as_ref()) + .map(|w| w.allowed_usernames.clone()) + .unwrap_or_default(); + + ResolvedAuthStrategies { + jwt_enabled, + mtls_enabled, + tls_present, + allowed_usernames, + } +} + +pub fn resolve_auth_mode_with_lookup(settings: &SettingsFile, lookup: F) -> AuthMode where F: Fn(&str) -> Option, { - if api_settings.authentication_strategies.is_empty() - && std::env::var("FABRO_LOCAL_NO_AUTH").ok().as_deref() == Some("1") - { + let ResolvedAuthStrategies { + jwt_enabled, + mtls_enabled, + tls_present, + allowed_usernames, + } = resolve_auth_strategies(settings); + + let any_strategy = jwt_enabled || mtls_enabled; + + if !any_strategy && std::env::var("FABRO_LOCAL_NO_AUTH").ok().as_deref() == Some("1") { warn!( "No authentication strategies configured; allowing unauthenticated local daemon access" ); return AuthMode::Disabled; } - if api_settings.authentication_strategies.is_empty() { + if !any_strategy { warn!("No authentication strategies configured; all requests will be rejected"); } @@ -135,34 +178,30 @@ where strategies.push(AuthStrategy::Cookie); } - strategies.extend(api_settings - .authentication_strategies - .iter() - .map(|s| match s { - ApiAuthStrategy::Jwt => { - let raw = lookup("FABRO_JWT_PUBLIC_KEY").unwrap_or_else(|| { - panic!( - "FABRO_JWT_PUBLIC_KEY is not set. Provide an Ed25519 public key in PEM \ - format (or base64-encoded PEM) for JWT authentication." - ) - }); - let pem = decode_pem_env("FABRO_JWT_PUBLIC_KEY", &raw); - let key = DecodingKey::from_ed_pem(pem.as_bytes()) - .expect("FABRO_JWT_PUBLIC_KEY contains an invalid Ed25519 PEM public key"); - AuthStrategy::Jwt { - key: Arc::new(key), - validation: Arc::new(jwt_validation()), - allowed_usernames: allowed_usernames.to_vec(), - } - } - ApiAuthStrategy::Mtls => { - assert!( - api_settings.tls.is_some(), - "mTLS authentication strategy requires [api.tls] configuration with cert, key, and ca" - ); - AuthStrategy::Mtls - } - })); + if jwt_enabled { + let raw = lookup("FABRO_JWT_PUBLIC_KEY").unwrap_or_else(|| { + panic!( + "FABRO_JWT_PUBLIC_KEY is not set. Provide an Ed25519 public key in PEM format \ + (or base64-encoded PEM) for JWT authentication." + ) + }); + let pem = decode_pem_env("FABRO_JWT_PUBLIC_KEY", &raw); + let key = DecodingKey::from_ed_pem(pem.as_bytes()) + .expect("FABRO_JWT_PUBLIC_KEY contains an invalid Ed25519 PEM public key"); + strategies.push(AuthStrategy::Jwt { + key: Arc::new(key), + validation: Arc::new(jwt_validation()), + allowed_usernames: allowed_usernames.clone(), + }); + } + + if mtls_enabled { + assert!( + tls_present, + "mTLS authentication strategy requires [server.listen.tls] configuration with cert, key, and ca" + ); + strategies.push(AuthStrategy::Mtls); + } AuthMode::Strategies(strategies) } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 65904f1e1..c05547e36 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -2,7 +2,6 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock}; use std::time::Duration; -use crate::jwt_auth::ApiSettings; use fabro_config::Storage; use fabro_config::resolve_storage_dir; use fabro_config::user::{active_settings_path, load_settings_config}; @@ -22,7 +21,7 @@ use fabro_types::settings::v2::SettingsFile; use crate::bind::{self, Bind, BindRequest}; use crate::github_webhooks::WebhookManager; -use crate::jwt_auth::{AuthMode, AuthStrategy, resolve_auth_mode_with_lookup}; +use crate::jwt_auth::{AuthMode, AuthStrategy, TlsSettings, resolve_auth_mode_with_lookup}; use crate::secret_store::SecretStore; use crate::server::{ RouterOptions, build_app_state_with_path, build_router_with_options, @@ -85,65 +84,6 @@ fn load_settings(path: Option<&Path>) -> anyhow::Result { Ok(load_settings_config(path)?.into()) } -/// Build the legacy `ApiSettings` shape that `resolve_auth_mode_with_lookup` -/// and the TLS branch still expect, extracting the pieces it needs from the -/// v2 tree. Stage 6.6 replaces this with a v2-aware auth resolver and drops -/// the legacy `ApiSettings` type entirely. -fn build_legacy_api_settings(file: &SettingsFile) -> ApiSettings { - use crate::jwt_auth::{ApiAuthStrategy, TlsSettings}; - use fabro_types::settings::v2::interp::InterpString; - use fabro_types::settings::v2::server::ServerListenLayer; - - let auth_api = file - .server - .as_ref() - .and_then(|s| s.auth.as_ref()) - .and_then(|a| a.api.as_ref()); - - let mut authentication_strategies = Vec::new(); - if auth_api - .and_then(|api| api.jwt.as_ref()) - .and_then(|jwt| jwt.enabled) - .unwrap_or(auth_api.and_then(|api| api.jwt.as_ref()).is_some()) - { - authentication_strategies.push(ApiAuthStrategy::Jwt); - } - if auth_api - .and_then(|api| api.mtls.as_ref()) - .and_then(|mtls| mtls.enabled) - .unwrap_or(auth_api.and_then(|api| api.mtls.as_ref()).is_some()) - { - authentication_strategies.push(ApiAuthStrategy::Mtls); - } - - // TLS files now live under `server.listen.tls.{cert,key,ca}` in v2. - // Build a legacy TlsSettings from the listen TLS subtree so the - // existing rustls config path keeps working. - let tls = file - .server - .as_ref() - .and_then(|s| s.listen.as_ref()) - .and_then(|listen| match listen { - ServerListenLayer::Tcp { tls, .. } => tls.as_ref(), - ServerListenLayer::Unix { .. } => None, - }) - .and_then(|tls_layer| { - let cert = tls_layer.cert.as_ref().map(InterpString::as_source)?; - let key = tls_layer.key.as_ref().map(InterpString::as_source)?; - let ca = tls_layer.ca.as_ref().map(InterpString::as_source)?; - Some(TlsSettings { - cert: cert.into(), - key: key.into(), - ca: ca.into(), - }) - }); - - ApiSettings { - authentication_strategies, - tls, - } -} - fn resolved_config_path(path: Option<&Path>) -> PathBuf { active_settings_path(path) } @@ -347,24 +287,14 @@ where std::fs::create_dir_all(&data_dir)?; let (auth_mode, client_auth, max_concurrent_runs) = { let cfg_file = shared_settings.read().expect("config lock poisoned"); - // Build the legacy ApiSettings + allowed_usernames shapes that the - // v1 auth resolver expects. Stage 6.6 replaces this with a direct - // v2-aware resolver. - let api = build_legacy_api_settings(&cfg_file); - let allowed_usernames = cfg_file - .server - .as_ref() - .and_then(|s| s.auth.as_ref()) - .and_then(|a| a.web.as_ref()) - .map(|w| w.allowed_usernames.clone()) - .unwrap_or_default(); - let auth_mode = resolve_auth_mode_with_lookup(&api, &allowed_usernames, |name| { + let auth_mode = resolve_auth_mode_with_lookup(&cfg_file, |name| { secret_snapshot .get(name) .cloned() .or_else(|| std::env::var(name).ok()) }); - let client_auth = api.tls.as_ref().map(|_| client_auth_from_mode(&auth_mode)); + let tls_present = TlsSettings::from_settings(&cfg_file).is_some(); + let client_auth = tls_present.then(|| client_auth_from_mode(&auth_mode)); let max_concurrent_runs = args .max_concurrent_runs .or_else(|| cfg_file.max_concurrent_runs()) @@ -516,7 +446,7 @@ where // Branch: TLS, plain TCP, or Unix socket let tls_settings = { let cfg_file = shared_settings.read().expect("config lock poisoned"); - build_legacy_api_settings(&cfg_file).tls.clone() + TlsSettings::from_settings(&cfg_file) }; let bound_listener = bind_listener(&bind_request).await?;