fix(serve): keep a public endpoint authenticated across respawns

apps/rocm/src/main.rs

@@ -4342,8 +4342,11 @@ fn validate_bind_host(host: &str, allow_public_bind: bool) -> Result<()> {
     Ok(())
 }
 
+/// Inverse of [`rocm_engine_protocol::is_public_bind_host`], which owns the
+/// policy so `rocmd` classifies a recorded `host` identically when it respawns
+/// the service.
 fn is_loopback_host(host: &str) -> bool {
-    matches!(host, "127.0.0.1" | "localhost" | "::1")
+    !rocm_engine_protocol::is_public_bind_host(host)
 }
 
 /// Resolve the API key that will guard this endpoint, applying the
@@ -4424,6 +4427,37 @@ fn ensure_public_bind_engine_supported(
     Ok(())
 }
 
+/// Refuse to (re)spawn a managed service recorded on a public host when its
+/// endpoint API key is gone, so a respawn cannot reopen the endpoint anonymously.
+///
+/// `serve()` applies the loopback-vs-public policy once, via
+/// [`resolve_endpoint_auth`], and persists the resulting key. Every later spawn
+/// — `rocm services restart`, and `rocmd`'s recovery supervisor — reads that key
+/// file back and would otherwise treat "no key file" as "no auth wanted",
+/// silently downgrading a protected public endpoint to an open one. The real
+/// invariant is a property of the *host*, not of the file: a non-loopback bind
+/// must always be authenticated.
+///
+/// The gap is reachable through ordinary commands, because a stop deletes the
+/// key file and `rocm services restart` accepts a stopped service id (its help
+/// points at `rocm services list --all`).
+///
+/// `key_present` is a plain `bool` rather than a path so both branches are
+/// unit-testable without touching the filesystem, mirroring `is_windows` in
+/// [`ensure_public_bind_engine_supported`].
+fn ensure_public_service_has_endpoint_key(host: &str, key_present: bool) -> Result<()> {
+    if rocm_engine_protocol::is_public_bind_host(host) && !key_present {
+        bail!(
+            "managed service is bound to the public host `{host}` but has no endpoint API key, \
+             so restarting it would reopen it without authentication. The key is dropped when a \
+             service stops and cannot be recovered. Launch it again with \
+             `rocm serve --host {host} --allow-public-bind` (add `--api-key <key>`, or set \
+             ROCM_SERVE_API_KEY, to choose the key instead of generating one)."
+        );
+    }
+    Ok(())
+}
+
 /// Write `contents` to `path` with owner-only (0600) permissions on Unix so a
 /// secret is not world-readable. On non-Unix, default permissions apply.
 pub(crate) fn write_private_file_0600(path: &Path, contents: &[u8]) -> Result<()> {
@@ -4636,7 +4670,17 @@ fn spawn_managed_engine_child(
     // environment. A path — not the secret value — is what the detached-spawn
     // primitives accept as an env override, and it keeps the key off both the argv
     // and the environment block. `serve()` wrote the file before spawning.
-    let endpoint_key_file = endpoint_keys::endpoint_key_file_if_present(paths, service_id);
+    // Validity, not mere existence: the engine adapters resolve the key with
+    // `endpoint_api_key_from_file` and enforce nothing when it yields `None`, so
+    // an empty or malformed key file would otherwise satisfy the guard below and
+    // still produce an unauthenticated public listener.
+    let endpoint_key_file = endpoint_keys::endpoint_key_file_if_present(paths, service_id)
+        .filter(|path| rocm_engine_protocol::endpoint_api_key_from_file(path).is_some());
+    // `serve()` already resolved and stored the key for a public bind, so this
+    // cannot fire on the fresh-launch path today. It is the shared choke point
+    // for managed spawns, so enforce the invariant here too rather than relying
+    // on every future caller having done so.
+    ensure_public_service_has_endpoint_key(host, endpoint_key_file.is_some())?;
     #[cfg(windows)]
     let child_pid = {
         let env_values = app_path_env_var_values(paths, engine_envs_root.as_deref());
@@ -12767,7 +12811,14 @@ fn stop_internal_managed_service(paths: &AppPaths, service_id: &str) -> Result<s
     // Drop the endpoint key with the service by deleting its 0600 key file.
     // Best-effort — a stopped service must not fail to stop just because key
     // cleanup did.
-    endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
+    //
+    // Gated on `all_stopped` for the same reason the status is: an unconfirmed
+    // stop may have left the engine alive and still enforcing the key, and
+    // discarding our only copy would lock the CLI's own probes, chat, and
+    // service discovery out of a service that is otherwise fine.
+    if all_stopped {
+        endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
+    }
     let engine_stop = match engine_stop {
         Ok(response) => serde_json::json!({
             "attempted": true,
@@ -12816,6 +12867,9 @@ fn restart_internal_managed_service(
     // service back on the same public host with the same auth. Capture it first and
     // re-store it after the stop so the spawn below hands the child the same key.
     let preserved_endpoint_key = endpoint_keys::endpoint_api_key(paths, service_id);
+    // Checked before the stop, so a refused restart leaves a running service
+    // running instead of stopping it and then failing to bring it back.
+    ensure_public_service_has_endpoint_key(&record.host, preserved_endpoint_key.is_some())?;
     let _ = stop_internal_managed_service(paths, service_id);
     if let Some(key) = preserved_endpoint_key.as_deref() {
         endpoint_keys::store_endpoint_api_key(paths, service_id, key)?;
@@ -13722,6 +13776,11 @@ fn refresh_managed_service_runtime_liveness(
     let has_tracked_pid = !tracked_pids.is_empty();
     let has_live_pid = tracked_pids.iter().any(|pid| process_is_running(*pid));
     if has_tracked_pid && !has_live_pid {
+        // Confirmed dead, so the endpoint key can finally go. `stop` only drops
+        // it when it could verify termination; this is where an unconfirmed stop
+        // (or a crash) gets its deferred cleanup, so a plaintext secret is not
+        // stranded on disk for a service that no longer exists.
+        endpoint_keys::clear_endpoint_api_key(paths, &record.service_id);
         if record.status != "stopped" {
             record.status = "stopped".to_owned();
             return true;
@@ -20579,6 +20638,73 @@ install therock";
         ensure_public_bind_engine_supported("lemonade", false, true).unwrap(); // loopback needs no key
     }
 
+    #[test]
+    fn respawn_fails_closed_for_a_public_service_whose_key_is_gone() {
+        // A stop deletes the key file, so a later restart of a public service
+        // would otherwise respawn it with no auth at all.
+        let error = ensure_public_service_has_endpoint_key("0.0.0.0", false).unwrap_err();
+        let message = error.to_string();
+        assert!(message.contains("0.0.0.0"), "{error:#}");
+        assert!(message.contains("without authentication"), "{error:#}");
+        // Actionable: name the command that mints a fresh key.
+        assert!(message.contains("--allow-public-bind"), "{error:#}");
+
+        // A public service that still has its key restarts normally.
+        ensure_public_service_has_endpoint_key("0.0.0.0", true).unwrap();
+    }
+
+    #[test]
+    fn respawn_allows_loopback_services_without_an_endpoint_key() {
+        // Loopback stays credential-free, so every accepted spelling must pass
+        // the guard with no key present.
+        for host in ["127.0.0.1", "localhost", "::1"] {
+            ensure_public_service_has_endpoint_key(host, false)
+                .unwrap_or_else(|error| panic!("{host} must not require a key: {error:#}"));
+        }
+    }
+
+    #[test]
+    fn restart_refuses_a_public_service_without_a_key_before_stopping_it() {
+        // The guard runs before the stop, so a refused restart must leave the
+        // record exactly as it was rather than taking down a running service.
+        let (root, paths) = test_paths("restart-public-no-key");
+        let service_id = "svc-public-nokey";
+        let mut record = ManagedServiceRecord::new(
+            &paths,
+            service_id,
+            "vllm",
+            "model-ref",
+            "canonical/model",
+            "0.0.0.0",
+            11435,
+            "managed",
+            std::process::id(),
+            None,
+            None,
+            Some("gpu_required".to_owned()),
+        );
+        record.status = "running".to_owned();
+        record.write().unwrap();
+
+        let error = restart_internal_managed_service(&paths, service_id).unwrap_err();
+        assert!(
+            error.to_string().contains("without authentication"),
+            "{error:#}"
+        );
+
+        // Proves the *ordering*, not just the refusal: had the guard run after
+        // `stop_internal_managed_service`, the stop would have written
+        // "stopped". It reaches that state here because the record's only pid is
+        // the test's own (`engine_pid` is None and `terminate_recorded_service_pids`
+        // skips the caller's pid), so the stop confirms termination trivially.
+        let after = load_managed_service(&paths, service_id).unwrap();
+        assert_ne!(
+            after.status, "stopped",
+            "a refused restart must not stop the service"
+        );
+        let _ = fs::remove_dir_all(root);
+    }
+
     #[test]
     fn endpoint_client_config_shows_key_once_with_bearer_guidance() {
         let rendered = render_endpoint_client_config("http://0.0.0.0:11435/v1", "secret-123");

apps/rocmd/src/lib.rs

@@ -2553,8 +2553,10 @@ fn build_watcher_enable_args(arguments: &serde_json::Map<String, Value>) -> Resu
     Ok(argv)
 }
 
+/// Inverse of [`rocm_engine_protocol::is_public_bind_host`], which owns the
+/// policy so `rocm` and `rocmd` never classify the same host differently.
 fn is_loopback_host(host: &str) -> bool {
-    matches!(host, "127.0.0.1" | "localhost" | "::1")
+    !rocm_engine_protocol::is_public_bind_host(host)
 }
 
 fn system_prefix_requires_ack(prefix: &std::path::Path) -> bool {
@@ -3081,6 +3083,16 @@ fn supervise_service(
     );
     record.gpu_indices = gpu_indices;
     record.engine_recipe_json = engine_recipe_json.clone();
+    // Refuse a keyless public respawn before the manifest write, so a refused
+    // attempt leaves the recorded restart_count and timestamps intact instead of
+    // clobbering them with a record no live process will ever back. The spawn
+    // site below re-checks against the key actually threaded onto the command.
+    ensure_public_service_has_endpoint_key(
+        &record.host,
+        rocm_engine_protocol::endpoint_key_file_if_present(paths, &record.service_id)
+            .and_then(|path| rocm_engine_protocol::endpoint_api_key_file_if_valid(&path))
+            .is_some(),
+    )?;
     record.write()?;
 
     let log_file = fs::File::create(&record.log_path)
@@ -3114,7 +3126,11 @@ fn supervise_service(
     // recovery (`restart_managed_service` re-execs `rocmd supervise`), so
     // without this a previously-authenticated public service would come back
     // up anonymous after a crash/recover cycle.
-    apply_endpoint_key_env(&mut command, paths, &record.service_id);
+    // If the key is gone the child would listen on the recorded public host with
+    // no auth, so fail closed instead — an unreachable service is recoverable,
+    // an anonymous public one is not.
+    let endpoint_key_applied = apply_endpoint_key_env(&mut command, paths, &record.service_id);
+    ensure_public_service_has_endpoint_key(&record.host, endpoint_key_applied)?;
     let mut child = command
         .spawn()
         .with_context(|| format!("failed to spawn engine supervisor child for {engine}"))?;
@@ -4622,6 +4638,31 @@ fn handle_server_recover_event_with_record(
             {
                 return Ok(());
             }
+            // A public service whose endpoint key is gone can never be recovered:
+            // the respawn guard in `supervise_service` refuses it by design.
+            // Report it and stop, rather than letting a permanent failure
+            // propagate out of `evaluate_watchers` and take the whole daemon —
+            // and every other watcher — down on each 30s recovery tick.
+            if let Err(error) = ensure_public_service_has_endpoint_key(
+                &record.host,
+                rocm_engine_protocol::endpoint_key_file_if_present(paths, &record.service_id)
+                    .and_then(|path| rocm_engine_protocol::endpoint_api_key_file_if_valid(&path))
+                    .is_some(),
+            ) {
+                return record_event(
+                    paths,
+                    state,
+                    "server-recover",
+                    "error",
+                    "restart_managed_service_refused",
+                    &format!(
+                        "cannot recover managed service {} on {}:{} after \
+                         {recovery_reason_display}: {error}",
+                        record.service_id, record.host, record.port
+                    ),
+                    Some(record.service_id.clone()),
+                );
+            }
             restart_managed_service(paths, &mut *record)?;
             record_event(
                 paths,
@@ -7017,7 +7058,7 @@ mod tests {
         // Loopback service: no key file has ever been written, so the child's
         // environment must be left untouched.
         let mut command = ProcessCommand::new("true");
-        apply_endpoint_key_env(&mut command, &paths, service_id);
+        assert!(!apply_endpoint_key_env(&mut command, &paths, service_id));
         assert!(
             command
                 .get_envs()
@@ -7032,7 +7073,7 @@ mod tests {
         fs::create_dir_all(paths.services_dir()).unwrap();
         fs::write(&key_path, "secret-key").unwrap();
         let mut command = ProcessCommand::new("true");
-        apply_endpoint_key_env(&mut command, &paths, service_id);
+        assert!(apply_endpoint_key_env(&mut command, &paths, service_id));
         let env_value = command
             .get_envs()
             .find_map(|(key, value)| {
@@ -7042,6 +7083,37 @@ mod tests {
             })
             .expect("endpoint key env var must be set once the key file exists");
         assert_eq!(env_value, key_path.as_os_str());
+
+        // An empty (or otherwise unusable) key file is not a key: the engine
+        // adapters would enforce nothing, so reporting `true` here would let the
+        // respawn guard pass and still open an anonymous public listener.
+        fs::write(&key_path, "   \n").unwrap();
+        let mut command = ProcessCommand::new("true");
+        assert!(!apply_endpoint_key_env(&mut command, &paths, service_id));
+        assert!(
+            command
+                .get_envs()
+                .all(|(key, _)| key != rocm_engine_protocol::ENDPOINT_API_KEY_FILE_ENV),
+            "an unusable key file must not be threaded onto the child"
+        );
+    }
+
+    #[test]
+    fn recovery_respawn_fails_closed_for_a_public_service_without_a_key() {
+        // Daemon recovery re-execs `rocmd supervise` for the recorded host. With
+        // the key gone the child would listen on that public host anonymously,
+        // so the spawn must be refused instead.
+        let error = ensure_public_service_has_endpoint_key("0.0.0.0", false).unwrap_err();
+        assert!(
+            error.to_string().contains("without authentication"),
+            "{error:#}"
+        );
+
+        ensure_public_service_has_endpoint_key("0.0.0.0", true).unwrap();
+        for host in ["127.0.0.1", "localhost", "::1"] {
+            ensure_public_service_has_endpoint_key(host, false)
+                .unwrap_or_else(|error| panic!("{host} must not require a key: {error:#}"));
+        }
     }
 
     #[test]
@@ -8924,13 +8996,49 @@ fn healthcheck_response_recoverable(response: &HealthcheckResponse) -> bool {
 }
 
 /// Re-thread the endpoint key file (public bind only) onto an engine child's
-/// environment, mirroring the initial `rocm serve` spawn. `None` for a
-/// loopback service with no stored key leaves the command's environment
-/// untouched, matching the unauthenticated default.
-fn apply_endpoint_key_env(command: &mut ProcessCommand, paths: &AppPaths, service_id: &str) {
-    if let Some(key_file) = rocm_engine_protocol::endpoint_key_file_if_present(paths, service_id) {
+/// environment, mirroring the initial `rocm serve` spawn. A loopback service
+/// with no stored key leaves the command's environment untouched, matching the
+/// unauthenticated default.
+///
+/// Returns whether a key was applied. Callers that spawn a *listener* must
+/// check it against the service's host — a public bind with no key would come
+/// up anonymous (see [`ensure_public_service_has_endpoint_key`]). Callers that
+/// only make a stdio plugin call can ignore it.
+///
+/// The file must hold a *usable* key, not merely exist: the engine adapters
+/// resolve it with `endpoint_api_key_from_file` and enforce nothing when that
+/// yields `None`, so treating an empty or malformed file as "protected" would
+/// let the guard pass while the endpoint served anonymously.
+#[must_use]
+fn apply_endpoint_key_env(
+    command: &mut ProcessCommand,
+    paths: &AppPaths,
+    service_id: &str,
+) -> bool {
+    if let Some(key_file) = rocm_engine_protocol::endpoint_key_file_if_present(paths, service_id)
+        .and_then(|path| rocm_engine_protocol::endpoint_api_key_file_if_valid(&path))
+    {
         command.env(rocm_engine_protocol::ENDPOINT_API_KEY_FILE_ENV, key_file);
+        return true;
     }
+    false
+}
+
+/// Refuse to respawn a recorded service on a public host without its endpoint
+/// API key, so daemon recovery cannot reopen a protected endpoint anonymously.
+///
+/// Mirrors the guard of the same name in `rocm`; the shared
+/// [`rocm_engine_protocol::is_public_bind_host`] keeps the two classifications
+/// identical for a given `ManagedServiceRecord::host`.
+fn ensure_public_service_has_endpoint_key(host: &str, key_present: bool) -> Result<()> {
+    if rocm_engine_protocol::is_public_bind_host(host) && !key_present {
+        bail!(
+            "refusing to respawn a service bound to the public host `{host}` without an endpoint \
+             API key: it would come back up without authentication. Relaunch it with \
+             `rocm serve --host {host} --allow-public-bind` to issue a new key."
+        );
+    }
+    Ok(())
 }
 
 fn engine_request<T, R>(
@@ -8962,7 +9070,10 @@ where
     // adapter's HTTP probe is unauthenticated and the endpoint looks
     // unreachable, which would misclassify a healthy protected service as
     // recoverable.
-    apply_endpoint_key_env(&mut command, paths, service_id);
+    //
+    // No host to check here: this is a stdio plugin call, not a listener, so a
+    // missing key only weakens this probe rather than opening a port.
+    let _ = apply_endpoint_key_env(&mut command, paths, service_id);
     let mut child = command.spawn().with_context(|| {
         format!(
             "failed to spawn engine stdio process {}",