Skip to content

Verify release attestation offline against published bundle asset - #421

Open
evandowning wants to merge 4 commits into
mainfrom
worktree-offline-attestation-verify
Open

Verify release attestation offline against published bundle asset#421
evandowning wants to merge 4 commits into
mainfrom
worktree-offline-attestation-verify

Conversation

@evandowning

@evandowning evandowning commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Problem

curl -fsSL …/install.sh | bash fails for users outside the trailofbits org:

Loading attestations from GitHub API failed. Error: HTTP 403: Resource protected by
organization SAML enforcement

install.sh and coop update both ran gh attestation verify … --repo trailofbits/coop,
which fetches the Sigstore bundle from the GitHub attestations API. gh always attaches its
stored credential and refuses to run without one. Once a token is attached, trailofbits SAML
enforcement rejects it unless it carries an SSO session for the org, so the request 403s even
though the data is public. The affected user cannot fix it if they have no SSO access. The
failure is unrelated to artifact integrity — the SHA256SUMS check already passed before
verification aborted.

The same code path also fails for anyone who has gh installed but is not logged in: gh
exits 4 ("please run gh auth login") and the install is refused outright. That is a larger
population than SAML users, and their only escape today is uninstalling gh, which silently
downgrades them to checksum-only. Coverage goes up here, not just error quality.

Fix

Publish the provenance bundle as a release asset (attestations.jsonl) and verify it
offline with gh attestation verify --bundle, which makes no API call and needs no
credential. Both install paths stop depending on the caller's GitHub identity.

  • .github/workflows/release.yml — give the attest step an id, normalize its bundle
    output to one-per-line with jq -c (runs on the runner only; not a user-facing dep), and
    publish attestations.jsonl alongside the tarballs and SHA256SUMS.
  • install.sh — download the bundle asset via the existing download_asset (gh → curl
    fallback) and pass --bundle. Both verification paths print a confirmation on success.
  • src/update.rs — same treatment for coop update: look up the attestations.jsonl
    asset in the release metadata, download it, and pass --bundle. The lookup is skipped when
    gh is absent or COOP_UPDATE_API_BASE_URL is overridden, so an update whose attestation
    step is a no-op does no pointless work.
  • Docs — README leads with the credential-free recipe; RELEASING.md adds the asset to
    the release checklist and pins VERSION in the credentials-stripped smoke test;
    docs/trust-model.md records that --bundle changes transport, not the guarantee.

Releases without the asset still install

Verification falls back to the API path — exactly what every release did before — when a
release publishes no bundle, or when the bundle download fails. Failing closed instead would
have broken the documented curl … | bash one-liner for every user between merge and the
next release (README serves install.sh from main, and latest is v0.5.4, which has no
asset), and would have broken VERSION=v0.5.x pinned installs permanently. So the bundle is
a strict improvement and never a regression.

install.sh and update.rs treat "no asset published" and "asset download failed" the same
way, because for the client they are the same situation: no bundle to read.

A bundle that downloads but fails to verify is refused, not retried through the API. To be
clear about what that does and does not buy: it is not stricter on integrity — a
substituted bundle fails --bundle and would then verify correctly against the genuine
attestation, and a tampered artifact fails both checks. The reason is diagnostic: a bundle
that downloaded and will not verify means a broken download or a gh that cannot read it,
and silently switching transports would hide both.

Verification strength is unchanged

This is not a softening of the update-verification chain. Verified against v0.5.4 with
credentials stripped (GH_CONFIG_DIR empty, GH_TOKEN/GITHUB_TOKEN unset):

Check Result
correct artifact + correct bundle, zero credentials exit 0
multi-subject bundle (what the release actually produces) exit 0
tampered tarball (one byte appended) exit 1 — integrity enforced
wrong --repo (trailofbits/not-coop) exit 1 — identity enforced
corrupt/truncated bundle exit 1 — install refused (see above for why not retried)
raw API JSON passed to --bundle exit 1

