fix: close what a second adversarial pass found

Three reviewers went over the previous round in isolated worktrees. Two of the
findings are attack surface the previous round introduced.

**`health_file` was an arbitrary-file read whose contents came back to an
anonymous caller.** `tokio::fs::File::open` follows symlinks and the agent runs
as root. The path is measured into the compose hash; what sits *at* that path
at runtime is not, and the docs tell the app to bind-mount the directory into a
container -- so the container can replace it with a symlink afterwards. The
two-line parser then quoted what it found: a line that did not parse as a
timestamp came back as `"...is not a unix time in seconds: <that line>"`, 64
bytes at a time, on the unauthenticated 0.0.0.0:8090 listener. The errno was an
oracle for any path on top of that. Opened `O_NOFOLLOW` now, refused unless it
is a regular file, and the contents are never quoted -- a verdict names the
rule that failed, which is what an operator needs and is not an exfiltration
channel.

**A FIFO at that path killed the agent's blocking pool, silently.**
`tokio::fs` opens on `spawn_blocking`, and a blocking task cannot be cancelled,
so `open(2)` on a reader-less FIFO parks that thread forever while the timeout
merely drops the future. One thread per refresh, 1 per 15s, pool dead in about
two hours -- after which no `tokio::fs` call in the root agent completes. The
watchdog would not catch it: it probes over HTTP and never touches the pool.
`O_NONBLOCK` makes that open return immediately. The test for it hangs if the
flag is removed, which is how it was confirmed.

**`COMPOSE_PROJECT_NAME` defeated the container path entirely.** Measured on
docker compose 5.1.4 and nerdctl 2.3.5: both honour that env var, and it
outranks the compose file's top-level `name:`. The app supplies it through
`.decrypted-env`, so deriving the project from the file was wrong exactly when
an app sets one -- an empty container list, which `judge` reads as "nothing
started yet", i.e. permanently unhealthy with a message saying the opposite.
`NERDCTL_NAMESPACE` had the same shape.

So `app-compose.sh` records the namespace and the project it actually used --
resolved by `docker compose config`, which applies the real precedence -- and
the agent reads that instead of guessing. Its absence is now meaningful too:
the file is written before anything starts, so "not there yet" is the boot
window rather than a guess that happens to find nothing. That also deletes the
YAML parsing this branch added, and its dependency: the compose file is no
longer parsed by the agent at all.

**The poll round had no budget.** Measured against a listener that accepts and
never answers: `ceil(n/16) * 2s`, so 256 mute instances make a 33s round, and
`MissedTickBehavior::Delay` then makes that the interval -- one tenant's mute
CVMs become every other tenant's detection latency, permanently, because being
marked unhealthy does not remove an instance from the target list. The round
now gets one interval and rotates where it starts, so nothing starves, and it
says how many it did not reach rather than truncating quietly.

The rest, smaller:

- The snapshot is rewritten in full whenever any verdict changes, so one
  flapping instance rewrote the whole fleet's file every round -- ~39 KB/s at
  2000 instances, paid by the operator. Debounced; losing 30s of it costs one
  poll.
- `select_targets` held the routing mutex across `latest_handshakes`, which
  clones every peer's key (3.9 ms at 5000 instances) and on a cold cache shells
  out to `wg show` synchronously. Fetched before the lock now.
- `warn_all_unhealthy`'s rate limit was a process-global mutex taken on the
  per-connection path, so one tenant's fail-open serialized against another
  tenant's connections. The state moved into `ProxyStateMut`, which the caller
  already holds.
- `sanitize` mixed character counts and byte counts three ways: a 400-character
  multi-byte reason was cut to 512 bytes with no truncation marker. And
  `char::is_control` is category Cc only, so U+2028, U+2029 and the bidi
  overrides survived it -- U+2028 is a line break to most log viewers, which is
  the log forging the function exists to prevent. Both copies now share
  `dstack_types::sanitize_for_log`.
- `HealthStore::restore` could hand a verdict to an instance the record says is
  not gated, leaving it out of rotation *and* out of the poll set with nothing
  able to lift it. The record wins.
