diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index b0a3fd13..c93826a8 100644 --- a/apps/rocm/src/main.rs +++ b/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 `, 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 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"); diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index a448cb76..0fbb2858 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -2553,8 +2553,10 @@ fn build_watcher_enable_args(arguments: &serde_json::Map) -> 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( @@ -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 {}", diff --git a/crates/rocm-engine-protocol/src/lib.rs b/crates/rocm-engine-protocol/src/lib.rs index ac7ab599..cd299ccd 100644 --- a/crates/rocm-engine-protocol/src/lib.rs +++ b/crates/rocm-engine-protocol/src/lib.rs @@ -30,6 +30,24 @@ pub const DEFAULT_LOG_TAIL_LINES: usize = 80; /// (e.g. a loopback bind). pub const ENDPOINT_API_KEY_FILE_ENV: &str = "ROCM_SERVE_API_KEY_FILE"; +/// Whether `host` names a public (non-loopback) bind, i.e. one that must be +/// protected by an endpoint API key. +/// +/// Shared so `rocm` (which decides at `serve` time whether to mint a key) and +/// `rocmd` (which respawns recorded services during recovery) apply the same +/// policy to the same `ManagedServiceRecord::host` value. +/// +/// Deliberately an exact match against the three loopback spellings the CLI +/// accepts, so anything unrecognized — an IPv6 loopback alias, a differently +/// cased `localhost`, a hostname that happens to resolve locally — is treated +/// as public and therefore *requires* a key. Misclassifying a loopback bind as +/// public costs a key nobody needed; the reverse would open an unauthenticated +/// network listener. +#[must_use] +pub fn is_public_bind_host(host: &str) -> bool { + !matches!(host, "127.0.0.1" | "localhost" | "::1") +} + /// Resolve the endpoint API key an engine adapter must enforce, if any. /// /// Reads the key file named by [`ENDPOINT_API_KEY_FILE_ENV`]. Returns `None` when @@ -664,6 +682,27 @@ mod tests { std::fs::remove_dir_all(&paths.data_dir).ok(); } + #[test] + fn is_public_bind_host_recognizes_only_the_accepted_loopback_spellings() { + for host in ["127.0.0.1", "localhost", "::1"] { + assert!(!is_public_bind_host(host), "{host} must be loopback"); + } + for host in [ + "0.0.0.0", + "::", + "192.168.1.10", + "example.test", + // Conservative on purpose: an unrecognized spelling of a loopback + // address is classified public, so it requires a key rather than + // opening an unauthenticated listener. + "LOCALHOST", + "127.0.0.2", + "[::1]", + ] { + assert!(is_public_bind_host(host), "{host} must be public"); + } + } + #[test] fn request_roundtrip_preserves_method() { let envelope = EngineRequestEnvelope { diff --git a/docs/testing.md b/docs/testing.md index d41a9fbb..f69d9949 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -426,8 +426,14 @@ cargo test -p rocm --bin rocm resolve_endpoint_auth cargo test -p rocm --bin rocm endpoint_client_config cargo test -p rocm --bin rocm api_key_client_config cargo test -p rocm --bin rocm public_bind_fails_closed +cargo test -p rocm --bin rocm respawn_fails_closed +cargo test -p rocm --bin rocm respawn_allows_loopback +cargo test -p rocm --bin rocm restart_refuses_a_public_service cargo test -p rocm-core generate_endpoint_api_key cargo test -p rocm-engine-protocol endpoint_api_key +cargo test -p rocm-engine-protocol is_public_bind_host +cargo test -p rocmd recovery_respawn_fails_closed +cargo test -p rocmd apply_endpoint_key_env ``` A loopback bind (`rocm serve `, default `127.0.0.1`) stays @@ -441,7 +447,18 @@ audit log. Verifying that the running server actually *rejects* an unauthenticated request requires a live engine; that end-to-end assertion is a deferred follow-up and is **not yet** exercised by the GPU acceptance scripts (`scripts/vllm_therock_gpu_test.py`). The unit tests cover key policy, -generation, the 0600 key file, key-file resolution, and cleanup on stop. +generation, the 0600 key file, and key-file resolution. + +The same requirement holds across respawns, because the invariant belongs to the +recorded *host*, not to the key file: a service recorded on a public host refuses +to respawn once its key is gone, rather than coming back unauthenticated. That +covers `rocm services restart` and `rocmd`'s recovery supervisor, and the refusal +happens before anything is stopped, so it cannot take down a running service. A +stop therefore drops the key only once it has confirmed the engine is gone — +otherwise the CLI would lock itself out of a service that is still running and +still enforcing the key — and the deferred cleanup lands on the liveness refresh +that later observes the process dead. There is no e2e coverage of `services +stop`/`restart` or endpoint auth; these paths are unit-tested only. Windows + Lemonade note: the Windows *managed* native-Lemonade server is launched via `spawn_hidden_console_with_log`, whose env-override API is path-valued only,