diff --git a/swarm-authelia-bridge/src/introspect.rs b/swarm-authelia-bridge/src/introspect.rs index 575e28d9..e12be0a8 100644 --- a/swarm-authelia-bridge/src/introspect.rs +++ b/swarm-authelia-bridge/src/introspect.rs @@ -9,6 +9,15 @@ //! HTTP call are exactly the conditions an attacker would like this to //! fall open under. //! +//! [`Verdict`] has three variants where admission has two: the extra one +//! carries **whose fault a denial was**, a different question from who is +//! admitted (`main`'s `classify` is the single place that answers the +//! latter). Collapsing them answered `401 missing or invalid bearer token` +//! when *this bridge's own* credentials were the problem — a false +//! accusation pointing away from the fault, in the case most likely right +//! after a config change. `Unavailable` is **not a softer denial**: it +//! denies exactly as hard and only changes the report. +//! //! Authenticates the introspection call itself with **this bridge's own** //! client credentials (a resource server introspecting a token proves its //! own identity to the `IdP`, per RFC 7662) — a separate authelia machine @@ -16,7 +25,6 @@ use std::time::Duration; -use anyhow::{Context, Result}; use serde::Deserialize; /// Generous relative to `swarm-nats-auth`'s 1.5s (that one is bounded by @@ -30,39 +38,71 @@ struct IntrospectionResponse { active: bool, } -/// Ask authelia whether `token` is currently valid. `Ok(true)` only on a -/// 2xx body with `active: true`; every other outcome is `Ok(false)` -/// (logged) or `Err` when the call could not be made at all — callers -/// must treat both as a denial. -pub async fn is_active( +/// The outcome of asking authelia about a token. +/// +/// Total by construction — there is no `Result` wrapper, because every way +/// the call can go wrong is already a variant. A caller that forgets a case +/// does not compile, and there is no error path that could be `?`-ed past +/// into an accidental admission. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Verdict { + /// 2xx with `active: true`. The only admitting outcome. + Active, + /// 2xx with `active: false` — authelia evaluated the token and said no. + /// The **caller's** credential is the problem. + Inactive, + /// The question could not be answered: the request failed, authelia + /// answered non-2xx (which for RFC 7662 means *the introspection call + /// itself* was rejected — an invalid token is a 200 with + /// `active: false`), or the body did not parse. In every case the fault + /// is on **this bridge's** side of the exchange, not the caller's. + Unavailable, +} + +/// Ask authelia whether `token` is currently valid. +/// +/// Never fails: see [`Verdict`]. Each non-admitting outcome is logged at +/// `warn` with the detail that distinguishes it, since the response the +/// caller gets is deliberately vague. +pub async fn introspect( http: &reqwest::Client, url: &str, client_id: &str, client_secret: &str, token: &str, -) -> Result { - let resp = http +) -> Verdict { + let resp = match http .post(url) .basic_auth(client_id, Some(client_secret)) .form(&[("token", token)]) .timeout(INTROSPECTION_TIMEOUT) .send() .await - .context("introspection request")?; + { + Ok(resp) => resp, + Err(e) => { + tracing::warn!(error = ?e, "introspection request failed; cannot validate"); + return Verdict::Unavailable; + } + }; let status = resp.status(); if !status.is_success() { - tracing::warn!(%status, "introspection returned non-2xx; denying"); - return Ok(false); + // Most often: this bridge's own client credentials are being + // rejected by authelia. Logged with the status because that is the + // one thing separating "my secret is stale" from "authelia is + // unwell", and neither is the caller's business. + tracing::warn!(%status, "introspection returned non-2xx; cannot validate"); + return Verdict::Unavailable; } - let body: IntrospectionResponse = match resp.json().await { - Ok(b) => b, + match resp.json::().await { + Ok(body) if body.active => Verdict::Active, + Ok(_) => Verdict::Inactive, Err(e) => { - tracing::warn!(error = ?e, "introspection body did not parse; denying"); - return Ok(false); + tracing::warn!(error = ?e, "introspection body did not parse; cannot validate"); + Verdict::Unavailable } - }; - Ok(body.active) + } } #[cfg(test)] @@ -77,4 +117,16 @@ mod tests { assert!(!no.active); assert!(serde_json::from_str::(r#"{"sub":"someone"}"#).is_err()); } + + /// A 2xx body whose `active` is false is a *verdict*, not an error — + /// the distinction `Verdict` exists to preserve, asserted at the one + /// place the two are told apart from the wire. + #[test] + fn an_inactive_token_is_a_verdict_not_a_failure() { + let body: IntrospectionResponse = serde_json::from_str(r#"{"active":false}"#).unwrap(); + assert!( + !body.active, + "authelia answering `active: false` is authelia working, not authelia failing" + ); + } } diff --git a/swarm-authelia-bridge/src/main.rs b/swarm-authelia-bridge/src/main.rs index 82029a25..da061911 100644 --- a/swarm-authelia-bridge/src/main.rs +++ b/swarm-authelia-bridge/src/main.rs @@ -156,44 +156,102 @@ async fn handle_request( } /// Extract + introspect the bearer token. `Err` carries the response to -/// return outright (401 for anything short of an active token) — kept -/// separate from `handle`'s error path, which is "the op itself failed," -/// a different case from "the caller was never let in." +/// return outright — kept separate from `handle`'s error path, which is +/// "the op itself failed," a different case from "the caller was never let +/// in." +/// +/// Anything short of an active token is refused; **which status** depends +/// on whose credential failed, see [`Refusal`]. async fn authorize(state: &AppState, headers: &HeaderMap) -> Result<(), Response> { - let deny = || { - ( - StatusCode::UNAUTHORIZED, - Json(BridgeResponse::Error { - message: "missing or invalid bearer token".to_owned(), - }), - ) - .into_response() - }; let Some(auth) = headers.get(axum::http::header::AUTHORIZATION) else { - return Err(deny()); + return Err(refuse(Refusal::Caller)); }; let Ok(auth) = auth.to_str() else { - return Err(deny()); + return Err(refuse(Refusal::Caller)); }; let Some(token) = auth.strip_prefix("Bearer ") else { - return Err(deny()); + return Err(refuse(Refusal::Caller)); }; - match introspect::is_active( + let verdict = introspect::introspect( &state.http, &state.config.introspection_url, &state.config.client_id, &state.config.client_secret, token, ) - .await - { - Ok(true) => Ok(()), - Ok(false) => Err(deny()), - Err(e) => { - tracing::warn!(error = %format!("{e:#}"), "introspection call failed; denying"); - Err(deny()) + .await; + classify(verdict).map_err(refuse) +} + +/// The admission decision, and the only place that makes it. +/// +/// Fail-closed: `Ok(())` is reachable from exactly one variant. Split out +/// of [`authorize`] so the rule is testable without an authelia — and it is +/// the *real* rule, not a restatement of it, because this is the function +/// the request path calls. +fn classify(verdict: introspect::Verdict) -> Result<(), Refusal> { + match verdict { + introspect::Verdict::Active => Ok(()), + introspect::Verdict::Inactive => Err(Refusal::Caller), + introspect::Verdict::Unavailable => Err(Refusal::Bridge), + } +} + +/// Whose credential was at fault when a request was refused. +/// +/// The distinction is the whole point: these used to be one 401, so a +/// caller whose token was fine was told its token was invalid whenever +/// *this bridge* could not authenticate itself to authelia. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Refusal { + /// No token, an unusable header, or a token authelia rejected. + Caller, + /// This bridge could not get an answer out of authelia at all, so the + /// request was never evaluated. + Bridge, +} + +impl Refusal { + /// `401` for the caller; `503` for us. + /// + /// 401 would be wrong for [`Refusal::Bridge`] under RFC 7235 — nothing + /// about the caller's credential was determined — and it is also the + /// least useful thing to say, because a client that believes its token + /// is bad will go and mint a new one, which cannot help. + /// + /// 503 rather than 500 because the overwhelmingly common cause is + /// transient by construction: this bridge's own secret is minted on + /// authelia's first boot and delivered by a separate unit, so the + /// window where it is absent closes on its own. A client retrying is + /// doing the right thing. + fn status(self) -> StatusCode { + match self { + Refusal::Caller => StatusCode::UNAUTHORIZED, + Refusal::Bridge => StatusCode::SERVICE_UNAVAILABLE, } } + + /// Deliberately coarse for [`Refusal::Caller`]: an unauthenticated + /// caller learns that it was refused, never which of the four ways. + /// The `Bridge` message says only that validation could not happen — + /// no status code, no upstream detail, nothing about the secret. The + /// discriminating detail is in this bridge's journal, where it belongs. + fn message(self) -> &'static str { + match self { + Refusal::Caller => "missing or invalid bearer token", + Refusal::Bridge => "cannot validate credentials", + } + } +} + +fn refuse(refusal: Refusal) -> Response { + ( + refusal.status(), + Json(BridgeResponse::Error { + message: refusal.message().to_owned(), + }), + ) + .into_response() } async fn handle(state: &AppState, name: String) -> Result { @@ -265,3 +323,65 @@ async fn generate_password(bin: &std::path::Path) -> Result { ) }) } + +#[cfg(test)] +mod tests { + use super::{Refusal, StatusCode, classify, introspect::Verdict}; + + /// Fail-closed, asserted per variant rather than as one negation: + /// splitting blame out of a single denial is exactly the change that + /// could have widened admission, so each refusing variant is named on + /// its own line. + #[test] + fn only_an_active_token_is_admitted() { + assert_eq!(classify(Verdict::Active), Ok(())); + assert_eq!( + classify(Verdict::Inactive), + Err(Refusal::Caller), + "authelia evaluated the token and said no" + ); + assert_eq!( + classify(Verdict::Unavailable), + Err(Refusal::Bridge), + "a bridge that cannot ask must not admit — this variant reports a \ + different fault, it is not a softer denial" + ); + } + + /// The whole point of the split, asserted on the value rather than + /// on message text — the previous code picked its status by matching on + /// a log message's wording, which is what let the two collapse. + #[test] + fn a_refusal_names_whose_credential_failed() { + assert_eq!( + Refusal::Caller.status(), + StatusCode::UNAUTHORIZED, + "the caller's token is the problem; retrying with the same one will not help" + ); + assert_eq!( + Refusal::Bridge.status(), + StatusCode::SERVICE_UNAVAILABLE, + "the request was never evaluated — 401 here accuses the caller of our fault" + ); + assert_ne!( + Refusal::Caller.status(), + Refusal::Bridge.status(), + "collapsing these back into one status is the bug this exists to prevent" + ); + } + + /// No response body may leak which of the four caller-side causes + /// applied, nor anything about this bridge's own credential. + #[test] + fn refusal_messages_say_nothing_useful_to_an_attacker() { + for refusal in [Refusal::Caller, Refusal::Bridge] { + let message = refusal.message(); + for leak in ["secret", "client_id", "authelia", "http", "status"] { + assert!( + !message.to_ascii_lowercase().contains(leak), + "{refusal:?} message leaks {leak:?}: {message:?}" + ); + } + } + } +}