diff --git a/pkg/api/authn.go b/pkg/api/authn.go index f146ec1655..04ff60efa7 100644 --- a/pkg/api/authn.go +++ b/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) diff --git a/pkg/api/authz.go b/pkg/api/authz.go index 3228e8b038..4c8b2d5dc7 100644 --- 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,9 +203,7 @@ 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 } } } @@ -217,20 +211,16 @@ func (ac *AccessController) isPermitted(userGroups []string, username, action st } // 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 + }) + } +} diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 8ab156e328..c6ed53da26 100644 --- 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 diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 1db9a8044b..6cd5d9a73b 100644 --- a/pkg/api/routes.go +++ b/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) diff --git a/pkg/extensions/README.md b/pkg/extensions/README.md index a6cb9ba33a..6f9df76ba3 100644 --- a/pkg/extensions/README.md +++ b/pkg/extensions/README.md @@ -30,9 +30,9 @@ package extensions IsAdmin bool Username string Groups []string - } + } ``` - This data can then be accessed from the request context so that every extension can apply its own authorization logic, if needed . + This data can then be accessed from the request context so that every extension can apply its own authorization logic, if needed . - 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. @@ -40,6 +40,6 @@ package extensions - 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: sync, scrub, metrics, search . +- the available extensions that can be used at the moment are: sync, search, scrub, metrics, lint, ui, mgmt, userprefs, imagetrust . NOTE: When multiple extensions are used, they should be listed in the above presented order. diff --git a/pkg/extensions/extension_metrics.go b/pkg/extensions/extension_metrics.go index 54e71982fa..1837d7ef78 100644 --- a/pkg/extensions/extension_metrics.go +++ b/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()) } } diff --git a/pkg/extensions/extension_metrics_disabled.go b/pkg/extensions/extension_metrics_disabled.go index 6d280b9ba6..624fc06125 100644 --- a/pkg/extensions/extension_metrics_disabled.go +++ b/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") } diff --git a/pkg/test/skip/skip_test.go b/pkg/test/skip/skip_test.go new file mode 100644 index 0000000000..b266f11cdb --- /dev/null +++ b/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) +} diff --git a/test/blackbox/helpers_metrics.bash b/test/blackbox/helpers_metrics.bash index cce55e5b84..4c225568b8 100644 --- a/test/blackbox/helpers_metrics.bash +++ b/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}) diff --git a/test/blackbox/metrics.bats b/test/blackbox/metrics.bats index 0d392ac2cf..63f29a4af2 100644 --- a/test/blackbox/metrics.bats +++ b/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,18 @@ function teardown_file() { } @test "unauthorized request to metrics" { +# anonymous policy: metrics endpoint should not be available 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 + run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 401 + [ "$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 ] } - diff --git a/test/blackbox/metrics_minimal.bats b/test/blackbox/metrics_minimal.bats index c693f64ec5..f1053ceaa9 100644 --- a/test/blackbox/metrics_minimal.bats +++ b/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,18 @@ function teardown_file() { } @test "unauthorized request to metrics" { +# anonymous policy: metrics endpoint should not be available 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 + run metrics_route_check 8080 "-u ${AUTH_USER}:${AUTH_PASS}" 401 + [ "$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 ] -} \ No newline at end of file +}