Skip to content

feat(ci): verify build-provenance attestations in repro-check - #11624

Draft
basvandijk wants to merge 12 commits into
masterfrom
bas/repro-check-verify-attestations
Draft

basvandijk wants to merge 12 commits into
masterfrom
bas/repro-check-verify-attestations

Conversation

@basvandijk

@basvandijk basvandijk commented Sep 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

ci/scripts/repro-check rebuilds the IC-OS images and compares them with the ones the CDN serves. When they differ it reports a mismatch, but it cannot say which of two very different things happened: the build is not bit-reproducible (common, benign, "re-run it"), or the CDN bytes were substituted (never observed, a security incident). Because the first cause dominates the prior, a real alarm is likely to be discounted as flakiness. The mismatch is also raised only after the full download and rebuild: 23 to 28 minutes for the CI repro-check job (Build IC alone about 15), longer on a laptop.

PRs #11323 and #11569 made CI attest every file it uploads. Checking those attestations splits the alarm at the point where it is raised:

  • attestation verified + local mismatch → reproducibility bug in our own build; an engineering P1, not a breach.
  • attestation not verified → the CDN is not serving what CI built for this commit, or nobody can show that it is; the run stops before the build.

What this does not claim

A reproducible build strictly subsumes provenance for the artifact→source binding: it does not require trusting GitHub's runners at all, and it catches two threats an attestation does not (a compromised runner injecting into the artifact, and a non-hermetic build input differing between CI and source). No threat is caught only by the attestation. Its value is the disambiguation above, plus failing in seconds instead of after the rebuild whenever the provenance cannot be established, including when the substituted bytes carry no attestation at all.

Failure policy

The provenance check is a strict precondition of the build:

Outcome Behaviour
Every CDN checksum file's attestation verifies (signer, pipeline, ref and subject bound on one entry) Continue to the local build
Anything else: a mismatching attestation, none on GitHub (404), the API or Sigstore unreachable, the rate limit exhausted, a gh bootstrap failure, a gh error of any wording, an unreadable gh JSON shape, an invalid commit id Abort, exit 1, before the build is submitted
--skip-attestation-check given Warn (at the start and again at the end) and continue to the local build; a mismatch report then says the provenance was not verified

An attestation that cannot be obtained is indistinguishable, from the verifier's seat, from a substituted file nobody attested: the lookup is keyed by digest. Earlier revisions of this PR warned and continued in that case, on the argument that a check redundant with the rebuild must never block a verifier. Treating the two differently was the unsafe half of that design, and also the complicated half: it needed one exception class, one catch site, one warning accumulator and three mechanisms whose only job was to choose warn versus abort (a positive allowlist of gh stderr strings, a pinned-gh retry, a per-directory degrade-and-continue loop). All of them are gone. fetch-attested-sums.sh, the CI-side twin, already aborted on everything.

The check runs before submit(build_locally): storage()'s finally joins running futures, so a failure raised after that point is logged in seconds but exits only once the build is done.

Cost of the strict policy. The IDX-side defects listed under Notes for reviewers (a failed Attest step, the cancel-in-progress race, an undisclosed ic-private hotfix) now redden a good release until they are fixed, and an external verifier behind a shared address can exhaust the anonymous 60 requests/hour/IP quota (a run needs at most eight calls; the README says to set GH_TOKEN). The abort for a missing attestation deliberately does not mention --skip-attestation-check, and a test guards that; --help and the README document the flag as what DFINITY's CI passes on dfinity/ic-private.

Mechanism

gh attestation verify normally needs a GitHub login, which external proposal verifiers do not have. The public attestations REST endpoint does not: repro-check GETs the Sigstore bundles from it (following the Link cursor, capped) and passes them to gh attestation verify --bundle, which needs no token at all. gh is always the checksum-pinned 2.98.0, downloaded into the run's temp directory and cached per commit; a gh installed on the machine is not used.

Only the SHA256SUMS files are attestation-verified; the images are already bound to them by sha256. launch-measurements.json never was, so it is now checked against SHA256SUMS too. A cached image is reused only after a HEAD request confirms that the CDN still serves the same bytes; otherwise it is downloaded again.

