[CI] Hash-pin workflow dependencies called with dangerous permissions

[diogoteles08]

Describe the enhancement requested

Hi! I'm Diogo and I'm back (see #35706) hoping to offer a bit more help with security enhancements.

I noticed that some of your workflows (e.g. the comment_bot.yml) are using pull_requests: write permissions while running unpinned external dependencies -- both external github actions and pip packages. This can be dangerous because this permission allows the usage of GitHub API to approve, merge or push to an existing PR, which could be abused in case a dependency gets hijacked and changes the code your tags are pointing to, for example.

If you wish to keep using the pull_requests: write permissions -- which I believe is the case --, a simple and effective way to eliminate that attack vector is to hash-pin the relevant dependencies, which ensures that the code you're calling won't be changed unless you directly change the hashes. Dependabot would still be able to suggest updates to the hash-pinned dependencies, also keeping comments next to the dependencies with their human-readable version.

To illustrate the required changes to achieve that:

For the GitHub Actions, the pinning would mean changing the calls like
- uses: r-lib/actions/pr-fetch@v2 to
- uses: r-lib/actions/pr-fetch@11a22a908006c25fe054c4ef0ac0436b1de3edbe # v1.3.1.

For the pip dependencies (like this one) it wouldn't be so easy, because we'd need a hash-pinned requirements.txt to install the dependencies using it (dependabot would also be able to update it). I'd totally understand if you prefer not to act on those as they are just a few, but I'd also be available to elaborate on the needed changes if you want.

If you have interest, I'd be happy to discuss the changes and raise a PR implementing them.

Cheers,

Component(s)

Continuous Integration

[assignUser]

Hm I was under the impression that merging a pr would also require contents:write but I might be mistaken.

It def makes sense to pin the actions and I would also be open to a PR pinning the pip dependencies in some way that doesn't cause problems for local development.

[diogoteles08]

Hm I was under the impression that merging a pr would also require contents:write but I might be mistaken.

Actually I think you are right, the endpoint PUT /repos/{owner}/{repo}/pulls/{pull_number}/merge that I mentioned is on this list of endpoints protected by the contents: write permission. My misunderstanding, sorry.

In any way, github documentation lists the endpoints to approve and update a PR as protected only by the pull-requests: write permissions and they could also be maliciously abused, so I think the changes are still valid.

If you still don't oppose to it, I'll soon raise one first PR hash-pinning the actions called with pull-requests: write and any action that is called in a context that uses secrets (which I realized it's the case for cpp.yml, for example), and then another PR hash-pinning the pip dependencies -- it won't affect local development, should only impact the CI pipelines.

[assignUser]

Sounds good, thanks!