From 53f49615fa0705b56081e6ba908bdca985d2296e Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 22 Jun 2026 02:07:14 +0200 Subject: [PATCH] refactor(#1834): derive cancel-loose-end privilege from the socket, not the MANAGER_AGENT name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cancel-loose-end guards (cancel approval / question / reminder) inferred manager-level privilege by string-matching the canceller against the hardcoded `hive_sh4re::MANAGER_AGENT` ("ruth"). That laundered privilege through a name: a request arrives on the privileged manager socket, gets stamped with the bootstrap agent's name, and the guards re-derive privilege from that name. Renaming or replacing the bootstrap agent would then silently move privilege. Privilege is a property of the SOCKET the request arrived on (the manager socket is the trust boundary), so thread an explicit `privileged: bool` through `dispatch_shared` → `handle_cancel_loose_end` → the three guards: - `Broker::cancel_reminder_as` and `OperatorQuestions::cancel`: the `== MANAGER_AGENT` leg becomes `privileged` (owner/asker + operator name legs unchanged). - `check_approval_canceller_is_manager(canceller)` → `check_can_cancel_approval(privileged)` (manager-socket-only); unit tests updated to assert on the flag. The manager socket passes `privileged = true`; the agent socket passes `false`. `MANAGER_AGENT` is still passed as the actor NAME for legitimate attribution/routing (notifications, schedule ownership, bootstrap destroy-protection) — those are not privilege checks and are left intact. Scope is the privilege guards only. --- hive-c0re/src/agent_server.rs | 14 ++++--- hive-c0re/src/broker.rs | 12 +++--- hive-c0re/src/manager_server.rs | 10 ++++- hive-c0re/src/operator_questions.rs | 16 +++++--- hive-c0re/src/questions.rs | 62 ++++++++++++++--------------- 5 files changed, 65 insertions(+), 49 deletions(-) diff --git a/hive-c0re/src/agent_server.rs b/hive-c0re/src/agent_server.rs index 66002c6d..6d02af4a 100644 --- a/hive-c0re/src/agent_server.rs +++ b/hive-c0re/src/agent_server.rs @@ -124,6 +124,7 @@ pub(crate) fn recv_timeout(wait_seconds: Option) -> std::time::Duration { pub(crate) async fn dispatch_shared( req: &hive_sh4re::Request, agent: &str, + privileged: bool, coord: &Arc, ) -> Option { Some(match req { @@ -178,10 +179,11 @@ pub(crate) async fn dispatch_shared( handle_get_agent_meta(coord, agent, name.as_deref()).await } hive_sh4re::Request::CancelLooseEnd { kind, id } => { - crate::questions::handle_cancel_loose_end(coord, agent, *kind, *id).map_or_else( - |message| hive_sh4re::Response::Err { message }, - |()| hive_sh4re::Response::Ok, - ) + crate::questions::handle_cancel_loose_end(coord, agent, privileged, *kind, *id) + .map_or_else( + |message| hive_sh4re::Response::Err { message }, + |()| hive_sh4re::Response::Ok, + ) } hive_sh4re::Request::CreateRepo { repo } => handle_create_repo(agent, repo).await, hive_sh4re::Request::AckTurn => handle_ack_turn(coord, agent), @@ -433,7 +435,9 @@ fn handle_requeue_inflight(coord: &Arc, agent: &str) -> hive_sh4re: } async fn dispatch(req: &AgentRequest, agent: &str, coord: &Arc) -> AgentResponse { - if let Some(resp) = dispatch_shared(req, agent, coord).await { + // Regular agent socket: never privileged. Privilege is reserved for + // requests arriving on the manager socket (see `manager_server`). + if let Some(resp) = dispatch_shared(req, agent, false, coord).await { return resp; } match req { diff --git a/hive-c0re/src/broker.rs b/hive-c0re/src/broker.rs index 52d67e2c..faf98ca8 100644 --- a/hive-c0re/src/broker.rs +++ b/hive-c0re/src/broker.rs @@ -942,9 +942,10 @@ impl Broker { /// Cancel a pending reminder on behalf of `canceller`. Returns /// the owner agent name on success (handy for logging). Auth - /// rules mirror `OperatorQuestions::cancel`: owner, operator, or - /// manager. - pub fn cancel_reminder_as(&self, id: i64, canceller: &str) -> Result { + /// rules mirror `OperatorQuestions::cancel`: the owner, the + /// operator, or a `privileged` caller (one that arrived on the + /// manager socket — the trust boundary, not a name match). + pub fn cancel_reminder_as(&self, id: i64, canceller: &str, privileged: bool) -> Result { let conn = self.conn.lock().unwrap(); let owner: Option = conn .query_row( @@ -956,9 +957,8 @@ impl Broker { let Some(owner) = owner else { anyhow::bail!("reminder {id} not pending (already delivered or unknown)"); }; - let authorised = canceller == owner - || canceller == hive_sh4re::OPERATOR_RECIPIENT - || canceller == hive_sh4re::MANAGER_AGENT; + let authorised = + privileged || canceller == owner || canceller == hive_sh4re::OPERATOR_RECIPIENT; if !authorised { anyhow::bail!("reminder {id}: '{canceller}' not allowed to cancel (owner = '{owner}')"); } diff --git a/hive-c0re/src/manager_server.rs b/hive-c0re/src/manager_server.rs index 21f94999..489ebce3 100644 --- a/hive-c0re/src/manager_server.rs +++ b/hive-c0re/src/manager_server.rs @@ -75,8 +75,14 @@ async fn serve(stream: UnixStream, coord: Arc) -> Result<()> { } async fn dispatch(req: &ManagerRequest, coord: &Arc) -> ManagerResponse { - // Delegate all variants shared with the agent socket to the common handler. - if let Some(resp) = crate::agent_server::dispatch_shared(req, MANAGER_AGENT, coord).await { + // Delegate all variants shared with the agent socket to the common + // handler. `privileged = true`: every request here arrived on the + // manager socket, which is the trust boundary — so the shared guards + // grant manager-level authority from the socket, not from matching the + // `MANAGER_AGENT` name. `MANAGER_AGENT` is still passed as the actor + // name for attribution/routing (notifications, ownership), not authz. + if let Some(resp) = crate::agent_server::dispatch_shared(req, MANAGER_AGENT, true, coord).await + { return resp; } match req { diff --git a/hive-c0re/src/operator_questions.rs b/hive-c0re/src/operator_questions.rs index 60f2639c..1dcf14ad 100644 --- a/hive-c0re/src/operator_questions.rs +++ b/hive-c0re/src/operator_questions.rs @@ -204,11 +204,18 @@ impl OperatorQuestions { /// - the original asker (an agent withdrawing their own ask), /// - the operator (already covered by the existing `answer` path /// but allowed here too for symmetry / dashboard cancel), - /// - the manager (privileged hive-wide cleanup). + /// - a `privileged` caller (one that arrived on the manager socket — + /// privileged hive-wide cleanup; derived from the socket, not a + /// name match). /// /// Not the target — that's covered by `answer` (responding with /// an actual reply, sentinel or otherwise). - pub fn cancel(&self, id: i64, canceller: &str) -> Result<(String, String, Option)> { + pub fn cancel( + &self, + id: i64, + canceller: &str, + privileged: bool, + ) -> Result<(String, String, Option)> { let conn = self.conn.lock().unwrap(); let row: Option<(String, String, Option, Option)> = conn .query_row( @@ -223,9 +230,8 @@ impl OperatorQuestions { if answered_at.is_some() { bail!("question {id} already answered/cancelled"); } - let authorised = canceller == asker - || canceller == hive_sh4re::OPERATOR_RECIPIENT - || canceller == hive_sh4re::MANAGER_AGENT; + let authorised = + privileged || canceller == asker || canceller == hive_sh4re::OPERATOR_RECIPIENT; if !authorised { bail!("question {id}: '{canceller}' not allowed to cancel (asker = '{asker}')"); } diff --git a/hive-c0re/src/questions.rs b/hive-c0re/src/questions.rs index 8e719d35..0a13039a 100644 --- a/hive-c0re/src/questions.rs +++ b/hive-c0re/src/questions.rs @@ -142,14 +142,17 @@ pub fn handle_answer( } /// Handle `CancelLooseEnd` from either surface. Dispatches by kind to -/// either `OperatorQuestions::cancel` or `Broker::cancel_reminder_as`, -/// both of which do their own auth check (canceller == owner / -/// asker, or `operator`, or `manager`). On question cancel, fires +/// the per-kind cancel, each of which does its own auth check +/// (canceller == owner / asker, or `operator`, or `privileged`). +/// `privileged` is `true` when the request arrived on the manager +/// socket — privilege derives from the socket (the trust boundary), +/// not from matching a hardcoded agent name. On question cancel, fires /// the `QuestionAnswered` event back to the asker so the harness /// loop can react (mirrors the operator-cancel dashboard path). pub fn handle_cancel_loose_end( coord: &Arc, canceller: &str, + privileged: bool, kind: hive_sh4re::CancelLooseEndKind, id: i64, ) -> Result<(), String> { @@ -157,7 +160,7 @@ pub fn handle_cancel_loose_end( hive_sh4re::CancelLooseEndKind::Question => { let (question, asker, target) = coord .questions - .cancel(id, canceller) + .cancel(id, canceller, privileged) .map_err(|e| format!("{e:#}"))?; let sentinel = format!("[cancelled by {canceller}]"); tracing::info!(%id, %canceller, %asker, "question cancelled"); @@ -183,17 +186,17 @@ pub fn handle_cancel_loose_end( hive_sh4re::CancelLooseEndKind::Reminder => { let owner = coord .broker - .cancel_reminder_as(id, canceller) + .cancel_reminder_as(id, canceller, privileged) .map_err(|e| format!("{e:#}"))?; tracing::info!(%id, %canceller, %owner, "reminder cancelled"); coord.emit_reminders_snapshot(); Ok(()) } hive_sh4re::CancelLooseEndKind::Approval => { - // Manager-only: only the agent that can submit approvals - // is allowed to withdraw them. Sub-agents would have no - // pending approvals of their own to cancel anyway. - check_approval_canceller_is_manager(canceller)?; + // Privileged-only: only a caller on the manager socket (which + // is the sole approval submitter) may withdraw approvals. + // Sub-agents have no pending approvals of their own anyway. + check_can_cancel_approval(privileged)?; let approval = coord .approvals .mark_cancelled(id, canceller) @@ -217,15 +220,19 @@ pub fn handle_cancel_loose_end( } } -/// Manager-only guard on the `Approval` cancel arm. Pulled out so +/// Privileged-only guard on the `Approval` cancel arm. Pulled out so /// the auth check has its own focused unit test — testing the full /// `handle_cancel_loose_end` flow would need a `Coordinator` fixture /// (broker + sqlite + in-memory questions), which we don't have -/// today. The check is a single string compare, so a function-level -/// test gives the same coverage with no harness. -fn check_approval_canceller_is_manager(canceller: &str) -> Result<(), String> { - if canceller != hive_sh4re::MANAGER_AGENT { - return Err("cancel_loose_end: only the manager can cancel approval rows".to_owned()); +/// today. Privilege is a property of the socket the request arrived +/// on (the manager socket), threaded in as `privileged` — not a match +/// against a hardcoded agent name. +fn check_can_cancel_approval(privileged: bool) -> Result<(), String> { + if !privileged { + return Err( + "cancel_loose_end: only a privileged (manager-socket) caller can cancel approval rows" + .to_owned(), + ); } Ok(()) } @@ -235,25 +242,18 @@ mod tests { use super::*; #[test] - fn approval_cancel_rejects_sub_agent_callers() { - // Sub-agents must not be able to cancel approval rows even - // if they invent an id. The guard is server-side so client - // cooperation is irrelevant. - let err = check_approval_canceller_is_manager("bitburner").unwrap_err(); - assert!(err.contains("only the manager"), "{err}"); - // Bonus: empty / operator strings also rejected (only the - // exact MANAGER_AGENT constant passes). - assert!(check_approval_canceller_is_manager("").is_err()); - assert!( - check_approval_canceller_is_manager(hive_sh4re::OPERATOR_RECIPIENT).is_err(), - "operator surface uses the dashboard cancel path, not this dispatcher", - ); + fn approval_cancel_rejects_unprivileged_callers() { + // A non-privileged caller (any regular agent socket) must not be + // able to cancel approval rows even if it invents an id. The guard + // is server-side so client cooperation is irrelevant — and it keys + // on the socket-derived `privileged` flag, not on any agent name. + let err = check_can_cancel_approval(false).unwrap_err(); + assert!(err.contains("only a privileged"), "{err}"); } #[test] - fn approval_cancel_allows_manager() { - check_approval_canceller_is_manager(hive_sh4re::MANAGER_AGENT) - .expect("MANAGER_AGENT must pass the guard"); + fn approval_cancel_allows_privileged() { + check_can_cancel_approval(true).expect("a privileged caller must pass the guard"); } }