The policy mirrors ci/scripts/fetch-attested-sums.sh byte for byte — signer ci-main.yml, --source-digest pinned to the commit, and the build pipeline, source ref and subject name all bound on one attestation entry. Proposal mode (-p) accepts a release-testing.yml build of rc--*/hotfix-*; commit mode (-c) also accepts ci-kickoff.yml on master.

Verification

90 unit tests (was 4), green under Python 3.12 (bazel) and Python 3.10 (the Ubuntu 22.04 audience). Live, anonymously, with no token:

  • rc tip 2967c1cc9b, which carries both an rc and a master attestation for the same digest: all four checksum files attested via the release-testing entry, through the pinned gh download.
  • master-only d91d72d773: rejected in proposal mode, accepted in commit mode.
  • pre-attestation 79c01052b5: 404 → abort in seconds; the message carries the advice and not the flag. With --skip-attestation-check the run continues to the build.
  • cross-directory subject and a one-character-flipped digest: both rejected.

gh's verbatim failure strings for a wrong --source-digest, a tampered file, a Sigstore setup failure and an empty bundle are kept in the tests as documentation of what a verifier will see. Every one of them aborts; the message quotes gh and gives both readings, since gh has no exit code separating "failed to verify" from "could not be set up".

Notes for reviewers

  • .github/workflows/repro-check.yml now runs the sparse checkout the PR step already used instead of piping the script from curl: that pipeline ran in a bash -c without pipefail, so a failed fetch fed python3 - an empty program and the job passed having verified nothing, a false green that this PR turned from latent into a hole in the fail-closed check. A fetch failure now fails the job. The step also gains GH_TOKEN: ${{ github.token }} (load-bearing: a rate-limited API fails the job; no new permission) and, via case(github.repository != 'dfinity/ic', '--skip-attestation-check', ''), the flag on the private mirror only. github.repository is the caller's repository inside a reusable workflow (ci-main.yml relies on the same), and dfinity/ic-private mints no attestations, since actions/attest in ci-main.yml is gated on the public repo. On dfinity/ic the check cannot be disabled from CI; a commit whose attestations predate the signer change of chore(CI): attest uploaded artifacts in the job that uploads them #11569 is verified by dispatching the workflow on that commit's own ref, which checks out its own contemporaneous script.
  • Behaviour change on dfinity/ic-private: the job used to be a false green for private-only hotfix commits, because the hardcoded raw.githubusercontent.com/dfinity/ic/<sha> 404s and the unguarded bash -c exited 0, so it passed in ~54 s having verified nothing (ic-private runs 33126787730 and 30104328970, against 0h 25m 57s for the equivalent public run). With the checkout the script runs there for those commits too, with the attestation check skipped, so the mirror's Release Testing now does a real repro check (about 25 minutes) and can fail on a reproducibility problem. Whether embargoed hotfixes should be repro-checked on the mirror at all is a question for IDX; this PR makes the job do what its name says.
  • Attestation flooding: ci-kickoff.yml grants attestations: write to same-repo PR and merge-group runs, so an insider could mint many attestations covering a legitimate digest. Pagination with a cap handles it here; fetch-attested-sums.sh shares the exposure and has no rebuild behind it. Producer-side follow-up for IDX: grant attestations: write only to release builds.
  • rc--*/hotfix-* refs carry no rulesets, so the ref pin proves which pipeline ran, not that a release was qualified. The wording avoids "qualified" throughout.

🤖 Generated with Claude Code

## Problem

`ci/scripts/repro-check` rebuilds the IC-OS images and compares them with the
ones the CDN serves. When they differ it reports a mismatch, but it cannot say
which of two very different things happened: the build is not bit-reproducible
(common, benign, "re-run it"), or the CDN bytes were substituted (never
observed, a security incident). Because the first cause dominates the prior, a
real alarm is likely to be discounted as flakiness. The mismatch also costs a
~6 hour, 100 GB build before it is raised at all.

PRs #11323 and #11569 made CI attest every file it uploads. Checking those
attestations splits the alarm at the point where it is raised:

- attestation OK + local mismatch -> reproducibility bug; an engineering P1.
- attestation mismatch            -> the CDN is not serving what CI built.

## What this does NOT claim

