search

ossf / allstar

GitHub App to set and enforce security policies
Apache License 2.0
1.24k stars 123 forks source link

Proposal: Smart Code Review feature #161

Open jeffmendoza opened 2 years ago

jeffmendoza commented 2 years ago

This proposal is for a feature that is a bit beyond the current scope of Allstar currently. Please chime in if you would like to see this in Allstar.

Problem: Secure code review (branch protection) settings requires 2 reviewers.

Why?: Sock puppet attacks. Branch protection typically protects against a single rouge or compromised writer/pusher. It prevents direct pushing of commits to the protected branch without a PR and approver. With approvals set to 1, this can easily be circumvented by creating a sock puppet account to create the PR and the compromised pusher can approve and merge the PR. The fix for this with current branch protection settings is to require 2 approvals.

However, requiring 2 approvers can hinder the progress of small teams. One member writing the PR, and two approving means you need three people to get code merged.

Solution: Smart Code Review Feature Proposal for Allstar.

With this feature enabled. Allstar will install itself as a required Status Check. Allstar will check all PRs for number of approvals (with push/write access), and whether or not the PR submitter has write access. If the total is 2 or greater, the check will pass, if 1 or less, the check will fail. The end result is:

Open question

How will users enable or use this feature?

As mentioned, this isn't a security policy + enforcement action like the current scope of Allstar. How should it be enabled and used?

Option 1: A part of Allstar

Option 2: A separate App

Option 3: Why not both?

Preview

@danielbankhead has been working on the code for this in the review-bot branch, but it is not integrated at all. We will need to answer the above question to decide how to proceed.

laurentsimon commented 2 years ago

I like this idea. Having a different app is definitely useful for users who just want this feature, so I vote 2 and 3 (to allow existing AllStar users to use it without installing yet another app)

azeemshaikh38 commented 2 years ago

LGTM too. Although my vote is for Option 1. Option 2 would require asking users to install 2 separate apps and Option 3 requires us maintaining 2 different apps.

How will this interact with the Branch Protection policy?

It might be helpful to keep this separate from BP policy. We can consider extending this feature in the future so that AllStar blocks PRs which do not comply with some custom user policy. E.g check newly added dependencies in the PR using https://docs.github.com/en/rest/reference/dependency-graph#get-a-diff-of-the-dependencies-between-commits and block if the dependencies do not meet certain requirements.

laurentsimon commented 2 years ago

LGTM too. Although my vote is for Option 1. Option 2 would require asking users to install 2 separate apps and Option 3 requires us maintaining 2 different apps.

I think we should aim for the best user experience, not necessarily what we will have to do to make it happen. If having another app increases adoption and improve security, it's something we should consider.

AllStar is marketed as "scalable enforcement", especially for orgs. Individual repos may just want something simpler instead. Just a thought

azeemshaikh38 commented 2 years ago

Both in terms of user experience and adoption a single app is better IMO. Users who have already installed AllStar, simply need to turn on a new policy which is a better user journey than having to install a new app on the organization. Even the adoption is better in this case since we can piggy back on existing installations of AllStar. Also, AllStar's opt-in/opt-out strategy works both at repo-level and at policy-level, making it easy for individual repos to enable this.

AllStar is marketed as "scalable enforcement", especially for orgs. Individual repos may just want something simpler instead. Just a thought

A GitHub app can only be installed at the organization level. Users can choose to include/exclude certain repos on which this app acts. My point being, even if individual repos wanted something simpler (option 2), they would still need to install an org-level app and specify the repos on which this bot can act which is not different than installing AllStar itself.

jeffmendoza commented 2 years ago

Good to hear that there is support to add this!

It might be helpful to keep this separate from BP policy

Agree, It might get too complicated in settings to merge this with the BP settings.

Option 3 requires us maintaining 2 different apps.

Not a big deal, it could even be the same binary/deployment that iterates through multiple app IDs / private keys. That said, I agree that simpler is better.

I believe there are good reasons for both 1 and 2. Let's just go with 1 for now, and add config to the existing Allstar to turn on this option. Then see if we want to do both later.

yorinasub17 commented 2 years ago

We actually have something similar we rolled out internally, so would be awesome if allstar implements this since that means we can retire that app!

With that said, I do have one scenario that might be a concern. In particular:

PRs opened by users with write access will require 1 approval (by a user with write access).

