[security] fix(proxy): require auth for node management#4579
[security] fix(proxy): require auth for node management#4579Hinotoi-agent wants to merge 1 commit into
Conversation
| '/health', # Health check endpoints | ||
| '/docs', # Swagger UI documentation | ||
| '/redoc', # ReDoc documentation | ||
| '/nodes', # Endpoints about node operation between proxy and api_server |
There was a problem hiding this comment.
These node management endpoints are only intended for internal communication between the api_server and the proxy. In that context, do we really need to enforce authentication on them?
There was a problem hiding this comment.
Thanks for raising this. I agree these endpoints are intended for api_server ↔ proxy coordination, but today that is only an architectural convention, not an enforced boundary.
The proxy defaults to binding on 0.0.0.0, and when api_keys are configured the public inference routes are protected while /nodes/* was still skipped by auth. If the proxy port is reachable, an unauthenticated caller can:
- read
/nodes/statusto learn the current node URLs/models; - call
/nodes/removeor/nodes/terminate*to disrupt serving; and - call
/nodes/addwith a non-emptystatus.models, whichNodeManager.add()accepts directly without first querying the backend.
That last case lets an attacker register their own backend for an existing model and potentially receive subsequent inference requests. Since the proxy forwards the original request headers to the selected backend, that can also expose client request metadata/authorization headers to the malicious node.
So the issue is exploitable whenever the proxy listener is reachable outside the trusted internal network. If deployments enforce isolation externally (localhost/private listener/firewall/service mesh), the risk is reduced, but when operators configure api_keys on a network-reachable proxy, leaving /nodes/* unauthenticated creates a separate unauthenticated management plane.
This patch keeps unauthenticated behavior for deployments without api_keys, and for authenticated deployments it makes the existing api_server registration path send the configured bearer token so the intended internal flow still works.
Summary
This PR hardens the proxy node-management boundary so
/nodes/*routes are no longer exempt from bearer-token authentication when proxy API keys are configured./nodesauthentication bypass fromAuthenticationMiddleware.api_keysvalues before middleware setup so a single configured key is treated as one token, not as an iterable of characters.Security issues covered
Before this PR
AuthenticationMiddlewareskipped every path with the/nodesprefix./nodes/add,/nodes/remove,/nodes/terminate, and/nodes/statuswere reachable without a bearer token when proxy API keys were enabled.status.modelslist, causing the proxy to route matching model requests to the supplied URL.After this PR
/health,/docs,/redoc) bypass bearer-token auth./nodes/*routes now require the configured bearer token whenever proxy API keys are enabled.Why this matters
The proxy node registry is a model-serving control plane. It decides which backend receives
/v1/chat/completionsand/v1/completionstraffic. If unauthenticated callers can mutate that registry, they can steer prompts, generation parameters, and proxied request headers to an arbitrary backend, tamper with completions, or disrupt service by removing/terminating legitimate nodes.How this differs from related PRs
PR #4338 introduced the shared authentication middleware and documented the
/nodesskip prefix as an operational exception for proxy-to-node coordination. This PR is a follow-up hardening change for the management-route trust boundary: it keeps health/docs public, but does not treat mutating node-management endpoints as public coordination traffic.PR #4354 is an open proxy refactor. This PR is intentionally narrow and applies to current
main: it fixes the authentication boundary in the existing middleware and adds a focused regression test for the route behavior.Attack flow
Affected code
lmdeploy/serve/utils/server_utils.py,lmdeploy/serve/proxy/proxy.py,lmdeploy/serve/openai/api_server.pyRoot cause
Issue: proxy node-management routes bypass API-key auth
/nodesprefix.CVSS assessment
CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:HRationale:
Safe reproduction steps
POST /nodes/addwith a JSON body containing a backend URL andstatus.modelsfor an existing model./nodes/statuswithout a bearer token./nodes/addand/nodes/statusreturn401; the same requests succeed withAuthorization: Bearer <configured-key>.The included regression test reproduces this boundary locally with FastAPI
TestClientand the realAuthenticationMiddlewareimplementation.Expected vulnerable behavior
/nodes/addreturns success and mutates the node registry./nodes/statusdiscloses registered backend nodes.Changes in this PR
/nodesfrom the authentication middleware skip-prefix list./health,/docs, and/redocpublic as passive endpoints.api_keysvalues in proxy setup before creatingAuthenticationMiddleware./nodes/*routes and public/healthbehavior.Files changed
lmdeploy/serve/utils/server_utils.py/nodesfrom unauthenticated skip prefixeslmdeploy/serve/proxy/proxy.pyapi_keysbefore middleware creationlmdeploy/serve/openai/api_server.pytests/test_lmdeploy/serve/test_authentication_middleware.pyMaintainer impact
--api-keysare unchanged.Fix rationale
Node registration, removal, and termination are privileged control-plane operations. They should not share the same unauthenticated treatment as health checks or documentation routes. The narrowest durable boundary is to keep passive endpoints public and require bearer-token authentication for all node-management routes when API keys are configured.
Type of change
Test plan
AuthenticationMiddlewareroute behavior.pre-commitwas not run locally becausepre-commitis not installed in this environment.Executed with:
Result:
Token usage
Disclosure notes
This PR is bounded to the proxy node-management authentication boundary. It does not claim to change model execution behavior, prompt handling, or the broader proxy refactor in #4354. No unrelated files were intentionally changed.