diff --git a/modules/distribution/internal/holder/holder_test.go b/modules/distribution/internal/holder/holder_test.go index f3f11868..36ae96ae 100644 --- a/modules/distribution/internal/holder/holder_test.go +++ b/modules/distribution/internal/holder/holder_test.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -117,13 +118,14 @@ func (c *fakeCatalog) Remove(name, version string) error { } return nil } + +// RemoveTag answers as OCI.RemoveTag over registry 2.8.3 does (fakeRegistry's DELETE by tag): the store cannot +// remove a tag alone, and nothing goes. tagErr is any other failure of the store. func (c *fakeCatalog) RemoveTag(name, version string) error { if c.tagErr != nil { return c.tagErr } - c.removed = append(c.removed, name+":"+version+" (tag)") - delete(c.tags, name+":"+version) - return nil + return fmt.Errorf("%w: DELETE of %s:%s by its tag answered 400 DIGEST_INVALID", ErrNoTagAlone, name, version) } func (c *fakeCatalog) RemoveDigest(name, digest string) error { c.removed = append(c.removed, name+"@"+digest) @@ -427,22 +429,42 @@ func TestTheHolderOfTheTwoImageSeats(t *testing.T) { if err := b.v.Remove(ArtifactSeat, "novox.mesh-delivery-builder", RemoveRequest{Name: "m/p", Version: "0.1.5-feat-x.1", Reason: "branch merged"}); err != nil || b.cat.tags["m/p:0.1.5-feat-x.1"] != "" { t.Errorf("the builder's removal of a tagged version: %v", err) } - // Security review F1: a reproducible build gives a branch's tagged version and the trunk's numbered one one - // digest; the tagged one's removal takes its tag alone, and the numbered one stays pullable. + // Security review F1 on registry 2.8.3: a reproducible build gives a branch's tagged version and the trunk's + // numbered one one digest; the store cannot remove the tag alone, so nothing goes, the numbered one stays + // pullable, and the removal is said removed with that reason: no refusal, so the builder raises no retry. b.cat.tags["m/p:0.1.6-feat-y.1"], b.cat.tags["m/p:0.1.6"] = part.Digest, part.Digest if err := b.v.Remove(ArtifactSeat, "novox.mesh-delivery-builder", RemoveRequest{Name: "m/p", Version: "0.1.6-feat-y.1", Reason: "branch merged"}); err != nil { - t.Fatalf("the removal of a tagged version sharing its digest: %v", err) + t.Fatalf("the removal of a tagged version sharing its digest: %v; want no error", err) } - if b.cat.tags["m/p:0.1.6-feat-y.1"] != "" || b.cat.tags["m/p:0.1.6"] != part.Digest { - t.Errorf("tags after the tagged version's removal: %v; want the tag gone and 0.1.6 still at %s", b.cat.tags, part.Digest) + if b.cat.tags["m/p:0.1.6-feat-y.1"] != part.Digest || b.cat.tags["m/p:0.1.6"] != part.Digest { + t.Errorf("tags after the tagged version's removal: %v; want both kept at %s", b.cat.tags, part.Digest) } - // A store that cannot delete a tag alone removes nothing, and the removal is refused naming the shared digest. + if last := b.said[len(b.said)-1]; last.event != EventRemoved || last.body.Reason != "branch merged; the store cannot remove a tag alone, so the tag 0.1.6-feat-y.1 is kept and its bytes stay under 0.1.6" { + t.Errorf("said %s %q; want removed, saying the tag is kept under 0.1.6", last.event, last.body.Reason) + } + // Any other failure of the store removes nothing and is refused naming the shared digest. b.cat.tags["m/p:0.1.7-feat-z.1"], b.cat.tags["m/p:0.1.7"] = part.Digest, part.Digest - b.cat.tagErr = errors.New("UNSUPPORTED") - if err := b.v.Remove(ArtifactSeat, "novox.mesh-delivery-builder", RemoveRequest{Name: "m/p", Version: "0.1.7-feat-z.1", Reason: "branch merged"}); err == nil || !strings.Contains(err.Error(), "shares its digest") || b.cat.tags["m/p:0.1.7"] != part.Digest { - t.Errorf("a store that cannot delete a tag alone: %v, tags %v; want refused and 0.1.7 kept", err, b.cat.tags) + b.cat.tagErr = errors.New("the store answered 500") + said := len(b.said) + if err := b.v.Remove(ArtifactSeat, "novox.mesh-delivery-builder", RemoveRequest{Name: "m/p", Version: "0.1.7-feat-z.1", Reason: "branch merged"}); err == nil || !strings.Contains(err.Error(), "shares its digest") || b.cat.tags["m/p:0.1.7"] != part.Digest || len(b.said) != said { + t.Errorf("a store failing otherwise: %v, tags %v; want refused, 0.1.7 kept, nothing said", err, b.cat.tags) } b.cat.tagErr = nil + // The store's own answers, as registry 2.8.3 gives them: DELETE by tag removes nothing (ErrNoTagAlone); by + // digest the manifest goes with every tag naming it. + r283 := newFakeRegistry() + sts := httptest.NewServer(r283) + defer sts.Close() + u, _ := url.Parse(sts.URL) + o := &OCI{Base: u} + manifest := []byte(`{"schemaVersion":2}`) + r283.manifests["m/p:0.1.8-feat-w.1"], r283.manifests["m/p:0.1.8"] = manifest, manifest + if err := o.RemoveTag("m/p", "0.1.8-feat-w.1"); !errors.Is(err, ErrNoTagAlone) || r283.manifests["m/p:0.1.8-feat-w.1"] == nil { + t.Errorf("RemoveTag on 2.8.3 = %v; want ErrNoTagAlone and the tag kept", err) + } + if err := o.Remove("m/p", "0.1.8"); err != nil || len(r283.manifests) != 0 { + t.Errorf("Remove by digest on 2.8.3 = %v, %d manifests left; want both tags gone", err, len(r283.manifests)) + } }) // CAT-AS1: what the sweep did over the store's door is the holder's: a removal for retention of an index removes // its platform manifests first, but those a tagged or held index of the repository names; a manifest such an index diff --git a/modules/distribution/internal/holder/mirror_fakes_test.go b/modules/distribution/internal/holder/mirror_fakes_test.go index 2ee12d86..d65f18d2 100644 --- a/modules/distribution/internal/holder/mirror_fakes_test.go +++ b/modules/distribution/internal/holder/mirror_fakes_test.go @@ -10,7 +10,10 @@ import ( ) // fakeRegistry is an OCI registry in memory, enough for a mirror's copy: blobs, uploads and manifests by -// reference. With token set it answers 401 with a Bearer challenge until a request carries it. +// reference. With token set it answers 401 with a Bearer challenge until a request carries it. A DELETE of a manifest +// answers as registry 2.8.3, the store's pinned image, does (distribution v2.8.3 registry/handlers/manifests.go, +// DeleteManifest): by digest it deletes the manifest and every tag naming it; by a tag it deletes nothing and answers +// 400 DIGEST_INVALID, the check of the empty digest it deletes instead failing. type fakeRegistry struct { mu sync.Mutex blobs map[string][]byte // name@digest @@ -69,6 +72,27 @@ func (f *fakeRegistry) ServeHTTP(w http.ResponseWriter, r *http.Request) { case strings.Contains(p, "/manifests/"): i := strings.Index(p, "/manifests/") key := p[:i] + ":" + p[i+11:] + if r.Method == http.MethodDelete { + ref := p[i+11:] + if !strings.HasPrefix(ref, "sha256:") { + w.WriteHeader(http.StatusBadRequest) + _, _ = io.WriteString(w, `{"errors":[{"code":"DIGEST_INVALID","message":"provided digest did not match uploaded content"}]}`) + return + } + found := false + for k, b := range f.manifests { + if strings.HasPrefix(k, p[:i]+":") && digestOf(b) == ref { + delete(f.manifests, k) + found = true + } + } + if !found { + w.WriteHeader(http.StatusNotFound) + return + } + w.WriteHeader(http.StatusAccepted) + return + } if r.Method == http.MethodPut { b, _ := io.ReadAll(r.Body) f.manifests[key] = b diff --git a/modules/distribution/internal/holder/oci.go b/modules/distribution/internal/holder/oci.go index c2bd00ef..7b41d9fe 100644 --- a/modules/distribution/internal/holder/oci.go +++ b/modules/distribution/internal/holder/oci.go @@ -3,6 +3,7 @@ package holder import ( "bytes" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -253,13 +254,22 @@ func (o *OCI) Digest(name, version string) (string, error) { return resp.Header.Get("Docker-Content-Digest"), nil } +// ErrNoTagAlone is a store that cannot delete a tag alone. registry 2.8.3, the store's pinned image, is one: its +// DeleteManifest (distribution v2.8.3 registry/handlers/manifests.go) ignores a tag reference and deletes the empty +// digest, whose check fails, answered 400 DIGEST_INVALID; registry 3 untags alone. +var ErrNoTagAlone = errors.New("the store cannot remove a tag alone") + // RemoveTag deletes the tag version of name alone (the OCI distribution protocol's DELETE of a manifest by its tag); -// the manifest stays under every other tag. A store that cannot delete a tag answers an error. +// the manifest stays under every other tag. A store that cannot delete a tag answers ErrNoTagAlone, any other failure +// an error of its own; either removes nothing. func (o *OCI) RemoveTag(name, version string) error { resp, b, err := o.do(http.MethodDelete, "/v2/"+name+"/manifests/"+version, "", nil) if err != nil { return err } + if resp.StatusCode == http.StatusBadRequest && bytes.Contains(b, []byte(`"DIGEST_INVALID"`)) { + return fmt.Errorf("%w: DELETE of %s:%s by its tag answered 400 DIGEST_INVALID", ErrNoTagAlone, name, version) + } if resp.StatusCode != http.StatusAccepted && resp.StatusCode != http.StatusNotFound { return fmt.Errorf("the store did not delete the tag %s:%s alone (%d): %s", name, version, resp.StatusCode, b) } diff --git a/modules/distribution/internal/holder/verbs.go b/modules/distribution/internal/holder/verbs.go index 095da21d..dd184de7 100644 --- a/modules/distribution/internal/holder/verbs.go +++ b/modules/distribution/internal/holder/verbs.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "slices" + "sort" "strings" "time" @@ -22,7 +23,7 @@ type Catalog interface { // collector frees its blobs. Remove(name, version string) error // RemoveTag deletes the tag version of name alone, the manifest staying under every other tag naming it; a store - // that cannot delete a tag alone answers an error and removes nothing. + // that cannot delete a tag alone answers ErrNoTagAlone and removes nothing. RemoveTag(name, version string) error // RemoveDigest deletes an untagged manifest of name by its digest (retention). RemoveDigest(name, digest string) error @@ -159,13 +160,18 @@ func (v *Verbs) Remove(seat, caller string, r RemoveRequest) error { if d != "" { // Reproducible builds share a digest between a branch's tagged version and the trunk's numbered one: a // manifest deleted by its digest would take the trunk's version with it (security review F1). When another - // tag names the digest, the tag alone goes; when the store cannot do that, nothing goes, said. + // tag names the digest, the tag alone goes. A store that cannot remove a tag alone (registry 2.8.3) keeps the + // tag: its bytes are kept under the other tags anyway, so the removal is said removed with that reason and + // the builder lets the version go from its keeping, no refusal and no retry. Any other failure is refused. others, err := v.otherTags(r.Name, r.Version, d) if err != nil { return fmt.Errorf("remove: the store cannot say which other tags of %s name %s, so nothing is removed: %w", r.Name, d, err) } if len(others) > 0 { - if err := v.Catalog.RemoveTag(r.Name, r.Version); err != nil { + if err := v.Catalog.RemoveTag(r.Name, r.Version); errors.Is(err, ErrNoTagAlone) { + sort.Strings(others) + ev.Reason = fmt.Sprintf("%s; the store cannot remove a tag alone, so the tag %s is kept and its bytes stay under %s", r.Reason, r.Version, strings.Join(others, ", ")) + } else if err != nil { return fmt.Errorf("remove: refused: %s@%s shares its digest %s with %v, and the store did not remove the tag alone, so nothing is removed: %w", r.Name, r.Version, d, others, err) } } else if err := v.Catalog.Remove(r.Name, r.Version); err != nil {