The bundle is signed and verified against Sigstore's trust root plus the --repo identity
constraint, so fetching it over an unauthenticated download cannot weaken the guarantee — a
substituted or tampered bundle fails. Only the transport of an already-signed object changes.

Review follow-ups (fd28783)

  • install.sh failure messaging — the credential explanation is now gated on gh
    actually reporting a 403 / SAML / gh auth login symptom. Previously a network error, a
    gh too old for the command, or a genuine provenance mismatch was all described as an SSO
    problem and pointed the user at a different release. Exercised with a stubbed gh across
    six outcomes: the explanation appears for the two credential cases and for neither the
    network error nor the "no matching attestations found" mismatch.
  • Symmetric fallback — a failed bundle download in update.rs falls back to the API
    instead of failing the update, matching install.sh. fetch_attestation_bundle is now
    infallible and returns Option<PathBuf> directly.
  • Tripwire hardening — the asset-name test asserts on the gh release create line, not
    just on the file containing the string. Confirmed it now fails when the asset is dropped
    from publication while the jq step still creates it; before, that regression stayed green.
  • release.yml step nameNormalize attestation bundle, which is what the step does;
    publishing happens in gh release create.
  • Deferred: making the API fallback itself credential-free (so pre-bundle releases also
    verify with no credential) is filed as Make the attestation-API fallback credential-free for pre-bundle releases #423 rather than grown into this PR.

Testing

  • shellcheck install.sh, bash -n install.sh — clean. actionlint reports only the
    pre-existing SC2086 on main in the unrelated Package step.
  • cargo fmt --check, cargo clippy --all-targets --all-features -- -D warnings,
    cargo test (1012 passed) — clean. ./tests/integration-update.sh — 5 passed.
  • End-to-end install.sh with zero credentials against a v0.5.4 release with the bundle
    asset shimmed in: installs cleanly. Corrupt bundle: refuses. No bundle: falls back to the
    API and reports the SSO cause on failure.
  • New unit tests pin the gh attestation verify argument list (with and without --bundle)
    and guard the attestations.jsonl name against drift between release.yml, install.sh,
    and update.rs. Both were confirmed to fail when the behavior is reverted.

Stage 2 (a real release carrying the asset) can only run after the next release publishes
attestations.jsonl.

🤖 Generated with Claude Code

install.sh ran `gh attestation verify --repo`, which fetches the Sigstore
bundle from the GitHub attestations API. `gh` always attaches its stored
credential, so a token without an SSO session for the trailofbits org 403s
on public data — external users cannot install even though the artifact is
public and its SHA256SUMS check already passed.

Publish the provenance bundle as a release asset (attestations.jsonl) and
verify it offline with `gh attestation verify --bundle`, which makes no API
call and needs no credential. The --repo identity constraint and the artifact
digest match are still enforced, so transporting the already-signed bundle
over an unauthenticated download does not weaken the guarantee.

- release.yml: give the attest step an id, normalize its bundle output to
  one-per-line with `jq -c`, and publish attestations.jsonl with the release.
- install.sh: download the bundle asset and pass --bundle; releases before
  the asset existed fail closed with a clear message.
- README.md / RELEASING.md: document the offline path and the credential-free
  manual recipe; strip credentials in the release smoke test.

src/update.rs has the identical bug (coop update shells out to the same
API-fetching verify) and needs the same fix in a follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evandowning evandowning self-assigned this Jul 30, 2026
Review of #421 found two problems with the installer change.

install.sh failed closed when a release published no attestations.jsonl.
Since README serves the installer from main, merging would have broken
`curl … | bash` for every user until the next release: latest is v0.5.4,
which has no bundle asset, and the error told users to "install a newer
version" that does not exist. It also broke `VERSION=v0.5.x` pinned
installs permanently. Fall back to the API path in that case instead —
exactly what every release did before — so the bundle is a strict
improvement and never a regression. A bundle that fails to verify still
refuses the install; only a missing bundle falls back.

