From 4947630d19e0dc85b68991aafea0c0c766d3723a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Wed, 9 Sep 2026 23:09:35 +0000 Subject: [PATCH 1/8] fix: distinguish execution failures from policy denials --- docs/agent-integration.md | 17 +++-- src/audit.rs | 55 +++++++++++++++ src/cli_client.rs | 64 ++++++++++++++++- src/gating/approval.rs | 6 ++ src/main.rs | 4 +- src/mcp.rs | 115 ++++++++++++++++++++++++++++-- src/server/admin.rs | 67 ++++++++++++++++-- src/server/execute.rs | 4 ++ src/server/gate_runtime.rs | 31 ++++++--- src/server/grants.rs | 5 +- src/server/mod.rs | 22 ++++-- src/server/tests/gating.rs | 2 + src/server/tests/sessions.rs | 7 ++ src/server/tests/verbs.rs | 2 + src/server/tests/wire.rs | 114 ++++++++++++++++++++++++++++++ src/server/transport.rs | 12 ++-- src/server/wire.rs | 124 ++++++++++++++++++++++++++++++--- src/session.rs | 12 ++++ src/session_store.rs | 131 ++++++++++++++++++++++++++++++++--- src/wire/mod.rs | 46 ++++++++++++ tests/cli_output.rs | 78 +++++++++++++++++++++ 21 files changed, 859 insertions(+), 59 deletions(-) diff --git a/docs/agent-integration.md b/docs/agent-integration.md index cfd92ab2..9682954d 100644 --- a/docs/agent-integration.md +++ b/docs/agent-integration.md @@ -125,7 +125,7 @@ propagates the executed child's exit status untranslated: | Code | Meaning | | ----- | ---------------------------------------------------------------- | -| 125 | Guard operational error (daemon unreachable, protocol failure) | +| 125 | Guard operational error, including execution failure | | 126 | Denied by policy | | 127 | Held for operator approval | | 2 | Invalid guard CLI usage (argument parsing) | @@ -135,9 +135,18 @@ The reserved range collides with codes a child can produce on its own: `sh -c` exits 127 when the named command is missing, and `git bisect skip` uses 125. An exit code of 125-127 therefore suggests, but cannot prove, a guard-origin outcome. An agent that needs certainty runs with `--json` and reads the -`allowed` and `status` fields; the exit code is a convenience for shell -pipelines, not the authoritative decision channel. `guard run` prints this -contract in its own help output (`guard help run`). +`policy`, `execution_failure`, `allowed`, and `status` fields; the exit code is +a convenience for shell pipelines, not the authoritative decision channel. +`guard run` prints this contract in its own help output (`guard help run`). + +An approved command that fails during setup or execution reports +`EXECUTION FAILED` and exits 125. The optional `policy` object preserves the +admission result and reason, and `decision_source` identifies that admission. +The optional `execution_failure` object carries `started`, `stage`, `errno`, +and a sanitized `message`. Stages are `identity`, `capabilities`, `cwd`, +`exec`, or `unknown`; `unknown` means no precise failing stage is established. +The legacy `allowed` field remains false for an execution failure. Policy +approval does not establish that the command started or completed. ## MCP diff --git a/src/audit.rs b/src/audit.rs index f8411a49..9e1ab8b6 100644 --- a/src/audit.rs +++ b/src/audit.rs @@ -179,6 +179,10 @@ impl std::fmt::Display for AuditKind { /// which preserves insertion order for the stderr projection. #[derive(Debug, Clone, Deserialize)] pub struct AuditEvent { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub policy: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_failure: Option, pub kind: AuditKind, #[serde(default, skip_serializing_if = "Option::is_none")] pub handle: Option, @@ -200,6 +204,10 @@ pub struct AuditEvent { #[derive(Serialize)] struct AuditEventSerializationView { + #[serde(skip_serializing_if = "Option::is_none")] + policy: Option, + #[serde(skip_serializing_if = "Option::is_none")] + execution_failure: Option, kind: AuditKind, #[serde(skip_serializing_if = "Option::is_none")] handle: Option, @@ -223,6 +231,8 @@ impl AuditEvent { fn serialization_view(&self) -> AuditEventSerializationView { let projected = redact_secret_exposure(self); AuditEventSerializationView { + policy: projected.policy, + execution_failure: projected.execution_failure, kind: projected.kind, handle: projected.handle, caller: projected.caller, @@ -252,6 +262,14 @@ impl AuditEvent { *value = crate::redact::redact_exact_and_registered_secrets(value, secrets); } } + if let Some(policy) = self.policy.as_mut() { + policy.reason = + crate::redact::redact_exact_and_registered_secrets(&policy.reason, secrets); + } + if let Some(failure) = self.execution_failure.as_mut() { + failure.message = + crate::redact::redact_exact_and_registered_secrets(&failure.message, secrets); + } redact(&mut self.handle, secrets); redact(&mut self.caller, secrets); redact(&mut self.session_fingerprint, secrets); @@ -268,6 +286,8 @@ impl AuditEvent { pub fn new(kind: AuditKind) -> Self { Self { + policy: None, + execution_failure: None, kind, handle: None, caller: None, @@ -280,6 +300,16 @@ impl AuditEvent { } } + pub fn execution( + mut self, + policy: crate::wire::PolicyDecision, + failure: Option, + ) -> Self { + self.policy = Some(policy); + self.execution_failure = failure.map(crate::wire::ExecutionFailure::sanitized); + self + } + pub fn handle(mut self, handle: impl Into) -> Self { self.handle = Some(handle.into()); self @@ -354,6 +384,23 @@ impl AuditEvent { if let Some(source) = &self.decision_source { push_field(&mut line, "decision_source", source, false); } + if let Some(policy) = &self.policy { + push_field( + &mut line, + "policy_allowed", + &policy.allowed.to_string(), + false, + ); + push_field(&mut line, "policy_reason", &policy.reason, true); + } + if let Some(failure) = &self.execution_failure { + push_field( + &mut line, + "execution_failure", + &serde_json::to_string(failure).expect("execution failure serializes"), + true, + ); + } for (key, value) in &self.fields { push_field(&mut line, key, value, value_needs_quoting(value)); } @@ -363,7 +410,15 @@ impl AuditEvent { fn redact_secret_exposure(event: &AuditEvent) -> AuditEvent { let mut redacted = event.clone(); + if let Some(policy) = redacted.policy.as_mut() { + policy.reason = crate::gating::sanitize_gate_text(&policy.reason); + } + redacted.execution_failure = redacted + .execution_failure + .map(crate::wire::ExecutionFailure::sanitized); if event.kind == AuditKind::SecretExposed { + redacted.policy = None; + redacted.execution_failure = None; redacted.cmd = redacted.cmd.map(|_| "[redacted]".to_string()); redacted.reason = redacted.reason.map(|_| "[redacted]".to_string()); redacted.fields = redacted diff --git a/src/cli_client.rs b/src/cli_client.rs index ce7e73ce..ab96093b 100644 --- a/src/cli_client.rs +++ b/src/cli_client.rs @@ -581,6 +581,19 @@ pub(crate) async fn run_exec( _ => {} } + if let Some(failure) = &resp.execution_failure { + eprintln!("{}", execution_failure_text(failure)); + if explain { + if let Some(policy) = &resp.policy { + eprintln!( + " policy allowed: {}; source: {}; reason: {}", + policy.allowed, resp.decision_source, policy.reason + ); + } + } + std::process::exit(EXIT_GUARD_ERROR); + } + if resp.allowed { tracing::info!( binary = %binary, @@ -622,6 +635,18 @@ pub(crate) async fn run_exec( } } +fn execution_failure_text(failure: &guard::wire::ExecutionFailure) -> String { + let errno = failure + .errno + .map(|errno| format!(", OS error {errno}")) + .unwrap_or_default(); + format!( + "EXECUTION FAILED [{}{errno}]: {}", + failure.stage.as_str(), + guard::gating::sanitize_gate_text(&failure.message) + ) +} + fn print_execute_response_json( kind: &str, binary: &str, @@ -655,6 +680,9 @@ fn exit_for_execute_response(response: &server::ExecuteResponse) -> ! { if let Some(failure) = response.containment_failure.as_ref() { std::process::exit(containment_failure_exit_code(failure)); } + if response.execution_failure.is_some() { + std::process::exit(EXIT_GUARD_ERROR); + } if !response.allowed { std::process::exit(EXIT_GUARD_DENIED); } @@ -846,6 +874,9 @@ pub(crate) fn provisional_detail_human(item: &server::ProvisionalSummary) -> Str } fn render_approval(item: &server::ApprovalSummary, include_transcript: bool) { + if let Some(failure) = &item.execution_failure { + eprintln!("{}", execution_failure_text(failure)); + } cli_println!( "[{}] handle={} cmd={:?} deadline={} reason={:?}", item.status, @@ -1116,19 +1147,26 @@ pub(crate) async fn handle_resume( .map_err(|error| describe_connect_failure(error, &client, source))?; match response { server::AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message, exit_code, stdout, stderr, } => { if json { - print_json(&resume_json_response( + let mut document = resume_json_response( &handle, &message, exit_code, stdout.as_deref(), stderr.as_deref(), - ))?; + ); + document["policy"] = serde_json::to_value(policy)?; + document["execution_failure"] = serde_json::to_value(&execution_failure)?; + document["decision_source"] = serde_json::to_value(decision_source)?; + print_json(&document)?; } else { if let Some(stdout) = stdout.as_deref() { cli_print!("{stdout}"); @@ -1145,6 +1183,12 @@ pub(crate) async fn handle_resume( None => eprintln!("exit status: unavailable"), } } + if let Some(failure) = execution_failure { + if !json { + eprintln!("{}", execution_failure_text(&failure)); + } + std::process::exit(EXIT_GUARD_ERROR); + } if let Some(code) = exit_code.filter(|code| *code != 0) { std::process::exit(code); } @@ -3446,11 +3490,19 @@ pub(crate) async fn handle_gate_action( .map_err(|e| describe_connect_failure(e, &client, source))? { server::AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message, exit_code, stdout, stderr, } => { + let _ = (policy, decision_source); + if let Some(failure) = execution_failure { + eprintln!("{}", execution_failure_text(&failure)); + std::process::exit(EXIT_GUARD_ERROR); + } cli_println!("{}", message); if let Some(out) = &stdout { cli_print!("{}", out); @@ -4245,6 +4297,8 @@ mod tests { #[test] fn denied_guidance_lists_every_durable_request_exactly() { let response = server::ExecuteResponse { + policy: None, + execution_failure: None, allowed: false, reason: "access required".to_string(), exit_code: None, @@ -4321,6 +4375,8 @@ mod tests { confirm_window_secs: Option, ) -> server::ExecuteResponse { server::ExecuteResponse { + policy: None, + execution_failure: None, allowed: true, reason: "recoverable change".to_string(), exit_code: Some(0), @@ -4553,6 +4609,8 @@ mod tests { fn denied_response(decision_source: &str) -> server::ExecuteResponse { server::ExecuteResponse { + policy: None, + execution_failure: None, allowed: false, reason: "rejected".to_string(), exit_code: None, @@ -5381,6 +5439,8 @@ mod tests { #[test] fn execute_json_envelope_keeps_decision_output_and_child_status() { let response = server::ExecuteResponse { + policy: None, + execution_failure: None, allowed: true, reason: "trusted verb".to_string(), exit_code: Some(75), diff --git a/src/gating/approval.rs b/src/gating/approval.rs index 1a81de0b..6ebdb499 100644 --- a/src/gating/approval.rs +++ b/src/gating/approval.rs @@ -225,6 +225,8 @@ pub struct ApprovalNote { /// One held command awaiting operator approval. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Approval { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_failure: Option, pub handle: String, pub snapshot: ApprovalSnapshot, /// Caller-facing rationale for the hold (the evaluator's allow reason). @@ -267,6 +269,9 @@ impl Approval { } let mut changed = sanitize(&mut self.reason); + if let Some(failure) = self.execution_failure.as_mut() { + changed |= sanitize(&mut failure.message); + } if let Some(trace) = self.decision_trace.as_mut() { changed |= trace.sanitize_explanatory_text(); } @@ -763,6 +768,7 @@ mod tests { fn held(handle: &str, created: u64, ttl: u64) -> Approval { Approval { + execution_failure: None, handle: handle.to_string(), snapshot: snap("rm"), reason: "destructive".into(), diff --git a/src/main.rs b/src/main.rs index fb79b2ba..157dfe8b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -245,14 +245,14 @@ enum MainArgs { disable_help_flag = true, after_help = "Use `guard run --help` to pass --help to the child command.\n\n\ Exit codes:\n \ - 125 guard operational error (daemon unreachable, protocol failure)\n \ + 125 guard operational error (connection, protocol, or execution failure)\n \ 126 denied by policy\n \ 127 held for operator approval\n \ 2 invalid guard CLI usage\n \ other the child's own exit status, propagated untranslated\n\n\ A child can itself exit 125-127 (`sh -c` exits 127 for a missing command;\n\ `git bisect skip` uses 125), so the exit code alone cannot prove a\n\ - guard-origin outcome. Use --json and read `allowed`/`status` for certainty." + guard-origin outcome. Use --json and read `policy`/`execution_failure`/`allowed`/`status` for certainty." )] Run { /// Override the daemon socket or Windows named pipe. diff --git a/src/mcp.rs b/src/mcp.rs index 763eb9c9..fd352a48 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -147,6 +147,8 @@ use guard::wire::mcp::{ #[derive(Debug, Clone)] struct GuardToolResponse { + policy: Option, + execution_failure: Option, allowed: bool, reason: String, exit_code: Option, @@ -177,6 +179,8 @@ struct AccessRequestArgs { impl From for GuardToolResponse { fn from(response: server::ExecuteResponse) -> Self { Self { + policy: response.policy, + execution_failure: response.execution_failure, allowed: response.allowed, reason: response.reason, exit_code: response.exit_code, @@ -1780,6 +1784,20 @@ impl McpServer { "containment_failure".to_string(), containment_failure_schema, ); + properties.insert( + "policy".to_string(), + json!({ + "type": ["object", "null"], "properties": { + "allowed": {"type": "boolean"}, "reason": {"type": "string"} + }, "required": ["allowed", "reason"], "additionalProperties": false + }), + ); + properties.insert("execution_failure".to_string(), json!({ + "type": ["object", "null"], "properties": { + "started": {"type": "boolean"}, "stage": {"type": "string"}, + "errno": {"type": ["integer", "null"]}, "message": {"type": "string"} + }, "required": ["started", "stage", "errno", "message"], "additionalProperties": false + })); output["required"] .as_array_mut() .expect("execution output required fields") @@ -2022,28 +2040,42 @@ impl McpServer { .await { Ok(server::AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message, exit_code, stdout, stderr, }) => { + let label = if execution_failure.is_some() { + "EXECUTION FAILED: " + } else { + "" + }; let text = format!( - "{message}\n{}{}", + "{label}{message}\n{}{}", stdout.as_deref().unwrap_or_default(), stderr.as_deref().unwrap_or_default() ); - admin_tool_result( + let is_error = execution_failure.is_some(); + let mut response = admin_tool_result( "approval_resume", text, json!({ "result": { + "policy": policy, + "execution_failure": execution_failure, + "decision_source": decision_source, "message": message, "exit_code": exit_code, "stdout": stdout, "stderr": stderr, } }), - ) + ); + response["isError"] = json!(is_error); + response } Ok(server::AdminResponse::Error { message }) => tool_error_result(message), Ok(_) => tool_error_result("unexpected response from guard daemon".to_string()), @@ -2172,6 +2204,13 @@ fn render_approval_text(item: &server::ApprovalSummary) -> String { "{} status={} command={} deadline={}", item.handle, item.status, item.command, item.deadline_unix ); + if let Some(failure) = &item.execution_failure { + line.push_str(&format!( + "\nEXECUTION FAILED [{}]: {}", + failure.stage.as_str(), + guard::gating::sanitize_gate_text(&failure.message) + )); + } if let Some(reason) = item.decided_reason.as_deref() { line.push_str(&format!("\nreason: {reason}")); } @@ -2373,7 +2412,8 @@ fn jsonrpc_error_response(id: Value, code: i64, message: String, data: Option Value { - let is_error = result.containment_failure.is_some() + let is_error = result.execution_failure.is_some() + || result.containment_failure.is_some() || (result.auto_revert_durable == Some(false) && result.containment_failure.is_none()); let structured = json!({ "schema_version": TOOL_SCHEMA_VERSION, @@ -2389,6 +2429,8 @@ fn tool_result(result: GuardToolResponse) -> Value { "confirm_window_secs": result.confirm_window_secs, "auto_revert_durable": result.auto_revert_durable, "containment_failure": result.containment_failure, + "policy": result.policy, + "execution_failure": result.execution_failure, "approval_options": result.approval_options, "access_requests": result.access_requests, "coverage": result.coverage, @@ -2424,6 +2466,8 @@ fn tool_error_result(message: String) -> Value { "confirm_window_secs": Value::Null, "auto_revert_durable": Value::Null, "containment_failure": Value::Null, + "policy": Value::Null, + "execution_failure": Value::Null, "approval_options": [], "access_requests": [], "coverage": Value::Null, @@ -2538,6 +2582,24 @@ fn render_tool_text(result: &Value) -> String { return out; } + if let Some(failure) = result + .get("execution_failure") + .filter(|failure| failure.is_object()) + { + let stage = failure + .get("stage") + .and_then(Value::as_str) + .unwrap_or("unknown"); + let message = failure + .get("message") + .and_then(Value::as_str) + .unwrap_or(reason); + return format!( + "EXECUTION FAILED [{stage}]: {}{decision}", + guard::gating::sanitize_gate_text(message) + ); + } + // Consequence-gate outcomes are not denials: surface the handle, the next // step, and the honest coverage so the model knows what was NOT verified. match status { @@ -2927,6 +2989,7 @@ mod tests { fn approval_summary(status: &str) -> server::ApprovalSummary { server::ApprovalSummary { + execution_failure: None, handle: "approval-example".to_string(), status: status.to_string(), command: "approved-command".to_string(), @@ -3304,6 +3367,8 @@ mod tests { async fn initialize_advertises_tools_capability() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -3524,6 +3589,8 @@ mod tests { async fn tools_list_returns_guard_tool() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -3889,6 +3956,8 @@ mod tests { async fn tool_call_returns_structured_output() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "allowed by policy".to_string(), exit_code: Some(0), @@ -4008,6 +4077,8 @@ mod tests { #[test] fn denied_tool_results_are_not_transport_errors() { let value = tool_result(GuardToolResponse { + policy: None, + execution_failure: None, allowed: false, reason: "policy denied".to_string(), exit_code: None, @@ -4044,6 +4115,8 @@ mod tests { (true, Some("executed"), None), ] { let value = tool_result(GuardToolResponse { + policy: None, + execution_failure: None, allowed, reason: "fixture result".to_string(), exit_code: Some(0), @@ -4082,6 +4155,8 @@ mod tests { fn execute_response_fixture() -> server::ExecuteResponse { server::ExecuteResponse { + policy: None, + execution_failure: None, allowed: true, reason: "recoverable change".to_string(), exit_code: Some(0), @@ -4103,6 +4178,28 @@ mod tests { } } + #[test] + fn execution_failure_mcp_is_an_error_without_denial_guidance() { + let response: server::ExecuteResponse = serde_json::from_value(json!({ + "allowed": false, "reason": "working directory permission denied", + "policy": {"allowed": true, "reason": "policy permits command"}, + "execution_failure": {"started": false, "stage": "cwd", "errno": 13, "message": "working directory permission denied"}, + "decision_source": "static_policy", "verb_guidance": "ask for approval" + })).unwrap(); + let value = tool_result(response.into()); + assert_eq!(value["isError"], true); + assert_eq!(value["structuredContent"]["allowed"], false); + assert_eq!(value["structuredContent"]["policy"]["allowed"], true); + assert_eq!( + value["structuredContent"]["execution_failure"]["stage"], + "cwd" + ); + let text = value["content"][0]["text"].as_str().unwrap(); + assert!(text.starts_with("EXECUTION FAILED")); + assert!(!text.contains("DENIED")); + assert!(!text.contains("ask for approval")); + } + #[test] fn mcp_provisional_result_preserves_and_renders_confirmation_window() { let value = tool_result(execute_response_fixture().into()); @@ -4254,6 +4351,8 @@ mod tests { async fn request_missing_method_gets_invalid_request_error() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -4292,6 +4391,8 @@ mod tests { async fn tools_list_has_stable_order_and_access_request_schema() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -4391,6 +4492,8 @@ mod tests { async fn verb_list_tool_proxies_daemon_catalog() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -4461,6 +4564,8 @@ mod tests { async fn access_list_tool_proxies_non_mutating_access_state() { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), @@ -4595,6 +4700,8 @@ mod tests { fn http_test_server() -> McpServer { let executor = Arc::new(FakeExecutor { response: Ok(GuardToolResponse { + policy: None, + execution_failure: None, allowed: true, reason: "ok".to_string(), exit_code: Some(0), diff --git a/src/server/admin.rs b/src/server/admin.rs index 5ddfb172..0b2bcc18 100644 --- a/src/server/admin.rs +++ b/src/server/admin.rs @@ -187,6 +187,7 @@ mod regeneration_proposal_tests { fn held_approval(handle: &str) -> Approval { Approval { + execution_failure: None, handle: handle.to_string(), snapshot: guard::gating::approval::ApprovalSnapshot { binary: "host-maintain".to_string(), @@ -3645,6 +3646,7 @@ async fn handle_session_appeal( server, Some(&token), SessionInteraction { + execution_failure: None, at_unix: 0, command: command_line.clone(), allowed: false, @@ -3702,6 +3704,7 @@ async fn handle_session_appeal( server, Some(&token), SessionInteraction { + execution_failure: None, at_unix: 0, command: command_line.clone(), allowed: true, @@ -3770,6 +3773,7 @@ async fn handle_session_appeal( server, Some(&token), SessionInteraction { + execution_failure: None, at_unix: 0, command: command_line.clone(), allowed: false, @@ -7143,6 +7147,9 @@ async fn handle_confirm( behavior: None, }); AdminResponse::GateAction { + policy: None, + execution_failure: None, + decision_source: None, message: format!("provisional {} confirmed; change kept", handle), exit_code: None, stdout: None, @@ -7315,6 +7322,9 @@ async fn handle_manual_revert( } let outcome = finish_revert(server, &claimed, caller, "manual").await; AdminResponse::GateAction { + policy: None, + execution_failure: None, + decision_source: None, message: outcome.0, exit_code: outcome.1, stdout: None, @@ -7620,6 +7630,9 @@ async fn arm_held_command( behavior: None, }); AdminResponse::GateAction { + policy: None, + execution_failure: None, + decision_source: None, message: format!("approved held command {handle}; awaiting requester-bound resume"), exit_code: None, stdout: None, @@ -7692,6 +7705,9 @@ async fn handle_approve_claimed( behavior: None, }); return AdminResponse::GateAction { + policy: None, + execution_failure: None, + decision_source: None, message: format!("approved held API request {handle}; the proxy is forwarding it"), exit_code: None, stdout: None, @@ -7804,16 +7820,26 @@ async fn handle_approve_claimed( message: super::AUDIT_UNAVAILABLE_REASON.to_string(), }; } - let reason = format!("operator-approved held command {}", handle); + let Some(approval) = server.state.approvals.read().await.get(handle).cloned() else { + return AdminResponse::Error { + message: format!("approval {handle} disappeared before execution"), + }; + }; + let reason = approval.reason.clone(); drop(_held_verb_lease); - let result = super::gate_runtime::execute_snapshot(server, &snapshot, &reason).await; + let result = super::gate_runtime::execute_snapshot(server, &snapshot, &reason) + .await + .with_admission_trace(approval.decision_trace.as_ref()); let now = now_unix(); let Some(expected) = server.state.approvals.read().await.get(handle).cloned() else { return AdminResponse::Error { message: format!("approval {handle} disappeared before terminal persistence"), }; }; - let (message, exit, stdout, stderr, next) = match result.exec { + let policy = Some(result.policy_decision()); + let execution_failure = result.execution_failure().cloned(); + let decision_source = Some(result.decision_source().to_string()); + let (message, exit, stdout, stderr, mut next) = match result.exec { ExecOutcome::Completed { exit_code, stdout, @@ -7847,6 +7873,11 @@ async fn handle_approve_claimed( ExecOutcome::Failed { reason: detail, .. } => { server.emit_audit_ungated( AuditEvent::new(AuditKind::ApproveExecFailed) + .execution( + policy.clone().expect("execution policy"), + execution_failure.clone(), + ) + .decision_source(decision_source.as_deref().unwrap_or("validation")) .handle(handle) .caller(caller) .session_fingerprint(snapshot.session_fingerprint.as_deref().unwrap_or("none")) @@ -7879,6 +7910,7 @@ async fn handle_approve_claimed( ) } }; + next.execution_failure = execution_failure.clone(); let terminal_status = next.status.as_str().to_string(); if let Err(message) = commit_terminal_approval(server, expected, next).await { return AdminResponse::Error { message }; @@ -7893,9 +7925,17 @@ async fn handle_approve_claimed( status: Some(terminal_status), behavior: None, }); + let exit_code = if execution_failure.is_some() { + Some(crate::EXIT_GUARD_ERROR) + } else { + exit + }; AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message, - exit_code: exit, + exit_code, stdout, stderr, } @@ -7990,18 +8030,32 @@ async fn handle_resume( handle: &str, ) -> AdminResponse { let result = resume_approval(server, caller, handle).await; + let policy = Some(result.policy_decision()); + let execution_failure = result.execution_failure().cloned(); + let decision_source = Some(result.decision_source().to_string()); match result.exec.clone() { ExecOutcome::Completed { exit_code, stdout, stderr, } => AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message: format!("resumed held command {handle} (exit {exit_code:?})"), exit_code, stdout, stderr, }, - ExecOutcome::Failed { reason, .. } => AdminResponse::Error { message: reason }, + ExecOutcome::Failed { reason, .. } => AdminResponse::GateAction { + policy, + execution_failure, + decision_source, + message: reason, + exit_code: Some(crate::EXIT_GUARD_ERROR), + stdout: None, + stderr: None, + }, ExecOutcome::NotAttempted => AdminResponse::Error { message: result.policy_reason().to_string(), }, @@ -8163,6 +8217,9 @@ async fn handle_deny( behavior: None, }); AdminResponse::GateAction { + policy: None, + execution_failure: None, + decision_source: None, message: reason.to_string(), exit_code: None, stdout: None, diff --git a/src/server/execute.rs b/src/server/execute.rs index aade3957..aa661861 100644 --- a/src/server/execute.rs +++ b/src/server/execute.rs @@ -850,6 +850,7 @@ async fn deny_and_record( phase.server, phase.session_token.as_deref(), SessionInteraction { + execution_failure: None, at_unix: 0, command: durable_command, allowed: false, @@ -907,6 +908,7 @@ async fn route_allow_and_record( phase.server, phase.session_token.as_deref(), SessionInteraction { + execution_failure: None, at_unix: 0, command: interaction_command, allowed: true, @@ -5328,6 +5330,7 @@ mod transactional_access_tests { advanced.record_interaction( &token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "fixture interaction".to_string(), allowed: true, @@ -5517,6 +5520,7 @@ mod transactional_access_tests { sessions.record_interaction( &token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "newer interaction".to_string(), allowed: false, diff --git a/src/server/gate_runtime.rs b/src/server/gate_runtime.rs index 2a6404c3..490d0bfe 100644 --- a/src/server/gate_runtime.rs +++ b/src/server/gate_runtime.rs @@ -1260,6 +1260,7 @@ impl guard::proxy::GateSink for DaemonGateSink { secret_binding: None, }; let approval = Approval { + execution_failure: None, handle: handle.clone(), snapshot, reason: reason.to_string(), @@ -2770,6 +2771,7 @@ pub(super) async fn hold_for_approval_with_trace( secret_binding, }; let approval = Approval { + execution_failure: None, handle: handle.clone(), snapshot, reason: reason.clone(), @@ -3058,8 +3060,10 @@ pub(super) async fn resume_approval( ); } - let reason = format!("requester resumed operator-approved hold {handle}"); - let result = execute_snapshot(server, &claimed.snapshot, &reason).await; + let reason = claimed.reason.clone(); + let result = execute_snapshot(server, &claimed.snapshot, &reason) + .await + .with_admission_trace(claimed.decision_trace.as_ref()); let completed_unix = now_unix(); let mut terminal = claimed.clone(); match &result.exec { @@ -3079,6 +3083,7 @@ pub(super) async fn resume_approval( terminal.status = ApprovalStatus::ExecFailed; terminal.decided_unix = Some(completed_unix); terminal.decided_reason = Some(reason.clone()); + terminal.execution_failure = result.execution_failure().cloned(); terminal.result_exit = None; terminal.result_stdout = None; terminal.result_stderr = None; @@ -3096,10 +3101,16 @@ pub(super) async fn resume_approval( if let Err(error) = commit_resumed_approval(server, claimed.clone(), terminal.clone(), true).await { - return ExecuteResult::exec_failed( - reason, - format!("held command ran but its result was not durable: {error}"), - ); + let message = format!("held command result was not durable: {error}"); + let failed = if matches!( + &result.exec, + ExecOutcome::Failed { started: false, .. } | ExecOutcome::NotAttempted + ) { + ExecuteResult::exec_failed(reason, message) + } else { + ExecuteResult::exec_failed_after_start(reason, message) + }; + return failed.with_admission_trace(claimed.decision_trace.as_ref()); } server.emit_audit_ungated( AuditEvent::new(AuditKind::ApprovedExecuted) @@ -3131,7 +3142,7 @@ pub(super) async fn resume_approval( /// Build the client-facing result from a decided approval record. pub(super) fn approval_to_result(a: &Approval) -> ExecuteResult { - match a.status { + let result = match a.status { ApprovalStatus::Approved => ExecuteResult::completed( a.reason.clone(), a.result_exit, @@ -3151,11 +3162,13 @@ pub(super) fn approval_to_result(a: &Approval) -> ExecuteResult { a.decided_reason .clone() .unwrap_or_else(|| "approved command failed to execute".to_string()), - ), + ) + .with_execution_failure(a.execution_failure.clone()), ApprovalStatus::Pending | ApprovalStatus::Approving => { ExecuteResult::held(a.reason.clone(), a.handle.clone(), Coverage::hold()) } - } + }; + result.with_admission_trace(a.decision_trace.as_ref()) } /// Sentinel stored in a [`SecretBinding`] for a secret that did not resolve at diff --git a/src/server/grants.rs b/src/server/grants.rs index ce55301b..66cab689 100644 --- a/src/server/grants.rs +++ b/src/server/grants.rs @@ -313,14 +313,15 @@ pub(super) async fn handle_grant_read( // Nothing survived the in-apply rollback, so drop the committed row too. delete_read_grant_row(server, &grant.target_path).await; let exec_reason = format!("failed to apply read grant: {e}"); + let result = ExecuteResult::exec_failed(reason, exec_reason); server.log_audit_exec_failed( caller, session_token.as_deref(), AUTO_READ_GRANT_LABEL, &audit_args, - &exec_reason, + &result, ); - return ExecuteResult::exec_failed(reason, exec_reason); + return result; } let traverse_count = grant.entries.len().saturating_sub(1); diff --git a/src/server/mod.rs b/src/server/mod.rs index b6bae901..726099e1 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -649,25 +649,33 @@ impl ServerContext { self.log_audit_policy(caller, session_token, binary, args, true, reason) } - /// Log a failed exec attempt. Only emitted when the policy allowed - /// the command but the kernel refused to run it (ENOENT, EACCES, - /// etc.). Paired with a corresponding `[AUDIT] ALLOWED` line so - /// downstream tooling can distinguish "policy denied" from "policy - /// approved, exec failed". + /// Record an execution failure separately from admission. Typed detail + /// retains whether the command started and names a stage only when that + /// failing operation is observed. fn log_audit_exec_failed( &self, caller: &CallerIdentity, session_token: Option<&str>, binary: &str, args: &[String], - reason: &str, + result: &wire::ExecuteResult, ) { self.emit_audit_ungated( AuditEvent::new(AuditKind::ExecFailed) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()) .caller(caller) .session_fingerprint(audit_session_fingerprint(session_token)) .cmd(self.redact_command_line(binary, args)) - .reason(reason), + .reason( + result + .execution_failure() + .map(|failure| failure.message.as_str()) + .unwrap_or("execution failed"), + ), ); } } diff --git a/src/server/tests/gating.rs b/src/server/tests/gating.rs index 54377f01..1200a9a6 100644 --- a/src/server/tests/gating.rs +++ b/src/server/tests/gating.rs @@ -3812,6 +3812,7 @@ async fn held_access_projection_expires_before_the_sweeper_and_hides_approval_op let handle = "held-access-projection".to_string(); let principal = PrincipalKey::from_uid(1_001); cfg.state.approvals.write().await.enqueue(Approval { + execution_failure: None, handle: handle.clone(), snapshot: ApprovalSnapshot { binary: "true".to_string(), @@ -5930,6 +5931,7 @@ fn held_verb_approval( principal: Option, ) -> Approval { Approval { + execution_failure: None, handle: handle.to_string(), snapshot: ApprovalSnapshot { binary: "true".to_string(), diff --git a/src/server/tests/sessions.rs b/src/server/tests/sessions.rs index 107ab87f..34ab8ef2 100644 --- a/src/server/tests/sessions.rs +++ b/src/server/tests/sessions.rs @@ -3132,6 +3132,7 @@ async fn revoked_access_session_cannot_be_resurrected_by_pending_extension() { ) }; let held = |handle: &str| Approval { + execution_failure: None, handle: handle.to_string(), snapshot: ApprovalSnapshot { binary: "true".to_string(), @@ -4925,6 +4926,7 @@ async fn session_inspection_surfaces_redact_credentials_in_text_and_json() { vec![crate::session::StoredSessionInteraction::from_typed_parts( token.clone(), SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: format!("kubectl --token={} get pods", fixture_bearer_jwt()), allowed: true, @@ -5384,6 +5386,7 @@ async fn session_show_reports_recent_stats() { reg.record_interaction_with_credential_references( &token, SessionInteraction { + execution_failure: None, at_unix: now.saturating_sub(1), command: "echo hi".into(), allowed: true, @@ -5403,6 +5406,7 @@ async fn session_show_reports_recent_stats() { reg.record_interaction( &token, SessionInteraction { + execution_failure: None, at_unix: now, command: "rm -rf /tmp/x".into(), allowed: false, @@ -5521,6 +5525,7 @@ async fn session_status_self_view_redacts_bearer_and_keeps_decision_trace() { cfg.state.sessions.write().await.record_interaction( &token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "uptime".to_string(), allowed: true, @@ -5932,6 +5937,7 @@ async fn grant_request_submit_enforces_suspension_quota_and_aggregate_size() { cfg.state.sessions.write().await.record_interaction( "suspended-request", SessionInteraction { + execution_failure: None, command: "denied".to_string(), allowed: false, source: SessionDecisionSource::Llm, @@ -6812,6 +6818,7 @@ async fn evaluate_batch_requires_owned_live_unsuspended_session_or_admin() { cfg.state.sessions.write().await.record_interaction( "batch-owner", SessionInteraction { + execution_failure: None, command: "denied".to_string(), allowed: false, source: SessionDecisionSource::Llm, diff --git a/src/server/tests/verbs.rs b/src/server/tests/verbs.rs index e65dccfc..df23ab4b 100644 --- a/src/server/tests/verbs.rs +++ b/src/server/tests/verbs.rs @@ -535,6 +535,7 @@ async fn interaction_suspension_before_process_start_denies_execution() { server.state.sessions.write().await.record_interaction( token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "denied command".to_string(), allowed: false, @@ -682,6 +683,7 @@ async fn held_replay_rejects_interaction_suspension_before_process_start() { server.state.sessions.write().await.record_interaction( token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "denied command".to_string(), allowed: false, diff --git a/src/server/tests/wire.rs b/src/server/tests/wire.rs index c68097c8..9c200cdd 100644 --- a/src/server/tests/wire.rs +++ b/src/server/tests/wire.rs @@ -415,3 +415,117 @@ fn containment_failure_is_parseable_by_origin_main_clients_in_both_response_shap } // ---- Audit emission end-to-end tests ------------------------------------ + +#[test] +fn execution_failure_preserves_policy_in_buffered_and_streamed_results() { + use guard::wire::ExecutionStage; + for stage in [ + ExecutionStage::Identity, + ExecutionStage::Capabilities, + ExecutionStage::Cwd, + ExecutionStage::Exec, + ExecutionStage::Unknown, + ] { + let result = ExecuteResult::launch_failed( + "permitted by policy", + stage, + Some(13), + "permission denied", + ) + .with_decision_source(crate::session::SessionDecisionSource::StaticPolicy); + let response = result.into_response(); + assert!(!response.allowed); + assert!(response.policy.as_ref().unwrap().allowed); + assert_eq!( + response.policy.as_ref().unwrap().reason, + "permitted by policy" + ); + assert_eq!(response.decision_source, "static_policy"); + let failure = response.execution_failure.as_ref().unwrap(); + assert!(!failure.started); + assert_eq!(failure.stage, stage); + assert_eq!(failure.errno, Some(13)); + let encoded = serde_json::to_value(&response).unwrap(); + let decoded: crate::server::ExecuteResponse = + serde_json::from_value(encoded.clone()).unwrap(); + assert_eq!(decoded.execution_failure, response.execution_failure); + let stream = serde_json::to_value(ExecuteStreamMessage::Result { response }).unwrap(); + assert_eq!(stream["response"], encoded); + } +} + +#[test] +fn execution_failure_unknown_preserves_started_and_legacy_deserialization() { + use guard::wire::ExecutionStage; + for result in [ + ExecuteResult::exec_failed("allow", "setup failed"), + ExecuteResult::exec_failed_after_start("allow", "stream lost"), + ] { + let started = matches!(result.exec, ExecOutcome::Failed { started: true, .. }); + let response = result.into_response(); + let failure = response.execution_failure.as_ref().unwrap(); + assert_eq!(failure.started, started); + assert_eq!(failure.stage, ExecutionStage::Unknown); + assert_eq!(failure.errno, None); + } + let legacy: crate::server::ExecuteResponse = + serde_json::from_value(serde_json::json!({"allowed":false,"reason":"denied"})).unwrap(); + assert!(legacy.policy.is_none()); + assert!(legacy.execution_failure.is_none()); + let unknown: guard::wire::ExecutionFailure = serde_json::from_value( + serde_json::json!({"started":false,"stage":"future_stage","errno":null,"message":"failed"}), + ) + .unwrap(); + assert_eq!(unknown.stage, ExecutionStage::Unknown); + let denied = ExecuteResult::denied("policy rejects this").into_response(); + assert!(!denied.policy.unwrap().allowed); + assert!(denied.execution_failure.is_none()); +} + +#[test] +fn execution_failure_audit_preserves_typed_details_and_redacts_prose() { + let result = ExecuteResult::launch_failed( + "password=fixture-private", + guard::wire::ExecutionStage::Cwd, + Some(13), + "password=fixture-private", + ) + .with_decision_source(crate::session::SessionDecisionSource::StaticPolicy); + let event = guard::audit::AuditEvent::new(guard::audit::AuditKind::ExecFailed) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()); + let json = serde_json::to_value(&event).unwrap(); + assert_eq!(json["policy"]["allowed"], true); + assert_eq!(json["execution_failure"]["stage"], "cwd"); + assert_eq!(json["execution_failure"]["errno"], 13); + assert!(!json.to_string().contains("fixture-private")); + assert!(!event.render_line().contains("fixture-private")); +} + +#[test] +fn execution_failure_held_projection_retains_admission_and_started_state() { + for started in [false, true] { + let approval: guard::gating::approval::Approval = serde_json::from_value(serde_json::json!({ + "handle": "ap-failure", "snapshot": { "binary": "true", "args": [], "env": {}, "secret_keys": {}, "verb_params": {} }, + "reason": "policy permits command", "created_unix": 1, "ttl_secs": 3600, "status": "exec_failed", + "decision_trace": guard::gating::DecisionTrace::source("static_policy"), + "execution_failure": {"started": started, "stage": "unknown", "errno": null, "message": "execution failed"} + })).unwrap(); + let summary = crate::server::wire::ApprovalSummary::from_row(&approval); + assert_eq!(summary.execution_failure, approval.execution_failure); + let response = crate::server::gate_runtime::approval_to_result(&approval).into_response(); + assert_eq!(response.execution_failure.unwrap().started, started); + assert_eq!(response.decision_source, "static_policy"); + assert_eq!( + response.policy.unwrap(), + guard::wire::PolicyDecision { + allowed: true, + reason: "policy permits command".into() + } + ); + assert!(!response.allowed); + } +} diff --git a/src/server/transport.rs b/src/server/transport.rs index 63bbd924..ca6c3757 100644 --- a/src/server/transport.rs +++ b/src/server/transport.rs @@ -231,6 +231,7 @@ mod admin_response_lease_tests { fn approval() -> Approval { Approval { + execution_failure: None, handle: "lease-test".to_string(), snapshot: ApprovalSnapshot { binary: "true".to_string(), @@ -455,6 +456,7 @@ impl guard::proxy::ApiSessionSink for DaemonApiSessionSink { &self.server, Some(token), SessionInteraction { + execution_failure: None, at_unix: 0, command: format!("api:{} {}", event.endpoint, event.operation), allowed: event.allowed, @@ -1959,6 +1961,8 @@ where /// Denial response for a request rejected before policy evaluation. fn validation_error_response(reason: String) -> ExecuteResponse { ExecuteResponse { + policy: None, + execution_failure: None, allowed: false, reason, exit_code: None, @@ -2346,8 +2350,8 @@ pub(super) fn emit_audit_events( // If the policy allowed but exec failed, emit a second event so the // audit stream can distinguish "LLM denied" from "LLM approved but // exec failed". Ignored by legacy grep patterns. - if let ExecOutcome::Failed { reason, .. } = &result.exec { - server.log_audit_exec_failed(caller, None, binary, args, reason); + if let ExecOutcome::Failed { .. } = &result.exec { + server.log_audit_exec_failed(caller, None, binary, args, result); } } @@ -2359,8 +2363,8 @@ fn emit_exec_audit_events( args: &[String], result: &ExecuteResult, ) { - if let ExecOutcome::Failed { reason, .. } = &result.exec { - server.log_audit_exec_failed(caller, session_token, binary, args, reason); + if let ExecOutcome::Failed { .. } = &result.exec { + server.log_audit_exec_failed(caller, session_token, binary, args, result); } } diff --git a/src/server/wire.rs b/src/server/wire.rs index 68894f99..0f47d697 100644 --- a/src/server/wire.rs +++ b/src/server/wire.rs @@ -9,6 +9,7 @@ use guard::gating::approval::{bound_approval_transcript, Approval, WaiterLease}; use guard::gating::provisional::{Provisional, ProvisionalStatus}; use guard::gating::{Coverage, DecisionTrace, DecisionVerbMatch}; use guard::principal::PrincipalKey; +use guard::wire::{ExecutionFailure, ExecutionStage, PolicyDecision}; use serde::{Deserialize, Serialize}; use super::execute::audit_session_fingerprint; @@ -751,6 +752,12 @@ pub enum AdminResponse { /// A gate action ran (confirm/revert/approve/deny). Carries a human message /// and, for approve/revert, the resulting exit/output. GateAction { + #[serde(default, skip_serializing_if = "Option::is_none")] + policy: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + execution_failure: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + decision_source: Option, message: String, #[serde(default, skip_serializing_if = "Option::is_none")] exit_code: Option, @@ -1073,6 +1080,8 @@ pub struct ProvisionalSummary { /// Operator-facing view of a held/decided approval. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ApprovalSummary { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_failure: Option, pub handle: String, pub status: String, pub command: String, @@ -1170,6 +1179,7 @@ impl ApprovalSummary { let (stdout, stdout_truncated) = exposed_transcript(a.result_stdout.as_deref()); let (stderr, stderr_truncated) = exposed_transcript(a.result_stderr.as_deref()); Self { + execution_failure: a.execution_failure.clone(), handle: a.handle.clone(), status: if approval_is_armed(&a) { "armed".to_string() @@ -1311,6 +1321,10 @@ pub(crate) const EXECUTE_FEATURE_TCP_NO_CWD: &str = "tcp-no-cwd-v1"; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ExecuteResponse { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub policy: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_failure: Option, pub allowed: bool, pub reason: String, #[serde(skip_serializing_if = "Option::is_none")] @@ -1609,6 +1623,7 @@ struct ExecutionAuditMetadata { } pub(super) struct ExecuteResult { + execution_failure: Option, policy: PolicyOutcome, pub(super) exec: ExecOutcome, request_handle: Option, @@ -1640,6 +1655,7 @@ impl ExecuteResult { access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1666,6 +1682,7 @@ impl ExecuteResult { access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1678,18 +1695,25 @@ impl ExecuteResult { policy_reason: impl Into, exec_reason: impl Into, ) -> Self { + let exec_reason = Self::sanitize_prose(exec_reason); Self { policy: PolicyOutcome::Allowed { reason: Self::sanitize_prose(policy_reason), }, exec: ExecOutcome::Failed { - reason: Self::sanitize_prose(exec_reason), + reason: exec_reason.clone(), started: false, }, request_handle: None, access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: Some(ExecutionFailure { + started: false, + stage: ExecutionStage::Unknown, + errno: None, + message: exec_reason, + }), verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1703,30 +1727,82 @@ impl ExecuteResult { policy_reason: impl Into, exec_reason: impl Into, ) -> Self { + let exec_reason = Self::sanitize_prose(exec_reason); Self { policy: PolicyOutcome::Allowed { reason: Self::sanitize_prose(policy_reason), }, exec: ExecOutcome::Failed { - reason: Self::sanitize_prose(exec_reason), + reason: exec_reason.clone(), started: true, }, request_handle: None, access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: Some(ExecutionFailure { + started: true, + stage: ExecutionStage::Unknown, + errno: None, + message: exec_reason, + }), verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, } } + #[cfg_attr(not(test), allow(dead_code))] + pub(super) fn launch_failed( + policy_reason: impl Into, + stage: ExecutionStage, + errno: Option, + message: impl Into, + ) -> Self { + let message = Self::sanitize_prose(message); + Self::exec_failed(policy_reason, message.clone()).with_execution_failure(Some( + ExecutionFailure { + started: false, + stage, + errno, + message, + }, + )) + } + + pub(super) fn with_execution_failure(mut self, failure: Option) -> Self { + if let (ExecOutcome::Failed { reason, started }, Some(failure)) = (&mut self.exec, failure) + { + let failure = failure.sanitized(); + *reason = failure.message.clone(); + *started = failure.started; + self.execution_failure = Some(failure); + } + self + } + + pub(super) fn execution_failure(&self) -> Option<&ExecutionFailure> { + self.execution_failure.as_ref() + } + + pub(super) fn policy_decision(&self) -> PolicyDecision { + PolicyDecision { + allowed: self.policy_allowed(), + reason: self.policy_reason().to_string(), + } + } + + pub(super) fn decision_source(&self) -> &str { + self.decision_source.as_str() + } + pub(super) fn dry_run(reason: impl Into) -> Self { Self { policy: PolicyOutcome::Allowed { reason: Self::sanitize_prose(reason), }, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, exec: ExecOutcome::DryRun { coverage: None }, request_handle: None, access_requests: Vec::new(), @@ -1751,6 +1827,7 @@ impl ExecuteResult { access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1769,6 +1846,7 @@ impl ExecuteResult { access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1804,6 +1882,7 @@ impl ExecuteResult { access_requests: Vec::new(), operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), + execution_failure: None, verb_matches: Vec::new(), verb_guidance: None, decision_source: SessionDecisionSource::Validation, @@ -1945,6 +2024,15 @@ impl ExecuteResult { self } + pub(super) fn with_admission_trace(mut self, trace: Option<&DecisionTrace>) -> Self { + if let Some(trace) = trace { + self.decision_source = + serde_json::from_value(serde_json::Value::String(trace.decision_source.clone())) + .unwrap_or(SessionDecisionSource::Validation); + } + self + } + pub(super) fn with_decision_source(mut self, source: SessionDecisionSource) -> Self { self.decision_source = source; self @@ -1968,6 +2056,11 @@ impl ExecuteResult { /// Build the `ExecuteResponse` wire payload. Callers that need to emit /// audit events first should do so before consuming the result. pub(super) fn into_response(self) -> ExecuteResponse { + let policy = Some(self.policy_decision()); + let execution_failure = self + .execution_failure + .clone() + .map(ExecutionFailure::sanitized); let allowed = self.policy_allowed(); let request_handle = self.request_handle; let access_requests = self.access_requests; @@ -2010,13 +2103,14 @@ impl ExecuteResult { }; let policy_reason = guard::gating::sanitize_gate_text(&policy_reason); match self.exec { - // Legacy arms keep status/handle/coverage = None so a gating-off - // response is byte-identical to today's wire format. + // Ungated execution leaves the consequence-gate fields absent. ExecOutcome::Completed { exit_code, stdout, stderr, } => ExecuteResponse { + policy, + execution_failure, allowed: true, reason: policy_reason, exit_code, @@ -2039,11 +2133,10 @@ impl ExecuteResult { ExecOutcome::Failed { reason: exec_msg, .. } => ExecuteResponse { - // Even though the policy allowed it, the command could not - // actually run. Surface this to the client as `allowed=false` - // with the exec error as the reason, because from the - // client's perspective nothing ran successfully. The audit - // stream still records both POLICY=ALLOWED and EXEC_FAILED. + policy, + execution_failure, + // Legacy clients fail closed. Typed fields retain admission + // separately from the failure and whether the command started. allowed: false, reason: format!( "execution error: {}", @@ -2067,13 +2160,14 @@ impl ExecuteResult { decision_trace, }, ExecOutcome::DryRun { coverage } => ExecuteResponse { + policy, + execution_failure, allowed: true, reason: policy_reason, exit_code: Some(0), stdout: Some("[DRY-RUN] policy allowed; command was not executed\n".to_string()), stderr: None, - // A gated dry-run carries its coverage and a DryRun status; a - // plain dry-run stays byte-identical to the pre-gating wire. + // A gated dry-run carries its coverage and a DryRun status. status: coverage.as_ref().map(|_| GateStatus::DryRun), handle: None, approval_options: Vec::new(), @@ -2089,6 +2183,8 @@ impl ExecuteResult { decision_trace, }, ExecOutcome::NotAttempted => ExecuteResponse { + policy, + execution_failure, allowed, reason: policy_reason, exit_code: None, @@ -2114,6 +2210,8 @@ impl ExecuteResult { let approval_options = access_request.approval_options.clone(); let access_requests = vec![access_request]; ExecuteResponse { + policy, + execution_failure, // Approved but held: allowed=true (not a denial), no exit code. allowed: true, reason: policy_reason, @@ -2144,6 +2242,8 @@ impl ExecuteResult { deadline_unix, window_secs, } => ExecuteResponse { + policy, + execution_failure, allowed: true, reason: policy_reason, exit_code, @@ -2188,6 +2288,8 @@ impl ExecuteResult { ), }; ExecuteResponse { + policy, + execution_failure, allowed: false, reason, exit_code, diff --git a/src/session.rs b/src/session.rs index 388ba632..bf76e10b 100644 --- a/src/session.rs +++ b/src/session.rs @@ -317,6 +317,9 @@ impl SessionInteraction { /// state database) and again on every inspection surface (so historical /// rows written before sanitization existed cannot leak either). pub fn redact_credentials(&mut self) { + if let Some(failure) = self.execution_failure.as_mut() { + failure.message = guard::gating::sanitize_gate_text(&failure.message); + } sanitize_credentials(&mut self.command); sanitize_credentials(&mut self.reason); sanitize_credentials_vec(&mut self.exposed_secret_refs); @@ -647,6 +650,8 @@ impl<'de> Deserialize<'de> for CredentialReference { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SessionInteraction { + #[serde(default, skip_serializing_if = "Option::is_none")] + pub execution_failure: Option, pub at_unix: u64, pub command: String, pub allowed: bool, @@ -2257,6 +2262,7 @@ mod tests { ); let interaction = SessionInteraction { + execution_failure: None, at_unix: 1, command: "true".to_string(), allowed: true, @@ -2750,6 +2756,7 @@ mod tests { reg.record_interaction( "tok", SessionInteraction { + execution_failure: None, at_unix: 10, command: "cat /tmp/a".into(), allowed: true, @@ -2765,6 +2772,7 @@ mod tests { reg.record_interaction( "tok", SessionInteraction { + execution_failure: None, at_unix: 11, command: "rm -rf /tmp/a".into(), allowed: false, @@ -2780,6 +2788,7 @@ mod tests { reg.record_interaction( "tok", SessionInteraction { + execution_failure: None, at_unix: 12, command: "echo hi".into(), allowed: true, @@ -2846,6 +2855,7 @@ mod tests { reg.record_interaction( "tok", SessionInteraction { + execution_failure: None, at_unix: now, command: command.into(), allowed, @@ -2914,6 +2924,7 @@ mod tests { reg.record_interaction( "tok", SessionInteraction { + execution_failure: None, at_unix: 10, command: format!("kubectl --token={} get pods", fixture_bearer_jwt()), allowed: false, @@ -3031,6 +3042,7 @@ mod tests { vec![StoredSessionInteraction::from_typed_parts( "tok".to_string(), SessionInteraction { + execution_failure: None, at_unix: now_unix(), command: format!("curl -H 'Authorization: Bearer {}'", fixture_bearer_jwt()), allowed: true, diff --git a/src/session_store.rs b/src/session_store.rs index 15517b36..e90c4d84 100644 --- a/src/session_store.rs +++ b/src/session_store.rs @@ -48,7 +48,8 @@ use std::os::unix::fs::{DirBuilderExt, MetadataExt, OpenOptionsExt, PermissionsE /// canonicalizes the full generated-access proposal envelope. Version 14 adds /// the inert pre-handoff provisional state and classifies ambiguous v13 API /// dispatch rows before older binaries can interpret their rollback authority. -const SCHEMA_VERSION: i64 = 14; +/// Version 15 retains typed execution failures alongside session admission. +const SCHEMA_VERSION: i64 = 15; const VACUUM_MIN_PAGES: u64 = 512; const VACUUM_MIN_FREE_PAGES: u64 = 128; const REGISTRY_GENERATION_KEY: &str = "registry_generation"; @@ -895,7 +896,7 @@ impl SessionStore { let mut interactions = Vec::new(); { let mut stmt = tx.prepare( - "SELECT token, at_unix, command, allowed, source, reason, risk, exec_status, exit_code, secret_refs_json, decision_trace_json + "SELECT token, at_unix, command, allowed, source, reason, risk, exec_status, exit_code, secret_refs_json, decision_trace_json, execution_failure_json FROM session_interactions ORDER BY at_unix ASC, id ASC", )?; @@ -908,6 +909,20 @@ impl SessionStore { Ok(StoredSessionInteraction::from_typed_parts( token, SessionInteraction { + execution_failure: row + .get::<_, Option>(11)? + .map(|json| { + serde_json::from_str::(&json) + .map(guard::wire::ExecutionFailure::sanitized) + }) + .transpose() + .map_err(|error| { + rusqlite::Error::FromSqlConversionFailure( + 11, + rusqlite::types::Type::Text, + Box::new(error), + ) + })?, at_unix: decode_u64(row.get(1)?)?, command: row.get(2)?, allowed: row.get::<_, i64>(3)? != 0, @@ -1149,6 +1164,9 @@ impl SessionStore { { interaction.command = redact_output_text(&interaction.command); interaction.reason = guard::gating::sanitize_gate_text(&interaction.reason); + interaction.execution_failure = interaction + .execution_failure + .map(guard::wire::ExecutionFailure::sanitized); if let Some(trace) = interaction.decision_trace.as_mut() { trace.sanitize_explanatory_text(); } @@ -1157,8 +1175,8 @@ impl SessionStore { } tx.execute( "INSERT INTO session_interactions - (token, at_unix, command, allowed, source, reason, risk, exec_status, exit_code, secret_refs_json, decision_trace_json) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)", + (token, at_unix, command, allowed, source, reason, risk, exec_status, exit_code, secret_refs_json, decision_trace_json, execution_failure_json) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)", params![ token, encode_u64(interaction.at_unix)?, @@ -1174,7 +1192,8 @@ impl SessionStore { .decision_trace .as_ref() .map(serde_json::to_string) - .transpose()? + .transpose()?, + interaction.execution_failure.as_ref().map(serde_json::to_string).transpose()? ], )?; } @@ -1468,6 +1487,12 @@ impl SessionStore { "TEXT NOT NULL DEFAULT '[]'", )?; ensure_column(&tx, "session_interactions", "decision_trace_json", "TEXT")?; + ensure_column( + &tx, + "session_interactions", + "execution_failure_json", + "TEXT", + )?; // Schema v7: bind sessions to their creating principal. Rows migrated // from v6 default to the `Unowned` sentinel and are refused for // execution until reissued. @@ -2991,6 +3016,7 @@ fn valid_approval_transition(previous: &Approval, next: &Approval) -> Result Result<()> { repair_sensitive_session_exact_authority(conn)?; { let mut stmt = conn.prepare( - "SELECT rowid, command, reason, secret_refs_json, decision_trace_json FROM session_interactions", + "SELECT rowid, command, reason, secret_refs_json, decision_trace_json, execution_failure_json FROM session_interactions", )?; let rows = stmt .query_map([], |row| { @@ -3525,29 +3551,39 @@ fn sanitize_persisted_credentials(conn: &Connection) -> Result<()> { row.get::<_, String>(2)?, row.get::<_, String>(3)?, row.get::<_, Option>(4)?, + row.get::<_, Option>(5)?, )) })? .collect::>>()?; - for (rowid, command, reason, secret_refs_json, trace_json) in rows { + for (rowid, command, reason, secret_refs_json, trace_json, failure_json) in rows { let sanitized_command = redact_output_text(&command); let sanitized_reason = redact_output_text(&reason); let sanitized_secret_refs = sanitize_string_vec_json(&secret_refs_json); let sanitized_trace = trace_json.as_deref().and_then(sanitize_decision_trace_json); + let sanitized_failure = failure_json + .as_deref() + .map(|json| { + let failure: guard::wire::ExecutionFailure = serde_json::from_str(json)?; + serde_json::to_string(&failure.sanitized()) + }) + .transpose()?; if sanitized_command != command || sanitized_reason != reason || sanitized_secret_refs != secret_refs_json || sanitized_trace != trace_json + || sanitized_failure != failure_json { conn.execute( "UPDATE session_interactions SET command = ?1, reason = ?2, secret_refs_json = ?3, - decision_trace_json = ?4 - WHERE rowid = ?5", + decision_trace_json = ?4, execution_failure_json = ?5 + WHERE rowid = ?6", params![ sanitized_command, sanitized_reason, sanitized_secret_refs, sanitized_trace, + sanitized_failure, rowid ], )?; @@ -4473,8 +4509,79 @@ mod tests { #[cfg(unix)] use std::os::unix::fs::{symlink, MetadataExt, PermissionsExt}; + #[tokio::test] + async fn execution_failure_survives_session_and_approval_restart() { + use guard::wire::{ExecutionFailure, ExecutionStage}; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("state.db"); + let store = SessionStore::open(path.clone(), 3600).await.unwrap(); + let failure = ExecutionFailure { + started: false, + stage: ExecutionStage::Cwd, + errno: Some(13), + message: "permission denied".into(), + }; + let registry = SessionRegistry::from_typed_parts( + HashMap::new(), + Vec::new(), + vec![StoredSessionInteraction::from_typed_parts( + "fixture-session".into(), + SessionInteraction { + execution_failure: Some(failure.clone()), + at_unix: guard::env::now_unix(), + command: "true".into(), + allowed: true, + source: SessionDecisionSource::StaticPolicy, + reason: "policy permits command".into(), + risk: None, + exec_status: SessionExecStatus::Failed, + exit_code: None, + exposed_secret_refs: Vec::new(), + decision_trace: None, + }, + Vec::new(), + )], + 3600, + ); + store.persist_registry(®istry).await.unwrap(); + let pending = pending_approval("ap-failure"); + store.save_approval(pending.clone()).await.unwrap(); + let mut approval = pending.clone(); + approval.status = ApprovalStatus::ExecFailed; + approval.decided_unix = Some(guard::env::now_unix()); + approval.decided_reason = Some(failure.message.clone()); + approval.execution_failure = Some(failure.clone()); + approval.decision_trace = Some(guard::gating::DecisionTrace::source("static_policy")); + store + .compare_and_swap_approval(pending, approval) + .await + .unwrap(); + drop(store); + let reopened = SessionStore::open(path, 3600).await.unwrap(); + let registry = reopened.load_registry().await.unwrap(); + let interactions = registry.typed_interactions_snapshot(); + assert_eq!(interactions.len(), 1); + assert_eq!(interactions[0].1.execution_failure.as_ref(), Some(&failure)); + assert!(interactions[0].1.allowed); + assert_eq!( + interactions[0].1.source, + SessionDecisionSource::StaticPolicy + ); + let approvals = reopened.load_approvals().await.unwrap(); + assert_eq!(approvals[0].execution_failure.as_ref(), Some(&failure)); + assert_eq!( + approvals[0] + .decision_trace + .as_ref() + .unwrap() + .decision_source, + "static_policy" + ); + } + fn pending_approval(handle: &str) -> Approval { Approval { + execution_failure: None, handle: handle.to_string(), snapshot: guard::gating::approval::ApprovalSnapshot { binary: "fixture-command".to_string(), @@ -7024,6 +7131,7 @@ mod tests { interaction.record_interaction( &token, SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "host-inspect".to_string(), allowed: true, @@ -7368,6 +7476,7 @@ mod tests { vec![StoredSessionInteraction::from_typed_parts( "expired-token".into(), SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix().saturating_sub(60), command: "true".into(), allowed: true, @@ -7400,6 +7509,7 @@ mod tests { let first = SessionStore::open(path.clone(), 3600).await.unwrap(); let second = SessionStore::open(path, 3600).await.unwrap(); let pending = Approval { + execution_failure: None, handle: "ap-shared-claim".to_string(), snapshot: guard::gating::approval::ApprovalSnapshot { binary: "fixture-command".to_string(), @@ -8372,6 +8482,7 @@ mod tests { vec![StoredSessionInteraction::from_typed_parts( "tok".into(), SessionInteraction { + execution_failure: None, at_unix: now.saturating_sub(1), command: "echo hi".into(), allowed: true, @@ -8469,6 +8580,7 @@ mod tests { registry.record_interaction( "safe", SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "fixturectl status".to_string(), allowed: true, @@ -8542,6 +8654,7 @@ mod tests { registry.record_interaction( "safe", SessionInteraction { + execution_failure: None, at_unix: guard::env::now_unix(), command: "fixturectl status".to_string(), allowed: true, diff --git a/src/wire/mod.rs b/src/wire/mod.rs index 841c4b68..94342bfb 100644 --- a/src/wire/mod.rs +++ b/src/wire/mod.rs @@ -10,6 +10,52 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::path::PathBuf; +/// The observed failing operation during command setup or execution. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ExecutionStage { + Identity, + Capabilities, + Cwd, + Exec, + #[serde(other)] + Unknown, +} + +impl ExecutionStage { + pub fn as_str(self) -> &'static str { + match self { + Self::Identity => "identity", + Self::Capabilities => "capabilities", + Self::Cwd => "cwd", + Self::Exec => "exec", + Self::Unknown => "unknown", + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct ExecutionFailure { + pub started: bool, + pub stage: ExecutionStage, + pub errno: Option, + pub message: String, +} + +impl ExecutionFailure { + pub fn sanitized(mut self) -> Self { + self.message = crate::gating::sanitize_gate_text(&self.message); + self + } +} + +/// Admission remains independent of whether the approved command ran. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PolicyDecision { + pub allowed: bool, + pub reason: String, +} + /// How ssh should treat the remote host key for a guarded ssh command. /// Default (`OnlyExisting`) preserves ssh's own strict behavior: the daemon /// injects nothing, so a first-contact host still fails closed. The relaxed diff --git a/tests/cli_output.rs b/tests/cli_output.rs index 9b2871cf..6a6f11d1 100644 --- a/tests/cli_output.rs +++ b/tests/cli_output.rs @@ -64,3 +64,81 @@ fn missing_subcommand_remains_invalid_usage() { let usage = format!("Usage: {executable} verb"); assert!(String::from_utf8_lossy(&output.stderr).contains(&usage)); } + +#[cfg(unix)] +#[tokio::test] +async fn execution_failure_cli_distinguishes_policy_in_text_and_json() { + use serde_json::json; + use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; + for json_output in [false, true] { + for execution_failed in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let socket = directory.path().join("guard.sock"); + let listener = tokio::net::UnixListener::bind(&socket).unwrap(); + let mut response = json!({ + "allowed": false, "reason": "fixture failure", "decision_source": "static_policy", + "policy": {"allowed": execution_failed, "reason": "fixture admission"} + }); + if execution_failed { + response["execution_failure"] = json!({"started": false, "stage": "cwd", "errno": 13, "message": "working directory permission denied"}); + } + let server = async { + let (stream, _) = listener.accept().await.unwrap(); + let (reader, mut writer) = stream.into_split(); + let mut line = String::new(); + BufReader::new(reader).read_line(&mut line).await.unwrap(); + let request: serde_json::Value = serde_json::from_str(&line).unwrap(); + let payload = if request["execute"]["stream"] == true { + json!({"type": "result", "response": response}) + } else { + response + }; + writer + .write_all(format!("{payload}\n").as_bytes()) + .await + .unwrap(); + }; + let client = async { + let mut command = tokio::process::Command::new(GUARD_BIN); + command + .env_clear() + .env("XDG_CONFIG_HOME", directory.path()) + .current_dir(directory.path()) + .kill_on_drop(true) + .args(["run", "--socket"]) + .arg(&socket); + if json_output { + command.arg("--json"); + } + command.arg("true").output().await.unwrap() + }; + let (_, output) = tokio::time::timeout(std::time::Duration::from_secs(10), async { + tokio::join!(server, client) + }) + .await + .unwrap(); + assert_eq!( + output.status.code(), + Some(if execution_failed { 125 } else { 126 }) + ); + if json_output { + let document: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(document["response"]["policy"]["allowed"], execution_failed); + assert_eq!(document["response"]["allowed"], false); + if execution_failed { + assert_eq!(document["response"]["execution_failure"]["stage"], "cwd"); + } + } else { + let stderr = String::from_utf8_lossy(&output.stderr); + if execution_failed { + assert!(stderr.contains("EXECUTION FAILED")); + assert!(!stderr.contains("DENIED")); + assert!(!stderr.contains("appeal:")); + } else { + assert!(stderr.contains("DENIED")); + assert!(stderr.contains("appeal:")); + } + } + } + } +} From 6533be54381262b87db6a42226750e58f96ac4db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Wed, 9 Sep 2026 23:19:47 +0000 Subject: [PATCH 2/8] fix: report child setup failures under execution authority --- Cargo.lock | 2 +- Cargo.toml | 2 +- fuzz/Cargo.lock | 2 +- src/server/execute.rs | 789 +++++++++++++++++++++++++------- src/server/tests/exec_policy.rs | 220 +++++++++ src/server/tests/gating.rs | 4 +- 6 files changed, 840 insertions(+), 179 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51edbdea..02e1b110 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -651,7 +651,7 @@ dependencies = [ [[package]] name = "guard" -version = "0.8.5" +version = "0.8.6" dependencies = [ "anyhow", "async-trait", diff --git a/Cargo.toml b/Cargo.toml index d418b472..b3a6675b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "guard" -version = "0.8.5" +version = "0.8.6" edition = "2021" rust-version = "1.95" description = "LLM-evaluated command gate for AI agents" diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 9f4f3da0..83f06602 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -630,7 +630,7 @@ dependencies = [ [[package]] name = "guard" -version = "0.8.0" +version = "0.8.6" dependencies = [ "anyhow", "async-trait", diff --git a/src/server/execute.rs b/src/server/execute.rs index aa661861..8112d79a 100644 --- a/src/server/execute.rs +++ b/src/server/execute.rs @@ -17,11 +17,18 @@ use guard::redact::{ command_contains_exact_secrets, command_line, redact_command_line, redact_exact_secrets, redact_output_text, redact_output_with_state, ExactSecretStreamRedactor, RedactionState, }; +use guard::wire::ExecutionStage; use sha2::{Digest, Sha256}; use std::collections::{BTreeMap, BTreeSet, HashMap}; #[cfg(unix)] use std::ffi::CString; #[cfg(unix)] +use std::io::{PipeReader, Read}; +#[cfg(unix)] +use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; +#[cfg(unix)] +use std::os::unix::ffi::OsStrExt; +#[cfg(unix)] use std::os::unix::fs::PermissionsExt; #[cfg(unix)] use std::os::unix::process::CommandExt; @@ -30,7 +37,7 @@ use std::process::Stdio; use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; use std::sync::Arc; use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite}; -use tokio::process::Command; +use tokio::process::{Child, Command}; use tokio::sync::mpsc; #[cfg(unix)] use uzers::os::unix::UserExt; @@ -483,33 +490,34 @@ async fn canonicalize_request_cwd( Ok(()) } -async fn revalidate_exec_cwd(cwd: &Path) -> std::result::Result<(), String> { - let canonical = tokio::fs::canonicalize(cwd).await.map_err(|e| { - format!( - "working directory '{}' changed before exec: cannot canonicalize: {}", - cwd.display(), - e +async fn revalidate_exec_cwd(cwd: &Path) -> std::result::Result<(), LaunchError> { + let canonical = tokio::fs::canonicalize(cwd).await.map_err(|error| { + LaunchError::from_io( + ExecutionStage::Cwd, + "working directory changed before exec: cannot canonicalize", + &error, ) })?; if canonical != cwd { - return Err(format!( - "working directory '{}' changed before exec: canonical path is now '{}'", - cwd.display(), - canonical.display() - )); + return Err(LaunchError { + stage: ExecutionStage::Cwd, + errno: None, + message: "working directory changed before exec: canonical path differs", + }); } - let meta = tokio::fs::metadata(&canonical).await.map_err(|e| { - format!( - "working directory '{}' changed before exec: cannot stat: {}", - canonical.display(), - e + let meta = tokio::fs::metadata(&canonical).await.map_err(|error| { + LaunchError::from_io( + ExecutionStage::Cwd, + "working directory changed before exec: cannot inspect directory", + &error, ) })?; if !meta.is_dir() { - return Err(format!( - "working directory '{}' changed before exec: not a directory", - canonical.display() - )); + return Err(LaunchError { + stage: ExecutionStage::Cwd, + errno: None, + message: "working directory changed before exec: not a directory", + }); } Ok(()) } @@ -908,7 +916,7 @@ async fn route_allow_and_record( phase.server, phase.session_token.as_deref(), SessionInteraction { - execution_failure: None, + execution_failure: result.execution_failure().cloned(), at_unix: 0, command: interaction_command, allowed: true, @@ -1065,11 +1073,6 @@ struct CommandInitiationLease { _session: Option>, } -struct ProcessInitiationLeases { - command: CommandInitiationLease, - tool_mapping: ToolMappingSpawnLease, -} - #[cfg(all(test, unix))] type CommandInitiationHook = ( std::sync::Arc, @@ -2775,12 +2778,18 @@ pub(super) fn resolve_exec_caller_context(uid: u32) -> Result }) } +#[derive(Clone)] +struct PreparedExecIdentity { + context: ExecCallerContext, + #[cfg(unix)] + groups: Vec, +} + #[cfg(unix)] -fn apply_exec_identity( - cmd: &mut Command, +fn resolve_exec_identity( server: &ServerContext, caller: &CallerIdentity, -) -> Result> { +) -> Result> { if !server.config.exec_as_caller { return Ok(None); } @@ -2792,59 +2801,45 @@ fn apply_exec_identity( let context = resolve_exec_caller_context(caller_uid)?; let username = CString::new(context.username.clone()) .context("caller username contains an interior NUL byte")?; - let gid = context.gid; - - cmd.gid(gid); - cmd.uid(context.uid); - unsafe { - cmd.pre_exec(move || { - if libc::initgroups(username.as_ptr(), gid as _) != 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) - }); + let mut groups = vec![0; 16]; + // NSS lookup and allocation happen in the parent, before the fork. + for _ in 0..3 { + let mut count = groups.len() as libc::c_int; + let result = unsafe { + libc::getgrouplist( + username.as_ptr(), + context.gid as _, + groups.as_mut_ptr().cast(), + &mut count, + ) + }; + let count = usize::try_from(count).context("invalid supplementary group count")?; + if result >= 0 && count > 0 && count <= groups.len() { + groups.truncate(count); + return Ok(Some(PreparedExecIdentity { context, groups })); + } + // Linux permits at most 65536 supplementary groups. This also bounds + // allocation when a group database returns an invalid required size. + if count <= groups.len() || count > 65536 { + bail!("cannot resolve supplementary groups for the execution identity"); + } + groups.resize(count, 0); } - - Ok(Some(context)) + bail!("supplementary group membership changed repeatedly during lookup") } #[cfg(not(unix))] -fn apply_exec_identity( - _cmd: &mut Command, +fn resolve_exec_identity( server: &ServerContext, _caller: &CallerIdentity, -) -> Result> { +) -> Result> { if server.config.exec_as_caller { bail!("--exec-as-caller is not supported on this platform"); } Ok(None) } -/// Strip inherited capabilities from a brokered child before `execve`. -/// -/// Under the packaged unit the daemon holds `CAP_FOWNER` and -/// `CAP_DAC_READ_SEARCH` in its ambient set so its own read-grant `setfacl`/ -/// `getfacl` calls can manipulate ACLs on files it does not own. Ambient -/// capabilities are, by design, preserved across `execve()` for a non-privileged -/// process, so without this every caller-requested command (a plain -/// `cat /etc/shadow`, an `ansible-playbook` reading arbitrary files) would -/// inherit those capabilities and bypass file DAC entirely -- `CAP_DAC_READ_SEARCH` -/// bypasses file read permission checks and `CAP_FOWNER` bypasses the file-owner -/// checks `chmod`/`setfacl` enforce -- defeating the scoped, policy-gated read -/// grants. This clears the ambient set (so nothing survives `execve`) and zeroes -/// the inheritable set (so a target binary carrying its own file-inheritable caps -/// cannot pick anything up via the `P(inh) & F(inh)` intersection). -/// -/// Applies only inside the forked child via `pre_exec`; the long-lived daemon -/// keeps its capabilities for its own direct `setfacl`/`getfacl` `Command`s, -/// which are separate and never pass through here. Clearing capabilities needs -/// no privilege (only raising them does), so it is safe under both the default -/// service-identity model and `--exec-as-caller`. -/// -/// The capget/capset structs and version magic are declared here because the -/// `libc` crate does not expose `capget`/`capset` or the `cap_user_*` types; the -/// calls go through `libc::syscall` with the stable `SYS_capget`/`SYS_capset` -/// numbers. +// Linux capget/capset structures are not exposed by the libc crate. #[cfg(all(unix, target_os = "linux"))] #[repr(C)] struct CapUserHeader { @@ -2866,63 +2861,305 @@ struct CapUserData { const LINUX_CAPABILITY_VERSION_3: u32 = 0x2008_0522; #[cfg(unix)] -fn drop_brokered_child_capabilities(cmd: &mut Command) { - // SAFETY: the closure runs in the forked child after `fork()` and before - // `execve`. It calls only async-signal-safe raw syscalls (prctl/capget/ - // capset) and performs no allocation. +fn drop_brokered_child_capabilities() -> std::io::Result<()> { + // SAFETY: raw capability syscalls operate on stack data only. The caller + // invokes this after installing the child identity and before chdir. + #[cfg(target_os = "linux")] unsafe { - cmd.pre_exec(|| { - #[cfg(target_os = "linux")] + if libc::prctl( + libc::PR_CAP_AMBIENT, + libc::PR_CAP_AMBIENT_CLEAR_ALL as libc::c_ulong, + 0 as libc::c_ulong, + 0 as libc::c_ulong, + 0 as libc::c_ulong, + ) != 0 + { + return Err(std::io::Error::last_os_error()); + } + let mut header = CapUserHeader { + version: LINUX_CAPABILITY_VERSION_3, + pid: 0, + }; + let mut data = [CapUserData { + effective: 0, + permitted: 0, + inheritable: 0, + }; 2]; + // An explicitly root execution identity retains its root + // authority. Non-root children lose every capability before + // directory traversal, including the daemon's DAC privileges. + if libc::geteuid() == 0 { + if libc::syscall( + libc::SYS_capget, + &mut header as *mut CapUserHeader, + data.as_mut_ptr(), + ) != 0 { - // 1. Clear the ambient set: these are the capabilities that would - // otherwise be preserved across `execve` for a non-privileged - // process. - if libc::prctl( - libc::PR_CAP_AMBIENT, - libc::PR_CAP_AMBIENT_CLEAR_ALL as libc::c_ulong, - 0 as libc::c_ulong, - 0 as libc::c_ulong, - 0 as libc::c_ulong, - ) != 0 - { - return Err(std::io::Error::last_os_error()); + return Err(std::io::Error::last_os_error()); + } + data[0].inheritable = 0; + data[1].inheritable = 0; + } + if libc::syscall( + libc::SYS_capset, + &header as *const CapUserHeader, + data.as_ptr(), + ) != 0 + { + return Err(std::io::Error::last_os_error()); + } + } + Ok(()) +} + +#[derive(Debug)] +struct LaunchError { + stage: ExecutionStage, + errno: Option, + message: &'static str, +} + +impl LaunchError { + fn from_io(stage: ExecutionStage, message: &'static str, error: &std::io::Error) -> Self { + Self { + stage, + errno: error.raw_os_error(), + message, + } + } + + fn into_result(self, policy_reason: String) -> ExecuteResult { + let message = match self.errno { + Some(errno) => format!( + "{}: {}", + self.message, + std::io::Error::from_raw_os_error(errno) + ), + None => self.message.to_string(), + }; + ExecuteResult::launch_failed(policy_reason, self.stage, self.errno, message) + } +} + +#[cfg(unix)] +#[derive(Clone, Copy)] +#[repr(u8)] +enum ChildSetupStage { + SupplementaryGroups = 1, + GroupIdentity = 2, + UserIdentity = 3, + Capabilities = 4, + Cwd = 5, + Ready = 6, +} + +#[cfg(unix)] +fn write_child_setup_report( + writer: &OwnedFd, + stage: ChildSetupStage, + errno: i32, +) -> std::io::Result<()> { + let mut record = [0u8; 8]; + record[0] = stage as u8; + record[4..].copy_from_slice(&errno.to_ne_bytes()); + loop { + // The empty private pipe receives exactly one record, below PIPE_BUF. + let written = + unsafe { libc::write(writer.as_raw_fd(), record.as_ptr().cast(), record.len()) }; + if written == record.len() as isize { + return Ok(()); + } + if written < 0 { + let error = std::io::Error::last_os_error(); + if error.kind() == std::io::ErrorKind::Interrupted { + continue; + } + return Err(error); + } + return Err(std::io::Error::from_raw_os_error(libc::EIO)); + } +} + +#[cfg(unix)] +fn report_child_setup_failure( + writer: &OwnedFd, + stage: ChildSetupStage, + error: std::io::Error, +) -> std::io::Error { + let _ = write_child_setup_report(writer, stage, error.raw_os_error().unwrap_or(libc::EIO)); + error +} + +#[cfg(unix)] +fn prepare_child_setup( + cmd: &mut Command, + cwd: Option<&Path>, + identity: Option<&PreparedExecIdentity>, +) -> std::result::Result { + let cwd = cwd + .map(|path| CString::new(path.as_os_str().as_bytes())) + .transpose() + .map_err(|_| LaunchError { + stage: ExecutionStage::Cwd, + errno: Some(libc::EINVAL), + message: "working directory contains an invalid path byte", + })?; + let identity = identity.cloned(); + // std creates both ends close-on-exec. A nonblocking reader also avoids + // waiting for unrelated forked children that briefly inherit a pipe end. + let (reader, writer) = std::io::pipe().map_err(|error| { + LaunchError::from_io( + ExecutionStage::Unknown, + "cannot create child setup report channel", + &error, + ) + })?; + let mut writer: OwnedFd = writer.into(); + if writer.as_raw_fd() <= libc::STDERR_FILENO { + let descriptor = unsafe { libc::fcntl(writer.as_raw_fd(), libc::F_DUPFD_CLOEXEC, 3) }; + if descriptor < 0 { + return Err(LaunchError::from_io( + ExecutionStage::Unknown, + "cannot protect child setup report channel from stdio setup", + &std::io::Error::last_os_error(), + )); + } + // SAFETY: fcntl returns a fresh descriptor owned by this launch. + writer = unsafe { OwnedFd::from_raw_fd(descriptor) }; + } + let flags = unsafe { libc::fcntl(reader.as_raw_fd(), libc::F_GETFL) }; + if flags < 0 + || unsafe { libc::fcntl(reader.as_raw_fd(), libc::F_SETFL, flags | libc::O_NONBLOCK) } < 0 + { + return Err(LaunchError::from_io( + ExecutionStage::Unknown, + "cannot configure child setup report channel", + &std::io::Error::last_os_error(), + )); + } + // SAFETY: this hook only reads parent-prepared buffers, makes raw identity, + // capability, chdir and write syscalls, and constructs errno-only errors. + // It performs no allocation, NSS lookup, logging, or locking after fork. + unsafe { + cmd.pre_exec(move || { + if let Some(identity) = &identity { + if libc::setgroups(identity.groups.len() as _, identity.groups.as_ptr()) != 0 { + return Err(report_child_setup_failure( + &writer, + ChildSetupStage::SupplementaryGroups, + std::io::Error::last_os_error(), + )); } - // 2. Zero the inheritable set. Reading the current sets first and - // only clearing `inheritable` leaves `permitted`/`effective` - // untouched (they collapse to the ambient set at `execve` - // anyway for a non-privileged target). Dropping bits is always - // permitted; only raising them requires CAP_SETPCAP. - let mut header = CapUserHeader { - version: LINUX_CAPABILITY_VERSION_3, - pid: 0, - }; - let mut data = [CapUserData { - effective: 0, - permitted: 0, - inheritable: 0, - }; 2]; - if libc::syscall( - libc::SYS_capget, - &mut header as *mut CapUserHeader, - data.as_mut_ptr(), - ) != 0 - { - return Err(std::io::Error::last_os_error()); + if libc::setgid(identity.context.gid as _) != 0 { + return Err(report_child_setup_failure( + &writer, + ChildSetupStage::GroupIdentity, + std::io::Error::last_os_error(), + )); } - data[0].inheritable = 0; - data[1].inheritable = 0; - if libc::syscall( - libc::SYS_capset, - &header as *const CapUserHeader, - data.as_ptr(), - ) != 0 - { - return Err(std::io::Error::last_os_error()); + if libc::setuid(identity.context.uid as _) != 0 { + return Err(report_child_setup_failure( + &writer, + ChildSetupStage::UserIdentity, + std::io::Error::last_os_error(), + )); } } - Ok(()) + drop_brokered_child_capabilities().map_err(|error| { + report_child_setup_failure(&writer, ChildSetupStage::Capabilities, error) + })?; + if let Some(cwd) = &cwd { + if libc::chdir(cwd.as_ptr()) != 0 { + return Err(report_child_setup_failure( + &writer, + ChildSetupStage::Cwd, + std::io::Error::last_os_error(), + )); + } + } + write_child_setup_report(&writer, ChildSetupStage::Ready, 0) }); } + Ok(reader) +} + +#[cfg(unix)] +fn read_child_setup_error( + reader: &mut PipeReader, + spawn_error: &std::io::Error, +) -> Option { + let mut record = [0u8; 8]; + loop { + match reader.read(&mut record) { + Ok(8) => break, + Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, + _ => return None, + } + } + let errno = i32::from_ne_bytes(record[4..].try_into().ok()?); + if record[1..4] != [0; 3] { + return None; + } + let (stage, message) = match record[0] { + 1 => ( + ExecutionStage::Identity, + "cannot install the child supplementary groups", + ), + 2 => ( + ExecutionStage::Identity, + "cannot install the child group identity", + ), + 3 => ( + ExecutionStage::Identity, + "cannot install the child user identity", + ), + 4 => ( + ExecutionStage::Capabilities, + "cannot remove the child inherited capabilities", + ), + 5 => ( + ExecutionStage::Cwd, + "the child execution identity cannot enter the requested working directory", + ), + 6 if errno == 0 => { + return Some(LaunchError::from_io( + ExecutionStage::Exec, + "cannot launch the executable after child setup", + spawn_error, + )) + } + _ => return None, + }; + (errno > 0 && spawn_error.raw_os_error() == Some(errno)).then_some(LaunchError { + stage, + errno: Some(errno), + message, + }) +} + +fn spawn_brokered_command( + mut cmd: Command, + cwd: Option<&Path>, + identity: Option<&PreparedExecIdentity>, +) -> std::result::Result { + #[cfg(unix)] + let mut report = prepare_child_setup(&mut cmd, cwd, identity)?; + #[cfg(not(unix))] + let _ = (cwd, identity); + let spawned = cmd.spawn(); + // Release the parent's copy of the report writer before inspecting it. + drop(cmd); + spawned.map_err(|error| { + #[cfg(unix)] + if let Some(reported) = read_child_setup_error(&mut report, &error) { + return reported; + } + LaunchError::from_io( + ExecutionStage::Unknown, + "process launch failed without a child setup report", + &error, + ) + }) } #[cfg(unix)] @@ -3526,15 +3763,25 @@ pub(super) async fn exec_after_approval_with_command_authority binary, - Err(e) => return ExecuteResult::exec_failed(allow_reason, e.to_string()), + Err(error) => { + return ExecuteResult::launch_failed( + allow_reason, + ExecutionStage::Exec, + error + .downcast_ref::() + .and_then(std::io::Error::raw_os_error), + "the executable cannot be resolved on the configured command path", + ); + } }; let mut cmd = Command::new(&exec_binary); cmd.args(&request.args); cmd.stdin(Stdio::null()); if let Some(cwd) = &request.cwd { - if let Err(reason) = revalidate_exec_cwd(cwd).await { - return ExecuteResult::exec_failed(allow_reason, reason); + if let Err(error) = revalidate_exec_cwd(cwd).await { + return error.into_result(allow_reason); } + #[cfg(not(unix))] cmd.current_dir(cwd); } @@ -3583,19 +3830,20 @@ pub(super) async fn exec_after_approval_with_command_authority context, - Err(e) => { - return ExecuteResult::exec_failed(allow_reason, format!("exec identity error: {}", e)); + Err(error) => { + return ExecuteResult::launch_failed( + allow_reason, + ExecutionStage::Identity, + error + .downcast_ref::() + .and_then(std::io::Error::raw_os_error), + "the child execution identity cannot be prepared", + ); } }; - // Drop the daemon's read-grant capabilities (CAP_FOWNER / CAP_DAC_READ_SEARCH) - // from the brokered child so they never survive execve into a caller-requested - // command. Applies to both the default and --exec-as-caller models. - #[cfg(unix)] - drop_brokered_child_capabilities(&mut cmd); - for (key, value) in &trusted_tool_env { cmd.env(key, value); } @@ -3603,7 +3851,8 @@ pub(super) async fn exec_after_approval_with_command_authority child, + Err(error) => return error.into_result(allow_reason), + }; + drop(tool_mapping_lease); + drop(initiation_lease); + #[cfg(all(test, unix))] + signal_command_started_for_test(server); + if context.stream_output { - let result = execute_spawn_streaming( - cmd, + let result = execute_streaming_child( + child, allow_reason, exec_timeout_secs, server, @@ -3685,31 +3946,12 @@ pub(super) async fn exec_after_approval_with_command_authority child, - Err(e) => { - return ExecuteResult::exec_failed( - allow_reason, - format!("failed to execute '{}': {}", request.binary, e), - ); - } - }; - drop(tool_mapping_lease); - drop(initiation_lease); - #[cfg(all(test, unix))] - signal_command_started_for_test(server); let mut process_guard = child .id() .map(|pid| server.state.process_tracker.track(pid)); @@ -4156,33 +4398,15 @@ struct OutputRedactionContext<'a> { exact_secrets: &'a [String], } -#[allow(clippy::too_many_arguments)] -async fn execute_spawn_streaming( - mut cmd: Command, +async fn execute_streaming_child( + mut child: Child, allow_reason: String, exec_timeout_secs: u64, server: &ServerContext, redaction: OutputRedactionContext<'_>, audit: SpawnAuditContext<'_>, writer: &mut W, - leases: ProcessInitiationLeases, ) -> ExecuteResult { - cmd.stdout(Stdio::piped()); - cmd.stderr(Stdio::piped()); - - let mut child = match cmd.spawn() { - Ok(child) => child, - Err(e) => { - return ExecuteResult::exec_failed( - allow_reason, - format!("failed to execute '{}': {}", audit.request.binary, e), - ); - } - }; - drop(leases.tool_mapping); - drop(leases.command); - #[cfg(all(test, unix))] - signal_command_started_for_test(server); let mut process_guard = child .id() .map(|pid| server.state.process_tracker.track(pid)); @@ -4707,6 +4931,223 @@ pub(super) fn merge_envelope_context( } } +#[cfg(all(test, unix))] +mod child_launch_tests { + use super::*; + use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; + + fn shell(command: &str) -> Command { + let mut cmd = Command::new("sh"); + cmd.args(["-c", command]); + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + cmd + } + + #[tokio::test] + async fn child_launch_distinguishes_cwd_and_executable_failures() { + if unsafe { libc::geteuid() } == 0 { + eprintln!("unprivileged cwd permission test requires a non-root test identity"); + return; + } + let temp = tempfile::tempdir().unwrap(); + let cwd = temp.path().canonicalize().unwrap(); + let denied = cwd.join("denied-directory"); + std::fs::DirBuilder::new().mode(0).create(&denied).unwrap(); + assert!(denied.canonicalize().unwrap().metadata().unwrap().is_dir()); + let error = + spawn_brokered_command(shell("printf started"), Some(&denied), None).unwrap_err(); + std::fs::remove_dir(&denied).unwrap(); + assert_eq!(error.stage, ExecutionStage::Cwd); + assert_eq!(error.errno, Some(libc::EACCES)); + + let ancestor = cwd.join("ancestor"); + let nested = ancestor.join("nested"); + std::fs::create_dir_all(&nested).unwrap(); + std::fs::set_permissions(&ancestor, std::fs::Permissions::from_mode(0)).unwrap(); + let error = + spawn_brokered_command(shell("printf started"), Some(&nested), None).unwrap_err(); + std::fs::set_permissions(&ancestor, std::fs::Permissions::from_mode(0o700)).unwrap(); + assert_eq!(error.stage, ExecutionStage::Cwd); + assert_eq!(error.errno, Some(libc::EACCES)); + + let missing = cwd.join("missing-executable"); + let error = spawn_brokered_command(Command::new(missing), Some(&cwd), None).unwrap_err(); + assert_eq!(error.stage, ExecutionStage::Exec); + assert_eq!(error.errno, Some(libc::ENOENT)); + + let denied_executable = cwd.join("denied-executable"); + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&denied_executable) + .unwrap(); + let error = + spawn_brokered_command(Command::new(denied_executable), Some(&cwd), None).unwrap_err(); + assert_eq!(error.stage, ExecutionStage::Exec); + assert_eq!(error.errno, Some(libc::EACCES)); + } + + #[tokio::test] + async fn child_launch_resolves_relative_executable_and_file_in_requested_cwd() { + use std::io::Write; + let temp = tempfile::tempdir().unwrap(); + let executable = temp.path().join("relative-tool"); + let mut file = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o700) + .open(executable) + .unwrap(); + file.write_all(b"#!/bin/sh\ncat relative-input\n").unwrap(); + drop(file); + std::fs::write(temp.path().join("relative-input"), "relative-cwd-ok").unwrap(); + let mut cmd = Command::new("./relative-tool"); + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + let child = spawn_brokered_command(cmd, Some(temp.path()), None).unwrap(); + let output = child.wait_with_output().await.unwrap(); + assert!(output.status.success(), "{output:?}"); + assert_eq!(output.stdout, b"relative-cwd-ok"); + } + + #[tokio::test] + async fn child_launch_keeps_unobserved_setup_failures_unknown() { + let mut cmd = shell("printf started"); + cmd.arg("invalid\0argument"); + let error = spawn_brokered_command(cmd, None, None).unwrap_err(); + assert_eq!(error.stage, ExecutionStage::Unknown); + assert_eq!(error.errno, None); + } + + #[tokio::test] + async fn child_launch_default_identity_preserves_supplementary_groups() { + let count = unsafe { libc::getgroups(0, std::ptr::null_mut()) }; + assert!(count >= 0); + let mut groups = vec![0; count as usize]; + assert_eq!( + unsafe { libc::getgroups(count, groups.as_mut_ptr()) }, + count + ); + groups.push(unsafe { libc::getegid() }); + groups.sort_unstable(); + groups.dedup(); + let child = spawn_brokered_command(shell("id -u; id -g; id -G"), None, None).unwrap(); + let output = child.wait_with_output().await.unwrap(); + assert!(output.status.success()); + let output = String::from_utf8(output.stdout).unwrap(); + let mut lines = output.lines(); + assert_eq!(lines.next().unwrap().parse::().unwrap(), unsafe { + libc::geteuid() + }); + assert_eq!(lines.next().unwrap().parse::().unwrap(), unsafe { + libc::getegid() + }); + let mut child_groups: Vec = lines + .next() + .unwrap() + .split_whitespace() + .map(|group| group.parse().unwrap()) + .collect(); + child_groups.sort_unstable(); + child_groups.dedup(); + assert_eq!(child_groups, groups); + } + + #[tokio::test] + async fn child_launch_reports_identity_failure_before_cwd() { + if unsafe { libc::geteuid() } == 0 { + eprintln!("unprivileged identity failure test requires a non-root test identity"); + return; + } + let mut server = super::super::tests::config_for_proposal_test(); + server.config.exec_as_caller = true; + let identity = resolve_exec_identity( + &server, + &CallerIdentity::Unix { + uid: unsafe { libc::geteuid() }, + }, + ) + .unwrap() + .unwrap(); + assert!(identity.groups.contains(&identity.context.gid)); + let missing = tempfile::tempdir().unwrap().path().join("missing"); + let error = + spawn_brokered_command(shell("printf started"), Some(&missing), Some(&identity)) + .unwrap_err(); + assert_eq!(error.stage, ExecutionStage::Identity); + assert_eq!(error.errno, Some(libc::EPERM)); + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn child_launch_non_root_capabilities_are_empty() { + if unsafe { libc::geteuid() } == 0 { + eprintln!("non-root capability test requires a non-root test identity"); + return; + } + let mut cmd = Command::new("cat"); + cmd.arg("/proc/self/status").stdout(Stdio::piped()); + let output = spawn_brokered_command(cmd, None, None) + .unwrap() + .wait_with_output() + .await + .unwrap(); + assert!(output.status.success()); + let status = String::from_utf8(output.stdout).unwrap(); + for field in ["CapInh:", "CapPrm:", "CapEff:", "CapAmb:"] { + let value = status + .lines() + .find_map(|line| line.strip_prefix(field)) + .expect("child capability field"); + assert_eq!(u64::from_str_radix(value.trim(), 16).unwrap(), 0, "{field}"); + } + } + + #[cfg(target_os = "linux")] + #[tokio::test] + async fn child_setup_report_descriptors_are_close_on_exec() { + let mut cmd = + shell("for descriptor in /proc/self/fd/*; do readlink \"$descriptor\"; done; exit 0"); + let report = prepare_child_setup(&mut cmd, None, None).unwrap(); + let report_pipe = + std::fs::read_link(format!("/proc/self/fd/{}", report.as_raw_fd())).unwrap(); + let output = cmd.output().await.unwrap(); + assert!(output.status.success()); + assert!(!String::from_utf8(output.stdout) + .unwrap() + .lines() + .any(|line| line == report_pipe.to_str().unwrap())); + } + + #[test] + fn child_setup_report_requires_a_complete_matching_record() { + use std::io::Write; + for record in [ + vec![5], + vec![99, 0, 0, 0, 13, 0, 0, 0], + vec![5, 1, 0, 0, 13, 0, 0, 0], + vec![5, 0, 0, 0, 0, 0, 0, 0], + ] { + let (mut reader, mut writer) = std::io::pipe().unwrap(); + writer.write_all(&record).unwrap(); + drop(writer); + assert!(read_child_setup_error( + &mut reader, + &std::io::Error::from_raw_os_error(libc::EACCES) + ) + .is_none()); + } + let (mut reader, writer) = std::io::pipe().unwrap(); + let writer: OwnedFd = writer.into(); + write_child_setup_report(&writer, ChildSetupStage::Cwd, libc::EACCES).unwrap(); + assert!(read_child_setup_error( + &mut reader, + &std::io::Error::from_raw_os_error(libc::ENOENT) + ) + .is_none()); + } +} + #[cfg(test)] mod decision_trace_feature_tests { use super::*; diff --git a/src/server/tests/exec_policy.rs b/src/server/tests/exec_policy.rs index fa3ddcc0..eb8ab189 100644 --- a/src/server/tests/exec_policy.rs +++ b/src/server/tests/exec_policy.rs @@ -2648,6 +2648,226 @@ async fn local_caller_cwd_is_canonicalized_and_used_for_execution() { } } +#[cfg(unix)] +#[tokio::test] +async fn launch_failures_are_typed_in_buffered_and_streaming_execution() { + use guard::wire::ExecutionStage; + use std::io::Write; + use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; + + let _env_guard = TEST_ENV_LOCK.lock().await; + let (mut cfg, _) = make_test_config(); + cfg.config.exec_timeout_secs = 5; + let caller = CallerIdentity::Unix { + uid: unsafe { libc::geteuid() }, + }; + let temp = tempfile::tempdir().unwrap(); + let cwd = temp.path().canonicalize().unwrap(); + let denied_cwd = cwd.join("denied-directory"); + std::fs::DirBuilder::new() + .mode(0) + .create(&denied_cwd) + .unwrap(); + let denied_binary = cwd.join("denied-executable"); + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&denied_binary) + .unwrap(); + let missing_binary = cwd.join("missing-executable"); + let missing_interpreter = cwd.join("missing-interpreter"); + let mut script = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o700) + .open(&missing_interpreter) + .unwrap(); + script + .write_all(b"#!/guard-nonexistent-interpreter\n") + .unwrap(); + drop(script); + + for streaming in [false, true] { + let mut cases = vec![ + ( + missing_binary.to_str().unwrap(), + &cwd, + ExecutionStage::Exec, + libc::ENOENT, + ), + ( + denied_binary.to_str().unwrap(), + &cwd, + ExecutionStage::Exec, + libc::EACCES, + ), + ( + missing_interpreter.to_str().unwrap(), + &cwd, + ExecutionStage::Exec, + libc::ENOENT, + ), + ]; + if unsafe { libc::geteuid() } != 0 { + cases.push(("sh", &denied_cwd, ExecutionStage::Cwd, libc::EACCES)); + } + for (binary, requested_cwd, stage, errno) in cases { + let mut request = basic_request( + binary, + vec!["-c".to_string(), "printf unexpected".to_string()], + ); + request.cwd = Some(requested_cwd.clone()); + let mut stream = Vec::new(); + let result = exec_after_approval_with_secret_authority( + &mut RequestContext { + server: &cfg, + caller: &caller, + depth: 0, + stream_output: streaming, + stream_writer: &mut stream, + }, + request, + "fixture policy approval".to_string(), + None, + ) + .await; + assert!(result.policy_allowed()); + assert!(matches!( + result.exec, + ExecOutcome::Failed { started: false, .. } + )); + let failure = result.execution_failure().expect("typed launch failure"); + assert!(!failure.started); + assert_eq!(failure.stage, stage, "{failure:?}"); + assert_eq!(failure.errno, Some(errno)); + assert!(!failure.message.contains(cwd.to_str().unwrap())); + assert!(!failure.message.contains("unexpected")); + assert!(stream.is_empty(), "a failed launch emits no child output"); + } + } + std::fs::remove_dir(denied_cwd).unwrap(); +} + +#[cfg(unix)] +#[tokio::test] +async fn launch_preserves_relative_file_access_in_both_output_modes() { + let _env_guard = TEST_ENV_LOCK.lock().await; + let (mut cfg, _) = make_test_config(); + cfg.config.exec_timeout_secs = 5; + let caller = CallerIdentity::Unix { + uid: unsafe { libc::geteuid() }, + }; + let temp = tempfile::tempdir().unwrap(); + std::fs::write(temp.path().join("relative-input"), "cwd-content").unwrap(); + for streaming in [false, true] { + let mut request = basic_request( + "sh", + vec!["-c".to_string(), "cat relative-input".to_string()], + ); + request.cwd = Some(temp.path().canonicalize().unwrap()); + let mut stream = Vec::new(); + let result = exec_after_approval_with_secret_authority( + &mut RequestContext { + server: &cfg, + caller: &caller, + depth: 0, + stream_output: streaming, + stream_writer: &mut stream, + }, + request, + "fixture policy approval".to_string(), + None, + ) + .await; + assert!(result.policy_allowed()); + assert!(result.execution_failure().is_none()); + match result.exec { + ExecOutcome::Completed { + exit_code, stdout, .. + } => { + assert_eq!(exit_code, Some(0)); + if streaming { + assert!(String::from_utf8(stream).unwrap().contains("cwd-content")); + } else { + assert_eq!(stdout.as_deref(), Some("cwd-content")); + } + } + other => panic!("expected completed relative-file execution: {other:?}"), + } + } +} + +#[cfg(unix)] +#[tokio::test] +async fn launch_failure_is_recorded_as_an_allowed_session_interaction_after_restart() { + use guard::wire::ExecutionStage; + use std::os::unix::fs::DirBuilderExt; + + let _env_guard = TEST_ENV_LOCK.lock().await; + let uid = unsafe { libc::geteuid() }; + if uid == 0 { + eprintln!("cwd permission test requires a non-root test identity"); + return; + } + let (mut cfg, _) = make_test_config(); + let temp = tempfile::tempdir().unwrap(); + let database = temp.path().join("state.db"); + cfg.state.session_store = Some( + crate::session_store::SessionStore::open(database.clone(), 3600) + .await + .unwrap(), + ); + let cwd = temp.path().join("denied-directory"); + std::fs::DirBuilder::new().mode(0).create(&cwd).unwrap(); + let token = format!("launch-failure-{}", std::process::id()); + let mut grant = unrestricted_session(); + grant.owner = crate::session::SessionOwner::Principal(PrincipalKey::from_uid(uid)); + grant.static_only = true; + grant.allow_exact = vec![SessionExactRule::with_cwd("id", Vec::new(), cwd.clone())]; + cfg.state.sessions.write().await.grant(token.clone(), grant); + let mut request = basic_request("id", Vec::new()); + request.cwd = Some(cwd.clone()); + request.session_token = Some(token.clone()); + + let result = execute_command(request, &cfg, &CallerIdentity::Unix { uid }).await; + std::fs::remove_dir(cwd).unwrap(); + assert!(result.policy_allowed()); + let failure = result.execution_failure().unwrap().clone(); + assert_eq!(failure.stage, ExecutionStage::Cwd); + let interactions = cfg.state.sessions.read().await.interactions_snapshot(); + let (_, interaction) = interactions.iter().find(|(key, _)| key == &token).unwrap(); + assert!(interaction.allowed); + assert_eq!( + interaction.exec_status, + crate::session::SessionExecStatus::Failed + ); + assert_eq!(interaction.execution_failure.as_ref(), Some(&failure)); + let source = interaction.source; + drop(cfg); + + let (mut restarted, _) = make_test_config(); + let reopened = crate::session_store::SessionStore::open(database, 3600) + .await + .unwrap(); + *restarted.state.sessions.write().await = reopened.load_registry().await.unwrap(); + restarted.state.session_store = Some(reopened); + let interactions = restarted + .state + .sessions + .read() + .await + .interactions_snapshot(); + let (_, interaction) = interactions.iter().find(|(key, _)| key == &token).unwrap(); + assert!(interaction.allowed); + assert_eq!(interaction.source, source); + assert_eq!( + interaction.exec_status, + crate::session::SessionExecStatus::Failed + ); + assert_eq!(interaction.execution_failure.as_ref(), Some(&failure)); +} + #[cfg(unix)] #[tokio::test] async fn default_service_execution_does_not_forward_ssh_auth_sock() { diff --git a/src/server/tests/gating.rs b/src/server/tests/gating.rs index 1200a9a6..1d7c68ec 100644 --- a/src/server/tests/gating.rs +++ b/src/server/tests/gating.rs @@ -7389,7 +7389,7 @@ async fn approved_snapshot_rejects_missing_snapshotted_cwd_before_exec() { assert!( reason.contains("working directory") && reason.contains("changed before exec") - && reason.contains(cwd.to_str().unwrap()), + && !reason.contains(cwd.to_str().unwrap()), "unexpected reason: {reason}" ); } @@ -7442,7 +7442,7 @@ async fn approved_snapshot_rejects_retargeted_snapshotted_cwd_before_exec() { assert!( reason.contains("working directory") && reason.contains("changed before exec") - && reason.contains(cwd.to_str().unwrap()), + && !reason.contains(cwd.to_str().unwrap()), "unexpected reason: {reason}" ); } From d695b7adedf0029b03b9b7afe985fc0b3589055b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Wed, 9 Sep 2026 23:27:08 +0000 Subject: [PATCH 3/8] test: align launch failure regressions with typed diagnostics --- src/server/execute.rs | 7 +++++-- src/server/tests/exec_policy.rs | 20 ++++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/server/execute.rs b/src/server/execute.rs index 8112d79a..426a58a4 100644 --- a/src/server/execute.rs +++ b/src/server/execute.rs @@ -4952,7 +4952,10 @@ mod child_launch_tests { let temp = tempfile::tempdir().unwrap(); let cwd = temp.path().canonicalize().unwrap(); let denied = cwd.join("denied-directory"); - std::fs::DirBuilder::new().mode(0).create(&denied).unwrap(); + std::fs::DirBuilder::new() + .mode(0o000) + .create(&denied) + .unwrap(); assert!(denied.canonicalize().unwrap().metadata().unwrap().is_dir()); let error = spawn_brokered_command(shell("printf started"), Some(&denied), None).unwrap_err(); @@ -4963,7 +4966,7 @@ mod child_launch_tests { let ancestor = cwd.join("ancestor"); let nested = ancestor.join("nested"); std::fs::create_dir_all(&nested).unwrap(); - std::fs::set_permissions(&ancestor, std::fs::Permissions::from_mode(0)).unwrap(); + std::fs::set_permissions(&ancestor, std::fs::Permissions::from_mode(0o000)).unwrap(); let error = spawn_brokered_command(shell("printf started"), Some(&nested), None).unwrap_err(); std::fs::set_permissions(&ancestor, std::fs::Permissions::from_mode(0o700)).unwrap(); diff --git a/src/server/tests/exec_policy.rs b/src/server/tests/exec_policy.rs index eb8ab189..2cdea50a 100644 --- a/src/server/tests/exec_policy.rs +++ b/src/server/tests/exec_policy.rs @@ -2665,7 +2665,7 @@ async fn launch_failures_are_typed_in_buffered_and_streaming_execution() { let cwd = temp.path().canonicalize().unwrap(); let denied_cwd = cwd.join("denied-directory"); std::fs::DirBuilder::new() - .mode(0) + .mode(0o000) .create(&denied_cwd) .unwrap(); let denied_binary = cwd.join("denied-executable"); @@ -2819,7 +2819,7 @@ async fn launch_failure_is_recorded_as_an_allowed_session_interaction_after_rest .unwrap(), ); let cwd = temp.path().join("denied-directory"); - std::fs::DirBuilder::new().mode(0).create(&cwd).unwrap(); + std::fs::DirBuilder::new().mode(0o000).create(&cwd).unwrap(); let token = format!("launch-failure-{}", std::process::id()); let mut grant = unrestricted_session(); grant.owner = crate::session::SessionOwner::Principal(PrincipalKey::from_uid(uid)); @@ -3485,14 +3485,18 @@ async fn shim_dir_only_path_fails_without_recursing_into_primary_shim() { req.session_token = Some(token); let result = execute_command(req, &cfg, &CallerIdentity::Unix { uid: 1000 }).await; + assert!(result.policy_allowed()); + let failure = result.execution_failure().unwrap(); + assert_eq!(failure.stage, guard::wire::ExecutionStage::Exec); + assert_eq!(failure.errno, None); match result.exec { ExecOutcome::Failed { reason, started } => { assert!(!started); assert!( - reason.contains("underlying executable 'missing-tool' is unavailable"), + reason.contains("cannot be resolved on the configured command path"), "got: {reason}" ); - assert!(reason.contains("non-shim directory"), "got: {reason}"); + assert!(!reason.contains("missing-tool"), "got: {reason}"); } other => panic!("expected pre-start exec failure, got {:?}", other), } @@ -3561,14 +3565,18 @@ async fn allowed_binary_floor_does_not_permit_shim_dir_recursion() { req.session_token = Some(token); let result = execute_command(req, &cfg, &CallerIdentity::Unix { uid: 1000 }).await; + assert!(result.policy_allowed()); + let failure = result.execution_failure().unwrap(); + assert_eq!(failure.stage, guard::wire::ExecutionStage::Exec); + assert_eq!(failure.errno, None); match result.exec { ExecOutcome::Failed { reason, started } => { assert!(!started); assert!( - reason.contains("underlying executable 'allowed-tool' is unavailable"), + reason.contains("cannot be resolved on the configured command path"), "got: {reason}" ); - assert!(reason.contains("non-shim directory"), "got: {reason}"); + assert!(!reason.contains("allowed-tool"), "got: {reason}"); } other => panic!("expected pre-start exec failure, got {:?}", other), } From 87b2a179ee0473f420d2259f4e016eea178943bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Wed, 9 Sep 2026 23:46:19 +0000 Subject: [PATCH 4/8] fix: preserve execution outcomes across approval and rollback --- DEPLOYMENT.md | 8 +- docs/agent-integration.md | 5 +- src/cli_client.rs | 132 ++------- src/cli_server.rs | 26 +- src/gating/approval.rs | 6 + src/mcp.rs | 9 +- src/server/admin.rs | 71 ++++- src/server/gate_runtime.rs | 259 +++++++++------- src/server/tests/gating.rs | 589 ++++++++++++++++++++++++++++++++++--- src/server/tests/verbs.rs | 6 +- src/server/tests/wire.rs | 15 +- src/server/wire.rs | 27 +- src/session_store.rs | 151 +++++++++- src/wire/mod.rs | 4 +- tests/cli_output.rs | 92 ++++++ 15 files changed, 1096 insertions(+), 304 deletions(-) diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index c816d4e6..02cadc55 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -249,9 +249,13 @@ admin envelope accepts only the current operation and field grammar, so removed or malformed authority operations fail closed instead of selecting a compatibility path. -The state database uses schema version 14. Startup migrates an older database in +The state database uses schema version 15. Startup migrates an older database in place. Treat the installed binary, configuration, API-revert body tree, and -complete SQLite file set as one rollback unit. Before the first schema-14 +complete SQLite file set as one rollback unit. Schema 15 adds nullable execution-failure +details to session history; rows without these details carry no launch-stage or +start-state evidence. An older reader refuses the migrated database. Rollback +requires the matching stopped binary and its consistent pre-upgrade snapshot, +not an older binary pointed at the migrated database. Before the first schema-15 startup, resolve armed provisionals where practical, stop the service, verify that it is inactive, and create a consistent SQLite backup with the SQLite backup API. Copying only `state.db` while a process can write it can omit diff --git a/docs/agent-integration.md b/docs/agent-integration.md index 9682954d..0ac7de03 100644 --- a/docs/agent-integration.md +++ b/docs/agent-integration.md @@ -146,7 +146,10 @@ The optional `execution_failure` object carries `started`, `stage`, `errno`, and a sanitized `message`. Stages are `identity`, `capabilities`, `cwd`, `exec`, or `unknown`; `unknown` means no precise failing stage is established. The legacy `allowed` field remains false for an execution failure. Policy -approval does not establish that the command started or completed. +approval does not establish that the command started or completed. `started` is +`true` or `false` only for an observed start outcome. A `null` or absent value +means execution may have started, including an interrupted approval recovered +after restart. Such an outcome must not be retried automatically. ## MCP diff --git a/src/cli_client.rs b/src/cli_client.rs index ab96093b..1f3fb2c0 100644 --- a/src/cli_client.rs +++ b/src/cli_client.rs @@ -530,109 +530,7 @@ pub(crate) async fn run_exec( exit_for_execute_response(&resp); } - // Consequence-gate outcomes: a held command did not run; a provisional ran - // behind an auto-revert timer. - if resp.containment_failure.is_some() { - print_containment_failure(&resp, streamed_output); - } - match resp.status { - Some(server::GateStatus::Held) => { - print_held_banner(&resp); - print_coverage(&resp.coverage); - // Not executed; exit non-zero so callers do not treat it as success. - std::process::exit(EXIT_GUARD_HELD); - } - Some(server::GateStatus::Provisional) => { - let color = color_enabled_for_stderr(); - if !streamed_output { - if let Some(stdout) = &resp.stdout { - cli_print!("{}", stdout); - } - if let Some(stderr) = &resp.stderr { - eprint!("{}", stderr); - } - } - let handle = resp.handle.clone().unwrap_or_default(); - eprintln!( - "{} containment envelope: {}", - paint("PROVISIONAL", AnsiColor::Yellow, color), - resp.reason - ); - eprintln!(" handle: {}", handle); - eprintln!(" confirm: {}", operator_confirm_command(&handle)); - eprintln!(" inspect: guard provisionals"); - print_provisional_window(&resp); - print_coverage(&resp.coverage); - if let Some(code) = resp.exit_code { - std::process::exit(code); - } - return Ok(()); - } - Some(server::GateStatus::DryRun) => { - let color = color_enabled_for_stdout(); - cli_println!( - "{} {}", - paint("[DRY-RUN]", AnsiColor::Cyan, color), - resp.reason - ); - print_coverage(&resp.coverage); - return Ok(()); - } - _ => {} - } - - if let Some(failure) = &resp.execution_failure { - eprintln!("{}", execution_failure_text(failure)); - if explain { - if let Some(policy) = &resp.policy { - eprintln!( - " policy allowed: {}; source: {}; reason: {}", - policy.allowed, resp.decision_source, policy.reason - ); - } - } - std::process::exit(EXIT_GUARD_ERROR); - } - - if resp.allowed { - tracing::info!( - binary = %binary, - reason = %resp.reason, - "ALLOWED" - ); - if !streamed_output { - if let Some(stdout) = &resp.stdout { - cli_print!("{}", stdout); - } - if let Some(stderr) = &resp.stderr { - eprint!("{}", stderr); - } - } - if explain { - print_verb_guidance(&resp); - eprintln!(" decision source: {}", resp.decision_source); - } - if let Some(code) = resp.exit_code { - std::process::exit(code); - } - Ok(()) - } else { - let color = color_enabled_for_stderr(); - tracing::warn!( - binary = %binary, - reason = %resp.reason, - "DENIED" - ); - eprintln!( - "{}: {}", - paint("DENIED", AnsiColor::Red, color), - resp.reason - ); - print_deny_source(&resp); - print_access_request_guidance(&resp); - print_verb_guidance(&resp); - std::process::exit(EXIT_GUARD_DENIED); - } + render_gated_response(&resp, streamed_output, &binary, explain) } fn execution_failure_text(failure: &guard::wire::ExecutionFailure) -> String { @@ -1163,7 +1061,7 @@ pub(crate) async fn handle_resume( stdout.as_deref(), stderr.as_deref(), ); - document["policy"] = serde_json::to_value(policy)?; + document["policy"] = serde_json::to_value(&policy)?; document["execution_failure"] = serde_json::to_value(&execution_failure)?; document["decision_source"] = serde_json::to_value(decision_source)?; print_json(&document)?; @@ -1189,6 +1087,12 @@ pub(crate) async fn handle_resume( } std::process::exit(EXIT_GUARD_ERROR); } + if let Some(policy) = policy.filter(|policy| !policy.allowed) { + if !json { + eprintln!("DENIED: {}", card_text(&policy.reason)); + } + std::process::exit(EXIT_GUARD_DENIED); + } if let Some(code) = exit_code.filter(|code| *code != 0) { std::process::exit(code); } @@ -3387,7 +3291,7 @@ fn grant_class_wait_refusal(item: &server::AccessItem) -> String { )) } -fn render_gated_response( +pub(crate) fn render_gated_response( resp: &server::ExecuteResponse, streamed: bool, label: &str, @@ -3396,6 +3300,18 @@ fn render_gated_response( if resp.containment_failure.is_some() { print_containment_failure(resp, streamed); } + if let Some(failure) = &resp.execution_failure { + eprintln!("{}", execution_failure_text(failure)); + if explain { + if let Some(policy) = &resp.policy { + eprintln!( + " policy allowed: {}; source: {}; reason: {}", + policy.allowed, resp.decision_source, policy.reason + ); + } + } + std::process::exit(EXIT_GUARD_ERROR); + } match resp.status { Some(server::GateStatus::Held) => { print_held_banner(resp); @@ -3498,11 +3414,15 @@ pub(crate) async fn handle_gate_action( stdout, stderr, } => { - let _ = (policy, decision_source); + let _ = decision_source; if let Some(failure) = execution_failure { eprintln!("{}", execution_failure_text(&failure)); std::process::exit(EXIT_GUARD_ERROR); } + if let Some(policy) = policy.filter(|policy| !policy.allowed) { + eprintln!("DENIED: {}", card_text(&policy.reason)); + std::process::exit(EXIT_GUARD_DENIED); + } cli_println!("{}", message); if let Some(out) = &stdout { cli_print!("{}", out); diff --git a/src/cli_server.rs b/src/cli_server.rs index 76443616..8d5ffd9e 100644 --- a/src/cli_server.rs +++ b/src/cli_server.rs @@ -1,6 +1,5 @@ use super::{ - color_enabled_for_stderr, env_pairs_to_map, paint, parse_env_bool, resolve_bool_flag, - secret_pairs_to_map, AnsiColor, ServerCommands, + env_pairs_to_map, parse_env_bool, resolve_bool_flag, secret_pairs_to_map, ServerCommands, }; use crate::cli_client::handle_status; use crate::injection::{collect_unique_pairs, is_valid_env_name}; @@ -1809,28 +1808,7 @@ pub(crate) async fn run_server(cmd: ServerCommands) -> Result<()> { ) .await?; - if resp.allowed { - if !streamed_output { - if let Some(stdout) = &resp.stdout { - cli_print!("{}", stdout); - } - if let Some(stderr) = &resp.stderr { - eprint!("{}", stderr); - } - } - if let Some(code) = resp.exit_code { - std::process::exit(code); - } - Ok(()) - } else { - let color = color_enabled_for_stderr(); - eprintln!( - "{}: {}", - paint("DENIED", AnsiColor::Red, color), - resp.reason - ); - std::process::exit(1); - } + crate::cli_client::render_gated_response(&resp, streamed_output, &binary, false) } ServerCommands::Status { socket, json } => handle_status(socket, json).await, } diff --git a/src/gating/approval.rs b/src/gating/approval.rs index 6ebdb499..0cee82e8 100644 --- a/src/gating/approval.rs +++ b/src/gating/approval.rs @@ -418,6 +418,12 @@ impl ApprovalRegistry { row.decided_unix = Some(now); row.decided_reason = Some("daemon restarted while executing; outcome unknown".to_string()); + row.execution_failure = Some(crate::wire::ExecutionFailure { + started: None, + stage: crate::wire::ExecutionStage::Unknown, + errno: None, + message: "daemon restarted while executing; outcome unknown".to_string(), + }); recovered.push(row.handle.clone()); } items.insert(row.handle.clone(), row); diff --git a/src/mcp.rs b/src/mcp.rs index fd352a48..674c26b8 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -1794,7 +1794,7 @@ impl McpServer { ); properties.insert("execution_failure".to_string(), json!({ "type": ["object", "null"], "properties": { - "started": {"type": "boolean"}, "stage": {"type": "string"}, + "started": {"type": ["boolean", "null"]}, "stage": {"type": "string"}, "errno": {"type": ["integer", "null"]}, "message": {"type": "string"} }, "required": ["started", "stage", "errno", "message"], "additionalProperties": false })); @@ -2048,8 +2048,11 @@ impl McpServer { stdout, stderr, }) => { + let denied = policy.as_ref().is_some_and(|policy| !policy.allowed); let label = if execution_failure.is_some() { "EXECUTION FAILED: " + } else if denied { + "DENIED: " } else { "" }; @@ -2058,7 +2061,9 @@ impl McpServer { stdout.as_deref().unwrap_or_default(), stderr.as_deref().unwrap_or_default() ); - let is_error = execution_failure.is_some(); + let is_error = execution_failure.is_some() + || denied + || exit_code.is_some_and(|code| code != 0); let mut response = admin_tool_result( "approval_resume", text, diff --git a/src/server/admin.rs b/src/server/admin.rs index 0b2bcc18..9d1364b0 100644 --- a/src/server/admin.rs +++ b/src/server/admin.rs @@ -3117,10 +3117,25 @@ async fn approve_held_access( }; drop(transition); return match handle_approve_claimed(server, caller, handle, snapshot).await { - AdminResponse::GateAction { message, .. } => AccessDecisionResult { + AdminResponse::GateAction { + message, + policy, + execution_failure, + exit_code, + .. + } => AccessDecisionResult { request: handle.to_string(), - success: true, - state: "approved".to_string(), + success: policy.as_ref().is_none_or(|p| p.allowed) + && execution_failure.is_none() + && exit_code.is_none_or(|code| code == 0), + state: if policy.as_ref().is_some_and(|p| !p.allowed) { + "denied" + } else if execution_failure.is_some() { + "exec_failed" + } else { + "approved" + } + .to_string(), target: None, remaining_uses: None, use_policy: "unavailable".to_string(), @@ -7321,14 +7336,22 @@ async fn handle_manual_revert( } } let outcome = finish_revert(server, &claimed, caller, "manual").await; + let response = outcome.result.into_response(); + let exit_code = if response.execution_failure.is_some() { + Some(crate::EXIT_GUARD_ERROR) + } else if !response.allowed { + Some(crate::EXIT_GUARD_DENIED) + } else { + response.exit_code.or(Some(crate::EXIT_GUARD_ERROR)) + }; AdminResponse::GateAction { - policy: None, - execution_failure: None, - decision_source: None, - message: outcome.0, - exit_code: outcome.1, - stdout: None, - stderr: None, + policy: response.policy, + execution_failure: response.execution_failure, + decision_source: Some(response.decision_source), + message: outcome.message, + exit_code, + stdout: response.stdout, + stderr: response.stderr, } } @@ -7640,7 +7663,7 @@ async fn arm_held_command( } } -async fn handle_approve_claimed( +pub(super) async fn handle_approve_claimed( server: &ServerContext, caller: &CallerIdentity, handle: &str, @@ -7870,6 +7893,22 @@ async fn handle_approve_claimed( next, ) } + ExecOutcome::NotAttempted if !policy.as_ref().expect("execution policy").allowed => { + let detail = policy.as_ref().expect("execution policy").reason.clone(); + let mut next = expected.clone(); + next.status = ApprovalStatus::Denied; + next.decided_unix = Some(now); + next.decided_reason = Some(detail.clone()); + server.emit_audit_ungated( + AuditEvent::new(AuditKind::Denied) + .execution(policy.clone().expect("execution policy"), None) + .decision_source(decision_source.as_deref().unwrap_or("validation")) + .handle(handle) + .caller(caller) + .reason(&detail), + ); + (detail, Some(crate::EXIT_GUARD_DENIED), None, None, next) + } ExecOutcome::Failed { reason: detail, .. } => { server.emit_audit_ungated( AuditEvent::new(AuditKind::ApproveExecFailed) @@ -7903,7 +7942,7 @@ async fn handle_approve_claimed( }; ( format!("approved {} but execution failed: {}", handle, detail), - None, + Some(crate::EXIT_GUARD_ERROR), None, None, next, @@ -8056,8 +8095,14 @@ async fn handle_resume( stdout: None, stderr: None, }, - ExecOutcome::NotAttempted => AdminResponse::Error { + ExecOutcome::NotAttempted => AdminResponse::GateAction { + policy, + execution_failure, + decision_source, message: result.policy_reason().to_string(), + exit_code: Some(crate::EXIT_GUARD_DENIED), + stdout: None, + stderr: None, }, _ => AdminResponse::Error { message: format!("held command {handle} did not produce a terminal execution result"), diff --git a/src/server/gate_runtime.rs b/src/server/gate_runtime.rs index 490d0bfe..3b807b7b 100644 --- a/src/server/gate_runtime.rs +++ b/src/server/gate_runtime.rs @@ -3079,6 +3079,15 @@ pub(super) async fn resume_approval( terminal.result_stdout = bound_persisted_transcript(stdout.clone()); terminal.result_stderr = bound_persisted_transcript(stderr.clone()); } + ExecOutcome::NotAttempted if !result.policy_allowed() => { + terminal.status = ApprovalStatus::Denied; + terminal.decided_unix = Some(completed_unix); + terminal.decided_reason = Some(result.policy_reason().to_string()); + terminal.execution_failure = None; + terminal.result_exit = None; + terminal.result_stdout = None; + terminal.result_stderr = None; + } ExecOutcome::Failed { reason, .. } => { terminal.status = ApprovalStatus::ExecFailed; terminal.decided_unix = Some(completed_unix); @@ -3101,6 +3110,9 @@ pub(super) async fn resume_approval( if let Err(error) = commit_resumed_approval(server, claimed.clone(), terminal.clone(), true).await { + if !result.policy_allowed() { + return result; + } let message = format!("held command result was not durable: {error}"); let failed = if matches!( &result.exec, @@ -3113,19 +3125,30 @@ pub(super) async fn resume_approval( return failed.with_admission_trace(claimed.decision_trace.as_ref()); } server.emit_audit_ungated( - AuditEvent::new(AuditKind::ApprovedExecuted) - .handle(handle) - .caller(caller) - .session_fingerprint( - claimed - .snapshot - .session_fingerprint - .as_deref() - .unwrap_or("none"), - ) - .field("phase", "completed") - .field("status", terminal.status.as_str()) - .field("exit", format!("{:?}", terminal.result_exit)), + AuditEvent::new(if !result.policy_allowed() { + AuditKind::Denied + } else if result.execution_failure().is_some() { + AuditKind::ApproveExecFailed + } else { + AuditKind::ApprovedExecuted + }) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()) + .handle(handle) + .caller(caller) + .session_fingerprint( + claimed + .snapshot + .session_fingerprint + .as_deref() + .unwrap_or("none"), + ) + .field("phase", "completed") + .field("status", terminal.status.as_str()) + .field("exit", format!("{:?}", terminal.result_exit)), ); server.emit_event(NotifyEvent { event: "decision_made", @@ -3157,7 +3180,7 @@ pub(super) fn approval_to_result(a: &Approval) -> ExecuteResult { ApprovalStatus::Expired => { ExecuteResult::denied("expired without operator approval (fail-closed)") } - ApprovalStatus::ExecFailed => ExecuteResult::exec_failed( + ApprovalStatus::ExecFailed => ExecuteResult::exec_failed_unknown_start( a.reason.clone(), a.decided_reason .clone() @@ -3247,8 +3270,7 @@ async fn execute_snapshot_with_access_request_inner( ); } if snapshot.session_fingerprint.is_some() != snapshot.session_revision.is_some() { - return ExecuteResult::exec_failed( - reason.to_string(), + return ExecuteResult::denied( "approval rejected: originating session identity is incomplete".to_string(), ); } @@ -3266,8 +3288,7 @@ async fn execute_snapshot_with_access_request_inner( || expected.is_empty() || expected != supplied { - return ExecuteResult::exec_failed( - reason.to_string(), + return ExecuteResult::denied( "approval rejected: originating access session expired or was revoked, or the held access binding is incomplete" .to_string(), ); @@ -3284,8 +3305,7 @@ async fn execute_snapshot_with_access_request_inner( .await .effective_revision_for_fingerprint(fingerprint); if current.as_deref() != Some(expected_revision) { - return ExecuteResult::exec_failed( - reason.to_string(), + return ExecuteResult::denied( "approval rejected: the issued session changed or was revoked after hold" .to_string(), ); @@ -3503,16 +3523,12 @@ async fn execute_snapshot_request( match admit_access_use(server, &request, &selected_verbs, preferred_access_requests).await { Ok(Some(_)) => {} Ok(None) if preferred_access_requests.is_some() => { - return ExecuteResult::exec_failed( - reason.to_string(), - "approval rejected: originating access session expired or was revoked before held-command admission" - .to_string(), + return ExecuteResult::denied( + "originating access session expired or was revoked before held-command admission", ) } Ok(None) => {} - Err(admission_reason) => { - return ExecuteResult::exec_failed(reason.to_string(), admission_reason) - } + Err(admission_reason) => return ExecuteResult::denied(admission_reason), } let mut sink = tokio::io::sink(); let mut context = RequestContext { @@ -3915,10 +3931,15 @@ pub(super) async fn run_provisional_check( .await } +pub(super) struct RevertResult { + pub message: String, + pub result: ExecuteResult, +} + pub(super) async fn finish_due_provisional( server: &ServerContext, p: &Provisional, -) -> (String, Option) { +) -> RevertResult { if p.confirm_check_binary.is_none() { return finish_revert(server, p, &CallerIdentity::Unknown, "auto").await; } @@ -3927,10 +3948,13 @@ pub(super) async fn finish_due_provisional( run_provisional_check(server, p), ) .await; - let check_exit = checked.ok().and_then(|result| match result.exec { - ExecOutcome::Completed { exit_code, .. } => exit_code, - _ => None, + let checked = checked.unwrap_or_else(|_| { + ExecuteResult::exec_failed_unknown_start( + "provisional confirmation check authorized", + "confirmation check timed out; outcome unknown", + ) }); + let check_exit = checked.exit_code(); if check_exit == Some(0) { let expected = server .state @@ -3973,10 +3997,10 @@ pub(super) async fn finish_due_provisional( status: Some("confirmed".to_string()), behavior: None, }); - return ( - format!("provisional {} confirmed by independent check", p.handle), - Some(0), - ); + return RevertResult { + message: format!("provisional {} confirmed by independent check", p.handle), + result: checked, + }; } Ok(false) => tracing::warn!( "confirmation check succeeded but provisional {} changed before publication", @@ -3999,6 +4023,11 @@ pub(super) async fn finish_due_provisional( } server.emit_audit_ungated( AuditEvent::new(AuditKind::ProvisionalCheckFailed) + .execution( + checked.policy_decision(), + checked.execution_failure().cloned(), + ) + .decision_source(checked.decision_source()) .handle(&p.handle) .reason("running rollback") .field("exit", format!("{check_exit:?}")), @@ -4159,7 +4188,8 @@ async fn defer_revert( caller: &CallerIdentity, kind: &str, detail: String, -) -> (String, Option) { + result: ExecuteResult, +) -> RevertResult { let updated = { let mut reg = server.state.provisional.write().await; reg.set_needs_operator_decision(&p.handle, detail.clone()); @@ -4172,17 +4202,22 @@ async fn defer_revert( p.handle, error ); - return ( - format!( + return RevertResult { + message: format!( "provisional {} revert was deferred but its durable state could not be recorded; retry the operator action", p.handle ), - None, - ); + result, + }; } } server.emit_audit_ungated( AuditEvent::new(AuditKind::RevertDeferred) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()) .handle(&p.handle) .caller(caller) .reason(&detail) @@ -4198,84 +4233,81 @@ async fn defer_revert( status: Some("needs_operator_decision".to_string()), behavior: None, }); - ( - format!("provisional {} revert deferred: {}", p.handle, detail), - None, - ) + RevertResult { + message: format!("provisional {} revert deferred: {}", p.handle, detail), + result, + } } -/// Run a claimed (`Reverting`) provisional's revert and record the outcome. -/// Returns `(message, exit_code)`. +/// Run a claimed provisional's revert and retain its policy and execution outcome. pub(super) async fn finish_revert( server: &ServerContext, p: &Provisional, caller: &CallerIdentity, kind: &str, -) -> (String, Option) { - // Bound the revert so a hung rollback cannot pin the sweeper (which also - // drives fail-closed hold expiry). A timeout is recorded as RevertFailed. - let (status_ok, exit, detail) = if let Some(api) = &p.api_revert { +) -> RevertResult { + // A timeout leaves the operation's start and completion unknown. + let result = if let Some(api) = &p.api_revert { + let reason = "provisional API rollback authorized"; match tokio::time::timeout( std::time::Duration::from_secs(REVERT_EXEC_TIMEOUT_SECS), run_api_revert(server, p, api), ) .await { - Ok(Ok(())) => (true, Some(0), None), - // Recoverable (no proxy for the protocol right now): route to the - // operator instead of terminal-failing, so a restart or flag change - // does not silently strand a live mutation. + Ok(Ok(())) => ExecuteResult::completed(reason, Some(0), None, None), Ok(Err(RevertError::Retryable(detail))) => { - return defer_revert(server, p, caller, kind, detail).await; + let result = ExecuteResult::exec_failed_unknown_start(reason, detail.clone()); + return defer_revert(server, p, caller, kind, detail, result).await; } - Ok(Err(RevertError::Failed(reason))) => (false, None, Some(reason)), - Err(_) => ( - false, - None, - Some(format!( - "api revert timed out after {}s", - REVERT_EXEC_TIMEOUT_SECS - )), + Ok(Err(RevertError::Failed(detail))) => { + ExecuteResult::exec_failed_unknown_start(reason, detail) + } + Err(_) => ExecuteResult::exec_failed_unknown_start( + reason, + "API rollback timed out; outcome unknown", ), } } else { - match tokio::time::timeout( + let result = tokio::time::timeout( std::time::Duration::from_secs(REVERT_EXEC_TIMEOUT_SECS), run_provisional_revert(server, p), ) .await + .unwrap_or_else(|_| { + ExecuteResult::exec_failed_unknown_start( + "provisional rollback authorized", + "rollback timed out; outcome unknown", + ) + }); + if matches!(&result.exec, ExecOutcome::Failed { started: false, .. }) + && (!p.secret_keys.is_empty() || !p.secret_file_keys.is_empty()) { - Ok(result) => match &result.exec { - ExecOutcome::Completed { exit_code, .. } => { - let ok = exit_code.unwrap_or(-1) == 0; - (ok, *exit_code, None) - } - ExecOutcome::Failed { - started: false, - reason, - .. - } if !p.secret_keys.is_empty() || !p.secret_file_keys.is_empty() => { - return defer_revert( - server, - p, - caller, - kind, - format!("revert secret resolution or pre-spawn setup failed: {reason}"), - ) - .await; - } - ExecOutcome::Failed { reason, .. } => (false, None, Some(reason.clone())), - _ => (false, None, Some("unexpected revert outcome".to_string())), - }, - Err(_) => ( - false, - None, - Some(format!( - "revert timed out after {}s", - REVERT_EXEC_TIMEOUT_SECS - )), - ), + let detail = format!( + "revert secret resolution or pre-spawn setup failed: {}", + result + .execution_failure() + .expect("typed execution failure") + .message + ); + return defer_revert(server, p, caller, kind, detail, result).await; } + result + }; + let exit = result.exit_code(); + let status_ok = matches!( + &result.exec, + ExecOutcome::Completed { + exit_code: Some(0), + .. + } + ); + let detail = if let Some(failure) = result.execution_failure() { + Some(failure.message.clone()) + } else if !result.policy_allowed() { + Some(result.policy_reason().to_string()) + } else { + None }; // `kind` names who drove this rollback ("auto"/"auto-check-failed" for the // deadline sweeper, "manual" for operator reversion). Only the sweeper's own @@ -4315,13 +4347,18 @@ pub(super) async fn finish_revert( p.handle, diagnostic ); - return ( - format!( + return RevertResult { + message: format!( "provisional {} rollback completed but its terminal state could not be recorded: {}", p.handle, diagnostic ), - exit, - ); + result: if result.execution_failure().is_some() || !result.policy_allowed() { + result + } else { + ExecuteResult::exec_failed_after_start(result.policy_reason(), + "rollback result could not be persisted") + }, + }; } } // The revert is terminal (whether it succeeded or failed); drop any @@ -4330,6 +4367,11 @@ pub(super) async fn finish_revert( if status_ok { server.emit_audit_ungated( AuditEvent::new(AuditKind::Revert) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()) .handle(&p.handle) .caller(caller) .field("kind", kind) @@ -4345,13 +4387,18 @@ pub(super) async fn finish_revert( status: Some("reverted".to_string()), behavior: None, }); - ( - format!("provisional {} reverted (exit {:?})", p.handle, exit), - exit, - ) + RevertResult { + message: format!("provisional {} reverted (exit {:?})", p.handle, exit), + result, + } } else { server.emit_audit_ungated( AuditEvent::new(AuditKind::RevertFailed) + .execution( + result.policy_decision(), + result.execution_failure().cloned(), + ) + .decision_source(result.decision_source()) .handle(&p.handle) .caller(caller) .field("kind", kind) @@ -4368,15 +4415,15 @@ pub(super) async fn finish_revert( status: Some("revert_failed".to_string()), behavior: None, }); - ( - format!( + RevertResult { + message: format!( "REVERT FAILED for provisional {} (exit {:?}); the change may still be in place: {}", p.handle, exit, detail.unwrap_or_default() ), - exit, - ) + result, + } } } diff --git a/src/server/tests/gating.rs b/src/server/tests/gating.rs index 1200a9a6..c46a3efd 100644 --- a/src/server/tests/gating.rs +++ b/src/server/tests/gating.rs @@ -1879,7 +1879,7 @@ async fn due_confirm_check_reuses_secret_bindings_and_keeps_the_change() { ); let outcome = finish_due_provisional(&cfg, &due[0]).await; - assert_eq!(outcome.1, Some(0)); + assert_eq!(outcome.result.exit_code(), Some(0)); let row = cfg .state .provisional @@ -1888,7 +1888,12 @@ async fn due_confirm_check_reuses_secret_bindings_and_keeps_the_change() { .get(&handle) .cloned() .unwrap(); - assert_eq!(row.status, ProvisionalStatus::Confirmed, "{}", outcome.0); + assert_eq!( + row.status, + ProvisionalStatus::Confirmed, + "{}", + outcome.message + ); assert_eq!( row.session_fingerprint.as_deref(), Some(audit_session_fingerprint(Some("check-session")).as_str()) @@ -1950,7 +1955,7 @@ async fn due_failed_confirm_check_runs_the_rollback() { .unwrap(); let outcome = finish_due_provisional(&cfg, &due[0]).await; - assert_eq!(outcome.1, Some(0)); + assert_eq!(outcome.result.exit_code(), Some(0)); assert_eq!( cfg.state .provisional @@ -2108,13 +2113,15 @@ async fn provisional_revert_reresolves_secret_after_restart() { .await .begin_revert(&handle) .expect("claim recovered provisional"); - let (message, exit) = finish_revert( + let outcome = finish_revert( &restarted, &missing_claim, &CallerIdentity::Unknown, "operator", ) .await; + let exit = outcome.result.exit_code(); + let message = outcome.message; assert_eq!(exit, None); assert!(message.contains("deferred"), "got: {message}"); assert_eq!( @@ -2143,8 +2150,19 @@ async fn provisional_revert_reresolves_secret_after_restart() { .await .begin_revert(&handle) .expect("retry deferred provisional"); - let (_message, exit) = - finish_revert(&restarted, &retry, &CallerIdentity::Unknown, "operator").await; + let expected = store + .load_provisionals() + .await + .unwrap() + .into_iter() + .find(|row| row.handle == handle) + .unwrap(); + store + .compare_and_swap_provisional(expected, retry.clone()) + .await + .unwrap(); + let outcome = finish_revert(&restarted, &retry, &CallerIdentity::Unknown, "operator").await; + let exit = outcome.result.exit_code(); assert_eq!(exit, Some(0)); assert_eq!( std::fs::read_to_string(&output).expect("read revert output"), @@ -2282,7 +2300,9 @@ async fn api_revert_without_running_proxy_defers_to_operator() { // A missing proxy is recoverable: the change is still live, so the revert // is deferred to the operator (NeedsOperatorDecision) rather than burned as // a terminal RevertFailed. - let (message, exit) = finish_revert(&cfg, &provisional, &CallerIdentity::Unknown, "auto").await; + let outcome = finish_revert(&cfg, &provisional, &CallerIdentity::Unknown, "auto").await; + let exit = outcome.result.exit_code(); + let message = outcome.message; assert!(message.contains("deferred"), "got: {message}"); assert_eq!(exit, None); let row = cfg @@ -2362,7 +2382,9 @@ async fn failed_revert_is_durable_queryable_and_notifies_operator() { .await .expect("persist reverting claim"); - let (message, exit) = finish_revert(&cfg, &claimed, &CallerIdentity::Unknown, "auto").await; + let outcome = finish_revert(&cfg, &claimed, &CallerIdentity::Unknown, "auto").await; + let exit = outcome.result.exit_code(); + let message = outcome.message; assert_eq!(exit, Some(1)); assert!(message.contains("REVERT FAILED"), "got: {message}"); @@ -2548,7 +2570,9 @@ async fn api_revert_executes_through_registered_proxy_upstream() { .await .insert(provisional.clone()); - let (message, exit) = finish_revert(&cfg, &provisional, &CallerIdentity::Unknown, "auto").await; + let outcome = finish_revert(&cfg, &provisional, &CallerIdentity::Unknown, "auto").await; + let exit = outcome.result.exit_code(); + let message = outcome.message; assert!(message.contains("reverted"), "got: {message}"); assert_eq!(exit, Some(0)); let row = cfg @@ -3390,7 +3414,9 @@ async fn hold_approval_arms_then_requester_resumes_once_with_output() { }, ) .await; - assert!(matches!(refused, AdminResponse::Error { .. })); + assert!( + matches!(refused, AdminResponse::GateAction { exit_code: Some(crate::EXIT_GUARD_DENIED), policy: Some(ref policy), .. } if !policy.allowed) + ); assert!(!marker.exists()); let resumed = handle_admin_request_for_test( @@ -3433,7 +3459,9 @@ async fn hold_approval_arms_then_requester_resumes_once_with_output() { }, ) .await; - assert!(matches!(replay, AdminResponse::Error { .. })); + assert!( + matches!(replay, AdminResponse::GateAction { exit_code: Some(crate::EXIT_GUARD_DENIED), policy: Some(ref policy), .. } if !policy.allowed) + ); let AdminResponse::AccessItem { item } = handle_admin_request_for_test( &cfg, @@ -3640,7 +3668,9 @@ async fn armed_hold_expires_across_restart_without_execution() { }, ) .await; - assert!(matches!(response, AdminResponse::Error { .. })); + assert!( + matches!(response, AdminResponse::GateAction { exit_code: Some(crate::EXIT_GUARD_DENIED), policy: Some(ref policy), .. } if !policy.allowed) + ); assert!(!marker.exists()); let durable = store.load_approvals().await.unwrap(); assert_eq!( @@ -4180,11 +4210,11 @@ async fn held_snapshot_does_not_fall_through_to_overlapping_authority() { assert!(items[0].success); assert_eq!(items[0].state, "armed"); let resumed = resume_approval(&cfg, &agent, &handle).await; - assert!(matches!( - resumed.exec, - ExecOutcome::Failed { ref reason, .. } - if reason.contains("access use limit is exhausted") - )); + assert!(matches!(resumed.exec, ExecOutcome::NotAttempted)); + assert!(!resumed.policy_allowed()); + assert!(resumed + .policy_reason() + .contains("access use limit is exhausted")); assert_eq!( cfg.state .sessions @@ -4498,13 +4528,9 @@ async fn held_access_replay_fails_if_staged_session_was_revoked() { Some(&access_requests), ) .await; - assert!(matches!( - result.exec, - ExecOutcome::Failed { - started: false, - ref reason - } if reason.contains("expired or was revoked") - )); + assert!(matches!(result.exec, ExecOutcome::NotAttempted)); + assert!(!result.policy_allowed()); + assert!(result.policy_reason().contains("expired or was revoked")); } #[cfg(unix)] @@ -6554,7 +6580,7 @@ async fn held_approval_catalog_race_is_linearized(replacement: VerbCatalog) { .get(&handle) .unwrap() .status, - ApprovalStatus::ExecFailed + ApprovalStatus::Denied ); } @@ -7060,7 +7086,9 @@ async fn sensitive_provisional_snapshots_are_redacted_and_cannot_replay() { .write() .await .insert(provisional.clone()); - let (message, exit) = finish_revert(&cfg, &provisional, &agent, "operator retry").await; + let outcome = finish_revert(&cfg, &provisional, &agent, "operator retry").await; + let exit = outcome.result.exit_code(); + let message = outcome.message; assert_eq!(exit, None); assert!(!message.contains(&sensitive)); let audit = std::fs::read_to_string(audit_directory.path().join("audit.jsonl")).unwrap(); @@ -7188,7 +7216,9 @@ async fn stored_entitlements_cover_tool_secrets_for_approval_check_and_revert() .await .unwrap(); cfg.state.provisional.write().await.insert(viable.clone()); - let (_, exit) = finish_revert(&cfg, &viable, &agent, "test").await; + let outcome = finish_revert(&cfg, &viable, &agent, "test").await; + let exit = outcome.result.exit_code(); + let _ = outcome.message; assert_eq!(exit, Some(0)); assert_eq!( cfg.state @@ -7258,11 +7288,11 @@ async fn approved_snapshot_rejects_changed_session_revision() { }; assert!(cfg.state.sessions.write().await.revoke(token)); let result = execute_snapshot(&cfg, &snapshot, "operator approved").await; - assert!(matches!( - result.exec, - ExecOutcome::Failed { started: false, ref reason } - if reason.contains("session changed or was revoked") - )); + assert!(matches!(result.exec, ExecOutcome::NotAttempted)); + assert!(!result.policy_allowed()); + assert!(result + .policy_reason() + .contains("session changed or was revoked")); } #[tokio::test] @@ -7495,9 +7525,15 @@ async fn provisional_revert_executes_in_snapshotted_cwd() { .await .insert(provisional.clone()); - let (_message, exit) = finish_revert(&cfg, &provisional, &agent, "test").await; + let outcome = finish_revert(&cfg, &provisional, &agent, "test").await; + let exit = outcome.result.exit_code(); - assert_eq!(exit, Some(0)); + assert_eq!(exit, None); + assert_eq!( + outcome.result.execution_failure().unwrap().started, + Some(true) + ); + assert!(outcome.message.contains("could not be recorded")); assert_eq!( std::fs::read_to_string(temp.path().join("provisional-cwd.txt")).unwrap(), "reverted" @@ -7534,3 +7570,488 @@ fn provisional_result_carries_contain_coverage() { assert_eq!(response.confirm_deadline_unix, Some(1_700_000_300)); assert_eq!(response.confirm_window_secs, Some(300)); } + +#[cfg(unix)] +async fn contract_hold( + cfg: &ServerContext, + agent: &CallerIdentity, + request: ExecuteRequest, + reason: &str, +) -> String { + let authority = match request.session_token.as_deref() { + Some(token) => live_authority(cfg, token).await, + None => None, + }; + let mut sink = tokio::io::sink(); + let result = hold_for_approval_with_trace( + &mut RequestContext { + server: cfg, + caller: agent, + depth: 0, + stream_output: false, + stream_writer: &mut sink, + }, + request, + agent.principal(), + GateInputs { + reason: reason.into(), + risk: Some(8), + reversibility: Some(Reversibility::Irreversible), + revert_preauthorized: false, + verb: None, + bypass: false, + authority, + consume_access_verbs: Vec::new(), + force_hold: false, + }, + Some(guard::gating::DecisionTrace::source("static_policy")), + ) + .await; + match result.exec { + ExecOutcome::Held { handle, .. } => handle, + other => panic!("{other:?}"), + } +} + +#[cfg(unix)] +async fn contract_launch_failure_tool(cfg: &ServerContext, directory: &std::path::Path) { + use std::os::unix::fs::PermissionsExt; + let executable = directory.join("fixture-launch-failure"); + std::fs::write(&executable, "#!/nonexistent-guard-fixture-interpreter\n").unwrap(); + std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); + *cfg.state.tool_registry.write().await = + crate::tool_config::ToolRegistry::load(directory.join("tools.yaml")).unwrap(); + cfg.state + .tool_registry + .write() + .await + .set( + "fixture-launch-failure", + crate::tool_config::ToolConfig { + env: HashMap::from([("PATH".into(), directory.to_string_lossy().into_owned())]), + ..Default::default() + }, + ) + .unwrap(); +} + +#[cfg(unix)] +#[tokio::test] +async fn legacy_and_crash_recovered_approval_keep_unknown_start_after_real_side_effect() { + for crash in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("state.db"); + let marker = directory.path().join("effects"); + let store = SessionStore::open(path.clone(), 3600).await.unwrap(); + let (mut cfg, _, agent) = gating_config(7090, 1000); + cfg.state.session_store = Some(store.clone()); + let handle = contract_hold( + &cfg, + &agent, + held_request( + "sh", + vec!["-c".into(), format!("printf x >> '{}'", marker.display())], + None, + ), + "approved fixture", + ) + .await; + let pending = cfg + .state + .approvals + .read() + .await + .get(&handle) + .unwrap() + .clone(); + let mut claimed = pending.clone(); + claimed.status = ApprovalStatus::Approving; + store + .compare_and_swap_approval(pending, claimed.clone()) + .await + .unwrap(); + let ran = execute_snapshot(&cfg, &claimed.snapshot, &claimed.reason).await; + assert_eq!(ran.exit_code(), Some(0)); + assert_eq!(std::fs::read_to_string(&marker).unwrap(), "x"); + if !crash { + let mut legacy = claimed.clone(); + legacy.status = ApprovalStatus::ExecFailed; + legacy.decided_unix = Some(now_unix()); + legacy.decided_reason = Some("legacy interrupted execution".into()); + assert!(legacy.execution_failure.is_none()); + store + .compare_and_swap_approval(claimed, legacy) + .await + .unwrap(); + } + drop(cfg); + drop(store); + let reopened = SessionStore::open(path, 3600).await.unwrap(); + let (registry, recovered) = guard::gating::approval::ApprovalRegistry::from_rows( + reopened.load_approvals().await.unwrap(), + now_unix(), + ); + assert_eq!(recovered.len(), usize::from(crash)); + let approval = registry.get(&handle).unwrap(); + let replay = approval_to_result(approval); + assert!( + matches!(replay.exec, ExecOutcome::Failed { started: true, .. }), + "unknown must contain conservatively" + ); + let response = replay.into_response(); + assert!(!response.allowed); + assert!(response.policy.unwrap().allowed); + let failure = response.execution_failure.unwrap(); + assert_eq!(failure.started, None); + assert_eq!(failure.stage, guard::wire::ExecutionStage::Unknown); + assert_eq!( + std::fs::read_to_string(&marker).unwrap(), + "x", + "observation never replays the command" + ); + } +} + +#[cfg(unix)] +#[tokio::test] +async fn requester_resume_launch_failure_preserves_completion_audit_and_restart_redaction() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("state.db"); + let (mut cfg, operator, agent) = gating_config(7091, 1000); + let (audit_directory, _audit) = super::attach_test_audit_log(&mut cfg); + cfg.state.session_store = Some(SessionStore::open(path.clone(), 3600).await.unwrap()); + contract_launch_failure_tool(&cfg, directory.path()).await; + let handle = contract_hold( + &cfg, + &agent, + held_request("fixture-launch-failure", Vec::new(), None), + "policy permits password=fixture-resume-private", + ) + .await; + let armed = handle_admin_request_for_test( + &cfg, + &operator, + AdminRequest::Approve { + handle: handle.clone(), + }, + ) + .await; + assert!( + matches!(armed, AdminResponse::GateAction { .. }), + "{armed:?}" + ); + let result = resume_approval(&cfg, &agent, &handle).await; + assert!(result.policy_allowed(), "{}", result.policy_reason()); + let failure = result + .execution_failure() + .expect("actual failed launch") + .clone(); + assert_eq!(failure.started, Some(false)); + let response = result.into_response(); + let audit = std::fs::read_to_string(audit_directory.path().join("audit.jsonl")).unwrap(); + assert!(!audit.contains("fixture-resume-private")); + let completion = contract_audit_events(audit_directory.path()) + .into_iter() + .find(|event| event.fields.contains(&("phase".into(), "completed".into()))) + .expect("completion audit"); + assert_eq!(completion.execution_failure, Some(failure)); + assert!(completion.policy.as_ref().unwrap().allowed); + assert_eq!(completion.decision_source.as_deref(), Some("static_policy")); + drop(cfg); + let reopened = SessionStore::open(path, 3600).await.unwrap(); + let approval = reopened.load_approvals().await.unwrap().pop().unwrap(); + let replay = approval_to_result(&approval).into_response(); + assert_eq!(replay.execution_failure, response.execution_failure); + assert_eq!(replay.policy, response.policy); + assert!(!serde_json::to_string(&approval) + .unwrap() + .contains("fixture-resume-private")); +} + +#[cfg(unix)] +#[tokio::test] +async fn revoked_held_authority_stays_denied_in_immediate_persisted_and_replayed_results() { + for immediate in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("state.db"); + let marker = directory.path().join("must-not-run"); + let store = SessionStore::open(path.clone(), 3600).await.unwrap(); + let (mut cfg, operator, agent) = gating_config(7092, 1000); + cfg.state.session_store = Some(store.clone()); + cfg.state + .sessions + .write() + .await + .grant("revoked-fixture".into(), active_session()); + store + .persist_registry(&cfg.state.sessions.read().await.clone()) + .await + .unwrap(); + let mut request = held_request( + "sh", + vec!["-c".into(), format!("printf x > '{}'", marker.display())], + None, + ); + request.session_token = Some("revoked-fixture".into()); + let handle = contract_hold(&cfg, &agent, request, "original admission").await; + let armed = handle_admin_request_for_test( + &cfg, + &operator, + AdminRequest::Approve { + handle: handle.clone(), + }, + ) + .await; + assert!( + matches!(armed, AdminResponse::GateAction { .. }), + "{armed:?}" + ); + cfg.state.sessions.write().await.revoke("revoked-fixture"); + let response = if immediate { + let armed = cfg + .state + .approvals + .read() + .await + .get(&handle) + .unwrap() + .clone(); + let mut claimed = armed.clone(); + claimed.status = ApprovalStatus::Approving; + store + .compare_and_swap_approval(armed, claimed.clone()) + .await + .unwrap(); + cfg.state + .approvals + .write() + .await + .install_persisted(claimed.clone(), false); + crate::server::admin::handle_approve_claimed(&cfg, &operator, &handle, claimed.snapshot) + .await + } else { + handle_admin_request_for_test( + &cfg, + &agent, + AdminRequest::Resume { + handle: handle.clone(), + }, + ) + .await + }; + let AdminResponse::GateAction { + policy, + execution_failure, + exit_code, + .. + } = response + else { + panic!("{response:?}") + }; + assert_eq!(exit_code, Some(crate::EXIT_GUARD_DENIED)); + assert!(execution_failure.is_none()); + let policy = policy.unwrap(); + assert!(!policy.allowed); + assert!(policy.reason.contains("revoked")); + assert!(!marker.exists()); + drop(cfg); + drop(store); + let reopened = SessionStore::open(path, 3600).await.unwrap(); + let approval = reopened.load_approvals().await.unwrap().pop().unwrap(); + assert_eq!(approval.status, ApprovalStatus::Denied); + let replay = approval_to_result(&approval).into_response(); + assert_eq!(replay.policy, Some(policy)); + assert!(!replay.allowed && replay.execution_failure.is_none()); + assert_eq!(replay.decision_source, "validation"); + assert!(!marker.exists()); + } +} + +#[cfg(unix)] +fn contract_audit_events(directory: &std::path::Path) -> Vec { + std::fs::read_to_string(directory.join("audit.jsonl")) + .unwrap() + .lines() + .map(|line| { + serde_json::from_str::(line) + .unwrap() + .event + }) + .collect() +} + +#[cfg(unix)] +#[tokio::test] +async fn manual_rollback_retains_launch_failure_and_completed_nonzero() { + for launch_failure in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let store = SessionStore::open(directory.path().join("state.db"), 3600) + .await + .unwrap(); + let (mut cfg, operator, agent) = gating_config(7093, 1000); + cfg.state.session_store = Some(store.clone()); + let (audit_directory, _audit) = super::attach_test_audit_log(&mut cfg); + contract_launch_failure_tool(&cfg, directory.path()).await; + let revert = if launch_failure { + RevertSpec::new("fixture-launch-failure", Vec::new()) + } else { + RevertSpec::new("sh", vec!["-c".into(), "exit 9".into()]) + }; + let mut sink = tokio::io::sink(); + let forward = arm_containment_with_authority( + &mut RequestContext { + server: &cfg, + caller: &agent, + depth: 0, + stream_output: false, + stream_writer: &mut sink, + }, + contain_request("true", &[], revert), + agent.principal(), + "permitted change".into(), + None, + ) + .await; + let ExecOutcome::Provisional { handle, .. } = forward.exec else { + panic!("{:?}", forward.exec) + }; + let response = + handle_admin_request_for_test(&cfg, &operator, AdminRequest::Revert { handle }).await; + let AdminResponse::GateAction { + policy, + execution_failure, + exit_code, + decision_source, + .. + } = response + else { + panic!("{response:?}") + }; + assert!(policy.as_ref().unwrap().allowed); + assert_eq!( + exit_code, + Some(if launch_failure { + crate::EXIT_GUARD_ERROR + } else { + 9 + }) + ); + assert_eq!(execution_failure.is_some(), launch_failure); + if let Some(failure) = &execution_failure { + assert_eq!(failure.started, Some(false)); + } + let event = contract_audit_events(audit_directory.path()) + .into_iter() + .find(|event| event.kind == guard::audit::AuditKind::RevertFailed) + .unwrap(); + assert_eq!(event.execution_failure, execution_failure); + assert_eq!(event.policy, policy); + assert_eq!(event.decision_source, decision_source); + let terminal = store.load_provisionals().await.unwrap().pop().unwrap(); + assert_eq!(terminal.status, ProvisionalStatus::RevertFailed); + assert_eq!( + terminal.revert_exit, + if launch_failure { None } else { Some(9) } + ); + } +} + +#[cfg(unix)] +#[tokio::test] +async fn confirmation_check_audits_launch_failure_separately_and_preserves_rollback() { + for launch_failure in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let marker = directory.path().join("rolled-back"); + let store = SessionStore::open(directory.path().join("state.db"), 3600) + .await + .unwrap(); + let (mut cfg, _, agent) = gating_config(7094, 1000); + cfg.state.session_store = Some(store.clone()); + let (audit_directory, _audit) = super::attach_test_audit_log(&mut cfg); + contract_launch_failure_tool(&cfg, directory.path()).await; + let mut revert = RevertSpec::new( + "sh", + vec!["-c".into(), format!("printf x > '{}'", marker.display())], + ); + revert.confirm_check = Some(crate::server::CommandSpec { + binary: if launch_failure { + "fixture-launch-failure" + } else { + "false" + } + .into(), + args: Vec::new(), + }); + let mut sink = tokio::io::sink(); + let forward = arm_containment_with_authority( + &mut RequestContext { + server: &cfg, + caller: &agent, + depth: 0, + stream_output: false, + stream_writer: &mut sink, + }, + contain_request("true", &[], revert), + agent.principal(), + "permitted change".into(), + None, + ) + .await; + assert!( + matches!(forward.exec, ExecOutcome::Provisional { .. }), + "{:?}", + forward.exec + ); + let due = cfg + .state + .provisional + .write() + .await + .take_due(now_unix() + 10_000_000) + .pop() + .unwrap(); + let durable = store.load_provisionals().await.unwrap().pop().unwrap(); + store + .compare_and_swap_provisional(durable, due.clone()) + .await + .unwrap(); + let expected_check = run_provisional_check(&cfg, &due).await; + let rollback = finish_due_provisional(&cfg, &due).await; + assert_eq!(rollback.result.exit_code(), Some(0)); + assert!(rollback.result.execution_failure().is_none()); + assert_eq!(std::fs::read_to_string(&marker).unwrap(), "x"); + let events = contract_audit_events(audit_directory.path()); + let check = events + .iter() + .find(|event| event.kind == guard::audit::AuditKind::ProvisionalCheckFailed) + .unwrap(); + assert!(check.policy.as_ref().unwrap().allowed); + assert!(check.decision_source.is_some()); + assert_eq!(check.execution_failure.is_some(), launch_failure); + assert_eq!( + check.execution_failure.as_ref(), + expected_check.execution_failure() + ); + assert_eq!(check.policy, Some(expected_check.policy_decision())); + assert_eq!( + check.decision_source.as_deref(), + Some(expected_check.decision_source()) + ); + if let Some(failure) = &check.execution_failure { + assert_eq!(failure.started, Some(false)); + assert!(!failure.message.is_empty()); + } else { + assert!(check.fields.contains(&("exit".into(), "Some(1)".into()))); + } + let reverted = events + .iter() + .find(|event| event.kind == guard::audit::AuditKind::Revert) + .unwrap(); + assert!(reverted.execution_failure.is_none()); + assert!(reverted.policy.as_ref().unwrap().allowed); + assert_eq!( + store.load_provisionals().await.unwrap()[0].status, + ProvisionalStatus::Reverted + ); + } +} diff --git a/src/server/tests/verbs.rs b/src/server/tests/verbs.rs index df23ab4b..acdce62e 100644 --- a/src/server/tests/verbs.rs +++ b/src/server/tests/verbs.rs @@ -626,7 +626,7 @@ async fn held_replay_rejects_session_revision_amendment_before_process_start() { .get(&handle) .unwrap() .status, - ApprovalStatus::ExecFailed + ApprovalStatus::Denied ); } @@ -711,7 +711,7 @@ async fn held_replay_rejects_interaction_suspension_before_process_start() { .get(&handle) .unwrap() .status, - ApprovalStatus::ExecFailed + ApprovalStatus::Denied ); } @@ -797,7 +797,7 @@ verbs: .get(&handle) .unwrap() .status, - ApprovalStatus::ExecFailed + ApprovalStatus::Denied ); } diff --git a/src/server/tests/wire.rs b/src/server/tests/wire.rs index 9c200cdd..fead088f 100644 --- a/src/server/tests/wire.rs +++ b/src/server/tests/wire.rs @@ -442,7 +442,7 @@ fn execution_failure_preserves_policy_in_buffered_and_streamed_results() { ); assert_eq!(response.decision_source, "static_policy"); let failure = response.execution_failure.as_ref().unwrap(); - assert!(!failure.started); + assert_eq!(failure.started, Some(false)); assert_eq!(failure.stage, stage); assert_eq!(failure.errno, Some(13)); let encoded = serde_json::to_value(&response).unwrap(); @@ -464,7 +464,7 @@ fn execution_failure_unknown_preserves_started_and_legacy_deserialization() { let started = matches!(result.exec, ExecOutcome::Failed { started: true, .. }); let response = result.into_response(); let failure = response.execution_failure.as_ref().unwrap(); - assert_eq!(failure.started, started); + assert_eq!(failure.started, Some(started)); assert_eq!(failure.stage, ExecutionStage::Unknown); assert_eq!(failure.errno, None); } @@ -477,6 +477,15 @@ fn execution_failure_unknown_preserves_started_and_legacy_deserialization() { ) .unwrap(); assert_eq!(unknown.stage, ExecutionStage::Unknown); + let unknown_start: guard::wire::ExecutionFailure = serde_json::from_value( + serde_json::json!({"stage":"future_stage","errno":null,"message":"outcome unknown"}), + ) + .unwrap(); + assert_eq!(unknown_start.started, None); + assert_eq!( + serde_json::to_value(unknown_start).unwrap()["started"], + serde_json::Value::Null + ); let denied = ExecuteResult::denied("policy rejects this").into_response(); assert!(!denied.policy.unwrap().allowed); assert!(denied.execution_failure.is_none()); @@ -517,7 +526,7 @@ fn execution_failure_held_projection_retains_admission_and_started_state() { let summary = crate::server::wire::ApprovalSummary::from_row(&approval); assert_eq!(summary.execution_failure, approval.execution_failure); let response = crate::server::gate_runtime::approval_to_result(&approval).into_response(); - assert_eq!(response.execution_failure.unwrap().started, started); + assert_eq!(response.execution_failure.unwrap().started, Some(started)); assert_eq!(response.decision_source, "static_policy"); assert_eq!( response.policy.unwrap(), diff --git a/src/server/wire.rs b/src/server/wire.rs index 0f47d697..50ca3b8b 100644 --- a/src/server/wire.rs +++ b/src/server/wire.rs @@ -1709,7 +1709,7 @@ impl ExecuteResult { operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), execution_failure: Some(ExecutionFailure { - started: false, + started: Some(false), stage: ExecutionStage::Unknown, errno: None, message: exec_reason, @@ -1741,7 +1741,7 @@ impl ExecuteResult { operator_guidance: false, audit_metadata: ExecutionAuditMetadata::default(), execution_failure: Some(ExecutionFailure { - started: true, + started: Some(true), stage: ExecutionStage::Unknown, errno: None, message: exec_reason, @@ -1762,7 +1762,7 @@ impl ExecuteResult { let message = Self::sanitize_prose(message); Self::exec_failed(policy_reason, message.clone()).with_execution_failure(Some( ExecutionFailure { - started: false, + started: Some(false), stage, errno, message, @@ -1775,12 +1775,28 @@ impl ExecuteResult { { let failure = failure.sanitized(); *reason = failure.message.clone(); - *started = failure.started; + // The internal containment flag is conservative when start is unknown. + *started = failure.started.unwrap_or(true); self.execution_failure = Some(failure); } self } + pub(super) fn exec_failed_unknown_start( + policy_reason: impl Into, + message: impl Into, + ) -> Self { + let message = Self::sanitize_prose(message); + Self::exec_failed_after_start(policy_reason, message.clone()).with_execution_failure(Some( + ExecutionFailure { + started: None, + stage: ExecutionStage::Unknown, + errno: None, + message, + }, + )) + } + pub(super) fn execution_failure(&self) -> Option<&ExecutionFailure> { self.execution_failure.as_ref() } @@ -2025,6 +2041,9 @@ impl ExecuteResult { } pub(super) fn with_admission_trace(mut self, trace: Option<&DecisionTrace>) -> Self { + if !self.policy_allowed() { + return self; + } if let Some(trace) = trace { self.decision_source = serde_json::from_value(serde_json::Value::String(trace.decision_source.clone())) diff --git a/src/session_store.rs b/src/session_store.rs index e90c4d84..287026a6 100644 --- a/src/session_store.rs +++ b/src/session_store.rs @@ -3028,6 +3028,7 @@ fn valid_approval_transition(previous: &Approval, next: &Approval) -> Result Vec>> { + [ + "gating_approval", + "gating_provisional", + "saved_grants", + "grant_requests", + "session_grants", + "session_history", + ] + .into_iter() + .map(|table| { + let mut stmt = conn + .prepare(&format!("SELECT * FROM {table} ORDER BY rowid")) + .unwrap(); + let count = stmt.column_count(); + stmt.query_map([], |row| (0..count).map(|i| row.get(i)).collect()) + .unwrap() + .collect::>>>() + .unwrap() + }) + .collect() + } + let authority = authority_rows(&conn); + drop(conn); + let migrated = SessionStore::open(path.clone(), 3600).await.unwrap(); + let conn = Connection::open(&path).unwrap(); + assert_eq!( + conn.query_row("PRAGMA user_version", [], |r| r.get::<_, i64>(0)) + .unwrap(), + 15 + ); + assert_eq!(authority_rows(&conn), authority); + let failure: Option = conn + .query_row( + "SELECT execution_failure_json FROM session_interactions", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(failure, None); + let interaction = migrated + .load_registry() + .await + .unwrap() + .typed_interactions_snapshot() + .pop() + .unwrap() + .1; + assert!(interaction.allowed); + assert_eq!(interaction.source, SessionDecisionSource::StaticPolicy); + assert!(interaction.execution_failure.is_none()); + assert_eq!( + migrated.load_approvals().await.unwrap()[0].snapshot, + approval.snapshot + ); + assert_eq!(migrated.load_provisionals().await.unwrap()[0], provisional); + assert_eq!(migrated.load_grant_requests().await.unwrap(), vec![request]); + drop(conn); + drop(migrated); + let before_refusal = std::fs::read(&path).unwrap(); + let reader = + Connection::open_with_flags(&path, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY).unwrap(); + let version = reader + .query_row("PRAGMA user_version", [], |r| r.get::<_, i64>(0)) + .unwrap(); + assert!(ensure_supported_schema_version(version, 14) + .unwrap_err() + .to_string() + .contains("newer than supported")); + drop(reader); + assert_eq!(std::fs::read(&path).unwrap(), before_refusal); + let rollback = + Connection::open_with_flags(&snapshot, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY) + .unwrap(); + let version = rollback + .query_row("PRAGMA user_version", [], |r| r.get::<_, i64>(0)) + .unwrap(); + assert_eq!(version, 14); + ensure_supported_schema_version(version, 14).unwrap(); + assert_eq!(authority_rows(&rollback), authority); + assert!(rollback + .prepare("SELECT execution_failure_json FROM session_interactions") + .is_err()); + let count = rollback + .query_row("SELECT COUNT(*) FROM session_interactions", [], |r| { + r.get::<_, i64>(0) + }) + .unwrap(); + assert_eq!(count, 1); + } + fn pending_approval(handle: &str) -> Approval { Approval { execution_failure: None, @@ -7670,11 +7807,15 @@ mod tests { let mut denied = approving.clone(); denied.status = ApprovalStatus::Denied; denied.decided_unix = Some(2); - denied.decided_reason = Some("late denial".to_string()); - assert!(store - .compare_and_swap_approval(approving, denied) + denied.decided_reason = Some("originating authority was revoked".to_string()); + store + .compare_and_swap_approval(approving.clone(), denied.clone()) .await - .is_err()); + .unwrap(); + assert!(store.save_approval(approving).await.is_err()); + let terminal = store.load_approvals().await.unwrap().pop().unwrap(); + assert_eq!(terminal.status, ApprovalStatus::Denied); + assert_eq!(terminal.decided_reason, denied.decided_reason); } #[tokio::test] diff --git a/src/wire/mod.rs b/src/wire/mod.rs index 94342bfb..fe157740 100644 --- a/src/wire/mod.rs +++ b/src/wire/mod.rs @@ -36,7 +36,9 @@ impl ExecutionStage { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct ExecutionFailure { - pub started: bool, + /// None means the execution may have started, but no observation survives. + #[serde(default)] + pub started: Option, pub stage: ExecutionStage, pub errno: Option, pub message: String, diff --git a/tests/cli_output.rs b/tests/cli_output.rs index 6a6f11d1..a74c519f 100644 --- a/tests/cli_output.rs +++ b/tests/cli_output.rs @@ -142,3 +142,95 @@ async fn execution_failure_cli_distinguishes_policy_in_text_and_json() { } } } + +#[cfg(unix)] +#[tokio::test] +async fn execution_result_consumers_share_failure_denial_and_child_exits() { + use serde_json::json; + use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; + for command in [ + vec!["verb", "run", "fixture"], + vec!["server", "connect", "true"], + vec!["revert", "pv-fixture"], + vec!["resume", "ap-fixture"], + ] { + for (started, denied, child_exit) in [ + (Some(false), false, None), + (Some(true), false, None), + (None, false, None), + (None, true, None), + (None, false, Some(7)), + ] { + let directory = tempfile::tempdir().unwrap(); + let socket = directory.path().join("guard.sock"); + let listener = tokio::net::UnixListener::bind(&socket).unwrap(); + let failed = !denied && child_exit.is_none(); + let mut response = json!({"allowed": child_exit.is_some(), "reason": "fixture outcome", + "policy": {"allowed": !denied, "reason": "fixture policy"}, "decision_source": "validation", + "exit_code": child_exit}); + if failed { + response["execution_failure"] = json!({"started": started, "stage": "cwd", + "errno": 13, "message": "working directory permission denied"}); + } + let admin = matches!(command[0], "revert" | "resume"); + if admin { + response["result"] = json!("gate_action"); + response["message"] = json!("fixture outcome"); + // A null legacy exit must not hide a typed failure or denial. + response["exit_code"] = json!(child_exit); + } + let server = async { + let (stream, _) = listener.accept().await.unwrap(); + let (reader, mut writer) = stream.into_split(); + let mut line = String::new(); + BufReader::new(reader).read_line(&mut line).await.unwrap(); + let request: serde_json::Value = serde_json::from_str(&line).unwrap(); + let payload = if request["execute"]["stream"] == true { + json!({"type": "result", "response": response}) + } else { + response + }; + writer + .write_all(format!("{payload}\n").as_bytes()) + .await + .unwrap(); + }; + let client = async { + tokio::process::Command::new(GUARD_BIN) + .env_clear() + .env("XDG_CONFIG_HOME", directory.path()) + .current_dir(directory.path()) + .kill_on_drop(true) + .args(&command) + .arg("--socket") + .arg(&socket) + .output() + .await + .unwrap() + }; + let (_, output) = tokio::time::timeout(std::time::Duration::from_secs(10), async { + tokio::join!(server, client) + }) + .await + .unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!( + output.status.code(), + Some(if failed { + 125 + } else if denied { + 126 + } else { + 7 + }), + "{command:?}: {stderr}" + ); + if failed { + assert!(stderr.contains("EXECUTION FAILED"), "{command:?}: {stderr}"); + assert!(!stderr.contains("DENIED") && !stderr.contains("appeal:")); + } else if denied { + assert!(stderr.contains("DENIED"), "{command:?}: {stderr}"); + } + } + } +} From 025e81f90de3229021b02652d0e89ea99439c343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Thu, 10 Sep 2026 00:06:20 +0000 Subject: [PATCH 5/8] test: wait for complete rollback notifications --- src/server/tests/gating.rs | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/server/tests/gating.rs b/src/server/tests/gating.rs index 5578ed78..e8ccf459 100644 --- a/src/server/tests/gating.rs +++ b/src/server/tests/gating.rs @@ -2334,12 +2334,16 @@ async fn failed_revert_is_durable_queryable_and_notifies_operator() { let (mut cfg, _operator, agent) = gating_config(7_015, 1_000); cfg.state.session_store = Some(store.clone()); let (audit_directory, _audit) = super::attach_test_audit_log(&mut cfg); - let event_path = state.path().join("notify-event.json"); + let events_directory = state.path().join("notifications"); + std::fs::create_dir(&events_directory).expect("create notification directory"); cfg.state.notify_hook = crate::server::runtime::NotifyHook::new( vec![ "sh".to_string(), "-c".to_string(), - format!("cat > '{}'", event_path.display()), + "capture=\"$1/notify-$$\"; cat > \"$capture.pending\" && mv \"$capture.pending\" \"$capture.json\"" + .to_string(), + "sh".to_string(), + events_directory.display().to_string(), ], 5, ); @@ -2430,23 +2434,28 @@ async fn failed_revert_is_durable_queryable_and_notifies_operator() { assert!(audit.contains("REVERT_FAILED"), "audit: {audit}"); let event = tokio::time::timeout(std::time::Duration::from_secs(2), async { - loop { - if event_path.exists() { - break std::fs::read_to_string(&event_path).expect("read notification event"); + 'notification: loop { + for entry in std::fs::read_dir(&events_directory).expect("read notifications") { + let path = entry.expect("notification entry").path(); + if path + .extension() + .is_some_and(|extension| extension == "json") + { + let event: serde_json::Value = serde_json::from_slice( + &std::fs::read(path).expect("read completed notification"), + ) + .expect("complete notification JSON"); + if event["event"] == "decision_made" && event["status"] == "revert_failed" { + break 'notification event; + } + } } - tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(10)).await; } }) .await .expect("revert notification event timed out"); - assert!( - event.contains("\"event\":\"decision_made\""), - "event: {event}" - ); - assert!( - event.contains("\"status\":\"revert_failed\""), - "event: {event}" - ); + assert_eq!(event["handle"], handle); } /// The sweeper executes a due API revert as an HTTP request through the From 5e366f4fd64d8fd72cb43fc5563bc47330cb0827 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Thu, 10 Sep 2026 00:26:58 +0000 Subject: [PATCH 6/8] fix: expose pending child cleanup and preserve termination errors --- src/server/execute.rs | 372 +++++++++++++++------------ src/server/runtime.rs | 435 +++++++++++++++++++++++++++++--- src/server/tests/exec_policy.rs | 77 ++++++ 3 files changed, 688 insertions(+), 196 deletions(-) diff --git a/src/server/execute.rs b/src/server/execute.rs index cd596010..c494f7db 100644 --- a/src/server/execute.rs +++ b/src/server/execute.rs @@ -53,7 +53,7 @@ use super::learning::{ }; #[cfg(unix)] use super::path_with_shim_dir; -use super::runtime::{ChildOwnership, NotifyEvent, ProcessGuard}; +use super::runtime::{ChildOwnership, CleanupOutcome, NotifyEvent, ProcessGuard}; use super::transport::{write_policy_decision, write_stream_message}; #[cfg(unix)] use super::wire::ExecOutcome; @@ -500,6 +500,7 @@ async fn revalidate_exec_cwd(cwd: &Path) -> std::result::Result<(), LaunchError> })?; if canonical != cwd { return Err(LaunchError { + cleanup: None, started: false, stage: ExecutionStage::Cwd, errno: None, @@ -515,6 +516,7 @@ async fn revalidate_exec_cwd(cwd: &Path) -> std::result::Result<(), LaunchError> })?; if !meta.is_dir() { return Err(LaunchError { + cleanup: None, started: false, stage: ExecutionStage::Cwd, errno: None, @@ -2916,6 +2918,7 @@ fn drop_brokered_child_capabilities() -> std::io::Result<()> { #[derive(Debug)] struct LaunchError { + cleanup: Option, started: bool, stage: ExecutionStage, errno: Option, @@ -2925,6 +2928,7 @@ struct LaunchError { impl LaunchError { fn from_io(stage: ExecutionStage, message: &'static str, error: &std::io::Error) -> Self { Self { + cleanup: None, started: false, stage, errno: error.raw_os_error(), @@ -3014,6 +3018,7 @@ fn prepare_child_setup( .map(|path| CString::new(path.as_os_str().as_bytes())) .transpose() .map_err(|_| LaunchError { + cleanup: None, started: false, stage: ExecutionStage::Cwd, errno: Some(libc::EINVAL), @@ -3146,6 +3151,7 @@ fn read_child_setup_error( _ => return None, }; (errno > 0 && spawn_error.raw_os_error() == Some(errno)).then_some(LaunchError { + cleanup: None, started: false, stage, errno: Some(errno), @@ -3180,6 +3186,7 @@ impl ManagedChild { .map(ChildStdout::from_std) .transpose() .map_err(|error| LaunchError { + cleanup: None, started: true, stage: ExecutionStage::Unknown, errno: error.raw_os_error(), @@ -3195,6 +3202,7 @@ impl ManagedChild { .map(ChildStderr::from_std) .transpose() .map_err(|error| LaunchError { + cleanup: None, started: true, stage: ExecutionStage::Unknown, errno: error.raw_os_error(), @@ -3300,8 +3308,11 @@ fn spawn_brokered_command( secret_files: Option, ) -> std::result::Result { let mut child = spawn_owned_command(cmd, cwd, identity, secret_files)?; - child.attach_stdout()?; - child.attach_stderr()?; + if let Err(mut error) = child.attach_stdout().and_then(|()| child.attach_stderr()) { + child.ownership.terminate(false); + error.cleanup = Some(child.ownership.clone()); + return Err(error); + } Ok(child) } @@ -4070,180 +4081,218 @@ pub(super) async fn exec_after_approval_with_command_authority { if error.started { audit_credential_access(server, caller, &request, &credential_references); - return error + let cleanup = match &error.cleanup { + Some(owner) => Some((owner.cleanup_id(), owner.wait_for_cleanup().await)), + None => None, + }; + let result = error .into_result(allow_reason) .with_credential_references(credential_references); + return match cleanup { + Some((cleanup_id, outcome)) => { + with_cleanup_outcome(result, cleanup_id, outcome) + } + None => result, + }; } return error.into_result(allow_reason); } }; - drop(tool_mapping_lease); - drop(initiation_lease); - #[cfg(all(test, unix))] - signal_command_started_for_test(server); - - if context.stream_output { - let result = execute_streaming_child( - child, - allow_reason, - exec_timeout_secs, - server, - OutputRedactionContext { - environment: &redaction_env, - exact_secrets: &exact_output_secrets, - }, - SpawnAuditContext { - caller, - request: &request, - credential_references, - }, - &mut *context.stream_writer, - ) - .await; - return result; - } - - let mut process_guard = Some(server.state.process_tracker.track(child.ownership.clone())); - audit_credential_access(server, caller, &request, &credential_references); - let stdout_pipe = child.stdout.take(); - let stderr_pipe = child.stderr.take(); - let exact_secrets = server - .config - .redact_secrets - .iter() - .chain(exact_output_secrets.iter()) - .map(|secret| secret.as_bytes().to_vec()) - .collect::>(); - let raw_total = Arc::new(AtomicUsize::new(0)); - let stdout_secrets = exact_secrets.clone(); - let stdout_total = raw_total.clone(); - let stdout_reader = async move { - match stdout_pipe { - Some(pipe) => read_bounded_redacted_output(pipe, stdout_secrets, stdout_total).await, - None => Ok(Vec::new()), - } - }; - let stderr_reader = async move { - match stderr_pipe { - Some(pipe) => read_bounded_redacted_output(pipe, exact_secrets, raw_total).await, - None => Ok(Vec::new()), - } - }; - let execution_deadline = - tokio::time::sleep(std::time::Duration::from_secs(exec_timeout_secs.max(1))); - tokio::pin!(execution_deadline); - let buffered_output = if exec_timeout_secs == 0 { - Ok(collect_bounded_output_pair(stdout_reader, stderr_reader).await) - } else { - tokio::select! { - result = collect_bounded_output_pair(stdout_reader, stderr_reader) => Ok(result), - _ = &mut execution_deadline => Err(()), - } - }; - let buffered_output = match buffered_output { - Err(()) => { - terminate_spawned_child(&mut child, &mut process_guard).await; - return ExecuteResult::exec_failed_after_start( - allow_reason, - exec_timeout_reason(exec_timeout_secs), - ) - .with_credential_references(credential_references); - } - Ok(Err(error)) => { - terminate_spawned_child(&mut child, &mut process_guard).await; - return ExecuteResult::exec_failed_after_start(allow_reason, error.to_string()) - .with_credential_references(credential_references); - } - Ok(Ok(output)) => output, - }; - let wait_result = if exec_timeout_secs == 0 { - Ok(child.wait().await) - } else { - tokio::select! { - result = child.wait() => Ok(result), - _ = &mut execution_deadline => Err(()), - } - }; - let status = match wait_result { - Err(()) => { - terminate_spawned_child(&mut child, &mut process_guard).await; - return ExecuteResult::exec_failed_after_start( - allow_reason, - exec_timeout_reason(exec_timeout_secs), - ) - .with_credential_references(credential_references); - } - Ok(Ok(status)) => status, - Ok(Err(e)) => { - return ExecuteResult::exec_failed_after_start( + let cleanup_owner = child.ownership.clone(); + let result = async { + drop(tool_mapping_lease); + drop(initiation_lease); + #[cfg(all(test, unix))] + signal_command_started_for_test(server); + + if context.stream_output { + let result = execute_streaming_child( + child, allow_reason, - format!("failed to wait for '{}': {}", request.binary, e), + exec_timeout_secs, + server, + OutputRedactionContext { + environment: &redaction_env, + exact_secrets: &exact_output_secrets, + }, + SpawnAuditContext { + caller, + request: &request, + credential_references, + }, + &mut *context.stream_writer, ) - .with_credential_references(credential_references); + .await; + return result; } - }; - if let Some(guard) = process_guard { - guard.complete(); - } - let (stdout_bytes, stderr_bytes) = buffered_output; - let retained_total = Arc::new(AtomicUsize::new(0)); - let stdout = if stdout_bytes.is_empty() { - None - } else { - let redacted = match redact_bounded_buffered_output( - server, - &redaction_env, - &exact_output_secrets, - String::from_utf8_lossy(&stdout_bytes).to_string(), - &retained_total, - ) { - Ok(redacted) => redacted, - Err(error) => { - return ExecuteResult::exec_failed_after_start(allow_reason, error.to_string()) - .with_credential_references(credential_references); + let mut process_guard = Some(server.state.process_tracker.track(child.ownership.clone())); + audit_credential_access(server, caller, &request, &credential_references); + let stdout_pipe = child.stdout.take(); + let stderr_pipe = child.stderr.take(); + let exact_secrets = server + .config + .redact_secrets + .iter() + .chain(exact_output_secrets.iter()) + .map(|secret| secret.as_bytes().to_vec()) + .collect::>(); + let raw_total = Arc::new(AtomicUsize::new(0)); + let stdout_secrets = exact_secrets.clone(); + let stdout_total = raw_total.clone(); + let stdout_reader = async move { + match stdout_pipe { + Some(pipe) => { + read_bounded_redacted_output(pipe, stdout_secrets, stdout_total).await + } + None => Ok(Vec::new()), } }; - Some(redacted) - }; - - let mut stderr = if stderr_bytes.is_empty() { - None - } else { - let redacted = match redact_bounded_buffered_output( - server, - &redaction_env, - &exact_output_secrets, - String::from_utf8_lossy(&stderr_bytes).to_string(), - &retained_total, - ) { - Ok(redacted) => redacted, - Err(error) => { + let stderr_reader = async move { + match stderr_pipe { + Some(pipe) => read_bounded_redacted_output(pipe, exact_secrets, raw_total).await, + None => Ok(Vec::new()), + } + }; + let execution_deadline = + tokio::time::sleep(std::time::Duration::from_secs(exec_timeout_secs.max(1))); + tokio::pin!(execution_deadline); + let buffered_output = if exec_timeout_secs == 0 { + Ok(collect_bounded_output_pair(stdout_reader, stderr_reader).await) + } else { + tokio::select! { + result = collect_bounded_output_pair(stdout_reader, stderr_reader) => Ok(result), + _ = &mut execution_deadline => Err(()), + } + }; + let buffered_output = match buffered_output { + Err(()) => { + terminate_spawned_child(&mut child, &mut process_guard).await; + return ExecuteResult::exec_failed_after_start( + allow_reason, + exec_timeout_reason(exec_timeout_secs), + ) + .with_credential_references(credential_references); + } + Ok(Err(error)) => { + terminate_spawned_child(&mut child, &mut process_guard).await; return ExecuteResult::exec_failed_after_start(allow_reason, error.to_string()) .with_credential_references(credential_references); } + Ok(Ok(output)) => output, }; - Some(redacted) - }; + let wait_result = if exec_timeout_secs == 0 { + Ok(child.wait().await) + } else { + tokio::select! { + result = child.wait() => Ok(result), + _ = &mut execution_deadline => Err(()), + } + }; + let status = match wait_result { + Err(()) => { + terminate_spawned_child(&mut child, &mut process_guard).await; + return ExecuteResult::exec_failed_after_start( + allow_reason, + exec_timeout_reason(exec_timeout_secs), + ) + .with_credential_references(credential_references); + } + Ok(Ok(status)) => status, + Ok(Err(e)) => { + terminate_spawned_child(&mut child, &mut process_guard).await; + return ExecuteResult::exec_failed_after_start( + allow_reason, + format!("failed to wait for '{}': {}", request.binary, e), + ) + .with_credential_references(credential_references); + } + }; + if let Some(guard) = process_guard { + guard.complete(); + } - let mut exit_code = status.code(); - if let Some(mut diagnostics) = - AnsibleInventoryDiagnostics::for_command(&request.binary, &request.args) - { - diagnostics.observe(&String::from_utf8_lossy(&stdout_bytes)); - diagnostics.observe(&String::from_utf8_lossy(&stderr_bytes)); - if diagnostics.normalizes_success_to_failure(exit_code) { - exit_code = Some(1); - stderr = append_accounted_diagnostic( - stderr, - ANSIBLE_INVENTORY_FAILURE_DIAGNOSTIC, + let (stdout_bytes, stderr_bytes) = buffered_output; + let retained_total = Arc::new(AtomicUsize::new(0)); + let stdout = if stdout_bytes.is_empty() { + None + } else { + let redacted = match redact_bounded_buffered_output( + server, + &redaction_env, + &exact_output_secrets, + String::from_utf8_lossy(&stdout_bytes).to_string(), &retained_total, - ); + ) { + Ok(redacted) => redacted, + Err(error) => { + return ExecuteResult::exec_failed_after_start(allow_reason, error.to_string()) + .with_credential_references(credential_references); + } + }; + Some(redacted) + }; + + let mut stderr = if stderr_bytes.is_empty() { + None + } else { + let redacted = match redact_bounded_buffered_output( + server, + &redaction_env, + &exact_output_secrets, + String::from_utf8_lossy(&stderr_bytes).to_string(), + &retained_total, + ) { + Ok(redacted) => redacted, + Err(error) => { + return ExecuteResult::exec_failed_after_start(allow_reason, error.to_string()) + .with_credential_references(credential_references); + } + }; + Some(redacted) + }; + + let mut exit_code = status.code(); + if let Some(mut diagnostics) = + AnsibleInventoryDiagnostics::for_command(&request.binary, &request.args) + { + diagnostics.observe(&String::from_utf8_lossy(&stdout_bytes)); + diagnostics.observe(&String::from_utf8_lossy(&stderr_bytes)); + if diagnostics.normalizes_success_to_failure(exit_code) { + exit_code = Some(1); + stderr = append_accounted_diagnostic( + stderr, + ANSIBLE_INVENTORY_FAILURE_DIAGNOSTIC, + &retained_total, + ); + } } + + ExecuteResult::completed(allow_reason, exit_code, stdout, stderr) + .with_credential_references(credential_references) } + .await; + with_cleanup_outcome( + result, + cleanup_owner.cleanup_id(), + cleanup_owner.cleanup_outcome(), + ) +} - ExecuteResult::completed(allow_reason, exit_code, stdout, stderr) - .with_credential_references(credential_references) +fn with_cleanup_outcome( + result: ExecuteResult, + cleanup_id: u128, + outcome: CleanupOutcome, +) -> ExecuteResult { + let Some(mut failure) = result.execution_failure().cloned() else { + return result; + }; + if let Some(diagnostic) = outcome.diagnostic(cleanup_id) { + failure.message.push_str("; "); + failure.message.push_str(&diagnostic); + return result.with_execution_failure(Some(failure)); + } + result } fn truncate_utf8_bytes(value: &mut String, limit: usize) { @@ -4437,20 +4486,20 @@ async fn cleanup_streaming_failure( child: &mut ManagedChild, process_guard: &mut Option, stream_tasks: &mut StreamTaskCleanup, -) { +) -> CleanupOutcome { stream_tasks.abort_and_join().await; - terminate_spawned_child(child, process_guard).await; + terminate_spawned_child(child, process_guard).await } async fn terminate_spawned_child( child: &mut ManagedChild, process_guard: &mut Option, -) { +) -> CleanupOutcome { if let Some(guard) = process_guard.take() { - guard.terminate_gracefully().await; + guard.terminate_gracefully().await } else { child.ownership.terminate(false); - child.ownership.wait_for_cleanup().await; + child.ownership.wait_for_cleanup().await } } @@ -4862,6 +4911,7 @@ async fn execute_streaming_child( } Ok(Ok(status)) => status, Ok(Err(e)) => { + terminate_spawned_child(&mut child, &mut process_guard).await; return ExecuteResult::exec_failed_after_start( allow_reason, format!("failed to wait for '{}': {}", audit.request.binary, e), diff --git a/src/server/runtime.rs b/src/server/runtime.rs index 3459b82d..506fa421 100644 --- a/src/server/runtime.rs +++ b/src/server/runtime.rs @@ -478,12 +478,65 @@ fn bounded_notify_event(mut event: NotifyEvent) -> NotifyEvent { #[derive(Clone)] pub(super) struct ChildOwnership(Arc>); +impl std::fmt::Debug for ChildOwnership { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("ChildOwnership") + .field("id", &self.id()) + .finish() + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum CleanupOperation { + GroupSignal, + ChildSignal, + Reap, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct CleanupError { + pub operation: CleanupOperation, + pub errno: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum CleanupOutcome { + Reaped, + Pending { errors: Vec }, +} + +impl CleanupOutcome { + pub(super) fn diagnostic(&self, cleanup_id: u128) -> Option { + match self { + Self::Reaped => None, + Self::Pending { errors } => Some(format!( + "cleanup incomplete; the command may still be running; cleanup_id={cleanup_id:032x}; cleanup_errors={errors:?}" + )), + } + } +} + +fn retry_interrupted(mut operation: impl FnMut() -> std::io::Result) -> std::io::Result { + loop { + match operation() { + Err(error) if error.kind() == std::io::ErrorKind::Interrupted => continue, + result => return result, + } + } +} + struct OwnedChildState { child: Option, status: Option, pending_launch: bool, cleanup: ChildCleanup, secret_files: Option, + cleanup_id: u128, + errors: Vec, + cleanup_started: Option, + reported_errors: usize, + reported_pending: bool, #[cfg(test)] signals: usize, } @@ -492,31 +545,72 @@ struct OwnedChildState { enum ChildCleanup { Running, Graceful(Instant), - Forced, + AwaitingExit, } impl OwnedChildState { + fn record_error(&mut self, operation: CleanupOperation, error: &std::io::Error) { + let error = CleanupError { + operation, + errno: error.raw_os_error(), + }; + if !self.errors.contains(&error) { + self.errors.push(error); + } + } + + fn may_signal(&self, operation: CleanupOperation) -> bool { + // A failed wait cannot establish that the PID still belongs to us. + // Permanent signal failures require intervention, not repeated kills. + !self + .errors + .iter() + .any(|error| error.operation == CleanupOperation::Reap || error.operation == operation) + } + fn signal(&mut self, graceful: bool) { - if let Some(child) = self.child.as_mut() { - #[cfg(test)] - { - self.signals += 1; - } + if let Some(pid) = self.child.as_ref().map(std::process::Child::id) { #[cfg(unix)] - unsafe { - libc::kill( - -(child.id() as i32), - if graceful { - libc::SIGTERM + if self.may_signal(CleanupOperation::GroupSignal) { + #[cfg(test)] + { + self.signals += 1; + } + let result = retry_interrupted(|| { + if unsafe { + libc::kill( + -(pid as i32), + if graceful { + libc::SIGTERM + } else { + libc::SIGKILL + }, + ) + } == 0 + { + Ok(()) } else { - libc::SIGKILL - }, - ); + Err(std::io::Error::last_os_error()) + } + }); + if let Err(error) = result { + self.record_error(CleanupOperation::GroupSignal, &error); + } } + #[cfg(not(unix))] + let _ = pid; // Windows uses the retained process handle. On Unix this also // covers a leader that moved itself out of its original group. - if !graceful || !cfg!(unix) { - let _ = child.kill(); + if (!graceful || !cfg!(unix)) && self.may_signal(CleanupOperation::ChildSignal) { + #[cfg(test)] + { + self.signals += 1; + } + if let Err(error) = + retry_interrupted(|| self.child.as_mut().expect("owned child").kill()) + { + self.record_error(CleanupOperation::ChildSignal, &error); + } } } } @@ -525,6 +619,16 @@ impl OwnedChildState { if let Some(status) = self.status { return Ok(Some(status)); } + if let Some(error) = self + .errors + .iter() + .find(|error| error.operation == CleanupOperation::Reap) + { + return Err(error + .errno + .map(std::io::Error::from_raw_os_error) + .unwrap_or_else(|| std::io::Error::other("child reaping failed"))); + } // Keep the leader unreaped until the final group signal. Its PID // cannot be reused while the grace deadline is outstanding. if matches!(self.cleanup, ChildCleanup::Graceful(_)) { @@ -533,7 +637,13 @@ impl OwnedChildState { let Some(child) = self.child.as_mut() else { return Ok(None); }; - let status = child.try_wait()?; + let status = match retry_interrupted(|| child.try_wait()) { + Ok(status) => status, + Err(error) => { + self.record_error(CleanupOperation::Reap, &error); + return Err(error); + } + }; if let Some(status) = status { self.status = Some(status); self.child = None; @@ -546,16 +656,40 @@ impl OwnedChildState { if let ChildCleanup::Graceful(deadline) = self.cleanup { if Instant::now() >= deadline { self.signal(false); - self.cleanup = ChildCleanup::Forced; + self.cleanup = ChildCleanup::AwaitingExit; } } - if matches!(self.cleanup, ChildCleanup::Forced) { - // Errors retain ownership for a later attempt; a foreground - // timeout never discards an unreaped child or its secret lease. + if matches!(self.cleanup, ChildCleanup::AwaitingExit) { + // try_wait records failures and retains ownership and the lease. let _ = self.try_wait(); } !self.pending_launch && self.child.is_none() } + + fn outcome(&self) -> CleanupOutcome { + if !self.pending_launch && self.child.is_none() { + CleanupOutcome::Reaped + } else { + CleanupOutcome::Pending { + errors: self.errors.clone(), + } + } + } + + fn take_diagnostic(&mut self) -> Option<(u128, CleanupOutcome)> { + let new_errors = self.errors.len() > self.reported_errors; + let overdue = !self.reported_pending + && self.child.is_some() + && self + .cleanup_started + .is_some_and(|started| started.elapsed() >= Duration::from_secs(3)); + if !new_errors && !overdue { + return None; + } + self.reported_errors = self.errors.len(); + self.reported_pending |= overdue; + Some((self.cleanup_id, self.outcome())) + } } fn register_child_cleanup(state: Arc>) -> std::io::Result<()> { @@ -589,10 +723,24 @@ fn register_child_cleanup(state: Arc>) -> std::io::Result } children.extend(rx.try_iter()); children.retain(|child| { - !child + let mut state = child .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .cleanup_tick() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let complete = state.cleanup_tick(); + let diagnostic = state.take_diagnostic(); + drop(state); + if let Some((cleanup_id, outcome)) = diagnostic { + if let Some(message) = outcome.diagnostic(cleanup_id) { + tracing::error!(cleanup_id = %format_args!("{cleanup_id:032x}"), "{message}"); + let _ = guard::audit::emit_global( + &guard::audit::AuditEvent::new(guard::audit::AuditKind::ExecFailed) + .field("phase", "cleanup") + .field("cleanup_id", format!("{cleanup_id:032x}")) + .reason(message) + ); + } + } + !complete }); } })?; @@ -620,6 +768,11 @@ impl ChildOwnership { pending_launch: true, cleanup: ChildCleanup::Running, secret_files, + cleanup_id: rand::random(), + errors: Vec::new(), + cleanup_started: None, + reported_errors: 0, + reported_pending: false, #[cfg(test)] signals: 0, })); @@ -645,6 +798,20 @@ impl ChildOwnership { .map(std::process::Child::id) } + pub(super) fn cleanup_id(&self) -> u128 { + self.0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .cleanup_id + } + + pub(super) fn cleanup_outcome(&self) -> CleanupOutcome { + self.0 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .outcome() + } + pub(super) fn take_stdout(&self) -> Option { self.0 .lock() @@ -682,24 +849,26 @@ impl ChildOwnership { state.secret_files = None; return; } + state.cleanup_started.get_or_insert_with(Instant::now); if graceful && matches!(state.cleanup, ChildCleanup::Running) { state.signal(true); state.cleanup = if cfg!(unix) { ChildCleanup::Graceful(Instant::now() + Duration::from_secs(2)) } else { - ChildCleanup::Forced + ChildCleanup::AwaitingExit }; - } else if !graceful { + } else if !graceful && !matches!(state.cleanup, ChildCleanup::AwaitingExit) { state.signal(false); - state.cleanup = ChildCleanup::Forced; + state.cleanup = ChildCleanup::AwaitingExit; } } - pub(super) async fn wait_for_cleanup(&self) { + pub(super) async fn wait_for_cleanup(&self) -> CleanupOutcome { let deadline = tokio::time::Instant::now() + Duration::from_secs(3); while self.id().is_some() && tokio::time::Instant::now() < deadline { tokio::time::sleep(Duration::from_millis(10)).await; } + self.cleanup_outcome() } } @@ -715,8 +884,9 @@ impl ProcessTracker { self.active .lock() .expect("process tracker poisoned") - .insert(generation, child); + .insert(generation, child.clone()); ProcessGuard { + child, generation, tracker: self.clone(), armed: true, @@ -754,6 +924,7 @@ impl Drop for ShutdownGuard { } pub(super) struct ProcessGuard { + child: ChildOwnership, generation: u64, tracker: ProcessTracker, armed: bool, @@ -765,12 +936,12 @@ impl ProcessGuard { self.armed = false; } - pub(super) async fn terminate_gracefully(mut self) { + pub(super) async fn terminate_gracefully(mut self) -> CleanupOutcome { if let Some(child) = self.tracker.take(self.generation) { child.terminate(true); - self.armed = false; - child.wait_for_cleanup().await; } + self.armed = false; + self.child.wait_for_cleanup().await } } @@ -784,6 +955,68 @@ impl Drop for ProcessGuard { } } +#[cfg(all(test, target_os = "linux"))] +pub(super) fn deny_cleanup_signals_for_test() { + // Install only in a fresh fixture subprocess, before its cleanup worker. + let deny = libc::SECCOMP_RET_ERRNO | libc::EPERM as u32; + let instructions = [ + libc::sock_filter { + code: 0x20, + jt: 0, + jf: 0, + k: 0, + }, + libc::sock_filter { + code: 0x15, + jt: 3, + jf: 0, + k: libc::SYS_kill as u32, + }, + libc::sock_filter { + code: 0x15, + jt: 2, + jf: 0, + k: libc::SYS_tkill as u32, + }, + libc::sock_filter { + code: 0x15, + jt: 1, + jf: 0, + k: libc::SYS_tgkill as u32, + }, + libc::sock_filter { + code: 0x15, + jt: 0, + jf: 1, + k: libc::SYS_pidfd_send_signal as u32, + }, + libc::sock_filter { + code: 0x06, + jt: 0, + jf: 0, + k: deny, + }, + libc::sock_filter { + code: 0x06, + jt: 0, + jf: 0, + k: libc::SECCOMP_RET_ALLOW, + }, + ]; + let program = libc::sock_fprog { + len: instructions.len() as u16, + filter: instructions.as_ptr() as *mut _, + }; + assert_eq!( + unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) }, + 0 + ); + assert_eq!( + unsafe { libc::prctl(libc::PR_SET_SECCOMP, libc::SECCOMP_MODE_FILTER, &program) }, + 0 + ); +} + #[cfg(test)] mod tests { use super::*; @@ -940,7 +1173,7 @@ mod tests { .await .expect("shell installed its SIGTERM trap"); - guard.terminate_gracefully().await; + assert_eq!(guard.terminate_gracefully().await, CleanupOutcome::Reaped); assert!(child.try_wait().unwrap().is_some(), "child must be reaped"); assert_eq!(std::fs::read_to_string(marker).unwrap(), "term"); } @@ -994,13 +1227,14 @@ mod tests { child.terminate(true); child.0.lock().unwrap().cleanup = ChildCleanup::Graceful(Instant::now() + Duration::from_secs(60)); - tokio::time::timeout(Duration::from_secs(4), child.wait_for_cleanup()) + let outcome = tokio::time::timeout(Duration::from_secs(4), child.wait_for_cleanup()) .await .unwrap(); + assert_eq!(outcome, CleanupOutcome::Pending { errors: Vec::new() }); let retained_child = child.id().is_some(); let retained_lease = bindings[0].1.exists(); child.terminate(false); - child.wait_for_cleanup().await; + assert_eq!(child.wait_for_cleanup().await, CleanupOutcome::Reaped); assert!(retained_child, "foreground timeout must retain ownership"); assert!( retained_lease, @@ -1048,6 +1282,137 @@ mod tests { child.terminate(false); } + #[test] + fn cleanup_retries_only_interrupted_operations() { + let mut calls = 0; + let result = retry_interrupted(|| { + calls += 1; + if calls == 1 { + Err(std::io::Error::from(std::io::ErrorKind::Interrupted)) + } else { + Ok(()) + } + }); + assert!(result.is_ok()); + assert_eq!(calls, 2); + calls = 0; + let result: std::io::Result<()> = retry_interrupted(|| { + calls += 1; + Err(std::io::Error::from(std::io::ErrorKind::PermissionDenied)) + }); + assert!(result.is_err()); + assert_eq!(calls, 1); + } + + #[cfg(target_os = "linux")] + #[test] + fn cleanup_denied_signals_report_pending_and_reap_natural_exit() { + if std::env::var_os("GUARD_TEST_CLEANUP_DENIED").is_none() { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "server::runtime::tests::cleanup_denied_signals_report_pending_and_reap_natural_exit", "--nocapture"]) + .env("GUARD_TEST_CLEANUP_DENIED", "1") + .output().unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stderr) + .contains("cleanup incomplete; the command may still be running")); + return; + } + use std::os::unix::process::CommandExt; + deny_cleanup_signals_for_test(); + tracing_subscriber::fmt() + .with_ansi(false) + .with_writer(std::io::stderr) + .with_max_level(tracing::Level::ERROR) + .try_init() + .unwrap(); + let directory = tempfile::tempdir().unwrap(); + let (lease, bindings) = super::super::secure_fs::SecretFileLease::create( + directory.path(), + &[("FIXTURE_FILE".into(), "fixture-value".into())], + ) + .unwrap(); + let child = ChildOwnership::prepare(Some(lease)).unwrap(); + let mut command = std::process::Command::new("sleep"); + command.arg("5").process_group(0); + child.adopt(command.spawn().unwrap()); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + child.terminate(false); + let outcome = runtime.block_on(child.wait_for_cleanup()); + let CleanupOutcome::Pending { errors } = outcome else { + panic!("termination was denied"); + }; + assert!(errors.contains(&CleanupError { + operation: CleanupOperation::GroupSignal, + errno: Some(libc::EPERM) + })); + assert!(errors.contains(&CleanupError { + operation: CleanupOperation::ChildSignal, + errno: Some(libc::EPERM) + })); + assert!(child.id().is_some()); + assert!(child.try_wait().unwrap().is_none()); + assert!(bindings[0].1.exists()); + let attempts = child.0.lock().unwrap().signals; + child.terminate(false); + assert_eq!(child.0.lock().unwrap().signals, attempts); + assert_eq!( + runtime.block_on(child.wait_for_cleanup()), + CleanupOutcome::Reaped + ); + assert!(!bindings[0].1.exists()); + child.terminate(false); + child.terminate(true); + assert_eq!(child.0.lock().unwrap().signals, attempts); + } + + #[cfg(unix)] + #[test] + fn cleanup_reap_error_is_retained_and_prevents_later_signals() { + use std::os::unix::process::CommandExt; + let directory = tempfile::tempdir().unwrap(); + let (lease, bindings) = super::super::secure_fs::SecretFileLease::create( + directory.path(), + &[("FIXTURE_FILE".into(), "fixture-value".into())], + ) + .unwrap(); + let child = ChildOwnership::prepare(Some(lease)).unwrap(); + let mut command = std::process::Command::new("true"); + command.process_group(0); + child.adopt(command.spawn().unwrap()); + let pid = child.id().unwrap(); + // The fixture acts as a competing reaper for this one owned child. + let mut status = 0; + assert_eq!( + unsafe { libc::waitpid(pid as i32, &mut status, 0) }, + pid as i32 + ); + assert_eq!( + child.try_wait().unwrap_err().raw_os_error(), + Some(libc::ECHILD) + ); + child.terminate(false); + let CleanupOutcome::Pending { errors } = child.cleanup_outcome() else { + panic!("reaping ownership is unknown"); + }; + assert!(errors.contains(&CleanupError { + operation: CleanupOperation::Reap, + errno: Some(libc::ECHILD) + })); + assert_eq!(child.0.lock().unwrap().signals, 0); + assert!(bindings[0].1.exists()); + // The fixture's waitpid above establishes exit; release its test state. + let mut state = child.0.lock().unwrap(); + state.child = None; + state.secret_files = None; + } + #[test] fn command_handler_admission_is_fair_per_principal() { let admission = CommandAdmission::new(CommandAdmissionConfig { diff --git a/src/server/tests/exec_policy.rs b/src/server/tests/exec_policy.rs index bad4f8cf..69bd5fa6 100644 --- a/src/server/tests/exec_policy.rs +++ b/src/server/tests/exec_policy.rs @@ -2868,6 +2868,83 @@ async fn launch_failure_is_recorded_as_an_allowed_session_interaction_after_rest assert_eq!(interaction.execution_failure.as_ref(), Some(&failure)); } +#[cfg(target_os = "linux")] +#[test] +fn cleanup_pending_preserves_execution_failure_policy_and_correlated_audit() { + if std::env::var_os("GUARD_TEST_EXEC_CLEANUP_DENIED").is_none() { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", "server::tests::exec_policy::cleanup_pending_preserves_execution_failure_policy_and_correlated_audit", "--nocapture"]) + .env("GUARD_TEST_EXEC_CLEANUP_DENIED", "1") + .output().unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout) + .contains("pending cleanup verified in both output modes")); + return; + } + crate::server::runtime::deny_cleanup_signals_for_test(); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + runtime.block_on(async { + for streaming in [false, true] { + let (mut cfg, _) = make_test_config(); + cfg.config.exec_timeout_secs = 1; + let (audit_directory, _audit) = super::attach_test_audit_log(&mut cfg); + let caller = CallerIdentity::Unix { + uid: unsafe { libc::geteuid() }, + }; + let request = basic_request("sleep", vec!["6".into()]); + let mut stream = Vec::new(); + let result = exec_after_approval_with_secret_authority( + &mut RequestContext { + server: &cfg, + caller: &caller, + depth: 0, + stream_output: streaming, + stream_writer: &mut stream, + }, + request.clone(), + "fixture policy approval".into(), + None, + ) + .await; + assert!(result.policy_allowed()); + assert_eq!(result.policy_reason(), "fixture policy approval"); + let failure = result.execution_failure().unwrap(); + assert_eq!(failure.started, Some(true)); + assert_eq!(failure.stage, guard::wire::ExecutionStage::Unknown); + assert!(failure.message.starts_with("exec_timeout:")); + assert!(failure + .message + .contains("cleanup incomplete; the command may still be running")); + assert!(failure.message.contains("cleanup_id=")); + assert!(failure.message.contains("errno: Some(1)")); + cfg.log_audit_exec_failed(&caller, None, &request.binary, &request.args, &result); + let audit = + std::fs::read_to_string(audit_directory.path().join("audit.jsonl")).unwrap(); + let event = audit + .lines() + .map(|line| { + serde_json::from_str::(line) + .unwrap() + .event + }) + .find(|event| event.kind == guard::audit::AuditKind::ExecFailed) + .unwrap(); + assert_eq!(event.execution_failure.as_ref(), Some(failure)); + assert!(event.policy.unwrap().allowed); + // The denied signal leaves this bounded fixture to exit naturally. + tokio::time::sleep(std::time::Duration::from_secs(3)).await; + } + }); + println!("pending cleanup verified in both output modes"); +} + #[cfg(unix)] #[tokio::test] async fn default_service_execution_does_not_forward_ssh_auth_sock() { From 30a7926b74950dcb770639cf7f585fbe27f32ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Thu, 10 Sep 2026 01:01:20 +0000 Subject: [PATCH 7/8] fix: scope Unix child cleanup helpers to Unix targets --- src/server/execute.rs | 2 +- src/server/runtime.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server/execute.rs b/src/server/execute.rs index c494f7db..298b82e6 100644 --- a/src/server/execute.rs +++ b/src/server/execute.rs @@ -3220,7 +3220,7 @@ impl ManagedChild { } } - #[cfg(test)] + #[cfg(all(test, unix))] async fn wait_with_output(mut self) -> std::io::Result { let stdout = self.stdout.take(); let stderr = self.stderr.take(); diff --git a/src/server/runtime.rs b/src/server/runtime.rs index 506fa421..97e9ff48 100644 --- a/src/server/runtime.rs +++ b/src/server/runtime.rs @@ -489,6 +489,7 @@ impl std::fmt::Debug for ChildOwnership { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum CleanupOperation { + #[cfg(unix)] GroupSignal, ChildSignal, Reap, From fe6e29ed6d4f7e409b53dc2da9a10fa0e280d12b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B6rg=C3=A6sis?= Date: Thu, 10 Sep 2026 01:09:00 +0000 Subject: [PATCH 8/8] test: avoid logging complete execution outcomes --- src/server/tests/exec_policy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/tests/exec_policy.rs b/src/server/tests/exec_policy.rs index 69bd5fa6..a0bcc532 100644 --- a/src/server/tests/exec_policy.rs +++ b/src/server/tests/exec_policy.rs @@ -2739,7 +2739,7 @@ async fn launch_failures_are_typed_in_buffered_and_streaming_execution() { )); let failure = result.execution_failure().expect("typed launch failure"); assert_eq!(failure.started, Some(false)); - assert_eq!(failure.stage, stage, "{failure:?}"); + assert_eq!(failure.stage, stage); assert_eq!(failure.errno, Some(errno)); assert!(!failure.message.contains(cwd.to_str().unwrap())); assert!(!failure.message.contains("unexpected")); @@ -2793,7 +2793,7 @@ async fn launch_preserves_relative_file_access_in_both_output_modes() { assert_eq!(stdout.as_deref(), Some("cwd-content")); } } - other => panic!("expected completed relative-file execution: {other:?}"), + _ => panic!("expected completed relative-file execution"), } } }