From 168d21da1e259b75cf76719299d17c0784c5b330 Mon Sep 17 00:00:00 2001 From: peusebiu Date: Fri, 18 Nov 2022 19:35:28 +0200 Subject: [PATCH] fix(storage): deleting manifests with identical digests (#951) Suppose we push two identical manifests (sharing same digest) but with different tags, then deleting by digest should throw an error otherwise we end up deleting all image tags (with gc) or dangling references (without gc) This behaviour is controlled via Authorization, added a new policy action named detectManifestsCollision which enables this behaviour Signed-off-by: Ramkumar Chinchani Signed-off-by: Petu Eusebiu Co-authored-by: Ramkumar Chinchani --- Makefile | 20 +-- errors/errors.go | 1 + examples/README.md | 21 ++- examples/config-policy.json | 4 +- pkg/api/authz.go | 68 ++++----- pkg/api/controller_test.go | 134 ++++++++++++++++++ pkg/api/routes.go | 37 +++-- pkg/api/routes_test.go | 55 ++++++- pkg/extensions/search/resolver.go | 36 ++--- pkg/extensions/search/resolver_test.go | 8 +- pkg/extensions/sync/sync_test.go | 2 +- pkg/requestcontext/context.go | 59 +++++++- pkg/storage/common.go | 23 +-- pkg/storage/local/local.go | 8 +- pkg/storage/local/local_test.go | 10 +- pkg/storage/s3/s3.go | 8 +- pkg/storage/s3/s3_test.go | 20 +-- pkg/storage/storage.go | 2 +- pkg/storage/storage_test.go | 14 +- pkg/test/mocks/image_store_mock.go | 6 +- ...ous_policiy.bats => anonymous_policy.bats} | 3 - test/blackbox/detect_manifest_collision.bats | 109 ++++++++++++++ 22 files changed, 507 insertions(+), 141 deletions(-) rename test/blackbox/{anonymous_policiy.bats => anonymous_policy.bats} (96%) create mode 100644 test/blackbox/detect_manifest_collision.bats diff --git a/Makefile b/Makefile index 0c0916e6..93c2e6e9 100644 --- a/Makefile +++ b/Makefile @@ -314,6 +314,18 @@ test-bats-metrics-verbose: EXTENSIONS=metrics test-bats-metrics-verbose: binary check-skopeo $(BATS) $(BATS) --trace -p --verbose-run --print-output-on-failure --show-output-of-passing-tests test/blackbox/metrics.bats +.PHONY: test-anonymous-push-pull +test-anonymous-push-pull: binary check-skopeo $(BATS) + $(BATS) --trace --print-output-on-failure test/blackbox/anonymous_policy.bats + +.PHONY: test-annotations +test-annotations: binary check-skopeo $(BATS) $(STACKER) $(NOTATION) $(COSIGN) + $(BATS) --trace --print-output-on-failure test/blackbox/annotations.bats + +.PHONY: test-detect-manifest-collision +test-detect-manifest-collision: binary check-skopeo $(BATS) + $(BATS) --trace --print-output-on-failure test/blackbox/detect_manifest_collision.bats + .PHONY: fuzz-all fuzz-all: fuzztime=${1} fuzz-all: @@ -325,10 +337,6 @@ fuzz-all: bash test/scripts/fuzzAll.sh ${fuzztime}; \ rm -rf pkg/storage/testdata; \ -.PHONY: test-anonymous-push-pull -test-anonymous-push-pull: binary check-skopeo $(BATS) - $(BATS) --trace --print-output-on-failure test/blackbox/anonymous_policiy.bats - $(STACKER): mkdir -p $(TOOLSDIR)/bin; \ curl -fsSL https://github.com/project-stacker/stacker/releases/latest/download/stacker -o $@; \ @@ -338,7 +346,3 @@ $(COSIGN): mkdir -p $(TOOLSDIR)/bin curl -fsSL https://github.com/sigstore/cosign/releases/download/v1.13.0/cosign-linux-amd64 -o $@; \ chmod +x $@ - -.PHONY: test-annotations -test-annotations: binary check-skopeo $(BATS) $(STACKER) $(NOTATION) $(COSIGN) - $(BATS) --trace --print-output-on-failure test/blackbox/annotations.bats diff --git a/errors/errors.go b/errors/errors.go index a713298f..8c7825f8 100644 --- a/errors/errors.go +++ b/errors/errors.go @@ -59,4 +59,5 @@ var ( ErrParsingHTTPHeader = errors.New("routes: invalid HTTP header") ErrBadRange = errors.New("storage: bad range") ErrBadLayerCount = errors.New("manifest: layers count doesn't correspond to config history") + ErrManifestConflict = errors.New("manifest: multiple manifests found") ) diff --git a/examples/README.md b/examples/README.md index 170edf74..3571bea0 100644 --- a/examples/README.md +++ b/examples/README.md @@ -175,8 +175,11 @@ Should authentication fail, to prevent automated attacks, a delayed response can ## Identity-based Authorization Allowing actions on one or more repository paths can be tied to user -identities. An additional per-repository default policy can be specified for -identities not in the whitelist. Furthermore, a global admin policy can also be +identities. Two additional per-repository policies can be specified for identities not in the whitelist: +- anonymousPolicy - applied for unathenticated users. +- defaultPolicy - applied for authenticated users. + +Furthermore, a global admin policy can also be specified which can override per-repository policies. Glob patterns can also be used as repository paths. @@ -191,7 +194,15 @@ because it will be longer. So that's why we have the option to specify an adminP Basically '**' means repositories not matched by any other per-repository policy. -create/update/delete can not be used without 'read' action, make sure read is always included in policies! +Method-based action list: +- "read" - list/pull images +- "create" - push images (needs "read") +- "update" - overwrite tags (needs "read" and "create") +- "delete" - delete images (needs "read") + +Behaviour-based action list +- "detectManifestCollision" - delete manifest by digest will throw an error if multiple manifests have the same digest (needs "read" and "delete") + ``` "accessControl": { @@ -202,8 +213,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 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 + "defaultPolicy": ["read", "create", "delete", "detectManifestCollision"], # default policy which is applied for authenticated users, other than "charlie"=> so these users can read/create/delete repositories and also can detect manifests collision. + "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-policy.json b/examples/config-policy.json index a2393177..8683ec08 100644 --- a/examples/config-policy.json +++ b/examples/config-policy.json @@ -30,7 +30,9 @@ ], "defaultPolicy": [ "read", - "create" + "create", + "delete", + "detectManifestCollision" ] }, "tmp/**": { diff --git a/pkg/api/authz.go b/pkg/api/authz.go index 4204007d..d2b22ac3 100644 --- a/pkg/api/authz.go +++ b/pkg/api/authz.go @@ -18,11 +18,13 @@ import ( ) const ( - // actions. - CREATE = "create" - READ = "read" - UPDATE = "update" - DELETE = "delete" + // method actions. + Create = "create" + Read = "read" + Update = "update" + Delete = "delete" + // behaviour actions. + DetectManifestCollision = "detectManifestCollision" ) // AccessController authorizes users to act on resources. @@ -38,19 +40,27 @@ func NewAccessController(config *config.Config) *AccessController { } } -// getReadRepos get glob patterns from config file that the user has or doesn't have READ perms. +// getGlobPatterns gets glob patterns from authz config on which has perms. // used to filter /v2/_catalog repositories based on user rights. -func (ac *AccessController) getReadGlobPatterns(username string) map[string]bool { +func (ac *AccessController) getGlobPatterns(username string, action string) map[string]bool { globPatterns := make(map[string]bool) for pattern, policyGroup := range ac.Config.Repositories { - // check default policy - if common.Contains(policyGroup.DefaultPolicy, READ) { - globPatterns[pattern] = true + if username == "" { + // check anonymous policy + if common.Contains(policyGroup.AnonymousPolicy, action) { + globPatterns[pattern] = true + } + } else { + // check default policy (authenticated user) + if common.Contains(policyGroup.DefaultPolicy, action) { + globPatterns[pattern] = true + } } + // check user based policy for _, p := range policyGroup.Policies { - if common.Contains(p.Users, username) && common.Contains(p.Actions, READ) { + if common.Contains(p.Users, username) && common.Contains(p.Actions, action) { globPatterns[pattern] = true } } @@ -102,10 +112,13 @@ func (ac *AccessController) isAdmin(username string) bool { // getContext builds ac context(allowed to read repos and if user is admin) and returns it. func (ac *AccessController) getContext(username string, request *http.Request) context.Context { - readGlobPatterns := ac.getReadGlobPatterns(username) + readGlobPatterns := ac.getGlobPatterns(username, Read) + dmcGlobPatterns := ac.getGlobPatterns(username, DetectManifestCollision) + acCtx := localCtx.AccessControlContext{ - GlobPatterns: readGlobPatterns, - Username: username, + ReadGlobPatterns: readGlobPatterns, + DmcGlobPatterns: dmcGlobPatterns, + Username: username, } if ac.isAdmin(username) { @@ -149,25 +162,6 @@ func isPermitted(username, action string, policyGroup config.PolicyGroup) bool { return result } -// returns either a user has or not rights on 'repository'. -func matchesRepo(globPatterns map[string]bool, repository string) bool { - var longestMatchedPattern string - - // because of the longest path matching rule, we need to check all patterns from config - for pattern := range globPatterns { - matched, err := glob.Match(pattern, repository) - if err == nil { - if matched && len(pattern) > len(longestMatchedPattern) { - longestMatchedPattern = pattern - } - } - } - - allowed := globPatterns[longestMatchedPattern] - - return allowed -} - func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) { @@ -231,25 +225,25 @@ func AuthzHandler(ctlr *Controller) mux.MiddlewareFunc { var action string if request.Method == http.MethodGet || request.Method == http.MethodHead { - action = READ + action = Read } if request.Method == http.MethodPut || request.Method == http.MethodPatch || request.Method == http.MethodPost { // assume user wants to create - action = CREATE + action = Create // if we get a reference (tag) if ok { is := ctlr.StoreController.GetImageStore(resource) tags, err := is.GetImageTags(resource) // if repo exists and request's tag exists then action is UPDATE if err == nil && common.Contains(tags, reference) && reference != "latest" { - action = UPDATE + action = Update } } } if request.Method == http.MethodDelete { - action = DELETE + action = Delete } can := acCtrlr.can(identity, action, resource) diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index 7c4c7ae5..7dc1705f 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -5789,6 +5789,140 @@ func TestManifestImageIndex(t *testing.T) { }) } +func TestManifestCollision(t *testing.T) { + Convey("Make a new controller", t, func() { + port := test.GetFreePort() + baseURL := test.GetBaseURL(port) + conf := config.New() + conf.HTTP.Port = port + + ctlr := api.NewController(conf) + dir := t.TempDir() + ctlr.Config.Storage.RootDirectory = dir + + conf.AccessControl = &config.AccessControlConfig{ + Repositories: config.Repositories{ + AuthorizationAllRepos: config.PolicyGroup{ + AnonymousPolicy: []string{api.Read, api.Create, api.Delete, api.DetectManifestCollision}, + }, + }, + } + + go startServer(ctlr) + defer stopServer(ctlr) + test.WaitTillServerReady(baseURL) + + // create a blob/layer + resp, err := resty.R().Post(baseURL + "/v2/index/blobs/uploads/") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + loc := test.Location(baseURL, resp) + So(loc, ShouldNotBeEmpty) + + // since we are not specifying any prefix i.e provided in config while starting server, + // so it should store index1 to global root dir + _, err = os.Stat(path.Join(dir, "index")) + So(err, ShouldBeNil) + + resp, err = resty.R().Get(loc) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNoContent) + content := []byte("this is a blob1") + digest := godigest.FromBytes(content) + So(digest, ShouldNotBeNil) + // monolithic blob upload: success + resp, err = resty.R().SetQueryParam("digest", digest.String()). + SetHeader("Content-Type", "application/octet-stream").SetBody(content).Put(loc) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + blobLoc := resp.Header().Get("Location") + So(blobLoc, ShouldNotBeEmpty) + So(resp.Header().Get("Content-Length"), ShouldEqual, "0") + So(resp.Header().Get(constants.DistContentDigestKey), ShouldNotBeEmpty) + + // check a non-existent manifest + resp, err = resty.R().SetHeader("Content-Type", ispec.MediaTypeImageManifest). + SetBody(content).Head(baseURL + "/v2/unknown/manifests/test:1.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + + // upload image config blob + resp, err = resty.R().Post(baseURL + "/v2/index/blobs/uploads/") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + loc = test.Location(baseURL, resp) + cblob, cdigest := test.GetRandomImageConfig() + + resp, err = resty.R(). + SetContentLength(true). + SetHeader("Content-Length", fmt.Sprintf("%d", len(cblob))). + SetHeader("Content-Type", "application/octet-stream"). + SetQueryParam("digest", cdigest.String()). + SetBody(cblob). + Put(loc) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + + // create a manifest + manifest := ispec.Manifest{ + Config: ispec.Descriptor{ + MediaType: ispec.MediaTypeImageConfig, + Digest: cdigest, + Size: int64(len(cblob)), + }, + Layers: []ispec.Descriptor{ + { + MediaType: ispec.MediaTypeImageLayer, + Digest: digest, + Size: int64(len(content)), + }, + }, + } + manifest.SchemaVersion = 2 + content, err = json.Marshal(manifest) + So(err, ShouldBeNil) + digest = godigest.FromBytes(content) + So(digest, ShouldNotBeNil) + resp, err = resty.R().SetHeader("Content-Type", ispec.MediaTypeImageManifest). + SetBody(content).Put(baseURL + "/v2/index/manifests/test:1.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + digestHdr := resp.Header().Get(constants.DistContentDigestKey) + So(digestHdr, ShouldNotBeEmpty) + So(digestHdr, ShouldEqual, digest.String()) + + resp, err = resty.R().SetHeader("Content-Type", ispec.MediaTypeImageManifest). + SetBody(content).Put(baseURL + "/v2/index/manifests/test:2.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusCreated) + digestHdr = resp.Header().Get(constants.DistContentDigestKey) + So(digestHdr, ShouldNotBeEmpty) + So(digestHdr, ShouldEqual, digest.String()) + + // Deletion should fail if using digest + resp, err = resty.R().Delete(baseURL + "/v2/index/manifests/" + digest.String()) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusConflict) + + // remove detectManifestCollision action from ** (all repos) + repoPolicy := conf.AccessControl.Repositories[AuthorizationAllRepos] + repoPolicy.AnonymousPolicy = []string{"read", "delete"} + conf.AccessControl.Repositories[AuthorizationAllRepos] = repoPolicy + + resp, err = resty.R().Delete(baseURL + "/v2/index/manifests/" + digest.String()) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusAccepted) + + resp, err = resty.R().Get(baseURL + "/v2/index/manifests/test:1.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + + resp, err = resty.R().Get(baseURL + "/v2/index/manifests/test:2.0") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) + }) +} + func TestPullRange(t *testing.T) { Convey("Make a new controller", t, func() { port := test.GetFreePort() diff --git a/pkg/api/routes.go b/pkg/api/routes.go index dac26cca..f8f492ad 100644 --- a/pkg/api/routes.go +++ b/pkg/api/routes.go @@ -582,7 +582,7 @@ func (rh *RouteHandler) UpdateManifest(response http.ResponseWriter, request *ht // could be syscall.EMFILE (Err:0x18 too many opened files), etc rh.c.Log.Error().Err(err).Msg("unexpected error: performing cleanup") - if err = imgStore.DeleteImageManifest(name, reference); err != nil { + if err = imgStore.DeleteImageManifest(name, reference, false); err != nil { // deletion of image manifest is important, but not critical for image repo consistancy // in the worst scenario a partial manifest file written to disk will not affect the repo because // the new manifest was not added to "index.json" file (it is possible that GC will take care of it) @@ -628,7 +628,20 @@ func (rh *RouteHandler) DeleteManifest(response http.ResponseWriter, request *ht return } - err := imgStore.DeleteImageManifest(name, reference) + // authz request context (set in authz middleware) + acCtx, err := localCtx.GetAccessControlContext(request.Context()) + if err != nil { + response.WriteHeader(http.StatusInternalServerError) + + return + } + + var detectCollision bool + if acCtx != nil { + detectCollision = acCtx.CanDetectManifestCollision(name) + } + + err = imgStore.DeleteImageManifest(name, reference, detectCollision) if err != nil { if errors.Is(err, zerr.ErrRepoNotFound) { //nolint:gocritic // errorslint conflicts with gocritic:IfElseChain WriteJSON(response, http.StatusBadRequest, @@ -636,6 +649,9 @@ func (rh *RouteHandler) DeleteManifest(response http.ResponseWriter, request *ht } else if errors.Is(err, zerr.ErrManifestNotFound) { WriteJSON(response, http.StatusNotFound, NewErrorList(NewError(MANIFEST_UNKNOWN, map[string]string{"reference": reference}))) + } else if errors.Is(err, zerr.ErrManifestConflict) { + WriteJSON(response, http.StatusConflict, + NewErrorList(NewError(MANIFEST_INVALID, map[string]string{"reference": reference}))) } else if errors.Is(err, zerr.ErrBadManifest) { WriteJSON(response, http.StatusBadRequest, NewErrorList(NewError(UNSUPPORTED, map[string]string{"reference": reference}))) @@ -1454,19 +1470,18 @@ func (rh *RouteHandler) ListRepositories(response http.ResponseWriter, request * } var repos []string - authzCtxKey := localCtx.GetContextKey() - // get passed context from authzHandler and filter out repos based on permissions - if authCtx := request.Context().Value(authzCtxKey); authCtx != nil { - acCtx, ok := authCtx.(localCtx.AccessControlContext) - if !ok { - response.WriteHeader(http.StatusInternalServerError) + // authz context + acCtx, err := localCtx.GetAccessControlContext(request.Context()) + if err != nil { + response.WriteHeader(http.StatusInternalServerError) - return - } + return + } + if acCtx != nil { for _, r := range combineRepoList { - if acCtx.IsAdmin || matchesRepo(acCtx.GlobPatterns, r) { + if acCtx.IsAdmin || acCtx.CanReadRepo(r) { repos = append(repos, r) } } diff --git a/pkg/api/routes_test.go b/pkg/api/routes_test.go index a43e3e4a..1494031d 100644 --- a/pkg/api/routes_test.go +++ b/pkg/api/routes_test.go @@ -21,6 +21,7 @@ import ( "zotregistry.io/zot/pkg/api" "zotregistry.io/zot/pkg/api/config" "zotregistry.io/zot/pkg/api/constants" + localCtx "zotregistry.io/zot/pkg/requestcontext" "zotregistry.io/zot/pkg/storage" "zotregistry.io/zot/pkg/test" "zotregistry.io/zot/pkg/test/mocks" @@ -54,6 +55,52 @@ func TestRoutes(t *testing.T) { // NOTE: the url or method itself doesn't matter below since we are calling the handlers directly, // so path routing is bypassed + Convey("List repositories authz error", func() { + var invalid struct{} + + ctx := context.TODO() + key := localCtx.GetContextKey() + ctx = context.WithValue(ctx, key, invalid) + + request, _ := http.NewRequestWithContext(ctx, http.MethodGet, baseURL, nil) + request = mux.SetURLVars(request, map[string]string{ + "name": "test", + "reference": "b8b1231908844a55c251211c7a67ae3c809fb86a081a8eeb4a715e6d7d65625c", + }) + response := httptest.NewRecorder() + + rthdlr.ListRepositories(response, request) + + resp := response.Result() + + defer resp.Body.Close() + So(resp, ShouldNotBeNil) + So(resp.StatusCode, ShouldEqual, http.StatusInternalServerError) + }) + + Convey("Delete manifest authz error", func() { + var invalid struct{} + + ctx := context.TODO() + key := localCtx.GetContextKey() + ctx = context.WithValue(ctx, key, invalid) + + request, _ := http.NewRequestWithContext(ctx, http.MethodGet, baseURL, nil) + request = mux.SetURLVars(request, map[string]string{ + "name": "test", + "reference": "b8b1231908844a55c251211c7a67ae3c809fb86a081a8eeb4a715e6d7d65625c", + }) + response := httptest.NewRecorder() + + rthdlr.DeleteManifest(response, request) + + resp := response.Result() + + defer resp.Body.Close() + So(resp, ShouldNotBeNil) + So(resp.StatusCode, ShouldEqual, http.StatusInternalServerError) + }) + Convey("Get manifest", func() { // overwrite controller storage ctlr.StoreController.DefaultStore = &mocks.MockedImageStore{ @@ -184,7 +231,7 @@ func TestRoutes(t *testing.T) { "reference": "reference", }, &mocks.MockedImageStore{ - DeleteImageManifestFn: func(repo, reference string) error { + DeleteImageManifestFn: func(repo, reference string, detectCollision bool) error { return zerr.ErrRepoNotFound }, }, @@ -199,7 +246,7 @@ func TestRoutes(t *testing.T) { "reference": "reference", }, &mocks.MockedImageStore{ - DeleteImageManifestFn: func(repo, reference string) error { + DeleteImageManifestFn: func(repo, reference string, detectCollision bool) error { return zerr.ErrManifestNotFound }, }, @@ -214,7 +261,7 @@ func TestRoutes(t *testing.T) { "reference": "reference", }, &mocks.MockedImageStore{ - DeleteImageManifestFn: func(repo, reference string) error { + DeleteImageManifestFn: func(repo, reference string, detectCollision bool) error { return ErrUnexpectedError }, }, @@ -229,7 +276,7 @@ func TestRoutes(t *testing.T) { "reference": "reference", }, &mocks.MockedImageStore{ - DeleteImageManifestFn: func(repo, reference string) error { + DeleteImageManifestFn: func(repo, reference string, detectCollision bool) error { return zerr.ErrBadManifest }, }, diff --git a/pkg/extensions/search/resolver.go b/pkg/extensions/search/resolver.go index 69e013cb..18f28ec1 100644 --- a/pkg/extensions/search/resolver.go +++ b/pkg/extensions/search/resolver.go @@ -12,7 +12,6 @@ import ( "strings" "github.com/99designs/gqlgen/graphql" - glob "github.com/bmatcuk/doublestar/v4" godigest "github.com/opencontainers/go-digest" ispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/vektah/gqlparser/v2/gqlerror" @@ -734,40 +733,21 @@ func BuildImageInfo(repo string, tag string, manifestDigest godigest.Digest, return imageInfo } -// returns either a user has or not rights on 'repository'. -func matchesRepo(globPatterns map[string]bool, repository string) bool { - var longestMatchedPattern string - - // because of the longest path matching rule, we need to check all patterns from config - for pattern := range globPatterns { - matched, err := glob.Match(pattern, repository) - if err == nil { - if matched && len(pattern) > len(longestMatchedPattern) { - longestMatchedPattern = pattern - } - } - } - - allowed := globPatterns[longestMatchedPattern] - - return allowed -} - // get passed context from authzHandler and filter out repos based on permissions. func userAvailableRepos(ctx context.Context, repoList []string) ([]string, error) { var availableRepos []string - authzCtxKey := localCtx.GetContextKey() - if authCtx := ctx.Value(authzCtxKey); authCtx != nil { - acCtx, ok := authCtx.(localCtx.AccessControlContext) - if !ok { - err := errors.ErrBadType + // authz request context (set in authz middleware) + acCtx, err := localCtx.GetAccessControlContext(ctx) + if err != nil { + err := errors.ErrBadType - return []string{}, err - } + return []string{}, err + } + if acCtx != nil { for _, r := range repoList { - if acCtx.IsAdmin || matchesRepo(acCtx.GlobPatterns, r) { + if acCtx.IsAdmin || acCtx.CanReadRepo(r) { availableRepos = append(availableRepos, r) } } diff --git a/pkg/extensions/search/resolver_test.go b/pkg/extensions/search/resolver_test.go index b8c4cb80..09698ade 100644 --- a/pkg/extensions/search/resolver_test.go +++ b/pkg/extensions/search/resolver_test.go @@ -337,8 +337,8 @@ func TestExtractImageDetails(t *testing.T) { authzCtxKey := localCtx.GetContextKey() ctx = context.WithValue(ctx, authzCtxKey, localCtx.AccessControlContext{ - GlobPatterns: map[string]bool{"*": true, "**": true}, - Username: "jane_doe", + ReadGlobPatterns: map[string]bool{"*": true, "**": true}, + Username: "jane_doe", }) configBlobContent, _ := json.MarshalIndent(&config, "", "\t") configDigest := godigest.FromBytes(configBlobContent) @@ -429,8 +429,8 @@ func TestExtractImageDetails(t *testing.T) { Convey("extractImageDetails without proper authz", func() { ctx = context.WithValue(ctx, authzCtxKey, localCtx.AccessControlContext{ - GlobPatterns: map[string]bool{}, - Username: "jane_doe", + ReadGlobPatterns: map[string]bool{}, + Username: "jane_doe", }) mockOlum := mocks.OciLayoutUtilsMock{ GetImageConfigInfoFn: func(repo string, digest godigest.Digest) ( diff --git a/pkg/extensions/sync/sync_test.go b/pkg/extensions/sync/sync_test.go index de527b7a..aa35fd2e 100644 --- a/pkg/extensions/sync/sync_test.go +++ b/pkg/extensions/sync/sync_test.go @@ -1408,7 +1408,7 @@ func TestBasicAuth(t *testing.T) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, 200) - err = dctlr.StoreController.DefaultStore.DeleteImageManifest(testImage, testImageTag) + err = dctlr.StoreController.DefaultStore.DeleteImageManifest(testImage, testImageTag, false) So(err, ShouldBeNil) resp, err = destClient.R().Get(destBaseURL + "/v2/" + testImage + "/manifests/" + "1.1.1") diff --git a/pkg/requestcontext/context.go b/pkg/requestcontext/context.go index fa199ea5..98248a84 100644 --- a/pkg/requestcontext/context.go +++ b/pkg/requestcontext/context.go @@ -1,5 +1,12 @@ package requestcontext +import ( + "context" + + glob "github.com/bmatcuk/doublestar/v4" //nolint:gci + "zotregistry.io/zot/errors" +) + type Key int // request-local context key. @@ -12,7 +19,53 @@ func GetContextKey() *Key { // AccessControlContext context passed down to http.Handlers. type AccessControlContext struct { - GlobPatterns map[string]bool - IsAdmin bool - Username string + // read method action + ReadGlobPatterns map[string]bool + // detectManifestCollision behaviour action + DmcGlobPatterns map[string]bool + IsAdmin bool + Username string +} + +func GetAccessControlContext(ctx context.Context) (*AccessControlContext, error) { + authzCtxKey := GetContextKey() + if authCtx := ctx.Value(authzCtxKey); authCtx != nil { + acCtx, ok := authCtx.(AccessControlContext) + if !ok { + return nil, errors.ErrBadType + } + + return &acCtx, nil + } + + return nil, nil //nolint: nilnil +} + +// returns either a user has or not rights on 'repository'. +func (acCtx *AccessControlContext) matchesRepo(globPatterns map[string]bool, repository string) bool { + var longestMatchedPattern string + + // because of the longest path matching rule, we need to check all patterns from config + for pattern := range globPatterns { + matched, err := glob.Match(pattern, repository) + if err == nil { + if matched && len(pattern) > len(longestMatchedPattern) { + longestMatchedPattern = pattern + } + } + } + + allowed := globPatterns[longestMatchedPattern] + + return allowed +} + +// returns either a user has or not read rights on 'repository'. +func (acCtx *AccessControlContext) CanReadRepo(repository string) bool { + return acCtx.matchesRepo(acCtx.ReadGlobPatterns, repository) +} + +// returns either a user has or not detectManifestCollision rights on 'repository'. +func (acCtx *AccessControlContext) CanDetectManifestCollision(repository string) bool { + return acCtx.matchesRepo(acCtx.DmcGlobPatterns, repository) } diff --git a/pkg/storage/common.go b/pkg/storage/common.go index 122f9ea1..df654596 100644 --- a/pkg/storage/common.go +++ b/pkg/storage/common.go @@ -178,7 +178,7 @@ func GetAndValidateRequestDigest(body []byte, digestStr string, log zerolog.Logg } /* - CheckIfIndexNeedsUpdate verifies if an index needs to be updated given a new manifest descriptor. +CheckIfIndexNeedsUpdate verifies if an index needs to be updated given a new manifest descriptor. Returns whether or not index needs update, in the latter case it will also return the previous digest. */ @@ -272,11 +272,14 @@ func GetIndex(imgStore ImageStore, repo string, log zerolog.Logger) (ispec.Index return index, nil } -func RemoveManifestDescByReference(index *ispec.Index, reference string) (ispec.Descriptor, bool) { +func RemoveManifestDescByReference(index *ispec.Index, reference string, detectCollisions bool, +) (ispec.Descriptor, bool, error) { var removedManifest ispec.Descriptor var found bool + foundCount := 0 + var outIndex ispec.Index for _, manifest := range index.Manifests { @@ -284,11 +287,13 @@ func RemoveManifestDescByReference(index *ispec.Index, reference string) (ispec. if ok && tag == reference { removedManifest = manifest found = true + foundCount++ continue } else if reference == manifest.Digest.String() { removedManifest = manifest found = true + foundCount++ continue } @@ -296,14 +301,17 @@ func RemoveManifestDescByReference(index *ispec.Index, reference string) (ispec. outIndex.Manifests = append(outIndex.Manifests, manifest) } + if foundCount > 1 && detectCollisions { + return ispec.Descriptor{}, false, zerr.ErrManifestConflict + } + index.Manifests = outIndex.Manifests - return removedManifest, found + return removedManifest, found, nil } /* - additionally, unmarshal an image index and for all manifests in that - +Unmarshal an image index and for all manifests in that index, ensure that they do not have a name or they are not in other manifest indexes else GC can never clean them. */ @@ -333,13 +341,12 @@ func UpdateIndexWithPrunedImageManifests(imgStore ImageStore, index *ispec.Index } /* -* -before an image index manifest is pushed to a repo, its constituent manifests +Before an image index manifest is pushed to a repo, its constituent manifests are pushed first, so when updating/removing this image index manifest, we also need to determine if there are other image index manifests which refer to the same constitutent manifests so that they can be garbage-collected correctly -pruneImageManifestsFromIndex is a helper routine to achieve this. +PruneImageManifestsFromIndex is a helper routine to achieve this. */ func PruneImageManifestsFromIndex(imgStore ImageStore, repo string, digest godigest.Digest, //nolint:gocyclo outIndex ispec.Index, otherImgIndexes []ispec.Descriptor, log zerolog.Logger, diff --git a/pkg/storage/local/local.go b/pkg/storage/local/local.go index 6a0750af..97e25d42 100644 --- a/pkg/storage/local/local.go +++ b/pkg/storage/local/local.go @@ -549,7 +549,7 @@ func (is *ImageStoreLocal) PutImageManifest(repo, reference, mediaType string, / } // DeleteImageManifest deletes the image manifest from the repository. -func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string) error { +func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string, detectCollision bool) error { var lockLatency time.Time dir := path.Join(is.rootDir, repo) @@ -562,7 +562,11 @@ func (is *ImageStoreLocal) DeleteImageManifest(repo, reference string) error { return err } - manifestDesc, found := storage.RemoveManifestDescByReference(&index, reference) + manifestDesc, found, err := storage.RemoveManifestDescByReference(&index, reference, detectCollision) + if err != nil { + return err + } + if !found { return zerr.ErrManifestNotFound } diff --git a/pkg/storage/local/local_test.go b/pkg/storage/local/local_test.go index f30f88ed..c47b9aea 100644 --- a/pkg/storage/local/local_test.go +++ b/pkg/storage/local/local_test.go @@ -164,7 +164,7 @@ func TestStorageFSAPIs(t *testing.T) { panic(err) } - err = imgStore.DeleteImageManifest(repoName, digest.String()) + err = imgStore.DeleteImageManifest(repoName, digest.String(), false) So(err, ShouldNotBeNil) err = os.RemoveAll(path.Join(imgStore.RootDir(), repoName)) @@ -440,7 +440,7 @@ func FuzzTestPutDeleteImageManifest(f *testing.F) { t.Errorf("the error that occurred is %v \n", err) } - err = imgStore.DeleteImageManifest(repoName, mdigest.String()) + err = imgStore.DeleteImageManifest(repoName, mdigest.String(), false) if err != nil { if isKnownErr(err) { return @@ -470,7 +470,7 @@ func FuzzTestDeleteImageManifest(f *testing.F) { if err != nil { return } - err = imgStore.DeleteImageManifest(string(data), digest.String()) + err = imgStore.DeleteImageManifest(string(data), digest.String(), false) if err != nil { if errors.Is(err, zerr.ErrRepoNotFound) || isKnownErr(err) { return @@ -1822,7 +1822,7 @@ func TestGarbageCollect(t *testing.T) { So(err, ShouldBeNil) So(hasBlob, ShouldEqual, true) - err = imgStore.DeleteImageManifest(repoName, digest.String()) + err = imgStore.DeleteImageManifest(repoName, digest.String(), false) So(err, ShouldBeNil) hasBlob, _, err = imgStore.CheckBlob(repoName, bdigest) @@ -1922,7 +1922,7 @@ func TestGarbageCollect(t *testing.T) { // sleep so orphan blob can be GC'ed time.Sleep(5 * time.Second) - err = imgStore.DeleteImageManifest(repoName, digest.String()) + err = imgStore.DeleteImageManifest(repoName, digest.String(), false) So(err, ShouldBeNil) hasBlob, _, err = imgStore.CheckBlob(repoName, bdigest) diff --git a/pkg/storage/s3/s3.go b/pkg/storage/s3/s3.go index 4b52cca6..8fe6c0c7 100644 --- a/pkg/storage/s3/s3.go +++ b/pkg/storage/s3/s3.go @@ -442,7 +442,7 @@ func (is *ObjectStorage) PutImageManifest(repo, reference, mediaType string, //n } // DeleteImageManifest deletes the image manifest from the repository. -func (is *ObjectStorage) DeleteImageManifest(repo, reference string) error { +func (is *ObjectStorage) DeleteImageManifest(repo, reference string, detectCollisions bool) error { var lockLatency time.Time dir := path.Join(is.rootDir, repo) @@ -455,7 +455,11 @@ func (is *ObjectStorage) DeleteImageManifest(repo, reference string) error { return err } - manifestDesc, found := storage.RemoveManifestDescByReference(&index, reference) + manifestDesc, found, err := storage.RemoveManifestDescByReference(&index, reference, detectCollisions) + if err != nil { + return err + } + if !found { return zerr.ErrManifestNotFound } diff --git a/pkg/storage/s3/s3_test.go b/pkg/storage/s3/s3_test.go index 3d6ab424..aef2a83e 100644 --- a/pkg/storage/s3/s3_test.go +++ b/pkg/storage/s3/s3_test.go @@ -783,7 +783,7 @@ func TestNegativeCasesObjectsStorage(t *testing.T) { err = imgStore.DeleteBlobUpload(testImage, upload) So(err, ShouldNotBeNil) - err = imgStore.DeleteImageManifest(testImage, "1.0") + err = imgStore.DeleteImageManifest(testImage, "1.0", false) So(err, ShouldNotBeNil) _, err = imgStore.PutImageManifest(testImage, "1.0", "application/json", []byte{}) @@ -887,13 +887,13 @@ func TestNegativeCasesObjectsStorage(t *testing.T) { return []byte{}, errS3 }, }) - err := imgStore.DeleteImageManifest(testImage, "1.0") + err := imgStore.DeleteImageManifest(testImage, "1.0", false) So(err, ShouldNotBeNil) }) Convey("Test DeleteImageManifest2", func(c C) { imgStore = createMockStorage(testDir, tdir, false, &StorageDriverMock{}) - err := imgStore.DeleteImageManifest(testImage, "1.0") + err := imgStore.DeleteImageManifest(testImage, "1.0", false) So(err, ShouldNotBeNil) }) @@ -1891,12 +1891,12 @@ func TestS3ManifestImageIndex(t *testing.T) { Convey("Deleting an image index", func() { // delete manifest by tag should pass - err := imgStore.DeleteImageManifest("index", "test:index3") + err := imgStore.DeleteImageManifest("index", "test:index3", false) So(err, ShouldNotBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index3") So(err, ShouldNotBeNil) - err = imgStore.DeleteImageManifest("index", "test:index1") + err = imgStore.DeleteImageManifest("index", "test:index1", false) So(err, ShouldBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldNotBeNil) @@ -1907,12 +1907,12 @@ func TestS3ManifestImageIndex(t *testing.T) { Convey("Deleting an image index by digest", func() { // delete manifest by tag should pass - err := imgStore.DeleteImageManifest("index", "test:index3") + err := imgStore.DeleteImageManifest("index", "test:index3", false) So(err, ShouldNotBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index3") So(err, ShouldNotBeNil) - err = imgStore.DeleteImageManifest("index", index1dgst.String()) + err = imgStore.DeleteImageManifest("index", index1dgst.String(), false) So(err, ShouldBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldNotBeNil) @@ -1983,7 +1983,7 @@ func TestS3ManifestImageIndex(t *testing.T) { _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldBeNil) - err = imgStore.DeleteImageManifest("index", "test:index1") + err = imgStore.DeleteImageManifest("index", "test:index1", false) So(err, ShouldBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldNotBeNil) @@ -1994,7 +1994,7 @@ func TestS3ManifestImageIndex(t *testing.T) { cleanupStorage(storeDriver, path.Join(testDir, "index", "blobs", index1dgst.Algorithm().String(), index1dgst.Encoded())) - err = imgStore.DeleteImageManifest("index", index1dgst.String()) + err = imgStore.DeleteImageManifest("index", index1dgst.String(), false) So(err, ShouldNotBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldNotBeNil) @@ -2009,7 +2009,7 @@ func TestS3ManifestImageIndex(t *testing.T) { _, err = wrtr.Write([]byte("deadbeef")) So(err, ShouldBeNil) wrtr.Close() - err = imgStore.DeleteImageManifest("index", index1dgst.String()) + err = imgStore.DeleteImageManifest("index", index1dgst.String(), false) So(err, ShouldBeNil) _, _, _, err = imgStore.GetImageManifest("index", "test:index1") So(err, ShouldNotBeNil) diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index d2d1907a..6402dc67 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -30,7 +30,7 @@ type ImageStore interface { //nolint:interfacebloat GetImageTags(repo string) ([]string, error) GetImageManifest(repo, reference string) ([]byte, godigest.Digest, string, error) PutImageManifest(repo, reference, mediaType string, body []byte) (godigest.Digest, error) - DeleteImageManifest(repo, reference string) error + DeleteImageManifest(repo, reference string, detectCollision bool) error BlobUploadPath(repo, uuid string) string NewBlobUpload(repo string) (string, error) GetBlobUpload(repo, uuid string) (int64, error) diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index 01c2e495..ab6015c8 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -356,7 +356,7 @@ func TestStorageAPIs(t *testing.T) { _, _, _, err = imgStore.GetImageManifest("test", "3.0") So(err, ShouldBeNil) - err = imgStore.DeleteImageManifest("test", "1.0") + err = imgStore.DeleteImageManifest("test", "1.0", false) So(err, ShouldBeNil) tags, err = imgStore.GetImageTags("test") @@ -368,8 +368,12 @@ func TestStorageAPIs(t *testing.T) { So(err, ShouldBeNil) So(hasBlob, ShouldEqual, true) + // with detectManifestCollision should get error + err = imgStore.DeleteImageManifest("test", digest.String(), true) + So(err, ShouldNotBeNil) + // If we pass reference all manifest with input reference should be deleted. - err = imgStore.DeleteImageManifest("test", digest.String()) + err = imgStore.DeleteImageManifest("test", digest.String(), false) So(err, ShouldBeNil) tags, err = imgStore.GetImageTags("test") @@ -541,13 +545,13 @@ func TestStorageAPIs(t *testing.T) { So(err, ShouldBeNil) So(len(index.Manifests), ShouldEqual, 1) - err = imgStore.DeleteImageManifest("test", "1.0") + err = imgStore.DeleteImageManifest("test", "1.0", false) So(err, ShouldNotBeNil) - err = imgStore.DeleteImageManifest("inexistent", "1.0") + err = imgStore.DeleteImageManifest("inexistent", "1.0", false) So(err, ShouldNotBeNil) - err = imgStore.DeleteImageManifest("test", digest.String()) + err = imgStore.DeleteImageManifest("test", digest.String(), false) So(err, ShouldBeNil) _, _, _, err = imgStore.GetImageManifest("test", digest.String()) diff --git a/pkg/test/mocks/image_store_mock.go b/pkg/test/mocks/image_store_mock.go index dfd504bc..a1b79198 100644 --- a/pkg/test/mocks/image_store_mock.go +++ b/pkg/test/mocks/image_store_mock.go @@ -21,7 +21,7 @@ type MockedImageStore struct { GetImageTagsFn func(repo string) ([]string, error) GetImageManifestFn func(repo string, reference string) ([]byte, godigest.Digest, string, error) PutImageManifestFn func(repo string, reference string, mediaType string, body []byte) (godigest.Digest, error) - DeleteImageManifestFn func(repo string, reference string) error + DeleteImageManifestFn func(repo string, reference string, detectCollision bool) error BlobUploadPathFn func(repo string, uuid string) string NewBlobUploadFn func(repo string) (string, error) GetBlobUploadFn func(repo string, uuid string) (int64, error) @@ -136,9 +136,9 @@ func (is MockedImageStore) GetImageTags(name string) ([]string, error) { return []string{}, nil } -func (is MockedImageStore) DeleteImageManifest(name string, reference string) error { +func (is MockedImageStore) DeleteImageManifest(name string, reference string, detectCollision bool) error { if is.DeleteImageManifestFn != nil { - return is.DeleteImageManifestFn(name, reference) + return is.DeleteImageManifestFn(name, reference, detectCollision) } return nil diff --git a/test/blackbox/anonymous_policiy.bats b/test/blackbox/anonymous_policy.bats similarity index 96% rename from test/blackbox/anonymous_policiy.bats rename to test/blackbox/anonymous_policy.bats index ccbfb4c8..1b79c3c2 100644 --- a/test/blackbox/anonymous_policiy.bats +++ b/test/blackbox/anonymous_policy.bats @@ -53,7 +53,6 @@ function setup_file() { } } EOF - git -C ${BATS_FILE_TMPDIR} clone https://github.com/project-zot/helm-charts.git setup_zot_file_level ${zot_config_file} wait_zot_reachable "http://127.0.0.1:8080/v2/_catalog" } @@ -66,7 +65,6 @@ function teardown_file() { rm -rf ${oci_data_dir} } - @test "push image user policy" { run skopeo --insecure-policy copy --dest-creds test:test --dest-tls-verify=false \ oci:${TEST_DATA_DIR}/golang:1.18 \ @@ -74,7 +72,6 @@ function teardown_file() { [ "$status" -eq 0 ] } - @test "pull image anonymous policy" { local oci_data_dir=${BATS_FILE_TMPDIR}/oci run skopeo --insecure-policy copy --src-tls-verify=false \ diff --git a/test/blackbox/detect_manifest_collision.bats b/test/blackbox/detect_manifest_collision.bats new file mode 100644 index 00000000..04d1bf4f --- /dev/null +++ b/test/blackbox/detect_manifest_collision.bats @@ -0,0 +1,109 @@ +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.19 oci:${TEST_DATA_DIR}/golang:1.19 + # 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}<