Add GitHub Actions to auto-review BCR PRs #1549
Conversation
| egress-policy: audit | ||
|
|
||
| - name: Run BCR PR Review Approver | ||
| uses: bazelbuild/continuous-integration/actions/bcr-pr-review-approver@b9b7f0dad8ab00a48cc262d1d38339a1dd11c7c5 # master |
There was a problem hiding this comment.
I had a cursory look at https://github.com/bazelbuild/continuous-integration/blob/b9b7f0dad8ab00a48cc262d1d38339a1dd11c7c5/actions/bcr-pr-review-approver/index.js.
In L88, we are getting the author date from the latest commit and then proceed to consider the PR approved if any approval happened after this time. But the commit author date can be freely modified by the PR owner. I haven't tried this, but couldn't the owner just get an innocuous PR approved and then amend malicious changes into it, keeping the date constant?
There was a problem hiding this comment.
Hmm, nice catch! That's indeed a concern. Let me check if we can get the push time of a commit.
There was a problem hiding this comment.
My worry is that we'll miss other things. Is there someone on the OSS team (GOSST?) at Google who could give this a proper security review?
There was a problem hiding this comment.
@mihaimaruseac Any advice? 😃
There was a problem hiding this comment.
We could do a go/gosst-review for this to look deeper if needed.
Just to check my understanding: this workflow is supposed to ensure that if a PR updates packages A and B, then it has been reviewed by an owner of A and an owner of B?
And the risk here is that owners of A approves for A and before an owner for B gets to review the PR author modifies package A?
For PRs with a single owner to review, having Require approval of the most recent reviewable push in branch protection should ensure that any change is reviewed by someone, but if we want 2 separate owners, maybe we need something to discard package A's review if a new commit lands to package A?
There was a problem hiding this comment.
Oh, I haven't thought about the patches. We could probably ask a BCR repo maintainer to review those PRs too (and rely only on package maintainers for other package reviews).
I think only nixpkgs matches the scenarios BCR has. But I'll need to see what processes for review they had.
Other package managers put all the responsibility on the package owners and have a team to react to mishaps after they occur (by yanking malicious packages), afaik.
There was a problem hiding this comment.
Of note: nearly every module has patches because of how the submit-to-bcr app works, mainly to change the version. #1566 might help get away from patches for that.
There was a problem hiding this comment.
I'll have to think more about this, I'll be back next week
There was a problem hiding this comment.
I think only nixpkgs matches the scenarios BCR has. But I'll need to see what processes for review they had.
@aherrmann Do you know what's the review process for nixpkgs?
There was a problem hiding this comment.
The nixpkgs contribution and review process is documented here, see also maintainer's README.
- Merged the module maintainer notifier and the PR approval into one action, which can be distinguished by the "action-type" input. - Implement a new functionality to dismiss stale approvals from all reviewers. The default `Dismiss stale pull request approvals when new commits are pushed` option from GitHub doesn't dismiss approvals from users who is not a repo maintainer. This helps address the concern that a local commit was made before the approval but pushed after it. See bazelbuild/bazel-central-registry#1549 (comment)
|
Sending #1621 to replace this one. |
The GitHub Action was implemented in bazelbuild/continuous-integration#1887
The goal of this GitHub Action is to automate and speed up the review process of trivial PRs that add a new version for existing modules without significant changes.
This GitHub Action will do
Fixes #130