- `Info()` carries the whole app-compose, so 1 MiB was too tight for the
  port-policy client; the guest-facing bound is sized for that call, in one
  place both gateway clients go through. The VMM's vsock client to a guest
  agent was unbounded and now is not -- that peer is a tenant's CVM and the
  VMM is the control plane for every other tenant on the host.
- Deploy-time validation rejected the wrong-runner case but not the likelier
  one: `enabled` with no `health_file` and no service declaring a
  `healthcheck:`, which reports healthy forever.
- Go could not express `health_file: ""` the way the other three SDKs can, so
  it hashed differently.

Tests for the four things a mutation showed nothing covered: the
`requirements.health_check.enabled` -> `RegisterCvmRequest` hop (the only hop
between the manifest and the gateway, and making it inert left all 95 tests
green), the two guest clients opting into the response bound,
`record_instance_health`'s return value (the sole trigger for the snapshot),
and the snapshot's wiring at both ends. Two more asserted a constant against
itself and now assert the literal `dstack` that `WorkingDirectory=/dstack`
implies.

Docs corrected where they were wrong rather than merely thin: `requirements`
only fails an older guest closed if the deployer writes `"manifest_version":
"3"` as a string, since `AppCompose` is not `deny_unknown_fields`; a
single-file bind mount plus write-then-rename cancel out, because the rename
never touches the inode the agent holds; and the changelog described a
transport-wide 1 MiB cap that was deliberately not what shipped.

Author: kvinwang

CHANGELOG.md

