Skip to content

Build fail-closed release pipeline - #135

Draft
plx wants to merge 6 commits into
mainfrom
agent/issue-63-release-pipeline
Draft

Build fail-closed release pipeline#135
plx wants to merge 6 commits into
mainfrom
agent/issue-63-release-pipeline

Conversation

@plx

@plx plx commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Summary

  • add a manual non-publishing rehearsal and a tag-only protected publication DAG behind one fail-closed aggregate
  • build, install, rebuild, normalize, smoke, checksum, inventory, and trace the crate plus native Linux, macOS, and Windows artifacts
  • restrict OIDC, attestation, and write authority to the owner-approved release environment, with immutable-release-first same-tag recovery
  • retain the approved personal-repository exception: no organization or independent approver is represented as present

Local validation

  • complete locked all-target/all-feature test suite
  • release helper and identity unit suites
  • real two-build archive/crate/SPDX/checksum/provenance bundle rehearsal
  • RustSec and attribution regeneration
  • rustfmt, Clippy with warnings denied, rustdoc with warnings denied
  • actionlint, zizmor pedantic, rumdl, Just formatting, and navigation-guide verification

Hosted acceptance still to record

The draft remains open while the full hosted non-publishing matrix and deliberate red injections run. The crates.io Trusted Publisher registration and GitHub future-release immutability switch also remain fail-closed external prerequisites because no controllable signed-in browser is attached.

Closes #63

@github-actions

Copy link
Copy Markdown

Review: Build fail-closed release pipeline (#135)

Overview

This PR adds .github/workflows/release.yml, a large fail-closed release DAG (identity -> quality/platform/MSRV -> package/native-archives -> assemble -> gate -> rehearsal-or-publish), plus scripts/release_artifacts.py (archives, checksums, SPDX SBOM, in-toto/SLSA provenance, crates.io state), release/pipeline.toml, and substantial documentation updates. The design is thorough: OIDC-only publication scoped to a protected release environment, immutable-GitHub-Release-before-crate-publish ordering, byte-reproducibility checks, checksum-pinned auditor tooling, and deliberate red-path failure injections (tag-mismatch, package-smoke) to prove the gate is actually fail-closed. Docs (README.md, docs/release-policy.md, docs/repository-protections.md, docs/v0.2-contract.md, CHANGELOG.md) and the navigation guide were all updated consistently, per repo conventions.

Strengths

  • Fail-closed structure is real, not aspirational: release-gate uses if: always() + needs on every prerequisite job and a jq 'all(.[]; .result == "success")' check, so a skipped (not just failed) dependency correctly blocks the gate. The two failure-injection paths (tests/test_release_artifacts.py::test_release_workflow_has_one_fail_closed_gate_and_publish_boundary) assert the gate/publish boundary structurally.
  • Least privilege: top-level permissions: contents: read; only the tag-triggered publish job (behind the release environment) gets id-token/attestations/contents: write. No long-lived tokens; crates.io auth uses rust-lang/crates-io-auth-action OIDC exchange.
  • Supply-chain hygiene: all remote actions pinned to full commit SHAs; actionlint/zizmor binaries are checksum-verified before use; SBOM/provenance/checksums are re-verified inside the protected job rather than trusted from the upstream assemble job.
  • Reproducibility claim is scoped correctly: docs explicitly narrow the claim to "same-runner, same build" rather than overclaiming cross-platform determinism.

Issues / Suggestions

  1. The pull_request: trigger will run the full release matrix on every future PR unless removed. The workflow comment at .github/workflows/release.yml:29 already flags this ("Temporary issue-Build a fail-closed release pipeline with trusted publishing and traceable artifacts #63 hosted rehearsal trigger; remove after PR evidence"), so the author is aware — but it's worth calling out explicitly as a merge blocker: as written, on: pull_request: branches: [main] has no paths: filter, so it fires on every PR (docs typo, unrelated refactor, etc.) and runs ~11 jobs including a 3-platform full test matrix, a 3-platform native rebuild-and-archive matrix, and the MSRV suite — largely duplicating what ci.yml already does. If this trigger is accidentally left in after merge, every future PR (including from forks) pays that cost forever. Since fork PRs don't get elevated permissions here (pull_request, not pull_request_target, and publish is gated on push+tag), there's no credential-exposure risk, but the CI-cost/time risk is real. Recommend removing the pull_request trigger in the same PR that merges this (or immediately after hosted rehearsal evidence is captured), rather than leaving it as a follow-up.

  2. release/pipeline.toml is mostly unread at runtime, so it can silently drift from what it documents. Of the fields declared (repository, environment, candidate_tag, crate, binary, supported_runners, trusted_publisher_owner/repository/workflow/environment, reproducibility_claim), scripts/release_artifacts.py only actually reads archive_license_files and workflow_filename at runtime. Everything else is validated solely by tests/test_release_artifacts.py::test_pipeline_identity_is_exact_and_personal, which asserts the file's contents equal a second, independently hardcoded set of literals in the test — it doesn't cross-check the file against release/identity.toml (e.g. candidate_tag vs. expected_tag(identity())) or against release.yml's hardcoded environment: name: release. If the version bumps in identity.toml without a matching update to pipeline.toml's candidate_tag, nothing in the runtime path would catch it — only the hardcoded test, which would need updating anyway. Consider either (a) deriving CANDIDATE_TAG/environment checks in release.yml from pipeline.toml at runtime, or (b) trimming pipeline.toml to just the fields actually consumed, to avoid a "machine-readable" file that's effectively only test-pinned prose.

  3. Test coverage gaps in scripts/release_artifacts.py:

    • crates_version_state (the crates.io HTTP lookup) has no unit test at all — worth mocking urllib.request.urlopen to cover the 404 -> publish-required, checksum-match -> already-published-matching, and checksum-mismatch -> ReleaseError branches, since this function directly gates whether cargo publish runs.
    • verify_sbom's failure branches (wrong spdxVersion, wrong documentNamespace suffix, zero/multiple root packages, empty relationships) aren't exercised — only the happy-path build_sbom is tested.
    • smoke_archive (extraction + member-set validation + delegated smoke_binary) has no dedicated test; only smoke_binary and archive creation are tested independently, so the extraction/member-matching logic (e.g. the .exe suffix substitution, single-root-directory requirement) is untested.
    • verify_checksums's "missing subject" branch and checksum_lines's "duplicate name" branch aren't covered.

    None of these are correctness-blocking today, but they're exactly the kind of fail-closed logic this PR is designed around, so it's worth matching the coverage discipline used elsewhere in this repo.

Minor / Nits

  • The identity job's "Require the exact checked-out commit and immutable tag source" step re-derives origin/main at workflow-run time and asserts it equals GITHUB_SHA for tag pushes — this is intentional per the runbook (tag must equal current protected main), but it's worth a one-line comment in the workflow itself (not just the docs) since it's easy to misread as "tag must match main at tag-creation time."
  • env.CANDIDATE_TAG's chained &&/|| GitHub Actions expression works correctly but is a common source of subtle bugs if a future edit inserts a falsy value in the middle; a short comment noting "this is a poor-man's ternary chain, order matters" next to it would help future editors.

Overall this is a well-structured, appropriately paranoid release pipeline consistent with the repo's stated security posture. My main actionable ask is #1 (don't let the pull_request trigger survive merge) and a lighter-weight ask on #2/#3 for follow-up hardening.

@github-actions

Copy link
Copy Markdown

Review: Build fail-closed release pipeline (#135)

Solid, well-documented addition that matches the repo's existing fail-closed conventions (pinned action SHAs, identity.toml/pipeline.toml cross-checks, gate-before-publish ordering, immutable-release-before-crate sequencing). The scripts/release_artifacts.py + tests/test_release_artifacts.py pair is thorough — reproducibility, checksum tampering/path-traversal, SBOM/provenance identity binding, and the "no early permission leakage before publish" workflow-text assertions are all exercised.

Main concern: temporary pull_request trigger runs the full pipeline on every future PR

release.yml triggers on pull_request: branches: [main], annotated only with a comment:

# Temporary issue-#63 hosted rehearsal trigger; remove after PR evidence.

As written, this means every subsequent PR to main — not just this one — will run the entire rehearsal: three-platform full test suites, MSRV gates, native archive builds on three runners, crate packaging, SBOM/provenance/checksum assembly, etc. That's a large, recurring CI time/cost increase for unrelated work, and it's easy for a "remove later" comment to be forgotten once the PR merges.

Suggestion: file/link a concrete follow-up issue to remove the pull_request trigger once hosted evidence is captured, or add something machine-checkable (e.g., a test that fails once a target date/condition is met) so its removal isn't only relying on the code comment being noticed later.

A secondary wrinkle: for pull_request events CANDIDATE_TAG is hardcoded to v0.2.0 (env.CANDIDATE_TAG ternary). That's correct today, but it's another value that will silently rot if this trigger outlives the v0.2.0 prep window.

Smaller notes (not blocking)

  • scripts/release_artifacts.py::check_pipeline_workflow() matches literal fragments against the workflow's raw text (e.g. "environment:\n name: release" with exact indentation). This is consistent with the project's "catch drift" philosophy, but it's brittle to incidental YAML reformatting — a future cargo fmt-equivalent pass on the workflow could break it for reasons unrelated to actual policy drift. Worth a short comment noting the exact-match sensitivity is intentional, so a future contributor doesn't "fix" the string match instead of the workflow.
  • crates_version_state() fails the whole publish job hard on any transient crates.io HTTP error (including presumably-retryable 5xx), with no retry/backoff. Given the documented runbook explicitly expects manual reruns for recovery, this looks intentional (fail-closed over auto-retry), but flagging it in case retry-on-5xx was meant to be added later.
  • verify_bundle has good coverage for "extra unreviewed asset," but I didn't see a direct unit test for the "wrong archive count" / "missing crate" branches (EXPECTED_ARCHIVE_COUNT / crate_name not in checksums) — minor gap given how central those invariants are to the bundle contract.

Things that look right

  • Least-privilege permissions: workflow-level contents: read default, with id-token/attestations/contents: write scoped only to the tag-triggered, environment-protected publish job; test_release_workflow_has_one_fail_closed_gate_and_publish_boundary asserts this structurally.
  • release-gate uses if: always() plus a .result == "success" check over all needs, so a skipped (not just failed) upstream job still fails the gate closed — correct handling of the "dependency failed → downstream skipped" GitHub Actions behavior.
  • Byte-reproducibility check (dual isolated builds + cmp) before archiving, with archive contents normalized (mtimes, modes, ownership) for deterministic .tar.gz/.zip output — matches the narrow, honestly-scoped reproducibility claim added to docs/release-policy.md.
  • Recovery runbook correctly never moves/deletes a tag and re-verifies checksums before treating any existing GitHub Release or crates.io version as authoritative.
  • Docs (README.md, docs/release-policy.md, docs/repository-protections.md, docs/v0.2-contract.md, AGENTIC_NAVIGATION_GUIDE.md) were all updated in the same change, per the repo's documentation-alignment rule.

Overall: I'd want the temporary PR-trigger question resolved (or at least tracked) before merge, since it's the one change with an ongoing cost to every contributor after this lands. Everything else looks like careful, well-tested work.

@github-actions

Copy link
Copy Markdown

Review: fail-closed release pipeline (#135)

This is a large, careful piece of work — a full non-publishing rehearsal + tag-only Trusted Publishing workflow, a deterministic artifact-building/verification script, and thorough tests. Overall design quality is high and matches the repo's fail-closed, exact-identity conventions. Notes below.

Strengths

  • Least-privilege permission model: top-level permissions: contents: read, with every job re-declaring only what it needs; only the tag-triggered publish job (gated by github.event_name == 'push' + the release-gate aggregate + the protected release environment) gets id-token/attestations/contents: write. No long-lived publish secret anywhere — OIDC → short-lived crates.io token via rust-lang/crates-io-auth-action.
  • Fail-closed aggregate gate: release-gate uses if: always() + toJSON(needs) + jq 'all(.[]; .result == "success")', a solid pattern that can't be silently bypassed by a skipped/cancelled dependency.
  • Publish ordering is deliberate and documented: GitHub Release (verified immutable via the API) is created/confirmed before the crates.io publish, specifically so a missing immutability control can't leave a crate published against a mutable asset set. The recovery runbook in docs/release-policy.md is unusually thorough (9 numbered cases) and consistently forbids moving/reusing tags.
  • Archive/extraction safety: normalized_member() rejects absolute paths and .. traversal, and it's applied to every member of an archive (archive_members()) before extraction ever runs (extract_archive calls archive_members first). Combined with filter="data" on tar extraction, this is good defense against zip-slip/tar-slip.
  • Reproducibility engineering: building the release binary twice per OS and diffing SHA-256 before archiving, plus the /Brepro MSVC linker flag + SOURCE_DATE_EPOCH fix for Windows nondeterminism, is a nice concrete fix — and the audit doc (audits/2026-07-27-issue-63-release-pipeline.md) explains why it was needed (the first hosted rehearsal caught differing Windows binaries).
  • Test coverage is genuinely adversarial, not just happy-path: tag mismatch, non-reproducible rebuild, checksum tampering, path-traversal checksum entries, injected smoke failure, provenance tampering (wrong commit), SBOM identity-boundary mutations, bundle file-set/asset-count mismatches, pipeline-identity drift, and all three crates.io-state branches (publish-required / already-published-matching / checksum-mismatch). The iterative commit history ("Tighten…", "Harden…", "Prove package smoke failure closes release gate") reads like real self-review, not just first-draft coverage.
  • Docs/README/AGENTIC_NAVIGATION_GUIDE.md/CHANGELOG were all updated consistently with the new files, per this repo's own contribution rules.

Things worth double-checking before merge

  1. Action version pins: several actions are pinned to versions I can't independently verify existed as of this PR — e.g. actions/upload-artifact@043fb... # v7.0.1, actions/download-artifact@3e5f45b... # v8.0.1, actions/attest@f7c74d2... # v4.2.0. The SHA is what actually matters for supply-chain safety, but the version comment is what a future reviewer will trust when deciding whether to bump — worth confirming each comment matches the real tag at that commit.
  2. download-artifact's digest-mismatch: error input — please confirm this input name is valid for the pinned version; if it's a typo'd/unsupported key it may be silently ignored rather than fail closed, which would quietly weaken exactly the guarantee this step is meant to add.
  3. Final trigger shape is not itself hosted-tested: the audit doc is upfront that the temporary pull_request trigger (and forced package-smoke/tag-mismatch injections) were removed after gathering hosted evidence, which is the right hygiene — but it means the exact final on: push tags / workflow_dispatch shape hasn't run end-to-end on GitHub yet. Worth a final manual workflow_dispatch rehearsal run against the merged shape before cutting the real tag, since job graph behavior (needs, if: always()) can behave subtly differently than a pull_request-triggered run.
  4. Minor consistency nit: crates_version_state() doesn't call require_tag() like the other subcommands do (it only checks the crate archive filename against identity()'s version). Given tag identity is already re-verified earlier in the same job, this is probably fine, but it's the one command in release_artifacts.py that doesn't take/check a --tag argument, which stands out given how consistently defensive the rest of the script is.

Nits (non-blocking)

  • verify_bundle's new file-set check iterates directory.iterdir() twice (once for files, once for non-files) — could be a single pass, purely cosmetic.
  • The literal-substring checks in check_pipeline_workflow() (e.g. requiring the runner-matrix string to appear exactly twice) are intentionally brittle per the inline comment ("reviewed configuration change instead of silently accepting equivalent drift") — reasonable given this repo's style, just flagging it's a maintenance cost if the workflow is ever reformatted for unrelated reasons.

No correctness bugs found in the Python logic itself (tag/checksum/provenance/SBOM verification all look sound), and I didn't find command-injection, path-traversal, or SSRF issues — subprocess calls use argument lists (no shell=True), the crates.io URL is built from trusted internal config rather than external input, and archive extraction is pre-validated. Nice work overall.

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.

Build a fail-closed release pipeline with trusted publishing and traceable artifacts

1 participant