From a8474f2aabafe58a8d962dc1f1be24ee39fd8b7a Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Thu, 30 Jul 2026 11:28:37 +0000 Subject: [PATCH 1/2] fix(serve): keep a public endpoint authenticated across respawns A public (non-loopback) `rocm serve` bind mints an endpoint API key and persists it in a 0600 file, but every path that *respawns* a recorded service treated the presence of that file as the source of truth. The real invariant is a property of the host, not the file: a non-loopback bind must always be authenticated. Reachable through ordinary commands: rocm serve --host 0.0.0.0 --allow-public-bind # key minted and stored rocm services stop # key file deleted rocm services restart # respawns with NO key `restart_internal_managed_service` captured `None` for the preserved key, stored nothing, and spawned the engine child without ROCM_SERVE_API_KEY_FILE, so the service came back on 0.0.0.0 completely unauthenticated. `rocm services restart` documents its argument as an id from `rocm services list --all`, which includes stopped records, so this is the documented flow rather than an edge case. `rocmd`'s recovery supervisor had the same gap. Enforce the invariant at each spawn site and fail closed, mirroring the existing `ensure_public_bind_engine_supported` precedent: - Add `rocm_engine_protocol::is_public_bind_host` so `rocm` and `rocmd` classify a recorded host identically; `is_loopback_host` now delegates to it. - Refuse to respawn a public service with no key in `restart_internal_managed_service` (before the stop, so a refused restart leaves a running service running), in `spawn_managed_engine_child`, and in `rocmd`'s `supervise_service`. - Stop only drops the key file once termination is confirmed. An unconfirmed stop may have left the engine alive and still enforcing the key, and discarding the only copy locked the CLI's own probes, chat, and service discovery out of a healthy service. Loopback behavior is unchanged: it stays credential-free. Closes EAI-7409. Signed-off-by: Eugene Volen --- apps/rocm/src/main.rs | 115 ++++++++++++++++++++++++- apps/rocmd/src/lib.rs | 70 +++++++++++++-- crates/rocm-engine-protocol/src/lib.rs | 39 +++++++++ 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index b0a3fd13..a5cd4ba9 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<()> { @@ -4637,6 +4671,11 @@ fn spawn_managed_engine_child( // 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); + // `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 +12806,14 @@ fn stop_internal_managed_service(paths: &AppPaths, service_id: &str) -> Result serde_json::json!({ "attempted": true, @@ -12816,6 +12862,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)?; @@ -20579,6 +20628,68 @@ 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:#}" + ); + + 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..32a428cc 100644 --- a/apps/rocmd/src/lib.rs +++ b/apps/rocmd/src/lib.rs @@ -3114,7 +3114,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}"))?; @@ -7017,7 +7021,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 +7036,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)| { @@ -7044,6 +7048,24 @@ mod tests { assert_eq!(env_value, key_path.as_os_str()); } + #[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] fn manifest_recovery_policy_covers_terminal_and_stale_transient_states() { let (root, paths) = temp_app_paths("manifest-recovery-policy"); @@ -8924,13 +8946,42 @@ 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) { +/// 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. +#[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) { 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 +9013,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..47079430 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.internal", + // 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 { From b77c6609333c2577c0bb2fc16e2462c97da22ef5 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Thu, 30 Jul 2026 12:01:16 +0000 Subject: [PATCH 2/2] fix(serve): address review of the respawn auth guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Check key *validity*, not file existence. `endpoint_key_file_if_present` is documented as an existence check only, and the engine adapters resolve the key with `endpoint_api_key_from_file` and enforce nothing when it yields `None`. An empty or malformed key file therefore satisfied the guard at two of its three sites and still produced an unauthenticated public listener — the exact failure the guard exists to prevent. - Do not let a refused recovery kill the daemon. `supervise_service`'s bail propagated through `restart_managed_service` and `evaluate_watchers` to `run_daemon`, which `?`-propagates on both the startup and ticker paths. Since a missing key is a permanent condition, that took down every watcher on each 30s recovery tick. Refuse at the recovery boundary instead, recording a `restart_managed_service_refused` event, mirroring the containment the webhook branch already uses. - Do not orphan the key file. Gating the stop-time clear on confirmed termination left the 0600 secret on disk forever: the liveness refresh that later observes the process dead sets "stopped" but never cleared the key, and no sweep exists. Clear it there, so the key's lifetime tracks confirmed death rather than the stop command. - Refuse before the manifest write in `supervise_service`, so a refused respawn leaves restart_count and timestamps intact. - Delegate rocmd's copied `is_loopback_host` to the shared predicate — a second hand-maintained classification is the divergence hazard this change closes. - Cover the empty-key-file case, record why the ordering assertion holds, and update docs/testing.md with the new invariant and test filters. Known gap, not addressed here: `rocmd`'s own `stop_managed_service` still clears the key unconditionally, because `terminate_process` there reports only that a signal was accepted, so it has no confirmed-termination notion to gate on. Giving it one is a larger change than this fix. Signed-off-by: Eugene Volen --- apps/rocm/src/main.rs | 17 ++++++- apps/rocmd/src/lib.rs | 61 +++++++++++++++++++++++++- crates/rocm-engine-protocol/src/lib.rs | 2 +- docs/testing.md | 19 +++++++- 4 files changed, 94 insertions(+), 5 deletions(-) diff --git a/apps/rocm/src/main.rs b/apps/rocm/src/main.rs index a5cd4ba9..c93826a8 100644 --- a/apps/rocm/src/main.rs +++ b/apps/rocm/src/main.rs @@ -4670,7 +4670,12 @@ 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 @@ -13771,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; @@ -20682,6 +20692,11 @@ install therock"; "{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", diff --git a/apps/rocmd/src/lib.rs b/apps/rocmd/src/lib.rs index 32a428cc..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) @@ -4626,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, @@ -7046,6 +7083,19 @@ 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] @@ -8954,13 +9004,20 @@ fn healthcheck_response_recoverable(response: &HealthcheckResponse) -> bool { /// 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) { + 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; } diff --git a/crates/rocm-engine-protocol/src/lib.rs b/crates/rocm-engine-protocol/src/lib.rs index 47079430..cd299ccd 100644 --- a/crates/rocm-engine-protocol/src/lib.rs +++ b/crates/rocm-engine-protocol/src/lib.rs @@ -691,7 +691,7 @@ mod tests { "0.0.0.0", "::", "192.168.1.10", - "example.internal", + "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. 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,