@@ -11,9 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - shared API authentication (`dstack-api-auth`) protecting the full VMM HTTP/pRPC/UI surface and unifying Gateway/KMS admin auth: bearer/`X-Admin-Token`/HTTP Basic/bcrypt htpasswd, constant-time verification (#796)
 - gateway: `Admin.SetInstanceReady` takes a CVM instance out of its app's load-balancing rotation without stopping it; instance-id routing stays open so the instance can still be investigated, and the setting survives re-registration
 - gateway: opt-in application-level health polling. An app sets `requirements.health_check.enabled` in its app-compose; the gateway then asks that CVM's guest agent (new `Worker.Health` RPC) whether the app is serving, and keeps instances that say no -- or that have not answered since registering -- out of app-id load balancing. Apps that do not opt in are never polled. Instance-id routing is never gated, and an app whose every instance reports unhealthy is routed to anyway rather than blackholed. Documented in `docs/app-health-checks.md`
-- app-compose: `requirements.health_check.health_file` names a file the app writes its own verdict into -- two lines, `healthy`/`unhealthy` and the unix timestamp it was written at, treated as unhealthy once older than 60s. Without it the agent judges the app's own Compose project: every container that declares a `healthcheck` must be running and healthy
+- app-compose: `requirements.health_check.health_file` names a file the app writes its own verdict into -- two lines, `healthy`/`unhealthy` and the unix timestamp it was written at, treated as unhealthy once older than 60s. It must be a regular file and is opened `O_NOFOLLOW`, and its contents are never quoted back into a report. Without it the agent judges the app's own Compose project: every container that declares a `healthcheck` must be running and healthy
 - guest-agent: container health also covers the `nerdctl-compose` runner, read through `nerdctl inspect` (its output is Docker-compatible). Requires nerdctl >= 2.3.1 for Compose `healthcheck:` to be honoured; the mkosi backend is pinned to 2.3.5
-- http-client: response bodies are capped at 1 MiB. Several callers talk to peers they do not control, and one of them now does it on a timer against the whole fleet
+- http-client: a caller can bound the response body (`http_request_bounded`, `PrpcClient::with_max_response_bytes`). Nothing is bounded by default — `dstack vmm logs --lines 100000` is a legitimate multi-megabyte fetch — but the gateway's two guest-agent clients opt in, because a CVM is untrusted and one of them polls on a timer against the whole fleet
 
 ### Changed
 - os/yocto: nerdctl 2.2.1 → 2.3.5, so `nerdctl compose` honours the Compose `healthcheck:` field (only translated into `--health-*` flags from 2.3.1 on). Requires openembedded-core to move to `wrynose` head for go 1.26.5, which also brings gcc 15.2 → 15.3 — every guest image measurement changes, so the new image hashes need whitelisting in KMS

docs/app-health-checks.md

@@ -32,10 +32,16 @@ That flag reaches the gateway at registration. An app that does not set it is
 never polled and is routed to exactly as it was before this feature existed.
 
 It lives under `requirements` rather than at the top level for a reason:
-`requirements` is rejected outright by guest images older than
-`manifest_version` 3, and unknown fields inside it are a hard error. A
-deployment that asks for health gating therefore cannot silently land on an
-image that would ignore it.
+`requirements` is rejected by guest images older than `manifest_version` 3, and
+unknown fields *inside* it are a hard error. So a deployment that asks for
+health gating does not silently land on an image that would ignore it.
+
+That protection is conditional, and the condition is yours: write
+`"manifest_version": "3"` — **the string form**, as above. `AppCompose` itself
+is not `deny_unknown_fields`, so an app-compose left at `"manifest_version": 2`
+with a `requirements` block is *rejected* by a current guest and *silently
+accepted, minus the requirements*, by an older one. The string form is what an
+older guest chokes on.
 
 ## Where the verdict comes from
 
@@ -125,10 +131,28 @@ to take yourself out of rotation a minute later.
 
 Anything else is not healthy: a missing file, a malformed one, an unparseable
 timestamp, or a state word the agent does not recognise. Writing it atomically
-(write a temporary file, then rename) avoids being judged on a half-written one.
-
-For a container to write this file, bind-mount the path into it. The agent reads
-it from the guest rootfs, not from inside any container.
+(write a temporary file in the same directory, then rename) avoids being judged
+on a half-written one.
+
+It must be a **regular file**, and the agent will not follow a symlink to it.
+The path is measured into the compose hash; what sits at that path at runtime is
+not, so a symlink would let a container redirect a root-owned read after the
+fact, and a FIFO would park one of the agent's threads on every refresh. Neither
+is reported as unhealthy-with-a-reason so much as refused outright.
+
+For the same reason the agent never quotes the file's contents back. A verdict
+names the rule that failed — "line 1 is neither healthy nor unhealthy" — because
+the report is served to anonymous callers and the bytes at that path may not be
+the bytes the app wrote.
+
+The agent reads the file from the guest rootfs, not from inside any container,
+so a container that writes it needs the path bind-mounted in. **Mount the
+directory, not the file.** A single-file bind mount pins an inode: a rename
+inside the container replaces the container's own directory entry and never
+touches the file the agent is reading, so the agent sees the original contents
+forever, the timestamp ages past the limit, and the instance goes permanently
+unhealthy. With the directory mounted, either write-then-rename or a plain
+in-place write works.
 
 Before the first refresh completes, the app reports **not** healthy.
 Registration happens long before the application is up, so "not determined yet"

dstack/Cargo.lock

@@ -556,6 +556,63 @@ pub struct HealthCheck {
     pub health_file: Option<String>,
 }
 
+/// Make a string that came from an untrusted party safe to put in a log line,
+/// and bound its length.
+///
+/// Both ends of the health path need this and neither can rely on the other:
+/// the guest agent sanitizes what an app wrote before reporting it, and the
+/// gateway sanitizes what a guest agent reported before logging it, because a
+/// guest agent is exactly the party the gateway does not trust.
+///
+/// "Unsafe" is wider than [`char::is_control`], which is only Unicode category
+/// Cc. It misses two families that do the same damage:
+///
+/// - `U+2028` LINE SEPARATOR and `U+2029` PARAGRAPH SEPARATOR, which many log
+///   viewers -- and every JavaScript or JSON consumer downstream of one --
+///   treat as a line break. That is the log-forging that stripping `\n` was
+///   meant to prevent.
+/// - The bidirectional formatting characters (`U+202A`..`U+202E`,
+///   `U+2066`..`U+2069`, `U+200E`, `U+200F`) and `U+FEFF`. `U+202E` reverses
+///   the rendering of everything after it, so an attacker controls how the
+///   rest of the line reads in a terminal without controlling its bytes.
+///
+/// Truncation is by bytes, and it says so when it happens: a bound that
+/// silently cuts a reason is worse than a short one, because the reader cannot
+/// tell a complete message from a clipped one.
+pub fn sanitize_for_log(text: &str, max_bytes: usize) -> String {
+    const MARKER: &str = "... (truncated)";
+    let mut cleaned = String::with_capacity(text.len().min(max_bytes));
+    let mut truncated = false;
+    for ch in text.chars() {
+        let ch = if is_unsafe_to_log(ch) { ' ' } else { ch };
+        if cleaned.len() + ch.len_utf8() > max_bytes {
+            truncated = true;
+            break;
+        }
+        cleaned.push(ch);
+    }
+    if truncated {
+        cleaned.push_str(MARKER);
+    }
+    cleaned
+}
+
+/// Whether a character must not survive into a log line. See
+/// [`sanitize_for_log`].
+fn is_unsafe_to_log(ch: char) -> bool {
+    ch.is_control()
+        || matches!(
+            ch,
+            '\u{2028}'
+                | '\u{2029}'
+                | '\u{200E}'
+                | '\u{200F}'
+                | '\u{202A}'..='\u{202E}'
+                | '\u{2066}'..='\u{2069}'
+                | '\u{FEFF}'
+        )
+}
+
 /// How old a `health_file` may be before it is read as unhealthy.
 ///
 /// Fixed rather than configurable: this is a liveness bound, and an app that
@@ -2484,3 +2541,71 @@ mod vm_config_device_count_tests {
         );
     }
 }
