From 3b038425f2ff63c80699daa1b710d0144bcc40b0 Mon Sep 17 00:00:00 2001 From: atlas Date: Wed, 2 Sep 2026 19:56:16 +0200 Subject: [PATCH] fix(#3987): join the agent-status key with a dot so a hive's write grant can be scoped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A KV entry publishes to $KV.. and NATS wildcards match whole .-delimited tokens, so a /-joined {hive}/{agent} key is a single token: the only expressible write grants are one exact subject per agent (needs a roster in the auth responder, which Policy::hive_name argues against) or a bucket-wide wildcard that lets any hive overwrite any other hive's agents. Joining with a dot puts the hive in its own token, so hive_subjects can grant $KV.agent-status..* — every agent of one hive and nothing else, the same containment hive-status already has. The grant lands with the bucket-open pair (STREAM.INFO + STREAM.CREATE): open_or_create resolves the bucket before it writes, so alone the publish subject is unreachable and the sweep fails one step later instead. Verified the client accepts a dotted key rather than assuming it: async-nats 0.50.0 VALID_KEY_RE is \A[-/_=.a-zA-Z0-9]+\z and is_valid_key rejects only empty / leading / trailing dot; the subject is prefix + key verbatim. --- swarm-nats-auth/src/policy.rs | 91 ++++++++++++++++++++------ swarm-queue-client/src/agent_status.rs | 52 +++++++++++---- 2 files changed, 109 insertions(+), 34 deletions(-) diff --git a/swarm-nats-auth/src/policy.rs b/swarm-nats-auth/src/policy.rs index 935e2da3..f58fc1ab 100644 --- a/swarm-nats-auth/src/policy.rs +++ b/swarm-nats-auth/src/policy.rs @@ -271,6 +271,31 @@ impl Policy { swarm_queue_client::wanted::BUCKET ), ]); + // Publishing this hive's agents into the per-agent status bucket, and + // **only its own agents**. + // + // `.*` matches exactly one token, so this is every agent of one hive + // and nothing deeper — `.>` would also admit `..`, + // which `agent_status::split_key` rejects on read anyway; the grant has + // no reason to be wider than the format. + // + // The scoping is only expressible because the key is `.`-joined: a KV + // entry publishes to `$KV..`, and a NATS wildcard matches + // whole tokens, so a key joined by anything else is one token and + // leaves only "one exact subject per agent" (needs a roster in here, + // which `hive_name`'s doc argues against) or a bucket-wide wildcard + // (any hive overwrites any other hive's agents). + // + // `STREAM.INFO` + `CREATE` come along for the same reason they do + // above: `open_or_create` resolves the bucket before it writes, and + // either end may be first on a fresh swarm. Without them the publish + // grant is unreachable — the client times out at bucket open. + let agent_status = Self::agent_status_stream(); + subjects.extend([ + format!("$JS.API.STREAM.INFO.{agent_status}"), + format!("$JS.API.STREAM.CREATE.{agent_status}"), + format!("$KV.{}.{hive}.*", swarm_queue_client::agent_status::BUCKET), + ]); subjects.extend( self.extra_hive_subjects .iter() @@ -338,12 +363,10 @@ impl Policy { // ephemeral consumer whose subject carries no name. // // Deliberately **no `$KV..…` subject**: that is the write - // side, and writing is what picks a key layout. Who publishes agent - // status, and under which key, is still open — so granting a write - // subject here would answer a question this change has no business - // answering. Reading needs none of it: a KV read is a `DIRECT.GET`, - // and the `.>` form is bucket-wide rather than per-key, so nothing - // below encodes a layout. + // side, and the controller only reads this bucket. The hive gets a + // write grant scoped to its own agents (see `hive_subjects`); a + // reader holding one could forge any agent's status on any hive. + // Reading needs none of it — a KV read is a `DIRECT.GET`. let agent_status = Self::agent_status_stream(); subjects.extend([ format!("$JS.API.STREAM.INFO.{agent_status}"), @@ -391,26 +414,54 @@ mod tests { } #[test] - fn no_grant_encodes_an_agent_status_key_layout() { - // Who writes agent status, and under which key, is undecided. A `$KV.` - // subject is the write side and would answer that by implication, so - // NOBODY gets one for this bucket until the layout is settled. + fn the_reader_gets_no_write_subject_for_the_agent_status_bucket() { + // The controller reads this bucket and never writes it. A `$KV.` + // subject here would let a reader forge any agent's status on any + // hive — the containment `hive_subjects` establishes is only worth + // anything if the other role cannot bypass it. let bucket = swarm_queue_client::agent_status::BUCKET; let prefix = format!("$KV.{bucket}."); - let p = policy(); - for (who, subjects) in [ - ("reader", p.reader_subjects()), - ("hive", p.hive_subjects("alpha")), + let subjects = policy().reader_subjects(); + assert!( + !subjects.iter().any(|s| s.starts_with(&prefix)), + "the reader was granted a {prefix}* subject" + ); + // Control: the prefix test does fire on a bucket the reader + // legitimately writes, so the pass above is about agent-status rather + // than about a matcher that never matches anything. + let wanted = format!("$KV.{}.", swarm_queue_client::wanted::BUCKET); + assert!(subjects.iter().any(|s| s.starts_with(&wanted))); + } + + #[test] + fn a_hive_may_write_only_its_own_agents_status_keys() { + let bucket = swarm_queue_client::agent_status::BUCKET; + let subjects = policy().hive_subjects("alpha"); + let s = Policy::agent_status_stream(); + for want in [ + // Without these the write subject is unreachable: `open_or_create` + // resolves the bucket first and times out before it publishes. + format!("$JS.API.STREAM.INFO.{s}"), + format!("$JS.API.STREAM.CREATE.{s}"), + format!("$KV.{bucket}.alpha.*"), + ] { + assert!(subjects.contains(&want), "hive alpha is missing {want}"); + } + // The scoping is the point, so pin what must NOT be there: a + // bucket-wide wildcard in any of its spellings, and another hive's + // slice. `.*` matches one token; `.>` would match deeper than the key + // format ever produces. + for forbidden in [ + format!("$KV.{bucket}.>"), + format!("$KV.{bucket}.*"), + format!("$KV.{bucket}.alpha.>"), + format!("$KV.{bucket}.beta.*"), ] { assert!( - !subjects.iter().any(|s| s.starts_with(&prefix)), - "{who} was granted a {prefix}* subject, which picks a key layout" + !subjects.contains(&forbidden), + "hive alpha was granted {forbidden}, which reaches past its own agents" ); } - // Control: the prefix test does fire on a bucket that legitimately has - // write grants, so a pass above is about agent-status, not the matcher. - let wanted = format!("$KV.{}.", swarm_queue_client::wanted::BUCKET); - assert!(p.reader_subjects().iter().any(|s| s.starts_with(&wanted))); } #[test] diff --git a/swarm-queue-client/src/agent_status.rs b/swarm-queue-client/src/agent_status.rs index c318bb60..9311fc6b 100644 --- a/swarm-queue-client/src/agent_status.rs +++ b/swarm-queue-client/src/agent_status.rs @@ -34,10 +34,19 @@ pub const BUCKET: &str = "agent-status"; /// The bucket key for one agent on one hive. /// -/// `/`-joined rather than NATS's usual `.`-joined subject style: this is a -/// KV key, not a subject, and neither `hive` nor `agent` names can contain -/// `/` (`hive_types::Ident`'s charset is `[a-z0-9-]`), so the join is -/// unambiguous to split back apart if a consumer ever needs to. +/// `.`-joined, and the separator is load-bearing rather than cosmetic: a KV +/// entry is published to `$KV..`, and NATS wildcards match +/// whole `.`-delimited tokens. Two tokens make a hive's write grant +/// expressible as `$KV.agent-status..*` — every agent of one hive and +/// nothing else. Joined by any character that is not `.`, the key is a +/// single token, and the only grants available are one exact subject per +/// agent or a bucket-wide wildcard that lets any hive overwrite any other +/// hive's agents. +/// +/// Neither name can be empty or contain a `.` (`hive_types::Ident` is +/// `[a-z0-9-]`, non-empty), so the join stays unambiguous to split back +/// apart — and a leading or trailing `.` would be rejected outright by the +/// client's own KV key validation. /// /// Plain code span above, deliberately not an intra-doc link — this crate /// doesn't depend on `hive_types`, and a bracketed reference to its `Ident` @@ -46,20 +55,20 @@ pub const BUCKET: &str = "agent-status"; /// errors. #[must_use] pub fn key(hive: &str, agent: &str) -> String { - format!("{hive}/{agent}") + format!("{hive}.{agent}") } /// The inverse of [`key`]: split a bucket key back into `(hive, agent)`. /// -/// `None` for a key with zero or more than one `/` — a hive publishing +/// `None` for a key with zero or more than one `.` — a hive publishing /// under [`key`] never produces one, so a malformed key means something -/// else wrote this bucket. Splitting on the *first* `/` would be equally +/// else wrote this bucket. Splitting on the *first* `.` would be equally /// valid today (neither name can contain one), but this rejects rather /// than guesses, so a future name-charset change can't silently start /// misreading old keys. #[must_use] pub fn split_key(key: &str) -> Option<(&str, &str)> { - let mut parts = key.splitn(3, '/'); + let mut parts = key.splitn(3, '.'); let hive = parts.next()?; let agent = parts.next()?; if parts.next().is_some() { @@ -125,11 +134,26 @@ pub async fn open_or_create( #[cfg(test)] mod tests { - use super::{AgentStatus, key, split_key}; + use super::{AgentStatus, BUCKET, key, split_key}; #[test] - fn key_joins_hive_and_agent_with_a_slash() { - assert_eq!(key("prod", "iris"), "prod/iris"); + fn key_joins_hive_and_agent_with_a_dot() { + assert_eq!(key("prod", "iris"), "prod.iris"); + } + + #[test] + fn the_published_subject_carries_the_hive_as_its_own_token() { + // This is what the hive's write grant `$KV.agent-status..*` + // matches on, so the separator is pinned here rather than left as a + // property of `key`'s formatting string. + assert_eq!( + format!("$KV.{BUCKET}.{}", key("prod", "iris")), + "$KV.agent-status.prod.iris" + ); + // Control: a single-token join lands in the same bucket and would + // pass any assertion that only checked the prefix — it is the hive + // token that the grant needs, and only the `.` produces one. + assert!(!format!("$KV.{BUCKET}.prod/iris").starts_with("$KV.agent-status.prod.")); } #[test] @@ -138,13 +162,13 @@ mod tests { } #[test] - fn split_key_rejects_a_key_with_no_slash() { + fn split_key_rejects_a_key_with_no_dot() { assert_eq!(split_key("iris"), None); } #[test] - fn split_key_rejects_a_key_with_two_slashes() { - assert_eq!(split_key("prod/iris/extra"), None); + fn split_key_rejects_a_key_with_two_dots() { + assert_eq!(split_key("prod.iris.extra"), None); } #[test]