A reproducible build strictly subsumes provenance for the artifact->source
binding: it does not require trusting GitHub's runners at all, and it catches
two threats an attestation does not (a compromised runner injecting into the
artifact, and a non-hermetic build input differing between CI and source). The
attestation adds no new detection to a completed run. Its value is the
disambiguation above, plus failing in seconds instead of hours.

That is why the local build stays the authoritative gate, and why the failure
policy is asymmetric:

- An attestation that MISMATCHES aborts the run, before the build is
  submitted (`storage()` joins running futures on the way out, so a failure
  raised after that point is logged in seconds but exits hours later).
- An attestation that CANNOT BE OBTAINED only warns, and the run continues to
  the local build. This covers GitHub or Sigstore being unreachable, the
  anonymous rate limit, a commit built before #11323, and a hotfix built on
  dfinity/ic-private. A check that is redundant with the rebuild must never
  block an external verifier during a live NNS vote.

`AttestationUnavailable` carries the warn path and is deliberately not a
`VerificationError` subclass; a test asserts that, so the split cannot regress
silently. It is raised inside one region and caught in one place
(`attestation_preflight`, which wraps both `ensure_gh` and
`verify_cdn_attestations`), because `main()` catches only `VerificationError`
and `RuntimeError`.

## Mechanism

`gh attestation verify` normally needs a GitHub login, which external proposal
verifiers do not have. The public attestations REST endpoint does not:
repro-check GETs the Sigstore bundles from it and passes them to
`gh attestation verify --bundle`, which needs no token at all. gh comes from
`PATH` when it is >= 2.68 (the first release with `--source-digest`), otherwise
a checksum-pinned 2.98.0 is downloaded into the run's temp directory.

Only the `SHA256SUMS` files are attestation-verified; the images are already
bound to them by sha256. `launch-measurements.json` never was, so it is now
checked against `SHA256SUMS` too.

The policy mirrors `ci/scripts/fetch-attested-sums.sh` byte for byte: signer
`ci-main.yml`, `--source-digest` pinned to the commit, and the build pipeline,
source ref and subject name all bound on ONE attestation entry. Proposal mode
(`-p`) accepts a `release-testing.yml` build of `rc--*`/`hotfix-*`; commit mode
(`-c`) also accepts `ci-kickoff.yml` on `master`.

## Verification

66 unit tests (was 4), green under Python 3.12 (bazel) and 3.10 (the Ubuntu
22.04 audience). Live, anonymously and with no token:

- rc tip `2967c1cc9b`, which carries both an rc and a master attestation:
  accepted in both modes via the release-testing entry.
- master-only `d91d72d773`: rejected in proposal mode, accepted in commit mode.
- pre-attestation `79c01052b5`: 404, warn path, run continues.
- cross-directory subject and a flipped digest: both rejected.
- the whole external-verifier path with gh hidden from PATH and a fresh HOME:
  downloads the pinned gh, checks the pin, extracts one member, verifies.

The classification of a non-zero `gh` exit is a positive allowlist of failure
messages captured from the pinned binary, not a deny-list of outage messages:
gh reports every Sigstore initialisation failure as
`error creating Sigstore verifier: no valid Sigstore verifiers could be
initialized` (also when `~/.cache/gh` is unwritable, which `GH_CONFIG_DIR` does
not relocate), and anything unmatched is treated as unavailable rather than as
evidence. Re-capture those strings when bumping `GH_CLI_VERSION`.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Credential exposure, stale CDN caching, and verification fallback paths can undermine the intended security guarantees.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 2 Medium severity

Open (4)
What changed in this PR

Adds build-provenance verification to distinguish reproducibility failures from CDN artifact substitution.

Changes:

  • Verifies CDN checksum attestations before local builds.
  • Adds pinned GitHub CLI bootstrapping and extensive tests.
  • Documents provenance behavior and supplies CI API authentication.
File Description
README.adoc Documents provenance verification and prerequisites.
ci/​scripts/​repro-check Implements attestation fetching, verification, and policy.
ci/​scripts/​repro_check_test.py Adds attestation and failure-policy tests.
.github/​workflows/​repro-check.yml Supplies a GitHub API token to repro-check.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/repro-check.yml
Comment thread ci/scripts/repro-check Outdated
Comment thread ci/scripts/repro-check Outdated
Comment thread ci/scripts/repro-check Outdated
All four findings were valid.

