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 <rchincha@cisco.com> Signed-off-by: Petu Eusebiu <peusebiu@cisco.com> Co-authored-by: Ramkumar Chinchani <rchincha@cisco.com>
2026-06-16 04:17:55 +08:00 · 2022-11-18 19:35:28 +02:00
parent 4e13619dc8
commit 168d21da1e
22 changed files with 507 additions and 141 deletions
@@ -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 <username> has <action> 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)
@@ -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()
@@ -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)
 			}
 		}
@@ -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
 					},
 				},