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.
This commit is contained in:
parent
04d5bf5f3e
commit
58f50ed506
2 changed files with 144 additions and 12 deletions
|
|
@ -156,7 +156,24 @@ in
|
||||||
# per-container toplevels stay fully cached.
|
# per-container toplevels stay fully cached.
|
||||||
cargo-test = craneLib.cargoTest {
|
cargo-test = craneLib.cargoTest {
|
||||||
src = cleanSrc;
|
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";
|
pname = "hyperhive-workspace";
|
||||||
version = "0.1.0";
|
version = "0.1.0";
|
||||||
cargoTestExtraArgs = "--workspace";
|
cargoTestExtraArgs = "--workspace";
|
||||||
|
|
|
||||||
|
|
@ -399,11 +399,35 @@ impl Client {
|
||||||
/// the operator-only-merge policy. Existing repos are untouched; push,
|
/// the operator-only-merge policy. Existing repos are untouched; push,
|
||||||
/// PRs and clone keep working.
|
/// PRs and clone keep working.
|
||||||
///
|
///
|
||||||
/// Unconditional rather than verify-then-patch: Forgejo's API never
|
/// The PATCH itself is unconditional rather than verify-then-patch:
|
||||||
/// returns `max_repo_creation` (it is a field of `EditUserOption` only,
|
/// Forgejo's API never returns `max_repo_creation` (it is a field of
|
||||||
/// not of `User`), so there is no current value to compare against.
|
/// `EditUserOption` only, not of `User`), so there is no current value
|
||||||
/// Setting 0 on an account already at 0 is a no-op.
|
/// 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<()> {
|
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
|
self.api
|
||||||
.admin_edit_user(agent, repo_creation_lockdown(agent))
|
.admin_edit_user(agent, repo_creation_lockdown(agent))
|
||||||
.await
|
.await
|
||||||
|
|
@ -423,7 +447,7 @@ impl Client {
|
||||||
.api
|
.api
|
||||||
.admin_create_user(CreateUserOption {
|
.admin_create_user(CreateUserOption {
|
||||||
created_at: None,
|
created_at: None,
|
||||||
email: format!("{agent}@hyperhive.local"),
|
email: agent_email(agent),
|
||||||
full_name: None,
|
full_name: None,
|
||||||
login_name: None,
|
login_name: None,
|
||||||
must_change_password: Some(false),
|
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"
|
"{\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`
|
/// The `admin_edit_user` body that sets `max_repo_creation = 0` on `agent`
|
||||||
/// and nothing else. Same shape as `hive-c0re::forge::users`'
|
/// and nothing else. Same shape as `hive-c0re::forge::users`'
|
||||||
/// `sparse_edit_user_option`: `login_name` + `source_id = 0` (local auth,
|
/// `sparse_edit_user_option`: `login_name` + `source_id = 0` (local auth,
|
||||||
|
|
@ -1389,9 +1426,14 @@ mod tests {
|
||||||
type Seen = std::sync::Arc<std::sync::Mutex<Vec<(String, String, serde_json::Value)>>>;
|
type Seen = std::sync::Arc<std::sync::Mutex<Vec<(String, String, serde_json::Value)>>>;
|
||||||
|
|
||||||
/// A stand-in forge on a loopback port — never the real one — answering
|
/// 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
|
/// the admin user create (`POST`) with `create`, the pre-lockdown
|
||||||
/// (`PATCH`) with `edit` (status, JSON body), and recording every request.
|
/// `user_get` (`GET`) with `get`, and the admin user edit (`PATCH`) with
|
||||||
async fn stub_forge(create: (u16, &'static str), edit: (u16, &'static str)) -> (Client, Seen) {
|
/// `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};
|
use axum::http::{Method, StatusCode, Uri, header::CONTENT_TYPE};
|
||||||
|
|
||||||
let seen = Seen::default();
|
let seen = Seen::default();
|
||||||
|
|
@ -1408,6 +1450,7 @@ mod tests {
|
||||||
));
|
));
|
||||||
let (status, body) = match method {
|
let (status, body) = match method {
|
||||||
Method::POST => create,
|
Method::POST => create,
|
||||||
|
Method::GET => get,
|
||||||
Method::PATCH => edit,
|
Method::PATCH => edit,
|
||||||
_ => (404, r#"{"message":"not stubbed"}"#),
|
_ => (404, r#"{"message":"not stubbed"}"#),
|
||||||
};
|
};
|
||||||
|
|
@ -1433,13 +1476,26 @@ mod tests {
|
||||||
.map(|(_, _, body)| body.clone())
|
.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
|
/// 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 —
|
/// this node itself — not left for a hive-level pass to catch later —
|
||||||
/// and the edit sets nothing beyond `max_repo_creation` plus the
|
/// and the edit sets nothing beyond `max_repo_creation` plus the
|
||||||
/// `login_name` / `source_id` pair Forgejo needs to leave the rest alone.
|
/// `login_name` / `source_id` pair Forgejo needs to leave the rest alone.
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn a_created_agent_user_gets_repo_creation_disabled() {
|
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();
|
client.ensure_agent_user("alice").await.unwrap();
|
||||||
|
|
||||||
|
|
@ -1464,8 +1520,12 @@ mod tests {
|
||||||
/// is disambiguated.
|
/// is disambiguated.
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn an_existing_agent_user_still_gets_repo_creation_disabled() {
|
async fn an_existing_agent_user_still_gets_repo_creation_disabled() {
|
||||||
let (client, seen) =
|
let (client, seen) = stub_forge(
|
||||||
stub_forge((409, r#"{"message":"user already exists"}"#), (200, "{}")).await;
|
(409, r#"{"message":"user already exists"}"#),
|
||||||
|
own_account_get("alice"),
|
||||||
|
(200, "{}"),
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
|
||||||
client.ensure_agent_user("alice").await.unwrap();
|
client.ensure_agent_user("alice").await.unwrap();
|
||||||
|
|
||||||
|
|
@ -1481,6 +1541,7 @@ mod tests {
|
||||||
async fn a_refused_lockdown_fails_the_node() {
|
async fn a_refused_lockdown_fails_the_node() {
|
||||||
let (client, _seen) = stub_forge(
|
let (client, _seen) = stub_forge(
|
||||||
(201, "{}"),
|
(201, "{}"),
|
||||||
|
own_account_get("alice"),
|
||||||
(403, r#"{"message":"token does not have required scope"}"#),
|
(403, r#"{"message":"token does not have required scope"}"#),
|
||||||
)
|
)
|
||||||
.await;
|
.await;
|
||||||
|
|
@ -1491,4 +1552,58 @@ mod tests {
|
||||||
assert!(msg.contains("max_repo_creation"), "{msg}");
|
assert!(msg.contains("max_repo_creation"), "{msg}");
|
||||||
assert!(msg.contains("token does not have required scope"), "{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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue