refactor(release): separate binary preparation and attachment - #11106
Conversation
gustavovalverde
added
A-devops
There was a problem hiding this comment.
Pull request overview
Separates release-binary preparation from GitHub Release mutation, enabling earlier parallel builds.
Changes:
- Adds reusable preparation and attachment workflows.
- Preserves the existing release-triggered orchestration.
- Updates workflow documentation.
Risk: Build provenance may identify the caller SHA rather than the explicit source SHA.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/zfnd-release-binaries.yml |
Prepares and signs binary bundles. |
.github/workflows/zfnd-attach-release-binaries.yml |
Attaches prepared bundles to releases. |
.github/workflows/release-binaries.yml |
Composes preparation and attachment. |
.github/workflows/README.md |
Documents the split workflow. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/zfnd-release-binaries.yml:55
actions/checkoutaccepts mutable refs and abbreviated SHAs, but the provenance copies this input directly intodigest.gitCommitand the later verification compares against the same unverified string. A caller passingmain, a tag, or a short SHA therefore produces an attestation that does not identify the commit actually built, despite this workflow's immutable-source contract. Reject anything except a full commit SHA and confirm that the checkout resolved to it before building.
ref: ${{ inputs.source_sha }}
|
@copilot review |
Reviewed the current state and made two fixes in commit
No other issues found — the |
|
@copilot review |
Found and fixed one remaining issue in the latest commit (
No other issues found across all four workflow files — the |
|
@copilot review |
No further issues found. The current state across all four changed files is correct:
The PR is ready to merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/zfnd-release-binaries.yml:55
source_shais described as immutable, butactions/checkoutalso accepts mutable branches and tags. A caller can therefore passmain; the job builds the branch tip while lines 169-170 record the literal stringmainas agitCommit, and the later check accepts that same inaccurate claim. Require a canonical full SHA and verifyHEADmatches it before generating provenance.
ref: ${{ inputs.source_sha }}
conradoplg
left a comment
conradoplg
left a comment
There was a problem hiding this comment.
Claude findings:
-
The reusable workflow is no longer a trusted builder (high). zfnd-release-binaries.yml:55 now checks out ref: ${{ inputs.source_sha }} — a caller-chosen input — where before it built the event commit. In
the same change, the if: inputs.release_tag != '' gates were dropped from attest (:182) and cosign (:221-226). That gate's own comment said it existed "so the test harness cannot mint attestations under the
same --signer-workflow path users are told to trust." Both mitigations are now gone at once: any caller can have an arbitrary commit built, attested, and cosigned under the exact identity install.md tells
users to trust. Not exploitable today — one caller, guarded to release:released + v tag + ZcashFoundation — but the guard now lives in a different file than the signing. GitHub's own docs make this the
explicit anti-pattern: a trusted builder is one "whose execution cannot be influenced by input provided through the caller workflow." Cheapest fix: assert inputs.source_sha == github.sha inside the build job. -
The new provenance assertion is circular (high). :239-247 verifies that statement.predicate…resolvedDependencies[].digest.gitCommit == source_sha — but that value was written by the same workflow at
:169-170 from the same input, and gh documents this exact property as forgeable: "only signature.certificate and verifiedTimestamps contain values that cannot be manipulated by the workflow that originated the
attestation." I tested it: it passes for the right sha, fails for a wrong one — and also passes against GitHub's default predicate, so it can't even detect the custom predicate being silently dropped. It
reads like a source-binding control but is a tautology in every reachable state. The unforgeable equivalent already exists: gh attestation verify --source-digest "${SOURCE_SHA}", which checks the GitHub-signed
SourceRepositoryDigest certificate extension. -
builder.id is hardcoded (medium). :135 builds job_workflow_ref from a literal zfnd-release-binaries.yml plus github.ref. GitHub's default reads it from the OIDC job_workflow_ref claim. Rename the file, or
pin the uses: to a sha instead of ./, and builder.id silently lies while the certificate SAN keeps the truth — and builder.id is precisely the field a SLSA consumer uses to identify the builder. -
The predicate override is a no-op as shipped (medium). The comment at :114-116 justifies it for "manual recovery runs," but release-binaries.yml:12-15 has only on: release: released — no workflow_dispatch.
So source_sha is always github.sha, and resolvedDependencies[1] is always an exact duplicate of [0] (confirmed in my test run: identical gitCommit). Net effect: GitHub's predicate, derived from the signed OIDC
JWT, is swapped for one derived from env vars, to record information already present, for a mode that doesn't exist.
There's a real design tension behind 2–4 worth deciding explicitly. If recovery runs are wanted, add the workflow_dispatch trigger — but then source_sha ≠ github.sha legitimately, the certificate's
SourceRepositoryDigest will point at the dispatch commit rather than the built source, and no self-asserted predicate can close that gap. If they aren't wanted, dropping source_sha, the predicate override, and
the jq check restores the stronger OIDC-derived provenance for free.
-
No way to exercise this before a real release (low). Removing the artifact-only mode leaves release:released as the only trigger, so the predicate override and jq assertion first execute on a real release.
A failure in bundle means the release publishes with no binaries attached. -
zfnd-attach-release-binaries.yml takes an unvalidated tag (low). It holds contents: write, has no guard of its own, and --clobbers whatever release_tag it's handed (:8-11, :32-37).
-
Nit. release-binaries.yml:67 writes ${RELEASE_TAG#v} to $GITHUB_OUTPUT without delimiter protection. Not reachable — git ref names can't contain control characters — but a printf/heredoc guard is free.
Two metadata/reliability fixes identified during review: 1. workflow_path in the SLSA predicate: the previous suffix strip used REF (the caller's release tag, e.g. refs/tags/v1.0.0) to remove the trailing @ref from WORKFLOW_REF (which ends with @refs/heads/main for a reusable workflow). Those never match, so the path retained the @refs/heads/main suffix. Use %@* to strip the ref regardless of its value. 2. release-binaries-success aggregator: only listed `build` and `attach-binaries` in `needs`. If `binary-version` fails, `prepare-binaries` and `attach-binaries` are skipped; alls-green treats skipped as success by default, so the aggregator passed despite no binaries being produced. Adding `binary-version` and `prepare-binaries` to `needs` surfaces the failure correctly.
The previous commit used REF (github.ref, the release event tag
e.g. refs/tags/v1.0.0) to construct job_workflow_ref, making the
SLSA predicate's builder.id claim the workflow file lives at the
release tag ref rather than its actual location on refs/heads/main.
WORKFLOW_REF (github.workflow_ref) already contains the canonical
form "{repository}/{path}@{ref}" for the reusable workflow and is
the correct source for builder.id.
f917a8c to
0070123
Compare
|
@conradoplg these were addressed |
Merge Queue Status
This pull request spent 45 seconds in the queue, including 9 seconds running CI. Required conditions to merge
|
Motivation
Release binaries cannot be prepared before the GitHub Release exists because the current reusable workflow combines building, signing, and release mutation. This keeps binary builds serialized behind crate publication even though preparation does not depend on the published release.
Part of #11092.
Solution
Split the existing reusable binary workflow at the external mutation boundary:
zfnd-release-binaries.ymlrequires an immutable source commit, version, and feature set, then builds, attests, signs, verifies, and uploads onerelease-binariesworkflow artifact.zfnd-attach-release-binaries.ymldownloads that artifact and owns the only GitHub Release mutation.release-binaries.ymlpreserves the currentrelease: releasedbehavior by composing preparation followed by attachment.source_shaas an explicit resolved dependency in GitHub's SLSA provenance and verifies that claim before bundling. The source remains accurate when a later manual recovery run uses a different workflow event commit.The existing build runners, glibc compatibility ceiling, asset names, checksums, provenance, signatures, and rerun behavior remain unchanged.
Tests
actionlintpassed for all changed workflows with ShellCheck enabled.zizmorpassed for the changed workflows with no findings.git diff --checkpassed.The reusable-workflow handoff and OIDC signing path still require a hosted Actions run because they depend on GitHub-hosted runners and artifact attestations.
Specifications & References
Follow-up Work
The final release cutover will invoke binary preparation in parallel with crate publication and attach the prepared artifact after the GitHub Release exists.
AI Disclosure
PR Checklist
type(scope): description