fix(#3394): distinguish the caller's bad token from the bridge failing to validate
swarm-authelia-bridge answered one 401 for four causes, including its own client credentials being rejected by authelia — telling a caller its token was invalid when the fault was ours. That cost a real diagnostic round-trip, because the only thing separating the two was a log line inside a container. Introspection now returns a three-way Verdict, and one classify() maps it to either admission, a 401 (the caller's credential) or a 503 (ours). Admission is unchanged: Ok is reachable from exactly one variant.
This commit is contained in:
parent
07d3d3060e
commit
266e8ac96b
2 changed files with 212 additions and 40 deletions
|
|
@ -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<BridgeResponse> {
|
||||
|
|
@ -265,3 +323,65 @@ async fn generate_password(bin: &std::path::Path) -> Result<String> {
|
|||
)
|
||||
})
|
||||
}
|
||||
|
||||
#[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:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue