Fix guest agent tenant-token exposure: worker-scoped credentials (GH-64)#96
Conversation
…H-64) Threat model: the guest agent inside a sandbox authenticates to the control plane, but until now it only ever carried the same tenant-wide bearer token the CLI uses, so a compromised sandbox could act as the whole tenant. This lays the data-model groundwork for a worker-scoped credential: - migrations/20260709000100_worker_tokens.sql adds a nullable, uniquely indexed workers.token_hash column (SHA-256 of the worker's minted token, never the raw value), following the tenant_boundaries.sql migration's alter-table pattern. - WorkerResponse gains an optional worker_token field, populated only on POST /workers/register's response and never returned again. No behavior changes yet: nothing mints, hashes, or verifies tokens against this column until the next commit.
Fixes #64: the guest agent running inside an untrusted sandbox authenticated with the same tenant-wide bearer token used by the CLI, so any compromised sandbox could act as the whole tenant (claim/forge any lease, post guest-health for any sandbox, etc). - register_worker now mints a high-entropy token (generate_worker_token, prefixed "sbw_wtok_" so the auth middleware can route it without an extra DB round trip for every other request), stores only its SHA-256 hash (hash_worker_token), and returns the raw token once in the response. - auth_and_tenant resolves a "sbw_wtok_"-prefixed bearer token by hashing it and looking it up against workers.token_hash (resolve_worker_token), producing a TenantContext carrying both tenant_id and worker_id. A token with this prefix that fails to resolve is always unauthorized -- it never falls through to the tenant-token checks, which would let a rejected worker token be retried as a tenant-wide one. Unlike the small in-memory tenant/ operator token lists (compared with constant_time_eq), the lookup key here is a cryptographic hash, so the security property comes from SHA-256 preimage resistance rather than timing-safe comparison -- a plain indexed equality lookup is the correct, standard pattern for hashed credentials at DB scale. - ensure_worker_scope/ensure_lease_worker_scope/ensure_sandbox_worker_scope enforce that a worker-scoped token may only act on its own worker id, its own leases (by lease.worker_id), and sandboxes it has actually provisioned or forked (worker_owns_sandbox, derived from completed provision/fork lease history -- sandboxes carry no persistent "owning worker" column, and completed-lease history is the only source of truth for which worker's guest environment a sandbox actually runs in). A tenant-wide token is rejected outright on these checks with 401, not merely denied access to another worker's resources. - Applied to every guest-facing route: claim_lease, renew_lease, append_lease_output, complete_lease, fail_lease, and the POST side of guest-health. The GET side of guest-health, CLI/admin routes (register, heartbeat, reconcile-runtime-resources, job creation, etc.) are unchanged and keep accepting tenant-wide tokens. Adds sha2 as a direct dependency (already vendored transitively) and enables uuid's v4 feature for token generation.
GH-64) The `run` subcommand (register + work loop in one process, what the checked-in worker Deployment actually runs) now captures the worker_token returned by registration and builds a second API client from it, using that -- not the tenant token passed in via SANDBOXWICH_API_TOKEN/--api-token -- for the rest of the process's lifetime: heartbeat, lease claim/renew/complete/fail. The tenant token is used for exactly one call, the initial POST /workers/register, since a worker has no worker id (and therefore no worker-scoped token) before that responds. `work-loop`/`claim`/`renew`/`complete`/`fail`/`heartbeat` invoked directly (not via `run`) are unchanged: they already take whatever token the operator passes via --api-token/SANDBOXWICH_API_TOKEN, so an operator now needs to pass the worker-scoped token returned by a prior `register` call for those to succeed against the guest-facing routes. Documented on the Cli::api_token field and in worker.yaml. Deviation from the task's "inject it into sandbox env" ask: no provider (KubernetesDryRunProvider/KubernetesApplyProvider) currently injects SANDBOXWICH_API_TOKEN/WORKER_ID into a guest pod's env at all -- only SANDBOXWICH_WORKSPACE/SSH_PORT/AUTHORIZED_KEYS_FILE/ VNC_PASSWORD are wired today, and the guest agent's Daemon subcommand that reads those env vars is a code path this provider doesn't drive. Wiring real secret-backed propagation into pod manifests (mirroring the ssh_authorized_keys_secret/vnc_password_secret pattern) is a distinct, sizable feature and left as a follow-up; this commit ensures that whenever/however that wiring lands, the only credential it can carry into a guest is this worker-scoped token, and the API enforces its scope regardless of injection mechanism.
Adds worker_scoped_tokens_enforce_guest_route_boundaries: registers two workers, gives each a sandbox it actually provisioned, and asserts worker B's token cannot claim on worker A's behalf, or renew/append-output/complete/fail worker A's lease, or post guest-health for sandbox A (all 404, matching ensure_tenant's existing convention of not confirming/denying another principal's resource exists); that a tenant-wide token is rejected outright (401) on every one of those same routes, including acting on its own tenant's worker/lease/sandbox -- not just cross-worker access; and that the happy path (the rightful worker completing its own lease and reporting its own sandbox's guest health) still works end to end. Updates every existing test that exercises lease claim/renew/complete/fail/output or guest-health (the full sqlite/ postgres lifecycle contract, provision/rollback/retry/expiry/prompt/ snapshot-fork helpers) to authenticate those specific calls with a worker_client(&worker) built from the new worker_token instead of the tenant client, since those routes now reject tenant-wide tokens. assert_guest_health_and_ssh_key_lifecycle's call in run_contract also moves to after a real provision lease completes, since guest-health now requires the worker-scoped token whose lease-history proves it owns the sandbox.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let sql = format!( | ||
| "select j.kind, j.payload | ||
| from job_leases jl | ||
| join jobs j on j.id = jl.job_id | ||
| where jl.worker_id = {} and jl.status = 'completed' | ||
| and j.kind in ('provision_sandbox', 'fork_sandbox')", | ||
| db.placeholder(1) | ||
| ); | ||
| let rows = sqlx::query(&sql) | ||
| .bind(worker_id.to_string()) | ||
| .fetch_all(&db.pool) | ||
| .await?; | ||
| let sandbox_id_str = sandbox_id.to_string(); | ||
| for row in rows { | ||
| let kind: String = row.try_get("kind")?; | ||
| let payload_raw: String = row.try_get("payload")?; | ||
| let payload: serde_json::Value = serde_json::from_str(&payload_raw)?; | ||
| let field = if kind == "fork_sandbox" { | ||
| "childSandboxId" | ||
| } else { | ||
| "sandboxId" | ||
| }; | ||
| if payload.get(field).and_then(serde_json::Value::as_str) == Some(sandbox_id_str.as_str()) { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| Ok(false) |
There was a problem hiding this comment.
📝 Info: Sandbox ownership query fetches all completed leases instead of filtering by sandbox in SQL
The worker_owns_sandbox function at crates/sandboxwich-api/src/main.rs:1683-1715 fetches ALL completed provision/fork job leases for a given worker, then iterates through each row in Rust to parse the JSON payload and check if the sandbox ID matches. For a long-running worker that has provisioned many sandboxes, this could return a large number of rows. The sandbox ID filter could be pushed into the SQL query itself (e.g., using JSON extraction functions or a LIKE clause on the payload column) to reduce the result set. This isn't a correctness issue but could become a performance bottleneck in production.
Was this helpful? React with 👍 or 👎 to provide feedback.
| })) | ||
| } | ||
|
|
||
| async fn heartbeat_worker( |
There was a problem hiding this comment.
🔍 Heartbeat route intentionally does not enforce worker-scoped auth
The heartbeat_worker API handler at crates/sandboxwich-api/src/main.rs:2837-2898 uses ensure_worker_tenant (tenant-level check) but does NOT call ensure_worker_scope (worker-scoped token check). Meanwhile, the worker-side Run command at crates/sandboxwich-worker/src/main.rs:591-594 switches to the worker-scoped client for heartbeats. This works because the worker-scoped token resolves to a valid TenantContext with a tenant_id, and ensure_worker_tenant only checks tenant_id. The asymmetry is intentional — heartbeat is not listed as a "guest-facing route" in the PR's design — but it means a tenant-wide token can still heartbeat on behalf of any worker in that tenant, which may or may not be the desired security posture.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// For `register`/`run`: a tenant-wide token, used only to authenticate | ||
| /// the initial `POST /workers/register` call. `run` then mints and | ||
| /// switches to a worker-scoped token (GH-64) for everything after | ||
| /// registration, so this value is never reused for lease/guest-health | ||
| /// calls in that path. For every other subcommand (`work-loop`, `claim`, | ||
| /// `renew`, `complete`, `fail`, `heartbeat`), pass the worker-scoped | ||
| /// token returned by `register` here instead -- those routes reject | ||
| /// tenant-wide tokens. |
There was a problem hiding this comment.
📝 Info: Standalone CLI subcommands require manual worker token management
The Run command at crates/sandboxwich-worker/src/main.rs:550-605 automatically mints and switches to the worker-scoped token after registration. However, the standalone WorkOnce (line 607), WorkLoop (line 625), Claim (line 514), Renew (line 518), Complete (line 528), and Fail (line 547) subcommands all use the original client built from --api-token. The CLI doc comment at lines 26-33 correctly documents that users must pass the worker-scoped token via SANDBOXWICH_API_TOKEN for these subcommands, but this is a usability concern — a user who copies their tenant token will get auth failures on guest-facing routes with no obvious indication that a different token type is needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn generate_worker_token() -> String { | ||
| format!( | ||
| "{WORKER_TOKEN_PREFIX}{}{}", | ||
| Uuid::new_v4().simple(), | ||
| Uuid::new_v4().simple() | ||
| ) | ||
| } |
There was a problem hiding this comment.
📝 Info: Worker token entropy relies on two v4 UUIDs rather than a dedicated CSPRNG
The generate_worker_token function at crates/sandboxwich-api/src/main.rs:1323-1329 concatenates two Uuid::new_v4() values (each 122 random bits) for ~244 bits of entropy. This is sufficient for a bearer token, and Uuid::new_v4() uses the OS CSPRNG internally, so the entropy source is sound. The approach is slightly unconventional compared to using rand::thread_rng().gen::<[u8; 32]>() directly, but avoids adding rand as a dependency. Not a bug, just a design choice worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
…st-tokens # Conflicts: # crates/sandboxwich-api/tests/http_contract.rs
| # GH-64: this tenant-wide credential authenticates only the | ||
| # `run` subcommand's initial `POST /workers/register` call. | ||
| # Immediately after registering, the worker process mints and | ||
| # switches internally to a worker-scoped token bound to its own | ||
| # worker id, and uses that (never this value) for every | ||
| # guest-facing call it makes afterward (heartbeat, lease | ||
| # claim/renew/complete/fail/output) -- those routes reject | ||
| # tenant-wide tokens outright. |
There was a problem hiding this comment.
📝 Info: Deploy YAML comment overstates which routes reject tenant-wide tokens
The comment at deploy/kubernetes/worker.yaml:113-114 says heartbeat routes "reject tenant-wide tokens outright," but the API's heartbeat_worker handler at crates/sandboxwich-api/src/main.rs:2860 only calls ensure_worker_tenant (tenant-level check), not ensure_worker_scope. Heartbeat accepts both tenant-wide and worker-scoped tokens. The code is correct — worker-scoped tokens carry a valid tenant_id so ensure_worker_tenant passes — but the comment is misleading. The same inaccuracy appears in the worker CLI doc comment at crates/sandboxwich-worker/src/main.rs:29.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // GH-64: guest-facing route -- only the worker that provisioned/forked | ||
| // this sandbox may report its guest health; tenant-wide tokens are | ||
| // rejected. (The read side, `get_guest_health`, stays on tenant auth -- | ||
| // CLI/dashboard callers need to read it too.) | ||
| ensure_sandbox_worker_scope(&state.db, sandbox_id, &ctx).await?; |
There was a problem hiding this comment.
🔍 Guest agent inside sandbox must receive the worker-scoped token to post health updates
The update_guest_health route now calls ensure_sandbox_worker_scope (crates/sandboxwich-api/src/main.rs:3179-3183), which requires a worker-scoped token. The guest agent (crates/sandboxwich-agent/src/main.rs:975) calls this endpoint using whatever SANDBOXWICH_API_TOKEN it was given. The diff doesn't show how the worker injects the worker-scoped token into the sandbox environment during provisioning — this presumably happens in the provider/kubectl layer. If the agent still receives a tenant-wide token (or no token), all guest-health POST calls will fail with 401. This is likely handled outside the diff (in the provisioning templates), but worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub worker_token: Option<String>, |
There was a problem hiding this comment.
📝 Info: WorkerResponse struct change is backward-compatible via serde attributes
The new worker_token field on WorkerResponse (crates/sandboxwich-core/src/lib.rs:1163-1164) uses #[serde(default, skip_serializing_if = "Option::is_none")], so existing serialized responses without this field will deserialize cleanly (defaulting to None), and responses where it's None won't include the key in JSON output. This preserves wire compatibility with older clients that don't expect the field.
Was this helpful? React with 👍 or 👎 to provide feedback.
…in race test PR #96's worker-scoped guest tokens (merged into main) require lease claim/renew/complete/fail routes to use a worker-scoped token instead of the tenant token. assert_job_completion_does_not_resurrect_concurrently_archived_sandbox predates that change and was still using the tenant client for its claim/complete calls, which now 401. Switch it to a worker_client for the registered race_worker, matching every other lease call site in this file.
1 participant
Threat model
The guest agent running inside a sandbox (untrusted code) authenticated to the control plane with the same tenant-wide bearer token the CLI and admin tooling use for everything. The API only ever mapped a token to a tenant (
auth_and_tenant→TenantContext { tenant_id }), never to a specific worker or sandbox. Any code executing inside a sandbox could read that token out of its own environment and then act as the entire tenant: claim other workers' leases (exfiltrating other jobs' argv/env), forge lease completions/failures, post guest-health for sandboxes it has nothing to do with, etc. Sandbox compromise was equivalent to tenant-wide control-plane compromise, which defeats the point of a sandbox.Design
1. Mint a worker-scoped credential at registration.
POST /workers/registernow generates a high-entropy token (sbw_wtok_<64 hex chars>), stores only its SHA-256 hash in a new nullable, uniquely-indexedworkers.token_hashcolumn (migration20260709000100_worker_tokens.sql, following thealter tablepattern intenant_boundaries.sql), and returns the raw token once in the response (WorkerResponse.worker_token). It is never returned again.2. The API resolves that token to
(tenant_id, worker_id), not just a tenant.auth_and_tenantroutes any bearer token with thesbw_wtok_prefix to a hash lookup (resolve_worker_token) instead of the static tenant/shared-token lists, producing aTenantContextthat now carries an optionalworker_id. A prefixed token that fails to resolve is unconditionally401— it never falls through to tenant-token checks (which would let a rejected worker-token attempt be retried as a tenant-wide one).3. Guest-facing routes reject tenant-wide tokens and enforce ownership.
ensure_worker_scope/ensure_lease_worker_scope/ensure_sandbox_worker_scoperequirectx.worker_idto beSomeand to match the resource being acted on:claim_lease— token's worker id must equal the{worker_id}path param.renew_lease/complete_lease/fail_lease/append_lease_output— token's worker id must equallease.worker_id.update_guest_health(POST only) — token's worker id must have completed aprovision_sandbox/fork_sandboxlease for that sandbox. Sandboxes carry no persistent "owning worker" column, so this is derived from completed-lease history (worker_owns_sandbox) rather than a new denormalized field — avoiding asandboxesschema change and matching what's actually the source of truth (whichever worker's guest environment the sandbox lives in).Cross-worker attempts return
404(mirroring the existingensure_tenantconvention of not confirming/denying another principal's resource exists); a missing worker scope (i.e. a tenant-wide token) returns401.GET /sandboxes/{id}/guest-healthand every CLI/admin route (register, heartbeat, reconcile-runtime-resources, job creation, snapshot cleanup, etc.) are unchanged and keep accepting tenant-wide tokens.4. The worker process propagates the new token.
sandboxwich-worker run(register + work loop in one process — what the checked-in Deployment actually runs) capturesworker_tokenfrom the registration response and builds a second API client from it, using that for everything afterward (heartbeat, claim, renew, complete, fail) instead of the tenant token passed in viaSANDBOXWICH_API_TOKEN. The tenant token is now used for exactly one call: the initialPOST /workers/register.Compatibility
token_hash = NULLand will get401on every guest-facing route until they next re-register (which mints a token). Since workers are ephemeral processes that re-register on every restart/redeploy, this self-heals on the next rolling deploy; it's called out here in case anyone runs a long-lived worker process across this deploy without a restart.work-loop/claim/renew/complete/fail/heartbeatinvoked directly (not viarun): unchanged code, but an operator must now pass the worker-scoped token from a priorregistercall via--api-token/SANDBOXWICH_API_TOKENfor those calls to succeed against guest-facing routes. Documented onCli::api_tokenand inworker.yaml.Deviations from the task's spec
sandboxesschema change. Rather than adding anassigned_worker_idcolumn (and updating the raw-SQLinsert_sandbox_sqltest helper andassert_database_rejects_invalid_typed_values), sandbox ownership for guest-health is derived from completedprovision_sandbox/fork_sandboxlease history (worker_owns_sandbox), which is already the actual source of truth and requires no migration.constant_time_eqnot reused for worker-token verification. Tenant/operator tokens are a small, static, in-memory list compared viaconstant_time_eq. Worker tokens are dynamic, per-worker DB rows; the lookup key is the SHA-256 hash itself, via an indexedWHERE token_hash = ?. The security property here comes from hash preimage resistance, not timing-safe comparison — this is the standard pattern for hashed API keys at DB scale, not a shortcut.KubernetesDryRunProvidernorKubernetesApplyProvidercurrently injectsSANDBOXWICH_API_TOKEN/SANDBOXWICH_WORKER_IDinto a guest pod's env at all today — onlySANDBOXWICH_WORKSPACE/SSH_PORT/AUTHORIZED_KEYS_FILE/VNC_PASSWORDare wired, and none of those follow the plain-env-var pattern for secrets (VNC password is secretKeyRef-only). Building that out (mirroringssh_authorized_keys_secret/vnc_password_secret) is a distinct, sizable feature — it needs the worker to create/manage a Secret per worker, thread a new field throughSandboxProvisionSpecand both providers' manifest rendering, and update their existing manifest-assertion tests. Left as a follow-up. This PR's job is to guarantee that whenever/however that wiring lands, the only credential it can carry into a guest is a worker-scoped token, and the API enforces its scope regardless of how it got there.sandboxwich-agent's actual HTTP calls: it only ever hits/workers/{id}/leases/claim,/leases/{id}/{fail,complete,output}, and/sandboxes/{id}/guest-health. File read/write are local CLI subcommands (no HTTP), and command execution goes through the worker'sexec_handoff(the worker execs into the sandbox, not the sandbox polling the API). No file/command HTTP routes needed scoping beyond what's above.Test plan
worker_scoped_tokens_enforce_guest_route_boundaries(new): worker B's token cannot claim/renew/append-output/complete/fail worker A's lease or post guest-health for worker A's sandbox (404s); a tenant-wide token is rejected outright on every one of those routes (401s), including against its own tenant's resources; the rightful worker completing its own lease and reporting its own sandbox's health still works end to end.cargo fmt --all -- --checkclean.cargo clippy --workspace --all-targets -- -D warningsclean.cargo test --workspace: 78 tests passing (14 agent, 6 api-unit, 10 http-contract including the new test, 3 bench, 4 cli, 2 core, 39 worker). Postgres-gated lifecycle test skips (noSANDBOXWICH_TEST_POSTGRES_URLin this environment) per repo convention.Fixes #64
🤖 Generated with Claude Code