Repository navigation
Replies: 1 comment
|
Closing this myself. Thanks for moving it to Ideas, and sorry for the noise.
Disclosure: drafted with AI assistance and posted by the account holder. |
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Problem
The Management API has a single credential class: the full management key (
management.secret-key). It authorizes everything, including config writes, credential upload/delete, plugin install/delete, quota resets, andapi-call(which executes upstream requests using stored credentials).Tooling that only needs to observe state — credential metadata, quota/usage snapshots, plugin quota — must therefore hold the full write key. Promising to "only ever send GET requests" on the client side doesn't reduce the blast radius if that tooling is buggy or compromised: the server will happily accept writes from it.
A related operational issue: failed management authentication is counted per source IP (5 failures → 30-minute ban in
AuthenticateManagementKey). A monitoring loop running on the same host with a mistyped key can lock the operator out of real management.Proposal
Add an optional second management credential scoped to read-only operations, for example
management.read-only-key(name/shape entirely up to you):secret-key(plaintext in config, hashed on startup). Empty/unset = disabled, and current behaviour is unchanged.internal/api/handlers/management/handler.go). No changes to/v0/managementendpoints themselves — enforcement would live in the middleware both route groups already share, so the read-only scope can't be bypassed via the other version prefix.Why an allow-list, not "GET only"
GETis not synonymous with safe in this API, so a verb check alone is not sufficient:GET /v8/management/credentials/downloadreturns credential file contents.GET /v8/management/config,/config.yamland/config/*pathreturn the full configuration (and perdocs/management-api-v8.md, the YAML form includes values the JSON form redacts).GET /v8/management/oauth/auth-urlstarts a login session — a state change via GET.Those should stay denied for the read-only scope. Conversely, read-shaped
POSTs that trigger upstream calls or mutate state (quota fetch, plugin quota fetch,credentials/verify,api-call) should also stay denied by default.Suggested initial allowed set (v8 paths), open to adjustment:
GET /credentials,GET /credentials/models,GET /plugins,GET /plugins/:id/quota,GET /observability/usage/api-keys,GET /observability/usage/queue,GET /routing/model-definitions/:channel,GET /server/latest-version. I'd default-deny logs and config reads as well, but that's your call. Plugin-registered management routes (thepluginManagementNoRoutefallback) would be denied for this scope unless there's a reason to do otherwise.Enforcement detail worth deciding up front: match on the registered route template (
c.FullPath()) plus method, not on raw URL prefixes, so wildcard/alias paths can't smuggle a write past the check.Open design questions
secret-keyas today.management.read-only-keyas a sibling ofsecret-key, or a more general per-key scope list if you anticipate more classes later? I'd keep it to the single optional key (KISS) unless you want the general form.Relationship to #6972
#6972 asked for an external-scheduler mode (client API keys reading management endpoints, strict AuthID dispatch, bypassing internal scheduling) and was closed as not planned in favour of the Home control plane. This request is deliberately much narrower: no scheduling delegation, no client-key reuse, no dispatch semantics — just a second credential class for the existing Management API, for operators who run management locally (Home mode disables the local management endpoints, so it doesn't cover local read-only monitoring). Happy to be told it's still unwanted, of course.
Acceptance sketch
read-only-keyunset, behaviour is identical to today.gofmt, compile check, andgo test ./...pass, perAGENTS.md.If the direction is acceptable, I'm happy to implement it as a small PR scoped to the config type, the management auth middleware, and tests, and to adjust the allow-list/semantics to your preferences first. If you'd rather not add a second credential class at all, no hard feelings — close away.
Disclosure: this issue was drafted with AI assistance and is being submitted by the account holder, who will own any follow-up PR.
All reactions