From 50da4e7cc976a9b51e2139730d09c44942500877 Mon Sep 17 00:00:00 2001 From: Nicol Draghici Date: Thu, 14 Jul 2022 18:13:46 +0300 Subject: [PATCH 1/4] Remove AllowReadOnly and ReadOnly Signed-off-by: Nicol Draghici --- .github/workflows/ecosystem-tools.yaml | 3 + Makefile | 4 + errors/errors.go | 1 + examples/README.md | 10 +- ...authz.json => config-anonymous-authz.json} | 8 +- examples/config-bench.json | 3 +- examples/config-commit.json | 3 +- examples/config-example.json | 3 +- examples/config-example.yaml | 1 - examples/config-gc.json | 3 +- examples/config-lint.json | 5 +- examples/config-minimal.json | 3 +- examples/config-multiple-cve.json | 3 +- examples/config-multiple.json | 3 +- examples/config-policy.json | 1 + examples/config-ratelimit.json | 1 - examples/config-s3.json | 3 +- pkg/api/authn.go | 82 +++-- pkg/api/authz.go | 50 +++- pkg/api/config/config.go | 12 +- pkg/api/controller.go | 3 +- pkg/api/controller_test.go | 279 ++++++++++++++++-- pkg/api/routes.go | 2 +- pkg/cli/root.go | 45 ++- pkg/cli/root_test.go | 24 +- pkg/extensions/sync/sync_test.go | 2 +- test/blackbox/anonymous_policiy.bats | 91 ++++++ test/blackbox/cve.bats | 3 +- test/blackbox/metrics.bats | 3 +- test/blackbox/pushpull.bats | 3 +- test/blackbox/scrub.bats | 3 +- test/blackbox/sync.bats | 6 +- test/cluster/config-minio.json | 3 +- 33 files changed, 505 insertions(+), 164 deletions(-) rename examples/{config-default-authz.json => config-anonymous-authz.json} (81%) create mode 100644 test/blackbox/anonymous_policiy.bats diff --git a/.github/workflows/ecosystem-tools.yaml b/.github/workflows/ecosystem-tools.yaml index eba0f63a0..f265a139c 100644 --- a/.github/workflows/ecosystem-tools.yaml +++ b/.github/workflows/ecosystem-tools.yaml @@ -49,3 +49,6 @@ jobs: - name: Run scrub tests run: | make bats-scrub + - name: Run anonymous-push-pull tests + run: | + make anonymous-push-pull diff --git a/Makefile b/Makefile index 24baaf377..b377ea4ca 100644 --- a/Makefile +++ b/Makefile @@ -315,3 +315,7 @@ fuzz-all: rm -rf test-data; \ bash test/scripts/fuzzAll.sh ${fuzztime}; \ rm -rf pkg/storage/testdata; \ + +.PHONY: anonymous-push-pull +anonymous-push-pull: binary check-skopeo $(BATS) + $(BATS) --trace --print-output-on-failure test/blackbox/anonymous_policiy.bats diff --git a/errors/errors.go b/errors/errors.go index c1f95a479..ee69022d1 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -54,4 +54,5 @@ var ( ErrSyncSignatureNotFound = errors.New("sync: couldn't find any upstream notary/cosign signatures") ErrSyncSignature = errors.New("sync: couldn't get upstream notary/cosign signatures") ErrImageLintAnnotations = errors.New("routes: lint checks failed") + ErrParsingAuthHeader = errors.New("auth: failed parsing authorization header") ) diff --git a/examples/README.md b/examples/README.md index 180566d5b..170edf749 100644 --- a/examples/README.md +++ b/examples/README.md @@ -49,13 +49,6 @@ Additionally, TLS configuration can be specified with: }, ``` -The registry can be deployed as a read-only service with: - -``` - "ReadOnly": false - }, -``` - ## Storage Configure storage with: @@ -209,7 +202,8 @@ create/update/delete can not be used without 'read' action, make sure read is al "actions": ["read", "create", "update"] } ], - "defaultPolicy": ["read", "create"] # default policy which is applied for all users => so all users can read/create repositories + "defaultPolicy": ["read", "create"], # default policy which is applied for authenticated users, other than "charlie"=> so these users can read/create repositories + "anonymousPolicy": ["read] # anonymous policy which is applied for unauthenticated users => so they can read repositories }, "tmp/**": { # matches all repos under tmp/ recursively "defaultPolicy": ["read", "create", "update"] # so all users have read/create/update on all repos under tmp/ eg: tmp/infra/repo diff --git a/examples/config-default-authz.json b/examples/config-anonymous-authz.json similarity index 81% rename from examples/config-default-authz.json rename to examples/config-anonymous-authz.json index ea41232de..f530e3cdf 100644 --- a/examples/config-default-authz.json +++ b/examples/config-anonymous-authz.json @@ -9,25 +9,25 @@ "realm": "zot", "accessControl": { "**": { - "defaultPolicy": [ + "anonymousPolicy": [ "read", "create" ] }, "tmp/**": { - "defaultPolicy": [ + "anonymousPolicy": [ "read", "create", "update" ] }, "infra/**": { - "defaultPolicy": [ + "anonymousPolicy": [ "read" ] }, "repos2/repo": { - "defaultPolicy": [ + "anonymousPolicy": [ "read" ] } diff --git a/examples/config-bench.json b/examples/config-bench.json index 99045d3a7..9cde9e587 100644 --- a/examples/config-bench.json +++ b/examples/config-bench.json @@ -5,8 +5,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug", diff --git a/examples/config-commit.json b/examples/config-commit.json index 90c2d9620..1e985e7ba 100644 --- a/examples/config-commit.json +++ b/examples/config-commit.json @@ -6,8 +6,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug" diff --git a/examples/config-example.json b/examples/config-example.json index 39267dab5..43f502b62 100644 --- a/examples/config-example.json +++ b/examples/config-example.json @@ -27,8 +27,7 @@ "path": "test/data/htpasswd" }, "failDelay": 5 - }, - "allowReadAccess": false + } }, "log": { "level": "debug", diff --git a/examples/config-example.yaml b/examples/config-example.yaml index 6ab3bd7ab..d33fc682e 100644 --- a/examples/config-example.yaml +++ b/examples/config-example.yaml @@ -1,7 +1,6 @@ distspecversion: 1.0.1-dev http: address: 127.0.0.1 - allowreadaccess: false auth: faildelay: 5 htpasswd: diff --git a/examples/config-gc.json b/examples/config-gc.json index b123722af..6033f2d37 100644 --- a/examples/config-gc.json +++ b/examples/config-gc.json @@ -7,8 +7,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug" diff --git a/examples/config-lint.json b/examples/config-lint.json index d46e56800..72fc7ad06 100644 --- a/examples/config-lint.json +++ b/examples/config-lint.json @@ -5,8 +5,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug" @@ -18,4 +17,4 @@ } } -} \ No newline at end of file +} diff --git a/examples/config-minimal.json b/examples/config-minimal.json index 3596f0ff3..206e3c04b 100644 --- a/examples/config-minimal.json +++ b/examples/config-minimal.json @@ -5,8 +5,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug" diff --git a/examples/config-multiple-cve.json b/examples/config-multiple-cve.json index 8ba86c5d9..cc2699c04 100644 --- a/examples/config-multiple-cve.json +++ b/examples/config-multiple-cve.json @@ -21,8 +21,7 @@ }, "http": { "address": "127.0.0.1", - "port": "5000", - "ReadOnly": false + "port": "5000" }, "log": { "level": "debug" diff --git a/examples/config-multiple.json b/examples/config-multiple.json index f6f2f2adf..be1dbee90 100644 --- a/examples/config-multiple.json +++ b/examples/config-multiple.json @@ -21,8 +21,7 @@ }, "http": { "address": "127.0.0.1", - "port": "5000", - "ReadOnly": false + "port": "5000" }, "log": { "level": "debug" diff --git a/examples/config-policy.json b/examples/config-policy.json index d481d3946..a23931770 100644 --- a/examples/config-policy.json +++ b/examples/config-policy.json @@ -15,6 +15,7 @@ }, "accessControl": { "**": { + "anonymousPolicy": ["read"], "policies": [ { "users": [ diff --git a/examples/config-ratelimit.json b/examples/config-ratelimit.json index 62ef488bf..27ced07a7 100644 --- a/examples/config-ratelimit.json +++ b/examples/config-ratelimit.json @@ -6,7 +6,6 @@ "http": { "address": "127.0.0.1", "port": "8080", - "ReadOnly": false, "Ratelimit": { "Rate": 10, "Methods": [ diff --git a/examples/config-s3.json b/examples/config-s3.json index 8b0911598..e345fdca9 100644 --- a/examples/config-s3.json +++ b/examples/config-s3.json @@ -52,8 +52,7 @@ }, "http": { "address": "127.0.0.1", - "port": "8080", - "ReadOnly": false + "port": "8080" }, "log": { "level": "debug" diff --git a/pkg/api/authn.go b/pkg/api/authn.go index 1d5ee4247..d5377b0a9 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -86,22 +86,6 @@ func noPasswdAuth(realm string, config *config.Config) mux.MiddlewareFunc { return } - if config.HTTP.AllowReadAccess && - config.HTTP.TLS.CACert != "" && - request.TLS.VerifiedChains == nil && - request.Method != http.MethodGet && request.Method != http.MethodHead { - authFail(response, realm, 5) //nolint:gomnd - - return - } - - if (request.Method != http.MethodGet && request.Method != http.MethodHead) && config.HTTP.ReadOnly { - // Reject modification requests in read-only mode - response.WriteHeader(http.StatusMethodNotAllowed) - - return - } - // Process request next.ServeHTTP(response, request) }) @@ -197,52 +181,30 @@ func basicAuthHandler(ctlr *Controller) mux.MiddlewareFunc { return } - if (request.Method == http.MethodGet || request.Method == http.MethodHead) && ctlr.Config.HTTP.AllowReadAccess { + if request.Header.Get("Authorization") == "" && checkAnonymousPolicyExists(ctlr.Config.AccessControl) { // Process request next.ServeHTTP(response, request) return } - if (request.Method != http.MethodGet && request.Method != http.MethodHead) && ctlr.Config.HTTP.ReadOnly { - response.WriteHeader(http.StatusMethodNotAllowed) - - return - } - - basicAuth := request.Header.Get("Authorization") - if basicAuth == "" { - authFail(response, realm, delay) - - return - } - - splitStr := strings.SplitN(basicAuth, " ", 2) //nolint:gomnd - - if len(splitStr) != 2 || strings.ToLower(splitStr[0]) != "basic" { - authFail(response, realm, delay) - - return - } - - decodedStr, err := base64.StdEncoding.DecodeString(splitStr[1]) + username, passphrase, err := getUsernamePasswordBasicAuth(request) if err != nil { + ctlr.Log.Error().Err(err).Msg("failed to parse authorization header") authFail(response, realm, delay) return } - pair := strings.SplitN(string(decodedStr), ":", 2) //nolint:gomnd - // nolint:gomnd - if len(pair) != 2 { - authFail(response, realm, delay) + // some client tools might send Authorization: Basic Og== (decoded into ":") + // empty username and password + if username == "" && passphrase == "" && checkAnonymousPolicyExists(ctlr.Config.AccessControl) { + // Process request + next.ServeHTTP(response, request) return } - username := pair[0] - passphrase := pair[1] - // first, HTTPPassword authN (which is local) passphraseHash, ok := credMap[username] if ok { @@ -297,3 +259,31 @@ func authFail(w http.ResponseWriter, realm string, delay int) { w.Header().Set("Content-Type", "application/json") WriteJSON(w, http.StatusUnauthorized, NewErrorList(NewError(UNAUTHORIZED))) } + +func getUsernamePasswordBasicAuth(request *http.Request) (string, string, error) { + basicAuth := request.Header.Get("Authorization") + + if basicAuth == "" { + return "", "", errors.ErrParsingAuthHeader + } + + splitStr := strings.SplitN(basicAuth, " ", 2) //nolint:gomnd + if len(splitStr) != 2 || strings.ToLower(splitStr[0]) != "basic" { + return "", "", errors.ErrParsingAuthHeader + } + + decodedStr, err := base64.StdEncoding.DecodeString(splitStr[1]) + if err != nil { + return "", "", err + } + + pair := strings.SplitN(string(decodedStr), ":", 2) //nolint:gomnd + if len(pair) != 2 { //nolint:gomnd + return "", "", errors.ErrParsingAuthHeader + } + + username := pair[0] + passphrase := pair[1] + + return username, passphrase, nil +} diff --git a/pkg/api/authz.go b/pkg/api/authz.go index fbe078ab6..71b19c4a2 100644 --- a/pkg/api/authz.go +++ b/pkg/api/authz.go @@ -2,10 +2,8 @@ package api import ( "context" - "encoding/base64" "fmt" "net/http" - "strings" "time" glob "github.com/bmatcuk/doublestar/v4" @@ -140,7 +138,14 @@ func isPermitted(username, action string, policyGroup config.PolicyGroup) bool { // check defaultPolicy if !result { - if common.Contains(policyGroup.DefaultPolicy, action) { + if common.Contains(policyGroup.DefaultPolicy, action) && username != "" { + result = true + } + } + + // check anonymousPolicy + if !result { + if common.Contains(policyGroup.AnonymousPolicy, action) && username == "" { result = true } } @@ -184,10 +189,19 @@ func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { acCtrlr := NewAccessController(ctlr.Config) // allow anonymous authz if no authn present and only default policies are present - username := "" + var username string + var err error - if isAuthnEnabled(ctlr.Config) { - username = getUsername(request) + /* To do: verify client certs and get its username(subject DN) + if request.TLS.VerifiedChains != nil, then get subject DN + issue: https: //github.com/project-zot/zot/issues/614 */ + + if isAuthnEnabled(ctlr.Config) && request.Header.Get("Authorization") != "" { + username, _, err = getUsernamePasswordBasicAuth(request) + + if err != nil { + authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) + } } ctx := acCtrlr.getContext(username, request) @@ -232,19 +246,23 @@ func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { } } -func getUsername(r *http.Request) string { - // this should work because it was already parsed in authn middleware - basicAuth := r.Header.Get("Authorization") - s := strings.SplitN(basicAuth, " ", 2) //nolint:gomnd - b, _ := base64.StdEncoding.DecodeString(s[1]) - pair := strings.SplitN(string(b), ":", 2) //nolint:gomnd - - return pair[0] -} - func authzFail(w http.ResponseWriter, realm string, delay int) { time.Sleep(time.Duration(delay) * time.Second) w.Header().Set("WWW-Authenticate", realm) w.Header().Set("Content-Type", "application/json") WriteJSON(w, http.StatusForbidden, NewErrorList(NewError(DENIED))) } + +func checkAnonymousPolicyExists(config *config.AccessControlConfig) bool { + if config == nil { + return false + } + + for _, repository := range config.Repositories { + if len(repository.AnonymousPolicy) > 0 { + return true + } + } + + return false +} diff --git a/pkg/api/config/config.go b/pkg/api/config/config.go index 6dab25330..76ada3f08 100644 --- a/pkg/api/config/config.go +++ b/pkg/api/config/config.go @@ -68,8 +68,6 @@ type HTTPConfig struct { Auth *AuthConfig RawAccessControl map[string]interface{} `mapstructure:"accessControl,omitempty"` Realm string - AllowReadAccess bool `mapstructure:",omitempty"` - ReadOnly bool `mapstructure:",omitempty"` Ratelimit *RatelimitConfig `mapstructure:",omitempty"` } @@ -112,8 +110,9 @@ type AccessControlConfig struct { type Repositories map[string]PolicyGroup type PolicyGroup struct { - Policies []Policy - DefaultPolicy []string + Policies []Policy + DefaultPolicy []string + AnonymousPolicy []string } type Policy struct { @@ -193,8 +192,11 @@ func (c *Config) LoadAccessControlConfig(viperInstance *viper.Viper) error { } defaultPolicy := viperInstance.GetStringSlice(fmt.Sprintf("http::accessControl::%s::defaultPolicy", policy)) - policyGroup.Policies = policies policyGroup.DefaultPolicy = defaultPolicy + + anonymousPolicy := viperInstance.GetStringSlice(fmt.Sprintf("http::accessControl::%s::anonymousPolicy", policy)) + policyGroup.Policies = policies + policyGroup.AnonymousPolicy = anonymousPolicy c.AccessControl.Repositories[policy] = policyGroup } diff --git a/pkg/api/controller.go b/pkg/api/controller.go index c4e34ef22..bb764968e 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -190,7 +190,8 @@ func (c *Controller) Run(reloadCtx context.Context) error { if c.Config.HTTP.TLS.CACert != "" { clientAuth := tls.VerifyClientCertIfGiven - if (c.Config.HTTP.Auth == nil || c.Config.HTTP.Auth.HTPasswd.Path == "") && !c.Config.HTTP.AllowReadAccess { + if (c.Config.HTTP.Auth == nil || c.Config.HTTP.Auth.HTPasswd.Path == "") && + !checkAnonymousPolicyExists(c.Config.AccessControl) { clientAuth = tls.RequireAndVerifyClientCert } diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index ed9908931..2aee0463a 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -928,7 +928,14 @@ func TestTLSWithBasicAuthAllowReadAccess(t *testing.T) { Cert: ServerCert, Key: ServerKey, } - conf.HTTP.AllowReadAccess = true + + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + AnonymousPolicy: []string{"read"}, + }, + }, + } ctlr := api.NewController(conf) ctlr.Config.Storage.RootDirectory = t.TempDir() @@ -960,7 +967,7 @@ func TestTLSWithBasicAuthAllowReadAccess(t *testing.T) { // without creds, writes should fail resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) }) } @@ -1051,7 +1058,14 @@ func TestTLSMutualAuthAllowReadAccess(t *testing.T) { Key: ServerKey, CACert: CACert, } - conf.HTTP.AllowReadAccess = true + + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + AnonymousPolicy: []string{"read"}, + }, + }, + } ctlr := api.NewController(conf) ctlr.Config.Storage.RootDirectory = t.TempDir() @@ -1079,7 +1093,7 @@ func TestTLSMutualAuthAllowReadAccess(t *testing.T) { // without creds, writes should fail resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) // setup TLS mutual auth cert, err := tls.LoadX509KeyPair("../../test/data/client.cert", "../../test/data/client.key") @@ -1210,7 +1224,14 @@ func TestTLSMutualAndBasicAuthAllowReadAccess(t *testing.T) { Key: ServerKey, CACert: CACert, } - conf.HTTP.AllowReadAccess = true + + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + AnonymousPolicy: []string{"read"}, + }, + }, + } ctlr := api.NewController(conf) ctlr.Config.Storage.RootDirectory = t.TempDir() @@ -1252,7 +1273,7 @@ func TestTLSMutualAndBasicAuthAllowReadAccess(t *testing.T) { // with only client certs, writes should fail resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) // with client certs and creds, should get expected status code resp, _ = resty.R().SetBasicAuth(username, passphrase).Get(secureBaseURL) @@ -1620,10 +1641,17 @@ func TestBearerAuthWithAllowReadAccess(t *testing.T) { Service: aurl.Host, }, } - conf.HTTP.AllowReadAccess = true ctlr := api.NewController(conf) ctlr.Config.Storage.RootDirectory = t.TempDir() + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + AnonymousPolicy: []string{"read"}, + }, + }, + } + go startServer(ctlr) defer stopServer(ctlr) test.WaitTillServerReady(baseURL) @@ -2360,7 +2388,60 @@ func TestAuthorizationWithBasicAuth(t *testing.T) { }) } -func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { +func TestGetUsername(t *testing.T) { + Convey("Make a new controller", t, func() { + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + + htpasswdPath := test.MakeHtpasswdFileFromString(getCredString(username, passphrase)) + defer os.Remove(htpasswdPath) + + conf := config.New() + conf.HTTP.Port = port + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + } + + ctlr := api.NewController(conf) + dir := t.TempDir() + ctlr.Config.Storage.RootDirectory = dir + + go startServer(ctlr) + defer stopServer(ctlr) + test.WaitTillServerReady(baseURL) + + resp, err := resty.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + // test base64 encode + resp, err = resty.R().SetHeader("Authorization", "Basic should fail").Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + // test "username:password" encoding + resp, err = resty.R().SetHeader("Authorization", "Basic dGVzdA==").Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + // failed parsing authorization header + resp, err = resty.R().SetHeader("Authorization", "Basic ").Get(baseURL + "/v2/_catalog") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusUnauthorized) + + var e api.Error + err = json.Unmarshal(resp.Body(), &e) + So(err, ShouldBeNil) + }) +} + +func TestAuthorizationWithOnlyAnonymousPolicy(t *testing.T) { Convey("Make a new controller", t, func() { const TestRepo = "my-repos/repo" port := test.GetFreePort() @@ -2372,7 +2453,7 @@ func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { conf.AccessControl = &config.AccessControlConfig{ Repositories: config.Repositories{ TestRepo: config.PolicyGroup{ - DefaultPolicy: []string{}, + AnonymousPolicy: []string{}, }, }, } @@ -2408,21 +2489,19 @@ func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) if entry, ok := conf.AccessControl.Repositories[TestRepo]; ok { - entry.DefaultPolicy = []string{"create", "read"} + entry.AnonymousPolicy = []string{"create", "read"} conf.AccessControl.Repositories[TestRepo] = entry } // now it should get 202 - resp, err = resty.R().SetBasicAuth(username, passphrase). - Post(baseURL + "/v2/" + TestRepo + "/blobs/uploads/") + resp, err = resty.R().Post(baseURL + "/v2/" + TestRepo + "/blobs/uploads/") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) loc := resp.Header().Get("Location") // uploading blob should get 201 - resp, err = resty.R().SetBasicAuth(username, passphrase). - SetHeader("Content-Length", fmt.Sprintf("%d", len(blob))). + resp, err = resty.R().SetHeader("Content-Length", fmt.Sprintf("%d", len(blob))). SetHeader("Content-Type", "application/octet-stream"). SetQueryParam("digest", digest). SetBody(blob). @@ -2433,16 +2512,14 @@ func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { cblob, cdigest := test.GetRandomImageConfig() - resp, err = resty.R().SetBasicAuth(username, passphrase). - Post(baseURL + "/v2/" + TestRepo + "/blobs/uploads/") + resp, err = resty.R().Post(baseURL + "/v2/" + TestRepo + "/blobs/uploads/") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) loc = test.Location(baseURL, resp) // uploading blob should get 201 - resp, err = resty.R().SetBasicAuth(username, passphrase). - SetHeader("Content-Length", fmt.Sprintf("%d", len(cblob))). + resp, err = resty.R().SetHeader("Content-Length", fmt.Sprintf("%d", len(cblob))). SetHeader("Content-Type", "application/octet-stream"). SetQueryParam("digest", cdigest.String()). SetBody(cblob). @@ -2531,7 +2608,7 @@ func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { // add update perm on repo if entry, ok := conf.AccessControl.Repositories[TestRepo]; ok { - entry.DefaultPolicy = []string{"create", "read", "update"} + entry.AnonymousPolicy = []string{"create", "read", "update"} conf.AccessControl.Repositories[TestRepo] = entry } @@ -2554,6 +2631,160 @@ func TestAuthorizationWithOnlyDefaultPolicy(t *testing.T) { }) } +func TestAuthorizationWithMultiplePolicies(t *testing.T) { + Convey("Make a new controller", t, func() { + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + + conf := config.New() + conf.HTTP.Port = port + // have two users: "test" user for user Policy, and "bob" for default policy + htpasswdPath := test.MakeHtpasswdFileFromString(getCredString(username, passphrase) + + "\n" + getCredString("bob", passphrase)) + defer os.Remove(htpasswdPath) + + conf.HTTP.Auth = &config.AuthConfig{ + HTPasswd: config.AuthHTPasswd{ + Path: htpasswdPath, + }, + } + // config with all policy types, to test that the correct one is applied in each case + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + Policies: []config.Policy{ + { + Users: []string{}, + Actions: []string{}, + }, + }, + DefaultPolicy: []string{}, + AnonymousPolicy: []string{}, + }, + }, + AdminPolicy: config.Policy{ + Users: []string{}, + Actions: []string{}, + }, + } + + ctlr := api.NewController(conf) + dir := t.TempDir() + err := test.CopyFiles("../../test/data", dir) + if err != nil { + panic(err) + } + ctlr.Config.Storage.RootDirectory = dir + + go startServer(ctlr) + defer stopServer(ctlr) + test.WaitTillServerReady(baseURL) + + blob := []byte("hello, blob!") + digest := godigest.FromBytes(blob).String() + + // unauthenticated clients should not have access to /v2/, no policy is applied since none exists + resp, err := resty.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, 401) + + repoPolicy := conf.AccessControl.Repositories[AuthorizationAllRepos] + repoPolicy.AnonymousPolicy = append(repoPolicy.AnonymousPolicy, "read") + conf.AccessControl.Repositories[AuthorizationAllRepos] = repoPolicy + + // should have access to /v2/, anonymous policy is applied, "read" allowed + resp, err = resty.R().Get(baseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // add "test" user to global policy with create permission + repoPolicy.Policies[0].Users = append(repoPolicy.Policies[0].Users, "test") + repoPolicy.Policies[0].Actions = append(repoPolicy.Policies[0].Actions, "create") + + // now it should get 202, user has the permission set on "create" + resp, err = resty.R().SetBasicAuth(username, passphrase). + Post(baseURL + "/v2/" + AuthorizationNamespace + "/blobs/uploads/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + loc := resp.Header().Get("Location") + + // uploading blob should get 201 + resp, err = resty.R().SetBasicAuth(username, passphrase). + SetHeader("Content-Length", fmt.Sprintf("%d", len(blob))). + SetHeader("Content-Type", "application/octet-stream"). + SetQueryParam("digest", digest). + SetBody(blob). + Put(baseURL + loc) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + + // head blob should get 403 without read perm + resp, err = resty.R().SetBasicAuth(username, passphrase). + Head(baseURL + "/v2/" + AuthorizationNamespace + "/blobs/" + digest) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + // get tags without read access should get 403 + resp, err = resty.R().SetBasicAuth(username, passphrase). + Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + repoPolicy.DefaultPolicy = append(repoPolicy.DefaultPolicy, "read") + conf.AccessControl.Repositories[AuthorizationAllRepos] = repoPolicy + + // with read permission should get 200, because default policy allows reading now + resp, err = resty.R().SetBasicAuth(username, passphrase). + Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // get tags with default read access should be ok, since the user is now "bob" and default policy is applied + resp, err = resty.R().SetBasicAuth("bob", passphrase). + Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // get tags with default policy read access + resp, err = resty.R().Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // get tags with anonymous read access should be ok + resp, err = resty.R().Get(baseURL + "/v2/" + AuthorizationNamespace + "/tags/list") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // without create permission should get 403, since "bob" can only read(default policy applied) + resp, err = resty.R().SetBasicAuth("bob", passphrase). + Post(baseURL + "/v2/" + AuthorizationNamespace + "/blobs/uploads/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + // add read permission to user "bob" + conf.AccessControl.AdminPolicy.Users = append(conf.AccessControl.AdminPolicy.Users, "bob") + conf.AccessControl.AdminPolicy.Actions = append(conf.AccessControl.AdminPolicy.Actions, "create") + + // added create permission to user "bob", should be allowed now + resp, err = resty.R().SetBasicAuth("bob", passphrase). + Post(baseURL + "/v2/" + AuthorizationNamespace + "/blobs/uploads/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + }) +} + func TestInvalidCases(t *testing.T) { Convey("Invalid repo dir", t, func() { port := test.GetFreePort() @@ -2627,7 +2858,13 @@ func TestHTTPReadOnly(t *testing.T) { conf := config.New() conf.HTTP.Port = port // enable read-only mode - conf.HTTP.ReadOnly = true + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + DefaultPolicy: []string{"read"}, + }, + }, + } htpasswdPath := test.MakeHtpasswdFileFromString(testString) defer os.Remove(htpasswdPath) @@ -2653,7 +2890,7 @@ func TestHTTPReadOnly(t *testing.T) { Post(baseURL + "/v2/" + AuthorizedNamespace + "/blobs/uploads/") So(err, ShouldBeNil) So(resp, ShouldNotBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusMethodNotAllowed) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) // with invalid creds, it should fail resp, _ = resty.R().SetBasicAuth("chuck", "chuck").Get(baseURL + "/v2/") diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 2eddb91cb..4d3d9fc3e 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -140,7 +140,7 @@ func (rh *RouteHandler) CheckVersionSupport(response http.ResponseWriter, reques response.Header().Set(constants.DistAPIVersion, "registry/2.0") // NOTE: compatibility workaround - return this header in "allowed-read" mode to allow for clients to // work correctly - if rh.c.Config.HTTP.AllowReadAccess { + if checkAnonymousPolicyExists(rh.c.Config.AccessControl) { if rh.c.Config.HTTP.Auth != nil { if rh.c.Config.HTTP.Auth.Bearer != nil { response.Header().Set("WWW-Authenticate", fmt.Sprintf("bearer realm=%s", rh.c.Config.HTTP.Auth.Bearer.Realm)) diff --git a/pkg/cli/root.go b/pkg/cli/root.go index bc63fa6e7..58bf25f01 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/root.go @@ -217,9 +217,9 @@ func validateConfiguration(config *config.Config) error { // check authorization config, it should have basic auth enabled or ldap if config.HTTP.RawAccessControl != nil { - if config.HTTP.Auth == nil || (config.HTTP.Auth.HTPasswd.Path == "" && config.HTTP.Auth.LDAP == nil) { - // checking for default policy only authorization config: no users, no policies but default policy - checkForDefaultPolicyConfig(config) + // checking for anonymous policy only authorization config: no users, no policies but anonymous policy + if err := validateAuthzPolicies(config); err != nil { + return err } } @@ -272,13 +272,17 @@ func validateConfiguration(config *config.Config) error { return nil } -func checkForDefaultPolicyConfig(config *config.Config) { - if !isDefaultPolicyConfig(config) { +func validateAuthzPolicies(config *config.Config) error { + if (config.HTTP.Auth == nil || (config.HTTP.Auth.HTPasswd.Path == "" && config.HTTP.Auth.LDAP == nil)) && + !doesAuthzContainOnlyAnonymousPolicies(config) { log.Error().Err(errors.ErrBadConfig). Msg("access control config requires httpasswd, ldap authentication " + - "or using only 'defaultPolicy' policies") - panic(errors.ErrBadConfig) + "or using only 'anonymousPolicy' policies") + + return errors.ErrBadConfig } + + return nil } func applyDefaultValues(config *config.Config, viperInstance *viper.Viper) { @@ -408,31 +412,44 @@ func LoadConfiguration(config *config.Config, configPath string) error { return nil } -func isDefaultPolicyConfig(cfg *config.Config) bool { +func doesAuthzContainOnlyAnonymousPolicies(cfg *config.Config) bool { adminPolicy := cfg.AccessControl.AdminPolicy + anonymousPolicyPresent := false - log.Info().Msg("checking if default authorization is possible") + log.Info().Msg("checking if anonymous authorization is the only type of authorization policy configured") if len(adminPolicy.Actions)+len(adminPolicy.Users) > 0 { - log.Info().Msg("admin policy detected, default authorization disabled") + log.Info().Msg("admin policy detected, anonymous authorization is not the only authorization policy configured") return false } for _, repository := range cfg.AccessControl.Repositories { + if len(repository.DefaultPolicy) > 0 { + log.Info().Interface("repository", repository). + Msg("default policy detected, anonymous authorization is not the only authorization policy configured") + + return false + } + + if len(repository.AnonymousPolicy) > 0 { + log.Info().Msg("anonymous authorization detected") + + anonymousPolicyPresent = true + } + for _, policy := range repository.Policies { if len(policy.Actions)+len(policy.Users) > 0 { log.Info().Interface("repository", repository). - Msg("repository with non-empty policy detected, default authorization disabled") + Msg("repository with non-empty policy detected, " + + "anonymous authorization is not the only authorization policy configured") return false } } } - log.Info().Msg("default authorization detected") - - return true + return anonymousPolicyPresent } func validateLDAP(config *config.Config) error { diff --git a/pkg/cli/root_test.go b/pkg/cli/root_test.go index 205ed35f7..24e4db6b6 100644 --- a/pkg/cli/root_test.go +++ b/pkg/cli/root_test.go @@ -174,14 +174,14 @@ func TestVerify(t *testing.T) { So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldNotPanic) }) - Convey("Test verify default authorization", t, func(c C) { + Convey("Test verify anonymous authorization", t, func(c C) { tmpfile, err := ioutil.TempFile("", "zot-test*.json") So(err, ShouldBeNil) defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", - "accessControl":{"**":{"defaultPolicy": ["read", "create"]}, - "/repo":{"defaultPolicy": ["read", "create"]} + "accessControl":{"**":{"anonymousPolicy": ["read", "create"]}, + "/repo":{"anonymousPolicy": ["read", "create"]} }}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -198,7 +198,7 @@ func TestVerify(t *testing.T) { content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "accessControl":{"**":{"defaultPolicy": ["read", "create"]}, - "/repo":{"defaultPolicy": ["read", "create"]}, + "/repo":{"anonymousPolicy": ["read", "create"]}, "adminPolicy":{"users":["admin"], "actions":["read","create","update","delete"]} }}}`) @@ -217,7 +217,7 @@ func TestVerify(t *testing.T) { content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "accessControl":{"**":{"defaultPolicy": ["read", "create"]}, - "/repo":{"defaultPolicy": ["read", "create"]}, + "/repo":{"anonymousPolicy": ["read", "create"]}, "/repo2":{"policies": [{ "users": ["charlie"], "actions": ["read", "create", "update"]}]} @@ -289,7 +289,7 @@ func TestVerify(t *testing.T) { content := []byte(`{"storage":{"rootDirectory":"/tmp/zot"}, "http":{"address":"127.0.0.1","port":"8080","realm":"zot", "auth":{"htpasswd":{"path":"test/data/htpasswd"},"failDelay":1}, - "accessControl":{"[":{"policies":[],"defaultPolicy":[]}}}}`) + "accessControl":{"[":{"policies":[],"anonymousPolicy":[]}}}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) err = tmpfile.Close() @@ -339,7 +339,7 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot"}, - "http": {"url": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "http": {"url": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -355,7 +355,7 @@ func TestVerify(t *testing.T) { defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot"}, "http": {"auth": {"ldap": {"address": "ldap", "userattribute": "uid"}}, - "address": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "address": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -371,7 +371,7 @@ func TestVerify(t *testing.T) { defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot"}, "http": {"auth": {"ldap": {"basedn": "ou=Users,dc=example,dc=org", "userattribute": "uid"}}, - "address": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "address": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -387,7 +387,7 @@ func TestVerify(t *testing.T) { defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot"}, "http": {"auth": {"ldap": {"basedn": "ou=Users,dc=example,dc=org", "address": "ldap"}}, - "address": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "address": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -402,7 +402,7 @@ func TestVerify(t *testing.T) { So(err, ShouldBeNil) defer os.Remove(tmpfile.Name()) // clean up content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot"}, - "http": {"address": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "http": {"address": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) _, err = tmpfile.Write(content) So(err, ShouldBeNil) @@ -503,7 +503,7 @@ func TestGC(t *testing.T) { defer os.Remove(file.Name()) content := []byte(`{"distSpecVersion": "1.0.0", "storage": {"rootDirectory": "/tmp/zot", - "gc": false}, "http": {"address": "127.0.0.1", "port": "8080", "ReadOnly": false}, + "gc": false}, "http": {"address": "127.0.0.1", "port": "8080"}, "log": {"level": "debug"}}`) err = ioutil.WriteFile(file.Name(), content, 0o600) diff --git a/pkg/extensions/sync/sync_test.go b/pkg/extensions/sync/sync_test.go index 90e6edd13..7eb572af8 100644 --- a/pkg/extensions/sync/sync_test.go +++ b/pkg/extensions/sync/sync_test.go @@ -807,7 +807,7 @@ func TestConfigReloader(t *testing.T) { }() content := fmt.Sprintf(`{"distSpecVersion": "0.1.0-dev", "storage": {"rootDirectory": "%s"}, - "http": {"address": "127.0.0.1", "port": "%s", "ReadOnly": false}, + "http": {"address": "127.0.0.1", "port": "%s"}, "log": {"level": "debug", "output": "%s"}}`, destDir, destPort, logFile.Name()) cfgfile, err := ioutil.TempFile("", "zot-test*.json") diff --git a/test/blackbox/anonymous_policiy.bats b/test/blackbox/anonymous_policiy.bats new file mode 100644 index 000000000..ccbfb4c88 --- /dev/null +++ b/test/blackbox/anonymous_policiy.bats @@ -0,0 +1,91 @@ +load helpers_pushpull + +function setup_file() { + # Verify prerequisites are available + if ! verify_prerequisites; then + exit 1 + fi + + # Download test data to folder common for the entire suite, not just this file + skopeo --insecure-policy copy --format=oci docker://ghcr.io/project-zot/golang:1.18 oci:${TEST_DATA_DIR}/golang:1.18 + # Setup zot server + local zot_root_dir=${BATS_FILE_TMPDIR}/zot + local zot_config_file=${BATS_FILE_TMPDIR}/zot_config.json + local oci_data_dir=${BATS_FILE_TMPDIR}/oci + local htpasswordFile=${BATS_FILE_TMPDIR}/htpasswd + mkdir -p ${zot_root_dir} + mkdir -p ${oci_data_dir} + echo 'test:$2a$10$EIIoeCnvsIDAJeDL4T1sEOnL2fWOvsq7ACZbs3RT40BBBXg.Ih7V.' >> ${htpasswordFile} + cat > ${zot_config_file}< Date: Thu, 28 Jul 2022 17:49:35 +0300 Subject: [PATCH 2/4] Remove check and set header every time Signed-off-by: Nicol Draghici --- pkg/api/routes.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/api/routes.go b/pkg/api/routes.go index 4d3d9fc3e..182e11c16 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -140,13 +140,11 @@ func (rh *RouteHandler) CheckVersionSupport(response http.ResponseWriter, reques response.Header().Set(constants.DistAPIVersion, "registry/2.0") // NOTE: compatibility workaround - return this header in "allowed-read" mode to allow for clients to // work correctly - if checkAnonymousPolicyExists(rh.c.Config.AccessControl) { - if rh.c.Config.HTTP.Auth != nil { - if rh.c.Config.HTTP.Auth.Bearer != nil { - response.Header().Set("WWW-Authenticate", fmt.Sprintf("bearer realm=%s", rh.c.Config.HTTP.Auth.Bearer.Realm)) - } else { - response.Header().Set("WWW-Authenticate", fmt.Sprintf("basic realm=%s", rh.c.Config.HTTP.Realm)) - } + if rh.c.Config.HTTP.Auth != nil { + if rh.c.Config.HTTP.Auth.Bearer != nil { + response.Header().Set("WWW-Authenticate", fmt.Sprintf("bearer realm=%s", rh.c.Config.HTTP.Auth.Bearer.Realm)) + } else { + response.Header().Set("WWW-Authenticate", fmt.Sprintf("basic realm=%s", rh.c.Config.HTTP.Realm)) } } From 870c88de1dab8df9900718a2dfffeaab059c90af Mon Sep 17 00:00:00 2001 From: Nicol Draghici Date: Tue, 2 Aug 2022 18:13:43 +0300 Subject: [PATCH 3/4] Get username when using certificates Signed-off-by: Nicol Draghici --- examples/config-tls.json | 9 +++- pkg/api/authn.go | 7 ++++ pkg/api/authz.go | 31 +++++++++----- pkg/api/controller_test.go | 84 ++++++++++++++++++++++++++++++++++++++ pkg/cli/root.go | 2 +- 5 files changed, 120 insertions(+), 13 deletions(-) diff --git a/examples/config-tls.json b/examples/config-tls.json index 6ebfa697c..156314f09 100644 --- a/examples/config-tls.json +++ b/examples/config-tls.json @@ -9,7 +9,14 @@ "realm": "zot", "tls": { "cert": "test/data/server.cert", - "key": "test/data/server.key" + "key": "test/data/server.key", + "cacert": "test/data/ca.crt" + }, + "accessControl": { + "**": { + "anonymousPolicy": ["read"], + "defaultPolicy": ["read"] + } } }, "log": { diff --git a/pkg/api/authn.go b/pkg/api/authn.go index d5377b0a9..b15604ec7 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -190,6 +190,13 @@ func basicAuthHandler(ctlr *Controller) mux.MiddlewareFunc { username, passphrase, err := getUsernamePasswordBasicAuth(request) if err != nil { + if request.TLS.VerifiedChains != nil && checkAnonymousPolicyExists(ctlr.Config.AccessControl) { + // Process request + next.ServeHTTP(response, request) + + return + } + ctlr.Log.Error().Err(err).Msg("failed to parse authorization header") authFail(response, realm, delay) diff --git a/pkg/api/authz.go b/pkg/api/authz.go index 71b19c4a2..2f0a69333 100644 --- a/pkg/api/authz.go +++ b/pkg/api/authz.go @@ -192,17 +192,26 @@ func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { var username string var err error - /* To do: verify client certs and get its username(subject DN) - if request.TLS.VerifiedChains != nil, then get subject DN - issue: https: //github.com/project-zot/zot/issues/614 */ - - if isAuthnEnabled(ctlr.Config) && request.Header.Get("Authorization") != "" { - username, _, err = getUsernamePasswordBasicAuth(request) - - if err != nil { - authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) - } - } + // allow anonymous authz if no authn present and only default policies are present + username = "" + if isAuthnEnabled(ctlr.Config) && request.Header.Get("Authorization") != "" { + username, _, err = getUsernamePasswordBasicAuth(request) + // no tls mutual auth + if err != nil && request.TLS.VerifiedChains == nil { + authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) + } + } + + // still no username, get it from tls certs + if username == "" && request.TLS.VerifiedChains != nil { + for _, cert := range request.TLS.PeerCertificates { + username = cert.Subject.CommonName + } + } + // if we still don't have a username and anon policy is not present + if username == "" && !checkAnonymousPolicyExists(ctlr.Config.AccessControl) { + authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) + } ctx := acCtrlr.getContext(username, request) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 2aee0463a..66cc40559 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -971,6 +971,90 @@ func TestTLSWithBasicAuthAllowReadAccess(t *testing.T) { }) } +func TestTLSWithDefaultUserPermission(t *testing.T) { + Convey("Make a new controller", t, func() { + caCert, err := ioutil.ReadFile(CACert) + So(err, ShouldBeNil) + caCertPool := x509.NewCertPool() + caCertPool.AppendCertsFromPEM(caCert) + htpasswdPath := test.MakeHtpasswdFile() + defer os.Remove(htpasswdPath) + + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + secureBaseURL := test.GetSecureBaseURL(port) + + resty.SetTLSClientConfig(&tls.Config{RootCAs: caCertPool, MinVersion: tls.VersionTLS12}) + defer func() { resty.SetTLSClientConfig(nil) }() + conf := config.New() + conf.HTTP.Port = port + + conf.HTTP.TLS = &config.TLSConfig{ + Cert: ServerCert, + Key: ServerKey, + CACert: CACert, + } + + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + Policies: []config.Policy{ + { + Users: []string{"*"}, + Actions: []string{"read"}, + }, + }, + }, + }, + } + + ctlr := api.NewController(conf) + ctlr.Config.Storage.RootDirectory = t.TempDir() + + go startServer(ctlr) + defer stopServer(ctlr) + test.WaitTillServerReady(baseURL) + + resp, err := resty.R().Get(baseURL) + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusBadRequest) + + repoPolicy := conf.AccessControl.Repositories[AuthorizationAllRepos] + + // setup TLS mutual auth + cert, err := tls.LoadX509KeyPair("../../test/data/client.cert", "../../test/data/client.key") + So(err, ShouldBeNil) + + resty.SetCertificates(cert) + defer func() { resty.SetCertificates(tls.Certificate{}) }() + + // with client certs but without creds, should succeed + resp, err = resty.R().Get(secureBaseURL + "/v2/") + So(err, ShouldBeNil) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusOK) + + // with creds, should get expected status code + resp, _ = resty.R().Get(secureBaseURL) + So(resp, ShouldNotBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + + // without creds, writes should fail + resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusForbidden) + + // empty default authorization and give user the permission to create + repoPolicy.Policies[0].Actions = append(repoPolicy.Policies[0].Actions, "create") + conf.AccessControl.Repositories[AuthorizationAllRepos] = repoPolicy + resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + + }) +} + func TestTLSMutualAuth(t *testing.T) { Convey("Make a new controller", t, func() { caCert, err := ioutil.ReadFile(CACert) diff --git a/pkg/cli/root.go b/pkg/cli/root.go index 58bf25f01..39316789d 100644 --- a/pkg/cli/root.go +++ b/pkg/cli/root.go @@ -425,7 +425,7 @@ func doesAuthzContainOnlyAnonymousPolicies(cfg *config.Config) bool { } for _, repository := range cfg.AccessControl.Repositories { - if len(repository.DefaultPolicy) > 0 { + if len(repository.DefaultPolicy) > 0 && cfg.HTTP.TLS == nil { log.Info().Interface("repository", repository). Msg("default policy detected, anonymous authorization is not the only authorization policy configured") From d6672dbe39849ecbd768ef2f63cd5d11d4ddac09 Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani Date: Fri, 29 Jul 2022 14:12:57 +0000 Subject: [PATCH 4/4] fix dependabot alerts https://github.com/project-zot/zot/pull/682 Signed-off-by: Ramkumar Chinchani --- pkg/api/authn.go | 2 +- pkg/api/authz.go | 40 ++++++++++++++++++++------------------ pkg/api/controller_test.go | 3 +-- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/pkg/api/authn.go b/pkg/api/authn.go index b15604ec7..22ec74729 100644 --- a/pkg/api/authn.go +++ b/pkg/api/authn.go @@ -193,7 +193,7 @@ func basicAuthHandler(ctlr *Controller) mux.MiddlewareFunc { if request.TLS.VerifiedChains != nil && checkAnonymousPolicyExists(ctlr.Config.AccessControl) { // Process request next.ServeHTTP(response, request) - + return } diff --git a/pkg/api/authz.go b/pkg/api/authz.go index 2f0a69333..3faa8c564 100644 --- a/pkg/api/authz.go +++ b/pkg/api/authz.go @@ -193,25 +193,27 @@ func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { var err error // allow anonymous authz if no authn present and only default policies are present - username = "" - if isAuthnEnabled(ctlr.Config) && request.Header.Get("Authorization") != "" { - username, _, err = getUsernamePasswordBasicAuth(request) - // no tls mutual auth - if err != nil && request.TLS.VerifiedChains == nil { - authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) - } - } - - // still no username, get it from tls certs - if username == "" && request.TLS.VerifiedChains != nil { - for _, cert := range request.TLS.PeerCertificates { - username = cert.Subject.CommonName - } - } - // if we still don't have a username and anon policy is not present - if username == "" && !checkAnonymousPolicyExists(ctlr.Config.AccessControl) { - authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) - } + username = "" + if isAuthnEnabled(ctlr.Config) && request.Header.Get("Authorization") != "" { + username, _, err = getUsernamePasswordBasicAuth(request) + // no tls mutual auth + if err != nil && request.TLS.VerifiedChains == nil { + authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) + } + } + + // still no username, get it from tls certs + verifiedChains := request.TLS.VerifiedChains + if username == "" && verifiedChains != nil && + len(verifiedChains) > 0 && len(verifiedChains[0]) > 0 { + for _, cert := range request.TLS.PeerCertificates { + username = cert.Subject.CommonName + } + } + // if we still don't have a username and anon policy is not present + if username == "" && !checkAnonymousPolicyExists(ctlr.Config.AccessControl) { + authFail(response, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay) + } ctx := acCtrlr.getContext(username, request) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 66cc40559..2dc745b79 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -971,7 +971,7 @@ func TestTLSWithBasicAuthAllowReadAccess(t *testing.T) { }) } -func TestTLSWithDefaultUserPermission(t *testing.T) { +func TestTLSWithUserPermissions(t *testing.T) { Convey("Make a new controller", t, func() { caCert, err := ioutil.ReadFile(CACert) So(err, ShouldBeNil) @@ -1051,7 +1051,6 @@ func TestTLSWithDefaultUserPermission(t *testing.T) { resp, err = resty.R().Post(secureBaseURL + "/v2/repo/blobs/uploads/") So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) - }) }