diff --git a/src/config.rs b/src/config.rs index 7aca4a7..5385028 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,8 +18,9 @@ pub struct OidcConfig { pub client_id: String, /// OIDC client secret. May be set in config.toml or via the /// `OIDC_CLIENT_SECRET` environment variable; the env var wins when - /// both are present. Validated at startup to be non-empty when - /// `[oidc]` is configured (see Config::load). + /// both are present. `serve` validates it is non-empty when `[oidc]` + /// is configured (see Config::validate_oidc_secret); admin CLI + /// subcommands do not require it. #[serde(default)] pub client_secret: Option, pub redirect_uri: String, @@ -1229,36 +1230,50 @@ impl Config { // OIDC client_secret resolution. The env var wins over whatever // is in the config file, and is the documented way to keep the - // secret out of TOML on disk. Either source is fine, but at least - // one must produce a non-empty value when `[oidc]` is present; - // bug #121 was that the field was non-Optional in serde, so - // omitting it from config.toml failed parsing before the env var - // could fill it in. + // secret out of TOML on disk. Only the override happens here: + // whether a secret is *required* depends on the subcommand, so + // the presence check lives in `validate_oidc_secret` and is run + // by `serve` alone. Admin CLI subcommands (add-admin, map-group, + // ...) only touch the SQLite database and must work from an + // interactive shell where the systemd EnvironmentFile has not + // been sourced. if let Some(ref mut oidc) = config.oidc { if let Ok(secret) = std::env::var("OIDC_CLIENT_SECRET") { if !secret.is_empty() { oidc.client_secret = Some(secret); } } - let has_secret = oidc - .client_secret - .as_ref() - .map(|s| !s.is_empty()) - .unwrap_or(false); - if !has_secret { - eprintln!( - "[config] ERROR: [oidc] is configured but no client_secret was provided.\n\ - [config] Set `client_secret = \"...\"` in config.toml, or export\n\ - [config] OIDC_CLIENT_SECRET in the rustguac environment\n\ - [config] (e.g. /opt/rustguac/env)." - ); - std::process::exit(1); - } } config } + /// Check that `[oidc]`, when present, has a usable client secret from + /// either config.toml or `OIDC_CLIENT_SECRET` (already merged by + /// `Config::load`). Returns the operator-facing error text on failure. + /// Only the server needs this; see the note in `Config::load`. Bug #121 + /// was that the field was non-Optional in serde, so omitting it from + /// config.toml failed parsing before the env var could fill it in. + pub fn validate_oidc_secret(&self) -> Result<(), String> { + let Some(ref oidc) = self.oidc else { + return Ok(()); + }; + let has_secret = oidc + .client_secret + .as_ref() + .map(|s| !s.is_empty()) + .unwrap_or(false); + if has_secret { + Ok(()) + } else { + Err("[oidc] is configured but no client_secret was provided.\n\ + [config] Set `client_secret = \"...\"` in config.toml, or export\n\ + [config] OIDC_CLIENT_SECRET in the rustguac environment\n\ + [config] (e.g. /opt/rustguac/env)." + .to_string()) + } + } + /// Effective recording path: `[recording].path` overrides top-level `recording_path`. pub fn effective_recording_path(&self) -> &std::path::Path { if let Some(ref rec) = self.recording { @@ -1446,6 +1461,71 @@ mod tests { assert_eq!(config.client_secret.as_deref(), Some("from-config")); } + #[test] + fn validate_oidc_secret_passes_without_oidc_section() { + let config = Config::default(); + assert!(config.oidc.is_none()); + assert!(config.validate_oidc_secret().is_ok()); + } + + #[test] + fn validate_oidc_secret_rejects_missing_secret() { + // The [oidc] block parses fine without a secret (see #121); the + // presence check is a separate, serve-only step so that admin CLI + // subcommands run from a shell that hasn't sourced /opt/rustguac/env. + let oidc: OidcConfig = toml::from_str( + r#" + issuer_url = "https://idp.example.com/" + client_id = "rustguac" + redirect_uri = "https://console.example.com/oidc/callback" + "#, + ) + .unwrap(); + let config = Config { + oidc: Some(oidc), + ..Config::default() + }; + let err = config.validate_oidc_secret().unwrap_err(); + assert!(err.contains("no client_secret was provided"), "{err}"); + assert!(err.contains("OIDC_CLIENT_SECRET"), "{err}"); + } + + #[test] + fn validate_oidc_secret_rejects_empty_secret() { + let oidc: OidcConfig = toml::from_str( + r#" + issuer_url = "https://idp.example.com/" + client_id = "rustguac" + client_secret = "" + redirect_uri = "https://console.example.com/oidc/callback" + "#, + ) + .unwrap(); + let config = Config { + oidc: Some(oidc), + ..Config::default() + }; + assert!(config.validate_oidc_secret().is_err()); + } + + #[test] + fn validate_oidc_secret_accepts_config_secret() { + let oidc: OidcConfig = toml::from_str( + r#" + issuer_url = "https://idp.example.com/" + client_id = "rustguac" + client_secret = "from-config" + redirect_uri = "https://console.example.com/oidc/callback" + "#, + ) + .unwrap(); + let config = Config { + oidc: Some(oidc), + ..Config::default() + }; + assert!(config.validate_oidc_secret().is_ok()); + } + #[test] fn oidc_config_debug_redacts_client_secret() { let config = OidcConfig { diff --git a/src/main.rs b/src/main.rs index 6649c59..e07c99e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -217,7 +217,17 @@ async fn main() { let database = db::init_db(&config.db_path).expect("Failed to open database"); match cli.command { - None | Some(Command::Serve) => run_server(config, database).await, + None | Some(Command::Serve) => { + // Only the server talks to the IdP, so only the server insists on + // an OIDC client secret. Checked here rather than in Config::load + // so `add-admin` & co. work from a shell where /opt/rustguac/env + // (systemd's EnvironmentFile) has not been sourced. + if let Err(msg) = config.validate_oidc_secret() { + eprintln!("[config] ERROR: {}", msg); + std::process::exit(1); + } + run_server(config, database).await + } Some(Command::AddAdmin { name, allowed_ips,