Skip to content

Commit d0d5463

Browse files
ab-ghoshab-ghosh
committed
Fix duplicate .att/.sig OCI layers for same-digest type hints
 When a Task declares the same OCI image through multiple type-hint formats (IMAGE_URL/IMAGE_DIGEST and IMAGES), Chains produces duplicate layers in cosign .att/.sig manifests. This adds dedup at two levels: 1. Extraction: track seen digests in ExtractOCIImagesFromResults to avoid returning the same digest from different type-hint formats 2. Storage: check existing layers in AttestationStorer and SimpleStorer before appending, preventing duplicates from independent signable types (TaskRunArtifact + OCIArtifact) storing to the same tag Also adds debug logging when existing layer checks fail instead of silently swallowing errors. Fixes #1596
1 parent 7290445 commit d0d5463

6 files changed

Lines changed: 342 additions & 0 deletions

File tree

‎pkg/artifacts/signable.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ func (oa *OCIArtifact) ExtractObjects(ctx context.Context, obj objects.TektonObj
164164
func ExtractOCIImagesFromResults(ctx context.Context, results []objects.Result) []interface{} {
165165
logger := logging.FromContext(ctx)
166166
objs := []interface{}{}
167+
seen := map[string]bool{}
167168

168169
extractor := structuredSignableExtractor{
169170
uriSuffix: OCIImageURLResultName,
@@ -176,6 +177,11 @@ func ExtractOCIImagesFromResults(ctx context.Context, results []objects.Result)
176177
logger.Errorf("error getting digest: %v", err)
177178
continue
178179
}
180+
if seen[dgst.Name()] {
181+
logger.Debugf("skipping duplicate image %s", dgst.Name())
182+
continue
183+
}
184+
seen[dgst.Name()] = true
179185
objs = append(objs, dgst)
180186
}
181187

@@ -196,6 +202,11 @@ func ExtractOCIImagesFromResults(ctx context.Context, results []objects.Result)
196202
logger.Errorf("error getting digest for img %s: %v", trimmed, err)
197203
continue
198204
}
205+
if seen[dgst.Name()] {
206+
logger.Debugf("skipping duplicate image %s", dgst.Name())
207+
continue
208+
}
209+
seen[dgst.Name()] = true
199210
objs = append(objs, dgst)
200211
}
201212
}

‎pkg/artifacts/signable_test.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,49 @@ func TestExtractOCIImagesFromResults(t *testing.T) {
205205
}
206206
}
207207

208+
func TestExtractOCIImagesFromResults_CrossFormatDedup(t *testing.T) {
209+
// Same image appears via IMAGE_URL/IMAGE_DIGEST and IMAGES — should be deduplicated.
210+
results := []objects.Result{
211+
{Name: "IMAGE_URL", Value: *v1.NewStructuredValues("img1")},
212+
{Name: "IMAGE_DIGEST", Value: *v1.NewStructuredValues(digest1)},
213+
{Name: "IMAGES", Value: *v1.NewStructuredValues(fmt.Sprintf("img1@%s", digest1))},
214+
}
215+
216+
want := []interface{}{
217+
createDigest(t, fmt.Sprintf("img1@%s", digest1)),
218+
}
219+
ctx := logtesting.TestContextWithLogger(t)
220+
got := ExtractOCIImagesFromResults(ctx, results)
221+
if !cmp.Equal(got, want, ignore...) {
222+
t.Fatalf("expected dedup across type-hint formats, got %s", cmp.Diff(want, got, ignore...))
223+
}
224+
}
225+
226+
func TestExtractOCIImagesFromResults_DifferentReposSameDigest(t *testing.T) {
227+
// Two different repositories with the same digest should NOT be deduplicated.
228+
results := []objects.Result{
229+
{Name: "img1_IMAGE_URL", Value: *v1.NewStructuredValues("gcr.io/project/img1")},
230+
{Name: "img1_IMAGE_DIGEST", Value: *v1.NewStructuredValues(digest1)},
231+
{Name: "img2_IMAGE_URL", Value: *v1.NewStructuredValues("quay.io/project/img2")},
232+
{Name: "img2_IMAGE_DIGEST", Value: *v1.NewStructuredValues(digest1)},
233+
}
234+
235+
want := []interface{}{
236+
createDigest(t, fmt.Sprintf("gcr.io/project/img1@%s", digest1)),
237+
createDigest(t, fmt.Sprintf("quay.io/project/img2@%s", digest1)),
238+
}
239+
ctx := logtesting.TestContextWithLogger(t)
240+
got := ExtractOCIImagesFromResults(ctx, results)
241+
sort.Slice(got, func(i, j int) bool {
242+
a := got[i].(name.Digest)
243+
b := got[j].(name.Digest)
244+
return a.String() < b.String()
245+
})
246+
if !cmp.Equal(got, want, ignore...) {
247+
t.Fatalf("different repos with same digest should not be deduped: %s", cmp.Diff(want, got, ignore...))
248+
}
249+
}
250+
208251
func TestExtractSignableTargetFromResults(t *testing.T) {
209252
tr := &v1.TaskRun{
210253
Status: v1.TaskRunStatus{

‎pkg/chains/storage/oci/attestation.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,25 @@ func (s *AttestationStorer) Store(ctx context.Context, req *api.StoreRequest[nam
7777
if err != nil {
7878
return nil, err
7979
}
80+
81+
// Check if an attestation with the same digest already exists.
82+
newDigest, err := att.Digest()
83+
if err != nil {
84+
return nil, errors.Wrap(err, "getting new attestation digest")
85+
}
86+
if existingAtts, err := se.Attestations(); err != nil {
87+
logger.Debugf("Could not fetch existing attestations for %s, skipping dedup check: %v", req.Artifact.String(), err)
88+
} else if layers, err := existingAtts.Get(); err != nil {
89+
logger.Debugf("Could not get attestation layers for %s, skipping dedup check: %v", req.Artifact.String(), err)
90+
} else {
91+
for _, l := range layers {
92+
if d, err := l.Digest(); err == nil && d == newDigest {
93+
logger.Infof("Attestation with digest %s already exists for %s, skipping", newDigest, req.Artifact.String())
94+
return &api.StoreResponse{}, nil
95+
}
96+
}
97+
}
98+
8099
newImage, err := mutate.AttachAttestationToEntity(se, att)
81100
if err != nil {
82101
return nil, err

‎pkg/chains/storage/oci/attestation_test.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"github.com/google/go-containerregistry/pkg/v1/remote"
2727
"github.com/google/go-containerregistry/pkg/v1/types"
2828
intoto "github.com/in-toto/attestation/go/v1"
29+
ociremote "github.com/sigstore/cosign/v2/pkg/oci/remote"
2930
"github.com/tektoncd/chains/pkg/chains/signing"
3031
"github.com/tektoncd/chains/pkg/chains/storage/api"
3132
logtesting "knative.dev/pkg/logging/testing"
@@ -109,3 +110,127 @@ func TestAttestationStorer_Store(t *testing.T) {
109110
})
110111
}
111112
}
113+
114+
func TestAttestationStorer_Store_Dedup(t *testing.T) {
115+
s := httptest.NewServer(registry.New())
116+
defer s.Close()
117+
registryName := strings.TrimPrefix(s.URL, "http://")
118+
119+
img, err := random.Image(1024, 2)
120+
if err != nil {
121+
t.Fatalf("failed to create random image: %s", err)
122+
}
123+
imgDigest, err := img.Digest()
124+
if err != nil {
125+
t.Fatalf("failed to get image digest: %v", err)
126+
}
127+
ref, err := name.NewDigest(fmt.Sprintf("%s/test/img@%s", registryName, imgDigest))
128+
if err != nil {
129+
t.Fatalf("failed to parse digest: %v", err)
130+
}
131+
if err := remote.Write(ref, img); err != nil {
132+
t.Fatalf("failed to write image to mock registry: %v", err)
133+
}
134+
135+
storer, err := NewAttestationStorer(WithTargetRepository(ref.Repository))
136+
if err != nil {
137+
t.Fatalf("failed to create storer: %v", err)
138+
}
139+
140+
ctx := logtesting.TestContextWithLogger(t)
141+
req := &api.StoreRequest[name.Digest, *intoto.Statement]{
142+
Artifact: ref,
143+
Payload: &intoto.Statement{},
144+
Bundle: &signing.Bundle{Signature: []byte("sig1")},
145+
}
146+
147+
// Store the same attestation twice.
148+
if _, err := storer.Store(ctx, req); err != nil {
149+
t.Fatalf("first Store() failed: %s", err)
150+
}
151+
if _, err := storer.Store(ctx, req); err != nil {
152+
t.Fatalf("second Store() failed: %s", err)
153+
}
154+
155+
// Verify only one attestation layer exists.
156+
se, err := ociremote.SignedEntity(ref)
157+
if err != nil {
158+
t.Fatalf("failed to get signed entity: %v", err)
159+
}
160+
atts, err := se.Attestations()
161+
if err != nil {
162+
t.Fatalf("failed to get attestations: %v", err)
163+
}
164+
layers, err := atts.Get()
165+
if err != nil {
166+
t.Fatalf("failed to get attestation layers: %v", err)
167+
}
168+
if got := len(layers); got != 1 {
169+
t.Errorf("expected 1 attestation layer, got %d", got)
170+
}
171+
}
172+
173+
func TestAttestationStorer_Store_DistinctNotDeduped(t *testing.T) {
174+
s := httptest.NewServer(registry.New())
175+
defer s.Close()
176+
registryName := strings.TrimPrefix(s.URL, "http://")
177+
178+
img, err := random.Image(1024, 2)
179+
if err != nil {
180+
t.Fatalf("failed to create random image: %s", err)
181+
}
182+
imgDigest, err := img.Digest()
183+
if err != nil {
184+
t.Fatalf("failed to get image digest: %v", err)
185+
}
186+
ref, err := name.NewDigest(fmt.Sprintf("%s/test/img@%s", registryName, imgDigest))
187+
if err != nil {
188+
t.Fatalf("failed to parse digest: %v", err)
189+
}
190+
if err := remote.Write(ref, img); err != nil {
191+
t.Fatalf("failed to write image to mock registry: %v", err)
192+
}
193+
194+
storer, err := NewAttestationStorer(WithTargetRepository(ref.Repository))
195+
if err != nil {
196+
t.Fatalf("failed to create storer: %v", err)
197+
}
198+
199+
ctx := logtesting.TestContextWithLogger(t)
200+
201+
// Store two attestations with different signatures (different layer content).
202+
req1 := &api.StoreRequest[name.Digest, *intoto.Statement]{
203+
Artifact: ref,
204+
Payload: &intoto.Statement{},
205+
Bundle: &signing.Bundle{Signature: []byte("sig1")},
206+
}
207+
req2 := &api.StoreRequest[name.Digest, *intoto.Statement]{
208+
Artifact: ref,
209+
Payload: &intoto.Statement{},
210+
Bundle: &signing.Bundle{Signature: []byte("sig2")},
211+
}
212+
213+
if _, err := storer.Store(ctx, req1); err != nil {
214+
t.Fatalf("first Store() failed: %s", err)
215+
}
216+
if _, err := storer.Store(ctx, req2); err != nil {
217+
t.Fatalf("second Store() failed: %s", err)
218+
}
219+
220+
// Verify both attestation layers are kept.
221+
se, err := ociremote.SignedEntity(ref)
222+
if err != nil {
223+
t.Fatalf("failed to get signed entity: %v", err)
224+
}
225+
atts, err := se.Attestations()
226+
if err != nil {
227+
t.Fatalf("failed to get attestations: %v", err)
228+
}
229+
layers, err := atts.Get()
230+
if err != nil {
231+
t.Fatalf("failed to get attestation layers: %v", err)
232+
}
233+
if got := len(layers); got != 2 {
234+
t.Errorf("expected 2 distinct attestation layers, got %d", got)
235+
}
236+
}

‎pkg/chains/storage/oci/simple.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,25 @@ func (s *SimpleStorer) Store(ctx context.Context, req *api.StoreRequest[name.Dig
7474
if err != nil {
7575
return nil, err
7676
}
77+
78+
// Check if a signature with the same payload digest already exists.
79+
newDigest, err := sig.Digest()
80+
if err != nil {
81+
return nil, errors.Wrap(err, "getting new signature digest")
82+
}
83+
if existingSigs, err := se.Signatures(); err != nil {
84+
logger.Debugf("Could not fetch existing signatures for %s, skipping dedup check: %v", req.Artifact.String(), err)
85+
} else if layers, err := existingSigs.Get(); err != nil {
86+
logger.Debugf("Could not get signature layers for %s, skipping dedup check: %v", req.Artifact.String(), err)
87+
} else {
88+
for _, l := range layers {
89+
if d, err := l.Digest(); err == nil && d == newDigest {
90+
logger.Infof("Signature with digest %s already exists, skipping", newDigest)
91+
return &api.StoreResponse{}, nil
92+
}
93+
}
94+
}
95+
7796
// Attach the signature to the entity.
7897
newSE, err := mutate.AttachSignatureToEntity(se, sig)
7998
if err != nil {

0 commit comments

Comments
 (0)