This sets permissions: contents: read on the CI workflow, which is all the jobs use.
On this repository the token is already read-only, through the repository's default. The workflow's permissions key then adjusts that default (GitHub docs), so the setting now lives in the workflow.
The same change for libsecp256k1 is bitcoin-core/secp256k1#1964.
Closes #36512.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | real-or-random |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Nothing is exploitable today: on this repository the token is already read-only
Can you please refer to the official docs as to why the token is already read-only and why this change is needed? Your description sounds plausible, but it would be good to have the official docs.
and adds
persist-credentials: falseto everyactions/checkoutstep, so the token no longer stays in.git/configfor later steps to read.
Seems a bit verbose and mostly irrelevant, as the token is read-only? I'd say to drop this change to keep the yaml slim.
Pinning actions by hash is left out, after #35754 and the discussion in #36512.
I think it is enough if the pull just lists the stuff it does. Listing all the stuff it doesn't do seems unrelated.
Set the workflow token to `contents: read`, which is all the jobs use.
Thanks. How permissions are calculated for a workflow job says the token starts from the enterprise, organization or repository default, and the workflow's permissions key then adjusts it. That default is not visible from outside, but each job log prints the result under Set up job: this master job shows Contents: read, Metadata: read, Packages: read. With this change Packages drops to none, since unlisted permissions are set to none, and no job uses it. The setting now lives in the workflow, so it also holds in forks.
Agreed on the rest: I dropped persist-credentials: false and the line on what the PR leaves out, and updated the description.
ACK 3b5c7fe3efd95cc88b1daac769f5beffaf332f62
metadata: read and packages: read sounds like a good idea for hardening (principle of least privilege)contents: read (instead of write) in the YAML file is more explicit and transparent than the repo settings. If anything, that means that it will also apply to forks automatically.That seems worth spending 2 low-maintenance lines.
I see this change as controversial.
While demoting the packages permission from read to none looks good in principle, it has no practical effect for this repository, as nothing in ci.yml pulls from GHCR.
Moreover, enforcing permissions on behalf of forks seems outside this project's scope.
- Dropping
metadata: read...
It is not dropped:
GITHUB_TOKEN Permissions
Contents: read
Metadata: read
You are right: Metadata is not dropped. The job log on the PR head lists Contents: read and Metadata: read. The master job lists Packages: read as well. So the change drops only Packages. Nothing in ci.yml uses it, so as you say it has no practical effect today.
On forks, I take your point: their token settings are for their owners. I have removed that claim from the description.
What remains is that the permission is written in the workflow, instead of only in the repository setting. If that is not worth two lines, I am fine with closing the PR.
Fwiw, I'm still ACK on this in principle. Explicit is better than implicit, also because it makes it clear to any reader that this workflow cannot write.
But maybe it's a more of philosophical point at this stage of the discussion. If this change is really controversional, then let's not spend further time on these two lines.