refactor(#1834): derive cancel-loose-end privilege from the socket, not the MANAGER_AGENT name
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.
This commit is contained in:
parent
e797b75ca9
commit
53f49615fa
5 changed files with 65 additions and 49 deletions
|
|
@ -124,6 +124,7 @@ pub(crate) fn recv_timeout(wait_seconds: Option<u64>) -> std::time::Duration {
|
||||||
pub(crate) async fn dispatch_shared(
|
pub(crate) async fn dispatch_shared(
|
||||||
req: &hive_sh4re::Request,
|
req: &hive_sh4re::Request,
|
||||||
agent: &str,
|
agent: &str,
|
||||||
|
privileged: bool,
|
||||||
coord: &Arc<Coordinator>,
|
coord: &Arc<Coordinator>,
|
||||||
) -> Option<hive_sh4re::Response> {
|
) -> Option<hive_sh4re::Response> {
|
||||||
Some(match req {
|
Some(match req {
|
||||||
|
|
@ -178,10 +179,11 @@ pub(crate) async fn dispatch_shared(
|
||||||
handle_get_agent_meta(coord, agent, name.as_deref()).await
|
handle_get_agent_meta(coord, agent, name.as_deref()).await
|
||||||
}
|
}
|
||||||
hive_sh4re::Request::CancelLooseEnd { kind, id } => {
|
hive_sh4re::Request::CancelLooseEnd { kind, id } => {
|
||||||
crate::questions::handle_cancel_loose_end(coord, agent, *kind, *id).map_or_else(
|
crate::questions::handle_cancel_loose_end(coord, agent, privileged, *kind, *id)
|
||||||
|message| hive_sh4re::Response::Err { message },
|
.map_or_else(
|
||||||
|()| hive_sh4re::Response::Ok,
|
|message| hive_sh4re::Response::Err { message },
|
||||||
)
|
|()| hive_sh4re::Response::Ok,
|
||||||
|
)
|
||||||
}
|
}
|
||||||
hive_sh4re::Request::CreateRepo { repo } => handle_create_repo(agent, repo).await,
|
hive_sh4re::Request::CreateRepo { repo } => handle_create_repo(agent, repo).await,
|
||||||
hive_sh4re::Request::AckTurn => handle_ack_turn(coord, agent),
|
hive_sh4re::Request::AckTurn => handle_ack_turn(coord, agent),
|
||||||
|
|
@ -433,7 +435,9 @@ fn handle_requeue_inflight(coord: &Arc<Coordinator>, agent: &str) -> hive_sh4re:
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn dispatch(req: &AgentRequest, agent: &str, coord: &Arc<Coordinator>) -> AgentResponse {
|
async fn dispatch(req: &AgentRequest, agent: &str, coord: &Arc<Coordinator>) -> 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;
|
return resp;
|
||||||
}
|
}
|
||||||
match req {
|
match req {
|
||||||
|
|
|
||||||
|
|
@ -942,9 +942,10 @@ impl Broker {
|
||||||
|
|
||||||
/// Cancel a pending reminder on behalf of `canceller`. Returns
|
/// Cancel a pending reminder on behalf of `canceller`. Returns
|
||||||
/// the owner agent name on success (handy for logging). Auth
|
/// the owner agent name on success (handy for logging). Auth
|
||||||
/// rules mirror `OperatorQuestions::cancel`: owner, operator, or
|
/// rules mirror `OperatorQuestions::cancel`: the owner, the
|
||||||
/// manager.
|
/// operator, or a `privileged` caller (one that arrived on the
|
||||||
pub fn cancel_reminder_as(&self, id: i64, canceller: &str) -> Result<String> {
|
/// manager socket — the trust boundary, not a name match).
|
||||||
|
pub fn cancel_reminder_as(&self, id: i64, canceller: &str, privileged: bool) -> Result<String> {
|
||||||
let conn = self.conn.lock().unwrap();
|
let conn = self.conn.lock().unwrap();
|
||||||
let owner: Option<String> = conn
|
let owner: Option<String> = conn
|
||||||
.query_row(
|
.query_row(
|
||||||
|
|
@ -956,9 +957,8 @@ impl Broker {
|
||||||
let Some(owner) = owner else {
|
let Some(owner) = owner else {
|
||||||
anyhow::bail!("reminder {id} not pending (already delivered or unknown)");
|
anyhow::bail!("reminder {id} not pending (already delivered or unknown)");
|
||||||
};
|
};
|
||||||
let authorised = canceller == owner
|
let authorised =
|
||||||
|| canceller == hive_sh4re::OPERATOR_RECIPIENT
|
privileged || canceller == owner || canceller == hive_sh4re::OPERATOR_RECIPIENT;
|
||||||
|| canceller == hive_sh4re::MANAGER_AGENT;
|
|
||||||
if !authorised {
|
if !authorised {
|
||||||
anyhow::bail!("reminder {id}: '{canceller}' not allowed to cancel (owner = '{owner}')");
|
anyhow::bail!("reminder {id}: '{canceller}' not allowed to cancel (owner = '{owner}')");
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -75,8 +75,14 @@ async fn serve(stream: UnixStream, coord: Arc<Coordinator>) -> Result<()> {
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> ManagerResponse {
|
async fn dispatch(req: &ManagerRequest, coord: &Arc<Coordinator>) -> ManagerResponse {
|
||||||
// Delegate all variants shared with the agent socket to the common handler.
|
// Delegate all variants shared with the agent socket to the common
|
||||||
if let Some(resp) = crate::agent_server::dispatch_shared(req, MANAGER_AGENT, coord).await {
|
// 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;
|
return resp;
|
||||||
}
|
}
|
||||||
match req {
|
match req {
|
||||||
|
|
|
||||||
|
|
@ -204,11 +204,18 @@ impl OperatorQuestions {
|
||||||
/// - the original asker (an agent withdrawing their own ask),
|
/// - the original asker (an agent withdrawing their own ask),
|
||||||
/// - the operator (already covered by the existing `answer` path
|
/// - the operator (already covered by the existing `answer` path
|
||||||
/// but allowed here too for symmetry / dashboard cancel),
|
/// 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
|
/// Not the target — that's covered by `answer` (responding with
|
||||||
/// an actual reply, sentinel or otherwise).
|
/// an actual reply, sentinel or otherwise).
|
||||||
pub fn cancel(&self, id: i64, canceller: &str) -> Result<(String, String, Option<String>)> {
|
pub fn cancel(
|
||||||
|
&self,
|
||||||
|
id: i64,
|
||||||
|
canceller: &str,
|
||||||
|
privileged: bool,
|
||||||
|
) -> Result<(String, String, Option<String>)> {
|
||||||
let conn = self.conn.lock().unwrap();
|
let conn = self.conn.lock().unwrap();
|
||||||
let row: Option<(String, String, Option<String>, Option<i64>)> = conn
|
let row: Option<(String, String, Option<String>, Option<i64>)> = conn
|
||||||
.query_row(
|
.query_row(
|
||||||
|
|
@ -223,9 +230,8 @@ impl OperatorQuestions {
|
||||||
if answered_at.is_some() {
|
if answered_at.is_some() {
|
||||||
bail!("question {id} already answered/cancelled");
|
bail!("question {id} already answered/cancelled");
|
||||||
}
|
}
|
||||||
let authorised = canceller == asker
|
let authorised =
|
||||||
|| canceller == hive_sh4re::OPERATOR_RECIPIENT
|
privileged || canceller == asker || canceller == hive_sh4re::OPERATOR_RECIPIENT;
|
||||||
|| canceller == hive_sh4re::MANAGER_AGENT;
|
|
||||||
if !authorised {
|
if !authorised {
|
||||||
bail!("question {id}: '{canceller}' not allowed to cancel (asker = '{asker}')");
|
bail!("question {id}: '{canceller}' not allowed to cancel (asker = '{asker}')");
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -142,14 +142,17 @@ pub fn handle_answer(
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Handle `CancelLooseEnd` from either surface. Dispatches by kind to
|
/// Handle `CancelLooseEnd` from either surface. Dispatches by kind to
|
||||||
/// either `OperatorQuestions::cancel` or `Broker::cancel_reminder_as`,
|
/// the per-kind cancel, each of which does its own auth check
|
||||||
/// both of which do their own auth check (canceller == owner /
|
/// (canceller == owner / asker, or `operator`, or `privileged`).
|
||||||
/// asker, or `operator`, or `manager`). On question cancel, fires
|
/// `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
|
/// the `QuestionAnswered` event back to the asker so the harness
|
||||||
/// loop can react (mirrors the operator-cancel dashboard path).
|
/// loop can react (mirrors the operator-cancel dashboard path).
|
||||||
pub fn handle_cancel_loose_end(
|
pub fn handle_cancel_loose_end(
|
||||||
coord: &Arc<Coordinator>,
|
coord: &Arc<Coordinator>,
|
||||||
canceller: &str,
|
canceller: &str,
|
||||||
|
privileged: bool,
|
||||||
kind: hive_sh4re::CancelLooseEndKind,
|
kind: hive_sh4re::CancelLooseEndKind,
|
||||||
id: i64,
|
id: i64,
|
||||||
) -> Result<(), String> {
|
) -> Result<(), String> {
|
||||||
|
|
@ -157,7 +160,7 @@ pub fn handle_cancel_loose_end(
|
||||||
hive_sh4re::CancelLooseEndKind::Question => {
|
hive_sh4re::CancelLooseEndKind::Question => {
|
||||||
let (question, asker, target) = coord
|
let (question, asker, target) = coord
|
||||||
.questions
|
.questions
|
||||||
.cancel(id, canceller)
|
.cancel(id, canceller, privileged)
|
||||||
.map_err(|e| format!("{e:#}"))?;
|
.map_err(|e| format!("{e:#}"))?;
|
||||||
let sentinel = format!("[cancelled by {canceller}]");
|
let sentinel = format!("[cancelled by {canceller}]");
|
||||||
tracing::info!(%id, %canceller, %asker, "question cancelled");
|
tracing::info!(%id, %canceller, %asker, "question cancelled");
|
||||||
|
|
@ -183,17 +186,17 @@ pub fn handle_cancel_loose_end(
|
||||||
hive_sh4re::CancelLooseEndKind::Reminder => {
|
hive_sh4re::CancelLooseEndKind::Reminder => {
|
||||||
let owner = coord
|
let owner = coord
|
||||||
.broker
|
.broker
|
||||||
.cancel_reminder_as(id, canceller)
|
.cancel_reminder_as(id, canceller, privileged)
|
||||||
.map_err(|e| format!("{e:#}"))?;
|
.map_err(|e| format!("{e:#}"))?;
|
||||||
tracing::info!(%id, %canceller, %owner, "reminder cancelled");
|
tracing::info!(%id, %canceller, %owner, "reminder cancelled");
|
||||||
coord.emit_reminders_snapshot();
|
coord.emit_reminders_snapshot();
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
hive_sh4re::CancelLooseEndKind::Approval => {
|
hive_sh4re::CancelLooseEndKind::Approval => {
|
||||||
// Manager-only: only the agent that can submit approvals
|
// Privileged-only: only a caller on the manager socket (which
|
||||||
// is allowed to withdraw them. Sub-agents would have no
|
// is the sole approval submitter) may withdraw approvals.
|
||||||
// pending approvals of their own to cancel anyway.
|
// Sub-agents have no pending approvals of their own anyway.
|
||||||
check_approval_canceller_is_manager(canceller)?;
|
check_can_cancel_approval(privileged)?;
|
||||||
let approval = coord
|
let approval = coord
|
||||||
.approvals
|
.approvals
|
||||||
.mark_cancelled(id, canceller)
|
.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
|
/// the auth check has its own focused unit test — testing the full
|
||||||
/// `handle_cancel_loose_end` flow would need a `Coordinator` fixture
|
/// `handle_cancel_loose_end` flow would need a `Coordinator` fixture
|
||||||
/// (broker + sqlite + in-memory questions), which we don't have
|
/// (broker + sqlite + in-memory questions), which we don't have
|
||||||
/// today. The check is a single string compare, so a function-level
|
/// today. Privilege is a property of the socket the request arrived
|
||||||
/// test gives the same coverage with no harness.
|
/// on (the manager socket), threaded in as `privileged` — not a match
|
||||||
fn check_approval_canceller_is_manager(canceller: &str) -> Result<(), String> {
|
/// against a hardcoded agent name.
|
||||||
if canceller != hive_sh4re::MANAGER_AGENT {
|
fn check_can_cancel_approval(privileged: bool) -> Result<(), String> {
|
||||||
return Err("cancel_loose_end: only the manager can cancel approval rows".to_owned());
|
if !privileged {
|
||||||
|
return Err(
|
||||||
|
"cancel_loose_end: only a privileged (manager-socket) caller can cancel approval rows"
|
||||||
|
.to_owned(),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
@ -235,25 +242,18 @@ mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn approval_cancel_rejects_sub_agent_callers() {
|
fn approval_cancel_rejects_unprivileged_callers() {
|
||||||
// Sub-agents must not be able to cancel approval rows even
|
// A non-privileged caller (any regular agent socket) must not be
|
||||||
// if they invent an id. The guard is server-side so client
|
// able to cancel approval rows even if it invents an id. The guard
|
||||||
// cooperation is irrelevant.
|
// is server-side so client cooperation is irrelevant — and it keys
|
||||||
let err = check_approval_canceller_is_manager("bitburner").unwrap_err();
|
// on the socket-derived `privileged` flag, not on any agent name.
|
||||||
assert!(err.contains("only the manager"), "{err}");
|
let err = check_can_cancel_approval(false).unwrap_err();
|
||||||
// Bonus: empty / operator strings also rejected (only the
|
assert!(err.contains("only a privileged"), "{err}");
|
||||||
// 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",
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn approval_cancel_allows_manager() {
|
fn approval_cancel_allows_privileged() {
|
||||||
check_approval_canceller_is_manager(hive_sh4re::MANAGER_AGENT)
|
check_can_cancel_approval(true).expect("a privileged caller must pass the guard");
|
||||||
.expect("MANAGER_AGENT must pass the guard");
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue