feat(auth): serve /metrics without auth by default (opt-out via metrics_require_auth)#4537
feat(auth): serve /metrics without auth by default (opt-out via metrics_require_auth)#4537mickgvirtu wants to merge 1 commit into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new ChangesConfigurable /metrics Auth
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| RequiredHeaders []string `json:"required_headers,omitempty"` // Headers that must be present on every request (case-insensitive) | ||
| LoggingHeaders []string `json:"logging_headers,omitempty"` // Headers to capture in log metadata | ||
| WhitelistedRoutes []string `json:"whitelisted_routes,omitempty"` // Routes that bypass auth middleware | ||
| MetricsRequireAuth bool `json:"metrics_require_auth"` // Require auth on the Prometheus /metrics endpoint (default false: /metrics is public, like /health — scrapers can't carry admin auth). Set true to keep /metrics behind the auth middleware. |
There was a problem hiding this comment.
metrics_require_auth missing from config schema
transports/config.schema.json is the declared source of truth for all config fields in this repo. metrics_require_auth is added to ClientConfig but is absent from the schema, so tools that validate or document the config against the schema (e.g., JSON Schema validators, the dashboard config editor, docs generation) will not recognise the field. The entry should appear between whitelisted_routes and hide_deleted_virtual_keys_in_filters, as a boolean with "default": false and a short description matching the Go doc comment.
Rule Used: transports/config.schema.json is the source of tru... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/clientconfig.go`:
- Line 98: The `metrics_require_auth` boolean field is defined in the
clientconfig structure but is missing from the transports/config.schema.json
schema file, which creates a mismatch between the code configuration and its
schema definition. Add the `metrics_require_auth` field entry to the schema file
with the appropriate type (boolean) and description, placing it alongside the
other auth-related fields like `required_headers`, `logging_headers`,
`whitelisted_routes`, and `hide_deleted_virtual_keys_in_filters` to maintain
consistency and parity with the actual code configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 607fd56a-c5cf-4e13-995d-97f1bf32bd27
📒 Files selected for processing (4)
framework/configstore/clientconfig.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/server/server.go
The Prometheus scrape endpoint is operational telemetry and scrapers can't carry admin auth, so /metrics bypasses the auth middleware by default (like /health). Because the metric labels can include provider/model/virtual-key/team/customer/cost, operators who consider that sensitive can set client config metrics_require_auth=true to keep /metrics behind auth. Implemented as an exact-match gate in APIMiddleware reading an atomic flag (default false), fed from ClientConfig.MetricsRequireAuth via UpdateMetricsRequireAuth — not a hardcoded unconditional whitelist. Also drops a duplicate /health entry. Tests cover public-by-default, gated-when-set, and the exact-match guard (/metricsX and /metrics/foo stay authenticated). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks — addressed:
On the design itself (per the PR description): this remains a proposal to flip the default to public — happy to defer to the whitelist-only approach from #2695 if you'd prefer. I also offered a companion UI toggle for the Security settings view; glad to send it if this lands. |
mickgvirtu
force-pushed
the
pr-metrics-public-default
branch
from
489e5dd to
48d8503
Compare
2 participants
Summary
Follow-up to #2695. That issue was resolved by letting operators add
/metricsto the path whitelist — which works. This proposes making the conventional behavior the default:/metrics(like/health) bypasses auth out of the box, since Prometheus scrapers typically can't carry admin credentials, with an explicit opt-out for operators who consider the metric labels sensitive.Changes
/metricsbypassesAPIMiddlewareauth by default, via an exact-match gate (no prefix widening:/metricsX,/metrics/foostay authenticated).ClientConfig.metrics_require_auth(defaultfalse); settrueto keep/metricsbehind auth — for deployments where labels (provider/model/virtual-key/team/customer/cost) are sensitive./healthentry in the whitelist.Type of change
Affected areas
How to test
go test ./transports/bifrost-http/handlers/ -run TestAuthMiddleware_MetricsCovers public-by-default, gated-when-
metrics_require_auth=true, and the exact-match negatives.Follow-up: UI toggle (happy to send as a companion PR)
This adds a
metrics_require_authclient-config field, which the dashboard should surface alongside the existing auth controls. The natural home is the Security settings view (ui/app/workspace/config/views/securityView.tsx) — next to the "Enable authentication" switch and the "Whitelisted routes" editor — plus theClientConfigtype inui/lib/types/config.ts(besidewhitelisted_routes): a single switch ("Require auth for /metrics", default off) wired through the existingsetLocalConfigflow. Kept this PR backend-only to stay reviewable; glad to open the matching UI PR following your securityView patterns if you'd like it.Deferential note: #2695 was closed with the position that the existing path-whitelist is the intended mechanism. We're proposing a default change on the principle that the conventional unauthenticated endpoint should require config to secure, not to expose — but we fully understand if you prefer to keep it whitelist-only. Happy to close this or rework it into docs for the whitelist path instead.