## GH_TOKEN reached the source build (high)

`repro-check.yml` declared no `permissions`, so as a reusable workflow it
inherited release-testing.yml's, including `actions: write`. Preserving
GH_TOKEN into the script then handed that write-capable credential to
`build_locally()`, which runs the *commit under verification*'s own
`ci/container/build-ic.sh` with the inherited environment. Verifying a commit
must not give that commit's build scripts a credential.

Fixed at both ends: the workflow now declares `permissions: contents: read`,
and `attestation_preflight` pops GH_TOKEN/GITHUB_TOKEN from `os.environ` in a
`finally`, before the build is submitted. The attestations API is the only
thing in the run that wants a token and it is done with it by then.

## Failure classification was calibrated to one gh version (high)

`GH_VERIFICATION_FAILED_MARKERS` was captured from the pinned gh 2.98.0, but
`ensure_gh` accepts any gh >= 2.68 from PATH. A release that words a
verification failure differently would fall in the unmatched branch, silently
downgrading a real mismatch to a warning and continuing the six-hour build.
(The local build would still catch the substitution, but the fast,
unambiguous signal is what this feature exists for.)

`gh_verify` now tracks whether the gh in use is the pinned release, and redoes
an unclassifiable failure with the pinned binary before believing it. The
retry costs one 15 MB download and only happens on failure, so the PATH fast
path is unaffected.

## The security check could read stale cached bytes (medium)

`cached_download` keeps any non-empty file it already has for a commit, so a
rerun after the CDN started serving different bytes would re-verify the first
run's SHA256SUMS and report success. Users should not have to know to pass
`--clean` to detect the very incident this feature targets.

`start_sums_downloads` now bypasses the cache. Each SHA256SUMS is about a
kilobyte, so this is cheap, and it makes both the provenance check and the
image checks that hang off it speak about what the CDN serves now: a cached
image whose bytes no longer match then fails against the fresh sums.

## select_attested_entry escaped the per-directory handler (medium)

It raises AttestationUnavailable for a gh JSON shape the script cannot read,
but sat outside the per-directory `try`, so a shape problem in one directory
abandoned the loop and could hide a real mismatch in a later one — exactly
what the surrounding comment promised it would not do. Moved inside; its
VerificationError is still uncaught there and still aborts the run.

Each fix has a regression test (73 total, up from 66), including one asserting
no GitHub token is visible to `build_locally`.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Cached artifacts can conceal current CDN substitutions, and the skip path can expose GitHub credentials to commit-controlled build code.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Resolved since last review (4)

Comment thread ci/scripts/repro-check
Comment thread ci/scripts/repro-check Outdated
Comment thread ci/scripts/repro-check
All three findings were valid.

## The skip path leaked GitHub credentials to the build (high)

`--skip-attestation-check` returned before the `finally` that removes
GH_TOKEN/GITHUB_TOKEN, so the documented escape hatch was the one way to hand
a credential to `build_locally()`, which runs the commit's own build scripts.
The skip branch now sits inside the try, so the cleanup covers every path out
of `attestation_preflight`.

## Cached artifacts could hide a current CDN substitution (high, medium)

Fetching SHA256SUMS fresh was not enough. An attacker who cannot forge the
attestation has every reason to leave SHA256SUMS alone and swap only the
image: a rerun with a warm cache then hashed the old cached image against the
genuine fresh sums and succeeded, never reading what the CDN serves now. The
same held for launch-measurements.json.

- `launch-measurements.json` joins SHA256SUMS in being fetched uncached; it is
  a few hundred bytes.
- Cached images are revalidated before reuse. `cached_download` records the
  CDN's ETag/Last-Modified/Content-Length beside the cached file and compares
  them against a HEAD on reuse, re-downloading when they differ. Verified that
  download.dfinity.systems and .network both serve a stable ETag. A HEAD is
  cheap next to a multi-GB re-download, and re-downloading everything on every
  run would defeat the cache, whose purpose is to survive a failed build.