+
+#[cfg(test)]
+mod log_sanitize_tests {
+    use super::sanitize_for_log;
+
+    #[test]
+    fn strips_the_characters_that_forge_a_log_line() {
+        let forged = sanitize_for_log("web\nJan 01 INFO all good", 512);
+        assert!(!forged.contains('\n'), "{forged:?}");
+        let escaped = sanitize_for_log("\u{1b}[2Jcleared", 512);
+        assert!(!escaped.contains('\u{1b}'), "{escaped:?}");
+    }
+
+    /// `char::is_control` is category Cc only, so these get through it -- and a
+    /// log viewer, or anything JSON downstream of one, treats them as breaks.
+    #[test]
+    fn strips_the_unicode_line_separators() {
+        for separator in ['\u{2028}', '\u{2029}', '\u{85}'] {
+            let out = sanitize_for_log(&format!("web{separator}forged"), 512);
+            assert!(!out.contains(separator), "{separator:?} survived: {out:?}");
+        }
+    }
+
+    /// U+202E reverses how everything after it renders, so an attacker decides
+    /// what a reader sees without controlling the bytes.
+    #[test]
+    fn strips_bidi_overrides() {
+        for bidi in ['\u{202E}', '\u{202A}', '\u{2066}', '\u{200F}', '\u{FEFF}'] {
+            let out = sanitize_for_log(&format!("web{bidi}txet"), 512);
+            assert!(!out.contains(bidi), "{bidi:?} survived: {out:?}");
+        }
+    }
+
+    /// Bounded in bytes, not characters. Counting characters lets a multi-byte
+    /// string reach four times the intended size.
+    #[test]
+    fn bounds_bytes_not_characters() {
+        let wide = "\u{1d54f}".repeat(400);
+        assert_eq!(wide.chars().count(), 400);
+        assert_eq!(wide.len(), 1600);
+        let out = sanitize_for_log(&wide, 512);
+        assert!(out.len() <= 512 + "... (truncated)".len(), "{}", out.len());
+    }
+
+    /// A bound that silently clips is worse than a short one: the reader cannot
+    /// tell a complete reason from a cut one.
+    #[test]
+    fn says_when_it_truncated() {
+        let wide = "\u{1d54f}".repeat(400);
+        assert!(sanitize_for_log(&wide, 512).ends_with("(truncated)"));
+        assert!(!sanitize_for_log("web is starting", 512).ends_with("(truncated)"));
+    }
+
+    #[test]
+    fn never_splits_a_character() {
+        // 512 is not a multiple of 4, so a naive byte cut would split one.
+        let wide = "\u{1d54f}".repeat(400);
+        let out = sanitize_for_log(&wide, 512);
+        let body = out.trim_end_matches("... (truncated)");
+        assert!(body.is_char_boundary(body.len()));
+        assert_eq!(body.len() % 4, 0);
+    }
+
+    #[test]
+    fn ordinary_text_is_left_alone() {
+        assert_eq!(sanitize_for_log("web is starting", 512), "web is starting");
+    }
+}

