From 00d6fe81bf1e93ea1d149cb1ce2fdf55448cfef8 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 4 Nov 2026 17:30:50 +0100 Subject: [PATCH] F-parts: on registry 2.8.3 a shared-digest tagged version's remove keeps the tag and says removed The store's pinned image is registry 2.8.3, whose DeleteManifest cannot delete a tag alone: by a tag it deletes the empty digest and answers 400 DIGEST_INVALID. The fakes accepted a DELETE by tag, a fixture agreeing with the mistake, so every merge of a reproducible build would have been a refused remove and a retry finding live. RemoveTag now answers ErrNoTagAlone for that answer; remove then keeps the tag, whose bytes stay under the other tag anyway, and says removed with that reason, so the builder lets it go. Any other store failure is still refused; a digest shared with nothing is still deleted by digest. The fakes answer as 2.8.3 does. Claude-Session: https://claude.ai/code/session_01JG73KJNrH5EgFXMKKYSrig --- .../internal/holder/holder_test.go | 46 ++++++++++++++----- .../internal/holder/mirror_fakes_test.go | 26 ++++++++++- modules/distribution/internal/holder/oci.go | 12 ++++- modules/distribution/internal/holder/verbs.go | 12 +++-- 4 files changed, 79 insertions(+), 17 deletions(-) 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 {