feat(api): Basic auth + read/write roles on the management API (S-1)#20
Merged
Conversation
The control-plane HTTP API had no authentication: anyone who could reach the bind address could replace the global config, create/delete streams, upload files, and read secrets (audit S-1, CRITICAL). Add HTTP Basic auth with two roles — read (GET/HEAD/OPTIONS) and write (all methods) — gating every management route. - Users live in the global config (auth.api.users) as bcrypt password_hash + role, managed like any other config and hot-reloaded (runtime diff → Server.SetAuthConfig, atomic-pointer swap; no restart). - New internal/api/apiauth.go: Auth.Middleware, bcrypt verify with a dummy-hash timing-safe unknown-user path, roleAllowsMethod. All admin routes are wrapped in an auth Group in buildRouter; /healthz, /readyz, /metrics and the media catch-all stay public. - Opt-in: enabled=false (default) is a pass-through, so existing deployments keep working until an operator turns it on. Fail-closed bootstrap: enabled + no users seeds an admin/write account from OPEN_STREAMER_API_ADMIN_PASSWORD, otherwise every request is rejected. - `open-streamer hashpw [password]` prints a bcrypt hash so plaintext never enters the config. Adversarial review found and this fixes a routing bypass: flat admin routes let an unregistered HTTP method fall through chi's trie to the unauthenticated media catch-all. /config* and /streams are now subrouters that return 405 in-group; regression test TestAdminRouteUnmatchedMethodDoesNotBypassAuth. Stream restart/switch are POST-gated so read users can't trigger mutations via GET. This is the control-plane half of S-1. Out of scope (separate findings): the non-mutating client-IP extractor (RealIP → S-15/S-17), media/playback auth, and redacting password_hash/secrets from GET /config responses (folded into S-9 secret redaction). Tests: TestAPIAuth_* (disabled pass-through, creds, read-cannot-write, bootstrap, fail-closed, invalid-user skip, hot-reload), TestRoleAllowsMethod, TestAdminRouteUnmatchedMethodDoesNotBypassAuth.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (audit S-1, CRITICAL)
The control-plane HTTP API had no authentication — anyone who could reach the bind address could replace the global config, create/delete streams, upload files, and read secrets. This is the enabling precondition for several other findings.
Fix — admin-plane auth (the control-plane half of S-1)
HTTP Basic auth with two roles:
read(GET/HEAD/OPTIONS) andwrite(all methods), gating every management route.auth.api.users):username+ bcryptpassword_hash+role. Managed like any other config and hot-reloaded (runtime.Manager.diff→Server.SetAuthConfig,atomic.Pointerswap — no restart).internal/api/apiauth.go:Auth.Middleware, bcrypt verify with a dummy-hash timing-safe unknown-user path,roleAllowsMethod. All admin routes wrapped in an authGroup;/healthz,/readyz,/metricsand the media catch-all stay public.enabled: false(default) is a pass-through, so existing deployments keep working until an operator turns it on. Fail-closed bootstrap: enabled + no users seedsadmin/writefromOPEN_STREAMER_API_ADMIN_PASSWORD, else every request is rejected.open-streamer hashpw [password]prints a bcrypt hash so plaintext never enters the config.Adversarial review → bypass found & fixed
A review found a routing bypass: flat admin routes let an unregistered HTTP method fall through chi's trie to the unauthenticated media catch-all.
/config*and/streamsare now subrouters that return 405 in-group (regression testTestAdminRouteUnmatchedMethodDoesNotBypassAuth). Confirmed no GET-triggered mutation escalation — streamrestart/switchare POST-gated.Scope / follow-ups (not in this PR)
RealIP→ S-15/S-17).password_hash/secrets fromGET /config+/config/yamlforreadusers — folded into S-9 (comprehensive secret redaction with round-trip preservation). Note: these are bcrypt hashes (offline-crack-only), behind auth once enabled.Test
TestAPIAuth_*(disabled pass-through, creds/401, read-cannot-write/403, bootstrap, fail-closed, invalid-user skip, hot-reload),TestRoleAllowsMethod,TestAdminRouteUnmatchedMethodDoesNotBypassAuth.go test -race ./internal/api/...green,golangci-lint0 issues, fullgo build ./...green.Usage