Degradations are deliberate: when the HEAD cannot be made, or the cached copy
predates this check, the cache is used rather than the run being blocked.
Artifacts under ic/<commit>/ are immutable by policy, so a mismatch is an
incident and says so.

A test reproduces the attack end to end: run once to warm the cache, replace
only the image on the CDN, rerun, and the run must fail on the hash mismatch
rather than report success.

76 tests, up from 73. README documents the freshness behaviour.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The GitHub CLI bootstrap can hang indefinitely during an outage, and the cache documentation overstates freshness guarantees.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Add timeout and expiry handling to prevent stalled gh downloads

ci/​scripts/​repro-check:1310

This bootstrap path is meant to degrade GitHub unavailability to a warning, but cached_download() ultimately calls urlopen(req) without any timeout. If the release download accepts a connection and then stalls, ensure_gh() never reaches the AttestationUnavailable handler and the authoritative local build does not start. Give the gh download a bounded connect/read timeout and translate expiry to AttestationUnavailable.

Comment thread README.adoc Outdated
Both findings of the third Copilot review were valid.

## Unbounded urlopen could hang the run (medium, in unchanged code)

`fetch_url_to_file` called `urlopen(req)` with no timeout, so a connection
that is accepted and then stalls hangs forever. That matters most for the gh
bootstrap: the whole point of routing its failures to AttestationUnavailable
is that GitHub being unavailable degrades to a warning, and a hang never
reaches that handler, so the authoritative local build never starts either.

Every urlopen now passes NETWORK_TIMEOUT_SECONDS. It is a socket timeout, so
it bounds each read rather than the whole transfer: a slow but progressing
multi-GB download is unaffected, while a dead connection raises and, for the
gh download, is already translated to AttestationUnavailable.

Fixed in the same pass, as it is the same defect one call away: the proposal
fetch in `process_proposal` was equally unbounded, so `repro-check -p <id>`
could hang before doing anything at all.

`fetch_url_validator` now reads headers with .get rather than indexing, which
is what the header object promises and what a plain dict supports.

## The README overstated the warm-cache guarantee (low)

`cached_copy_is_current` deliberately reuses a cached image without comparing
it to the CDN in two cases: when the HEAD request fails, so that a network
problem warns rather than blocks, and when the cached copy predates this
check and carries no recorded validator. Both are now documented, together
with `--clean` (or a fresh `--cache-dir`) for a run that reads nothing from
the cache.

80 tests, up from 76: each urlopen is asserted to be bounded, and a stalled
gh download is asserted to degrade to a warning rather than abort.

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

Copy link
Copy Markdown
Collaborator Author

Good catch, and it was the more serious of the two. fetch_url_to_file called urlopen(req) with no timeout, so a connection that is accepted and then stalls hangs forever — and for the gh bootstrap that defeats the entire point of routing its failures to AttestationUnavailable, since a hang never reaches the handler and the authoritative local build never starts.

Fixed in 5534d3e: every urlopen now passes NETWORK_TIMEOUT_SECONDS. It is a socket timeout, so it bounds each read rather than the whole transfer — a slow but progressing multi-GB download is unaffected, while a dead connection raises and, for the gh download, is already translated to AttestationUnavailable by the existing handler. I fixed the proposal fetch in process_proposal in the same pass, since it was the same defect one call away and meant repro-check -p <id> could hang before doing anything at all. Tests assert each call site is bounded and that a stalled gh download degrades to a warning.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Token permissions and PATH-based pinned-version detection can undermine the intended attestation behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Grant attestations read permission to authenticated API calls

.github/​workflows/​repro-check.yml:32

Declaring an explicit permissions map sets every omitted scope to none, but authenticated calls to the repository attestations endpoint require attestations: read. Consequently this github.token will be rejected and the script will retry anonymously, so it does not provide the advertised quota increase. Grant attestations: read here and on the release-testing.yml caller job (a called workflow cannot elevate beyond its caller); the existing attestation consumers grant this scope in tag-release.yml:3-5 and ledger-suite-release.yml:46-48.

Comment thread ci/scripts/repro-check Outdated
… gaps

Findings from an adversarial sweep of the whole branch, after the third
review round turned up a defect in code it had already seen twice.

