Implement HTTP Authentication via admin-key. #540
Conversation
- Implement Authorization header (HTTP basic authentication) - Is additive. App remains open as is if not set. - This protects the /, UI pages and API endpoints. - /v1 are as is (Governance API keys for those) - Metrics endpoint is left as is. - Documentation page added in.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds optional admin authentication using HTTP Basic Auth to the bifrost-http UI and admin API endpoints, configurable via CLI flag, environment variable, or config file. Extends client configuration with an AdminKey field and sets its default. Updates Makefile targets to split UI-inclusive build and Go-only build. Adds admin auth documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User/Browser
participant S as bifrost-http Server
participant M as adminAuthMiddleware
participant H as Admin/UI Handlers
participant C as Config (CLI/ENV/File)
rect rgb(245,248,255)
note over S,C: Startup configuration
S->>C: Load config.json
S->>C: Read ENV ADMIN_KEY
S->>C: Parse --adminkey flag
C-->>S: Effective AdminKey (flag > env > file)
S->>S: Register routes (UI, /api/*) via M
end
rect rgb(240,255,245)
note over U,S: Request to admin/UI endpoint
U->>S: HTTP GET /ui or /api/...
S->>M: Pass request
alt AdminKey configured
M->>M: Validate Authorization: Basic
alt Valid (user=admin, pass=AdminKey)
M-->>H: Next handler
H-->>U: 200 OK
else Invalid/missing
M-->>U: 401 Unauthorized + WWW-Authenticate
end
else No AdminKey configured
M-->>H: Next handler
H-->>U: 200 OK
end
end
sequenceDiagram
autonumber
participant U as Client
participant S as bifrost-http
participant M as adminAuthMiddleware
participant T as MCP Tool Exec (/v1/mcp/tool/execute)
U->>S: POST /v1/mcp/tool/execute
S->>M: Check admin auth
alt Authorized or no AdminKey
M-->>T: Execute tool
T-->>U: 200/Result
else Unauthorized
M-->>U: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)docs/features/admin-authentication.mdx(1 hunks)framework/configstore/clientconfig.go(1 hunks)transports/bifrost-http/lib/config.go(1 hunks)transports/bifrost-http/main.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
transports/bifrost-http/main.go (3)
transports/bifrost-http/lib/config.go (1)
Config(114-137)framework/configstore/clientconfig.go (1)
ClientConfig(28-39)transports/bifrost-http/handlers/utils.go (1)
SendError(27-36)
🪛 Gitleaks (8.28.0)
docs/features/admin-authentication.mdx
[high] 126-132: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 120-122: Discovered a potential basic authorization token provided in a curl command, which could compromise the curl accessed resource.
(curl-auth-user)
[high] 175-178: Discovered a potential basic authorization token provided in a curl command, which could compromise the curl accessed resource.
(curl-auth-user)
[high] 182-186: Discovered a potential basic authorization token provided in a curl command, which could compromise the curl accessed resource.
(curl-auth-user)
| // Register public endpoints (no auth required) | ||
| completionHandler.RegisterRoutes(r) | ||
| mcpHandler.RegisterRoutes(r) | ||
| integrationHandler.RegisterRoutes(r) | ||
| configHandler.RegisterRoutes(r) | ||
| pluginsHandler.RegisterRoutes(r) | ||
| if cacheHandler != nil { | ||
| cacheHandler.RegisterRoutes(r) | ||
| } | ||
| if governanceHandler != nil { | ||
| governanceHandler.RegisterRoutes(r) | ||
| } | ||
| if loggingHandler != nil { | ||
| loggingHandler.RegisterRoutes(r) | ||
| } | ||
| if wsHandler != nil { | ||
| wsHandler.RegisterRoutes(r) | ||
| } | ||
|
|
||
| // Register MCP tool execution endpoint (public) - this needs to be separate from admin routes | ||
| r.POST("/v1/mcp/tool/execute", func(ctx *fasthttp.RequestCtx) { | ||
| // Create a temporary router just for this handler | ||
| tempRouter := router.New() | ||
| mcpHandler.RegisterRoutes(tempRouter) | ||
| tempRouter.Handler(ctx) | ||
| }) | ||
|
|
||
| // Apply admin authentication middleware to all /api/* routes | ||
| r.ANY("/api/{path:*}", adminAuthMiddleware(config, func(ctx *fasthttp.RequestCtx) { | ||
| // Create admin router and register all admin handlers | ||
| adminRouter := router.New() | ||
|
|
||
| providerHandler.RegisterRoutes(adminRouter) | ||
| configHandler.RegisterRoutes(adminRouter) | ||
| pluginsHandler.RegisterRoutes(adminRouter) | ||
| mcpHandler.RegisterRoutes(adminRouter) // This includes the admin MCP routes | ||
|
|
||
| if cacheHandler != nil { | ||
| cacheHandler.RegisterRoutes(adminRouter) | ||
| } | ||
| if governanceHandler != nil { | ||
| governanceHandler.RegisterRoutes(adminRouter) | ||
| } | ||
| if loggingHandler != nil { | ||
| loggingHandler.RegisterRoutes(adminRouter) | ||
| } | ||
| if wsHandler != nil { | ||
| wsHandler.RegisterRoutes(adminRouter) | ||
| } | ||
|
|
||
| // Handle the request with the admin router | ||
| adminRouter.Handler(ctx) | ||
| })) |
There was a problem hiding this comment.
Avoid rebuilding routers on every request
The /api/{path:*} handler instantiates a fresh router.New() and re-registers every admin route for each incoming call. That alloc/allocation/reg path is O(number of handlers) per request and will hammer latency/CPU under load. Please register the admin router once during startup, wrap it with adminAuthMiddleware, and reuse the resulting handler instead of reconstructing the routing tree on every request.
|
thanks for pr @anilgulecha ❤️ . Can you check the code rabbit comments. Ill review it today |
| flag.StringVar(&port, "port", DefaultPort, "Port to run the server on") | ||
| flag.StringVar(&host, "host", defaultHost, "Host to bind the server to (default: localhost, override with BIFROST_HOST env var)") | ||
| flag.StringVar(&appDir, "app-dir", DefaultAppDir, "Application data directory (contains config.json and logs)") | ||
| flag.StringVar(&adminKey, "adminkey", "", "Admin authentication key for protected endpoints (can also be set via ADMIN_KEY env var)") |
There was a problem hiding this comment.
we are discouraging adding more command line flags (as it could pollute overall init command). can we remove it please
| @rm -rf ui/.next | ||
| @cd ui && npm run build && npm run copy-build | ||
|
|
||
| build: build-ui ## Build bifrost-http binary |
There was a problem hiding this comment.
any reason to add this as a separate recipe?
There was a problem hiding this comment.
Every build was unnecessarily taking longer (the UI build would occur, with 0 changes in code). Was getting a bit annoying so make a golang build only subcommand.
There was a problem hiding this comment.
during development you can just use
make dev
that runs both UI and Go code in dev mode.
This is used in our CI/CD pipeline (make build). We cant remove this
|
@anilgulecha would you be able to pick these feedback pointers? |
|
closing this due to inactivity - core team has picked it up #726 |
2 participants
Fixes #518