From 88e571a46329b9c345cea9ec4c7b497bf4153ef6 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 24 Sep 2026 14:30:31 +0200 Subject: [PATCH] swarm-controller: refuse a new agent name the forge would reject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every agent becomes a Forgejo user of the same name, and nothing upstream of `CreateForgeUser` knew what Forgejo refuses: `admin`, `api`, `foo-` or a 41-character name passed name validation and failed one node into provisioning with Forgejo's 422. `nix/reserved-names.nix` gains the 22 reserved usernames of Forgejo v16.0.5 (`models/user/user.go:639-680`) that `[a-z0-9-]` can spell, and the bare `-` (`models/repo/repo.go:67`). The dot and underscore entries are left out, since our charset cannot produce them. The header's admission rule grows a third class — a username the forge refuses — because that is a failure behind the refusal. The shape rules are not literals, so they live in `hive_types::forge_username_violation`: no leading `-`, no `--`, no trailing `-`, at most 40 characters. Beside `is_reserved_name`, not in `Ident::parse`: an `Ident` is also a hive, label, account and subagent name, and parsing runs on every read of an existing name. `create_agent` used to WARN on a reserved name, deliberately: an operator with agents already created under a colliding name would otherwise be unable to re-run creation. That reason is kept, and narrowed to what it protects. A name breaking either rule is now refused with a 400 naming the rule when the name is NOT in the swarm roster, and still only warned about when it is, so re-creating an existing agent keeps working. The roster is read only for a rule-breaking name; when it cannot be read, a new name and an existing one look alike, and this warns as before. The hive-collision warning is unchanged. --- hive-types/src/lib.rs | 64 ++++++++- nix/reserved-names.nix | 37 ++++- swarm-controller/src/main.rs | 269 ++++++++++++++++++++++++++++++----- 3 files changed, 327 insertions(+), 43 deletions(-) diff --git a/hive-types/src/lib.rs b/hive-types/src/lib.rs index d1a51508..90735cfa 100644 --- a/hive-types/src/lib.rs +++ b/hive-types/src/lib.rs @@ -64,6 +64,35 @@ pub fn is_reserved_name(name: &str, reserved: &[&str]) -> bool { reserved.contains(&name) } +/// Forgejo's cap on a username (`MaxSize(40)` on the admin create-user body, +/// `modules/structs/admin_user.go:14` in v16.0.5) — tighter than +/// [`Ident::MAX_LEN`], so an agent name can be a valid [`Ident`] and still +/// never get a forge account. +pub const FORGE_USERNAME_MAX_LEN: usize = 40; + +/// The Forgejo username shape rule an [`Ident`]-valid `name` breaks, or +/// `None`. Forgejo v16.0.5 `modules/validation/helpers.go:97-108`. +/// +/// The shape half of the forge's refusals; its reserved literals are in +/// `nix/reserved-names.nix`. Creation sites only, for the reason given at +/// [`is_reserved_name`] — and not inside [`Ident::parse`], because an +/// `Ident` is also a hive, label, account and subagent name that never +/// becomes a forge user. +#[must_use] +pub fn forge_username_violation(name: &str) -> Option<&'static str> { + if name.starts_with('-') { + Some("must not start with '-'") + } else if name.contains("--") { + Some("must not contain '--'") + } else if name.ends_with('-') { + Some("must not end with '-'") + } else if name.len() > FORGE_USERNAME_MAX_LEN { + Some("must be 40 characters or fewer") + } else { + None + } +} + /// A validated hive identifier: 1-63 chars of `[a-z0-9-]`. /// /// The single ident type for agent names, forge labels, and matrix / github @@ -156,7 +185,10 @@ impl<'de> serde::Deserialize<'de> for Ident { #[cfg(test)] mod ident_tests { - use super::{Ident, is_reserved_name, parse_reserved_names}; + use super::{ + FORGE_USERNAME_MAX_LEN, Ident, forge_username_violation, is_reserved_name, + parse_reserved_names, + }; #[test] fn accepts_canonical_shapes() { @@ -240,6 +272,36 @@ mod ident_tests { } } + /// Each Forgejo shape rule is named on its own, so a refusal tells the + /// operator which character to change. The 63-char case is an `Ident` + /// that is still no forge username: the gap between the two caps. + #[test] + fn forge_shape_rules_are_each_refused() { + let over = "a".repeat(FORGE_USERNAME_MAX_LEN + 1); + let longest_ident = "a".repeat(Ident::MAX_LEN); + for (bad, rule) in [ + ("-", "must not start with '-'"), + ("-agent", "must not start with '-'"), + ("my--agent", "must not contain '--'"), + ("agent-", "must not end with '-'"), + (over.as_str(), "must be 40 characters or fewer"), + (longest_ident.as_str(), "must be 40 characters or fewer"), + ] { + assert!(Ident::parse(bad).is_ok(), "{bad:?} must be a valid Ident"); + assert_eq!(forge_username_violation(bad), Some(rule), "{bad:?}"); + } + } + + /// The accept arm: without it, a predicate refusing everything passes + /// the test above. 40 characters is the boundary, not past it. + #[test] + fn forge_shape_rules_accept_ordinary_names() { + let at_cap = "a".repeat(FORGE_USERNAME_MAX_LEN); + for ok in ["my-agent", "a", "v3", "a-b-c", at_cap.as_str()] { + assert_eq!(forge_username_violation(ok), None, "{ok:?}"); + } + } + #[test] fn round_trips_and_serde_validates() { let id = Ident::parse("damocles").unwrap(); diff --git a/nix/reserved-names.nix b/nix/reserved-names.nix index bfc26857..70d9b449 100644 --- a/nix/reserved-names.nix +++ b/nix/reserved-names.nix @@ -22,9 +22,9 @@ # here forbids it for both. That is the point: two lists is how they drift. # # ⚠️ Every entry must be a value some component actually PRODUCES as a message -# `from`/`to`, or a component name the collector builds pipelines from — not a -# word that merely looked risky. A name in here that nothing emits is a refusal -# with no failure behind it. +# `from`/`to`, a component name the collector builds pipelines from, or a +# username the forge refuses — not a word that merely looked risky. A name in +# here that nothing emits or refuses is a refusal with no failure behind it. # # ⚠️ Matched by EQUALITY. Words forbidden *inside* a hive name live in # `./reserved-hive-fragments.nix` — read its header before merging the two. @@ -59,6 +59,37 @@ # pushes into a pipeline that routes nowhere. Previously enforced only # against hive names, in `swarm-otel.nix`'s own `reservedOwners`. "swarm" + + # ---- usernames the forge refuses ------------------------------------- + # Every agent becomes a Forgejo user of the same name, and Forgejo answers + # these with a 422 one node into provisioning: `reservedUsernames` in + # Forgejo v16.0.5 `models/user/user.go:639-680`, and the bare `-` that + # `models/repo/repo.go:67` also refuses as a repo name. Forgejo's shape + # rules are not literals and live in `hive_types::forge_username_violation`. + # Its entries containing `.` or `_` are left out: `[a-z0-9-]` cannot spell them. + "-" + "admin" + "api" + "assets" + "attachments" + "avatar" + "avatars" + "captcha" + "explore" + "forgejo-actions" + "ghost" + "gitea-actions" + "issues" + "login" + "metrics" + "milestones" + "notifications" + "org" + "pulls" + "repo" + "repo-avatars" + "user" + "v2" ] # Deliberately absent, and it is a load-bearing omission: # diff --git a/swarm-controller/src/main.rs b/swarm-controller/src/main.rs index bc22d654..6ebb4c40 100644 --- a/swarm-controller/src/main.rs +++ b/swarm-controller/src/main.rs @@ -560,10 +560,12 @@ struct AppState { /// one are different facts. /// /// ⚠️ The two `/api/agents` verbs treat this differently on purpose. - /// **POST does not consult it**: creation only queues a job, and + /// **POST does not need it**: creation only queues a job, and /// whether a bridge exists is `run_swarm_node`'s concern (it holds its /// own clone from `spawn_jobq_worker`), so a request still queues - /// cleanly on a bridge-less host and fails loud once claimed. **GET + /// cleanly on a bridge-less host and fails loud once claimed. POST reads + /// it only for a name that breaks a naming rule, to tell a new agent + /// from an existing one, and warns when it cannot (`name_verdict`). **GET /// has nowhere to defer to** — there is no job, only an answer it /// either has or does not. auth: Option>, @@ -1199,7 +1201,7 @@ struct CreateAgentResponse { request_body = CreateAgentRequest, responses( (status = 200, description = "job chain queued", body = CreateAgentResponse), - (status = 400, description = "`name` or `hive` is not a valid identifier, or `hive` is not in this swarm (problem+json)", body = String), + (status = 400, description = "`name` or `hive` is not a valid identifier, `name` is new and reserved or refused by the forge, or `hive` is not in this swarm (problem+json)", body = String), (status = 500, description = "the job chain could not be queued (problem+json)", body = String), ), tag = "agents" @@ -1222,43 +1224,24 @@ async fn create_agent( // `state.hives` is the swarm directory loaded from `SWARM_CONTROLLER_HIVES` // at startup and never mutated, so this is a scan of a handful of // entries against a value an operator just chose. - // The name is shaped like an identifier; these two ask whether it is - // *available*. Both WARN rather than refuse: an operator with agents - // already created under a colliding name would otherwise be unable to - // re-run creation at all, so the refusal comes after a grace period, - // once the warning has had time to be seen. + // The name is shaped like an identifier; the checks below ask whether it + // is *available*. An operator with agents already created under a + // colliding name must still be able to re-run creation, so a name that + // breaks a naming rule is refused only when it is new — see + // `name_verdict`. The hive collision further down still only warns, on + // its grace period, once the warning has had time to be seen. // // Collected rather than logged-and-dropped — see `CreateAgentResponse`. let mut warnings = Vec::new(); - // The blacklist comes from nix via `HIVE_RESERVED_NAMES` — one file, read - // by this daemon, by hive-c0re and by the swarm collector's own owner - // assertion. An UNSET variable means this process was never told, which - // is not the same as "no name is reserved": staying quiet there would be - // a check that reports clean because it could not run. - let raw = hive_types::reserved_names_raw(); - match raw.as_deref().map(hive_types::parse_reserved_names) { - None => { - tracing::error!( - var = hive_types::RESERVED_NAMES_ENV, - "create_agent: reserved-name check could not run — variable not set" - ); - warnings.push(format!( - "the reserved-name check did not run: {} is unset, so {agent:?} was accepted \ - without being checked against the protocol literals", - hive_types::RESERVED_NAMES_ENV - )); - } - Some(reserved) if hive_types::is_reserved_name(&agent, &reserved) => { - // A protocol literal: the message layer already produces this - // name as a sender or recipient, so wakes from the component and - // messages from the agent become the same broker row. - tracing::warn!(agent = %agent, "create_agent: reserved name"); - warnings.push(format!( - "agent name {agent:?} is a reserved protocol name — messages from this agent will \ - be indistinguishable from hyperhive's own; this will become an error" - )); - } - Some(_) => {} + let broken = broken_name_rules( + &agent, + hive_types::reserved_names_raw().as_deref(), + &mut warnings, + ); + if !broken.is_empty() { + let existing = in_roster(state.auth.as_deref(), &agent).await; + name_verdict(&agent, &broken, existing, &mut warnings) + .map_err(|detail| error_problem(axum::http::StatusCode::BAD_REQUEST, &detail))?; } let hive = hive_types::Ident::parse(&req.hive) @@ -1312,6 +1295,107 @@ async fn create_agent( })) } +/// Every naming rule `agent` breaks, each as a sentence naming the rule. +/// +/// `reserved` is [`hive_types::reserved_names_raw`]: the list nix owns in +/// `nix/reserved-names.nix`, read by this daemon, by hive-c0re and by the +/// swarm collector's own owner assertion. `None` means this process was never +/// told, which is not the same as "no name is reserved" — staying quiet there +/// would be a check that reports clean because it could not run, so it goes +/// onto `warnings` instead. +fn broken_name_rules( + agent: &str, + reserved: Option<&str>, + warnings: &mut Vec, +) -> Vec { + let mut broken = Vec::new(); + match reserved.map(hive_types::parse_reserved_names) { + None => { + tracing::error!( + var = hive_types::RESERVED_NAMES_ENV, + "create_agent: reserved-name check could not run — variable not set" + ); + warnings.push(format!( + "the reserved-name check did not run: {} is unset, so {agent:?} was accepted \ + without being checked against the reserved names", + hive_types::RESERVED_NAMES_ENV + )); + } + Some(reserved) if hive_types::is_reserved_name(agent, &reserved) => { + broken.push(format!( + "agent name {agent:?} is reserved (nix/reserved-names.nix): hyperhive's message \ + layer already uses it, or the forge refuses it as a username" + )); + } + Some(_) => {} + } + if let Some(rule) = hive_types::forge_username_violation(agent) { + broken.push(format!( + "agent name {agent:?} {rule}: the forge refuses it as a username" + )); + } + broken +} + +/// Whether `agent` is already in the swarm roster, or why that could not be +/// told. Asked only of a name that breaks a rule, so an ordinary creation +/// still never waits on the bridge. +async fn in_roster(auth: Option<&auth::AuthBridge>, agent: &str) -> Result { + let Some(auth) = auth else { + return Err("no identity bridge is configured".to_owned()); + }; + auth.list_agent_identities() + .await + .map(|roster| roster.iter().any(|name| name == agent)) + .map_err(|e| format!("{e:#}")) +} + +/// Refuse a NEW agent whose name breaks a rule; warn for one that exists. +/// +/// Re-creating an agent under its own name is a migration path (mara, on +/// the duplicate-name refusal), so an agent already in the roster keeps its +/// name with a warning. When the roster cannot be read, a new name and an +/// existing one look the same, and this warns rather than refuse: refusing +/// would block that re-creation, and the forge still refuses the account one +/// node in, as it did before. +/// +/// # Errors +/// Every rule `agent` breaks, as one caller-ready sentence, when `agent` is +/// not in the roster. +fn name_verdict( + agent: &str, + broken: &[String], + existing: Result, + warnings: &mut Vec, +) -> Result<(), String> { + match existing { + Ok(false) => Err(format!("{}; choose another name", broken.join("; "))), + Ok(true) => { + tracing::warn!(agent, "create_agent: existing agent breaks a naming rule"); + warnings.extend( + broken + .iter() + .map(|rule| format!("{rule} — accepted only because {agent:?} already exists")), + ); + Ok(()) + } + Err(why) => { + tracing::warn!( + agent, + why, + "create_agent: naming rule broken, roster unreadable" + ); + warnings.extend(broken.iter().map(|rule| { + format!( + "{rule} — not refused, because whether {agent:?} already exists could not \ + be checked: {why}" + ) + })); + Ok(()) + } + } +} + /// The authority agent leaves are issued from, or `None` on a host that was /// given none. /// @@ -2078,8 +2162,8 @@ mod tests { config_prs: None, swarm_name: None, // No bridge: these tests drive agent *creation*, which queues a - // job and never consults one. The roster read is the verb that - // needs it, and it has its own test below. + // job and consults one only for a rule-breaking name. The roster + // read is the verb that needs it, and it has its own test below. auth: None, forge: None, }; @@ -2191,6 +2275,113 @@ mod tests { assert!(queued > 0, "an accepted creation must queue work"); } + /// Forgejo v16.0.5's reserved usernames our charset can spell, plus the + /// bare `-` — the forge section of `nix/reserved-names.nix`. Read from + /// the list nix exports, so dropping one from the file reds this. + const FORGE_REFUSED_LITERALS: [&str; 23] = [ + "-", + "admin", + "api", + "assets", + "attachments", + "avatar", + "avatars", + "captcha", + "explore", + "forgejo-actions", + "ghost", + "gitea-actions", + "issues", + "login", + "metrics", + "milestones", + "notifications", + "org", + "pulls", + "repo", + "repo-avatars", + "user", + "v2", + ]; + + /// The list nix renders, panicking when absent for the reason + /// `hive-sh4re`'s drift test gives: a skip would read as green. + fn nix_reserved() -> String { + hive_types::reserved_names_raw() + .expect("HIVE_RESERVED_NAMES is exported by nix/checks.nix and nix/devshell.nix") + } + + #[test] + fn a_new_agent_named_a_forge_refused_literal_is_refused() { + let raw = nix_reserved(); + for name in FORGE_REFUSED_LITERALS { + let mut warnings = Vec::new(); + let broken = super::broken_name_rules(name, Some(&raw), &mut warnings); + let err = super::name_verdict(name, &broken, Ok(false), &mut warnings).expect_err(name); + assert!(err.contains("nix/reserved-names.nix"), "{name:?}: {err}"); + } + } + + /// One case per shape rule, each named in the refusal. Checked against a + /// list without them, so the literal half cannot carry this test. + #[test] + fn a_new_agent_breaking_a_forge_shape_rule_is_refused() { + let over = "a".repeat(hive_types::FORGE_USERNAME_MAX_LEN + 1); + for (name, rule) in [ + ("-agent", "must not start with '-'"), + ("my--agent", "must not contain '--'"), + ("agent-", "must not end with '-'"), + (over.as_str(), "must be 40 characters or fewer"), + ] { + let mut warnings = Vec::new(); + let broken = super::broken_name_rules(name, Some("operator"), &mut warnings); + let err = super::name_verdict(name, &broken, Ok(false), &mut warnings).expect_err(name); + assert!(err.contains(rule), "{name:?}: {err}"); + } + } + + /// The control for both tests above, against the real list. + #[test] + fn an_ordinary_name_and_a_40_char_name_break_no_rule() { + let raw = nix_reserved(); + let at_cap = "a".repeat(hive_types::FORGE_USERNAME_MAX_LEN); + for name in ["my-agent", at_cap.as_str()] { + let mut warnings = Vec::new(); + let broken = super::broken_name_rules(name, Some(&raw), &mut warnings); + assert!(broken.is_empty(), "{name:?}: {broken:?}"); + assert!(warnings.is_empty(), "{name:?}: {warnings:?}"); + } + } + + /// Re-creating an agent under its own name must keep working: a name + /// already in the roster is warned about, never refused. + #[test] + fn an_existing_agent_keeps_a_rule_breaking_name_with_a_warning() { + let mut warnings = Vec::new(); + let broken = super::broken_name_rules("admin", Some("admin"), &mut warnings); + super::name_verdict("admin", &broken, Ok(true), &mut warnings) + .expect("an existing agent must not be refused"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("already exists"), "{warnings:?}"); + } + + /// A roster that cannot be read cannot tell new from existing, so it + /// warns — and the warning says the check did not decide. + #[test] + fn an_unreadable_roster_warns_rather_than_refusing() { + let mut warnings = Vec::new(); + let broken = super::broken_name_rules("agent-", Some("operator"), &mut warnings); + super::name_verdict( + "agent-", + &broken, + Err("bridge down".to_owned()), + &mut warnings, + ) + .expect("an unreadable roster must not refuse"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("bridge down"), "{warnings:?}"); + } + /// The backfill route's roster check, asserted by effect for the same /// reason its sibling above is: a refusal that queued first would still /// re-mint the agent's certificate, which every running agent on the