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)