From 58f50ed50694f9a56f81b62d9bfdf349ac279613 Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 24 Sep 2026 15:11:23 +0200 Subject: [PATCH] swarm-controller: guard the lockdown PATCH against a forge name collision disable_repo_creation now reads the account once before PATCHing max_repo_creation/source_id, and fails the node if the email isn't the {agent}@hyperhive.local marker create_agent_user itself sets. The 409/422 create-fold (#4681) only proves some account with that name exists, not that this node created it, so a pre-existing non-agent account sharing an agent's chosen name could otherwise get locked onto local auth with repo creation disabled. Leaves the fold untouched (Option B, per atlas/argus on #4693); the read moves into disable_repo_creation instead. Also fixes the nix-sandboxed cargo-test check: forgejo_api::Forgejo::new builds a reqwest client that eagerly resolves TLS roots via rustls-native-certs even for the tests' plain-http loopback stub server, which panics with "No CA certificates were loaded from the system" in the CA-less build sandbox. Gives that check's nativeBuildInputs pkgs.cacert and sets SSL_CERT_FILE, same pattern this repo's runtime deployment already uses for the same reqwest/rustls resolution. --- nix/checks.nix | 19 ++++- swarm-controller/src/forge.rs | 137 +++++++++++++++++++++++++++++++--- 2 files changed, 144 insertions(+), 12 deletions(-) diff --git a/nix/checks.nix b/nix/checks.nix index 706fba9e..c5238637 100644 --- a/nix/checks.nix +++ b/nix/checks.nix @@ -156,7 +156,24 @@ in # per-container toplevels stay fully cached. cargo-test = craneLib.cargoTest { src = cleanSrc; - inherit cargoArtifacts nativeBuildInputs; + inherit cargoArtifacts; + # `swarm-controller`'s forge tests build a real `forgejo_api::Forgejo` + # (hence a real `reqwest::Client`) against a loopback stub server, never + # the real forge — but `reqwest`/`rustls` still resolves roots through + # `rustls-native-certs` at *client-build* time, unconditionally, even + # though every request that client ever sends is plain `http://` to + # `127.0.0.1`. The nix build sandbox has no system CA store, so that + # resolution finds zero certs and `ClientBuilder::build()` itself fails + # with "No CA certificates were loaded from the system" — the tests + # never get as far as making a request. Same root cause the runtime + # deployment already works around (see `nix/host-modules/swarm- + # controller.nix`'s `caTrust` comment) and the same fix: give + # `rustls-native-certs` something to find via `SSL_CERT_FILE`. The + # bundle's actual contents don't matter here — nothing in these tests + # ever presents or verifies a real certificate — only that the store is + # non-empty. + nativeBuildInputs = nativeBuildInputs ++ [ pkgs.cacert ]; + SSL_CERT_FILE = "${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt"; pname = "hyperhive-workspace"; version = "0.1.0"; cargoTestExtraArgs = "--workspace"; diff --git a/swarm-controller/src/forge.rs b/swarm-controller/src/forge.rs index 8ec3a112..f1b4c7a3 100644 --- a/swarm-controller/src/forge.rs +++ b/swarm-controller/src/forge.rs @@ -399,11 +399,35 @@ impl Client { /// the operator-only-merge policy. Existing repos are untouched; push, /// PRs and clone keep working. /// - /// Unconditional rather than verify-then-patch: Forgejo's API never - /// returns `max_repo_creation` (it is a field of `EditUserOption` only, - /// not of `User`), so there is no current value to compare against. - /// Setting 0 on an account already at 0 is a no-op. + /// The PATCH itself is unconditional rather than verify-then-patch: + /// Forgejo's API never returns `max_repo_creation` (it is a field of + /// `EditUserOption` only, not of `User`), so there is no current value + /// to compare against, and setting 0 on an account already at 0 is a + /// no-op. But `create_agent_user`'s idempotent fold only proves *some* + /// account named `agent` exists — a 409 folds with no read at all, and + /// a 422's follow-up `user_get` only confirms the username, not that + /// swarm-controller created it — so before that PATCH this reads the + /// account once and checks the one thing that *is* comparable: the + /// email `create_agent_user` sets at creation (see [`agent_email`]), + /// the same marker `hive-c0re`'s legacy `ensure_user_exists` / + /// `ensure_user_email` set and converge to. A mismatch means the name + /// collided with an account this node didn't create, so it fails the + /// node rather than sending the PATCH (or silently skipping) to an + /// account nobody verified is agent-controlled. async fn disable_repo_creation(&self, agent: &str) -> Result<()> { + let user = self + .api + .user_get(agent) + .await + .with_context(|| format!("read forge user {agent} before locking it down"))?; + let expected = agent_email(agent); + if user.email.as_deref() != Some(expected.as_str()) { + anyhow::bail!( + "forge account `{agent}` exists but is not this agent's (email {:?}, expected \ + `{expected}`); refusing to lock it down", + user.email + ); + } self.api .admin_edit_user(agent, repo_creation_lockdown(agent)) .await @@ -423,7 +447,7 @@ impl Client { .api .admin_create_user(CreateUserOption { created_at: None, - email: format!("{agent}@hyperhive.local"), + email: agent_email(agent), full_name: None, login_name: None, must_change_password: Some(false), @@ -1109,6 +1133,19 @@ fn initial_flake_nix() -> &'static str { "{\n description = \"hyperhive agent\";\n inputs = { };\n outputs =\n { self, ... }@inputs:\n {\n nixosModules.default = {\n imports = [ ./agent.nix ];\n _module.args.flakeInputs = builtins.removeAttrs inputs [ \"self\" ];\n };\n };\n}\n" } +/// Canonical email for an agent's Forgejo account, set by +/// [`Client::create_agent_user`] at creation and checked by +/// [`Client::disable_repo_creation`] before it sends the account's PATCH. Same +/// format string as `hive-c0re::forge::users::agent_email` — not called +/// across the crate boundary (see this module's doc comment), but both +/// creators (and `hive-c0re`'s `ensure_user_email`, which aligns older +/// accounts to it) converge on this exact value, so it doubles as the +/// marker that distinguishes an agent's own account from an unrelated one +/// that happens to share its username. +fn agent_email(agent: &str) -> String { + format!("{agent}@hyperhive.local") +} + /// The `admin_edit_user` body that sets `max_repo_creation = 0` on `agent` /// and nothing else. Same shape as `hive-c0re::forge::users`' /// `sparse_edit_user_option`: `login_name` + `source_id = 0` (local auth, @@ -1389,9 +1426,14 @@ mod tests { type Seen = std::sync::Arc>>; /// A stand-in forge on a loopback port — never the real one — answering - /// the admin user create (`POST`) with `create` and the admin user edit - /// (`PATCH`) with `edit` (status, JSON body), and recording every request. - async fn stub_forge(create: (u16, &'static str), edit: (u16, &'static str)) -> (Client, Seen) { + /// the admin user create (`POST`) with `create`, the pre-lockdown + /// `user_get` (`GET`) with `get`, and the admin user edit (`PATCH`) with + /// `edit` (status, JSON body each), and recording every request. + async fn stub_forge( + create: (u16, &'static str), + get: (u16, &'static str), + edit: (u16, &'static str), + ) -> (Client, Seen) { use axum::http::{Method, StatusCode, Uri, header::CONTENT_TYPE}; let seen = Seen::default(); @@ -1408,6 +1450,7 @@ mod tests { )); let (status, body) = match method { Method::POST => create, + Method::GET => get, Method::PATCH => edit, _ => (404, r#"{"message":"not stubbed"}"#), }; @@ -1433,13 +1476,26 @@ mod tests { .map(|(_, _, body)| body.clone()) } + /// The email [`stub_forge`]'s `user_get` stub should answer with for an + /// account this agent actually owns — same value + /// [`Client::create_agent_user`] itself sets. + fn own_account_get(agent: &str) -> (u16, &'static str) { + // Leaked rather than borrowed: `stub_forge`'s `get` param needs + // `&'static str` (it moves into a `'static` axum handler), and this + // helper builds the body per-agent instead of hardcoding "alice". + ( + 200, + Box::leak(format!(r#"{{"email":"{agent}@hyperhive.local"}}"#).into_boxed_str()), + ) + } + /// A freshly created agent account is locked out of repo creation by /// this node itself — not left for a hive-level pass to catch later — /// and the edit sets nothing beyond `max_repo_creation` plus the /// `login_name` / `source_id` pair Forgejo needs to leave the rest alone. #[tokio::test] async fn a_created_agent_user_gets_repo_creation_disabled() { - let (client, seen) = stub_forge((201, "{}"), (200, "{}")).await; + let (client, seen) = stub_forge((201, "{}"), own_account_get("alice"), (200, "{}")).await; client.ensure_agent_user("alice").await.unwrap(); @@ -1464,8 +1520,12 @@ mod tests { /// is disambiguated. #[tokio::test] async fn an_existing_agent_user_still_gets_repo_creation_disabled() { - let (client, seen) = - stub_forge((409, r#"{"message":"user already exists"}"#), (200, "{}")).await; + let (client, seen) = stub_forge( + (409, r#"{"message":"user already exists"}"#), + own_account_get("alice"), + (200, "{}"), + ) + .await; client.ensure_agent_user("alice").await.unwrap(); @@ -1481,6 +1541,7 @@ mod tests { async fn a_refused_lockdown_fails_the_node() { let (client, _seen) = stub_forge( (201, "{}"), + own_account_get("alice"), (403, r#"{"message":"token does not have required scope"}"#), ) .await; @@ -1491,4 +1552,58 @@ mod tests { assert!(msg.contains("max_repo_creation"), "{msg}"); assert!(msg.contains("token does not have required scope"), "{msg}"); } + + /// The guard this PR adds: a name collision with a pre-existing + /// non-agent account (or any account whose email isn't the + /// `{agent}@hyperhive.local` marker) must fail the node rather than + /// PATCH `max_repo_creation`/`source_id` onto an account nobody + /// verified this node created. 409 create-fold, same as + /// `an_existing_agent_user_still_gets_repo_creation_disabled` — the + /// fold itself is untouched by this PR; the fix lives in the read right + /// before the PATCH. + #[tokio::test] + async fn a_name_collision_with_a_non_agent_account_fails_without_patching() { + let (client, seen) = stub_forge( + (409, r#"{"message":"user already exists"}"#), + (200, r#"{"email":"someone@example.com"}"#), + (200, "{}"), + ) + .await; + + let err = client.ensure_agent_user("alice").await.unwrap_err(); + + let msg = format!("{err:#}"); + assert!(msg.contains("alice"), "{msg}"); + assert!( + msg.contains("not this agent's"), + "must name the collision, got: {msg}" + ); + assert!( + lockdown_patch(&seen, "alice").is_none(), + "a collision must never reach the PATCH" + ); + } + + /// A legacy account `hive-c0re::forge::users::ensure_user_exists` (or + /// its `ensure_user_email` alignment pass) created carries the same + /// `{agent}@hyperhive.local` marker, plus the `login_name`/`source_id` + /// fields its own `sparse_edit_user_option` sets — this node must still + /// lock such an account down, not just ones it created itself. + #[tokio::test] + async fn a_legacy_hive_created_agent_account_still_gets_locked_down() { + let (client, seen) = stub_forge( + (409, r#"{"message":"user already exists"}"#), + ( + 200, + r#"{"id":42,"login":"alice","login_name":"alice","email":"alice@hyperhive.local","source_id":0}"#, + ), + (200, "{}"), + ) + .await; + + client.ensure_agent_user("alice").await.unwrap(); + + let body = lockdown_patch(&seen, "alice").expect("no lockdown PATCH sent"); + assert_eq!(body["max_repo_creation"], 0); + } }