ci: make attestation-check actually verify instead of failing open (LAB-984) - #63
ci: make attestation-check actually verify instead of failing open (LAB-984)#6327Bslash6 wants to merge 3 commits into
Conversation
…AB-984) The Attestation Health Check reported green for weeks while verifying nothing. Three independent defects, all verified against the live repo: 1. `runs-on: cachekit` — the self-hosted ARC runner has no `gh` CLI (LAB-899). The tag lookup's `2>/dev/null || echo ""` swallowed exit 127, `TAG` came back empty, and the "No releases found, skipping" branch gated every verification step off. Run 30243800492 logged that line while `cachekit-core-v0.4.0` demonstrably existed. 2. `gh attestation list` is not a real subcommand (only `download`, `verify`, `trusted-root` exist), so both no-asset fallback branches called a command that cannot succeed — and releases here carry zero assets, so that was the only reachable path. 3. The SBOM predicate types were wrong. actions/attest-sbom emits `https://cyclonedx.org/bom` (unversioned); the file looked for `.../bom/v1.6` with an SPDX fallback. Neither matches anything, so the check could not have passed even with an artifact in hand. The job now runs on ubuntu-latest, for the same reason release.yml's publish job already does: the ARC pods lack both `gh` and reliable Sigstore egress, and `gh attestation verify` is the only tool that does Sigstore bundle verification — there is no github-script equivalent. API calls go through SHA-pinned actions/github-script; `gh` is confined to the one job only it can do. Release assets are no longer consulted. The attestation subject is the `.crate` release.yml passes to attest-build-provenance/attest-sbom, which is the identical file cargo publish uploads to crates.io — verified byte-identical by digest. crates.io is the canonical copy, so a tagged release whose crate never published now fails loudly rather than degrading to a skip. Also: getLatestRelease 404s only when the repo genuinely has no releases, so "nothing to verify" is unreachable via an API error; `--signer-workflow` pins which workflow was permitted to sign; `--format json --jq` puts the verified digest in the log, because gh prints its success banner only to a TTY and a passing verify was otherwise as silent as a skip; and the failure issue is deduped so a weekly red job cannot file 52 issues.
crates.io's API rejects a generic user agent: the first live dispatch run (30359037048) resolved cachekit-core-v0.4.0 correctly and then died on curl exit 22 / HTTP 403. Verified locally that the same URL returns 200 with a descriptive -A and that the bytes hash to the attested digest 6aba1513135a7b92a124ad6983f7e80e5f5c78c4f9c74384079efa3fbf491eab.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
…AB-984) Expert-panel review found four real defects in the first cut. 1. The 404 branch was still a fail-open. getLatestRelease returns the latest NON-draft, NON-prerelease release, so a repo whose releases are all drafts or prereleases 404s as well — and the old branch skipped, gated every verification off and exited 0. Flipping v0.4.0 to prerelease would have resurrected the exact bug this file was rewritten to remove. A 404 now consults listReleases and only a genuinely empty list may skip; drafts/prereleases present is a hard failure. 2. The release tag was interpolated straight into two `run:` shells via template position while every other value went through `env:`. Git refnames legally contain `$`, backticks and `(`, so a crafted tag would execute in a job holding GITHUB_TOKEN. Both steps now pass TAG via env, matching the convention pr-title-lint.yml and release.yml already document. The tag regex is also tightened from `.+` / `.*` to explicit character classes, since those groups build both a URL and a file path. 3. `--signer-workflow` alone did not pin what it appeared to. Verified: it is an unanchored prefix match on the certificate SAN — a truncated `.../release` with no `.yml` passes — so it pinned neither the ref nor the full filename, and an actor with push access could mint provenance from a modified release.yml on any branch. Both verifies now add `--source-ref refs/heads/main`, which is the claim actually worth making. Confirmed to pass on the real artifact and to fail with a clear error on a wrong ref. 4. Failure-issue dedupe keyed on a title prefix, so an open issue for one tag silenced a genuine new failure on the next. It is now a per-tag HTML marker matched on the body, so retitling or relabelling cannot break it. Also: added a never-cancel concurrency group (siblings all have one; two overlapping runs could both file an issue), and cut ~22 lines of post-mortem narration from the comments — that content lives in the commit history, where it cannot rot into a lie. Three factual errors in those comments are fixed: the claim that every `gh` call in the old file hit the same `|| echo ""` (only the first ever ran), the SPDX/CycloneDX fallback order (it tried SPDX first), and the asserted TTY mechanism behind gh's silence (replaced with the observation that a redirected passing verify prints nothing at all, which is the part that actually matters). Rejected, with reasons: splitting the job to drop `issues: write` from the verify shell (Actions has no step-level permissions, and the injection vector that made it reachable is now closed); asserting crates.io max_stable_version equals the verified tag (real coverage gap, but a different assertion than "verify the latest release" and it races the publish window — follow-up); and the 60-day scheduled-workflow auto-disable risk (real, out of scope here).
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 46 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 36 minutes. |
1 participant
Closes LAB-984.
The Attestation Health Check has reported green every week while verifying nothing. Three independent defects, each verified against the live repo rather than inferred:
1.
ghdoes not exist on the runner.runs-on: cachekitis the self-hosted ARC runner, which has noghCLI — a constraint this repo already documents onrelease.yml's "Assign release PR" step (LAB-899). The tag lookup's2>/dev/null || echo ""swallowed exit 127,TAGcame back empty, and theNo releases found, skippingbranchif:-gated every verification step off. Run 30243800492 logged that line whilecachekit-core-v0.4.0demonstrably existed.2.
gh attestation listis not a real subcommand. Onlydownload,verify, andtrusted-rootexist. Both no-asset fallback branches called a command that cannot succeed — and this repo publishes zero release assets (v0.4.0has an empty asset list), so that was the only reachable path. cachekit-py hit this identical phantom subcommand (root cause of py #126) and fixed it the same way: download the published artifact, thengh attestation verify.3. The SBOM predicate types matched nothing.
actions/attest-sbomemitshttps://cyclonedx.org/bom(unversioned). The file triedhttps://spdx.dev/Document/v2.3withhttps://cyclonedx.org/bom/v1.6as an||fallback. Verified against the real attestation: both of those fail, only the unversioned string passes. So the SBOM check could not have passed even with an artifact in hand.What changed
runs-on: ubuntu-latest, for the same reasonrelease.yml's publish job already is: the ARC pods lack bothghand reliable Sigstore (Fulcio/Rekor) egress.gh attestation verifyis the only tool that performs Sigstore bundle verification — there is nogithub-scriptequivalent — soghis confined to exactly that, and every API call goes through SHA-pinnedactions/github-script..craterelease.ymlhands toattest-build-provenance/attest-sbom, which is the identical filecargo publishuploads to crates.io — confirmed byte-identical by digest (6aba1513…91eab). crates.io is the canonical copy, so the "missing.crateasset" branch dissolves rather than needing a documented degrade. A tagged release whose crate never published now fails loudly.getLatestReleaseconsultslistReleasesand only a genuinely empty list may skip. An unparseable tag is a hard failure.curl -fwith no|| true.--signer-workflowis an unanchored prefix match on the certificate SAN — verified: a truncated.../releasewith no.ymlalso passes — so alone it pins neither the ref nor the filename.--source-ref refs/heads/mainis what asserts "built by the release pipeline on main".--format json --jqputs the verified digest, predicate and signer URI in the log. Not cosmetic: with output redirected, a passinggh attestation verifyprints nothing at all and exits 0 — indistinguishable in a log from the skip this job used to do. That ambiguity is what let this rot unnoticed.-Aon the crates.io fetch is mandatory, not politeness — the API answers 403 to a generic user agent.Expert-panel review
Ran the mandatory panel gate (bug-hunter, security-specialist, code-craftsman, catchphrase) at high stakes. It found four real defects in the first cut, all fixed in
54995bf:getLatestReleasereturns the latest non-draft, non-prerelease release, so a repo whose releases are all drafts/prereleases 404s too — and the branch skipped, gated verification off, exited 0. Flipping v0.4.0 to prerelease would have resurrected the exact bug this PR exists to kill. All four agents caught this independently.run:shells via template position while every other value went throughenv:. Git refnames legally contain$, backticks and(. Both now useenv:, and the tag regex is tightened from.+/.*to explicit character classes since those groups build a URL and a file path.--signer-workflowdid not pin what it appeared to — see above;--source-refadded.Also applied: never-cancel
concurrencygroup (all sibling workflows have one), and ~22 lines of post-mortem narration cut from the comments — that belongs in commit history, where it cannot rot into a lie. Three factual errors in my own comments were corrected in the process.Rejected, with reasons. Splitting the job to drop
issues: writefrom the verify shell — Actions has no step-level permissions, so this costs a second job plus output plumbing for residual risk whose only vector is now closed. Asserting crates.iomax_stable_versionequals the verified tag — a real coverage gap, but a different assertion than "verify the latest release", and it races the publish window; worth its own ticket. The 60-day scheduled-workflow auto-disable risk — real and the same invisible-no-op class, but out of scope here.One panel claim was not applied because it was wrong: that gh writes its banner to stderr on non-TTY runs and
--format jsonis what suppresses it. Direct test contradicts it — a plaingh attestation verifywith output redirected produces an empty file and exit 0. The comment was reworded to state that observation instead of my originally-asserted TTY mechanism, which was the genuinely unsupported part.Verified live
Positive — run 30360190050, green, with real work in the log:
Negative — run 30359248927 on a throwaway branch with a deliberately wrong predicate type: job red at
Verify provenance attestation, and the dedupe branch logged that a tracking issue was already open. Branch deleted.Both branches of the alert path are live-verified: issue creation fired in run 30359037048 (filed #62, closed as a deliberate control), and dedupe fired in the negative run. That step had never once executed before this PR, because the job had never been capable of failing.
Also verified out-of-band, against the real
v0.4.0artifact: wrong predicate type, wrong signer workflow, wrong--source-ref, and a byte-appended artifact each exit 1.Docs
No docs needed. No
.mdin this repo (README,SECURITY.md,fuzz/) mentions attestation, SBOM, or provenance, and nothing incachekit-io/docsor the protocol spec describes this health check. The workflow file is the documentation surface, and it now carries in-file rationale for every non-obvious choice — runner, artifact source, predicate string, the two-flag signer pin, and why the log must print the digest.Out of scope
security.yml's SBOM release-asset upload is LAB-983 — different job, different workflow.Makefile's localsbomtarget has its own2>/dev/null ||fallback; noted, not touched.