Fix egress carve-out bypass and secret leakage in the Kubernetes provider#99
Conversation
…rule `ip_block` only applied the `egress_excluded_cidrs` carve-out when a rule was exactly `0.0.0.0/0`. An allowlist entry like `10.0.0.0/8` fully contains the default excluded ranges (10.42.0.0/16, 10.43.0.0/16) and, if it happened to cover 169.254.0.0/16 too, would expose the cloud metadata endpoint (169.254.169.254) to every sandbox with no way to carve it out. Now every allow-rule CIDR is checked for overlap against the excluded set using ipnet (already resolved in Cargo.lock via hyper-util, so no new transitive dependency): an excluded CIDR that's a subset of the allowed CIDR is rendered as an `except` entry; one that fully contains (or equals) the allowed CIDR causes the rule to be rejected outright, since k8s NetworkPolicy requires `except` to be a strict subset of `cidr` and there would be nothing left to allow. CIDR blocks are power-of-two aligned so containment is the only overlap case to handle. Also: `with_egress_excluded_cidrs` used to replace the default excluded set outright, so an operator override could silently drop the 169.254.0.0/16 metadata carve-out. It now merges (dedup'd) with the defaults instead; `with_egress_excluded_cidrs_replace` / `--egress-excluded-cidrs-replace` is the explicit opt-out for operators who deliberately need to replace the set for a non-k3s cluster. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
Two secret-exposure fixes in the Kubernetes provider: 1. `exec_args` rendered job env vars as `env KEY=VALUE` positional arguments to `kubectl exec`, so any secret passed as a command env var (API tokens, etc.) was visible in the guest process table (/proc/*/cmdline) to every other process in the pod, and in the worker host's own argv. Values are now piped to the guest over stdin instead: when the request carries env vars, `exec_args` wires up `kubectl exec -i` plus a `bash -c` wrapper that reads NUL-delimited `KEY=VALUE` pairs from stdin and `export`s them before `exec`ing the real command (NUL is safe because env var values can't contain it, unlike newlines). `run_kubectl_command_with_stdin` pipes the payload and closes stdin so the wrapper's read loop terminates. The dry-run provider's `exec_handoff`, which already only logs env *keys*, is unchanged. 2. The VNC password was injected via `secretKeyRef` as a plain container env var, while the SSH authorized-keys secret was correctly mounted as a read-only file -- pod env vars are visible to anything that can read the pod spec/status via the Kubernetes API (kubectl describe, any ServiceAccount with `get pods`), not just the process itself. It's now mounted read-only at /run/sandboxwich/vnc/vnc-password, exposed to the guest as SANDBOXWICH_VNC_PASSWORD_FILE, mirroring the SSH secret's handling. Updated the guest entrypoint script (which we control) to read the password from that file, falling back to a random per-start password as before when unset. Extended provider.rs unit tests to assert no secret values ever appear on rendered argv, cover the with/without-cwd wrapper variants, and check the new VNC file mount/env var. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
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}
| if needs_env { | ||
| args.push("-i".to_string()); | ||
| } | ||
| args.extend([ | ||
| "exec".to_string(), | ||
| self.pod_name(sandbox_id), | ||
| "--".to_string(), | ||
| ]); |
There was a problem hiding this comment.
🔴 Interactive stdin flag placed before the kubectl subcommand, so environment variables are silently dropped during command execution
The stdin flag is appended before the subcommand word (args.push("-i".to_string()) at crates/sandboxwich-worker/src/provider.rs:1399), producing kubectl -n <ns> -i exec <pod> -- bash -c ... instead of kubectl -n <ns> exec -i <pod> -- ..., so kubectl never connects stdin to the container and the environment-variable payload is silently lost.
Impact: Every sandbox command that carries environment variables will either error out or silently execute without any of the requested env vars set.
kubectl flag parsing requires subcommand-specific flags after the subcommand name
kubectl uses cobra for CLI parsing. The -i (--stdin) flag is defined on the exec subcommand, not as a global/persistent flag. Cobra does not parse subcommand-specific flags that appear before the subcommand name. The kubectl_base_args() method (crates/sandboxwich-worker/src/provider.rs:1298-1307) returns ["-n", "<ns>"] (plus optional --context), then line 1399 pushes "-i", and only then does line 1401-1404 push ["exec", pod_name, "--"].
The resulting argv is: kubectl -n <ns> -i exec <pod> -- bash -c <script> ...
But kubectl expects: kubectl -n <ns> exec -i <pod> -- bash -c <script> ...
Without -i being recognized, kubectl won't pipe stdin to the container. The run_kubectl_command_with_stdin function (crates/sandboxwich-worker/src/provider.rs:1529) writes the NUL-delimited env payload to the child's stdin, but kubectl ignores it because it doesn't know stdin should be forwarded. The bash wrapper's while read loop inside the container sees immediate EOF and exports nothing.
| if needs_env { | |
| args.push("-i".to_string()); | |
| } | |
| args.extend([ | |
| "exec".to_string(), | |
| self.pod_name(sandbox_id), | |
| "--".to_string(), | |
| ]); | |
| if needs_env { | |
| args.push("-i".to_string()); | |
| } | |
| args.extend([ | |
| "exec".to_string(), | |
| self.pod_name(sandbox_id), | |
| "--".to_string(), | |
| ]); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if needs_env { | ||
| args.extend([ | ||
| "bash".to_string(), | ||
| "-c".to_string(), | ||
| EXEC_ENV_WRAPPER_SCRIPT.to_string(), | ||
| "sandboxwich-exec".to_string(), | ||
| ]); | ||
| if let Some(cwd) = &request.cwd { | ||
| args.extend([ | ||
| "sh".to_string(), | ||
| "-lc".to_string(), | ||
| "cd \"$1\" && shift && exec \"$@\"".to_string(), | ||
| "sandboxwich-cwd".to_string(), | ||
| cwd.clone(), | ||
| ]); | ||
| args.extend(request.argv.clone()); | ||
| return args; | ||
| args.push("1".to_string()); | ||
| args.push(cwd.clone()); | ||
| } else { | ||
| args.push("0".to_string()); | ||
| } | ||
| args.extend(request.argv.clone()); | ||
| return args; |
There was a problem hiding this comment.
🔍 Login shell profile sourcing lost when both env vars and cwd are set
The old code path for commands with both env vars and a cwd used sh -lc (login shell), which sources ~/.profile and other login scripts before running the command. The new env-wrapper path at crates/sandboxwich-worker/src/provider.rs:1407-1421 uses bash -c (non-login shell) for all env-carrying commands, including those with a cwd. This means commands that previously relied on login-shell profile setup (e.g. PATH additions from ~/.profile) will no longer have those settings when env vars are also present.
The cwd-only path (no env vars) at crates/sandboxwich-worker/src/provider.rs:1424-1433 still uses sh -lc, so only the combined env+cwd case changed behavior. This asymmetry could surprise users: the same command with a cwd behaves differently depending on whether env vars are also passed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const EXEC_ENV_WRAPPER_SCRIPT: &str = concat!( | ||
| "while IFS= read -r -d '' kv; do ", | ||
| "case \"$kv\" in *=*) export \"${kv%%=*}\"=\"${kv#*=}\" ;; esac; ", | ||
| "done; ", | ||
| "has_cwd=\"$1\"; shift; ", | ||
| "if [ \"$has_cwd\" = \"1\" ]; then cd \"$1\" || exit 1; shift; fi; ", | ||
| "exec \"$@\"" | ||
| ); |
There was a problem hiding this comment.
📝 Info: Guest container must have bash installed for env-var-carrying commands
The new exec wrapper at crates/sandboxwich-worker/src/provider.rs:1465-1472 uses bash -c with bash-specific features (read -d '' for NUL-delimited input, ${kv%%=*} and ${kv#*=} parameter expansion). The cwd-only and plain paths still use sh or direct execution. This introduces a hard dependency on bash being available in the guest container image for any command that carries environment variables. The current ubuntu-dev image includes bash, but custom or minimal guest images (e.g. Alpine with only busybox ash) would fail silently or with a confusing error when env vars are passed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn normalize_cidrs(cidrs: Vec<String>) -> Vec<String> { | ||
| let mut seen = std::collections::BTreeSet::new(); | ||
| cidrs | ||
| .into_iter() | ||
| .map(|cidr| cidr.trim().to_string()) | ||
| .filter(|cidr| !cidr.is_empty() && seen.insert(cidr.clone())) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
📝 Info: CIDR dedup uses string equality, not semantic network equality
The normalize_cidrs method at crates/sandboxwich-worker/src/provider.rs:253-260 and the merge check at line 226 both use string comparison for deduplication. Semantically equivalent CIDRs with different string representations (e.g. 10.42.0.0/16 vs 10.42.0.000/16, or ::ffff:10.42.0.0/112 vs 10.42.0.0/16) would not be deduped and could appear as duplicate except entries in the NetworkPolicy. In practice this is unlikely since CIDRs are typically written in canonical form, and Kubernetes would accept duplicate except entries without error, so this is cosmetic rather than functional.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # crates/sandboxwich-worker/src/provider.rs
| if let Some(secret_name) = &self.vnc_password_secret { | ||
| env.push(json!({ | ||
| "name": "SANDBOXWICH_VNC_PASSWORD", | ||
| "valueFrom": { | ||
| "secretKeyRef": { | ||
| "name": secret_name, | ||
| "key": "vnc-password" | ||
| } | ||
| // Mounted as a read-only file rather than a `secretKeyRef` env | ||
| // var, mirroring the SSH authorized-keys handling above: | ||
| // container env vars are visible to anything that can read | ||
| // this pod's spec/status via the Kubernetes API (e.g. | ||
| // `kubectl describe pod`, or any ServiceAccount with `get pods` | ||
| // in this namespace), not just the process itself, whereas a | ||
| // mounted file is only readable by whoever can exec into the | ||
| // container or read the volume directly. | ||
| volume_mounts.push(json!({ | ||
| "name": "vnc-password", | ||
| "mountPath": "/run/sandboxwich/vnc", | ||
| "readOnly": true | ||
| })); | ||
| volumes.push(json!({ | ||
| "name": "vnc-password", | ||
| "secret": { | ||
| "secretName": secret_name, | ||
| "items": [{ | ||
| "key": "vnc-password", | ||
| "path": "vnc-password" | ||
| }] | ||
| } | ||
| })); | ||
| env.push(json!({ | ||
| "name": "SANDBOXWICH_VNC_PASSWORD_FILE", | ||
| "value": "/run/sandboxwich/vnc/vnc-password" | ||
| })); |
There was a problem hiding this comment.
🔍 VNC password delivery mechanism changed from env var to file mount — a breaking contract change for existing deployments
The VNC password delivery changed from a secretKeyRef env var (SANDBOXWICH_VNC_PASSWORD) to a volume-mounted file (SANDBOXWICH_VNC_PASSWORD_FILE at /run/sandboxwich/vnc/vnc-password). This is a coordinated change across the provider (crates/sandboxwich-worker/src/provider.rs:512-539) and the guest entrypoint (deploy/runtime/ubuntu-dev/sandboxwich-entrypoint.sh:52-56). Both sides are updated consistently. However, any existing deployment using --vnc-password-secret will need to rebuild and redeploy the guest runtime image in lockstep with the worker binary — if the worker is upgraded but the guest image still reads SANDBOXWICH_VNC_PASSWORD (the old env var), the password will silently fall through to the random-generation path, and operators won't know their configured password is being ignored.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn with_egress_excluded_cidrs_replace(mut self, cidrs: Vec<String>) -> Self { | ||
| let cidrs = Self::normalize_cidrs(cidrs); | ||
| if !cidrs.is_empty() { | ||
| self.egress_excluded_cidrs = cidrs; | ||
| } | ||
| self |
There was a problem hiding this comment.
🔍 Empty-list replace is a documented no-op, but CLI flag interaction may surprise operators
When --egress-excluded-cidrs-replace is set but no --egress-excluded-cidr values are provided, with_egress_excluded_cidrs_replace at crates/sandboxwich-worker/src/provider.rs:251-256 receives an empty vec and silently keeps the defaults (the if !cidrs.is_empty() guard). The doc comment says this is intentional for parity, but an operator who sets --egress-excluded-cidrs-replace expecting to clear all exclusions will get the opposite of what they intended — the defaults remain in place with no warning. The CLI help text at crates/sandboxwich-worker/src/main.rs:258-269 doesn't mention this edge case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn excepted_cidrs_for(&self, cidr: &str) -> anyhow::Result<Vec<String>> { | ||
| if self.egress_excluded_cidrs.is_empty() { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let allowed = | ||
| IpNet::from_str(cidr).with_context(|| format!("invalid egress allow CIDR {cidr}"))?; | ||
| let mut except = Vec::new(); | ||
| for excluded_str in &self.egress_excluded_cidrs { | ||
| let excluded = IpNet::from_str(excluded_str) | ||
| .with_context(|| format!("invalid egress excluded CIDR {excluded_str}"))?; | ||
| if excluded.contains(&allowed) { | ||
| bail!( | ||
| "egress allow rule {cidr} is entirely covered by excluded control-plane/metadata CIDR {excluded_str}; refusing to render a NetworkPolicy that would either expose it or leave nothing allowed" | ||
| ); | ||
| } | ||
| if allowed.contains(&excluded) { | ||
| except.push(excluded_str.clone()); | ||
| } | ||
| } | ||
| Ok(except) |
There was a problem hiding this comment.
📝 Info: CIDR containment logic correctly handles IPv4/IPv6 disjointness via ipnet
The excepted_cidrs_for method at crates/sandboxwich-worker/src/provider.rs:666-685 uses IpNet::contains(&IpNet) from the ipnet crate. IpNet is an enum over V4/V6, and cross-family comparisons always return false (an IPv4 network can never contain an IPv6 network and vice versa). This means the default IPv4 excluded CIDRs (169.254.0.0/16, 10.42.0.0/16, 10.43.0.0/16) will never produce spurious except entries for an IPv6 allow rule, which is the correct behavior confirmed by the test at line 2875. No bug here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const EXEC_ENV_WRAPPER_SCRIPT: &str = concat!( | ||
| "while IFS= read -r -d '' kv; do ", | ||
| "case \"$kv\" in *=*) export \"${kv%%=*}\"=\"${kv#*=}\" ;; esac; ", | ||
| "done; ", | ||
| "has_cwd=\"$1\"; shift; ", | ||
| "if [ \"$has_cwd\" = \"1\" ]; then cd \"$1\" || exit 1; shift; fi; ", | ||
| "exec \"$@\"" | ||
| ); |
There was a problem hiding this comment.
📝 Info: Wrapper script's read loop correctly handles the trailing-NUL protocol
The exec_stdin_payload at crates/sandboxwich-worker/src/provider.rs:1517-1529 appends a NUL byte after every KEY=VALUE pair, including the last one. The bash wrapper's while IFS= read -r -d '' kv at line 1539 reads up to each NUL delimiter. After the final NUL, read encounters EOF and returns non-zero, cleanly ending the loop with all entries processed. If the payload were missing the trailing NUL on the last entry, read would still populate $kv but return non-zero, causing the last entry to be silently dropped — but the current implementation always appends the trailing NUL, so this edge case doesn't arise.
Was this helpful? React with 👍 or 👎 to provide feedback.
Review finding on PR #109: the raw worker-scoped token leaked through provider handle metadata -- worker_token_secret_manifest (stringData holding the raw token) was embedded in the "manifests" block of both dry-run provision metadata and dry-run fork metadata. The API persists handle metadata verbatim into the sandboxes table's provider_metadata column and returns it to tenant clients on sandbox reads, and apply mode builds its handle from the dry-run one, so the token was headed for plaintext storage in the control-plane DB and the tenant API -- the exact exposure class GH-64/GH-99 were closing. Fix: the raw token now exists only in the manifest sets piped to `kubectl apply` stdin (provision_manifests/fork_manifests). Every serialized path -- provision metadata, fork metadata -- carries worker_token_secret_manifest_redacted instead, which preserves the Secret's kind/name/labels but replaces stringData with "[redacted]" (WORKER_TOKEN_REDACTED). smoke_plan was audited and already excludes the token Secret entirely (pod manifests reference it by name only, and the smoke CLI paths never configure worker credentials); that invariant is now documented and regression-tested. Tests: flipped the metadata assertion from raw to redacted, added whole-metadata serialization asserts (!contains(token)) for both provision and fork handles, and a smoke-plan serialization regression test. The applied-manifest tests still assert the raw token IS present in the kubectl-stdin set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
1 participant
Summary
Two independent security fixes in
crates/sandboxwich-worker/src/provider.rs, each landed as its own commit.1. Egress allowlist CIDRs could bypass the metadata/control-plane carve-out
Threat model: the provider carves the control-plane/link-local/cluster-service CIDRs (default
169.254.0.0/16,10.42.0.0/16,10.43.0.0/16) out of egress rules via NetworkPolicyexcept, specifically so a sandbox can never reach the cloud metadata endpoint (169.254.169.254, a classic path to instance-credential theft) or the apiserver/cluster-service ranges. The carve-out was only applied when a rule was exactly0.0.0.0/0. An operator-configured allowlist entry like10.0.0.0/8fully contains the default excluded ranges but got noexceptat all — a sandbox on that allowlist could reach10.42.0.0/16/10.43.0.0/16directly, and if an operator's allowed range ever covered169.254.0.0/16, metadata credential theft.Fix: every allow-rule CIDR is now checked for overlap against the excluded set using
ipnet(already resolved inCargo.lockviahyper-util, so this adds no new transitive dependency, just promotes it to a direct one). CIDR blocks are power-of-two aligned, so two CIDRs can only ever be identical, nested, or disjoint — there's no partial-overlap case. An excluded CIDR that's a subset of the allowed CIDR becomes anexceptentry; one that fully contains (or equals) the allowed CIDR causes the rule to be rejected outright with a clear error, since k8s NetworkPolicy requiresexceptto be a strict subset ofcidrand there would be nothing left to allow.Also fixed:
with_egress_excluded_cidrsused to replace the default excluded set outright, so an operator passing a custom list could silently drop the169.254.0.0/16metadata carve-out. It now merges (dedup'd) with the defaults.with_egress_excluded_cidrs_replace/--egress-excluded-cidrs-replaceis the explicit, documented opt-out for operators who deliberately need to replace the set (e.g. a non-k3s cluster where the k3s-shaped defaults are meaningless).2. Secrets leaked via process args and pod spec
Threat model (a):
exec_argsrendered job env vars asenv KEY=VALUEpositional arguments tokubectl exec. Any secret passed as a command env var (API tokens, etc.) was visible in the guest's process table (/proc/*/cmdline) to every other process in the pod, and in the worker host's own argv to anything withps/process-listing access there.Fix: values are now piped to the guest over stdin instead of argv. When the request carries env vars,
exec_argswires upkubectl exec -iplus a smallbash -cwrapper that reads NUL-delimitedKEY=VALUEpairs from stdin andexports them beforeexecing the real command (NUL is a safe delimiter because POSIX env var values can never contain one, unlike newlines — verified against values containing spaces).run_kubectl_command_with_stdinpipes the payload and closes stdin so the wrapper's read loop terminates. The dry-run provider'sexec_handoff, which already only logs env keys (not values) in its diagnostic output, is unchanged.Threat model (b): the VNC password was injected via
secretKeyRefas a plain container env var, while the SSH authorized-keys secret was correctly mounted as a read-only file. Pod env vars are visible to anything that can read the pod spec/status via the Kubernetes API (kubectl describe pod, any ServiceAccount withget podsin the namespace) — a materially wider blast radius than a mounted file, which requires exec access into the container or direct volume access.Fix: the VNC password secret is now mounted read-only at
/run/sandboxwich/vnc/vnc-password, exposed to the guest asSANDBOXWICH_VNC_PASSWORD_FILE, mirroring the SSH secret's handling. The guest entrypoint script (deploy/runtime/ubuntu-dev/sandboxwich-entrypoint.sh, which this repo controls) was updated to read the password from that file, falling back to a random per-container-start password as before when unset — verified withshellcheckand a manual dry run of the read loop.Deviations from the task brief
deploy/runtime/ubuntu-dev/), so I updated it in the same PR rather than treating this as a "document and scope as low-risk" case. No env-var back-compat path was kept; the file is the only source now (with the existing random-password fallback preserved).ipnetas a direct dependency ofsandboxwich-worker(and to[workspace.dependencies]) for CIDR containment math. It was already resolved inCargo.lockat the same version viahyper-util, soCargo.lockonly gained one line (promoting it from transitive to direct) — no new crate, no version change.with_egress_excluded_cidrs_replace/--egress-excluded-cidrs-replaceescape hatch that wasn't in the original API, per the task's suggestion to consider one if warranted.Test plan
cargo fmt --checkcleancargo clippy --workspace --all-targetsclean (workspace lints deny warnings)cargo test --workspace— all 88 tests pass, including 20 new/updated tests covering:10.0.0.0/8allowlist carve-out, disjoint CIDRs left untouched, rejection when an excluded range fully covers (or equals) the allowed CIDR, IPv6 cases, operator-CIDR merge vs. explicit replace, exec argv never containing secret values (assert on both raw value and key name), the with/without/cwd-combined wrapper variants, the stdin payload's NUL-delimited encoding, and the VNC secret being mounted as a file with no env var.shellcheckclean on the updated entrypoint script; manually exercised thebash -cenv-wrapper script end-to-end (including a value containing spaces) to confirm export +cd+execbehavior before wiring it intoexec_args.🤖 Generated with Claude Code