[service] Bearer HTTP auth for admin/debug HTTP services - #202
Conversation
Add RFC 6750 Bearer authentication to the Admin and Debug HTTP services only. The Meta HTTP service stays unauthenticated (it is the data-plane endpoint used by inference engines, and is expected to be network-isolated).
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Adds RFC 6750 Bearer auth as middleware on the admin/debug HTTP services, with a TokenVerifier interface, a StaticBearerTokenVerifier, constant-time comparison, and config plumbing for a comma-separated token list. The scope is well bounded and the parsing/verifier code has good test coverage.
Key observations
- Plaintext channel: Bearer tokens flow over unauthenticated HTTP — RFC 6750 §5.3 mandates TLS. Either add TLS or document/log this loudly so operators don't deploy it on untrusted networks under a false sense of security.
- No token strength / format validation: weak operator-chosen tokens are silently accepted; tokens cannot contain
,and there's no escape, which can silently split valid tokens. - Secret hygiene in process memory: tokens live in plain
std::vector<std::string>; consider zeroize-on-destruction. - Auth-denied logging is unbounded per failing request — easy log flood under brute force, and there's no metric for alerting.
- Minor:
ConstantTimeEqualsdoc comment doesn't quite match the implementation; one branch inStaticBearerTokenVerifier::Verifyis unreachable given prior trimming.
Inline comments left on the relevant lines.
🤖 Generated by Qoder
Add an Admin service endpoint that lets operators set / rotate / inspect `kvcm.service.admin_auth_token` at runtime, without restarting the server. Rotation includes the open → enforcing transition: if the server started with no tokens, the operator can call the endpoint to lock admin/debug down without a restart. Behavioural Decisions 1. **Online lockdown**: yes. The auth wrapper is always installed on admin/debug; an empty token list means "open mode" and is wired through the verifier itself. First non-empty `Set` flips the service to enforcing. 2. **Transport**: both gRPC and HTTP, mirroring `UpdateLogger` / `UpdateLeaderElectorConfig`. 3. **Persistence**: ephemeral — no Registry storage write; matches existing runtime-only knobs. Operators are responsible for editing the config file for durability. 4. **Inspection**: companion list endpoint returns count + per-token non-reversible fingerprint, never the raw tokens. API Surface HTTP routes (Admin port 6492): - `POST /api/setAdminAuthTokens` - `POST /api/rotateAdminAuthToken` - `POST /api/listAdminAuthTokens` These three endpoints are themselves served by the Admin HTTP service, so they are auth-protected by the same Bearer middleware. The implication is intentional: an unauthenticated caller cannot lock themselves out, and a caller with the *current* token can install a new one and revoke the old one. `RotateAdminAuthToken` semantics A convenience over `Set`: | `old_token` | `new_token` | Effect | |---|---|---| | empty | non-empty | append `new_token` (additive) | | non-empty (matches) | non-empty | append `new_token`, remove `old_token` | | non-empty (no match) | any | `INVALID_ARGUMENT` | | any | empty | `INVALID_ARGUMENT` | This keeps the typical add-new-then-drop-old flow atomic and avoids the gap where `Set([new])` would be served by a node whose own caller is still using the old token. Fingerprint 8 hex chars of FNV-1a over the raw token bytes. Non-cryptographic but collision-resistant enough for human eyeballing, and revealing it to holders of the admin token leaks negligible information given the hash truncation.
There was a problem hiding this comment.
Review Summary
Incremental observations on top of the prior review (which already covered TLS, weak-token validation, secret hygiene, denial-log flooding, and the comment/dead-branch nits — author has acknowledged those as follow-ups):
- Non-atomic rotation:
RotateAdminAuthTokendoes aSnapshot → mutate → SetTokenssequence with a lock gap; concurrentRotate/Setcalls can lose updates despite the doc calling this "atomic". - Doc / port mismatch: the Debug HTTP service does not run on
6492; it ismeta_http_port + 3000(server.cc:267). The English and Chinese docs both list it as6492, which will mislead operators on firewalling/scrape config. - Header lookup case sensitivity:
req.get_header_value("Authorization")may miss lowercasedauthorizationheaders (HTTP/2 mandates lowercase); worth verifyingcoro_httpdoes case-insensitive lookup or normalising explicitly. - No de-duplication in
SetAdminAuthTokens: duplicates inflatetoken_countand the per-request compare loop, asymmetric to the dedupRotateAdminAuthTokenalready does.
Inline comments on the relevant lines.
🤖 Generated by Qoder
🤖 Generated by Qoder
| KVCacheManager exposes three HTTP services on two ports: | ||
|
|
||
| | Service | Default Port | Authentication | | ||
| |---|---|---| | ||
| | Meta | 6382 | always open (data plane) | | ||
| | Admin | 6492 | optional Bearer token | | ||
| | Debug | 6492 | optional Bearer token | |
There was a problem hiding this comment.
The port table is misleading: kv_cache_manager/service/server.cc:267 computes the Debug HTTP port as meta_http_port + 3000 (i.e. 6382 + 3000 = 9382 by default), not 6492. The Admin port (default 6492) and the Debug port are different services on different ports — they only happen to share the same auth verifier. Operators following this doc to scrape Debug on :6492 (or to firewall/route Debug traffic) will hit the wrong port. Suggest either (a) correcting the table to show the actual default Debug port and that it is derived from kvcm.service.http_port, or (b) introducing a dedicated kvcm.service.debug_http_port and using it consistently. The Chinese doc has the same issue.
🤖 Generated by Qoder
🤖 Generated by Qoder
| auto verifier = token_verifier_; | ||
| return [api, handler, verifier](coro_http::coro_http_request &req, | ||
| coro_http::coro_http_response &res) -> async_simple::coro::Lazy<void> { | ||
| auto authz = req.get_header_value("Authorization"); |
There was a problem hiding this comment.
HTTP header field names are case-insensitive (RFC 9110 §5.1), but req.get_header_value("Authorization") looks up by an exact-case key. If the underlying coro_http request stores headers as transmitted on the wire (some clients/proxies send authorization lowercased; HTTP/2 mandates lowercase field names), this lookup will silently miss and return empty, which the verifier then maps to kMissingCredentials — i.e. a perfectly well-formed HTTP/2 request with a valid Bearer token would be rejected as "no credentials". Worth verifying the lookup is case-insensitive (and adding a unit/integration test for authorization: Bearer … lowercased) or normalising the lookup explicitly.
🤖 Generated by Qoder
🤖 Generated by Qoder
| std::vector<std::string> tokens(request->tokens().begin(), request->tokens().end()); | ||
| // drop empty entries to match config-file parsing semantics | ||
| tokens.erase(std::remove_if(tokens.begin(), tokens.end(), [](const std::string &s) { return s.empty(); }), | ||
| tokens.end()); |
There was a problem hiding this comment.
SetAdminAuthTokens does not de-duplicate the request list. An operator submitting ["alpha","alpha"] ends up with two identical tokens in tokens_, which inflates the token_count and fingerprints reported by ListAdminAuthTokens and (combined with the fixed-list Verify loop) causes a needless extra constant-time compare per request. RotateAdminAuthToken already explicitly skips appending an existing new_token (line 1059–1061) — applying the same dedup here keeps the two RPCs symmetric and avoids confusing operator-facing output. A simple std::sort + std::unique (or a std::set pass) before SetTokens would suffice.
🤖 Generated by Qoder
🤖 Generated by Qoder
| cur.push_back(request->new_token()); | ||
| } | ||
| const std::size_t count = cur.size(); | ||
| token_verifier_->SetTokens(std::move(cur)); |
There was a problem hiding this comment.
RotateAdminAuthToken does a non-atomic read-modify-write on the verifier: SnapshotTokens() releases its shared lock, the local cur is mutated, then SetTokens(std::move(cur)) takes a fresh exclusive lock. If two operators (or scripts) call RotateAdminAuthToken concurrently — or one calls SetAdminAuthTokens while another is rotating — one of the writes silently overwrites the other's mutation, contradicting the doc claim that it is an "atomic add-then-drop" (docs/http-auth-en_US.md §Rotate). Two options: (a) extend StaticBearerTokenVerifier with a write-locked Mutate(fn) / atomic RotateTokens(old,new) helper that does the diff under the unique lock; or (b) document that the runtime token RPCs are not safe under concurrent operators and rely on operator coordination.
🤖 Generated by Qoder
🤖 Generated by Qoder
1 participant
Add RFC 6750 Bearer authentication to the Admin and Debug HTTP services only. The Meta HTTP service stays unauthenticated (it is the data-plane endpoint used by inference engines, and is expected to be network-isolated).
Add an Admin service endpoint that lets operators set / rotate / inspect
kvcm.service.admin_auth_tokenat runtime, without restarting the server.