Authenticate public inference endpoints - #124
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Review findings against head P1: restart drops authentication
Preserve/read the key before stopping, or split restart-safe process termination from terminal key cleanup. Add a regression test proving a public service retains the same key across restart and removes it only on a final stop. P1: protected Lemonade fallback fails its own probesThe direct llama-server fallback enables Thread the resolved key into both probes and send P1: local chat cannot discover protected services
Make service discovery readiness key-aware, or select the manifest first and perform an authenticated probe. Add a protected-service local-chat discovery test. Current checks
Verdict: not ready. Fix order: preserve restart authentication, authenticate Lemonade fallback probes, authenticate local service discovery, add regressions, then clear required checks. |
|
Thanks for the thorough review — all three P1s were real. Fixed in P1 restart drops authentication — P1 protected engine probes fail themselves — the readiness/smoke probes were anonymous. Made the shared path auth-aware ( P1 local chat cannot discover protected services — Regression test — added Hygiene
Full workspace tests pass and |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Automated review: requesting changes.
The overall authentication design is sound, but one secret-lifecycle defect should be fixed before merge: engines/lemonade/src/lib.rs:1863 writes .llama-server-api-key for the packaged llama-server path, but no stop path removes that file. The sibling <service_id>.endpoint-key is cleared on stop, and the endpoint-key module documents that lifecycle. As written, a stopped or rotated endpoint leaves the old plaintext key on disk indefinitely. Please remove the llama-server key copy during service teardown and add a regression test for cleanup.
Additional coverage strongly recommended for this security boundary:
- Assert that
VLLM_API_KEY,LEMONADE_API_KEY, and--api-key-filereach the spawned engine commands. - Verify the key is absent from service, log, and audit output.
- Verify the private key files have mode 0600 on Unix.
The CodeQL path-injection alerts appear false-positive: the production path comes from ROCM_SERVE_API_KEY_FILE, which the CLI sets only for its own child process from the service directory; the other alerts are test temporary paths. These still need dismissal or narrow suppression so the gate is green.
The branch also conflicts with current main in apps/rocm/src/main.rs and engines/lemonade/src/lib.rs; please rebase before merge.
Minor accuracy notes: one comment says “keychain” although this feature reads a private file, and docs/testing.md defers live auth rejection to GPU scripts that currently contain no authentication check.
Public (non-loopback) `rocm serve` binds now require an API key instead of launching an anonymous, network-reachable server. Loopback stays credential-free (unchanged default). The key is taken from --api-key / ROCM_SERVE_API_KEY or generated, then handed to the engine off-argv: vLLM via VLLM_API_KEY, Lemonade via LEMONADE_API_KEY, and the packaged llama-server fallback via --api-key-file. The CLI's own readiness probe, smoke test, and local chat send it as a bearer token so they keep working against the now-protected endpoint. The key is printed once as client configuration and never appears in logs, `services`, or audit output. Persistence is a 0600 per-service file rather than the OS keychain: public serving is a headless-server action and those hosts routinely lack a Secret Service/D-Bus session. Windows managed Lemonade cannot enforce the key through the platform's path-only spawn primitive, so that combination fails closed. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Address review of the public-endpoint auth change: - Restart preserved authentication: `restart_internal_managed_service` read the key file only after `stop` had deleted it, so a public service came back unauthenticated. Capture the key before stop and re-store it before respawn. - Protected engine probes no longer self-reject: the CLI's readiness and smoke probes issued anonymous requests, so a correctly protected server answered 401 and was treated as failed/killed. Thread the key through the shared `http_get_text_with_auth` / `openai_models_endpoint_has_model` / `managed_service_endpoint_model_ready` helpers and the Lemonade fallback readiness + chat-smoke probes and the vLLM healthcheck. - Local service discovery is key-aware: `ready_local_services` and the managed-service liveness refresh now probe with the service key, so protected public services are discoverable by `rocm chat` instead of being filtered out. - docs/testing.md no longer claims keychain storage (it is a 0600 file). Adds a rocm-core regression test proving the readiness probe sends the bearer token (401 without the key, ready with it). Formatting brought to `cargo fmt`. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The packaged llama-server fallback wrote a second copy of the endpoint API key (.llama-server-api-key) to pass via --api-key-file, but no stop path removed it — leaving a plaintext key on disk after a service stopped or its key rotated. Point --api-key-file at the existing CLI-managed 0600 key file (ROCM_SERVE_API_KEY_FILE) instead of copying the secret, so the key's lifecycle stays owned by `rocm serve` (created before spawn, deleted on stop) with no stale copy left behind. Add resolve_endpoint_api_key_file() with an env-free, testable gate, and cover key-file resolution, the 0600 mode, and store/clear cleanup. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The smoke-test comment said the local provider reads the endpoint key from the OS keychain; it reads the per-service 0600 key file. And docs/testing.md claimed the GPU acceptance script asserts rejection of unauthenticated requests, but that script has no such check — mark it a deferred follow-up and list the coverage the unit tests actually provide. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
84edba5 to
cb6e931
Compare
CodeQL
|
| Alert | Location | Flagged op | Why it's safe |
|---|---|---|---|
| #702 | crates/rocm-engine-protocol/src/lib.rs:73 |
read_to_string in endpoint_api_key_from_file |
Path is ROCM_SERVE_API_KEY_FILE, which the CLI sets only on its own __engine-serve-http child, pointing at a 0600 file under the app services dir. Never external input. |
| #718 | apps/rocm/src/endpoint_keys.rs:56 |
remove_file in clear_endpoint_api_key |
Path = services_dir/{service_id}.endpoint-key. |
| #724 / #729 | apps/rocm/src/main.rs:4385,4395 |
create_dir_all / open in write_private_file_0600 |
Same service_id-derived path under the services dir. |
service_id cannot traverse the services dir: it is either produced by generate_service_id (sanitize_component(engine) + a sanitized 24-char model slug + unix_time_millis), or — on the user-facing stop / restart --service-id paths — passed through validate_service_id, which rejects / and \.
Test code (15) — used in tests: unit-test temp-dir paths in the rocm-engine-protocol and apps/rocm/endpoint_keys test modules (#703–708, #719–723, #725–728). Not production surface.
Durable follow-up: this query re-fires on any test that touches a path, so per-alert dismissal is a treadmill. A paths-ignore for test modules in the CodeQL config would stop the recurring test-path noise, but that's a repo-wide change and out of scope for this PR.
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
The authentication flow is substantially improved, but the supplied API key still crosses a raw HTTP-header boundary without rejecting embedded CR/LF. Please reject those characters at input validation and add a regression test before merge.
| } | ||
| match supplied { | ||
| Some(raw) => { | ||
| let trimmed = raw.trim(); |
There was a problem hiding this comment.
trim() only removes leading/trailing whitespace; an embedded \r or \n remains in the accepted key. That key is later interpolated directly into raw Authorization headers in the core probe, the local-service probe, and Lemonade's smoke request. For example, a supplied value containing key\r\nX-Injected: value becomes an additional header on those requests. Please reject CR/LF here (and defensively when reading the key file, if appropriate) and add a regression test covering an embedded newline.
There was a problem hiding this comment.
Good catch — fixed in 72efed0.
Root cause confirmed: the key is interpolated verbatim into raw Authorization: Bearer {key}\r\n header lines (core probe, local-service probe, and Lemonade's smoke request), and trim() only strips the ends, so an embedded CR/LF survived and injected an extra header line.
Fix:
- New shared predicate
rocm_core::endpoint_api_key_has_forbidden_charsrejects any control character (covers CR/LF and the whole class). resolve_endpoint_authnow rejects a supplied--api-key/ROCM_SERVE_API_KEYcontaining a control char at input validation, with a clear error.endpoint_api_key_from_filedefensively returnsNonefor a key file holding control chars — defense in depth behind the input validation.
Regression tests at all three sites:
resolve_endpoint_auth_public_rejects_embedded_crlf(coversgood-key\r\nX-Injected: value, bare\n, bare\r)endpoint_api_key_from_file_rejects_embedded_control_charsendpoint_api_key_has_forbidden_chars_flags_control_chars(plus asserting a generated key passes)
cargo clippy -D warnings and cargo fmt --all --check are clean.
The endpoint API key is interpolated verbatim into raw
`Authorization: Bearer {key}\r\n` header lines by the core, local, and
Lemonade probes. `trim()` only strips surrounding whitespace, so a
supplied key carrying an embedded CR/LF (e.g. `key\r\nX-Injected: value`)
survived validation and injected an extra header line on those requests.
Reject any control character at input validation in resolve_endpoint_auth,
and defensively when reading the key file in endpoint_api_key_from_file,
via a shared endpoint_api_key_has_forbidden_chars predicate in rocm-core.
Add regression tests covering embedded newlines at all three sites.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Two issues should be addressed before merge:
-
Daemon recovery can restart a public endpoint without authentication. The initial
rocm servepath persists the endpoint key, butrestart_managed_servicelaunchesrocmd superviseusingrecovery_supervise_args, which carries no endpoint-key information.supervise_servicethen starts the engine withoutROCM_SERVE_API_KEY_FILE. With automation enabled, a managed public service that crashes or enters a recoverable state can therefore be restarted anonymously. The daemon health-check subprocess also receives no endpoint key, so it may classify the protected endpoint as unhealthy and trigger this path. Please propagate the service key into health checks and recovery, fail closed when a public service's key is unavailable, and add a regression test proving recovery preserves authentication. -
The attached
AlreadyRunningpath leaves an orphan endpoint-key file.serve()stores the newly generated key before spawning. The background path clears that unused key when an equivalent service already exists, butrun_attached_servicereturns fromManagedSpawn::AlreadyRunningwithout doing so. Repeated attached public-bind invocations therefore accumulate unused*.endpoint-keyfiles. Please mirror the background cleanup before returning.
I verified the relevant unit tests and cargo clippy --workspace --all-targets -- -D warnings; those passed. The aggregate CodeQL check is currently red even though its individual language-analysis jobs are green, so that check also needs triage before merge.
volen-silo
left a comment
volen-silo
left a comment
There was a problem hiding this comment.
Note: posted as a COMMENT review because GitHub does not permit the PR author to formally REQUEST_CHANGES on their own PR. Intended verdict: REQUEST_CHANGES — the confirmed blockers below must be resolved before merge.
Follow-up review (review-only). I independently re-verified the two blocking concerns from the previous review (#124 (review)) against the current PR head 72efed0, plus their blast radius. No new commits have landed since that review, so both concerns remain open, and the blast-radius pass surfaced two further instances of the same root gap. Requesting changes.
Blocking (confirmed against source)
1. Daemon recovery restarts a public endpoint unauthenticated — CONFIRMED, still open.
The engine obtains the key only from the ROCM_SERVE_API_KEY_FILE env var (crates/rocm-engine-protocol/src/lib.rs:38-56; no service-id-based fallback). The initial spawn sets it (apps/rocm/src/main.rs:4603-4626) and the CLI-side rocm services restart re-threads it (apps/rocm/src/main.rs:12781-12784, 12844-12846). But the daemon's recovery path is a separate function that never does: restart_managed_service → recovery_supervise_args → supervise_service → engine_serve_http_args (apps/rocmd/src/lib.rs:4751, 4796, 3038, 3148) spawn rocm __engine-serve-http with no .env(ENDPOINT_API_KEY_FILE_ENV, …) anywhere. apps/rocmd has zero references to the (crate-private) endpoint_keys module, and ManagedServiceRecord carries no key-file field, so the daemon has no way to re-thread it as written. A Contained-mode server-recover restart therefore brings a previously-protected public service back up anonymous. Reachable via automatic recovery and the agent-exposed restart_server sandbox tool (apps/rocmd/src/lib.rs:593-599).
Fix: persist the key-file path on ManagedServiceRecord (or resolve it deterministically in a shared crate) and set ROCM_SERVE_API_KEY_FILE on the re-spawned child in supervise_service; fail closed when a public service's key is unavailable; add a regression test proving recovery preserves auth.
2. Attached AlreadyRunning leaves an orphan endpoint-key file — CONFIRMED, still open.
serve() stores the fresh key before choosing background vs attached (apps/rocm/src/main.rs:4187-4189). The background path clears it on already_running (4215-4217), but run_attached_service's ManagedSpawn::AlreadyRunning arm returns without clearing (4894-4902). Repeated attached public-bind invocations against an already-running equivalent service accumulate unused *.endpoint-key files.
Fix: mirror the background cleanup in that arm (clear the fresh service_id's key before returning).
3. Daemon stop_managed_service never clears the key file — CONFIRMED (blast radius of #1).
stop_managed_service (apps/rocmd/src/lib.rs:2567-2612) signals PIDs and marks the record stopped but never deletes the endpoint-key file. This backs the stop_server sandbox tool and the MCP handler — stop paths distinct from the CLI rocm services stop (which does clear). Stopping a public service via the daemon/assistant path leaves a stale plaintext key on disk.
Fix: clear the key file on this stop path too (same shared-resolution mechanism as #1).
Resolved since earlier rounds (independently confirmed)
- llama-server key copy: no separate
.llama-server-api-keyis written; the managed 0600 file is reused via--api-key-file(engines/lemonade/src/lib.rs:1754-1761); repo-wide grep for the old filename is empty. - CR/LF / control-char injection: rejected at input (
resolve_endpoint_auth,apps/rocm/src/main.rs:4338-4364) and defensively on file read (crates/rocm-engine-protocol/src/lib.rs:72-84) viaendpoint_api_key_has_forbidden_chars(rejects the full control class), with regression tests at both layers; generated keys are alphanumeric-only. - "keychain" comment: no inaccurate keychain reference remains for this feature.
- docs/testing.md: honestly discloses the live unauth-rejection assertion as a deferred GPU-script gap rather than overclaiming.
- Rebase: branch no longer conflicts with
main.
Non-blocking
apps/rocm/src/provider_keys.rs:261:with_keyring_entrydoc comment implies endpoint keys will reuse the OS-keyring chokepoint, but this feature deliberately uses 0600 files (headless hosts lack Secret Service/D-Bus). Comment-rot worth correcting so a future contributor doesn't route endpoint keys through the keyring.- Test coverage (recommended in round 1, still partial): no test asserts
VLLM_API_KEYreaches the vLLM command env or--api-key-filereaches the llama-server argv, nor an integration test that the key is absent fromservices/logs/audit output. Structurally safe today (ManagedServiceRecordhas no key field) but not pinned by a regression test.
CI
The aggregate CodeQL check is red while Analyze (actions/python/rust) are all green — matches the prior review's triage note; still needs resolving before merge. cargo clippy --workspace --all-targets -- -D warnings is clean locally; endpoint-auth unit tests pass across rocm, rocm-core, rocm-engine-protocol, and both engines. (Two proc_lifecycle tree-stop tests fail locally, but that file is untouched by this PR — environment-sensitive, unrelated.)
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Two secret-lifecycle issues remain at head 5a1ad28 and should be fixed before merge:
-
Daemon stop leaves the endpoint credential on disk.
stop_managed_serviceinapps/rocmd/src/lib.rs:2567-2612terminates the recorded processes and marks the service stopped, but it never removes<service_id>.endpoint-key. This path backs the MCP and assistantstop_serveroperations, so stopping a protected public endpoint through either path leaves its plaintext bearer credential indefinitely. The CLI stop path already performs best-effort cleanup inapps/rocm/src/main.rs:12730-12733; please apply equivalent cleanup here and add a regression test for daemon-driven stop. -
Attached
AlreadyRunningleaves an orphan endpoint-key file.serve()persists the newly generated key before spawning (apps/rocm/src/main.rs:4184-4189). The background path clears that unused key when an equivalent service is already running, butrun_attached_servicereturns fromManagedSpawn::AlreadyRunningatapps/rocm/src/main.rs:4894-4902without clearing it. Repeated attached public-bind invocations therefore accumulate unused credential files that are not associated with any service record and have no later cleanup path. Please mirror the background cleanup in this branch and add a regression test.
I re-ran the endpoint-auth tests and cargo clippy --workspace --all-targets -- -D warnings; both passed. The aggregate CodeQL check is still red and also needs disposition before merge.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
|
On the CodeQL disposition: This branch introduces zero new code-scanning alerts — the three analysis jobs (
The red aggregate
I'll keep the baseline triage out of this PR's scope so the auth fixes stay reviewable on their own. |
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Follow-up review at head 00f4db48: both requested production fixes are correct. Daemon-driven stop now removes the endpoint key, and the attached AlreadyRunning path removes the fresh orphan key. One regression-test issue remains before approval:
The daemon-stop test removes the entire test directory before checking the key file. In stop_managed_service_removes_endpoint_key_file (apps/rocmd/src/lib.rs:7861-7905), fs::remove_dir_all(root) runs at line 7890 before the assertion at lines 7900-7903 that key_path no longer exists. The assertion is therefore true even when the production remove_file call is removed. I confirmed this empirically: reverting the production cleanup still leaves this test passing.
Please move the key-file assertion before remove_dir_all(root) so the requested regression test fails without the fix.
Local formatting, clippy, rocmd library tests, rocm-engine-protocol tests, and the targeted endpoint-key tests pass. Separately, the aggregate CodeQL gate remains red even though its language-analysis jobs pass, and still needs disposition before merge.
stop_managed_service_removes_endpoint_key_file called remove_dir_all on the temp root before asserting the endpoint-key file was gone, so the assertion held even when the production remove_file was absent. The test could not fail for the regression it was written to guard. Capture the key file's absence from the real filesystem immediately after stop and before the blanket cleanup, then assert on that. Verified empirically: reverting the remove_file in stop_managed_service now fails this test, and it passes with the fix in place. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The endpoint-key file path is built as
services_dir().join("{service_id}.endpoint-key"). Three call sites
independently re-checked that a service id carried no path separator
before reaching that join, so the sanitizer was invisible across
functions (CodeQL flags the joins as path-injection).
Introduce a validated rocm_core::ServiceId newtype (rejects empty, path
separators, "..", and control characters) as the single source of
truth, route the three existing checks through it, and assert in
endpoint_key_file_path that the built path is a direct child of the
services directory so a stray id fails closed at the sink rather than
escaping the directory. No behaviour change for valid ids; adds
ServiceId unit tests.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The previous commit routed the local-webhook service_id check through
ServiceId::new but wrapped it in .with_context("invalid service_id ..."),
and anyhow's top-level Display shows only the outer context. That hid the
"must not contain path separators" text that
local_webhook_event_rejects_service_id_path_separators asserts on, failing
the rocmd tests on Linux and Windows.
Propagate the ServiceId error verbatim so its own message surfaces.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
4 participants
Summary
rocm serveno longer launches an anonymous, network-reachable server when bound to a public (non-loopback) interface. Public binds now require an API key; loopback binds stay credential-free (unchanged default).--api-key/ROCM_SERVE_API_KEY, or is generated (48-char CSPRNG). An empty supplied key is rejected.VLLM_API_KEY, Lemonade serverLEMONADE_API_KEY, packaged llama-server fallback--api-key-file.rocm chatsendAuthorization: Bearerso they keep working against the protected endpoint.rocm services,rocm logs, or the audit log.public bind + --engine lemonade + Windowsis refused with guidance to use vLLM or a loopback host.Testing
rocm,rocm-core, androcm-engine-protocol(key policy, generator, key-file reader, client-config rendering, redaction, fail-closed guard).cargo clippy -D warningsclean.docs/testing.md.Follow-up