src/update.rs had the same API-only verification, so `coop update` still
403'd for the users this fixes. It now downloads the release's
attestations.jsonl and passes --bundle, with the same fallback. The
bundle is skipped when gh is absent or the API base is overridden, so a
fallible download cannot fail an update whose attestation step is a
no-op anyway.

Also: install.sh's no-gh hint and update.rs's equivalent advised the
credential-requiring recipe; both now show --bundle. README leads with
the offline recipe, RELEASING.md pins VERSION in the smoke test, and
docs/trust-model.md records that --bundle changes transport, not the
guarantee — --repo still pins signer identity and the bundle is signed.

Verified against v0.5.4 with GH_CONFIG_DIR empty and GH_TOKEN/GITHUB_TOKEN
unset: offline bundle path installs (exit 0), corrupt bundle refuses
(exit 1), missing bundle falls back to the API, and a multi-subject
bundle — which is what the release actually produces — verifies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@hbrodin hbrodin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down — the diagnosis is right and this is a genuinely useful catch. I'm looking into the bigger picture around this separately.

I verified the central claims independently rather than taking the description on faith, against the real v0.5.4 release with gh 2.93.0:

  • The attestations API is readable anonymously (200, 2 attestations). It really is only gh attaching its token that converts public data into a 403 — same class as cli/cli#6675. I also confirmed the obvious workaround does not exist: stripping the token and running the API path exits 4, so --bundle is the only credential-free route.
  • Credential-free --bundle verification works: correct artifact → exit 0 with GH_CONFIG_DIR empty and both token vars unset. Negative controls all fail closed — tampered tarball → 1, wrong --repo → 1, garbage bundle → 1, empty bundle → 1, raw API JSON as --bundle → 1.
  • So the guarantee is unchanged: Sigstore trust root, --repo signer identity, and subject-digest match are all still enforced offline. Agreed this is not a softening of the chain.
  • bundle-path is declared at the exact pinned action SHA, and --bundle documents and accepts JSON Lines — so the jq -c normalization is sound whichever shape the action emits.

One thing the description undersells in the other direction: this fixes a second, independent failure. On main, anyone with gh installed but not logged in gets exit 4 ("please run gh auth login") → die → the install is refused outright. That's a larger population than SAML users, and today their only escape is uninstalling gh, which silently downgrades them to checksum-only. Coverage genuinely goes up here.

Comments inline. The only one I'd want resolved before merge is the failure messaging in install.sh. Worth stating plainly though: the fix is latent — install.sh is served from main and latest is v0.5.4 with no asset, so the reported symptom persists for every user until the next release ships, and permanently for VERSION=v0.5.x pinned installs.

Comment thread install.sh Outdated
Comment thread src/update.rs Outdated
Comment thread src/update.rs Outdated
Comment thread src/update.rs Outdated
Comment thread install.sh
Comment thread .github/workflows/release.yml Outdated
Comment thread install.sh
- install.sh: gate the credential explanation on gh actually reporting a
  403 / SAML / "gh auth login" symptom. A network error, a gh too old for
  the command, or a genuine provenance mismatch previously got described
  as an SSO problem and pointed the user at a different release.
- install.sh: confirm a successful verify on both paths — the offline
  bundle path printed nothing, which read the same as a skip.
- update.rs: a failed bundle download now falls back to the API instead
  of failing the update, matching install.sh. One policy, one behavior.
  fetch_attestation_bundle is infallible, so it returns Option directly.
- update.rs: the asset-name tripwire now asserts on the `gh release
  create` line. Dropping the asset from publication alone left the test
  green while the asset silently stopped shipping.
- install.sh: record why a bundle that fails to verify is refused rather
  than retried — it is not stricter on integrity, it surfaces a broken
  download or an unusable gh.
