download: also emit old-format attestations when a bundle exists (#4573) - #4976
download: also emit old-format attestations when a bundle exists (#4573)#4976arpitjain099 wants to merge 1 commit into
Conversation
cosign attestation download short-circuited after handling new-format sigstore bundles, so images that carry any bundle referrer (for example a signature-only bundle) never had their old-format sha256-<digest>.att attestations fetched. Since GetBundles returns every bundle referrer, download returned zero old-format attestations on any image that also has a new-bundle signature. download is an inspection command, so it now gathers both formats: it writes any new-format bundles, then also fetches old-format attestations, and only surfaces a missing-old-format error when no new-format bundle was written. verify paths are unchanged. Adds the package's first unit tests covering all three cases. Fixes sigstore#4573 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4976 +/- ##
==========================================
- Coverage 40.10% 38.53% -1.57%
==========================================
Files 155 209 +54
Lines 10044 13017 +2973
==========================================
+ Hits 4028 5016 +988
- Misses 5530 7287 +1757
- Partials 486 714 +228 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
steiza
left a comment
steiza
left a comment
There was a problem hiding this comment.
I do appreciate that this adds tests, but I wonder if we should have just removed the
line 😅 - the tests add quite a bit of complexity.At the very least, let's not swallow the error from cosign.GetBundles().
| continue | ||
| } | ||
| if err != nil || len(newBundles) == 0 { | ||
| return 0, nil |
There was a problem hiding this comment.
We probably don't want to swallow the error here?
| return 0, nil | |
| return 0, err |
|
We landed #4996 instead, but thank you for the push to get this fixed! |
ah I was slow to respond but thank you for merging the change! Happy to see the changes made it through! |
2 participants
I work on supply-chain security tooling and ran into this while inspecting an image's attestations.
cosign attestation downloadreturns zero old-format.attattestations whenever the image also carries a new-bundle-format signature.GetBundlesreturns every bundle referrer (including signature-only ones), so the new-format branch is taken even when there are no matching attestations, and the command then returned before it ever fetched the old-formatsha256-<digest>.attentries. This is #4573, and @steiza confirmed on that issue that download (unlike verify) should handle both formats.The fix drops that early return so download writes any new-format bundles and then also fetches the old-format attestations, emitting both. A missing old-format entity is only treated as an error when nothing was written yet, so existing behavior is preserved for old-format-only images. verify paths are untouched, and I've scoped this to inspection-only download semantics so it doesn't conflict with the broader new-bundle work in the draft #4959.
The download package had no tests, so I added the first ones: the main case fails on the old code (old-format attestation dropped) and passes with the fix, plus two cases covering old-format-only errors and tolerating a missing old-format entity.
go test,go vet, andgofmtare clean on the package.