Repository navigation
ci: build release tags without workflow caches - #138
Conversation
irzhywau
left a comment
There was a problem hiding this comment.
Security review — #138 ci: build release tags without workflow caches
@andersalm — security review of head 96d9e8d2 against develop. Review only; no changes were made to the branch.
Verdict: approve the diff. It correctly closes the cache-poisoning path into release builds raised on #120 (F2), and the manual-dispatch publication hole. One Medium, pre-existing governance gap is adjacent to this PR's threat model (F1); it is not a blocker for this diff. F2–F3 are Low. A fix plan at the end separates what belongs in this PR, in follow-up PRs, and in repository settings.
Severity scale: Critical exploitable now with direct impact · High realistic path to code execution or artifact integrity loss · Medium needs an extra condition · Low hardening · Info no action.
F1 — Medium (pre-existing, outside the diff) · Release publication relies only on write access, and the published tarballs are unsigned
Where: ci.yml release job (if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v'), permissions: contents: write). Repository state checked at review time:
GET /repos/Elacity/elastos-runtime/rulesets→[](no tag rulesets);- legacy tag protection → none;
- no deployment environments.
Threat. The publication gate is "a v* tag was pushed". Branch protection on main (1 review) does not cover tags, so an account with write access can tag a commit that never went through review. CI then builds it and publishes it as a public GitHub Release (v0.7.1 is currently Latest). The tarballs carry only a .sha256 produced by the same job, with no publisher-DID signature, so a downloader cannot tell a rogue release from a real one.
Why Medium, not High:
- No new capability. On GitHub, the write role can already create, edit and delete releases and upload assets directly. So the CI tag path gives a write account, or a stolen write token, nothing it lacks today; it only makes such a release look CI-built.
- Narrow audience. GitHub Releases are not the documented install or update channel.
install.shandelastos updateuse the signed Carrier release, which needs the publisher key and is unaffected. - The real gap is release governance plus unsigned public binaries, not this workflow.
Fix: a tag ruleset alone does not close this, because write users can still edit release assets by hand. Recommended:
- Sign what is published: sign the tarballs, or the
.sha256list, with the publisher DID or Sigstore, and document how to verify them. This is the change that makes a rogue upload detectable. - Release environment: add
environment: releasewith required reviewers to thereleasejob. That gives a second human gate and an audit record for CI-built releases. - Tag ruleset on
refs/tags/v*: restrict creation, update and deletion to release managers, and block moving a tag. Note that manual release editing stays possible for the write role. Narrowing that means reducing who holds write access. - Ancestry check in the
releasejob:- uses: actions/checkout@v5 with: { fetch-depth: 0 } - name: tag must be on main run: git merge-base --is-ancestor "$GITHUB_SHA" origin/main || { echo "::error::tag is not on main"; exit 1; }
F2 — Low (pre-existing) · Cache-free is not pinned: release jobs still run mutable third-party actions
Where: jobs in the release dependency set use tag-pinned actions: actions/checkout@v5, actions/setup-node@v4, dtolnay/rust-toolchain@1.91.0, actions/upload-artifact@v5, actions/download-artifact@v5, and softprops/action-gh-release@v2 (with contents: write).
Threat. Removing caches stops the cache path, but code executed in the release build can still change under the same tag if an upstream action is compromised or its tag is moved. That is the remaining supply-chain input into release binaries.
Fix: pin these actions to full commit SHAs with a version comment (uses: actions/checkout@<sha> # v5.x.y). Let Dependabot (package-ecosystem: github-actions) keep them current. At minimum, pin the actions used by source-home-linux, source-home-macos and release.
F3 — Low · The policy test can miss a new cache path
Where: scripts/ci-release-policy-test.py. Cache steps are found with uses: (Swatinem/rust-cache|actions/cache)@, guarded by the len(caches) == 9 assertion.
Threat. This test is the regression guard for the property "tag builds restore nothing". It would not notice a cache introduced in another form:
actions/cache/restore@…/actions/cache/save@…;cache:input onactions/setup-node(or othersetup-*actions);docker/build-push-actioncache-from/cache-to;- a second
type=ghabuildx invocation.
The test would stay green while a tag build silently restores state again.
Fix: widen the detector, and fail when any of these appear without the CI_USE_CACHE guard:
CACHE_RE = re.compile(r"uses: (Swatinem/rust-cache|actions/cache(/restore|/save)?)@|^\s+cache(-from|-to)?:|type=gha", re.M)Add one negative self-test: an injected unguarded actions/cache/restore@v4 step must fail the check.
Note (Info): evaluate() uses eval with empty builtins. That is not a sandbox, but the input is this repo's own ci.yml, which already executes in CI with the same trust. Acceptable. Add a comment so it is never pointed at external input.
Checked and fine
CI_USE_CACHEis false for tag refs and for any manual dispatch with a non-emptyrefoverride. That also stops a dispatch onmainfrom writingmain-scoped caches built from an arbitrary ref, which closes a second poisoning path.- All nine cache steps plus the buildx
type=ghaimport/export are guarded. The test executes the real buildx shell with a stubbeddocker. I ran the test locally:Ran 3 tests … OK. - Publication now needs
github.event_name == 'push'; a manual dispatch on av*ref can no longer publish. - All jobs stay on GitHub-hosted runners, and the release
needslist and check names are unchanged and asserted. - Kubo is downloaded fresh on tags and verified by its existing checksum path.
Fix plan
In this PR (recommended before merge, small): widen the policy test (F3)
scripts/ci-release-policy-test.py: replace the cache-step detector with one that also matchesactions/cache/restore@,actions/cache/save@, acache:/cache-from:/cache-to:input on any step, andtype=gha. Assert that every match is guarded byenv.CI_USE_CACHE == 'true'(orCI_USE_CACHEin shell).- Add a negative self-test: inject an unguarded
uses: actions/cache/restore@v4step into an in-memory copy of the workflow and assert the check fails. - Add a comment on
evaluate(): it usesevaland must only read this repo's ownci.yml.
Follow-up PR (author): pin actions in the release path (F2)
- Pin to full commit SHAs, with a version comment, every action used by
source-home-linux,source-home-macosandrelease:actions/checkout,actions/setup-node,dtolnay/rust-toolchain,actions/upload-artifact,actions/download-artifact,softprops/action-gh-release. - Add
.github/dependabot.ymlwithpackage-ecosystem: github-actionsso the pins stay current. - Extend
ci-release-policy-test.py: everyuses:in those three jobs must match@[0-9a-f]{40}.
Follow-up PR (author): tag ancestry guard (F1, workflow part)
- In the
releasejob, beforedownload-artifact: check out withfetch-depth: 0and rungit merge-base --is-ancestor "$GITHUB_SHA" origin/main, failing otherwise. - Add a policy-test case asserting the step exists and precedes publication.
Repository settings (repo admin, not code) (F1)
- A tag ruleset on
refs/tags/v*: restrict creation, update and deletion to release managers, and block moving a tag. - A
releasedeployment environment with required reviewers. Then addenvironment: releaseto thereleasejob, which can go in the ancestry-guard PR. - Review who holds write access, since that role can still edit release assets by hand.
Design follow-up (issue under #93): signed GitHub Release assets (F1, root cause)
- Sign the tarballs, or the
.sha256list, with the publisher DID (same key and trust as Carrier releases) or with Sigstore keyless signing tied to this workflow. Document the verification command next to the release notes. - Acceptance: a tampered asset uploaded by hand fails the documented verification.
Merge guidance: this PR can merge once its own CI is green. The F3 test widening is recommended in it. The other items are tracked separately and don't block it.
A manual dispatch on a version tag could publish a GitHub Release, and tag builds could reuse workflow caches. Publication now requires a push to a
v*tag. Tag workflow refs and every nonempty manual checkout override build without workflow cache restore/save or buildx layer-cache import/export. The override rule also covers a branch-selected dispatch that checks out a tag.Branch pushes, PRs, and manual dispatch without an override keep their existing caches. All jobs stay on GitHub-hosted runners with the same eight required check names and release dependencies. Kubo can still download and verify fresh bytes through its existing retry path.
Validation: three focused tests cover 12 event/ref cases across all nine cache actions and publication, execute the actual buildx shell with a recording Docker function, and check runner/name/dependency invariants. The tests run in source-gate. Actionlint 1.7.12 and the four required repo gates pass. Independent source review accepted commit
96d9e8d2; CI and Irzhy review remain merge gates.Closes #137. Part of #93. This carries the release guard from #120 and adds cache-free tag builds. Release execution proof remains with #93; these checks create no tags or releases and perform no signing.