chore(dispatcher): Add attestation verifier package for Sigstore bundle verification#1670
Merged
Conversation
Introduces cli/dispatcher/internal/attestation/, a self-contained package that verifies GitHub Artifact Attestation bundles emitted by the release workflows. The verifier is unused at this commit — Phase B2 and B3 will wire it into dispatcher self-update (update.go) and project-runner download (dispatcher_download.go) respectively — so the dispatcher binary still behaves exactly as before. Landing the package alone keeps the review surface small before the runtime call sites change. Trust anchor is the embedded Sigstore public-good trusted_root.json, fetched once via the new cli/dispatcher/cmd/refresh-attestation-trusted-root helper (which drives sigstore-go's TUF client). Embedding keeps verification offline and deterministic; a companion Go test in trusted_root_test.go fails the build 90 days before any embedded authority expires so a missed quarterly refresh surfaces as a red CI rather than a fleet-wide self-update outage after Rekor or Fulcio rotates. Refresh commits must land as fix(dispatcher) so release-please cuts a new dispatcher release that actually ships the refreshed root. Verification policy binds each attestation to (a) the local asset's SHA-256 digest appearing among the bundle's subjects, (b) a SAN allowlist of exact workflow@ref identities for hatayama/unity-cli-loop, and (c) the Fulcio Source Repository Digest extension (OID 1.3.6.1.4.1.57264.1.13) matching the release tag's commit SHA. The commit SHA binding prevents a leaked OIDC token from being reused on a different tag. The SAN is a closed allowlist (v3-beta and main branches for either dispatcher-publish.yml or native-cli-publish.yml at the call site) rather than a regex so an unrelated ref cannot forge releases. Fetcher helpers fail closed on every non-2xx status (including 403 and 429) so an attacker cannot strip verification by DoS'ing the attestation endpoint. GITHUB_TOKEN/GH_TOKEN authorization is passed through when present to lift rate limits in CI. Tests cover the five required patterns (happy path, digest mismatch, identity mismatch by SAN, identity mismatch by commit SHA, malformed bundle) plus fetcher fail-closed behavior on 404/403/429/empty-body and tag lookup for both lightweight and annotated tag objects. The happy path fixture is the real backfilled dispatcher-v3.0.1-beta.12 arm64 bundle so tests exercise a bundle with the same media type production callers will hand the verifier. The release-automation architecture guard now allows internal/attestation alongside the existing dispatcher, install, nativepath, uninstall, update packages.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesAttestation verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Verify
participant TrustedMaterial
participant SigstoreVerifier
Caller->>Verify: provide bundle, digest, commit SHA, and identity
Verify->>TrustedMaterial: use trusted root material
Verify->>SigstoreVerifier: verify signature, log, timestamp, certificate, and digest
SigstoreVerifier-->>Verify: verification result
Verify-->>Caller: success or classified error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cli/dispatcher/internal/attestation/verifier_test.go (1)
230-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
FetchTagCommitSHAerror paths andfetchGitRefdecode failure.The code handles three error paths that lack test coverage:
- Unexpected object type (verifier.go line 98–100): ref returns type
"tree"or"blob".- Bad SHA format (verifier.go line 101–103): ref returns a non-hex or wrong-length SHA.
- JSON decode failure (fetcher.go line 126–128): 200 response with invalid JSON body.
These are fail-closed security paths — a regression here could silently accept invalid tag resolutions.
🧪 Suggested test additions
// Verifies FetchTagCommitSHA fails closed on unexpected object type. func TestFetchTagCommitSHA_UnexpectedObjectType(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, `{"object":{"sha":"1eb1ebb9841b1bcb8fc7dec3fa282568a1c31a4f","type":"tree"}}`) })) defer server.Close() original := githubAPIBase() setGithubAPIBase(server.URL) defer setGithubAPIBase(original) _, err := FetchTagCommitSHA(context.Background(), "hatayama/unity-cli-loop", "any") if err == nil { t.Fatalf("expected unexpected object type to fail") } if !errors.Is(err, ErrTagRefFetch) { t.Fatalf("expected ErrTagRefFetch, got: %v", err) } } // Verifies FetchTagCommitSHA fails closed on a non-hex SHA. func TestFetchTagCommitSHA_BadSHA(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, `{"object":{"sha":"not-a-hex-sha-but-40-chars-long-xxxxxxxxx","type":"commit"}}`) })) defer server.Close() original := githubAPIBase() setGithubAPIBase(server.URL) defer setGithubAPIBase(original) _, err := FetchTagCommitSHA(context.Background(), "hatayama/unity-cli-loop", "any") if err == nil { t.Fatalf("expected bad SHA to fail") } if !errors.Is(err, ErrTagRefFetch) { t.Fatalf("expected ErrTagRefFetch, got: %v", err) } } // Verifies fetchGitRef fails closed on invalid JSON. func TestFetchTagCommitSHA_InvalidJSON(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") fmt.Fprintln(w, `this is not json`) })) defer server.Close() original := githubAPIBase() setGithubAPIBase(server.URL) defer setGithubAPIBase(original) _, err := FetchTagCommitSHA(context.Background(), "hatayama/unity-cli-loop", "any") if err == nil { t.Fatalf("expected invalid JSON to fail") } if !errors.Is(err, ErrTagRefFetch) { t.Fatalf("expected ErrTagRefFetch, got: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/dispatcher/internal/attestation/verifier_test.go` around lines 230 - 302, Add coverage for the untested fail-closed paths in FetchTagCommitSHA: add separate HTTP-server tests for an unexpected object type, an invalid-length or non-hex SHA, and invalid JSON returned by fetchGitRef. For each test, configure githubAPIBase to the test server, invoke FetchTagCommitSHA, assert an error is returned, and verify errors.Is(err, ErrTagRefFetch); restore the original API base and close the server as existing tests do.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/dispatcher/internal/attestation/fetcher.go`:
- Around line 61-64: Limit the bundle response size before reading it in
FetchBundle: wrap resp.Body with io.LimitReader using a 10 MB cap (and account
for an over-limit response), then pass the limited reader to io.ReadAll and
return an appropriate ErrBundleFetch error when the limit is exceeded.
In `@cli/dispatcher/internal/attestation/trusted_root_test.go`:
- Around line 107-125: Update isValidBeyond to parse and require validFor.start
to be at or before the evaluation time, while preserving the existing end-date
and unlimited-end handling. Adjust its call site to provide the current time
separately from the horizon (or derive it as
horizon.Add(-trustedRootWarningHorizon)), and ensure both timestamp precisions
are supported consistently.
---
Nitpick comments:
In `@cli/dispatcher/internal/attestation/verifier_test.go`:
- Around line 230-302: Add coverage for the untested fail-closed paths in
FetchTagCommitSHA: add separate HTTP-server tests for an unexpected object type,
an invalid-length or non-hex SHA, and invalid JSON returned by fetchGitRef. For
each test, configure githubAPIBase to the test server, invoke FetchTagCommitSHA,
assert an error is returned, and verify errors.Is(err, ErrTagRefFetch); restore
the original API base and close the server as existing tests do.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 097c0ecb-2906-49c5-887b-fbfd3ac48482
⛔ Files ignored due to path filters (5)
cli/dispatcher/go.modis excluded by none and included by nonecli/dispatcher/go.sumis excluded by!**/*.sumand included by nonecli/dispatcher/internal/attestation/testdata/happy_asset_digest.txtis excluded by none and included by nonecli/dispatcher/internal/attestation/testdata/happy_commit_sha.txtis excluded by none and included by nonecli/go.work.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (10)
cli/dispatcher/cmd/refresh-attestation-trusted-root/main.gocli/dispatcher/internal/attestation/errors.gocli/dispatcher/internal/attestation/fetcher.gocli/dispatcher/internal/attestation/testdata/happy_bundle.jsoncli/dispatcher/internal/attestation/trusted_root.gocli/dispatcher/internal/attestation/trusted_root.jsoncli/dispatcher/internal/attestation/trusted_root_test.gocli/dispatcher/internal/attestation/verifier.gocli/dispatcher/internal/attestation/verifier_test.gocli/release-automation/internal/architecture/architecture_test.go
Comment on lines
+61
to
+64
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: read body: %v", ErrBundleFetch, err) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded io.ReadAll on bundle response risks OOM.
FetchBundle reads the entire response body without a size cap. A compromised CDN, buggy server, or MITM could send a multi-GB response within the 30s client timeout, exhausting memory in the dispatcher process. Sigstore bundles are typically a few KB; a generous limit (e.g., 10 MB) would protect this security-sensitive path without rejecting valid bundles.
🛡️ Proposed fix: wrap body in `io.LimitReader`
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, fmt.Errorf("%w: read body: %v", ErrBundleFetch, err)
}+ const maxBundleBytes = 10 << 20 // 10 MB — well above any valid Sigstore bundle
+
body, err := io.ReadAll(io.LimitReader(resp.Body, maxBundleBytes+1))
if err != nil {
return nil, fmt.Errorf("%w: read body: %v", ErrBundleFetch, err)
}
if len(body) > maxBundleBytes {
return nil, fmt.Errorf("%w: bundle exceeds %d bytes from %s", ErrBundleFetch, maxBundleBytes, bundleURL)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: read body: %v", ErrBundleFetch, err) | |
| } | |
| const maxBundleBytes = 10 << 20 // 10 MB — well above any valid Sigstore bundle | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxBundleBytes+1)) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: read body: %v", ErrBundleFetch, err) | |
| } | |
| if len(body) > maxBundleBytes { | |
| return nil, fmt.Errorf("%w: bundle exceeds %d bytes from %s", ErrBundleFetch, maxBundleBytes, bundleURL) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/dispatcher/internal/attestation/fetcher.go` around lines 61 - 64, Limit
the bundle response size before reading it in FetchBundle: wrap resp.Body with
io.LimitReader using a 10 MB cap (and account for an over-limit response), then
pass the limited reader to io.ReadAll and return an appropriate ErrBundleFetch
error when the limit is exceeded.
Comment on lines
+107
to
+125
| // isValidBeyond returns true when the entry is either unlimited (no end date) | ||
| // or its end date is on/after the given horizon. Entries whose start hasn't | ||
| // arrived yet are not considered "future-capable" — they cannot verify | ||
| // present-day bundles. | ||
| func isValidBeyond(t *testing.T, v validFor, horizon time.Time) bool { | ||
| t.Helper() | ||
| if v.End == "" { | ||
| return true | ||
| } | ||
| end, err := time.Parse(time.RFC3339, v.End) | ||
| if err != nil { | ||
| // Some sigstore roots use millisecond precision. Try that too. | ||
| end, err = time.Parse("2006-01-02T15:04:05.999Z07:00", v.End) | ||
| if err != nil { | ||
| t.Fatalf("cannot parse validFor.end %q: %v", v.End, err) | ||
| } | ||
| } | ||
| return !end.Before(horizon) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
isValidBeyond doesn't check the start date despite documenting it should.
The comment states "Entries whose start hasn't arrived yet are not considered 'future-capable' — they cannot verify present-day bundles," but the implementation only checks v.End. During a Sigstore root rotation where the old authority has expired and the replacement has a future start with no end, this test would incorrectly pass, allowing a root that can't verify present-day bundles to ship.
🔧 Proposed fix: add start-date check to isValidBeyond
func isValidBeyond(t *testing.T, v validFor, horizon time.Time) bool {
- t.Helper()
- if v.End == "" {
- return true
- }
- end, err := time.Parse(time.RFC3339, v.End)
- if err != nil {
- // Some sigstore roots use millisecond precision. Try that too.
- end, err = time.Parse("2006-01-02T15:04:05.999Z07:00", v.End)
- if err != nil {
- t.Fatalf("cannot parse validFor.end %q: %v", v.End, err)
- }
- }
- return !end.Before(horizon)
+ t.Helper()
+ if v.Start != "" {
+ start, err := time.Parse(time.RFC3339, v.Start)
+ if err != nil {
+ start, err = time.Parse("2006-01-02T15:04:05.999Z07:00", v.Start)
+ if err != nil {
+ t.Fatalf("cannot parse validFor.start %q: %v", v.Start, err)
+ }
+ }
+ if start.After(horizon.Add(-trustedRootWarningHorizon)) {
+ return false
+ }
+ }
+ if v.End == "" {
+ return true
+ }
+ end, err := time.Parse(time.RFC3339, v.End)
+ if err != nil {
+ // Some sigstore roots use millisecond precision. Try that too.
+ end, err = time.Parse("2006-01-02T15:04:05.999Z07:00", v.End)
+ if err != nil {
+ t.Fatalf("cannot parse validFor.end %q: %v", v.End, err)
+ }
+ }
+ return !end.Before(horizon)
}And update the call site at line 77 to pass now (recoverable as horizon.Add(-trustedRootWarningHorizon) or by adding a now parameter):
- if isValidBeyond(t, entry, horizon) {
+ if isValidBeyond(t, entry, now, horizon) {With the updated signature:
-func isValidBeyond(t *testing.T, v validFor, horizon time.Time) bool {
+func isValidBeyond(t *testing.T, v validFor, now, horizon time.Time) bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/dispatcher/internal/attestation/trusted_root_test.go` around lines 107 -
125, Update isValidBeyond to parse and require validFor.start to be at or before
the evaluation time, while preserving the existing end-date and unlimited-end
handling. Adjust its call site to provide the current time separately from the
horizon (or derive it as horizon.Add(-trustedRootWarningHorizon)), and ensure
both timestamp precisions are supported consistently.
…rization Address Fable 5 review on PR #1670: - verifier: add verify.WithSignedCertificateTimestamps(1) so a Fulcio-CA-issued cert that skipped CT logs is rejected instead of accepted. TransparencyLog + IntegratedTimestamps alone did not cover this failure mode. - fetcher: stop attaching Authorization to release download URLs. GitHub's release download host is not API-rate-limited so the token buys nothing, and the http.Client strips it across the cross-host redirect to objects.githubusercontent.com anyway. Restrict the header to github.com/ghapi calls (fetchGitRef) for least privilege. - trusted_root_test: align the isValidBeyond comment with the actual gate. The guard only inspects End; the Sigstore public-good root has never shipped an entry with a future Start, so document that scope instead of promising a Start check the code does not perform.
This was referenced Jul 10, 2026
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
1 participant
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.
Summary
cli/dispatcher/internal/attestation/, a self-contained verifier package for GitHub Artifact Attestation bundles. Unused at this commit; runtime behavior unchanged.update.go) and B3 (dispatcher_download.go) will wire it in — keeping this PR narrow shrinks the review surface before the call sites change.What lands
internal/attestation/verifier.go:Verify(trustedMaterial, VerifyOptions) error, backed bysigstore-go v1.2.2. Enforces (a) local asset digest ∈ bundle subjects, (b) SAN in a closed allowlist (workflow@refs/heads/{v3-beta,main}), and (c) Fulcio OID 1.3.6.1.4.1.57264.1.13 == release tag's commit SHA — binding the attestation to a specific tree so a leaked OIDC token cannot be reused on a different tag.internal/attestation/fetcher.go:FetchBundle(<asset>.sigstore.jsonHTTP GET) andFetchTagCommitSHA(git-refs REST). Every non-2xx (incl. 403/429) isErrBundleFetch— never treated as "skip verification" or an attacker can strip it.GITHUB_TOKEN/GH_TOKENis forwarded when set.FetchTagCommitSHAfollows annotated-tag indirection.internal/attestation/trusted_root.json(embedded): Sigstore public-good root, fetched once via the newcmd/refresh-attestation-trusted-roothelper (drivessigstore-go's TUF client). Keeps verification offline and deterministic.internal/attestation/trusted_root_test.goCI guard: fails 90 days before any embedded Fulcio / Rekor / CT log / TSA authority expires, so a missed quarterly refresh surfaces as a red build, not a fleet-wide self-update outage after Rekor v2 or a Fulcio CA rotation. Refresh commits must befix(dispatcher):so release-please cuts a dispatcher release that actually ships the refreshed root.cmd/refresh-attestation-trusted-root/main.go: one-shot Go program that fetches the currenttrusted_root.jsonvia TUF and writes it underinternal/attestation/. Only compiled duringgo build ./...— never bundled into the released dispatcher binary becausescripts/build-go-cli.shbuilds./cmd/dispatcherexclusively.attestationalongsidedispatcher,install,nativepath,uninstall,update.Test plan
scripts/check-go-cli.sh— fmt/vet/lint/build/test all greengo test ./internal/attestation/...— 12 tests: happy / digest-mismatch / SAN-mismatch / commit-SHA-mismatch / malformed-bundle / 404 / 403 / 429 / empty-body / lightweight-tag / annotated-tag / server-errordispatcher-v3.0.1-beta.12arm64 bundle, so tests exercise the same bundle shape production callers will hand the verifier