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 <dev@vrajashkr.com>

* 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 <dev@vrajashkr.com>

* fix(authz): fix additional review comments

Fix a few more review comments

Signed-off-by: Vishwas Rajashekar <dev@vrajashkr.com>

---------

Signed-off-by: Vishwas Rajashekar <dev@vrajashkr.com>

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

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)