-
Notifications
You must be signed in to change notification settings - Fork 8
fix(sandbox,server): isolate REPL host ops and gate /api/repl behind Cedar (ARN-166) #342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,6 +227,70 @@ pub(crate) async fn require_policy_auth( | |
| None | ||
| } | ||
|
|
||
| /// Authorize a REPL sandbox execution request against Cedar policies. | ||
| /// | ||
| /// `POST /api/repl` runs agent-supplied code that can invoke every non-host | ||
| /// `temper.*` method, so executing the REPL at all is a privileged capability | ||
| /// and must be authorized — not open to any caller who can reach the port. | ||
| /// | ||
| /// Returns `Some(response)` (403 + `AuthorizationDenied`, with the recorded | ||
| /// decision id) if denied, `None` if allowed. Admin principals bypass, matching | ||
| /// [`require_policy_auth`]. | ||
| /// | ||
| /// Enforcement strength is bounded by the identity source: until the principal | ||
| /// is a resolved credential rather than a self-asserted `x-temper-*` header | ||
| /// (ARN-167), this gate trusts the claimed principal kind. The host-op | ||
| /// isolation in `temper-sandbox` — not this gate — is the unconditional stop | ||
| /// for the arbitrary file-read / RCE the endpoint otherwise exposed. | ||
|
Comment on lines
+243
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The doc comment cites ARN-167 as the pending fix for the Admin/principal bypass, but per the PR description, ARN-167 is the TemperPaw sibling and ARN-170 is the kernel-specific tracker ("kernel Class A: header-trusted admin + fail-open"). A developer looking up ARN-167 to understand the kernel residual will land on the TemperPaw issue, not the open kernel work. Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/temper-server/src/api/mod.rs
Line: 243-244
Comment:
**Wrong ARN reference in doc comment**
The doc comment cites ARN-167 as the pending fix for the Admin/principal bypass, but per the PR description, ARN-167 is the TemperPaw sibling and ARN-170 is the kernel-specific tracker ("kernel Class A: header-trusted admin + fail-open"). A developer looking up ARN-167 to understand the kernel residual will land on the TemperPaw issue, not the open kernel work.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| pub(crate) async fn require_repl_auth( | ||
| state: &ServerState, | ||
| headers: &HeaderMap, | ||
| tenant: &str, | ||
| ) -> Option<axum::response::Response> { | ||
| let security_ctx = security_context_from_headers(headers, None, None, None); | ||
| if matches!(security_ctx.principal.kind, PrincipalKind::Admin) { | ||
| return None; | ||
| } | ||
|
Comment on lines
+250
to
+253
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/temper-server/src/api/mod.rs
Line: 250-253
Comment:
**Admin bypass is fully header-spoofable — Layer A gate provides no real barrier until ARN-170 lands**
`security_context_from_headers` reads `PrincipalKind` from the unverified `X-Temper-Principal-Kind` header. Any caller who can reach the port can self-assert `X-Temper-Principal-Kind: admin` and bypass the Cedar check entirely, reaching the REPL sandbox with access to all non-host `temper.*` methods. This is the system-wide header-trust issue tracked under ARN-170 (and ARN-167 for TemperPaw) and is consistent with how every other Cedar gate in the codebase (`require_policy_auth`, `require_observe_auth`) behaves — the PR description acknowledges this explicitly. Layer B (`allow_host_ops: false`) is the durable stop for the worst-case attack regardless, but Layer A as written is effectively a no-op against a motivated unauthenticated caller until the credential-resolution path lands.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, and this is the load-bearing point. This is the systemic Class A header-trust issue (ARN-170), not a repl-specific hole — fixing it locally in |
||
| if let Err(denial) = state.authorize_with_context( | ||
| &security_ctx, | ||
| "execute_repl", | ||
| "Sandbox", | ||
| &std::collections::BTreeMap::new(), | ||
| tenant, | ||
| ) { | ||
|
Comment on lines
+254
to
+260
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/temper-server/src/api/mod.rs
Line: 254-260
Comment:
**`resource_attrs` empty in Cedar call, `{tenant}` in denial record**
The `authorize_with_context` call passes `&std::collections::BTreeMap::new()` while `record_authz_denial` records `{"tenant": tenant}`. This means any Cedar policy that inspects `resource.tenant` on a `Sandbox` resource (e.g., restricting REPL access to a specific tenant) would never match — Cedar sees an attribute-less resource. The PR description notes this as a known P2 with no security impact, and `require_policy_auth` has the same inconsistency, so this is at least consistent. Populating the Cedar call with the same attrs as the denial record would let future policies use `resource.tenant`.
How can I resolve this? If you propose a fix, please make it concise. |
||
| let reason = denial.to_string(); | ||
| let pd = record_authz_denial( | ||
| state, | ||
| DenialInput { | ||
| tenant, | ||
| security_ctx: &security_ctx, | ||
| agent_id_override: None, | ||
| action: "execute_repl", | ||
| resource_type: "Sandbox", | ||
| resource_id: tenant, | ||
| resource_attrs: serde_json::json!({"tenant": tenant}), | ||
| reason: &reason, | ||
| module_name: None, | ||
| from_status: None, | ||
| }, | ||
| ) | ||
| .await; | ||
| return Some( | ||
| ( | ||
| StatusCode::FORBIDDEN, | ||
| axum::Json(serde_json::json!({ | ||
| "error": { | ||
| "code": "AuthorizationDenied", | ||
| "message": format!("{reason} Decision {}", pd.id), | ||
| } | ||
| })), | ||
| ) | ||
| .into_response(), | ||
| ); | ||
| } | ||
| None | ||
| } | ||
|
|
||
| /// Cedar policy-management gate as an axum extractor. | ||
| /// | ||
| /// Runs [`require_policy_auth`] against the `{tenant}` path parameter before | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| //! `/api/repl` authorization gate (ARN-166). | ||
| //! | ||
| //! `POST /api/repl` executes agent-supplied code that can drive every non-host | ||
| //! `temper.*` method, so executing it is a privileged capability gated behind | ||
| //! Cedar (`execute_repl` on `Sandbox`). These tests pin the gate against a real, | ||
| //! deny-by-default tenant policy set: | ||
| //! | ||
| //! - an agent with no `execute_repl` grant is denied (403) before any code runs; | ||
| //! - an admin bypasses the gate (matching `require_policy_auth`) and runs; | ||
| //! - an agent that IS granted `execute_repl` runs — proving the gate is a real | ||
| //! Cedar evaluation keyed on the action, not a blanket agent-deny. | ||
| //! | ||
| //! The route is mounted only under the `observe` feature — the same feature | ||
| //! production (`temper-cli`) builds with — so this file compiles to nothing | ||
| //! without it. Run with: `cargo test -p temper-server --test repl_auth_gate | ||
| //! --features observe`. | ||
| #![cfg(feature = "observe")] | ||
|
|
||
| use axum::body::Body; | ||
| use axum::http::{Request, StatusCode}; | ||
| use temper_runtime::ActorSystem; | ||
| use temper_server::{ServerState, SpecRegistry, build_router}; | ||
| use temper_spec::csdl::parse_csdl; | ||
| use tower::ServiceExt; | ||
|
|
||
| const CSDL_XML: &str = include_str!("../../../test-fixtures/specs/model.csdl.xml"); | ||
| const ORDER_IOA: &str = include_str!("../../../test-fixtures/specs/order.ioa.toml"); | ||
|
|
||
| /// Grants `execute_repl` to admins only — an agent is not covered, so Cedar | ||
| /// default-denies it. (Admins bypass the gate before Cedar anyway; the grant | ||
| /// keeps the policy realistic rather than empty.) | ||
| const ADMIN_ONLY_REPL: &str = | ||
| r#"permit(principal is Admin, action == Action::"execute_repl", resource);"#; | ||
|
|
||
| /// Grants `execute_repl` to agents — the authorized-agent surface. | ||
| const AGENT_REPL: &str = | ||
| r#"permit(principal is Agent, action == Action::"execute_repl", resource);"#; | ||
|
|
||
| /// Build server state whose `default` tenant has `policy` loaded, so the gate is | ||
| /// evaluated against real Cedar policies rather than the permissive fallback | ||
| /// that `from_registry` installs (which would allow everything and make the | ||
| /// deny assertion vacuous). | ||
| fn state_with_policy(policy: &str) -> ServerState { | ||
| let csdl = parse_csdl(CSDL_XML).expect("CSDL should parse"); | ||
| let mut registry = SpecRegistry::new(); | ||
| registry.register_tenant( | ||
| "default", | ||
| csdl, | ||
| CSDL_XML.to_string(), | ||
| &[("Order", ORDER_IOA)], | ||
| ); | ||
| let state = ServerState::from_registry(ActorSystem::new("repl-auth-gate-test"), registry); | ||
| state | ||
| .authz | ||
| .reload_tenant_policies("default", policy) | ||
| .expect("tenant policy should load"); | ||
| state | ||
| } | ||
|
|
||
| fn repl_request(principal_kind: &str, code: &str) -> Request<Body> { | ||
| Request::post("/api/repl") | ||
| .header("Content-Type", "application/json") | ||
| .header("X-Temper-Principal-Kind", principal_kind) | ||
| .header("X-Temper-Principal-Id", "arn166-test-principal") | ||
| .body(Body::from(serde_json::json!({ "code": code }).to_string())) | ||
| .unwrap() | ||
| } | ||
|
|
||
| /// An agent with no `execute_repl` grant is denied with 403 before any code | ||
| /// runs — the tenant policy grants the action to admins only, so Cedar | ||
| /// default-denies the agent. | ||
| #[tokio::test] | ||
| async fn repl_denied_for_unauthorized_agent() { | ||
| let app = build_router(state_with_policy(ADMIN_ONLY_REPL)); | ||
| let response = app | ||
| .oneshot(repl_request("agent", "result = 1 + 1")) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.status(), StatusCode::FORBIDDEN); | ||
| let body = axum::body::to_bytes(response.into_body(), 1024 * 1024) | ||
| .await | ||
| .unwrap(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
| assert_eq!(json["error"]["code"], "AuthorizationDenied"); | ||
| } | ||
|
|
||
| /// Admin bypasses the gate (matching `require_policy_auth`) even against the | ||
| /// deny-by-default policy set, so the REPL still runs for an authorized surface | ||
| /// — the gate closes the hole without removing the capability. A clean run | ||
| /// returns 200 with `error: null`; a sandbox execution error would surface as a | ||
| /// non-null `error` at 200 (see `handle_repl`), so asserting both status and a | ||
| /// null `error` pins that the code actually executed rather than silently | ||
| /// erroring. | ||
| #[tokio::test] | ||
| async fn repl_allowed_for_admin() { | ||
| let app = build_router(state_with_policy(ADMIN_ONLY_REPL)); | ||
| let response = app | ||
| .oneshot(repl_request("admin", "result = 1 + 1")) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| let body = axum::body::to_bytes(response.into_body(), 1024 * 1024) | ||
| .await | ||
| .unwrap(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
| assert_eq!(json["error"], serde_json::Value::Null); | ||
| } | ||
|
|
||
| /// An agent that IS granted `execute_repl` passes the gate and runs — proving | ||
| /// the gate is a real Cedar evaluation keyed on the action, not a hardcoded | ||
| /// agent-deny. Same success shape as the admin case: 200 with `error: null`. | ||
| #[tokio::test] | ||
| async fn repl_allowed_for_authorized_agent() { | ||
| let app = build_router(state_with_policy(AGENT_REPL)); | ||
| let response = app | ||
| .oneshot(repl_request("agent", "result = 1 + 1")) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(response.status(), StatusCode::OK); | ||
| let body = axum::body::to_bytes(response.into_body(), 1024 * 1024) | ||
| .await | ||
| .unwrap(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
| assert_eq!(json["error"], serde_json::Value::Null); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upload_wasm_allowed_with_host_ops_reaches_filesystemassertserr.contains("failed to read")to prove the filesystem read was attempted (gate did not fire). Ifdispatch_wasmever changes its IO error format (e.g., to"io error"or"cannot read"), this assertion will fail on a refactoring-only change and will need updating alongside the error message, creating a maintenance coupling that is easy to overlook.Prompt To Fix With AI