## http.client.HTTPException escaped every handler (the important one)

`http.client.HTTPException` is neither `OSError` nor `ValueError`, and urllib
re-raises it unwrapped (`do_open` wraps only `h.request()` errors in
`URLError`). So `IncompleteRead` from a body truncated mid-transfer, or
`BadStatusLine` from a captive portal, passed through
`get_attestations_page`'s retry handler, through `verify_cdn_attestations`,
through `attestation_preflight` and through `main()`: the run died with a
traceback before the authoritative local build had even started. Exactly the
"never block an external verifier" outcome the AttestationUnavailable design
exists to prevent. Reproduced against a local socket server that truncates
the body, and again with a malformed status line.

`fetch_url_to_file` was never affected: it catches bare `Exception`, which is
why this had gone unnoticed in the one seam that predates the branch.

Both narrow handlers now include `http.client.HTTPException`, and
`attestation_preflight` gains a backstop so it can keep its documented
promise that only `VerificationError` leaves it: `VerificationError` and
`RuntimeError` propagate (a CDN checksum file that cannot be downloaded at
all still stops the run as before), anything else warns.

## Cache revalidation had three gaps

- The validator was sampled by a HEAD *after* the GET, so it could describe
  an object the CDN began serving during the download, blessing bytes this
  run never had. Recorded before the GET instead: a change during the
  download then leaves a validator that no longer matches, the next run
  re-downloads, and the sums comparison surfaces it.
- A cache entry with no recorded validator was adopted silently, so on a CDN
  or proxy that refuses HEAD the freshness check was permanently inert with
  no sign of it. It still adopts, since the alternative is a multi-GB
  re-download of something that was never suspect, but now warns and names
  `--clean`, and the warning is replayed at the end of the run like the
  others.
- An `OSError` in the attestation loop abandoned every remaining CDN
  directory, defeating the per-directory isolation documented three lines
  below it. It now degrades that one directory, like its sibling handler.

## Tests

89, up from 81. New: a transport fault warns rather than aborting; an
unexpected exception in the preflight warns; a download failure still stops
the run (the backstop must not swallow it); gh actually runs with the
sanitised environment and a timeout (nothing asserted this); and the two
README-documented cache fallbacks, plus the validator ordering.
`test_raises_once_interrupted`'s stub is now finite, so a regression of the
interrupt check fails the suite by name instead of hanging it.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The integrity flow contains unresolved fail-open attestation handling and cache/classifier correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 High severity

Open (3)

Comment thread ci/scripts/repro-check Outdated
Comment thread ci/scripts/repro-check Outdated
… and close two review gaps

Copilot review round 5 on #11624.

- A substituted SHA256SUMS nobody attested 404s at the digest-keyed lookup
  and takes the warn path, so the fast hard-fail only fires for a
  substitution that carries someone else's attestation. The policy stays:
  there is no public commit-keyed index to define "expected to be
  attested", and every heuristic turns the known IDX-side defects (Attest
  step failed, cancel-in-progress race, ic-private hotfix) into hard
  failures on good releases. But the claim is narrowed in the README, the
  docstrings and the warning text, and the disambiguation is delivered
  where it is needed: compare_with_local_build appends provenance_verdict()
  to the mismatch report and repeats the attestation warnings, which had
  scrolled past before the build started.
- A PATH gh is never gh_is_pinned, whatever version it reports: pinned
  means download_gh checked the archive hash, and a distro build may reword
  the failure messages the classifier was calibrated against. Only the
  download sets the flag.
- cached_download samples the validator before the GET but records it only
  after a successful one, removing the previous copy and its record first.
  Recorded earlier, a failed re-download left the stale bytes under the
  CDN's new validator, and the next run reused them.

92 tests (was 89), green under Python 3.12 (bazel) and 3.10.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unvalidated cached artifacts can still produce an incorrectly confirmed provenance verdict.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (3)

Comment thread ci/scripts/repro-check Outdated
…ct precise

The PR and the comments said the rebuild takes hours. Measured on
2026-09-19: the CI repro-check job takes 23-28 minutes end to end and
Build IC alone about 15, so every "hours" became the measured figure or
"the rebuild".

Copilot review round 6: the confirmed verdict said "the CDN serves what
CI built", which overclaims when a cached image was reused without a
current validator (HEAD failed, or no record). The verdict itself is
sound in that case: compare_cdn_hash binds every compared artifact to
the fresh, attested SHA256SUMS before the comparison, so the local build
was measured against CI's artifact whatever the CDN serves now, and only
the latter goes unobserved. The wording now says exactly that, and the
attestation warnings, the cache ones included, are repeated with the
mismatch report whatever the verdict. Test: a warm cache, a failing HEAD
and a differing local build yield the confirmed verdict with the reuse
warning repeated.

93 tests (was 92).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

A malformed attestations response can abort the remaining preflight checks and conceal a later mismatch.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Validate attestations to prevent skipping later checksum verification

ci/​scripts/​repro-check:573

A truthy non-list attestations value raises TypeError here. That escapes the per-directory AttestationUnavailable handler, reaches the broad preflight fallback, and abandons verification of every later checksum file, so an API shape problem in one directory can hide a real mismatch in the next. Validate the field and raise AttestationUnavailable so the loop continues.

Comment thread .github/workflows/repro-check.yml Outdated
Comment on lines +28 to +31
# This job builds the IC-OS images of the commit under verification, i.e. it executes that
# commit's own ci/container/build-ic.sh. Its token must therefore carry the least possible
# authority, and in particular not the `actions: write` that release-testing.yml grants itself:
# a reusable workflow inherits the caller's permissions unless it narrows them here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let's get rid of this slop:

Suggested change
# This job builds the IC-OS images of the commit under verification, i.e. it executes that
# commit's own ci/container/build-ic.sh. Its token must therefore carry the least possible
# authority, and in particular not the `actions: write` that release-testing.yml grants itself:
# a reusable workflow inherits the caller's permissions unless it narrows them here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed in 7b5ffce, along with the other explanatory paragraphs in this file; each setting keeps at most a one-line comment and the rationale is in the PR description.

basvandijk and others added 5 commits September 19, 2026 23:42
Every outcome other than a verified attestation now stops the run before
the build is submitted, exactly as a mismatch does: a REST 404, an
exhausted rate limit, an unreachable API, a gh bootstrap failure, a gh
error of any wording, an unreadable gh JSON shape, an invalid commit id.
An attestation that cannot be obtained is indistinguishable, from the
verifier's seat, from a substituted file nobody attested (the lookup is
keyed by digest), so treating the two differently was the unsafe half of
the earlier asymmetric policy. It was also the complicated half: the
policy needed one exception class, one catch site, one warning
accumulator and three mechanisms whose only job was to choose warn versus
abort. All of them go:

- AttestationUnavailable: every raise site raises VerificationError.
- GH_VERIFICATION_FAILED_MARKERS and the gh_verify/gh_is_pinned retry:
  both outcomes abort, so classifying gh's stderr decided nothing. A
  non-zero gh exit raises one error that quotes gh and gives both
  readings (Sigstore could not be set up; attested for another commit or
  signer).
- The per-directory degrade-and-continue loop and its "N of M" summary:
  the first checksum file that does not verify stops the run, named.
- warn_about_attestations / attestation_warnings / log_attestation_warnings:
  the only warning left is the skip notice, said when the check is
  skipped and again at the end of the run.
- provenance_confirmed: the build is reached only with every checksum
  file verified or with --skip-attestation-check, so provenance_verdict
  branches on the flag and names it in the mismatch report.

The abort for a missing attestation deliberately does not name the flag
(a test guards that); --help and the README document it as what
DFINITY's CI passes on dfinity/ic-private, where no attestations are
minted. repro-check.yml passes it there via
case(github.repository != 'dfinity/ic', ...): github.repository is the
caller's repository inside a workflow_call (ci-main.yml relies on the
same), and the check could only ever fail on the mirror. The transport
backstop is inverted rather than removed, so an http.client.HTTPException
still aborts with a message and not a traceback. GH_TOKEN on CI becomes
load-bearing and its comment says so.

The image cache's two warn-and-reuse fallbacks keep warning here, through
a plain logger.warning; a follow-up commit turns them into cache misses.

