From 7129ee0015b920c0182b5705bc0d6ad02b0044e4 Mon Sep 17 00:00:00 2001 From: atlas Date: Fri, 9 Oct 2026 13:01:50 +0200 Subject: [PATCH] swarm: keep the webhook secret on read errors; surface hidden KV/auth/forge failures - swarm-controller webhook: regenerate the secret only when the file is absent. Any other read error is returned with the path; the caller already logs "webhook secret unavailable" and the endpoint answers 503. Overwriting meant every registered hook kept the old secret and every delivery failed HMAC, with nothing re-registering them. - swarm-queue-client wanted: open_read_only/watch return Result; only STREAM_NOT_FOUND on the bucket maps to None. hive-c0re's wanted pull fails the DAG node with the cause instead of logging "nothing has been declared"; the watch warns with the error chain. - swarm-nats-auth: warn when the OIDC secret re-read task dies. - swarm-controller forge: listing a repo's config PRs failing is warn, not debug. Closes #5056 --- hive-c0re/src/workers/wanted.rs | 33 +++++++++++----- swarm-controller/src/forge.rs | 2 +- swarm-controller/src/webhook.rs | 53 +++++++++++++++++++++----- swarm-nats-auth/src/oidc_secret.rs | 13 ++++++- swarm-queue-client/src/lib.rs | 16 ++++++++ swarm-queue-client/src/wanted.rs | 60 +++++++++++++++++++++++++----- 6 files changed, 147 insertions(+), 30 deletions(-) diff --git a/hive-c0re/src/workers/wanted.rs b/hive-c0re/src/workers/wanted.rs index c8ee89dd..a488c5ee 100644 --- a/hive-c0re/src/workers/wanted.rs +++ b/hive-c0re/src/workers/wanted.rs @@ -63,7 +63,10 @@ pub async fn pull(coord: &Arc) -> Result<()> { // A hung node is worse than a failed one: the dashboard shows it running. swarm_queue_client::ensure_connected(&client)?; - let Some(store) = swarm_queue_client::wanted::open_read_only(&client, &hive).await else { + let Some(store) = swarm_queue_client::wanted::open_read_only(&client, &hive) + .await + .context("opening this hive's wanted-state bucket")? + else { tracing::info!(%hive, "wanted state: no bucket yet; nothing has been declared"); return Ok(()); }; @@ -89,7 +92,7 @@ pub async fn pull(coord: &Arc) -> Result<()> { /// /// ⚠️ A refused watch and a quiet one are told apart here, unlike the core-NATS /// subscriptions in [`crate::swarm_status`]: a watch is a `JetStream` consumer, -/// so the request is answered, and a hive lacking the grant gets `None` rather +/// so the request is answered, and a hive lacking the grant gets an error rather /// than silence. That is why this warns instead of returning quietly. pub async fn watch_declarations( client: async_nats::Client, @@ -99,13 +102,25 @@ pub async fn watch_declarations( ) { use futures_util::StreamExt as _; - let Some(mut updates) = swarm_queue_client::wanted::watch(&client, &hive).await else { - tracing::warn!( - %hive, - "wanted state: cannot watch this hive's bucket; changes will be \ - picked up at the next boot instead" - ); - return; + let mut updates = match swarm_queue_client::wanted::watch(&client, &hive).await { + Ok(Some(updates)) => updates, + Ok(None) => { + tracing::warn!( + %hive, + "wanted state: cannot watch this hive's bucket; changes will be \ + picked up at the next boot instead" + ); + return; + } + Err(e) => { + tracing::warn!( + %hive, + error = %swarm_queue_client::chain(&e), + "wanted state: cannot watch this hive's bucket; changes will be \ + picked up at the next boot instead" + ); + return; + } }; tracing::info!(%hive, "wanted state: watching for declarations"); diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 658936f1..b9d7892a 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -755,7 +755,7 @@ impl Client { { Ok(prs) => prs, Err(e) => { - tracing::debug!(%agent, error = %e, "swarm forge: listing config PRs failed, skipping repo"); + tracing::warn!(%agent, error = %e, "swarm forge: listing config PRs failed, skipping repo"); continue; } }; diff --git a/swarm-controller/src/webhook.rs b/swarm-controller/src/webhook.rs index 95c44cd5..e0ad142a 100644 --- a/swarm-controller/src/webhook.rs +++ b/swarm-controller/src/webhook.rs @@ -85,8 +85,11 @@ fn state_dir_from(raw: Option<&str>) -> std::path::PathBuf { /// /// # Errors /// -/// Returns an error when the secret cannot be generated (no `/dev/urandom`) -/// or cannot be persisted. A malformed existing file is **not** an error: it +/// Returns an error, leaving the file untouched, when an existing file cannot +/// be read: hooks already registered with the stored secret are never +/// re-registered, so replacing it would fail every delivery's HMAC check. +/// Also errors when the secret cannot be generated (no `/dev/urandom`) or +/// cannot be persisted. A malformed existing file is **not** an error: it /// is regenerated, because a secret that cannot be parsed is /// indistinguishable from one that was never written, and failing startup /// over it would strand the daemon with no path forward. @@ -98,15 +101,21 @@ pub(super) fn load_or_generate_secret() -> Result { /// same reason as [`secret_path_from`]: a test can point it at a scratch /// file without mutating a process-global env var. fn load_or_generate_at(path: &std::path::Path) -> Result { - if let Ok(raw) = std::fs::read_to_string(path) { - let trimmed = raw.trim().to_owned(); - if trimmed.len() == 64 && trimmed.chars().all(|c| c.is_ascii_hexdigit()) { - return Ok(trimmed); + match std::fs::read_to_string(path) { + Ok(raw) => { + let trimmed = raw.trim().to_owned(); + if trimmed.len() == 64 && trimmed.chars().all(|c| c.is_ascii_hexdigit()) { + return Ok(trimmed); + } + tracing::warn!( + path = %path.display(), + "webhook-secret file malformed (wrong length/chars); regenerating" + ); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + return Err(e).with_context(|| format!("read webhook secret {}", path.display())); } - tracing::warn!( - path = %path.display(), - "webhook-secret file malformed (wrong length/chars); regenerating" - ); } let secret = generate_hex_secret()?; std::fs::create_dir_all(path.parent().unwrap_or(path)) @@ -591,6 +600,30 @@ mod tests { ); } + /// A path that exists but cannot be read is an error, and nothing is + /// written over it. A directory stands in for the unreadable file because + /// its read fails the same way for root and non-root test runners. + #[test] + fn an_unreadable_secret_path_is_an_error_and_left_untouched() { + let dir = + std::env::temp_dir().join(format!("swarm-hook-unreadable-{}", std::process::id())); + let path = dir.join("webhook-secret"); + std::fs::create_dir_all(&path).expect("mkdir at the secret path"); + + let result = load_or_generate_at(&path); + let still_dir = path.is_dir(); + let entries = std::fs::read_dir(&path).map(Iterator::count).ok(); + std::fs::remove_dir_all(&dir).ok(); + + let err = result.expect_err("an unreadable secret must not be replaced"); + assert!( + format!("{err:#}").contains("webhook-secret"), + "the error names the path: {err:#}" + ); + assert!(still_dir, "the existing path was replaced"); + assert_eq!(entries, Some(0), "something was written under the path"); + } + /// `StateDirectory=` may name several directories, in which case systemd /// exports them colon-separated — take the first, never the raw value. /// diff --git a/swarm-nats-auth/src/oidc_secret.rs b/swarm-nats-auth/src/oidc_secret.rs index 63425f1b..ea158762 100644 --- a/swarm-nats-auth/src/oidc_secret.rs +++ b/swarm-nats-auth/src/oidc_secret.rs @@ -197,7 +197,18 @@ impl OidcSecret { } }); let handle = task.abort_handle(); - let shared = task.map(|read| read.unwrap_or(false)).boxed().shared(); + let shared = task + .map(|read| { + read.unwrap_or_else(|e| { + tracing::warn!( + error = %e, + "the OIDC client secret re-read task died; keeping the one in use" + ); + false + }) + }) + .boxed() + .shared(); *slot = Some((handle, shared.clone())); shared } diff --git a/swarm-queue-client/src/lib.rs b/swarm-queue-client/src/lib.rs index 041fb7ac..dc03f3a6 100644 --- a/swarm-queue-client/src/lib.rs +++ b/swarm-queue-client/src/lib.rs @@ -118,6 +118,22 @@ pub enum Error { source: async_nats::jetstream::context::CreateKeyValueError, }, + #[cfg(feature = "kv")] + #[error("opening the {bucket} bucket")] + OpenBucket { + bucket: String, + #[source] + source: async_nats::jetstream::context::KeyValueError, + }, + + #[cfg(feature = "kv")] + #[error("watching key {key}")] + WatchKey { + key: String, + #[source] + source: async_nats::jetstream::kv::WatchError, + }, + #[cfg(feature = "notices")] #[error("creating the {stream} stream")] CreateStream { diff --git a/swarm-queue-client/src/wanted.rs b/swarm-queue-client/src/wanted.rs index b10d8d7f..451039c5 100644 --- a/swarm-queue-client/src/wanted.rs +++ b/swarm-queue-client/src/wanted.rs @@ -134,34 +134,76 @@ pub async fn open_or_create( /// **Hive-side.** `None` is the ordinary pre-publication state, not a failure: /// a hive holds no grant to create this bucket and must not treat its absence /// as a reason to converge to an empty agent set — see the module docs. +/// +/// # Errors +/// +/// [`Error::OpenBucket`] for every failure other than the bucket's stream not +/// existing: a refused grant, a timeout, `JetStream` unavailable. #[cfg(feature = "kv")] pub async fn open_read_only( client: &async_nats::Client, hive: &str, -) -> Option { +) -> Result, Error> { let js = async_nats::jetstream::new(client.clone()); - js.get_key_value(bucket(hive)).await.ok() + let bucket = bucket(hive); + match js.get_key_value(&bucket).await { + Ok(store) => Ok(Some(store)), + Err(e) if is_bucket_not_found(&e) => Ok(None), + Err(source) => Err(Error::OpenBucket { bucket, source }), + } +} + +/// `get_key_value` reports a missing bucket as `GetBucket` wrapping the +/// stream lookup's `STREAM_NOT_FOUND`; every other kind is a real failure. +#[cfg(feature = "kv")] +fn is_bucket_not_found(e: &async_nats::jetstream::context::KeyValueError) -> bool { + use async_nats::jetstream::context::{GetStreamError, GetStreamErrorKind, KeyValueErrorKind}; + + e.kind() == KeyValueErrorKind::GetBucket + && std::error::Error::source(e) + .and_then(|s| s.downcast_ref::()) + .is_some_and(|s| { + matches!( + s.kind(), + GetStreamErrorKind::JetStream(js) + if js.error_code() == async_nats::jetstream::ErrorCode::STREAM_NOT_FOUND + ) + }) } /// Watch this hive's own declaration for changes. /// -/// **Hive-side.** `None` on the same terms as [`open_read_only`] — no bucket -/// yet — plus one more: a watch is a `JetStream` *consumer*, so it needs a grant -/// a plain `get` does not. A hive missing `CONSUMER.CREATE` on its own stream -/// gets `None` here while `get` keeps working, which is why the caller must -/// treat this as "not watching yet" and retry rather than as a dead end. +/// **Hive-side.** `None` on the same terms as [`open_read_only`]: no bucket +/// yet. A watch is a `JetStream` *consumer*, so it needs a grant a plain `get` +/// does not; a hive missing `CONSUMER.CREATE` on its own stream gets +/// [`Error::WatchKey`] here while `get` keeps working. /// /// Updates only, deliberately: the boot-time read already has the current /// value, and a watch that replayed history would re-converge the whole /// declaration on every reconnect for nothing. +/// +/// # Errors +/// +/// [`Error::OpenBucket`] as for [`open_read_only`], and [`Error::WatchKey`] +/// when the watch itself is refused or fails. #[cfg(feature = "kv")] pub async fn watch( client: &async_nats::Client, hive: &str, -) -> Option { +) -> Result, Error> { + let Some(store) = open_read_only(client, hive).await? else { + return Ok(None); + }; // The bucket holds exactly one key, named for the hive — same key // `open_read_only`'s caller reads, so both paths address one declaration. - open_read_only(client, hive).await?.watch(hive).await.ok() + store + .watch(hive) + .await + .map(Some) + .map_err(|source| Error::WatchKey { + key: hive.to_owned(), + source, + }) } #[cfg(test)]