From 780df10d9d34247f3e8f72fd52ce039ddc932dee Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 31 Aug 2026 18:10:18 +0200 Subject: [PATCH] swarm: name the agent client after its hive, not after "agent" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `agent-` reads as "the agent named " — which is the one thing that identity does not carry, since it is minted per hive. It becomes `hive--agent`: the hive's own id, extended. The rename is not a string swap. `hive-foo-agent` satisfies the hive parse too (it strips to a hive named `foo-agent`), so the responder's agent rule now runs BEFORE its hive rule — most specific wins. Hive-first would have handed every agent its hive's grant, including writing that hive's status key, with nothing to report it: the client authenticates and is merely able to do more than it should. `Policy::new`'s overlap check goes with the prefix it was written for. The invariant the suffix form needs instead is that the suffix is non-empty: an empty one makes `strip_suffix` succeed on every hive id, so the two principals become one string and whichever arm runs first answers for both. The suffix form also introduces a collision the prefix form did not have: a hive genuinely named `foo-agent` mints `hive-foo-agent`, which is hive `foo`'s agent id. The responder cannot see it — it has no roster, deliberately — so `swarm-authelia.nix` asserts at eval that no hive name ends with the suffix. The existing duplicate-id assertion does not cover this: it fires only when both `foo` and `foo-agent` are on the roster, and with `foo-agent` alone there is no duplicate, just a hive quietly receiving its agents' grant. A test written by analogy with `the_prefix_alone_names_no_hive` failed, correctly — `hive--agent` is a hive named `-agent` under the hive parse, which this module cannot rule out. It now asserts only the part this module owns: no empty hive name is ever expanded into a subject. --- nix/host-modules/swarm-authelia.nix | 60 ++++++++-- nix/host-modules/swarm-nats.nix | 2 +- swarm-nats-auth/src/main.rs | 22 ++-- swarm-nats-auth/src/policy.rs | 180 ++++++++++++++++++---------- 4 files changed, 181 insertions(+), 83 deletions(-) diff --git a/nix/host-modules/swarm-authelia.nix b/nix/host-modules/swarm-authelia.nix index dc4dc517..38aac164 100644 --- a/nix/host-modules/swarm-authelia.nix +++ b/nix/host-modules/swarm-authelia.nix @@ -156,17 +156,24 @@ let # stop being static, which is the same problem the users-database writer # already solves for identities. # + # 🏷️ The id EXTENDS the hive's own (`hive--agent`) rather than taking a + # prefix of its own (`agent-`), because that reads as *the agent called + # ``* — which is the one thing this identity does not carry. The cost is + # that the two ids are no longer distinguishable by prefix, so the responder's + # agent rule must be tried BEFORE its hive rule; `policy.rs` says so at the + # match site, and the assertion below covers the case that ordering cannot. + # # Mirrors `hiveClients` field for field so the two stay comparable. The # signing algorithm is not load-bearing here — this client's only consumer is # the queue's auth-callout responder, which *introspects* rather than # verifying offline — but it matches its sibling rather than inventing a # second answer to a question nobody asked. agentClients = lib.mapAttrsToList (name: _: { - id = "${cfg.agentClientPrefix}${name}"; + id = "${cfg.hiveClientPrefix}${name}${cfg.agentClientSuffix}"; description = "HyperHive agents on hive ${name}"; kind = "machine"; redirectUris = [ ]; - audience = [ "${cfg.agentClientPrefix}${name}" ]; + audience = [ "${cfg.hiveClientPrefix}${name}${cfg.agentClientSuffix}" ]; accessTokenSignedResponseAlg = "RS256"; }) hyperhiveCfg.swarm.hives; @@ -446,7 +453,7 @@ in Mint machine clients per hive in {option}`services.hyperhive.swarm.hives`, so each hive can authenticate to swarm services as itself: `hive-` for the - hive's own daemons, and `agent-` for the agent containers + hive's own daemons, and `hive--agent` for the agent containers running on it. Two clients rather than one because they are not the same @@ -706,19 +713,22 @@ in ''; }; - agentClientPrefix = lib.mkOption { + agentClientSuffix = lib.mkOption { type = lib.types.str; readOnly = true; - default = "agent-"; + default = "-agent"; description = '' - Prefix of the OAuth2 client id minted for the *agents* of each hive - in `services.hyperhive.swarm.hives` — agents on hive `alpha` all - present - `${config.services.hyperhive.swarm.authelia.agentClientPrefix}alpha`. + Suffix appended to a hive's own client id to name the client its + *agent containers* present — agents on hive `alpha` all present + `${config.services.hyperhive.swarm.authelia.hiveClientPrefix}alpha${config.services.hyperhive.swarm.authelia.agentClientSuffix}`. Read-only for the same reason as `hiveClientPrefix`, and read by the same consumer with the same failure mode: a split spelling denies every agent as a timeout. + A suffix on the hive's id rather than a prefix of its own, because + `agent-alpha` reads as *the agent named alpha* — which is precisely + what this identity does not say. + One id per hive rather than per agent, because agents are created at runtime and a per-agent client would make creating one a config change plus a reload. The consequence is that this identity says @@ -937,6 +947,38 @@ in + "services.hyperhive.swarm.hives; rename the hive or the " + "colliding client."; } + { + # A hive whose name ENDS with the agent suffix mints an id the + # queue's responder reads as somebody else's agents: + # `hive-foo-agent` parses as *the agents of hive foo* before it + # parses as *the hive foo-agent*, because the agent rule is the + # more specific one and therefore runs first. + # + # ⚠️ NOT covered by the duplicate-id assertion above, and the gap is + # the interesting half. That one fires only when BOTH `foo` and + # `foo-agent` are on the roster, because only then are two clients + # actually named the same string. With `foo-agent` alone there is no + # duplicate and nothing to see — the hive simply receives its + # agents' grant instead of its own, and a NATS denial arrives as a + # timeout, so the symptom names nothing. + # + # Checkable here and nowhere downstream: the responder runs in a + # container with no view of the roster (`policy.rs`'s `hive_name` + # says why that is deliberate), so this is the last place that knows + # both the naming scheme and the set of names. + assertion = + !cfg.oidc.hiveIdentities + || !lib.any (h: lib.hasSuffix cfg.agentClientSuffix h) (lib.attrNames hyperhiveCfg.swarm.hives); + message = + "services.hyperhive.swarm.hives contains " + + lib.concatMapStringsSep ", " (h: "'${h}'") ( + lib.filter (h: lib.hasSuffix cfg.agentClientSuffix h) (lib.attrNames hyperhiveCfg.swarm.hives) + ) + + " — names ending with '${cfg.agentClientSuffix}', the suffix that " + + "marks a hive's agent containers. Such a hive's own client id is " + + "indistinguishable from another hive's agent client, and the " + + "queue resolves it as the agents. Rename the hive."; + } # The two obligations `authelia.bearer.authz` carries. Authelia # enforces both itself — but in its `preStart` validator, so a # violation produces a green `nixos-rebuild switch` and an authelia diff --git a/nix/host-modules/swarm-nats.nix b/nix/host-modules/swarm-nats.nix index 90c7d0d0..4aef2a49 100644 --- a/nix/host-modules/swarm-nats.nix +++ b/nix/host-modules/swarm-nats.nix @@ -769,7 +769,7 @@ in # so a drift here is silent at the point of change and # misattributed at the point of failure. "--hive-client-prefix ${lib.escapeShellArg autheliaCfg.hiveClientPrefix}" - "--agent-client-prefix ${lib.escapeShellArg autheliaCfg.agentClientPrefix}" + "--agent-client-suffix ${lib.escapeShellArg autheliaCfg.agentClientSuffix}" "--reader-client ${lib.escapeShellArg controllerCfg.queueClientId}" ]; # Every credential arrives by `LoadCredential` and is named diff --git a/swarm-nats-auth/src/main.rs b/swarm-nats-auth/src/main.rs index 52969100..f98c896f 100644 --- a/swarm-nats-auth/src/main.rs +++ b/swarm-nats-auth/src/main.rs @@ -82,15 +82,20 @@ struct Args { #[arg(long, default_value = "hive-")] hive_client_prefix: String, - /// Client-id prefix that marks an agent container. `swarm-authelia.nix` - /// mints one machine client per roster entry as `agent-` — per - /// **hive**, not per agent, because agents are created at runtime and a - /// per-agent client would make creating one a config change plus a reload. + /// Suffix that marks an agent container, appended to the hive's own client + /// id: `swarm-authelia.nix` mints one machine client per roster entry as + /// `hive--agent` — per **hive**, not per agent, because agents are + /// created at runtime and a per-agent client would make creating one a + /// config change plus a reload. /// /// So this identity says which hive an agent belongs to and never which /// agent: two agents on one hive are indistinguishable to this responder. - #[arg(long, default_value = "agent-")] - agent_client_prefix: String, + /// + /// A suffix on the hive's id rather than a prefix of its own, because + /// `agent-` reads as *the agent called ``* — the one thing + /// this identity does not carry. + #[arg(long, default_value = "-agent")] + agent_client_suffix: String, /// Client ids allowed to read every hive's status. Repeatable. The /// default is the swarm controller, which is the only reader that exists. @@ -108,7 +113,8 @@ struct Args { hive_publish_subjects: Vec, /// Subjects an agent may publish to, with `{hive}` standing for the hive - /// its identity names. Repeatable, empty by default. + /// its identity names — `hive-alpha-agent` expands it to `alpha`. + /// Repeatable, empty by default. /// /// Empty means agents get **no grant at all** rather than a grant that can /// do nothing — the identity exists, and what it may say is a deployment's @@ -161,7 +167,7 @@ async fn main() -> anyhow::Result<()> { // silently over-broad grant is not. let policy = policy::Policy::new( args.hive_client_prefix.clone(), - args.agent_client_prefix.clone(), + args.agent_client_suffix.clone(), swarm_queue_client::status::BUCKET.to_owned(), args.reader_clients.clone(), args.hive_publish_subjects.clone(), diff --git a/swarm-nats-auth/src/policy.rs b/swarm-nats-auth/src/policy.rs index 0433b444..0edd6536 100644 --- a/swarm-nats-auth/src/policy.rs +++ b/swarm-nats-auth/src/policy.rs @@ -40,7 +40,7 @@ pub struct Permissions { #[derive(Debug, Clone)] pub struct Policy { hive_prefix: String, - agent_prefix: String, + agent_suffix: String, bucket: String, readers: Vec, extra_hive_subjects: Vec, @@ -53,11 +53,16 @@ const HIVE_PLACEHOLDER: &str = "{hive}"; impl Policy { /// `hive_prefix` is the client-id prefix that marks a hive and - /// `agent_prefix` the one that marks a hive's agent containers; `bucket` is - /// the KV bucket hives report status in, `readers` the client ids allowed - /// to read every hive's key, and `extra_hive_subjects` / - /// `extra_agent_subjects` additional subjects each may publish to (with - /// `{hive}` standing for the hive's own name). + /// `agent_suffix` what a hive's agent containers carry **on top of** it — + /// `hive-` and `hive--agent`. `bucket` is the KV bucket hives + /// report status in, `readers` the client ids allowed to read every hive's + /// key, and `extra_hive_subjects` / `extra_agent_subjects` additional + /// subjects each may publish to (with `{hive}` standing for the hive's own + /// name). + /// + /// The agent id is deliberately the hive's id extended, not a second + /// top-level prefix: `agent-` reads as *the agent called ``*, + /// which is the one thing this identity does not carry. /// /// # Errors /// @@ -68,13 +73,13 @@ impl Policy { /// checking at the call site is deliberate: it makes an unscoped policy /// unconstructible instead of merely unlikely. /// - /// Two prefixes where one of them is a prefix of the other is refused for - /// the same reason: the arms are tried in order, so the overlap does not - /// error at match time — it silently hands one principal the other's - /// grant. + /// An empty `agent_suffix` is refused for the same reason: every hive id + /// would also parse as its own agent id, and the arms are tried in order, + /// so the overlap does not error at match time — it silently hands one + /// principal the other's grant. pub fn new( hive_prefix: String, - agent_prefix: String, + agent_suffix: String, bucket: String, readers: Vec, extra_hive_subjects: Vec, @@ -103,23 +108,24 @@ impl Policy { rather than a per-hive namespace" ); } - // Checked here rather than trusted from nix, because the two prefixes - // are two flags and nothing downstream compares them. `permissions` - // tries the hive arm first, so an agent prefix of `""` — or either one - // being a prefix of the other — would route agents into the hive grant - // with no error anywhere: a client that authenticates fine and is - // granted more than it should have, which is the one failure this - // module must not have. - if hive_prefix.starts_with(&agent_prefix) || agent_prefix.starts_with(&hive_prefix) { + // Checked here rather than trusted from nix, because these are two + // flags and nothing downstream compares them. An agent id is a hive id + // plus this suffix, so an empty one makes `strip_suffix` succeed on + // every hive id and the two principals become the same string. What + // that costs is not a parse error but a **grant**: whichever arm runs + // first answers for both, and a client that authenticates fine is + // handed more than it should have — the one failure this module must + // not have. + if agent_suffix.is_empty() { anyhow::bail!( - "--hive-client-prefix {hive_prefix:?} and --agent-client-prefix \ - {agent_prefix:?} overlap: one is a prefix of the other, so a client id \ - matching the longer one is granted by whichever rule is tried first" + "--agent-client-suffix is empty: an agent id is a hive id plus this suffix, \ + so every hive would also parse as its own agents and one of the two would \ + silently receive the other's grant" ); } Ok(Self { hive_prefix, - agent_prefix, + agent_suffix, bucket, readers, extra_hive_subjects, @@ -133,11 +139,15 @@ impl Policy { /// a connected client with no permissions still holds a slot and still /// looks admitted in the logs, which is a worse answer than a refusal. pub fn permissions(&self, client_id: &str) -> Option { - if let Some(hive) = self.hive_name(client_id) { - return Some(Permissions { - publish: self.hive_subjects(hive), - }); - } + // ⚠️ THE AGENT ARM RUNS FIRST, AND THE ORDER IS LOAD-BEARING. An agent + // id is a hive id with a suffix, so `hive-foo-agent` satisfies the hive + // arm too — it strips to the hive `foo-agent`. Hive-first would + // therefore hand every agent its hive's grant, including writing that + // hive's status key, and nothing would report it: the client + // authenticates, connects, and is simply able to do more than it + // should. Most specific wins, so the more specific test is the one + // that has to be asked first. + // // An agent's identity names its hive, never the agent — see // `Self::agent_hive`. The grant is whatever the deployment configured // for agents, expanded for that hive, and **nothing when it configured @@ -147,6 +157,11 @@ impl Policy { let publish = self.agent_subjects(hive); return (!publish.is_empty()).then_some(Permissions { publish }); } + if let Some(hive) = self.hive_name(client_id) { + return Some(Permissions { + publish: self.hive_subjects(hive), + }); + } if self.readers.iter().any(|r| r == client_id) { return Some(Permissions { publish: self.reader_subjects(), @@ -186,14 +201,24 @@ impl Policy { /// The hive whose agents a client id names, when it names one. /// - /// Deliberately the same shape as [`Self::hive_name`] one prefix over, and - /// every caveat above applies unchanged. One more on top: the agent client - /// is minted per **hive**, so this answers *whose agents*, never *which - /// agent*. Two agents on one hive are indistinguishable here — not a gap in - /// the parsing but the identity itself, which does not carry the agent. + /// [`Self::hive_name`]'s caveats all apply — this is that parse with the + /// suffix stripped as well, so `hive-foo-agent` answers `foo`. Two more on + /// top: + /// + /// - The agent client is minted per **hive**, so this answers *whose + /// agents*, never *which agent*. Two agents on one hive are + /// indistinguishable here — not a gap in the parsing but the identity + /// itself, which does not carry the agent. + /// - A hive genuinely named `foo-agent` mints `hive-foo-agent` and is + /// therefore **indistinguishable from hive `foo`'s agents** right here. + /// Unguardable from inside this responder, which has no roster (see + /// [`Self::hive_name`]) — `swarm-authelia.nix` asserts it at eval, where + /// the roster is known, so the deployment fails to build rather than + /// minting two principals with one id. fn agent_hive<'a>(&self, client_id: &'a str) -> Option<&'a str> { client_id - .strip_prefix(&self.agent_prefix) + .strip_prefix(&self.hive_prefix)? + .strip_suffix(&self.agent_suffix) .filter(|name| !name.is_empty()) } @@ -495,7 +520,7 @@ mod tests { fn policy() -> Policy { Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), vec!["swarm-controller".to_owned()], Vec::new(), @@ -510,7 +535,7 @@ mod tests { fn policy_with_agent_subject() -> Policy { Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), vec!["swarm-controller".to_owned()], Vec::new(), @@ -613,7 +638,7 @@ mod tests { // way to remove it - silently, and only in the deployment that set it. let err = Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), Vec::new(), vec!["$SWARM.events.all".to_owned()], @@ -633,7 +658,7 @@ mod tests { // from the option being broken. Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), Vec::new(), vec!["$SWARM.events.{hive}.>".to_owned()], @@ -1041,7 +1066,7 @@ mod tests { // another's events even though its status key is scoped. let p = Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), Vec::new(), vec!["$SWARM.events.{hive}.>".to_owned()], @@ -1059,7 +1084,7 @@ mod tests { // A grant with an empty publish list would let it connect and then // fail on every publish, which reads as a broken queue rather than as // a deployment that configured nothing. - assert!(policy().permissions("agent-alpha").is_none()); + assert!(policy().permissions("hive-alpha-agent").is_none()); } #[test] @@ -1067,18 +1092,28 @@ mod tests { // The presence control for the test above - without it, "refused" // would also be the answer if the agent arm never matched at all. let g = policy_with_agent_subject() - .permissions("agent-alpha") + .permissions("hive-alpha-agent") .expect("an agent with a configured subject is admitted"); assert_eq!(g.publish, vec!["$SWARM.term.alpha.>".to_owned()]); } #[test] fn an_agent_does_not_get_its_hives_grant() { - // The whole point of a second prefix. An agent holds a credential that - // sits in a container; a hive's grant includes writing that hive's - // status key, which an agent must not be able to forge. + // An agent holds a credential that sits in a container; a hive's grant + // includes writing that hive's status key, which an agent must not be + // able to forge. + // + // 🩸 This became a REGRESSION test when the id moved from `agent-` + // to `hive--agent`: the agent id now satisfies the HIVE parse too + // (`hive-alpha-agent` strips to a hive named `alpha-agent`), so a + // hive-first arm order hands every agent its hive's grant, and nothing + // reports it — the client authenticates and is merely able to do more + // than it should. Together with the `assert_eq` in + // `an_agent_publishes_inside_its_own_hives_namespace` (which pins the + // subject to `alpha`, not `alpha-agent`) this is what holds the + // ordering in place. let g = policy_with_agent_subject() - .permissions("agent-alpha") + .permissions("hive-alpha-agent") .expect("an agent is admitted"); assert!(!g.publish.iter().any(|s| s.starts_with("$KV."))); assert!(!g.publish.iter().any(|s| s.starts_with("$JS.API."))); @@ -1088,7 +1123,7 @@ mod tests { fn an_agent_subject_without_the_placeholder_is_refused() { let err = Policy::new( "hive-".to_owned(), - "agent-".to_owned(), + "-agent".to_owned(), "hive-status".to_owned(), Vec::new(), Vec::new(), @@ -1102,27 +1137,42 @@ mod tests { } #[test] - fn overlapping_prefixes_are_refused() { - // 🩸 The arms are tried in order, so an overlap does not error at match - // time - it hands one principal the other's grant. Both directions, - // because which one wins depends only on the order above. - for (hive, agent) in [("hive-", "hive-agent-"), ("agent-x-", "agent-"), ("h", "h")] { - Policy::new( - hive.to_owned(), - agent.to_owned(), - "hive-status".to_owned(), - Vec::new(), - Vec::new(), - Vec::new(), - ) - .expect_err("prefixes where one contains the other must not construct"); - } + fn an_empty_agent_suffix_is_refused() { + // 🩸 An agent id is a hive id plus the suffix, so an empty suffix makes + // `strip_suffix` succeed on every hive id: the two principals become + // one string, and whichever arm runs first answers for both. That is a + // GRANT, not a parse error, which is why it has to fail at + // construction. + Policy::new( + "hive-".to_owned(), + String::new(), + "hive-status".to_owned(), + Vec::new(), + Vec::new(), + Vec::new(), + ) + .expect_err("an empty agent suffix must not construct"); } #[test] - fn the_agent_prefix_alone_names_no_hive() { - // Same trap as `the_prefix_alone_names_no_hive`: `agent-` with nothing - // after it would expand `{hive}` to the empty string. - assert!(policy_with_agent_subject().permissions("agent-").is_none()); + fn the_agent_suffix_alone_names_no_hive() { + // `hive--agent` is the prefix and the suffix with nothing between them. + // The agent arm must refuse it rather than expand `{hive}` to the empty + // string and hand out `$SWARM.term..>`. + // + // 🩸 Written first as `is_none()`, by analogy with + // `the_prefix_alone_names_no_hive`, and it FAILED — correctly. The + // analogy does not hold: under the hive parse the same id is a hive + // named `-agent`, which is a non-empty name this responder cannot rule + // out, because it has no roster. `swarm-authelia.nix` refuses such a + // name at eval, where the roster is known. So the assertion here is + // narrowed to the only part this module owns. + let g = policy_with_agent_subject().permissions("hive--agent"); + assert!( + !g.iter() + .flat_map(|p| &p.publish) + .any(|s| s.contains("$SWARM.term..")), + "an empty hive name must never be expanded into a subject: {g:?}" + ); } }