dstack/dstack-types/src/lib.rs

@@ -18,6 +18,23 @@ pub struct ComposeInfo {
     pub service_names: std::collections::HashSet<String>,
 }
 
+/// Whether any service in a compose file declares a `healthcheck:`.
+///
+/// `None` means the file could not be parsed, which is not the same as "no" --
+/// the caller must not turn an unreadable file into a verdict about its
+/// contents.
+pub fn compose_declares_a_healthcheck(compose_content: &str) -> Option<bool> {
+    let docs = YamlLoader::load_from_str(compose_content).ok()?;
+    let Yaml::Hash(services) = &docs.first()?["services"] else {
+        return None;
+    };
+    Some(
+        services
+            .values()
+            .any(|service| !service["healthcheck"].is_badvalue()),
+    )
+}
+
 /// Parse a docker-compose file and extract project name and service names
 pub fn parse_docker_compose_file(compose_file: impl AsRef<Path>) -> Result<ComposeInfo> {
     let compose_content =

dstack/dstack-util/src/docker_compose.rs

@@ -474,16 +474,7 @@ impl<'a> GatewayContext<'a> {
                 .collect(),
             restrict_mode: self.shared.app_compose.port_policy.restrict_mode,
         };
-        // Opt-in, not a build-time fact: the guest agent only refreshes a
-        // verdict when the app asked for one, so telling the gateway to poll an
-        // app that did not would cost every gateway node a round trip per
-        // interval to be told what it already assumes.
-        let health_check = self
-            .shared
-            .app_compose
-            .requirements
-            .as_ref()
-            .is_some_and(|requirements| requirements.health_check_enabled());
+        let health_check = health_check_requested(&self.shared.app_compose);
         let client = self.create_gateway_client(
             gateway_url,
             &key_store.client_key,
@@ -987,6 +978,24 @@ fn verify_app_compose_policy(shared: &HostShared) -> Result<()> {
     Ok(())
 }
 
+/// Whether this app asked the gateway to gate its traffic on health.
+///
+/// Opt-in, not a build-time fact: the guest agent only refreshes a verdict when
+/// the app asked for one, so telling the gateway to poll an app that did not
+/// would cost every gateway node a round trip per interval to be told what it
+/// already assumes.
+///
+/// This is the only hop between the app's manifest and the gateway's behaviour,
+/// which is why it is a named function rather than an expression inside the
+/// registration call: getting it wrong makes the whole feature inert fleet-wide
+/// with nothing to show for it.
+fn health_check_requested(app_compose: &AppCompose) -> bool {
+    app_compose
+        .requirements
+        .as_ref()
+        .is_some_and(|requirements| requirements.health_check_enabled())
+}
+
 fn verify_manifest_feature_requirements(app_compose: &AppCompose) -> Result<()> {
     let manifest_version = verify_manifest_version(app_compose)?;
     if app_compose.requirements.is_some() && manifest_version < MANIFEST_VERSION_3 {
@@ -1043,6 +1052,22 @@ fn verify_health_check_requirement(app_compose: &AppCompose) -> Result<()> {
                     app_compose.runner
                 );
             }
+            // Same failure, one level down and rather more likely: only
+            // containers that declare a `healthcheck:` are judged, so a compose
+            // file with none reports healthy the moment any container is
+            // created and keeps doing so after every one of them has exited.
+            // An unparseable file is not evidence either way and is left to
+            // whatever fails on it next.
+            let declares = app_compose
+                .docker_compose_file
+                .as_deref()
+                .and_then(crate::docker_compose::compose_declares_a_healthcheck);
+            if declares == Some(false) {
+                bail!(
+                    "requirements.health_check is enabled but no service in docker_compose_file \
+                     declares a healthcheck; add one, or set health_file to report health directly"
+                );
+            }
         }
     }
     Ok(())
@@ -3521,13 +3546,39 @@ fn test_app_compose(
     serde_json::from_value(value).unwrap()
 }
 
+/// The single hop between `requirements.health_check.enabled` and
+/// `RegisterCvmRequest.health_check`. Nothing else connects the app's manifest
+/// to the gateway's behaviour, so if this is wrong the feature is inert
+/// fleet-wide and nothing else fails.
+#[test]
+fn health_check_opt_in_reaches_registration() {
+    let opted_in =
+        compose_with_health_check("docker-compose", serde_json::json!({"enabled": true}));
+    assert!(health_check_requested(&opted_in));
+
+    let opted_out =
+        compose_with_health_check("docker-compose", serde_json::json!({"enabled": false}));
+    assert!(!health_check_requested(&opted_out));
+}
+
+/// An app that says nothing must register as "do not poll me", not as an
+/// opt-in by omission.
+#[test]
+fn an_app_that_says_nothing_does_not_ask_to_be_polled() {
+    let no_requirements = test_app_compose(serde_json::json!("3"), None, None);
+    assert!(!health_check_requested(&no_requirements));
+
+    let empty_requirements = test_app_compose(serde_json::json!("3"), Some(">=0.6.1"), None);
+    assert!(!health_check_requested(&empty_requirements));
+}
+
 #[cfg(test)]
 fn compose_with_health_check(runner: &str, health_check: serde_json::Value) -> AppCompose {
     serde_json::from_value(serde_json::json!({
         "manifest_version": "3",
         "name": "health-app",
         "runner": runner,
-        "docker_compose_file": "services: {}\n",
+        "docker_compose_file": "services:\n  web:\n    image: app:1\n    healthcheck:\n      test: [\"CMD\", \"true\"]\n",
         "requirements": { "health_check": health_check },
     }))
     .unwrap()
@@ -3539,6 +3590,43 @@ fn health_check_on_a_container_runner_needs_no_health_file() {
     verify_manifest_feature_requirements(&compose).unwrap();
 }
 
+/// Only containers that declare a `healthcheck:` are judged, so a compose file
+/// with none reports healthy forever -- the same silent no-op the runner check
+/// above exists to remove, and the likelier mistake.
+#[test]
+fn health_check_without_any_declared_healthcheck_is_rejected() {
+    let mut compose =
+        compose_with_health_check("docker-compose", serde_json::json!({"enabled": true}));
+    compose.docker_compose_file = Some("services:\n  web:\n    image: app:1\n".to_string());
+    let err = verify_manifest_feature_requirements(&compose).unwrap_err();
+    assert!(
+        err.to_string().contains("declares a healthcheck"),
+        "unexpected error: {err}"
+    );
+}
+
+#[test]
+fn a_declared_healthcheck_on_any_service_is_enough() {
+    let mut compose =
+        compose_with_health_check("docker-compose", serde_json::json!({"enabled": true}));
+    compose.docker_compose_file = Some(
+        "services:\n  init:\n    image: app:1\n  web:\n    image: app:1\n    \
+         healthcheck:\n      test: [\"CMD\", \"true\"]\n"
+            .to_string(),
+    );
+    verify_manifest_feature_requirements(&compose).unwrap();
+}
+
+/// An unparseable compose file is not evidence that it declares nothing, and
+/// turning it into one here would reject a deployment for the wrong reason.
+#[test]
+fn an_unparseable_compose_file_is_left_to_whatever_fails_on_it_next() {
+    let mut compose =
+        compose_with_health_check("docker-compose", serde_json::json!({"enabled": true}));
+    compose.docker_compose_file = Some("\tservices: [unbalanced".to_string());
+    verify_manifest_feature_requirements(&compose).unwrap();
+}
+
 /// The `bash` runner starts no containers, so the container fallback has
 /// nothing to look at and would answer healthy forever. That is exactly the
 /// silent no-op the opt-in exists to avoid.