90 tests (was 93: the pinned-gh retry tests and the class-invariant test
go; the warn-path tests invert to abort tests; exit codes are now pinned
at the process boundary).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…idate it

The image cache had two warn-and-reuse fallbacks: a cached copy without a
record of what the CDN served when it was downloaded (a cache from an
older version of this script) was reused and the record adopted, and a
cached copy whose HEAD revalidation failed was reused as well, each with
a warning that this run had not checked the bytes against the CDN. A
fallback that continues past what it could not check is the shape of
thing the abort-on-unavailable policy argues against, and they were the
last users of the warning plumbing.

Both now count as a cache miss: cached_copy_is_current returns True only
when the CDN confirms it still serves the recorded bytes. A HEAD that
fails is in practice followed by a GET that fails the same way, so the
run stops with the usual "Could not download" instead of silently
reusing; a cache from an older version downloads its images once more.
record_cache_validator goes; write_cache_validator stays for
cached_download. The README's "two deliberate exceptions" paragraph goes
with them.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ensure_gh preferred a gh >= 2.68 from PATH and fell back to the
checksum-pinned 2.98.0 download. The only behaviour that ever depended
on which gh ran, the classification retry, is gone, so what remained was
a second code path, a version probe, a snap special case and the
GH_CLI_MIN_VERSION gate, all to save a 15 MB download that is cached per
commit anyway. One gh, the one the messages and the --format json
handling were validated against: ensure_gh is the download, pin check,
single-member extraction and smoke test that download_gh was.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- A 404 from the attestations API means "nothing attested" only on the
  first page; on a later page it is a cursor GitHub no longer honours, and
  it now aborts instead of passing a truncated list as complete.
- A non-list `attestations` field aborts with a message instead of a
  TypeError (Copilot's "previously missed" item on the seventh review).
- The pinned gh's smoke test runs under gh_env like the verification
  does, so no gh process sees the caller's GH_*/GITHUB_* settings.
- Two comments still justified the cache cleanup with the deleted
  adopt-a-record-less-copy behaviour; both now state the surviving
  reason. The run() ordering comment quotes the same rebuild figure as
  the preflight docstring.
- The README no longer calls every listed host required: the mode-scoped
  CDN and dashboard hosts are not, the GitHub and Sigstore hosts are.
- The re-download test resets build_ran between its two runs, so its
  final assertion can fail.
- repro-check.yml: the explanatory paragraphs go (review comment on the
  permissions block); each setting keeps at most a one-line comment, the
  rationale lives in the PR description.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The non-PR step fetched the script with `curl -fsSL ... | python3 -`
inside `sudo ... bash -c`, a shell without pipefail: a failed fetch fed
python an empty program, which exited 0, and the job passed having
verified nothing. That predates this PR, but the attestation check is
now fail-closed and this step is its only carrier on dfinity/ic. It was
also why the flag passed on dfinity/ic-private could not take effect for
a private-only hotfix: the URL named dfinity/ic, where such a commit does
not exist.

The step now runs the sparse checkout the PR step already used, at the
same github.sha the curl fetched, so a dispatched ref still brings its
own contemporaneous repro-check. On the mirror the script therefore runs
for private-only hotfixes too, with the attestation check skipped, and
the job does a real repro check there instead of a 54 s no-op.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Workflow input handling permits bypass or command injection, private hotfix builds cannot clone correctly, and weak cache validators may hide substituted artifacts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 High severity

Open (3)

# sudo resets the environment, so GH_TOKEN has to be preserved explicitly.
set -euo pipefail
sudo --preserve-env=GH_TOKEN -u ubuntu -- \
"$CI_PROJECT_DIR"/ci/scripts/repro-check --commit ${{ inputs.github_sha }} $ATTESTATION_ARGS
Comment on lines +81 to +84
# sudo resets the environment, so GH_TOKEN has to be preserved explicitly.
set -euo pipefail
sudo --preserve-env=GH_TOKEN -u ubuntu -- \
"$CI_PROJECT_DIR"/ci/scripts/repro-check --commit ${{ inputs.github_sha }} $ATTESTATION_ARGS
Comment thread ci/scripts/repro-check
Comment on lines +1052 to +1053
if current == recorded:
return True
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants