Stopping users remove owners reviewers

We use owners files checked into our repo and a script run by ci to generate the equivalent packages in phabricator. We have the add blocking reviewers for each of these packages. Currently a user can manually remove these blocking reviewers and bypass this control. While I understand why this would help unblock reviews we want stricter controls to stop a user from removing these blocking reviewers. We have explored 2 options. Firstly, having a herald rule for each package (on revisions) when affected packages is any of X and authors packages none of X add blocking reviewer X. The problem is we have 1000s of packages and there is no conduit call to create herald rules. The other thing we have tried is to create a webhook to try to recreate the logic of evaluating the package reviewers on every change. This quickly gets hairy given the different options for behaviours on owner packages. What would you recommend?

we want stricter controls to stop a user from removing these blocking reviewers.

Why?

Because we have security controls which require people to get approval from owners for changes to areas which will make application or infrastructure changes to prod environments.

Why are users bypassing those controls?

Occasionally for breakglass reasons however the thing we are keen to avoid some malicious actor or compromised account to take advantage of this.

How do you currently prevent a malicious actor from submitting a safe change (like a typo fix) for review, then landing a dangerous change (like an intentional backdoor) after the typo fix receives appropriate review?

We don’t currently but are aware of the sticky review settings and evaluating whether this is appropriate for us given the slow down it will inevitably cause

Sticky review settings will not prevent a malicious actor from landing changes that differ arbitrarily from reviewed changes: they can push any set of changes with a commit message that says “Differential Revision: X”, where X is a safe, appropriately reviewed change (like a typo fix) without interacting with sticky review settings.

If you want to provide a technical guarantee that published changes differ from reviewed changes by only a “legitimate” merge or rebase, you must prevent all users from pushing directly to the repository and land all changes through Phabricator. See https://secure.phabricator.com/T182 for discussion, particularly the “Chain of Custody” piece. There are technical barriers today which prevent enacting this policy in practice; current customers generally have not expressed interest in enforcing this guarantee so it is not an upstream priority. You could modify Phabricator to support these changes (although this may be difficult), or pursue these changes in the upstream by purchasing a support plan (although there is very little interest in this from customers today).

There is currently no effective way to prevent users from removing reviewers, mostly because I am not aware of any customer use case for this feature. Although an install like yours may reasonably decide to care about removing reviewers but not care about landing arbitrary changes, this feels like putting the cart before the horse to me: it’s leaving the far more powerful, inviting, and straightforward attack surface open while closing off a comparatively obscure attack. In particular, the “remove reviewers” attack generates an unusual and suspicious notification to the removed reviewers!

If you are only concerned narrowly about removing reviewers, fork Phabricator and modify DifferentialRevisionReviewersTransaction to reject transactions that remove reviewers. You can purchase a support plan if you’d like specific guidance from me on modifying the code, or another user here may be able to help you.

Some feature in this vein may come upstream eventually, attached to https://secure.phabricator.com/T10574, but this isn’t likely to happen until the “chain of custody” guarantee can be plausibly configured or some other stronger customer use case for immutable reviewers in isolation arises, since I think it’s misleading to provide a feature aimed at deterring malicious actors while leaving the obvious attack available.

Oh I should add we do disable direct push to repos. we use Land revision and do not use arc land

On its own, that’s insufficient because users may push an arbitrary change to the Staging Area, then submit a different change for review. “Land Revision” does not currently guarantee that the change in the staging area is the same as the reviewed change. See:

https://secure.phabricator.com/T182#142269

“Land Revision” can not trivially provide this guarantee: for example, it can’t compare hashes because the hash of the reviewed change is also client-controlled. Providing this guarantee is complicated, and further entangled with issues like https://secure.phabricator.com/T13426.

If you’ve modified Phabricator to provide this guarantee, modifying ReviewersTransaction is probably comparatively trivial, and reasonable until “Review Rulesets” or similar becomes available in the upstream. If you haven’t provided this guarantee, an adversarial user can land arbitrary code by making a one-line change to arc or, in many cases, running git push --force <evil-commit-hash>:refs/tags/phabricator/diff/123 after running arc diff.

Ok thanks so much for the advice. I will have to take a look at our options and come up with a plan given this is a much larger piece of work than I was hoping.