- Docs: trust-model records the failed-download fallback and the
  refuse-don't-retry rule; RELEASING checks for the positive confirmation
  line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@evandowning

Copy link
Copy Markdown
Contributor Author

Thanks for the independent verification — especially the negative controls and the bundle-path-at-pinned-SHA check. All seven inline comments are addressed in fd28783 and the threads are resolved. Branch also merged up to main (b3da317, the proxy work) in 19a35c3.

Changed

  • Failure messaging (install.sh) — the blocker. gh output is captured, replayed to stderr unconditionally, and the credential explanation is gated on 403 / SAML / gh auth login. Verified with a stubbed gh: the explanation fires for the 403 and the exit-4 no-credential case, and stays silent for a DNS failure and for no matching attestations found. A real provenance mismatch is no longer described as an SSO problem.
  • Symmetric fallback (update.rs) — a failed bundle download falls back to the API instead of failing the update, matching install.sh. fetch_attestation_bundle had no fallible path left, so it returns Option<PathBuf> rather than Result<Option<…>>.
  • Tripwire hardening — the asset-name test asserts on the gh release create line. Confirmed it fails on the exact regression you described (asset dropped from publication while the jq step still creates it); the old assertion stayed green on that edit.
  • Success confirmation — both paths now print one, so the offline path is no longer indistinguishable from a skip. RELEASING.md checks for that positive line instead of the absence of a negative one.
  • Step nameNormalize attestation bundle.
  • Rationale correction — you were right that "refusing is stricter" does not hold. Removed from the description and replaced at the call site with the actual reason (it surfaces a broken download or an unusable gh; it buys nothing on integrity). docs/trust-model.md now states both the failed-download fallback and the refuse-don't-retry rule.

Deferred to #423 — making the API fallback itself credential-free, with your verified curl | jq -c | --bundle recipe. That is the part that reaches latest and VERSION=v0.5.x pinned installs, so it is the real fix for the latency you flagged; kept separate because it changes fallback transport rather than the bundle mechanism.

Gates on the merged tree: shellcheck, bash -n, cargo fmt --check, cargo clippy --all-targets --all-features -D warnings clean; cargo test 1061 passed; ./tests/integration-update.sh 5 passed. src/update.rs is in exclude_globs in .cargo/mutants.toml, so no mutation-scope change is needed for the new code.

