fix(security): validate snapshot_tag in create_sandbox + token guards

Turns the two failing tests from the previous commit green and adds
defense-in-depth around adjacent footguns.

## create_sandbox path traversal (HIGH, post-auth)

`POST /v1/sandboxes` previously fed `req.snapshot_tag` straight into
`snapshot_root.join(...)` without validation. `delete_snapshot` and
`branch_sandbox` both call `is_safe_tag`; this handler was an
asymmetric oversight. CI on the prior commit captured the failure
(404 from file-existence oracle, instead of 400 input validation):

  test create_sandbox_rejects_unsafe_snapshot_tag_traversal ... FAILED
  test create_sandbox_rejects_unsafe_snapshot_tag_chars ... FAILED

Fix: call `is_safe_tag(&req.snapshot_tag)` immediately after the
`n`-range checks, before any filesystem op.

Defense in depth: `read_snapshot_volumes` now also checks `is_safe_tag`
on its tag argument, so a future caller forgetting to validate (or a
state.json reconstructed from a pre-validation registry row) can't
make the daemon dereference an attacker-chosen path.

## K8s placeholder bearer token (HIGH)

The shipped `packaging/k8s/forkd-controller.yaml` ships
`token: REPLACE_ME_WITH_32_BYTES_BASE64`. Users who forget to sed it
get a daemon protected only by a publicly-known token. Fix: daemon
refuses to start if the token begins with `REPLACE_ME` / `CHANGE_ME`
or is under 16 bytes. Validation is extracted to a pure `validate_token`
helper with 5 unit tests.

## boot_wait_secs DoS (LOW)

`POST /v1/snapshots` accepted any u64 for `boot_wait_secs`. A hostile
caller could pass u64::MAX and tie up a daemon worker. Cap at 60 s
(well above largest measured boot wait in our recipes).

## Test plan

- All 4 new tests pass; 21 prior tests untouched
- New `validate_token` unit tests cover empty / REPLACE_ME /
  CHANGE_ME / short / realistic
- K8s manifest comment now documents the start-up refusal so the
  failure mode is discoverable

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: WaylandYang

crates/forkd-controller/src/http.rs

@@ -130,6 +130,13 @@ async fn create_snapshot(
         return bad_request(&format!("rootfs not found: {}", rootfs.display()));
     }
 
