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