@evandowning
evandowning requested a review from hbrodin July 30, 2026 18:40
Comment thread src/update.rs
"release.yml no longer publishes {BUNDLE_ASSET} as a release asset"
);
assert!(
installer.contains(BUNDLE_ASSET),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion cannot detect what its message claims. attestations.jsonl appears as a literal in install.sh exactly once — line 13, BUNDLE="attestations.jsonl" — and every use site goes through ${BUNDLE}. So installer.contains(BUNDLE_ASSET) is satisfied by the variable declaration alone: delete the download_asset "$BUNDLE" "${TMPDIR}/${BUNDLE}" probe and the whole --bundle branch, leave the now-dead assignment behind, and this still passes while asserting "install.sh no longer downloads attestations.jsonl". It would also pass with the name mentioned only in the no-gh help text.

The workflow half was tightened in the previous round to require gh release create on the same line; the installer half needs the equivalent — a line containing both download_asset and BUNDLE, or both gh attestation verify and --bundle — so the tripwire keys on the behavior rather than on a string that survives the behavior's removal.

- name: Normalize attestation bundle
env:
BUNDLE_PATH: ${{ steps.attest.outputs.bundle-path }}
run: jq -c . "$BUNDLE_PATH" > attestations.jsonl

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jq -c . exits 0 and writes zero bytes when its input is empty or whitespace-only (confirmed: : > empty.json; jq -c . empty.json → exit 0, 0 bytes), so a degenerate bundle-path yields a green step and a 0-byte attestations.jsonl release asset.

That is worse for clients than the asset being absent, because neither client checks the bundle it got. install.sh's probe succeeds (the file downloads fine), gh attestation verify --bundle <empty> then fails, and the deliberate no-fallback rule fires — die "... refusing to install". coop update fails identically via fetch_attestation_bundle returning Some(dest) on any Ok(()). Every gh-equipped install of that release is blocked with no API fallback, and the version cannot be re-cut. (On gh older than 2.56.0 it fails the other way: cli/cli#9541 fixed empty-bundle handling, and before that an empty JSONL reported success — so the installer would print "Attestation verified offline" having verified nothing.)

The release-notes step three below already models the guard: if [ ! -s RELEASE_NOTES.md ]; then ... exit 1; fi. Suggest jq -c . "$BUNDLE_PATH" > attestations.jsonl && [ -s attestations.jsonl ], and/or a non-empty check before passing --bundle in both clients.

Comment thread install.sh
# Releases published before the bundle asset existed verify through the
# API, exactly as every release did before. That needs a credential
# authorized for the org, so it is the fallback and not the default.
info "No ${BUNDLE} published for ${VERSION} — verifying through the GitHub API instead."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line asserts as fact something the code has just discarded. It runs on every non-zero exit of the silenced probe download_asset "$BUNDLE" "${TMPDIR}/${BUNDLE}" > /dev/null 2>&1, and download_asset returns non-zero for a 404 and for DNS/TLS failure, a connection reset, a 5xx, a gh release download auth failure, a curl partial transfer, or an unwritable $TMPDIR — with 2>&1 throwing away the evidence that would tell them apart.

On a current release plus a transient blip, the user is told the release publishes no provenance bundle, verification silently moves to the credential-requiring transport, and if their token has no SSO session the gate below then prints ${VERSION} predates the ${BUNDLE} asset — flatly false for that tag. The comment block above this line makes the same claim ("Releases published before the bundle asset existed verify through the API").

src/update.rs keeps the two apart deliberately, with a tracing::info! for the missing asset and a tracing::warn! carrying ({err:#}) for a failed download — so the installer is the weaker of the two, and RELEASING.md:110 builds a release-acceptance check directly on this wording. Suggest stating what was observed: Could not fetch ${BUNDLE} for ${VERSION} (not published, or the download failed) — verifying through the GitHub API instead.

Comment thread src/update.rs
.args(attestation_verify_args(tarball, bundle))
.run()
.with_context(|| {
let hint = if bundle.is_some() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch_attestation_bundle collapses four distinct outcomes into Option<PathBuf> — skipped (test mode / no gh), asset not published, download failed, bundle present — and this hint then re-derives a reason from the surviving None, asserting one that can be false.

On the download-failure path (added last round to resolve the ?-aborts-the-update thread) the release does publish attestations.jsonl, yet the error tells the user "the release publishes no attestations.jsonl" — contradicting the tracing::warn!("Failed to download {BUNDLE_ASSET} ...") emitted moments earlier in the same run. The distinction is known at the if let Err(err) above and thrown away before it is needed here.

Returning the decision once as an enum (Offline(PathBuf) / ApiFallback(reason) / Skipped) makes the wrong reason unrepresentable, carries the reason fetch_attestation_bundle already computed for its info!/warn!, and drops the replicated api_base_overridden() || !command_exists("gh") guard that currently duplicates this function's own two skip checks by convention only. It would also make the branch testable — it is pure string composition and untested in both directions today, and it is the only place a user is told a 403 means a missing SSO session rather than a provenance failure.

Comment thread docs/trust-model.md
floor.
4. **Best-effort attestation.** `gh attestation verify --repo trailofbits/coop
--bundle attestations.jsonl` (Sigstore provenance), against the bundle asset
downloaded from the same release. `--bundle` makes verification offline: no

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two corrections to this paragraph, in the document that is the checklist future reviewers apply.

"offline" overstates what --bundle does. The no-credential half is right — gh marks the bundle flag with cmdutil.DisableAuthCheckFlag, so no token or gh auth login is needed, true since gh v2.49.0. But --bundle does not make the command network-free: with no --custom-trusted-root, NewLiveSigstoreVerifier builds TUF clients against https://tuf-repo.github.com and the Sigstore public-good CDN with CacheValidity = 1 (one day), and fails with "no valid Sigstore verifiers could be initialized" if neither can be reached. There is no --offline flag; GitHub's own offline-verification doc requires supplying the root out of band via gh attestation trusted-root + --custom-trusted-root. Narrowing the wording to "no attestations-API call, no credential" is accurate and is already enough to justify the change. The same overstatement is in README.md:134-136, install.sh:141, src/update.rs:391-392, and RELEASING.md:100/109.

--repo does not pin the signer. It pins the certificate's source repository: any workflow on any ref in trailofbits/coop holding id-token: write + attestations: write mints a bundle that satisfies --repo trailofbits/coop; gh exposes --signer-workflow / --cert-identity for the actual pin, and neither attestation_verify_args nor install.sh passes one. What defeats a substituted bundle attesting a different artifact is the subject-digest binding (gh digests the artifact and requires a matching subject), not --repo. The "does not weaken the check" conclusion still holds — the API path is keyed by digest against the same repo-scoped store and is equally unpinned as to workflow — but since this diff newly moves the signer material into a release asset, recording the real pin boundary here is worth doing.

Refs: https://cli.github.com/manual/gh_attestation_verify · https://docs.github.com/en/actions/security-for-github-actions/using-artifact-attestations/verifying-attestations-offline

Comment thread README.md
Both `install.sh` and `coop update` run this verification automatically when
the [GitHub CLI](https://cli.github.com/) is installed. Both verify offline
against the `attestations.jsonl` bundle published with the release, so
verification makes no GitHub API call and needs no authentication. Releases

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This over-claims relative to docs/trust-model.md:218, which states it precisely ("no attestations-API call"). Verification as a whole does reach GitHub, with a credential when one exists, to obtain the bundle: install.sh's download_asset tries gh release download "$VERSION" --repo "$REPO" ... first and only curls on failure, and coop update routes the bundle through download_assetselect_auth_strategy_from_env(), i.e. AuthStrategy::Gh or AuthStrategy::CurlBearer(token) when GITHUB_TOKEN is set — on top of the api.github.com release-metadata fetch it already performs. Only the gh attestation verify step is credential-free.

That matters for exactly the SSO-less user this PR targets, whose gh release download of the bundle can itself 403. Aligning the README with the trust-model wording keeps the security spec and the user-facing doc from drifting.

Comment thread src/update.rs
/// resolves it the same way. The download is also skipped when
/// `verify_attestation` would not read the result, so an update whose
/// attestation step is a no-op does no pointless work.
fn fetch_attestation_bundle(release: &Release, dir: &Path) -> Option<PathBuf> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only genuinely new decision logic in the diff — five outcomes (API-base override, gh absent, asset not published, download failed, success) — and it has no coverage of any kind. No test in mod tests references it or verify_attestation; it is unreachable from tests/integration-update.sh, which exports COOP_UPDATE_API_BASE_URL and so short-circuits the function on its first line; and src/update.rs is whole-module excluded in .cargo/mutants.toml exclude_globs, so no mutation sweep reaches it either. The || on the next line could become &&, or either return None could become Some(dest), and nothing anywhere fails.

It is untestable as written — it probes env, probes $PATH, and shells out. The seam is the one this PR already applied to argv construction one function up: split the decision from the IO, e.g. fn bundle_decision(release, api_overridden, gh_present) -> BundleDecision returning Skip / NoAsset / Fetch(&Asset), unit-tested over all three, leaving fetch_attestation_bundle as the thin download wrapper. That composes with the enum suggested on the hint below, and would give bundle_asset_is_found_on_a_release_that_publishes_it something new to assert against — as written it only exercises the pre-existing Release::find_asset with a different string.

Comment thread install.sh
# The probe is silenced: a missing bundle is expected on older releases, so
# curl's bare "404" would read as a hard error.
if download_asset "$BUNDLE" "${TMPDIR}/${BUNDLE}" > /dev/null 2>&1; then
# A failure here is not retried through the API. That is no stricter on

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enumeration omits the security-relevant cause. gh attestation verify --bundle fails precisely when the tarball's digest does not match the bundle's signed subject — a tampered tarball published alongside a matching SHA256SUMS is caught here and nowhere else in the installer. Telling a future reader this failure "means a broken download or a gh that cannot read it" invites them to read the die as an availability nuisance and add the API retry the comment argues against.

Worth adding integrity failure to the list, or trimming the block to the one durable why: not retried through the API, because switching transports on a bundle that downloaded but will not verify would mask the failure.

Comment thread src/update.rs

/// Download the release's provenance bundle, if it published one.
///
/// `None` means verification falls back to the attestations API, or is skipped

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this records the path taken during review rather than the function's contract. "A download that blips is the same situation as a release that never published the asset, and install.sh resolves it the same way" is an argument for if let Err over ?, and it asserts a fact about a sibling shell script that nothing checks — if install.sh's fallback changes, this comment silently lies (the cross-implementation invariant already lives in docs/trust-model.md:220-222). The final sentence narrates the function's own next two lines and then defends them with a truism.

The first sentence plus "None means verification falls back to the attestations API, or is skipped outright — never that the update fails" is the whole contract a caller needs.

Comment thread src/update.rs
fn verify_attestation(tarball: &Path) -> Result<()> {
/// Build the `gh attestation verify` argument list.
///
/// With `bundle`, `gh` reads the Sigstore bundle from disk and makes no API

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fourth copy of the same SSO/403 rationale in the diff — it is also in install.sh:127-130, README.md:146-149, and docs/trust-model.md item 4. It also hangs on a pure argument-list builder that appends two strings: the paragraph describes gh's network behavior, not this function's.

That matters for drift more than for length, because "always attaches its stored token" is a claim about the current gh implementation; when it changes, four independent copies have to be found and corrected. Suggest keeping the substance in docs/trust-model.md as the system of record and reducing this to the first line plus a pointer. The five-line 403 troubleshooting block at README.md:146-149 — attached to the command the section tells readers not to use — is the one that adds least.

Comment thread RELEASING.md
```

The run must print `Attestation verified offline against attestations.jsonl`.
A "No `attestations.jsonl` published" line instead means the asset is missing

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnosis states one cause as the only cause. install.sh prints the "No attestations.jsonl published" line whenever the silenced probe download_asset "$BUNDLE" ... > /dev/null 2>&1 returns non-zero — a network blip, a TLS failure, a 5xx, or a gh+curl double failure all produce it exactly like a genuinely missing asset. install.sh's own comment concedes the probe cannot distinguish them.

A maintainer following this step after a transient failure would conclude the release asset is missing — a costly wrong conclusion given releases are immutable and the remedy is a version bump. Suggest: "means the bundle could not be downloaded — confirm the asset on the release page (step 9's first bullet) before concluding it is missing."

Comment thread install.sh
# when gh actually reported one of its symptoms.
case "$out" in
*403* | *SAML* | *"gh auth login"*)
info "${VERSION} predates the ${BUNDLE} asset, so verification used the GitHub API,"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three lines go to stdout — info() { printf ' %s\n' "$1"; } — while the captured gh output above them (printf '%s\n' "$out" >&2) and the die below them both go to stderr. For a curl … | bash installer, which is the documented usage, that separates the explanation of the failure from the failure itself whenever the two streams are redirected differently, and lets them interleave out of order on a terminal.

Suggest emitting the explanation on stderr too — a warn-style helper, or printf … >&2 — so the whole failure report stays on one stream.

subject-path: "coop-*.tar.gz"

# Normalize to one bundle per line so the published asset has a stable
# shape regardless of whether the action emits a single bundle or several.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stated rationale describes a shape the pinned action cannot emit, and omits the constraint that actually makes this step necessary.

At actions/attest-build-provenance v4.1.1 (a wrapper over actions/attest v4.1.1), the action writes bundle-path to <mkdtemp>/attestation.json as exactly one JSON.stringify(att.bundle) line, and multiple subject-path matches produce a single attestation referencing all three tarballs rather than several bundles — so "regardless of whether the action emits a single bundle or several" is a case that cannot occur, and the one-bundle-per-line normalization is a no-op on content. (A multi-bundle file would also have worked: gh uses any-match semantics across a .jsonl, erroring only when verifyCount == 0, and sigstore-go matches the artifact digest against every entry of statement.Subject.)

What the step does accomplish, unremarked, is giving the asset a .jsonl extension: gh dispatches on extension and rejects anything else with bundle file extension not supported, must be json or jsonl. The published asset name is therefore load-bearing for --bundle, and a future "simplification" to e.g. attestations.txt would break verification. Worth saying that instead — a plain cp would do the same work.

@hbrodin

hbrodin commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

13 findings posted inline.

Three claims I verified directly rather than reasoning about, since the review turns on them:

  • attestations.jsonl appears as a literal in install.sh exactly once — line 13, BUNDLE="attestations.jsonl". Every use site goes through ${BUNDLE}, so the new tripwire's installer.contains(BUNDLE_ASSET) is satisfied by the declaration alone and cannot fail as its message claims.
  • jq -c . on an empty input exits 0 and writes 0 bytes (: > empty.json; jq -c . empty.json → exit 0, 0 bytes), so the normalize step can publish an empty bundle green.
  • src/update.rs is whole-module excluded in .cargo/mutants.toml exclude_globs, which confirms both that no mutants.toml update is owed by this PR and that fetch_attestation_bundle has no mutation coverage either.

Out of diff, so not inline: docs/ARCHITECTURE.md:176-179 still describes the pre-PR asset set — "download the platform tarball + SHA256SUMS, verify the checksum (mandatory), verify Sigstore attestation via gh (best-effort)". The chain now downloads a third asset and the attestation step is no longer a bare gh attestation verify. The paragraph does defer to trust-model.md for the full chain, so it is a one-clause fix, but CLAUDE.md's cross-file-sync rule points at it.

No diff noise. shellcheck is clean on the post-change install.sh; the new workflow step omits shell: exactly like its siblings in the release job and uses the env:-indirection zizmor wants; the test module's #[expect(clippy::unwrap_used, …)] matches every other test module in the repo.

Checked and deliberately not raised: the #421 reference in the BUNDLE_ASSET doc comment (bare #NNN cross-references have clear precedent — #349, #411, #303, #147, anthropics/claude-code#8938); the include_str! note in the tripwire test (added at a reviewer's explicit request in the previous round); the bundle-vs-API strictness rationale at install.sh:134-138 as a design question (reviewer said they were not asking for a change — the inline comment there is scoped narrowly to the failure-cause enumeration omitting digest mismatch); and the credential-free-API-fallback idea, which was explicitly deferred to a follow-up.

All seven prior inline threads were confirmed genuinely addressed at 19a35c3, not just marked resolved.

Coverage: all eight agents ran — review-correctness, review-design, review-conventions, review-security (update-chain / subprocess / credential surfaces), review-api-usage (gh attestation verify, actions/attest-build-provenance v4.1.1, jq), review-tests, review-docs, review-comments. None skipped. 21 raw findings deduplicated to 13 after per-file validation against the post-change tree; 8 dropped as ungrounded, already-handled by repo convention, previously-resolved, or folded into a stronger neighbour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants