diff --git a/swarm-nats-auth/src/introspect.rs b/swarm-nats-auth/src/introspect.rs index dfce9c0f..bd2b7d85 100644 --- a/swarm-nats-auth/src/introspect.rs +++ b/swarm-nats-auth/src/introspect.rs @@ -47,10 +47,11 @@ pub const INTROSPECTION_TIMEOUT: Duration = Duration::from_millis(1500); pub enum Answer { /// A 2xx: the caller, or `None` for every shape that is not an admission. Caller(Option), - /// A non-2xx: the `IdP` did not answer about the token at all, which is - /// also how it refuses this responder's own credential. `oidc_secret` - /// re-reads that credential and retries once before this denies. - Refused, + /// A non-2xx, with its status: the `IdP` did not answer about the token + /// at all. A 400 or 401 is how it refuses this responder's own + /// credential, which `oidc_secret` re-reads and retries once before this + /// denies. + Refused(reqwest::StatusCode), } /// The two fields this responder acts on — one per decision. Authelia returns @@ -92,7 +93,7 @@ impl IntrospectionResponse { /// Returns `Ok(Answer::Caller(Some(client_id)))` **only** on a 2xx whose /// body says `active: true` and names the client. Every other 2xx is /// `Ok(Answer::Caller(None))` with the reason logged, a non-2xx is -/// `Ok(Answer::Refused)`, and `Err` is a call that could not be made at all +/// `Ok(Answer::Refused(_))`, and `Err` is a call that could not be made at all /// or did not finish by `deadline` — callers must treat all but the first as /// a denial. /// @@ -123,7 +124,7 @@ pub async fn identify_caller( let status = resp.status(); if !status.is_success() { tracing::warn!(%status, "introspection returned non-2xx"); - return Ok(Answer::Refused); + return Ok(Answer::Refused(status)); } let body: IntrospectionResponse = match resp.json().await { Ok(b) => b, diff --git a/swarm-nats-auth/src/oidc_secret.rs b/swarm-nats-auth/src/oidc_secret.rs index 4dd4fa9a..2db27083 100644 --- a/swarm-nats-auth/src/oidc_secret.rs +++ b/swarm-nats-auth/src/oidc_secret.rs @@ -4,18 +4,19 @@ //! //! The secret can change under a running responder: authelia's host //! re-publishes it when it is re-minted. So when the `IdP` refuses an -//! introspection (a non-2xx), the secret is read from the store again -//! ([`StoredSecret::after_refusal`]) and, when it changed, the introspection -//! is tried once more. Both fit in the one request's +//! introspection as a credential (a 400 or 401), the secret is read from the +//! store again ([`StoredSecret::after_refusal`]) and, when it changed, the +//! introspection is tried once more. Both fit in the one request's //! [`INTROSPECTION_TIMEOUT`](crate::introspect::INTROSPECTION_TIMEOUT). A //! re-read that does not is left running: that request is denied, and the -//! next one introspects with what the re-read landed. +//! next one introspects with what the re-read landed. Any other non-2xx is +//! denied without a re-read. use std::future::Future; use std::sync::Arc; use anyhow::{Context, Result}; -use swarm_queue_client::stored_secret::StoredSecret; +use swarm_queue_client::stored_secret::{StoredSecret, is_credential_refusal}; use swarm_secret_client::client::{DEFAULT_CERT_MOUNT, Settings}; use swarm_secret_client::path::{Kind, principal_prefix}; use tokio::time::Instant; @@ -89,9 +90,10 @@ impl OidcSecret { } /// The caller `attempt` names, introspecting with the current secret. - /// When the `IdP` refuses that, the secret is re-read and, if it changed - /// and `deadline` has not passed, `attempt` runs once more with the new - /// one. `attempt` bounds itself by the same `deadline`. + /// When the `IdP` refuses that as a credential (a 400 or 401), the secret + /// is re-read and, if it changed and `deadline` has not passed, `attempt` + /// runs once more with the new one. Any other refusal is a denial. + /// `attempt` bounds itself by the same `deadline`. /// /// # Errors /// When an attempt could not be made at all; the caller denies. @@ -101,8 +103,10 @@ impl OidcSecret { Fut: Future>, { let used = self.secret.current(); - if let Answer::Caller(caller) = attempt(used.clone()).await? { - return Ok(caller); + match attempt(used.clone()).await? { + Answer::Caller(caller) => return Ok(caller), + Answer::Refused(status) if !is_credential_refusal(status) => return Ok(None), + Answer::Refused(_) => {} } let Ok(fresh) = tokio::time::timeout_at(deadline, self.secret.after_refusal(&used)).await else { @@ -117,7 +121,7 @@ impl OidcSecret { }; match attempt(fresh).await? { Answer::Caller(caller) => Ok(caller), - Answer::Refused => Ok(None), + Answer::Refused(_) => Ok(None), } } } @@ -161,7 +165,7 @@ mod tests { std::future::ready(Ok(if secret == registered { Answer::Caller(Some("hive-alpha".to_owned())) } else { - Answer::Refused + Answer::Refused(reqwest::StatusCode::UNAUTHORIZED) })) } } @@ -243,6 +247,41 @@ mod tests { assert_eq!(calls.load(Ordering::SeqCst), 1); } + /// An `IdP` outage answers 503s and 429s with the new secret already in + /// the store: none of them reads it, so none pushes the re-read back, and + /// the first 401 once the `IdP` is back re-reads and is admitted. + #[tokio::test] + async fn only_a_credential_refusal_re_reads_the_secret() { + use reqwest::StatusCode; + + let (secret, reads) = secret("old", "new", Duration::ZERO); + let calls = AtomicUsize::new(0); + for status in [ + StatusCode::SERVICE_UNAVAILABLE, + StatusCode::BAD_GATEWAY, + StatusCode::TOO_MANY_REQUESTS, + ] { + let caller = secret + .introspect(budget(), |_| { + calls.fetch_add(1, Ordering::SeqCst); + std::future::ready(Ok(Answer::Refused(status))) + }) + .await + .expect("answered"); + assert_eq!(caller, None, "{status}"); + } + assert_eq!(calls.load(Ordering::SeqCst), 3); + assert_eq!(reads.load(Ordering::SeqCst), 0); + + let caller = secret + .introspect(budget(), idp("new", &calls)) + .await + .expect("answered"); + assert_eq!(caller.as_deref(), Some("hive-alpha")); + assert_eq!(calls.load(Ordering::SeqCst), 5); + assert_eq!(reads.load(Ordering::SeqCst), 1); + } + /// A store slower than the budget: the request is denied at its /// deadline, the re-read finishes behind it, and the next request is /// admitted on its first attempt without reading again. diff --git a/swarm-queue-client/README.md b/swarm-queue-client/README.md index 15cc1b69..0d4708b1 100644 --- a/swarm-queue-client/README.md +++ b/swarm-queue-client/README.md @@ -44,9 +44,10 @@ The client secret is never in the environment, which would publish it to anything that can read `/proc//environ`. A caller passes it either as a **path** (`ClientSecret::File`), read per token request rather than cached, so a rotation the operator believes took effect actually did, or as a value it -fetched itself (`ClientSecret::Value`), held in memory. `swarm-controller` and -`hive-c0re` both do the latter with the secret they read from the swarm's -secret store. +fetched itself (`ClientSecret::Stored`), held in memory and fetched again +through the caller's callback when the token endpoint refuses it as a +credential. `swarm-controller`, `hive-c0re` and `swarm-review-classifier` do the +latter with the secret they read from the swarm's secret store. ## Token request shape: HTTP Basic, `audience`, `scope` diff --git a/swarm-queue-client/src/lib.rs b/swarm-queue-client/src/lib.rs index 7ba6f233..849c85b3 100644 --- a/swarm-queue-client/src/lib.rs +++ b/swarm-queue-client/src/lib.rs @@ -505,15 +505,11 @@ pub enum ClientSecret { impl ClientSecret { /// The secret to read again after `error`: a held one the token endpoint - /// refused as a credential (400 or 401). A 429 or a 5xx says nothing - /// about the secret. + /// refused as a credential ([`stored_secret::is_credential_refusal`]). fn refused(&self, error: &Error) -> Option<&stored_secret::StoredSecret> { match (self, error) { (Self::Stored(stored), Error::TokenRefused { status, .. }) - if matches!( - *status, - reqwest::StatusCode::BAD_REQUEST | reqwest::StatusCode::UNAUTHORIZED - ) => + if stored_secret::is_credential_refusal(*status) => { Some(stored) } @@ -1159,22 +1155,37 @@ mod tests { /// A refusal that is not about the credential reads nothing. #[tokio::test] - async fn a_rate_limited_mint_does_not_re_read() { - let store = stored_secret::tests::Fake::holding("secret-b"); - let cfg = QueueConfig { - client_secret: ClientSecret::Stored(store.secret("hive-alpha", "secret-a")), - ..token_cfg() - }; - let limited = Error::TokenRefused { - status: reqwest::StatusCode::TOO_MANY_REQUESTS, - body: String::new(), - }; - assert!(cfg.client_secret.refused(&limited).is_none()); - let refused = Error::TokenRefused { - status: reqwest::StatusCode::UNAUTHORIZED, - body: String::new(), - }; - assert!(cfg.client_secret.refused(&refused).is_some()); + async fn a_rate_limited_or_failing_mint_does_not_re_read() { + use axum::http::StatusCode; + + let http = loopback_client(); + for status in [ + StatusCode::TOO_MANY_REQUESTS, + StatusCode::SERVICE_UNAVAILABLE, + ] { + let app = axum::Router::new().fallback(move || async move { (status, "") }); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind"); + let url = format!("http://{}/token", listener.local_addr().expect("addr")); + tokio::spawn(async move { axum::serve(listener, app).await }); + + let store = stored_secret::tests::Fake::holding("secret-b"); + let cfg = QueueConfig { + token_endpoint: url, + client_secret: ClientSecret::Stored(store.secret("hive-alpha", "secret-a")), + ..token_cfg() + }; + let err = mint_token(&http, &cfg, None, None) + .await + .err() + .expect("refused"); + assert!( + matches!(err, Error::TokenRefused { status: s, .. } if s == status.as_u16()), + "{err}" + ); + assert_eq!(store.reads(), 0, "{status}"); + } } /// The blocking mint re-reads the same way, from a thread with no diff --git a/swarm-queue-client/src/stored_secret.rs b/swarm-queue-client/src/stored_secret.rs index 878b0716..9e2320e4 100644 --- a/swarm-queue-client/src/stored_secret.rs +++ b/swarm-queue-client/src/stored_secret.rs @@ -205,6 +205,17 @@ impl Inner { } } +/// Whether an `IdP` answering `status` refused the client's credential (400 or +/// 401), the one answer a re-read can fix. A 429 or a 5xx says nothing about +/// the secret, and re-reading on one would spend the backoff on an outage. +#[must_use] +pub fn is_credential_refusal(status: reqwest::StatusCode) -> bool { + matches!( + status, + reqwest::StatusCode::BAD_REQUEST | reqwest::StatusCode::UNAUTHORIZED + ) +} + /// [`FIRST_REREAD_DELAY`] doubled per fruitless re-read after the first, /// capped at [`MAX_REREAD_DELAY`]. fn delay_after(fruitless: u32) -> Duration {