fix: Metrics should be protected behind authZ

Signed-off-by: Alexei Dodon <[email protected]>

pkg/api/authn.go

@@ -60,7 +60,7 @@ func AuthHandler(ctlr *Controller) mux.MiddlewareFunc {
 		return bearerAuthHandler(ctlr)
 	}
 
-	return authnMiddleware.TryAuthnHandlers(ctlr)
+	return authnMiddleware.tryAuthnHandlers(ctlr)
 }
 
 func (amw *AuthnMiddleware) sessionAuthn(ctlr *Controller, userAc *reqCtx.UserAccessControl,
@@ -245,7 +245,7 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
 	return false, nil
 }
 
-func (amw *AuthnMiddleware) TryAuthnHandlers(ctlr *Controller) mux.MiddlewareFunc { //nolint: gocyclo
+func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFunc { //nolint: gocyclo
 	// no password based authN, if neither LDAP nor HTTP BASIC is enabled
 	if !ctlr.Config.IsBasicAuthnEnabled() {
 		return noPasswdAuth(ctlr)

pkg/api/authz.go

@@ -191,14 +191,10 @@ func (ac *AccessController) getAuthnMiddlewareContext(authnType string, request
 func (ac *AccessController) isPermitted(userGroups []string, username, action string,
 	policyGroup config.PolicyGroup,
 ) bool {
-	var result bool
-
 	// check repo/system based policies
 	for _, p := range policyGroup.Policies {
 		if common.Contains(p.Users, username) && common.Contains(p.Actions, action) {
-			result = true
-
-			return result
+			return true
 		}
 	}
 
@@ -207,30 +203,24 @@ func (ac *AccessController) isPermitted(userGroups []string, username, action st
 			if common.Contains(p.Actions, action) {
 				for _, group := range p.Groups {
 					if common.Contains(userGroups, group) {
-						result = true
-
-						return result
+						return true
 					}
 				}
 			}
 		}
 	}
 
 	// check defaultPolicy
-	if !result {
-		if common.Contains(policyGroup.DefaultPolicy, action) && username != "" {
-			result = true
-		}
+	if common.Contains(policyGroup.DefaultPolicy, action) && username != "" {
+		return true
 	}
 
 	// check anonymousPolicy
-	if !result {
-		if common.Contains(policyGroup.AnonymousPolicy, action) && username == "" {
-			result = true
-		}
+	if common.Contains(policyGroup.AnonymousPolicy, action) && username == "" {
+		return true
 	}
 
-	return result
+	return false
 }
 
 func BaseAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
@@ -343,3 +333,46 @@ func DistSpecAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
 		})
 	}
 }
+
+func MetricsAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
+			if request.Method == http.MethodOptions {
+				next.ServeHTTP(response, request)
+
+				return
+			}
+
+			if ctlr.Config.HTTP.AccessControl == nil {
+				// allow access to authenticated user as anonymous policy does not exist
+				next.ServeHTTP(response, request)
+
+				return
+			}
+			if len(ctlr.Config.HTTP.AccessControl.Metrics.Users) == 0 {
+				log := ctlr.Log
+				log.Warn().Msg("auth is enabled but no metrics users in accessControl: /metrics is unaccesible")
+				common.AuthzFail(response, request, "", ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
+
+				return
+			}
+
+			// get access control context made in authn.go
+			userAc, err := reqCtx.UserAcFromContext(request.Context())
+			if err != nil { // should never happen
+				common.AuthzFail(response, request, "", ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
+
+				return
+			}
+
+			username := userAc.GetUsername()
+			if !common.Contains(ctlr.Config.HTTP.AccessControl.Metrics.Users, username) {
+				common.AuthzFail(response, request, username, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
+
+				return
+			}
+
+			next.ServeHTTP(response, request) //nolint:contextcheck
+		})
+	}
+}

pkg/api/config/config.go

