ci: add zizmor, fix found issues - #5387
Open
kolyshkin wants to merge 7 commits into
Open
Conversation
Contributor
Author
|
Rebased; no longer a draft; PTAL @opencontainers/runc-maintainers |
kolyshkin
requested review from
lifubang and
rata
and removed request for
lifubang
August 19, 2026 09:31
lifubang
reviewed
Aug 24, 2026
A version tag (even a "full" one like v7.0.1) is mutable: whoever controls the action's repository can move it to point to any other commit. Pinning to a full commit hash is the only way to get the exact same action code on every run. Found by zizmor's unpinned-uses audit. Dependabot is already enabled for the github-actions ecosystem and knows how to update hash pins together with their version comments, so this should not add maintenance burden. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Two things, both applied by "zizmor --fix": - persist-credentials: false for actions/checkout. By default checkout leaves the credentials it used in .git/config, where any later step (or anything that archives the workspace) can pick them up. Nothing here pushes back to the repository, so they are not needed. - cache: false for actions/setup-go. These workflows are also triggered by pushing a v* tag, and validate.yml builds and uploads release binaries, so a cache entry poisoned from a pull request could in principle end up in a release build. The remaining cache-poisoning finding (Lima VM images in test.yml) is ignored inline: it caches downloaded VM images rather than build output, and the suggested lookup-only: true would defeat the cache entirely by re-downloading the image on every run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As recommended by zizmor. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
zizmor is a static analysis tool for GitHub Actions workflows, focused on security issues rather than correctness (which is what the separate actionlint job is for). Use the official action, which runs zizmor from a digest-pinned container image. A few non-default settings: - version: pin it, so a new zizmor release adding new audits does not suddenly fail CI on an unrelated pull request. Bumping it is then a deliberate (and dependabot-able) change. - advanced-security: false + annotations: true. The default uploads SARIF to the repository's security tab, which needs the job to have security-events: write. Inline annotations are enough for us and keep the workflow at contents: read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Move "actions: write" from the workflow level to the only job that needs it, so a job added later won't silently inherit it. Found by zizmor's excessive-permissions audit (--persona=pedantic). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Cancel CI runs that are already superseded, rather than letting them run to completion and waste runner time. This is most of what our CI queue is spent on when a pull request is force-pushed a few times in a row. For the three workflows that run on pull requests, the concurrency group is keyed by PR number, so only runs of the same pull request cancel each other. For anything else -- in particular pushes to main, to release-*, and to a v* tag, the latter also producing the release binaries -- there is no PR number, so the group falls back to the unique run_id and no run is ever cancelled. Keying by github.head_ref instead would be wrong, as two pull requests from different forks can use the same branch name. scheduled.yml only triggers other workflows, so a single group for the whole workflow is enough there. Found by zizmor's concurrency-limits audit (--persona=pedantic). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The lint job does not need read permission: golangci-lint-action only uses pull-requests: read for its only-new-issues option, which we do not set. The lint-extra step does its own "new code only" filtering with --new-from-rev=HEAD~1, which is plain git against the fetch-depth: 2 checkout, and the PR annotations come from checks: write. Found by zizmor's undocumented-permissions audit (--persona=pedantic). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added
the
backport/1.5-todo
thaJeztah
reviewed
Aug 25, 2026
thaJeztah
left a comment
thaJeztah
left a comment
Member
There was a problem hiding this comment.
overall looks good; left two questions
Comment on lines
228
to
+239
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||
| with: | ||
| persist-credentials: false | ||
|
|
||
| - uses: lima-vm/lima-actions/setup@55627e31b78637bf254a8b2a14da8ea7d12564e5 # v1.1.0 | ||
| id: lima-actions-setup | ||
|
|
||
| # This caches downloaded Lima VM images, not build output, and this job | ||
| # does not produce anything that is released. The fix suggested by zizmor | ||
| # (lookup-only: true) would re-download the image on every run. | ||
| - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 | ||
| with: | ||
| with: # zizmor: ignore[cache-poisoning] |
Member
There was a problem hiding this comment.
Wondering if changing the order works, and would avoid some of the warning; first setup lima, then checkout the source code? (not sure at what point information from the branch / commit is needed)
Comment on lines
+13
to
+16
| # There is no point in triggering the downstream workflows twice. | ||
| concurrency: | ||
| group: ${{ github.workflow }} | ||
| cancel-in-progress: true |
Member
There was a problem hiding this comment.
Could this cancel other (release) branches if they're also scheduled?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is based on top of (and currently includes) #5385, thus the draft status. Will rebase once that one is merged.Add a zizmor job to CI, and fix everything it finds (including a few
--persona=pedanticfindings). See individual commits for details.not; dependabot already knows how to bump them.
persist-credentials: falsefor actions/checkout,
cache: falsefor actions/setup-go.stays at
contents: read).actions: writeto theonly job that needs it.
so pushes to main/release-/v tags are never cancelled.
pull-requests: read.