✨ Add GitHub artifact attestation for Signed-Releases#5001
Open
martincostello wants to merge 1 commit intoossf:mainfrom
Open
✨ Add GitHub artifact attestation for Signed-Releases#5001martincostello wants to merge 1 commit intoossf:mainfrom
martincostello wants to merge 1 commit intoossf:mainfrom
Conversation
Consider a release signed if all assets a digest with a matching GitHub attestation. Signed-off-by: martincostello <martin@martincostello.com>
martincostello
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5001 +/- ##
==========================================
+ Coverage 66.80% 68.25% +1.44%
==========================================
Files 230 252 +22
Lines 16602 15743 -859
==========================================
- Hits 11091 10745 -346
+ Misses 4808 4156 -652
- Partials 703 842 +139 🚀 New features to boost your workflow:
|
martincostello
requested review from
AdamKorcz,
Copilot and
spencerschrock
and removed request for
a team
dosubot
Bot
added
the
size:XL
There was a problem hiding this comment.
Pull request overview
Adds support for treating GitHub Artifact Attestations as an alternative “signed release” signal in the Signed-Releases check, using GitHub release asset digests + attestations API lookups.
Changes:
- Extend release assets to carry
DigestandHasAttestation, populated by the GitHub repo client via attestations API calls. - Add a new
releasesHaveAttestationprobe (plus docs/tests) and wire it into the Signed-Releases probe set. - Update Signed-Releases evaluation logic and documentation to award max score when attestations cover all assets.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| probes/releasesHaveAttestation/impl.go | New probe that reports per-release whether all assets have digests + attestations. |
| probes/releasesHaveAttestation/impl_test.go | Unit tests for the new attestation probe behavior and lookback. |
| probes/releasesHaveAttestation/def.yml | Probe metadata definition for documentation/remediation text. |
| probes/entries.go | Registers the new probe under the Signed-Releases check. |
| docs/probes.md | Documents the new probe and its outcomes. |
| docs/checks/internal/checks.yaml | Updates Signed-Releases check definition to include attestations as a max-score path. |
| docs/checks.md | Updates public Signed-Releases documentation to include attestations guidance. |
| clients/release.go | Adds Digest and HasAttestation fields to ReleaseAsset. |
| clients/githubrepo/releases.go | Populates digest from release assets and queries attestations API to set HasAttestation. |
| clients/githubrepo/releases_test.go | Adds tests for digest parsing and attestations API behavior. |
| checks/evaluation/signed_releases.go | Incorporates the attestation probe into scoring/log suppression logic. |
| checks/evaluation/signed_releases_test.go | Extends evaluation tests to cover attestation-driven scoring/logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+94
to
+96
| loc := &finding.Location{ | ||
| Type: finding.FileTypeURL, | ||
| Path: release.URL, |
There was a problem hiding this comment.
The failing-case finding uses release.URL, which is the GitHub API URL (r.GetURL()), while the success-case uses asset.URL (HTML URL). This yields inconsistent/unfriendly links in findings; since len(release.Assets) > 0 here, consider using the release HTML URL consistently (e.g., reusing release.Assets[0].URL or storing HTMLURL on clients.Release).
Suggested change
| loc := &finding.Location{ | |
| Type: finding.FileTypeURL, | |
| Path: release.URL, | |
| // Use the first asset URL as a representative HTML URL for the release, | |
| // matching the success case and avoiding the GitHub API URL in findings. | |
| loc := &finding.Location{ | |
| Type: finding.FileTypeURL, | |
| Path: release.Assets[0].URL, |
|
|
||
| if len(findings) == 0 { | ||
| f, err := finding.NewWith(fs, Probe, | ||
| "no GitHub releases found", |
There was a problem hiding this comment.
When findings is empty this reports "no GitHub releases found", but findings can also be empty when releases exist but all of them have zero assets (those are continued above). Consider adjusting the message (or emitting OutcomeNotApplicable per asset-less release) so the result is accurate.
Suggested change
| "no GitHub releases found", | |
| "no GitHub releases with assets found", |
Comment on lines
+74
to
+83
| func (handler *releasesHandler) resolveOwnerEndpointPrefix() string { | ||
| user, _, err := handler.client.Users.Get(handler.ctx, handler.repourl.owner) | ||
| if err != nil { | ||
| // Fall back to users; hasAttestation will skip on 404. | ||
| return ownerEndpointUser | ||
| } | ||
| if strings.EqualFold(user.GetType(), "Organization") { | ||
| return ownerEndpointOrg | ||
| } | ||
| return ownerEndpointUser |
There was a problem hiding this comment.
The comment says "hasAttestation will skip on 404", but hasAttestation currently returns false for all errors and does not special-case 404s. This also means if Users.Get fails (rate limit/network) for an org-owned repo, the fallback to users/... can cause all attestation checks to incorrectly return false due to hitting the wrong endpoint. Consider either (a) updating the comment, or (b) handling 404 by retrying the alternate users/orgs prefix once before returning false.
Comment on lines
+64
to
69
| handler.ownerEndpointPrefix = handler.resolveOwnerEndpointPrefix() | ||
| handler.checkAttestations() | ||
| }) | ||
| return handler.errSetup | ||
| } | ||
|
|
There was a problem hiding this comment.
checkAttestations() is invoked unconditionally during ListReleases() setup and iterates over every release/asset returned by Repositories.ListReleases (default 30 releases). This adds a potentially large number of extra REST calls (and likely 401/403s without an auth token) even for checks that don't need attestations, which can noticeably increase runtime and risk API rate limiting. Consider deferring attestation lookups to the attestation probe (or gating/limiting them to the same lookback window the probe evaluates).
Suggested change
| handler.ownerEndpointPrefix = handler.resolveOwnerEndpointPrefix() | |
| handler.checkAttestations() | |
| }) | |
| return handler.errSetup | |
| } | |
| }) | |
| return handler.errSetup | |
| } | |
| // setupAttestations lazily enriches the already-loaded releases with | |
| // attestation metadata. This is intentionally separate from setup so that | |
| // generic release listing does not incur the additional per-asset API calls. | |
| func (handler *releasesHandler) setupAttestations() error { | |
| if err := handler.setup(); err != nil { | |
| return err | |
| } | |
| handler.ownerEndpointPrefix = handler.resolveOwnerEndpointPrefix() | |
| handler.checkAttestations() | |
| return nil | |
| } |
Comment on lines
124
to
133
| logLevel = checker.DetailWarn | ||
| if f.Probe == releasesAreSigned.Probe && hasProvenance[releaseName] { | ||
| if f.Probe == releasesAreSigned.Probe && hasProvenanceOrAttestation[releaseName] { | ||
| continue | ||
| } | ||
| // Attestation is an optional alternative to signing or provenance. | ||
| // Suppress warnings when attestation is absent to avoid noise for projects | ||
| // that use other verification methods. | ||
| if f.Probe == releasesHaveAttestation.Probe { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The code treats attestation as an alternative for scoring (sets 10) and suppresses warnings when the attestation probe is false, but it still logs a warning when provenance is false even if attestation is true for that release. If attestation is intended as an alternative to provenance, consider also suppressing the provenance warning for releases that already have an attestation OutcomeTrue to avoid confusing/noisy output when the check is already at max score.
Contributor
Author
|
Results from local manual run: |
|
This pull request has been marked stale because it has been open for 10 days with no activity |
Contributor
Author
|
Not stale - waiting for feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
New feature
What is the current behavior?
GitHub attestations are not considered.
What is the new behavior (if this is a feature change)?
Consider a GitHub release signed if all assets a digest with a matching GitHub attestation.
Which issue(s) this PR fixes
Fixes #4667
Special notes for your reviewer
None.
Does this PR introduce a user-facing change?