fix: metrics should be protected behind authZ

Signed-off-by: Alexei Dodon <[email protected]>
project-zot · Oct 18, 2023 · a208dbc · a208dbc
1 parent a44ca57
commit a208dbc
Show file tree

Hide file tree

Showing 27 changed files with 965 additions and 511 deletions.
diff --git a/examples/config-metrics-authn.json b/examples/config-metrics-authn.json
@@ -0,0 +1,26 @@
+{
+    "distSpecVersion": "1.1.0-dev",
+    "storage": {
+        "rootDirectory": "/tmp/zot"
+    },
+    "http": {
+        "address": "127.0.0.1",
+        "port": "8080",
+        "auth": {
+            "htpasswd": {
+              "path": "test/data/htpasswd"
+            }
+        }
+    },
+    "log": {
+        "level": "debug"
+    },
+    "extensions": {
+        "metrics": {
+            "enable": true,
+            "prometheus": {
+                "path": "/metrics"
+            }
+        }
+    }
+}
diff --git a/pkg/api/authn.go b/pkg/api/authn.go
@@ -57,7 +57,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,
@@ -247,7 +247,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)

diff --git a/pkg/api/authn_test.go b/pkg/api/authn_test.go
@@ -79,7 +79,9 @@ func TestAPIKeys(t *testing.T) {
 		conf := config.New()
 		conf.HTTP.Port = port
 
-		htpasswdPath := test.MakeHtpasswdFile()
+		username := test.GenerateRandomString()
+		password := test.GenerateRandomString()
+		htpasswdPath := test.MakeHtpasswdFileFromString(test.GetCredString(username, password))
 		defer os.Remove(htpasswdPath)
 
 		mockOIDCServer, err := authutils.MockOIDCRun()
@@ -145,7 +147,7 @@ func TestAPIKeys(t *testing.T) {
 		Convey("API key retrieved with basic auth", func() {
 			resp, err := resty.R().
 				SetBody(reqBody).
-				SetBasicAuth("test", "test").
+				SetBasicAuth(username, password).
 				Post(baseURL + constants.APIKeyPath)
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
@@ -162,15 +164,15 @@ func TestAPIKeys(t *testing.T) {
 			So(email, ShouldNotBeEmpty)
 
 			resp, err = resty.R().
-				SetBasicAuth("test", apiKeyResponse.APIKey).
+				SetBasicAuth(username, apiKeyResponse.APIKey).
 				Get(baseURL + "/v2/_catalog")
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
 			So(resp.StatusCode(), ShouldEqual, http.StatusOK)
 
 			// get API key list with basic auth
 			resp, err = resty.R().
-				SetBasicAuth("test", "test").
+				SetBasicAuth(username, password).
 				Get(baseURL + constants.APIKeyPath)
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
@@ -189,7 +191,7 @@ func TestAPIKeys(t *testing.T) {
 			// add another one
 			resp, err = resty.R().
 				SetBody(reqBody).
-				SetBasicAuth("test", "test").
+				SetBasicAuth(username, password).
 				Post(baseURL + constants.APIKeyPath)
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
@@ -199,15 +201,15 @@ func TestAPIKeys(t *testing.T) {
 			So(err, ShouldBeNil)
 
 			resp, err = resty.R().
-				SetBasicAuth("test", apiKeyResponse.APIKey).
+				SetBasicAuth(username, apiKeyResponse.APIKey).
 				Get(baseURL + "/v2/_catalog")
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
 			So(resp.StatusCode(), ShouldEqual, http.StatusOK)
 
 			// get API key list with api key auth
 			resp, err = resty.R().
-				SetBasicAuth("test", apiKeyResponse.APIKey).
+				SetBasicAuth(username, apiKeyResponse.APIKey).
 				Get(baseURL + constants.APIKeyPath)
 			So(err, ShouldBeNil)
 			So(resp, ShouldNotBeNil)
@@ -600,7 +602,7 @@ func TestAPIKeys(t *testing.T) {
 			So(len(apiKeyListResponse.APIKeys), ShouldEqual, 0)
 
 			resp, err = client.R().
-				SetBasicAuth("test", "test").
+				SetBasicAuth(username, password).
 				SetQueryParam("id", apiKeyResponse.UUID).
 				Delete(baseURL + constants.APIKeyPath)
 			So(err, ShouldBeNil)
@@ -832,7 +834,9 @@ func TestAPIKeys(t *testing.T) {
 func TestAPIKeysOpenDBError(t *testing.T) {
 	Convey("Test API keys - unable to create database", t, func() {
 		conf := config.New()
-		htpasswdPath := test.MakeHtpasswdFile()
+		username := test.GenerateRandomString()
+		password := test.GenerateRandomString()
+		htpasswdPath := test.MakeHtpasswdFileFromString(test.GetCredString(username, password))
 		defer os.Remove(htpasswdPath)
 
 		mockOIDCServer, err := authutils.MockOIDCRun()

diff --git a/pkg/api/authz.go b/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,40 @@ 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 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
+		})
+	}
+}
diff --git a/pkg/api/config/config.go b/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