@@ -131,6 +131,7 @@ type AccessControlConfig struct {
 	Repositories Repositories `json:"repositories" mapstructure:"repositories"`
 	AdminPolicy  Policy
 	Groups       Groups
+	Metrics      Metrics
 }
 
 func (config *AccessControlConfig) AnonymousPolicyExists() bool {
@@ -168,6 +169,10 @@ type Policy struct {
 	Groups  []string
 }
 
+type Metrics struct {
+	Users []string
+}
+
 type Config struct {
 	DistSpecVersion string `json:"distSpecVersion" mapstructure:"distSpecVersion"`
 	GoVersion       string

pkg/api/routes.go

@@ -183,7 +183,7 @@ func (rh *RouteHandler) SetupRoutes() {
 	pprof.SetupPprofRoutes(rh.c.Config, prefixedRouter, authHandler, rh.c.Log)
 
 	// Preconditions for enabling the actual extension routes are part of extensions themselves
-	ext.SetupMetricsRoutes(rh.c.Config, rh.c.Router, authHandler, rh.c.Log, rh.c.Metrics)
+	ext.SetupMetricsRoutes(rh.c.Config, rh.c.Router, authHandler, MetricsAuthzHandler(rh.c), rh.c.Log, rh.c.Metrics)
 	ext.SetupSearchRoutes(rh.c.Config, prefixedRouter, rh.c.StoreController, rh.c.MetaDB, rh.c.CveScanner,
 		rh.c.Log)
 	ext.SetupImageTrustRoutes(rh.c.Config, prefixedRouter, rh.c.MetaDB, rh.c.Log)

pkg/extensions/README.md

@@ -30,16 +30,16 @@ package extensions
     IsAdmin         bool
     Username        string
     Groups          []string
-    } 
+    }
     ```
-  This data can then be accessed from the request context so that <b>every extension can apply its own authorization logic, if needed </b>. 
+  This data can then be accessed from the request context so that <b>every extension can apply its own authorization logic, if needed </b>.
 
 - when a new extension comes out, the developer should also write some blackbox tests, where a binary that contains the new extension should be tested in a real usage scenario. See [test/blackbox](test/blackbox/sync.bats) folder for multiple extensions examples.
 
 - newly added blackbox tests should have targets in Makefile. You should also add them as Github Workflows, in [.github/workflows/ecosystem-tools.yaml](.github/workflows/ecosystem-tools.yaml)
 
 - with every new extension, you should modify the EXTENSIONS variable in Makefile by adding the new extension. The EXTENSIONS variable represents all extensions and is used in Make targets that require them all (e.g make test).
 
-- the available extensions that can be used at the moment are: <b>sync, scrub, metrics, search </b>.
+- the available extensions that can be used at the moment are: <b>sync, search, scrub, metrics, lint, ui, mgmt, userprefs, imagetrust </b>.
 NOTE: When multiple extensions are used, they should be listed in the above presented order.
 

pkg/extensions/extension_metrics.go

@@ -26,13 +26,14 @@ func EnableMetricsExtension(config *config.Config, log log.Logger, rootDir strin
 }
 
 func SetupMetricsRoutes(config *config.Config, router *mux.Router,
-	authFunc mux.MiddlewareFunc, log log.Logger, metrics monitoring.MetricServer,
+	authnFunc, authzFunc mux.MiddlewareFunc, log log.Logger, metrics monitoring.MetricServer,
 ) {
 	log.Info().Msg("setting up metrics routes")
 
 	if config.IsMetricsEnabled() {
 		extRouter := router.PathPrefix(config.Extensions.Metrics.Prometheus.Path).Subrouter()
-		extRouter.Use(authFunc)
+		extRouter.Use(authnFunc)
+		extRouter.Use(authzFunc)
 		extRouter.Methods("GET").Handler(promhttp.Handler())
 	}
 }

pkg/extensions/extension_metrics_disabled.go

@@ -22,13 +22,14 @@ func EnableMetricsExtension(config *config.Config, log log.Logger, rootDir strin
 
 // SetupMetricsRoutes ...
 func SetupMetricsRoutes(conf *config.Config, router *mux.Router,
-	authFunc mux.MiddlewareFunc, log log.Logger, metrics monitoring.MetricServer,
+	authnFunc, authzFunc mux.MiddlewareFunc, log log.Logger, metrics monitoring.MetricServer,
 ) {
 	getMetrics := func(w http.ResponseWriter, r *http.Request) {
 		m := metrics.ReceiveMetrics()
 		zcommon.WriteJSON(w, http.StatusOK, m)
 	}
 
-	router.Use(authFunc)
+	router.Use(authnFunc)
+	router.Use(authzFunc)
 	router.HandleFunc("/metrics", getMetrics).Methods("GET")
 }

pkg/test/skip/skip_test.go

@@ -0,0 +1,37 @@
+package skip_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	tskip "zotregistry.io/zot/pkg/test/skip"
+)
+
+// for code coverage.
+func TestSkipS3(t *testing.T) {
+	envName := "S3MOCK_ENDPOINT"
+	envVal := os.Getenv(envName)
+
+	if len(envVal) > 0 {
+		defer os.Setenv(envName, envVal)
+		err := os.Unsetenv(envName)
+		assert.Equal(t, err, nil, "Error should be nil")
+	}
+
+	tskip.SkipS3(t)
+}
+
+func TestSkipDynamo(t *testing.T) {
+	envName := "DYNAMODBMOCK_ENDPOINT"
+	envVal := os.Getenv(envName)
+
+	if len(envVal) > 0 {
+		defer os.Setenv(envName, envVal)
+		err := os.Unsetenv(envName)
+		assert.Equal(t, err, nil, "Error should be nil")
+	}
+
+	tskip.SkipDynamo(t)
+}

test/blackbox/helpers_metrics.bash

@@ -1,3 +1,6 @@
+METRICS_USER=observability
+METRICS_PASS=MySecreTPa55
+
 function metrics_route_check () {
     local servername="http://127.0.0.1:${1}/metrics"
     status_code=$(curl --write-out '%{http_code}' ${2}  --silent --output /dev/null ${servername})

test/blackbox/metrics.bats

@@ -32,6 +32,7 @@ function setup_file() {
     zot_config_file=${BATS_FILE_TMPDIR}/zot_config.json
     zot_htpasswd_file=${BATS_FILE_TMPDIR}/zot_htpasswd
     htpasswd -Bbn ${AUTH_USER} ${AUTH_PASS} >> ${zot_htpasswd_file}
+    htpasswd -Bbn ${METRICS_USER} ${METRICS_PASS} >> ${zot_htpasswd_file}
 
     mkdir -p ${zot_root_dir}
     touch ${zot_log_file}
@@ -48,6 +49,20 @@ function setup_file() {
             "htpasswd": {
                 "path": "${zot_htpasswd_file}"
             }
+        },
+        "accessControl": {
+            "metrics":{
+                "users": ["${METRICS_USER}"]
+            },
+            "repositories": {
+                "**": {
+                    "anonymousPolicy": [
+                        "read",
+                        "create"
+                    ],
+                    "defaultPolicy": ["read"]
+                }
+            }
         }
     },
     "log": {
@@ -80,14 +95,20 @@ function teardown_file() {
 }
 
 @test "unauthorized request to metrics" {
+# anonymous policy: metrics endpoint should not be available
+# 401 - http.StatusUnauthorized
     run metrics_route_check 8080 "" 401
     [ "$status" -eq 0 ]
+# user is not in htpasswd
     run metrics_route_check 8080 "-u unlucky:wrongpass" 401
     [ "$status" -eq 0 ]
+# proper user/pass tuple from htpasswd, but user not allowed to access metrics
+# 403 - http.StatusForbidden
+    run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 403
+    [ "$status" -eq 0 ]
 }
 
 @test "authorized request: metrics enabled" {
-    run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 200
+    run metrics_route_check 8080 "-u ${METRICS_USER}:${METRICS_PASS}" 200
     [ "$status" -eq 0 ]
 }
-

test/blackbox/metrics_minimal.bats

@@ -32,6 +32,7 @@ function setup_file() {
     zot_config_file=${BATS_FILE_TMPDIR}/zot_config.json
     zot_htpasswd_file=${BATS_FILE_TMPDIR}/zot_htpasswd
     htpasswd -Bbn ${AUTH_USER} ${AUTH_PASS} >> ${zot_htpasswd_file}
+    htpasswd -Bbn ${METRICS_USER} ${METRICS_PASS} >> ${zot_htpasswd_file}
 
     mkdir -p ${zot_root_dir}
     touch ${zot_log_file}
@@ -48,6 +49,20 @@ function setup_file() {
             "htpasswd": {
                 "path": "${zot_htpasswd_file}"
             }
+        },
+        "accessControl": {
+            "metrics":{
+                "users": ["${METRICS_USER}"]
+            },
+            "repositories": {
+                "**": {
+                    "anonymousPolicy": [
+                        "read",
+                        "create"
+                    ],
+                    "defaultPolicy": ["read"]
+                }
+            }
         }
     },
     "log": {
@@ -72,13 +87,20 @@ function teardown_file() {
 }
 
 @test "unauthorized request to metrics" {
+# anonymous policy: metrics endpoint should not be available
+# 401 - http.StatusUnauthorized
     run metrics_route_check 8080 "" 401
     [ "$status" -eq 0 ]
+# user is not in htpasswd
     run metrics_route_check 8080 "-u test:wrongpass" 401
     [ "$status" -eq 0 ]
+# proper user/pass tuple from htpasswd, but user not allowed to access metrics
+# 403 - http.StatusForbidden
+    run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 403
+    [ "$status" -eq 0 ]
 }
 
 @test "authorized request: metrics enabled" {
-    run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 200
+    run metrics_route_check 8080 "-u ${METRICS_USER}:${METRICS_PASS}" 200
     [ "$status" -eq 0 ]
-}
+}