[ci] enforce zizmor checks, drop codecov

[jameslamb]

Proposing a couple of security-related changes:

• dropping codecov
• enforcing zizmor checks (GitHub Actions pinned to SHAs, protections against script injection, limiting permissions, etc.)

Notes for Reviewers

Also proposing doing the following manual steps after this is merged:

• remove codecov integration from the repo
• set an allowlist of GitHub Actions third-party workflows

[jayqi]

I've got two comments that I don't hold strongly, but just want to open up for some discussion.

[jayqi]

Are we asking developers to install pre-commit so that they run zizmor on pre-commit? Otherwise, wouldn't it be better to just install and run the zizmor CLI, or to use zizmorcore/zizmore-action? This is adding pre-commit as a dependency in the chain.

Also the pre-commit package being installed here also isn't pinned to a commit hash.

[jameslamb]

It's convenient to be able to run zizmor locally so you don't have to iterate with pushes + CI runs, but definitely not required. If you're not touching the GitHub Actions configs (which would be the case for any outside contributor, I'm guessing, as they'd probably only touch R code), then you don't need to care about it.

I chose a general task runner like pre-commit for that instead of, say, a shell script running the zizmor CLI because I could imagine wanting more tools like this in the future. In {uptasticsearch} Austin and I use pre-commit to also run codespell (typos), shellcheck (shell code correctness/portability), and shfmt (shell code autoformatting): https://github.com/uptake/uptasticsearch/blob/main/.pre-commit-config.yaml

It's convenient to have those things bundled together, and pre-commit takes care of things like installing the tools on different platforms.

If you'd prefer not to have this here I'm fine to switch to the zizmor GitHub action, would you like me to do that?

Also the pre-commit package being installed here also isn't pinned to a commit hash.

The pre-commit GitHub Actions I've introduced here is:

- uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd #v3.0.1

I'm unsure how tightly that controls the pre-commit Python package and its dependencies or how to pin those too.

Or did you mean by this comment that you'd want the zizmor-pre-commit here to be pinned to a commit instead of rev: 'v1.23.1'?

[jayqi]

I don't have a super strong position on whether we use the tools directly or if we use pre-commit. If we do want to use pre-commit, it probably makes sense to document that in CONTRIBUTING.md so that it's more clear and standardized how to use it and what the expectations are? I'm okay with treating that as a separate PR though, but just want to point out that adopting pre-commit would make the most sense if we're intentionally using it as designed.

---

Or did you mean by this comment that you'd want the zizmor-pre-commit here to be pinned to a commit instead of rev: 'v1.23.1'?

Yes, I think we should pin zizmor-pre-commit in the pre-commit manifest here if we're generally looking to harden against supply chain attacks where GitHub releases are modified.

[jameslamb]

Ok, I've just pushed a commit pinning the pre-commit hook to a SHA and documenting how to use it in CONTRIBUTING.md.

7b9ecdb

Note that with that change, it'll now need to be manually updated to pull in new versions... pre-commit autoupdate replaces the commit SHA with a tag like v.1.24.1.

I haven't pinned the zizmor Python package to a SHA (it'd be a checksum of the wheel contents, not a git commit) because that package publishes different wheels for different architectures:

https://pypi.org/project/zizmor/#files

It's == pinned to a version in the hook: https://github.com/zizmorcore/zizmor-pre-commit/blob/a4727cbbcd26d7098e96b9cb738169b59711ae51/pyproject.toml#L6

I suspect it's possible to do with platform markers but would be pretty annoying to update. And I REALLY think that'd be overkill, zizmor is probably better hardened against supply chain attacks than 99.9% of other packages on PyPI.

---

If you want to make any changes to the CONTRIBUTING.md or other details here, totally fine with me if you just want to push them directly to the branch. I'm interested in reducing our attack surface here and using zizmor to ensure we maintain those improvements, and don't have strongly-held opinions about how that's accomplished.

[jayqi]

I think it's fine for PyPI packages to be pinned to versions and not SHA hashes. I'm pretty sure PyPI packages are not mutable. Once you've published a version, you can't publish a new distribution over that version number. The main thing is that anything that installs directly from a GitHub repository needs to be hardened, because GitHub does not do anything like that.

[jameslamb]

Ok thanks, yes I agree we can feel confident about not pinning the zizmor Python package any further.

Everything else I'm about to write is just for your interest, and shouldn't affect this PR at all.

Once you've published a version, you can't publish a new distribution over that version number.

There are a couple ways this is not technically true:

1. a pin like ==1.24.1 would also match the version 1.24.1.0, which could be uploaded later
2. PyPI doesn't prevent you from uploading more files to an existing release, including an old one... for example, you could publish an sdist of zizmor 1.24.1 and then much later upload a new wheel with that same version
3. PyPI allows deletions of individual files, which can work like that but in reverse... if you've uploaded only wheels, you could upload an sdist and then delete the wheels (so installers would now be pulling in the sdist)

It's for reasons like this that pinning to a SHA that's a checksum of file contents is safer than pinning to version numbers.

There's more discussion about this here:

• this withdrawn PEP: https://peps.python.org/pep-0763
• https://discuss.python.org/t/pep-763-limiting-deletions-on-pypi/69487
• https://pypi.org/help/ (search for "delete" and "deletion" and there are some details about what's allowed)

But still, anyway, the current state of this PR is more than enough for our purposes here and a net improvement in this repo's security posture.

[jayqi]

This feels a little like overkill to me. Feels like setting to read explicitly here should be fine? Seems like every job is going to need it anyways, and as a baseline, a job with read shouldn't be in danger of doing something problematic.

[jameslamb]

The re-actors/alls-green job in this workflow, for example, does not need to be able to read the repo's contents, only the statuses of workflows.

As a general rule I still prefer this pattern of zeroing out the permissions for the workflow and having to opt in each job with only the permissions it needs, but I'm fine to switch these to read. This is a public repo so allowing contents: read would be totally fine.

[jameslamb]

Pushed that change in 3d23a68

[jayqi]

I don't have a super strong position on whether we use the tools directly or if we use pre-commit. If we do want to use pre-commit, it probably makes sense to document that in CONTRIBUTING.md so that it's more clear and standardized how to use it and what the expectations are? I'm okay with treating that as a separate PR though, but just want to point out that adopting pre-commit would make the most sense if we're intentionally using it as designed.

---

Or did you mean by this comment that you'd want the zizmor-pre-commit here to be pinned to a commit instead of rev: 'v1.23.1'?

Yes, I think we should pin zizmor-pre-commit in the pre-commit manifest here if we're generally looking to harden against supply chain attacks where GitHub releases are modified.

[jayqi]

I think it's fine for PyPI packages to be pinned to versions and not SHA hashes. I'm pretty sure PyPI packages are not mutable. Once you've published a version, you can't publish a new distribution over that version number. The main thing is that anything that installs directly from a GitHub repository needs to be hardened, because GitHub does not do anything like that.

[jameslamb]

Changes look good to me, thanks for reviewing @jayqi !