Would someone be able to workaround this by hijacking another person's PR? That is, assuming Alice and Bob both have write access, but Bob is compromised, could Bob sneak malicious unreviewed code by hijacking a PR that is opened by Alice by making commits to Alice's PR and then approving it?

I am assuming the check passes in this scenario given that Alice is the one who opened the PR, and Bob is "another person" that is approving the PR despite committing to it, but that would mean that one rogue account only has to wait until a collaborator opens a PR to workaround the check. Or am I misreading the proposed status check?

FWIW, in our internal app, we are trying to protect against this scenario by going one step further and requiring one approval from a user with write access AND who has not made any commits to the branch. There are some nuances that we are working through though, such as how to handle coauthorship and cherry-picks (e.g., can you use a sock puppet user to cherry-pick malicious commits onto the PR?).

brikis98 commented 2 years ago

+1 on this feature and +1 on @yorinasub17 thoughts around requiring a review from a non-author.

laurentsimon commented 2 years ago

+1, this is something scorecard currently falls for https://github.com/ossf/scorecard/issues/1783#issue-1181023061 Maybe at Scorecard we can re-use some of your verification code too :-)

Additional notes: I've heard that in some scenarios, the Login of a user can be spoofed. I've not investigated this further, but I wonder if this is a problem for your implementation too. Do you trust the Login to identify users or you require some sort of signature?

yorinasub17 commented 2 years ago

I've heard that in some scenarios, the Login of a user can be spoofed. I've not investigated this further, but I wonder if this is a problem for your implementation too. Do you trust the Login to identify users or you require some sort of signature?

Yes this is a scenario we verified too, where a naive implementation of verifying commit authors will fail since you can tack any github login to the commit using git config settings (e.g., I can check the commit metadata of another user, and then use the exact same metadata to push a commit to make the commit look like it came from someone else). The only way we found to protect against that is to require verified commits in the branch protection setting.

laurentsimon commented 2 years ago

Can you explain further? Login is not the same as Username/email that can be spoofed via git config. Do you mean login or username?

yorinasub17 commented 2 years ago

The login of a commit is derived from the Name and Email settings of the commit, so all you have to do is set the Name + Email of the commit to that person. E.g., see this commit I just created on our allstar fork:

https://github.com/gruntwork-io/allstar/commit/821eae112279f6c91bbb01b4148220e93b128171 (NOTE: the branch has been deleted from GitHub. Looks like the commit is still accessible for now, but I expect it to be garbage collected eventually)

If you grab the commit metadata from the API (https://api.github.com/repos/gruntwork-io/allstar/commits/821eae112279f6c91bbb01b4148220e93b128171), you will see that there is no mention of my login. For all intents and purposes, it is as if jeffmendoza made that commit, if not for the fact that it is not signed/verified. But if you don't required signed commits, it could bypass a naive implementation of the check (unless the check is checking for signed commits).

All I did to create this commit was set user.name and user.email to be what was used on one of the other commits by jeffmendoza.

(@jeffmendoza Apologies for using your username, but thought it had the maximum effect of demonstrating the problem. ~I will be deleting that commit/branch in a few days.~ The branch has been deleted, since the demo has served its purpose.)

laurentsimon commented 2 years ago

Thanks for the demo, maximum effect :-) This app is something we should discuss in the context of SLSA: it can be used to attest to code reviews. Wdut?

cc @hepwori

azeemshaikh38 commented 2 years ago

All I did to create this commit was set user.name and user.email to be what was used on one of the other commits by jeffmendoza.

Ouch! This is really really bad.

laurentsimon commented 2 years ago

/cc @josepalafox anything in the pipeline to be able to get the real author of an event (commit, etc)... like a new API or option?

agonzalez-plume commented 2 years ago

I'd like to ask if this feature would change the permissions needed for the app.

It adds write access to PRs?

One other question, should we require the same level of reviewers depending on the branch, such as only require this feature on the release branch/default branch, etc.

jeffmendoza commented 2 years ago

Thanks for all the comments. It does seem like it could be worked-around (ex: approve a PR then add your own commits to it), but not much more so than the current GitHub branch protection settings. Likely turning on "dismiss stale reviews" will close some of those gaps. I think it makes sense to go ahead and enable this as an experimental feature of Allstar (Option 1 above) and we can work through what it can and cannot protect against and plan further enhancements.

To do so, I see the following steps: