Skip to content

CI: address the zizmor workflow audit findings - #10281

Merged
ThomasWaldmann merged 7 commits into
borgbackup:masterfrom
ThomasWaldmann:zizmor-workflow-hardening
Aug 29, 2026
Merged

CI: address the zizmor workflow audit findings#10281
ThomasWaldmann merged 7 commits into
borgbackup:masterfrom
ThomasWaldmann:zizmor-workflow-hardening

Conversation

@ThomasWaldmann

@ThomasWaldmann ThomasWaldmann commented Aug 29, 2026

Copy link
Copy Markdown
Member

Ran zizmor 1.29.0 (online mode, so the
known-vulnerable-actions and impostor-commit audits ran too) over
.github/workflows/, and took the findings one audit per commit.

Good news first: no known-vulnerable action versions and no impostor
commits - every SHA pin in the repo verified against the GitHub API.

audit n what happened
unpinned-uses 8 fixed - pinned
artipacked 16 15 fixed, 1 documented exception
cache-poisoning 4 mitigated, then marked reviewed
template-injection 2 fixed (was a false positive, but the safer form is free)
dangerous-triggers 1 reviewed, documented
excessive-permissions 3 fixed

zizmor . is clean on this branch, with the accepted findings recorded in
the new .github/zizmor.yml.

unpinned-uses

32bit.yml and bigendian.yml were the only two workflows still on
floating tags - both were added after 4dfcf29 ("CI: pin all GitHub
Actions to commit SHAs"), so they just missed the policy. The SHAs for
checkout/setup-python/cache are the ones the other workflows already use.
docker/setup-qemu-action was not pinned anywhere yet; v4 currently
resolves to the SHA used here (v4.2.0), so it is a no-op today.

artipacked

actions/checkout leaves the job token in .git/config. 15 checkouts do
not need it and now set persist-credentials: false.

backport.yml is a deliberate exception: korthout/backport-action pushes
the backport branch with a plain git push, so it needs what checkout left
behind. Setting persist-credentials: false there would have broken
backporting - noted in the file so nobody "fixes" it later.

cache-poisoning

native_tests, vm_tests and windows_tests each restore a pip/tox cache
and, on tags, build the release binaries, attest their provenance and
upload them - so a poisoned cache entry could reach an attested artifact.
The caches are now skipped on tag pushes: PR/branch runs keep caching, and
release builds start cold.

zizmor's own autofix sets lookup-only: true, which disables cache restore
on every run - not what we want, hence the ignore on top of the real
mitigation.

Also drops the second pip restore-keys entry: <os>-<arch>- is a prefix
of the tox cache keys (<os>-<arch>-tox-...), so a pip cache miss could
restore a tox cache archive. That one is a plain bug, unrelated to security.

template-injection

${{ matrix.binary }} was expanded straight into a run: block. The value
is a literal from this workflow's own matrix, so it was not exploitable -
but it now goes through env:, which stays correct if that ever changes.

dangerous-triggers

backport.yml uses pull_request_target, which it needs for a writable
token. It never checks out or runs PR code (the checkout takes the base
branch), so this is fine - now written down rather than re-derived.

excessive-permissions

backport.yml declared contents: write + pull-requests: write for the
whole workflow; that moves to the job. One job, so nothing actually ran
with more than it does now, but this is the pull_request_target workflow.
Its comments were also swapped - contents: write is what pushes the
branch, not what comments.

codeql-analysis.yml had no workflow-level permissions: at all. Its one
job is scoped correctly, but a second job added later would fall through to
the repository default.

Not in this PR: a repository setting

default_workflow_permissions for this repository is write, so any
job without an explicit permissions: block gets a read-write token. After
this PR every job declares its own, so switching the default to read-only
would change nothing about how CI runs today, while removing the trap:

gh api -X PUT repos/borgbackup/borg/actions/permissions/workflow \
  -f default_workflow_permissions=read

Worth not touching, though: "Allow GitHub Actions to create and approve
pull requests" (can_approve_pull_request_reviews). It covers creating as
well as approving, and both fame.yml and backport.yml create pull
requests with GITHUB_TOKEN.


Workflow YAML parses at every commit; no CI behaviour changes except the
deliberate cache skip on tags.

🤖 Generated with Claude Code

ThomasWaldmann and others added 5 commits August 29, 2026 22:26
Both workflows were added after 4dfcf29 ("CI: pin all GitHub Actions to
commit SHAs") and used floating tags, so they were the only two workflows
not following that policy.  zizmor flagged all eight uses as unpinned-uses.

The SHAs for checkout, setup-python and cache are the ones already used by
the other workflows.  docker/setup-qemu-action was not pinned anywhere yet;
v4 currently resolves to 96fe6ef7f3 (v4.2.0), so this is a no-op today.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
actions/checkout leaves the job token behind in .git/config, so anything
that later archives the workspace also archives the credential (zizmor's
artipacked audit).  None of these workflows push with it - release.yml
authenticates gh via GH_TOKEN, and peter-evans/create-pull-request uses
its own token input - so they can all opt out.

backport.yml is the exception and keeps the credentials: korthout/backport-action
pushes the backport branch with a plain `git push`, which only works with
what actions/checkout left in .git/config.  Marked accordingly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
native_tests, vm_tests and windows_tests all restore a pip/tox cache and,
on tags, also build the release binaries, attest their provenance and
upload them.  A cache entry is writable by any job that can write to the
cache scope, so this is the cache-poisoning path zizmor points at: a
poisoned dependency could end up inside an attested release artifact.

Skip the caches on tag pushes.  Branch and pull request runs - where the
speed actually matters - keep caching, and release builds start cold,
which for a handful of tag pushes per year is a good trade.

zizmor's own autofix for this sets lookup-only: true, which would disable
cache restore on *every* run; that is why these are marked ignored instead.

While here, drop the second pip restore-key.  "<os>-<arch>-" is a prefix
of the tox cache keys ("<os>-<arch>-tox-..."), so a pip cache miss could
restore a tox cache archive.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
${{ ... }} is expanded into the script before the shell sees it, so the
value becomes code rather than data.  matrix.binary is a literal from the
matrix in this very workflow, so nothing is exploitable here, but going
through env is the form that stays correct if the value ever stops being
a literal - and it silences zizmor's template-injection audit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pull_request_target is dangerous when the workflow checks out or runs code
from the pull request, because it runs with a writable token.  This one
does not: the checkout takes the base branch (no `ref:`), and the only
thing that looks at pull request content is backport-action cherry-picking
commits.  Say so, so the next reader does not have to re-derive it, and
mark the finding as reviewed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.48%. Comparing base (5fb717a) to head (468cb70).
⚠️ Report is 11 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10281   +/-   ##
=======================================
  Coverage   87.48%   87.48%           
=======================================
  Files         103      103           
  Lines       18540    18540           
  Branches     2844     2844           
=======================================
  Hits        16219    16219           
  Misses       1624     1624           
  Partials      697      697           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

ThomasWaldmann and others added 2 commits August 29, 2026 22:56
The inline "# zizmor: ignore[...]" comments had to go after the version
comment on the `uses:` line, which left two comments there:

    uses: actions/cache@55cc8345...  # v6.1.0  # zizmor: ignore[cache-poisoning]

Dependabot maintains those version comments when it bumps a pin (see
.github/dependabot.yml, the github-actions ecosystem is updated weekly),
and zizmor's own ref-version-mismatch audit stopped recognising the
version, so the line is better left in the plain "@sha  # version" form.

The explanations stay in the workflows next to what they explain; the new
config file only records which findings were accepted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backport.yml declared contents: write and pull-requests: write for the
whole workflow.  It has only one job, so nothing actually ran with more
than it does now, but this is the workflow triggered by
pull_request_target, which is where the scope should be tightest.

The comments there were also the wrong way round: contents: write is what
lets backport-action push the backport branch, not what lets it comment.

codeql-analysis.yml had no workflow-level permissions at all.  Its one job
is scoped correctly, but a second job added later would silently fall
through to the repository default - which is "write" for this repository.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ThomasWaldmann
ThomasWaldmann merged commit 5176961 into borgbackup:master Aug 29, 2026
24 of 25 checks passed
@ThomasWaldmann
ThomasWaldmann deleted the zizmor-workflow-hardening branch August 29, 2026 21:33
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.

1 participant