From c18a4a975d87ccd79202d56739ba81eb633d6c46 Mon Sep 17 00:00:00 2001 From: Vishwas Rajashekar Date: Sun, 14 Jun 2026 19:59:22 +0530 Subject: [PATCH] fix(authz): metrics: deny authenticated users not in ACL even with anonymous read (#4131) * fix(authz): metrics: reject users not in list even with anonymous read Even when anonymous reads are enabled for metrics, users not in the allowed list should not be allowed. This change also refactors the MetricsAuthzHandler to align better with this logic. Signed-off-by: Vishwas Rajashekar * fix(authz): address review comments Address comments to pass username when present to AuthzFail if user is not allowed for metrics. This changes the response to Forbidden instead of Unauthorized. Use isAnonymous() check instead of only checking for empty username. Signed-off-by: Vishwas Rajashekar * fix(authz): fix additional review comments Fix a few more review comments Signed-off-by: Vishwas Rajashekar --------- Signed-off-by: Vishwas Rajashekar --- pkg/api/authz.go | 43 ++++++------ pkg/extensions/monitoring/monitoring_test.go | 70 ++++++++++++++++++-- 2 files changed, 88 insertions(+), 25 deletions(-) diff --git a/pkg/api/authz.go b/pkg/api/authz.go index 99efa287..ce55d26f 100644 --- a/pkg/api/authz.go +++ b/pkg/api/authz.go @@ -741,21 +741,6 @@ func MetricsAuthzHandler(ctlr *Controller) mux.MiddlewareFunc { return } - metricsConfig := accessControlConfig.GetMetrics() - if slices.Contains(metricsConfig.AnonymousPolicy, constants.ReadPermission) { - next.ServeHTTP(response, request) - - return - } - - if len(metricsConfig.Users) == 0 { - log := ctlr.Log - log.Warn().Msg("auth is enabled but no metrics users in accessControl: /metrics is unaccesible") - common.AuthzFail(response, request, "", realm, failDelay) - - return - } - // get access control context made in authn.go userAc, err := reqCtx.UserAcFromContext(request.Context()) if err != nil { // should never happen @@ -764,11 +749,31 @@ func MetricsAuthzHandler(ctlr *Controller) mux.MiddlewareFunc { return } - username := userAc.GetUsername() - if !slices.Contains(metricsConfig.Users, username) { - common.AuthzFail(response, request, username, realm, failDelay) + metricsAccessConfig := accessControlConfig.GetMetrics() - return + if userAc.IsAnonymous() { + // If anonymous read is not specified in access control, deny. + if !slices.Contains(metricsAccessConfig.AnonymousPolicy, constants.ReadPermission) { + common.AuthzFail(response, request, "", realm, failDelay) + + return + } + } else { + username := userAc.GetUsername() + if len(metricsAccessConfig.Users) == 0 { + log := ctlr.Log + log.Warn().Msg("no users configured in metrics user list; " + + "metrics are not accessible to any authenticated user.") + common.AuthzFail(response, request, username, realm, failDelay) + + return + } + + if !slices.Contains(metricsAccessConfig.Users, username) { + common.AuthzFail(response, request, username, realm, failDelay) + + return + } } next.ServeHTTP(response, request) //nolint:contextcheck diff --git a/pkg/extensions/monitoring/monitoring_test.go b/pkg/extensions/monitoring/monitoring_test.go index 3a8b76f3..7ae6fd98 100644 --- a/pkg/extensions/monitoring/monitoring_test.go +++ b/pkg/extensions/monitoring/monitoring_test.go @@ -280,14 +280,14 @@ func TestMetricsAuthorization(t *testing.T) { resp, err := client.R().Get(baseURL + "/metrics") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) // authenticated but not authorized user should not have access to/metrics client.SetBasicAuth(metricsuser, metricspass) resp, err = client.R().Get(baseURL + "/metrics") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) }) Convey("with basic auth: metrics users in accessControl", func() { conf.HTTP.AccessControl = &config.AccessControlConfig{ @@ -450,14 +450,72 @@ func TestMetricsAuthorization(t *testing.T) { So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) - // valid credentials should also work - client := resty.New() - client.SetBasicAuth(metricsuser, metricspass) - resp, err = client.R().Get(baseURL + "/metrics") + // Scrape should not be allowed for the metrics user when allowed metrics users + // are not configured. + metricsUserClient := resty.New() + metricsUserClient.SetBasicAuth(metricsuser, metricspass) + resp, err = metricsUserClient.R().Get(baseURL + "/metrics") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + // Scrape should not be allowed for a valid user when allowed metrics users + // are not configured. + normalUserClient := resty.New() + normalUserClient.SetBasicAuth(username, password) + resp, err = normalUserClient.R().Get(baseURL + "/metrics") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + // anonymous access to registry endpoints should remain protected + resp, err = resty.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + }) + Convey("with basic auth: metrics.anonymousPolicy and users list, allow permitted user", func() { + conf.HTTP.AccessControl = &config.AccessControlConfig{ + Metrics: config.Metrics{ + Users: []string{metricsuser}, + AnonymousPolicy: []string{"read"}, + }, + } + ctlr := api.NewController(conf) + ctlr.Config.Storage.RootDirectory = t.TempDir() + + cm := test.NewControllerManager(ctlr) + cm.StartAndWait(port) + defer cm.StopServer() + + // unauthenticated client should be allowed to scrape /metrics + resp, err := resty.R().Get(baseURL + "/metrics") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusOK) + // wrong credentials should still be rejected at authn + resp, err = resty.R().SetBasicAuth("hacker", "wrongpass").Get(baseURL + "/metrics") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + // scrape should be allowed for a valid user in the metrics users list + metricsUserClient := resty.New() + metricsUserClient.SetBasicAuth(metricsuser, metricspass) + resp, err = metricsUserClient.R().Get(baseURL + "/metrics") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // scrape should not be allowed for a valid user who is not in the metrics users list + normalUserClient := resty.New() + normalUserClient.SetBasicAuth(username, password) + resp, err = normalUserClient.R().Get(baseURL + "/metrics") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + // anonymous access to registry endpoints should remain protected resp, err = resty.R().Get(baseURL + "/v2/") So(err, ShouldBeNil)