From 1bdca045c2daa1d74440d3e20bb4dcbbc83f1799 Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 4 Nov 2026 17:04:43 +0100 Subject: [PATCH] F-catalog-B: distribution removes a tagged version's tag alone when another tag names its digest Deleting the manifest by its digest removed every tag on it, and reproducible builds share a digest between a branch's tagged version and the trunk's numbered one, so a branch's end deleted the trunk's image (security review F1). The fake catalog now deletes by digest as the store does; a store that cannot delete a tag alone removes nothing and the removal is refused. Claude-Session: https://claude.ai/code/session_01JG73KJNrH5EgFXMKKYSrig --- .../internal/holder/holder_test.go | 32 +++++++++++++++ modules/distribution/internal/holder/oci.go | 15 ++++++- modules/distribution/internal/holder/verbs.go | 41 ++++++++++++++++++- 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/modules/distribution/internal/holder/holder_test.go b/modules/distribution/internal/holder/holder_test.go index 5e15abfa..c4324bcd 100644 --- a/modules/distribution/internal/holder/holder_test.go +++ b/modules/distribution/internal/holder/holder_test.go @@ -89,6 +89,7 @@ type fakeCatalog struct { tags map[string]string // name:version -> digest removed []string mirrorErr error + tagErr error indexes map[string][]string // name@digest -> the platform manifests it names } @@ -104,8 +105,23 @@ func (c *fakeCatalog) Versions(name string) ([]string, error) { func (c *fakeCatalog) Digest(name, version string) (string, error) { return c.tags[name+":"+version], nil } + +// Remove deletes by the digest, as the store does: every tag naming the digest goes with it. func (c *fakeCatalog) Remove(name, version string) error { c.removed = append(c.removed, name+":"+version) + d := c.tags[name+":"+version] + for k, v := range c.tags { + if n, _, _ := strings.Cut(k, ":"); n == name && v == d { + delete(c.tags, k) + } + } + return nil +} +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 } @@ -368,6 +384,22 @@ 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. + 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) + } + 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) + } + // A store that cannot delete a tag alone removes nothing, and the removal 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 = nil }) // 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/oci.go b/modules/distribution/internal/holder/oci.go index c722d03b..c2bd00ef 100644 --- a/modules/distribution/internal/holder/oci.go +++ b/modules/distribution/internal/holder/oci.go @@ -253,7 +253,20 @@ func (o *OCI) Digest(name, version string) (string, error) { return resp.Header.Get("Docker-Content-Digest"), nil } -// Remove deletes the manifest name holds at version. +// 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. +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.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) + } + return nil +} + +// Remove deletes the manifest name holds at version, by its digest: every tag naming it goes with it. func (o *OCI) Remove(name, version string) error { d, err := o.Digest(name, version) if err != nil || d == "" { diff --git a/modules/distribution/internal/holder/verbs.go b/modules/distribution/internal/holder/verbs.go index cc90a063..31af720b 100644 --- a/modules/distribution/internal/holder/verbs.go +++ b/modules/distribution/internal/holder/verbs.go @@ -18,8 +18,12 @@ type Catalog interface { Versions(name string) ([]string, error) // Digest is the digest name holds at version, empty when none. Digest(name, version string) (string, error) - // Remove deletes name at version (its manifest); the store's collector frees its blobs. + // Remove deletes name at version by its manifest's digest, every tag naming that digest with it; the store's + // 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. + RemoveTag(name, version string) error // RemoveDigest deletes an untagged manifest of name by its digest (retention). RemoveDigest(name, digest string) error // Platforms are the manifests an index of name names; none for a manifest that is not an index or is not held. @@ -149,7 +153,18 @@ func (v *Verbs) Remove(seat, caller string, r RemoveRequest) error { return fmt.Errorf("remove: the store cannot read %s@%s: %w", r.Name, r.Version, err) } if d != "" { - if err := v.Catalog.Remove(r.Name, r.Version); err != nil { + // 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. + 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 { + 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 { return fmt.Errorf("remove: the store did not remove %s@%s: %w", r.Name, r.Version, err) } } @@ -158,6 +173,28 @@ func (v *Verbs) Remove(seat, caller string, r RemoveRequest) error { return v.Say(seat, EventRemoved, ev) } +// otherTags are the tags of name but version that name digest. +func (v *Verbs) otherTags(name, version, digest string) ([]string, error) { + tags, err := v.Catalog.Versions(name) + if err != nil { + return nil, err + } + var out []string + for _, t := range tags { + if t == version { + continue + } + d, err := v.Catalog.Digest(name, t) + if err != nil { + return nil, err + } + if d == digest { + out = append(out, t) + } + } + return out, nil +} + // HoldRequest is `hold` and `release`. type HoldRequest struct { Name string `json:"name"`