Update and pin GitHub Actions + pre-commit hooks

[agriyakhetarpal]

See #2742 (comment)

[henryiii]

I'm not a huge fan of fully pinned GHA's. Since the runner images can't be pinned, things change under you anyway. So pinning has negatives:

• Keeps you from getting fixes quickly - including fixes for breakages in runners, like our own cibuildwheel v2.16.5, which fixed a change in powershell!
• Adds churn to the CI since they have to be updated regularly.
• Means old commits can stop building since you need fixes only in new commits, which is a problem if you ever have to backport fixes.

The positives:

• Protects against intentional breakages. This is pretty rare, especially for the official actions.
• Protects against short-lived bugs. Also pretty rare for official actions.
• Protects against malicious releases. But do we really check every hashed release?

I'd argue the negatives outweigh the positives for the official actions most of the time. I'd go with fully pinned for a) less-trustworthy/used actions, b) critical parts of a workflow like the release part, and/or c) for really critical packages.

I'm against this, but I'm also not for it.

[agriyakhetarpal]

I'm not a huge fan of fully pinned GHA's. Since the runner images can't be pinned, things change under you anyway. So pinning has negatives:

• Keeps you from getting fixes quickly - including fixes for breakages in runners, like our own cibuildwheel v2.16.5, which fixed a change in powershell!
• Adds churn to the CI since they have to be updated regularly.
• Means old commits can stop building since you need fixes only in new commits, which is a problem if you ever have to backport fixes.

The positives:

• Protects against intentional breakages. This is pretty rare, especially for the official actions.
• Protects against short-lived bugs. Also pretty rare for official actions.
• Protects against malicious releases. But do we really check every hashed release?

I'd argue the negatives outweigh the positives for the official actions most of the time. I'd go with fully pinned for a) less-trustworthy/used actions, b) critical parts of a workflow like the release part, and/or c) for really critical packages.

I'm against this, but I'm also not for it.

I agree, pins are not the best solution. However, in my understanding, they are the only way by which I can enable "Require actions to be pinned to a full-length commit SHA" to my downstream repositories where I use cibuildwheel. It's a shame that GitHub stopped working on github/roadmap#1103.

Your stance is similar to that of other projects in PyPA-land, and is also coupled with your experience maintaining cibuildwheel throughout its history, so I don't have sound reasons to disagree with it.

I can change this to pin a few selected workflows. In particular:

• we should pin action.yml (so that downstream users like me and others can enable the actions pinning in their settings) – it only uses actions/setup-python@v6, which is not that prone to breakage.
• we should pin release.yml, of course, as a critical workflow
• update-dependencies.yml is also a "safe" workflow to keep pinned, and so is update-major-minor-tag.yml.
• we should remove pins from all other actions, especially GitHub's own actions (I assume we can place them on a higher pedestal in terms of trust rating).

[joerick]

I'm actually slightly -1 on changing to SHA pins. I saw a situation recently (apologies, couldn't find the link) where a project was using SHA pins and somebody's bot changed the SHA but updated the comment to refer to the wrong tag. The issue there is that as humans we read the comment, and the SHA is basically opaque. It would be easy for a malicious actor to slip in a dodgy SHA, we're not gonna check that. But we might spot a suspicious change in the tag in a PR review.

[agriyakhetarpal]

I'm actually slightly -1 on changing to SHA pins. I saw a situation recently (apologies, couldn't find the link) where a project was using SHA pins and somebody's bot changed the SHA but updated the comment to refer to the wrong tag. The issue there is that as humans we read the comment, and the SHA is basically opaque. It would be easy for a malicious actor to slip in a dodgy SHA, we're not gonna check that. But we might spot a suspicious change in the tag in a PR review.

Do you mean #2348? That is a real risk, yes, but I would suggest it was truly an anomaly – I haven't seen it happen again in any of the repos I maintain or watch where Dependabot is involved in pushing these updates. The merged PR #2749 should satisfy my downstream requirement of having SHA-pinned actions in workflows. I'd say that since we have a limited number of workflows and actions, it might actually be easy to spot a suspicious change based on the release note comments that Dependabot inserts in the PR description.

We should still keep these in release.yml at the very least, since it's a critical workflow and helps us commit to good security guarantees for everyone, in an age when GitHub Actions is flawed by design because it lacks a concept of an "actions lockfile" and https://github.com/actions/publish-immutable-action was killed due to priority shifts. What do you think?

[joerick]

Do you mean #2348?

Ah! yes thats the one

That is a real risk, yes, but I would suggest it was truly an anomaly

I'm not so worried about the Dependabot bug, more just the fact that it was so easy for a case where the SHA didn't match the comment to pass through the review net. I think making review easy and reliable is our best defense as an open source project.

There's another thing about SHAs - they have a global namespace. They can even refer to forks from any user. I tried it-

• take this commit from a fork of cibuildwheel - fossabot@31a81df
• created a PR pointing at that commit [offtopic] Test building with a weird SHA joerick/pyinstrument#425

           python-version: '3.8'
 
       - name: Build wheels
-        uses: joerick/cibuildwheel@v3.3.1
+        uses: joerick/cibuildwheel@31a81df7ebfd84ae5c23dc536e255811aa25b630
         env:
           CIBW_SKIP: pp*
           CIBW_ARCHS_MACOS: auto universal2

• GHA builds using the source from the forked repo without complaint. https://github.com/joerick/pyinstrument/actions/runs/22555069381

What's crazy here is that the repo isn't checked by github, it just silently grabs it even though the repo name doesn't match. That's the case with SHAs, but tags have to live on the repo, so at least there's some protection there.

[neutrinoceros]

somebody's bot changed the SHA but updated the comment to refer to the wrong tag.

this pattern can be auto-detected with zizmor:
https://docs.zizmor.sh/audits/#ref-version-mismatch
see #2770

[agriyakhetarpal]

somebody's bot changed the SHA but updated the comment to refer to the wrong tag.

this pattern can be auto-detected with zizmor: docs.zizmor.sh/audits#ref-version-mismatch see #2770

Awesome! That would be pretty useful as a meta-test of whether Dependabot is behaving correctly. Thanks for the link :D