swarm-queue-client: re-read a held OIDC client secret when the IdP refuses it
hive-c0re (`hive-<name>`), swarm-controller and swarm-review-classifier read their queue client secret from the store once and held it as `ClientSecret::Value` for the life of the process. After authelia re-minted a secret and swarm-secret-publish stored the new one, every later token mint presented the old value and was refused until the daemon restarted. `ClientSecret::Stored` replaces `Value`: a `StoredSecret` holds the secret in memory with a caller-supplied read callback, so the crate takes no store dependency. When the token endpoint answers 400 or 401, `mint_token` (and its blocking sibling, used by the controller's OTLP exporters) re-reads it and, if the store holds a different secret, mints once more with it. One re-read runs at a time and is spawned so it outlives a caller that gives up. A re-read that fails or finds the refused value delays the next one, 5 s doubling to 5 min, so a secret that stays wrong costs one store read per delay. Logs name the client id and never the secret. swarm-nats-auth's `oidc_secret` was the template; its re-read machinery now sits on `StoredSecret`, so the tree has one copy. The responder gains the same re-read backoff. The controller keeps its `IDENTITY` OnceLock: it now holds a `QueueConfig` whose secret is re-read, and the lazily built vcs-metrics exporter reads the identity from it. agent_renewal.rs's module doc said a re-minted agent queue secret only reaches the agent at its next start; hive-agent reads the store on every connection attempt after its first, so the doc now says that. Part of #5017 (slice Q).
This commit is contained in:
parent
be9b2475a9
commit
a6935e57c3
11 changed files with 819 additions and 376 deletions
|
|
@ -402,6 +402,8 @@ pub mod subagent_term;
|
|||
/// same shapes.
|
||||
pub mod review_classifier;
|
||||
|
||||
pub mod stored_secret;
|
||||
|
||||
/// Only the fields this needs; authelia returns several.
|
||||
#[derive(serde::Deserialize)]
|
||||
struct TokenResponse {
|
||||
|
|
@ -489,28 +491,36 @@ pub struct QueueConfig {
|
|||
pub ca_file: Option<PathBuf>,
|
||||
}
|
||||
|
||||
/// Where a [`QueueConfig`]'s client secret comes from.
|
||||
#[derive(Clone)]
|
||||
/// Where a [`QueueConfig`]'s client secret comes from. `Debug` never prints
|
||||
/// a held secret.
|
||||
#[derive(Clone, Debug)]
|
||||
pub enum ClientSecret {
|
||||
/// A file holding the plaintext, read at every token mint so a rotated
|
||||
/// file takes effect without a restart.
|
||||
File(PathBuf),
|
||||
/// The plaintext itself, held only in this process's memory.
|
||||
Value(String),
|
||||
}
|
||||
|
||||
/// Redacts [`ClientSecret::Value`]: a [`QueueConfig`] is `Debug`, and printing
|
||||
/// one must not put the secret in a log.
|
||||
impl std::fmt::Debug for ClientSecret {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
match self {
|
||||
Self::File(path) => f.debug_tuple("File").field(path).finish(),
|
||||
Self::Value(_) => f.write_str("Value(<redacted>)"),
|
||||
}
|
||||
}
|
||||
/// The plaintext, held only in this process's memory and read again
|
||||
/// when the token endpoint refuses it.
|
||||
Stored(stored_secret::StoredSecret),
|
||||
}
|
||||
|
||||
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.
|
||||
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
|
||||
) =>
|
||||
{
|
||||
Some(stored)
|
||||
}
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
async fn read(&self) -> Result<String, Error> {
|
||||
match self {
|
||||
Self::File(path) => {
|
||||
|
|
@ -521,7 +531,7 @@ impl ClientSecret {
|
|||
source,
|
||||
})
|
||||
}
|
||||
Self::Value(value) => Ok(value.clone()),
|
||||
Self::Stored(stored) => Ok(stored.current()),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -533,7 +543,7 @@ impl ClientSecret {
|
|||
source,
|
||||
})
|
||||
}
|
||||
Self::Value(value) => Ok(value.clone()),
|
||||
Self::Stored(stored) => Ok(stored.current()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -545,6 +555,10 @@ impl ClientSecret {
|
|||
/// (a machine client receives an access token and never an id-token), so a
|
||||
/// caller passing `None` for `scope` requests none at all — see
|
||||
/// [`token_request`] for the one scope a caller does have to ask for.
|
||||
///
|
||||
/// A [`ClientSecret::Stored`] the endpoint refuses is read again, and when
|
||||
/// that lands a different secret the request is sent once more with it; see
|
||||
/// [`stored_secret`] for how often a re-read may run.
|
||||
async fn mint_token(
|
||||
http: &reqwest::Client,
|
||||
cfg: &QueueConfig,
|
||||
|
|
@ -554,7 +568,32 @@ async fn mint_token(
|
|||
// Read per call rather than caching: the file is small, and a cached
|
||||
// secret would survive a rotation that the operator believes took effect.
|
||||
let secret = cfg.client_secret.read().await?;
|
||||
let refused = match mint_with(http, cfg, &secret, audience, scope).await {
|
||||
Err(e) => e,
|
||||
minted => return minted,
|
||||
};
|
||||
let Some(stored) = cfg.client_secret.refused(&refused) else {
|
||||
return Err(refused);
|
||||
};
|
||||
tracing::warn!(
|
||||
client_id = %cfg.client_id,
|
||||
error = %chain(&refused),
|
||||
"the token endpoint refused this client's secret; re-reading it"
|
||||
);
|
||||
match stored.after_refusal(&secret).await {
|
||||
Some(fresh) => mint_with(http, cfg, &fresh, audience, scope).await,
|
||||
None => Err(refused),
|
||||
}
|
||||
}
|
||||
|
||||
/// One token request presenting `secret`.
|
||||
async fn mint_with(
|
||||
http: &reqwest::Client,
|
||||
cfg: &QueueConfig,
|
||||
secret: &str,
|
||||
audience: Option<&str>,
|
||||
scope: Option<&str>,
|
||||
) -> Result<CachedToken, Error> {
|
||||
let response = token_request(http, cfg, secret.trim(), audience, scope)
|
||||
.send()
|
||||
.await
|
||||
|
|
@ -700,13 +739,46 @@ pub fn mint_token_for_blocking(
|
|||
audience: Option<&str>,
|
||||
scope: Option<&str>,
|
||||
) -> Result<String, Error> {
|
||||
let http = build_blocking_http_client(cfg)?;
|
||||
mint_token_blocking(&build_blocking_http_client(cfg)?, cfg, audience, scope)
|
||||
}
|
||||
|
||||
/// [`mint_token`] on the blocking client.
|
||||
fn mint_token_blocking(
|
||||
http: &reqwest::blocking::Client,
|
||||
cfg: &QueueConfig,
|
||||
audience: Option<&str>,
|
||||
scope: Option<&str>,
|
||||
) -> Result<String, Error> {
|
||||
// Read per call rather than caching — see `mint_token`'s identical
|
||||
// comment on the async path; the reasoning does not change with the
|
||||
// client type.
|
||||
let secret = cfg.client_secret.read_blocking()?;
|
||||
let refused = match mint_with_blocking(http, cfg, &secret, audience, scope) {
|
||||
Err(e) => e,
|
||||
minted => return minted,
|
||||
};
|
||||
let Some(stored) = cfg.client_secret.refused(&refused) else {
|
||||
return Err(refused);
|
||||
};
|
||||
tracing::warn!(
|
||||
client_id = %cfg.client_id,
|
||||
error = %chain(&refused),
|
||||
"the token endpoint refused this client's secret; re-reading it"
|
||||
);
|
||||
match stored.after_refusal_blocking(&secret) {
|
||||
Some(fresh) => mint_with_blocking(http, cfg, &fresh, audience, scope),
|
||||
None => Err(refused),
|
||||
}
|
||||
}
|
||||
|
||||
/// [`mint_with`] on the blocking client.
|
||||
fn mint_with_blocking(
|
||||
http: &reqwest::blocking::Client,
|
||||
cfg: &QueueConfig,
|
||||
secret: &str,
|
||||
audience: Option<&str>,
|
||||
scope: Option<&str>,
|
||||
) -> Result<String, Error> {
|
||||
let mut form = vec![("grant_type", "client_credentials")];
|
||||
if let Some(audience) = audience {
|
||||
form.push(("audience", audience));
|
||||
|
|
@ -947,6 +1019,9 @@ where
|
|||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
|
||||
use super::*;
|
||||
|
||||
/// The names the publishers, the relays and the queue's grant all spell.
|
||||
|
|
@ -983,24 +1058,149 @@ mod tests {
|
|||
.with_no_client_auth();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_held_secret_is_read_back_as_itself() {
|
||||
let secret = ClientSecret::Value("s3cret".to_owned());
|
||||
assert_eq!(
|
||||
secret.read_blocking().expect("a value needs no I/O"),
|
||||
"s3cret"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_debug_print_of_a_config_does_not_carry_a_held_secret() {
|
||||
#[tokio::test]
|
||||
async fn a_debug_print_of_a_config_does_not_carry_a_held_secret() {
|
||||
let cfg = QueueConfig {
|
||||
client_secret: ClientSecret::Value("s3cret".to_owned()),
|
||||
client_secret: ClientSecret::Stored(
|
||||
stored_secret::tests::Fake::holding("new").secret("hive-alpha", "s3cret"),
|
||||
),
|
||||
..token_cfg()
|
||||
};
|
||||
let printed = format!("{cfg:?}");
|
||||
assert!(!printed.contains("s3cret"), "{printed}");
|
||||
assert!(printed.contains("<redacted>"), "{printed}");
|
||||
assert!(printed.contains("StoredSecret"), "{printed}");
|
||||
}
|
||||
|
||||
/// A token endpoint on a loopback port that mints for
|
||||
/// `hive-alpha:{registered}` and answers any other secret with a 401,
|
||||
/// counting its requests.
|
||||
async fn stub_idp(registered: &'static str, calls: Arc<AtomicUsize>) -> String {
|
||||
use axum::http::{HeaderMap, StatusCode, header::AUTHORIZATION};
|
||||
|
||||
let expected = format!(
|
||||
"Basic {}",
|
||||
data_encoding::BASE64.encode(format!("hive-alpha:{registered}").as_bytes())
|
||||
);
|
||||
let app = axum::Router::new().fallback(move |headers: HeaderMap| async move {
|
||||
calls.fetch_add(1, Ordering::SeqCst);
|
||||
if headers.get(AUTHORIZATION).and_then(|v| v.to_str().ok()) == Some(&*expected) {
|
||||
(
|
||||
StatusCode::OK,
|
||||
r#"{"access_token":"a-token","expires_in":3599}"#,
|
||||
)
|
||||
} else {
|
||||
(StatusCode::UNAUTHORIZED, r#"{"error":"invalid_client"}"#)
|
||||
}
|
||||
});
|
||||
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 });
|
||||
url
|
||||
}
|
||||
|
||||
/// A plain-HTTP loopback client: no proxy, and no platform trust store,
|
||||
/// which the nix build sandbox does not have (see [`offline_client`]).
|
||||
fn loopback_client() -> reqwest::Client {
|
||||
reqwest::Client::builder()
|
||||
.no_proxy()
|
||||
.danger_accept_invalid_certs(true)
|
||||
.build()
|
||||
.expect("a client that verifies nothing needs no system trust store")
|
||||
}
|
||||
|
||||
/// The rotation the re-read exists for: the token endpoint refuses the
|
||||
/// secret held since start, the store holds the new one, and the same
|
||||
/// mint succeeds with it.
|
||||
#[tokio::test]
|
||||
async fn a_refused_secret_is_re_read_and_the_mint_retried_with_the_new_one() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let store = stored_secret::tests::Fake::holding("secret-b");
|
||||
let cfg = QueueConfig {
|
||||
token_endpoint: stub_idp("secret-b", Arc::clone(&calls)).await,
|
||||
client_secret: ClientSecret::Stored(store.secret("hive-alpha", "secret-a")),
|
||||
..token_cfg()
|
||||
};
|
||||
let minted = mint_token(&loopback_client(), &cfg, None, None)
|
||||
.await
|
||||
.expect("minted with the re-read secret");
|
||||
assert_eq!(minted.token, "a-token");
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 2);
|
||||
assert_eq!(store.reads(), 1);
|
||||
}
|
||||
|
||||
/// A secret the store still holds as refused: the mint fails, and the
|
||||
/// mints after it inside the backoff ask the token endpoint once each
|
||||
/// and read nothing.
|
||||
#[tokio::test]
|
||||
async fn a_permanently_refused_secret_is_not_re_read_on_every_mint() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let store = stored_secret::tests::Fake::holding("secret-a");
|
||||
let cfg = QueueConfig {
|
||||
token_endpoint: stub_idp("secret-b", Arc::clone(&calls)).await,
|
||||
client_secret: ClientSecret::Stored(store.secret("hive-alpha", "secret-a")),
|
||||
..token_cfg()
|
||||
};
|
||||
let http = loopback_client();
|
||||
for _ in 0..5 {
|
||||
let err = mint_token(&http, &cfg, None, None)
|
||||
.await
|
||||
.err()
|
||||
.expect("refused");
|
||||
assert!(
|
||||
matches!(err, Error::TokenRefused { status, .. } if status == 401),
|
||||
"{err}"
|
||||
);
|
||||
}
|
||||
assert_eq!(calls.load(Ordering::SeqCst), 5);
|
||||
assert_eq!(store.reads(), 1);
|
||||
}
|
||||
|
||||
/// 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());
|
||||
}
|
||||
|
||||
/// The blocking mint re-reads the same way, from a thread with no
|
||||
/// runtime of its own.
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn the_blocking_mint_re_reads_a_refused_secret() {
|
||||
let calls = Arc::new(AtomicUsize::new(0));
|
||||
let store = stored_secret::tests::Fake::holding("secret-b");
|
||||
let cfg = QueueConfig {
|
||||
token_endpoint: stub_idp("secret-b", Arc::clone(&calls)).await,
|
||||
client_secret: ClientSecret::Stored(store.secret("hive-alpha", "secret-a")),
|
||||
..token_cfg()
|
||||
};
|
||||
let token = tokio::task::spawn_blocking(move || {
|
||||
let http = reqwest::blocking::Client::builder()
|
||||
.no_proxy()
|
||||
.danger_accept_invalid_certs(true)
|
||||
.build()
|
||||
.expect("client");
|
||||
mint_token_blocking(&http, &cfg, None, None)
|
||||
})
|
||||
.await
|
||||
.expect("joined")
|
||||
.expect("minted with the re-read secret");
|
||||
assert_eq!(token, "a-token");
|
||||
assert_eq!(store.reads(), 1);
|
||||
}
|
||||
|
||||
fn token_cfg() -> QueueConfig {
|
||||
|
|
|
|||
Loading…
Reference in a new issue