Repository navigation
ci: Resolve most zizmor findings as of zizmor@1.31.1 - #5522
hashcatHitman wants to merge 21 commits into
Conversation
See: https://github.com/actions/checkout/releases/tag/v6.1.0 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
See: https://github.com/actions/cache/releases/tag/v5.1.0 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
See: https://github.com/actions/upload-artifact/releases/tag/v7.0.2 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
See: https://github.com/actions/download-artifact/releases/tag/v8.0.2 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
See: https://github.com/actions/create-github-app-token/releases/tag/v3.3.0 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
See: https://github.com/vmactions/freebsd-vm/releases/tag/v1.5.9 This is the same version we're already implicitly using. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
These are already documented, but `zizmor` expects the comments to be on the same line as the permissions. I think this makes sense, since it seems slightly less likely for a comment in such a position to be anything else. A comment on the line just above, on the other hand, I could see being about something else entirely. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
The template expansion is already stored in an environment variable, we just need to use it. Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
|
This can be reviewed by another member of t-crabwatch (yet to exist) if you don't understand/don't want to review it yourself. See: rust-lang/team#2889 |
| permissions: | ||
| # The cronjob needs to be able to push to the repo... | ||
| contents: write | ||
| # ... and create a PR. | ||
| pull-requests: write | ||
| contents: write # The cronjob needs to be able to push to the repo... | ||
| pull-requests: write # ... and create a PR. | ||
| if: ${{ github.event_name == 'schedule' }} | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 | ||
| with: | ||
| fetch-depth: 256 # get a bit more of the history | ||
| persist-credentials: false | ||
| - name: install josh-sync | ||
| run: cargo +stable install --locked --git https://github.com/rust-lang/josh-sync | ||
| - name: setup bot git name and email | ||
| run: | | ||
| git config --global user.name 'The Miri Cronjob Bot' | ||
| git config --global user.email 'miri@cron.bot' | ||
| - name: Install nightly toolchain | ||
| run: rustup toolchain install nightly --profile minimal | ||
| - name: Install rustup-toolchain-install-master | ||
| run: cargo install --locked -f rustup-toolchain-install-master | ||
| # Create a token for the next step so it can create a PR that actually runs CI. | ||
| - uses: actions/create-github-app-token@v3 | ||
| - uses: actions/create-github-app-token@64ab729a6294000f5aa4a4bc0b73d1f32bc45510 # v3.3.0 | ||
| id: app-token | ||
| with: | ||
| app-id: ${{ vars.APP_CLIENT_ID }} | ||
| private-key: ${{ secrets.APP_PRIVATE_KEY }} |
There was a problem hiding this comment.
# Create a token for the next step so it can create a PR that actually runs CI.
- uses: actions/create-github-app-token@64ab729a6294000f5aa4a4bc0b73d1f32bc45510 # v3.3.0
id: app-token
with:
+ skip-token-revoke: false
+ owner: rust-lang
+ repositories: rust-lang/miri
+ permission-contents: write # The cronjob needs to be able to push to the repo...
+ permission-pull-requests: write # ... and create a PR.
+ permission-workflows: write # If files in .github/workflows/ need to change, this will be required.
app-id: ${{ vars.APP_CLIENT_ID }}
private-key: ${{ secrets.APP_PRIVATE_KEY }}This would resolve github-app, but I'm not sure if it would be correct. I'm also not sure if we'd still need the permissions defined on the cron-rustc-pull level anymore (I'm not sure if we even need them currently... we're getting our token elsewhere, so it shouldn't matter?).
There was a problem hiding this comment.
So maybe you also need workflows permissions to write?
There was a problem hiding this comment.
Ah, thanks. That does seem to suggest actions: write. I updated the original comment to include it. My best guess is that it needs that to start CI?
There was a problem hiding this comment.
It's permission-workflows: write, which allows to push commits that create or change files under .github/workflows/.
There was a problem hiding this comment.
Ah, is that the permission? I was just going off the convention permissions-<permission> and https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions. Good to know, thanks.
There was a problem hiding this comment.
This discussion is way over my head, I don't know much about GHA permissions. Happy to go with whatever you recommend. :)
There was a problem hiding this comment.
Well, if it were my own personal repo, I'd start with the least privileges and increase them as needed when things don't work. I'm pretty sure we could get rid of the job-level permissions on cron-rustc-pull, and I'm honestly not entirely convinced that permission-workflows: write is actually required here, so I'd start without those and see what happened. If the cronjob successfully opens a PR that changes anything under .github/workflows/, I'd consider it successful.
We could do that if you're willing to deal with the cronjob possibly not working and needing some follow-up PRs. Otherwise, I think we could go with the "most likely to work but possibly overkill" solution.
I don't really like the idea of testing this in production, but I think the consequences of the cronjob failing to open a PR are pretty mild?
The secret third solution would be to get rid of the PR-opening and app altogether. I'm pretty sure we could just have the cronjob prepare the branch and send a Zulip message to say it's ready for someone to make into a PR. But I think that might be a bit radical for this.
In short: how much are you willing to deal with maybe needing follow-up visits to fix a broken cronjob?
There was a problem hiding this comment.
FWIW, so far if the cronjob tried to change .github/workflows, that failed. That only happens around once a year or so so I was not bothered much by it. I just did those syncs manually. People should not usually change Miri CI config from the r-l repo anyway as it is entirely untested there.
I am still quite confused about the entire thing though. I thought we are configuring permissions further up:
cron-rustc-pull:
name: automatic pull from rustc
runs-on: ubuntu-latest
permissions:
contents: write # The cronjob needs to be able to push to the repo...
pull-requests: write # ... and create a PR.
Now we have to configure the same permissions again down here? Or is this moving the config from up there to down here?
|
I'm actually going to go ahead and mark this as a draft until I can get someone to look at the potential |
rustbot
added
S-waiting-on-author
| - name: Add cache for cargo | ||
| id: cache | ||
| uses: actions/cache@v5 | ||
| uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5.1.0 |
There was a problem hiding this comment.
Is this really a good idea? An opaque unreviewable identifier? Seems pretty bad to me. How are we supposed to ever update these?
There was a problem hiding this comment.
It's preferable to:
@v4, which just uses the latestv4.X.Yimplicitly@master/main/etc, which always uses the latest commit
It is perhaps less ideal than the recently added actions lockfiles, but we do not want to adopt those yet. See: #t-infra > github actions workflow lockfiles
You update them through Dependabot or Renovate, as I mentioned at the bottom of the PR body. Or if you really wanted to do it manually, you could. You'd go get the commit hash and update it, alongside the comment.
There was a problem hiding this comment.
(As a concrete example of why this is a supply chain risk, see: https://unit42.paloaltonetworks.com/github-actions-supply-chain-attack/)
There was a problem hiding this comment.
Also, re: unreviewable, if it helps, there are lints that would detect if the comment and hash do not match.
See:
These would both be enabled after this PR merges.
There was a problem hiding this comment.
We trust github anyway for all GHA things. So why do we have to defend against GH force-pushing a tag? That seems like not something they would do?
There was a problem hiding this comment.
I agree. That said, if maintainers don't like commit hash for this repository, we can wait until zizmor supports GitHub Actions lockfiles and use them instead of commit hash.
Sounds good then, I'll just switch to full version tags for now.
There was a problem hiding this comment.
Also, re: unreviewable, if it helps, there are lints that would detect if the comment and hash do not match.
Oh okay, I did not know that. If I can treat the version in the comment as authoritative and the commit as just an in-line lockfile, that does change things for me. So you are saying if I receive a PR from someone that updates the GHA config, I can ignore the commit hash and just check the version number?
There was a problem hiding this comment.
Once we update the policy, as long as we enable those two lints I mentioned, yeah, I think that'd be fine, as long as you make sure Crabwatch passes.
We do not strictly require it to pass, so you'd be able to merge a PR that doesn't pass. This is because certain lints are capable of failing for reasons external to the organization, as stated here: #t-infra/announcements > GitHub Actions linter @ 💬
I believe I have a more complete listing of such lints here: rust-lang/crabwatch#92
There are other lints you'd want enabled alongside those, such as impostor-commit, archived-uses, known-vulnerable-actions and typosquat-uses, but those are already enabled by default.
As for updates, I'd be happy to set up automatic update PRs. If you left it up to me, I'd probably go with "1 PR to update all GHA every month, with a 7-day minimum age requirement" via Renovate, since that is the preferred choice of t-infra (I think security updates might bypass the "once a month part", but they should definitely bypass the "7-day minimum age").
My personal recommendation here would still be to do the full hash pin. In theory, the new actions lockfiles cover more than pinning does, but they're still very new, and so far I haven't seen anyone actually confirm that they obsolete certain lints, only speculate. So I'd like to see them mature a bit more and get a little confirmation before coming to rely on them.
There was a problem hiding this comment.
We trust github anyway for all GHA things. So why do we have to defend against GH force-pushing a tag? That seems like not something they would do?
By the way, see the lints I mentioned in action in these PRs:
- ops: Resolve
ref-version-mismatch,stale-action-refs, andunpinned-usesrust-forge#1127 - ci: Resolve
concurrency-limits,ref-version-mismatch,stale-action-refs,superfluous-actions, andunpinned-useslog#742 - ci: Resolve
unpinned-usesandref-version-mismatchcc-rs#1982 - ops: Resolve
ref-version-mismatch,stale-action-refs, andunpinned-useslibz-sys#278
Namely, in rust-lang/rust-forge#1127, actions/deploy-pages was already pinned to a full hash with a comment saying it was for v5, but it actually belongs to the tag v3.0.2-node.24. How exactly that happened, I can't say for sure...
Similarly, in rust-lang/log#742, it looks like they decided to re-tag actions/checkout 6.0.2 at some point. It was released this year, but it isn't an immutable release, so it's definitely possible: https://github.com/actions/checkout/releases#release-v6.0.2
There was a problem hiding this comment.
FWIW, it is kind of odd to have that config about which lints are being enforced out-of-tree. I generally prefer having everything CI needs in-tree so that in principle you can always reproduce any past CI run. Also makes it easier to atomically get the code into compliance with new checks & enforce those checks going forward. But I guess you have reasons for going with external configuration.
We do not strictly require it to pass, so you'd be able to merge a PR that doesn't pass.
I usually press "merge when CI passes", I don't wait for everything to be green. Does that not wait for crabwatch? That seems like a problem then.
Let's discuss automated updates separately, this PR and discussion is already quite big anyway because of how much it is trying to do at once.^^
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
| - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 | ||
| with: | ||
| persist-credentials: false | ||
| - uses: $/.github/workflows/setup |
There was a problem hiding this comment.
What difference does the leading $ make?
There was a problem hiding this comment.
Ah I guess that is https://docs.zizmor.sh/audits/#self-repository.
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
Signed-off-by: hashcatHitman <155700084+hashcatHitman@users.noreply.github.com>
hashcatHitman
moved this from zizmor PR opened
to zizmor PR in review
in t-crabwatch policy expansion
|
I kind of hate this TBH but it seems GHA is just really terrible at its job and the best we can do is work around it. Thanks for helping with that! I assume if we accidentally break one of these things in the future (which is bound to happen), the crabwatch job will complain? |
There will be a short period between merging and when that starts since we need to update the policy. But afterwards, yes. (Except for the version tags, I don't think we support such yet (specifically, I don't think our wrapper around |
|
Since this has already been reviewed, I'll undraft it so it can get merged before things get a chance to drift. I can revisit |
rustbot
added
S-waiting-on-review
In pursuit of getting more
zizmorlints enforced acrossrust-langrepos throughcrabwatch, this PR fixes mostzizmorfindings as ofzizmor@1.31.1, using theauditorpersona. The lints that actually fired per-persona level are:regularpersona findings:artipackedself-repository(See also: Solve theself-repositorylint crabwatch#96)template-injectiongithub-app(I'm not sure I have the knowledge or information necessary to deal with this yet)unpinned-uses(By request)pedanticpersona findings:Everything from
regularFixed in this PR:
template-injectionundocumented-permissionsNot fixed in this PR:
anonymous-definition(Because coming up with names is hard and there is minimal benefit to this)concurrency-limits(Due to concerns about special cases. See also: Determine special cases forconcurrency-limitscrabwatch#107)auditorpersona findings:pedanticsecrets-outside-env(Requires careful consideration and cannot currently be checked bycrabwatch)The hash pin format is supported by tools like Dependabot and Renovate, which can update the pinned hashes and their associated comments. If you'd like help setting either of those up, let me know which you prefer, and if you'd like it in this PR or a new one. By default, I'd assume to set it up to only update GitHub Actions.