+    // Cap boot_wait_secs so a hostile caller can't tie up a daemon worker
+    // for u64::MAX seconds. 60 s is well above the largest measured boot
+    // time in our recipes (postgres-fixture warms up in ~10 s).
+    if req.boot_wait_secs > 60 {
+        return bad_request("boot_wait_secs must be ≤ 60");
+    }
+
     let snap_dir = s.snapshot_root.join(&req.tag);
     if snap_dir.join("vmstate").exists() {
         return bad_request(&format!(
@@ -257,6 +264,15 @@ async fn create_sandbox(
     if req.n > 1000 {
         return bad_request("n must be ≤ 1000 (sanity cap)");
     }
+    // Validate snapshot_tag BEFORE any filesystem op. Without this, a tag
+    // like `../../etc` makes `snapshot_root.join(tag)` traverse outside
+    // snapshot_root (Rust's Path::join doesn't normalise `..`), and the
+    // unvalidated tag would also persist into SandboxInfo.snapshot_tag,
+    // later feeding `read_snapshot_volumes` and letting an attacker pick
+    // the JSON file the daemon parses for grandchild volume specs.
+    if !is_safe_tag(&req.snapshot_tag) {
+        return bad_request("snapshot_tag must be 1-64 chars, ASCII alnum or dash/underscore");
+    }
 
     // Snapshot can come either from the daemon's registry (created via
     // future POST /v1/snapshots) or from the on-disk XDG location (created
@@ -546,6 +562,13 @@ fn read_snapshot_volumes(
     snapshot_root: &std::path::Path,
     tag: &str,
 ) -> anyhow::Result<Vec<forkd_vmm::VolumeSpec>> {
+    // Defense in depth: every caller is expected to have validated `tag` via
+    // `is_safe_tag` before persisting it, but if a future caller forgets, or
+    // a registry row gets reconstructed from an older state.json written
+    // before tag validation existed, refuse to dereference the join.
+    if !is_safe_tag(tag) {
+        anyhow::bail!("refusing to read snapshot with unsafe tag (defense in depth)");
+    }
     let path = snapshot_root.join(tag).join("snapshot.json");
     if !path.exists() {
         return Ok(Vec::new());
@@ -899,6 +922,27 @@ mod tests {
         assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
     }
 
+    #[tokio::test]
+    async fn create_snapshot_rejects_boot_wait_over_cap() {
+        // Regression: `boot_wait_secs` was untyped u64 with no cap, so a
+        // hostile caller could pass u64::MAX to tie up a daemon worker.
+        let app = router(test_state());
+        let resp = app
+            .oneshot(
+                Request::builder()
+                    .method("POST")
+                    .uri("/v1/snapshots")
+                    .header("content-type", "application/json")
+                    .body(Body::from(
+                        r#"{"tag":"ok","kernel":"/dev/null","rootfs":"/dev/null","boot_wait_secs":999999}"#,
+                    ))
+                    .unwrap(),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+    }
+
     #[tokio::test]
     async fn create_sandbox_rejects_unsafe_snapshot_tag_chars() {
         // Defense in depth: also reject tags containing characters that aren't

crates/forkd-controller/src/lib.rs

@@ -79,9 +79,7 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> {
             let raw = std::fs::read_to_string(p)
                 .with_context(|| format!("read token file {}", p.display()))?;
             let tok = raw.trim().to_string();
-            if tok.is_empty() {
-                anyhow::bail!("token file {} is empty", p.display());
-            }
+            validate_token(&tok).with_context(|| format!("validate token from {}", p.display()))?;
             tracing::info!(token_file = %p.display(), "bearer-token auth enabled");
             AuthConfig::with_token(tok)
         }
@@ -185,3 +183,70 @@ fn spawn_shutdown_signal(handle: Handle) {
         handle.graceful_shutdown(Some(Duration::from_secs(30)));
     });
 }
+
+/// Reject tokens that are empty, obvious placeholders, or below a minimum
+/// entropy budget. Pure function so it's exercised by unit tests without
+/// having to spin up the daemon.
+fn validate_token(tok: &str) -> Result<()> {
+    if tok.is_empty() {
+        anyhow::bail!("token is empty");
+    }
+    // Reject the literal placeholder shipped in packaging/k8s/. A user who
+    // runs `kubectl apply -f` without first running the documented
+    // `sed`/Secret-replacement step would otherwise get a daemon protected
+    // only by a publicly-known bearer token.
+    if tok.starts_with("REPLACE_ME") || tok.starts_with("CHANGE_ME") {
+        anyhow::bail!(
+            "token still contains the manifest placeholder ({tok}); \
+             replace it with a real 32-byte secret before starting the daemon"
+        );
+    }
+    // Reject suspiciously short tokens — sufficient entropy is the user's
+    // responsibility, but anything under 16 bytes is almost certainly a
+    // copy-paste mistake rather than a deliberate choice.
+    if tok.len() < 16 {
+        anyhow::bail!(
+            "token is only {} bytes; use at least 16 bytes of high-entropy randomness",
+            tok.len()
+        );
+    }
+    Ok(())
+}
+
+#[cfg(test)]
+mod tests {
+    use super::validate_token;
+
+    #[test]
+    fn rejects_empty_token() {
+        assert!(validate_token("").is_err());
+    }
+
+    #[test]
+    fn rejects_replace_me_placeholder() {
+        // Regression: this exact string is shipped in packaging/k8s/
+        // forkd-controller.yaml.
+        let err =
+            validate_token("REPLACE_ME_WITH_32_BYTES_BASE64").expect_err("placeholder accepted");
+        let msg = format!("{err:#}");
+        assert!(msg.contains("placeholder"), "msg was: {msg}");
+    }
+
+    #[test]
+    fn rejects_change_me_variant() {
+        assert!(validate_token("CHANGE_ME_PLEASE").is_err());
+    }
+
+    #[test]
+    fn rejects_too_short_token() {
+        assert!(validate_token("short").is_err());
+        // 15 bytes is one under the cap.
+        assert!(validate_token("123456789012345").is_err());
+    }
+
+    #[test]
+    fn accepts_realistic_token() {
+        // 32 hex chars = 16 bytes of entropy if random.
+        assert!(validate_token("a1b2c3d4e5f60718293a4b5c6d7e8f90").is_ok());
+    }
+}

crates/forkd-controller/tests/http_integration.rs

@@ -138,7 +138,7 @@ async fn end_to_end_version_round_trips() {
 
 #[tokio::test]
 async fn end_to_end_auth_rejects_missing_token() {
-    let d = TestDaemon::start_with_token("s3cret").await;
+    let d = TestDaemon::start_with_token("s3cret-test-token-1234567890").await;
     // /healthz is intentionally exempt — load balancers must probe.
     let h = reqwest::get(format!("{}/healthz", d.base)).await.unwrap();
     assert_eq!(h.status(), 200);
@@ -149,11 +149,11 @@ async fn end_to_end_auth_rejects_missing_token() {
 
 #[tokio::test]
 async fn end_to_end_auth_accepts_valid_token() {
-    let d = TestDaemon::start_with_token("s3cret").await;
+    let d = TestDaemon::start_with_token("s3cret-test-token-1234567890").await;
     let client = reqwest::Client::new();
     let resp = client
         .get(format!("{}/version", d.base))
-        .bearer_auth("s3cret")
+        .bearer_auth("s3cret-test-token-1234567890")
         .send()
         .await
         .unwrap();

packaging/k8s/forkd-controller.yaml

@@ -44,6 +44,10 @@ type: Opaque
 stringData:
   # Replace with `head -c 32 /dev/urandom | base64`. The controller
   # reads /etc/forkd/token; clients pass it as `Authorization: Bearer`.
+  # The daemon REFUSES to start if this value is left at the literal
+  # placeholder (any string beginning with "REPLACE_ME" or "CHANGE_ME"
+  # is rejected at startup) — so forgetting this step is a noisy fail,
+  # not a silent compromise.
   token: REPLACE_ME_WITH_32_BYTES_BASE64
 ---
 apiVersion: apps/v1