The best way to enforce Two-person review
You might be familiar with the practice of code review, but the more formulaic flavor of Two-person review might be new to you.
Two-person review is a formalized version of code review that optimizes the balance between team velocity and catching bugs/deterring bad actors. Requiring more reviewers on code changes improves the chances of catching bugs and making it impossible for malicious actors to do damage with compromised account. On the other hand, every person that you have to loop into the change adds friction to the process that could make it impossible to get anything done.
Over the years, the industry self-learned that requiring anything beyond two trusted contributors leads to diminishing returns. Most of the issues that could be caught with code reviews are caught by the first reviewer. This is where the formalized practice of two-person review came about.
However, trying to enforce two-person review can be a challenge even today. Unfortunately, the tools available to us offer limited configuration options that make it difficult to capture all the nuances of two-person review for an optimal process.
In this post, we’ll walk through what exactly two-person review is, why it is hard to enforce using today’s tools (in particular, on GitHub), and the trade offs to consider when selecting an enforcement option.
- What is two-person review
- Enforcing two-person review
- Limitations of GitHub required reviews
- Using status checks to workaround limitations
- Summary
What is two-person review
Two-person review is a formalized code review process that tries to enforce that every change pushed to source control was agreed to by two trusted contributors.
The semantics of this is straightforward:
- If a trusted contributor opens a pull request (or merge request on GitLab), then another trusted contributor must approve the pull request to merge it.
- If an untrusted contributor opens a pull request, then two trusted contributors must approve it for a merge.
The keyword here is trusted. You wouldn’t want to take in changes that were approved by a third party user who has never contributed to the code base. Similarly, trusting changes that were approved by a single trusted contributor can have risks, as that contributor may have proposed the changes themselves using a faux account.
Requiring two trusted contributors ensures that there are two individuals involved. You want to make sure you maximize the benefits of code review, and having trusted members of the repository who understand the code well ensures that bugs are properly being caught. It also ensures that no single compromised account can get changes merged, making it harder for attackers.
So how do you actually go about implementing this in your organization? The answer is through enforcement by branch protection policies.
Enforcing two-person review
Typically you enforce two-person review using some form of pull request gatekeeper so that people can’t circumvent it on the client side. On GitHub, this is done using protected branches with the required reviews feature.
When a repository is configured with a protected branch setting that has required reviews configured, then the pull request is prevented from being merged unless the pull request has been approved by the required number of reviewers. Conveniently, you can enforce who the reviewers are: either contributors with write access to the repository, or designated code owners. Both options can be considered trusted contributors as far as two-person review is concerned.
However, there are certain edge cases where this doesn’t fully implement two-person review. Let’s look into some of those.
Limitations of GitHub required reviews
At first glance, it might seem like GitHub protected branches is sufficient to enforce two-person review. You just set the required reviewers to 1 and then call it a day. However, when you dig deep into it, you will quickly realize that this is insufficient to protect against the wide range of scenarios at play in practice.
For example, for open source projects, this is insufficent to enforce two-person review because you need to require two reviews for any PRs originating from a fork. Fork PRs are contributions proposed by an untrusted contributor, so naturally you need two trusted contributors to review the change to satisfy the constraint of two-person review.
Another example is the presence of machine users and automated PRs in closed source projects. Machine users should be considered untrusted contributors, since the credentials are typically shared in most organizations. This means that a single person can act as both the machine user and themselves in a review scenario, breaking two-person review. As such, contributions from machine users should be approved by two trusted parties to fully comply with two-person review (unless there is reason to trust the content is a routine trivial change, like in a CD workflow).
In both scenarios, GitHub protected branches with 1 required review would only require 1 trusted contributor review the change.
For the machine user scenario, you can work around the limitation by maintaining a code owner list that doesn’t include the machine user.
For the fork PRs scenario though, you don’t have much choice beyond requiring two reviewers, which can significantly reduce your velocity for standard PRs. You pretty much have to compromise on either requiring three-person review and slowing things down, or accepting the risk that a single compromised account can merge things through by leveraging a faux account to propose changes.
To resolve this, we need to look at using status checks.
Using status checks to workaround limitations
Status checks are arbitrary conditions you can impose on commits and pull requests. These conditions are computed and reported by external applications. Since the applications reporting the checks are external, you can impose any arbitrary constraint for deciding when a check passes or fails.
GitHub protected branches allow you to require status checks to pass for merges. Leveraging this feature, you can implement the requirements around two-person review as a status check, instead of using the standard required review feature.
To do this, you would need to implement an external web app that can use the GitHub APIs to determine if the change is proposed by a trusted or untrusted party, and then dynamically change the required number of approvals from trusted parties. You can then link the external web app to GitHub through a GitHub App, which can configure webhooks so the app knows about PR activities.
Since the external app is fully in your control, you can configure the rules however way you want. For example, you can assign certain untrusted users (users without write access) as trusted so that they only require a single review. This might be useful for long running open source projects where you have regular contributors who you might not trust enough to give full write access to, but trust enough that it can pass with a single reviewer.
The downside of this approach is that you would need to invest in building and maintaining the integration app. This might not be feasible for resource constrained teams and startups.
But if you have a large enough team where throwing up an internal web app isn’t too painful, then this is certainly the best option for properly enforcing two-person review.
Shameless plug: Fensak
If you don’t want to develop and maintain your own app, check out Fensak. Fensak is a managed solution that offers configuration options for implementing two-person review. You can dynamically choose different numbers of required reviewers based on who proposed the pull request. On top of that, you can specify custom auto approval rules using JavaScript and TypeScript for changes that don’t need any reviewer.
Summary
Two-person review is a standard practice for reducing bugs and mitigating compromised accounts. Every team should strive for enforcing two-person review, but care must be taken with the actual enforcement policy. Be fully aware of the limitations of GitHub required reviews, and consider investing in a status check, or using a third-party solution like Fensak.
Everyone wins when there is a better, secure software supply chain!
