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}<