Skip to content

Require optional admin auth for /metrics endpoint - #1537

Open
rodneyosodo wants to merge 3 commits into
confidential-containers:mainfrom
rodneyosodo:fix/6-protect-metrics
Open

rodneyosodo wants to merge 3 commits into
confidential-containers:mainfrom
rodneyosodo:fix/6-protect-metrics

Conversation

@rodneyosodo

@rodneyosodo rodneyosodo commented Aug 3, 2026

Copy link
Copy Markdown

Protect /metrics with admin auth (opt-in)

KBS serves Prometheus metrics on /metrics without authentication, and the metric
labels expose operational detail such as resource paths and TEE types. Those values
are not secrets in the KBS threat model (without TLS they are already observable on
the wire), but operators may still want to lock the endpoint down without adding an
external component. This adds an opt-in way to do that.

What changed

  • New config flag http_server.require_admin_auth_metrics (default false). Set
    it to true to require an admin JWT to scrape /metrics:

    [http_server]
    require_admin_auth_metrics = true
  • When enabled, /metrics goes through the same admin authentication/authorization
    as the other admin endpoints, and failures map to AdminAuthAccess. Scrapers send
    a bearer admin JWT whose role ACL covers /metrics (e.g. ^/metrics$). With
    DenyAll, /metrics is fully locked.

  • The ACL anchor check is narrowed, not removed: allowed_endpoints must still
    start with ^/kbs or ^/metrics and end with $, so enabling this does not let
    admin auth be silently extended to arbitrary paths. A rule spanning both namespaces
    must be split into one ACL entry per namespace. Existing ^/kbs...$ rules are
    unaffected.

  • Only /metrics is guarded. /healthz and other status/liveness handlers stay
    unauthenticated; this is not a general mechanism for putting operational endpoints
    behind admin auth.

  • Docs updated (config.md, admin.md, metrics.md).

Deployment guidance

This is defense-in-depth for deployments that do not put an authenticating gateway in
front of KBS. Restricting network access to the metrics port (and running a gateway)
remains the recommended primary control; the flag exists so that operators who cannot
rely on one still have a built-in option.

Tests

Integration tests cover no token, valid token, DenyAll, and a restricted ACL that
excludes /metrics, plus a test asserting the default stays unauthenticated. Unit
tests cover the narrowed ACL anchor validation.

@rodneyosodo
rodneyosodo requested a review from a team as a code owner August 3, 2026 13:06
@rodneyosodo
rodneyosodo force-pushed the fix/6-protect-metrics branch from d950629 to f814d3b Compare August 3, 2026 13:06
@fitzthum

fitzthum commented Aug 3, 2026

Copy link
Copy Markdown
Member

This was discussed when the metrics feature was first added. Note that neither of thing things in your example are considered secret. They are both conveyed revealed by the KBS protocol itself (unless TLS Is enabled).

On the other hand, maybe it's reasonable to add this option. wdyt @Xynnn007 @pmores

@Xynnn007

Xynnn007 commented Aug 4, 2026

Copy link
Copy Markdown
Member

Oh. This is a following question for admin module integration. imo, we do not need this in KBS logic, as it would make the core KBS logic complexer. Usually, the API only exposes in intranet (with proper deployment with k8s) thus would not be called by outer clients.

If the API needs to be exposed to outer callers in some cases, it's rercommended to use a network gateway in front of KBS. That gate way can control the inbound/outbound network flow and the allowlists. This is about the deployment model, than core logic.

Furthurmore, in future some more APIs about HTTP service status/liveness/... should not be covered by admin auth imo.

@pmores

pmores commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

I don't really have a strong opinion of this as it's really much more about auth than Prometheus. That said, the option doesn't seem likely to break anything, and it seems to bring the metric endpoint to the same level of auth as the admin endpoints, for whatever that's worth. The change isn't overly complex either. On the other hand, extending the set of auth'd endpoints this way does seem excessive.

@fitzthum

fitzthum commented Aug 4, 2026

Copy link
Copy Markdown
Member

I am open to it. In theory metrics don't reveal any information, but perhaps better safe than sorry. If something is potentially sensitive, I think we we should provide a way to lock it down without needing any extra tools.

@rodneyosodo
rodneyosodo force-pushed the fix/6-protect-metrics branch from f814d3b to f9b2f22 Compare August 20, 2026 14:57
The /metrics endpoint was unauthenticated, leaking internal label values
such as resource paths and TEE types. Require a valid admin JWT (via the
configured admin authentication/authorization backend) before serving
Prometheus metrics, and map failures to AdminAuthAccess like the other
admin-protected endpoints.

Relax the regex_acl anchor validation from '^/kbs' to '^/' so that
/metrics can be granted to admin roles via allowed_endpoints; existing
'^/kbs...$' rules remain valid.

Add an integration test covering no token, valid token, DenyAll admin
backend, and a restricted ACL that excludes /metrics.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Add http_server.require_admin_auth_metrics (default false) so operators
can opt in to protecting /metrics with admin JWT auth instead of it being
mandatory. The default preserves unauthenticated metric scraping for
existing deployments; enabling it applies the check_admin_access guard
added previously.

Update the metrics integration test to opt in to the protection, add a
test asserting /metrics stays accessible without a token by default, and
document the new flag in config.md and metrics.md.

Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
Signed-off-by: Rodney Osodo <socials@rodneyosodo.com>
@rodneyosodo

Copy link
Copy Markdown
Author

@Xynnn007 Thanks for the review. Agreed on all three points and adjusted the change accordingly.

  1. Complexity: this is opt-in and defaults to false, so the existing behaviour (/metrics unauthenticated) is unchanged unless an operator explicitly sets http_server.require_admin_auth_metrics = true. The handler just adds one guarded check_admin_access call, reusing the existing admin backend rather than introducing new logic.
  2. Gateway vs. core: I don't intend this to replace a network gateway/allowlist in front of KBS; that stays the recommended deployment control. The flag is only a fallback for deployments that can't rely on one, which matches fitzthum's suggestion of being able to lock it down without extra tooling.
  3. Other status endpoints: fully agreed. Only /metrics is affected; /healthz and liveness/readiness handlers stay unauthenticated, and this isn't meant to grow into a general mechanism for those.

I also tightened the ACL change: instead of ^/kbs^/, the anchor check now only accepts ^/kbs or ^/metrics (still anchored with $), so the admin-auth surface isn't silently widened to arbitrary paths. If you'd still prefer this handled purely at the gateway and would rather not carry the option in KBS, I'm happy to close it; just let me know your preference.

@Xynnn007 Xynnn007 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The option would make sense to me.

Comment thread kbs/docs/metrics.md
Comment on lines +82 to +86
# Only needed when http_server.require_admin_auth_metrics = true:
# present a valid admin JWT.
authorization:
type: Bearer
credentials_file: /path/to/admin.jwt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants