From 7b5f383b0554b5f573b8b134aa9764122ab0780c Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 17 Aug 2026 01:02:31 +0200 Subject: [PATCH] fix(#3297): reject a publish template that names no hive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--hive-publish-subject` exists to put a second stream inside one hive's namespace. A template with no `{hive}` in it expands to the same subject for every hive, so the option whose only purpose is scoping becomes the way to remove it — silently, and only in the deployment that set it. `Policy::new` returns a `Result` rather than checking at the call site: that makes an unscoped policy unconstructible instead of merely unlikely, the same reason `grant` takes its permissions by value. The error names the offending template and says what goes wrong with it, because an operator meets it at boot with no other context. Also documents what the prefix match does not do. A client id is a hive here because it starts with the configured prefix, not because it appears in the roster — the responder runs in a container and cannot see `swarm.hives`. Passing the roster in would close that and would also be a second place deciding who may connect as what, which `introspect`'s docs argue against for the same reason admission lives in one place. The two intra-doc links to `open_or_create` become plain backticks. Un-gating the `status` module means its module doc now renders in builds without the `kv` feature, where the item it linked does not exist. --- swarm-nats-auth/src/main.rs | 6 ++- swarm-nats-auth/src/policy.rs | 77 ++++++++++++++++++++++++++++++-- swarm-queue-client/src/status.rs | 8 ++-- 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/swarm-nats-auth/src/main.rs b/swarm-nats-auth/src/main.rs index eff84b8d..61fceabb 100644 --- a/swarm-nats-auth/src/main.rs +++ b/swarm-nats-auth/src/main.rs @@ -130,12 +130,16 @@ async fn main() -> anyhow::Result<()> { // way for two deployments to disagree about which one that is. This // responder is the third end that names it, so it takes the same // constant rather than a copy of the literal. + // Fails the process rather than warning: a policy that cannot express a + // per-hive namespace is not a policy this responder should run with, and + // the queue's fail-closed state (no responder) is a legible outage where a + // silently over-broad grant is not. let policy = policy::Policy::new( args.hive_client_prefix.clone(), swarm_queue_client::status::BUCKET.to_owned(), args.reader_clients.clone(), args.hive_publish_subjects.clone(), - ); + )?; let http = reqwest::Client::new(); let issuer = nkeys::KeyPair::from_seed(&read_secret(&args.issuer_seed_file)?) .context("parse the account signing seed")?; diff --git a/swarm-nats-auth/src/policy.rs b/swarm-nats-auth/src/policy.rs index 69d36b69..9b7f2006 100644 --- a/swarm-nats-auth/src/policy.rs +++ b/swarm-nats-auth/src/policy.rs @@ -65,18 +65,37 @@ impl Policy { /// KV bucket hives report status in, `readers` the client ids allowed to /// read every hive's key, and `extra_hive_subjects` additional subjects a /// hive may publish to (with `{hive}` standing for its own name). + /// + /// # Errors + /// + /// An extra subject with no `{hive}` in it is refused. Such a template + /// expands to the **same** subject for every hive, which is not a per-hive + /// namespace — it is the absence of one, arrived at through the option + /// whose only purpose is to provide one. Returning a `Result` rather than + /// checking at the call site is deliberate: it makes an unscoped policy + /// unconstructible instead of merely unlikely. pub fn new( hive_prefix: String, bucket: String, readers: Vec, extra_hive_subjects: Vec, - ) -> Self { - Self { + ) -> anyhow::Result { + if let Some(bad) = extra_hive_subjects + .iter() + .find(|s| !s.contains(HIVE_PLACEHOLDER)) + { + anyhow::bail!( + "--hive-publish-subject {bad:?} contains no {HIVE_PLACEHOLDER}: every hive \ + would be granted that exact subject, so it is a shared channel rather than \ + a per-hive namespace" + ); + } + Ok(Self { hive_prefix, bucket, readers, extra_hive_subjects, - } + }) } /// The permissions for `client_id`, or `None` when no rule matches. @@ -106,6 +125,21 @@ impl Policy { /// hive-shaped ends up with a hive's permissions. An id that is the prefix /// and nothing else names no hive and is refused — `$KV..` is not /// a narrower subject than `$KV..`, it is a different one. + /// + /// # This matches the prefix; it does not verify the roster + /// + /// A client id is a hive here because it *starts with the prefix*, not + /// because it appears in `services.hyperhive.swarm.hives`. This responder + /// runs inside a container and has no view of the roster, so an + /// operator-declared client called `hive-anything` would be granted + /// `$KV..anything` — a key no real hive owns, which the controller + /// renders as a hive it does not recognise. + /// + /// Passing the roster in would close that, and would also be a **second + /// place deciding who may connect as what**, which `crate::introspect`'s + /// docs argue against for the same reason admission lives in one place. + /// The prefix is a contract with `swarm-authelia.nix`, and this is the end + /// of it that can be checked from in here. fn hive_name<'a>(&self, client_id: &'a str) -> Option<&'a str> { client_id .strip_prefix(&self.hive_prefix) @@ -175,6 +209,40 @@ mod tests { vec!["swarm-controller".to_owned()], Vec::new(), ) + .expect("the default policy is valid") + } + + #[test] + fn an_extra_subject_without_the_placeholder_is_refused() { + // 🩸 The option exists to put a second stream inside ONE hive's + // namespace. A template with no `{hive}` expands to the same subject + // for all of them, so the flag whose purpose is scoping becomes the + // way to remove it - silently, and only in the deployment that set it. + let err = Policy::new( + "hive-".to_owned(), + "hive-status".to_owned(), + Vec::new(), + vec!["$SWARM.events.all".to_owned()], + ) + .expect_err("a subject shared by every hive must not be accepted"); + let msg = format!("{err}"); + assert!( + msg.contains("$SWARM.events.all"), + "the error must name the offending template, got: {msg}" + ); + } + + #[test] + fn an_extra_subject_with_the_placeholder_is_accepted() { + // The other half: a check that only ever rejects would be indistinguishable + // from the option being broken. + Policy::new( + "hive-".to_owned(), + "hive-status".to_owned(), + Vec::new(), + vec!["$SWARM.events.{hive}.>".to_owned()], + ) + .expect("a per-hive template is the shape this option is for"); } #[test] @@ -275,7 +343,8 @@ mod tests { "hive-status".to_owned(), Vec::new(), vec!["$SWARM.events.{hive}.>".to_owned()], - ); + ) + .expect("a per-hive template is valid"); let g = p.permissions("hive-alpha").expect("a hive is admitted"); assert!(g.publish.contains(&"$SWARM.events.alpha.>".to_owned())); assert!(!g.publish.iter().any(|s| s.contains("{hive}"))); diff --git a/swarm-queue-client/src/status.rs b/swarm-queue-client/src/status.rs index d4f96c2a..1f0e69c2 100644 --- a/swarm-queue-client/src/status.rs +++ b/swarm-queue-client/src/status.rs @@ -7,7 +7,7 @@ //! repeated across crates is an agreement nothing checks.** //! //! The name is the obvious half. The sharper half is the *config*: both -//! ends open the bucket with [`crate::status::open_or_create`], because either may +//! ends open the bucket with `open_or_create`, because either may //! arrive first on a fresh swarm and neither can assume the other has //! run. If the two ends passed different `Config`s, whichever created it //! would win and the other's `get_key_value` would succeed against a @@ -15,8 +15,10 @@ //! nobody chose. Sharing the constructor makes the race have one outcome //! instead of two. //! -//! The bucket *name* is unconditional; only [`open_or_create`] is behind the -//! `kv` feature. A third end names the bucket without ever opening it — the +//! The bucket *name* is unconditional; only `open_or_create` is behind the +//! `kv` feature. (Named in backticks rather than linked: with `kv` off the item +//! does not exist, and an intra-doc link to it fails the rustdoc gate in +//! exactly the configuration this split exists to support.) A third end names the bucket without ever opening it — the //! auth-callout responder, which derives the subjects a hive may publish to //! from it — and it speaks neither `jetstream` nor `kv`. Gating the name too //! would have made that consumer choose between a JetStream stack it does not