Moving the discussion out of the GitHub issue. Backlog starts here.
I also recommend scrolling through RFC 79 from the bottom up, because the discussion also covered a lot of ground on that topic.
3 Likes
This issue gets a ping every time someone pushes to master directly. Would be great to get a more formal commitment around this though, as in: how many people are checking it? What are the acceptable cases (eg this looks like a legitimate hotfix)?
Ideally, I’d like to move to a place where there are no more direct pushes to master, but that might be a red herring/give a false sense of security.
3 Likes
We could completely restric pushes to master by setting up a branch protection rule to require PR reviews before merging. Usually this doesn’t apply to admins, but there is an extra option to also enforce this rule, even for admins, if desired.
2 Likes
What I’d like to see is more fine-grained control than “either you get commit access to the full repository or you don’t”. Of course GitHub won’t let us do this directly, but it should be implementable via a merge bot of some kind. Expanding on this concept, it might also give us a path towards an “evergreen master” branch (zero Hydra failures all the time).
3 Likes
As for this, I always thought it would be nice to use bors and make use of their “delegation” feature, which allows a person with commit access to delegate bors access on a per PR basis, thus allowing PR authors to merge their own work after it has been delegated to them by someone who has reviewed it.
3 Likes
Is this actually less friction for the reviewer than just merging? It carries the same weight, AFAICT.
Sure, that’s an improvement. There still are cases where the author self-merges, or PRs are not reviewed properly, due to lack of human-power. So I think we shouldn’t assume that switching to mandatory PRs is automatically gonna make us safe. But it is an improvement.
An improvement in what?
The recent events with GitHub taking too long to notice that web UI merge is indeed a PR merge notwithstanding, I think most of the previous batch of bot complaints was about things that were slightly less annoying to merge via CLI.
Why exactly do we care about enforcement as long as approximately everyone who has expressed any kind of care is informed of any actions not in line with the requirement, and experimentallly there are more things that are in line with the intent but against the way GitHub would implement the policy than things that are against the intent, and the latter are actions taken reasonably?
I am not sure I understand what you mean.
Would you mind breaking down and “unrolling”/“flattening” the sentence structure?
Thanks! 
The discussion notifies everyone in the issue; it’s a lot of people. Feel free to subscribe, to…
The notifications fall into the following groups:
- small fixes, especially for things with large rev-deps in pre-release times or eval fixes
- CLI merges that Github takes too long to recognise
- Web UI merges when GitHub gets completely broken again
Presumably branch protection would be less buggy than the action for the third group. Although by now I will not be surprised.
It is possible that it will break a subset of CLI merge workflows, which is undesirable from the intent point of view.
Are the remaining quickfixes worth the damage to CLI merges? I see nothing convincing in favour. What are the real benefits given the global human effort balance situation is quite unlikely to change? Maybe now that we have specific and rare examples we want more to discuss the thresholds from the policy point of view, and then handle the (pretty rare) borderline cases via watching the notifications from the action?
Yes, sure, conditional-merge bots would be a great improvement. Also a significant effort: revealed preferences from no-documentation format updates imply GitHub as good as wants things — that are not GHA — to break from time to time.
In case you mean that branch protection wouldn’t have false negatives through the Web UI, I would certainly hope that’s the case?! (what good is this branch protection otherwise…)
Do you have any examples in mind? It shouldn’t be too hard, I imagine, to test this out on a separate repo.
- We’d need to know what the specific breakage to CLI merges would be, to define that
- The status quo is only comparable IFF we assume someone is actively reading all the notifications on that issue, and has a strategy to react in case of misuse. Is anything like this in place?
Let’s look at the exceptions named in the issue:
There are exceptions that are allowed for the time being:
automatic updates: those should transition to https://cli.github.com/manual/gh_pr_create or plain github actions
Agreed, a bot should definitely not have direct commit access to the repo (it’s another set of credentials that can be compromised, and less likely to be rotated, IME)
release managers
We could use a branch protection rule, remembering to keep the list of white-listed users updated.
time-sensitive changes (evaluation fix, security fix, etc)
Same as above, we could have an exception for the security team
trivial changes (typos)
Disagree on this one. A typo can wait and is not worth the risk of having an unprotected master branch, IMO.
In case you mean that branch protection wouldn’t have false negatives through the Web UI, I would certainly hope that’s the case?! (what good is this branch protection otherwise…)
Yes, and I hope so too, but we have already had a few «what good» moments with GitHub by now.
CLI merges
Do you have any examples in mind? It shouldn’t be too hard, I imagine, to test this out on a separate repo.
Well, go to the issue and look, there are a few «merge» commits that are not via web UI, will branch protection allow merge-and-test-locally-then-push workflow? Current action tries not to nag in this case, but GitHub is GitHub so it is not perfect.
- The status quo is only comparable IFF we assume someone is actively reading all the notifications on that issue, and has a strategy to react in case of misuse. Is anything like this in place?
Bot nagging is not completely zero-effect, and the more notifications there are, the more people subscribed to the issue will get annoyed at the author if direct pushes were a wrong call.
I am subscribed and will react to all misuse with the caveat that I do not consider direct pushes misuse…
Agreed, a bot should definitely not have direct commit access to the repo (it’s another set of credentials that can be compromised, and less likely to be rotated, IME)
Moreover, as far as I know all the update the bots create PRs anyway.
time-sensitive changes (evaluation fix, security fix, etc)Same as above, we could have an exception for the security team
Evaluation fixes do not have a well-defined team.
trivial changes (typos)Disagree on this one. A typo can wait and is not worth the risk of having an unprotected master branch, IMO.
Again, an unprotected master branch is a risk of what undersirable outcome?
Supply chain attacks.
How do we reduce the risk that someone sneaks in something malicious? (Note: I’m not looking for perfect solutions here, I’m perfectly aware that requiring PRs is not a panacea. But it is a step in the right direction, as others have said.)
Let’s try to play out a couple scenarios:
In the current state of affairs, a malicious actor trying to sneak in some code can:
- commit directly to master, hoping nobody will notice (or notice too late)
- create a PR and self-merge it, counting on the huge level of PR churn
- create a PR, and conspire with someone who merges the PR (should raise less suspicions than self-merging)
- obfuscate the malicious bit, using either a PR or with direct push
Switching to mandatory PRs eliminates the possibility of 1 - which seems like a net win in my book.
Does it make the others more likely? I don’t think so.
We should definitely test this out.
This is heartening (and, really, THANK YOU!). Is anyone else doing the same? (I, for one, am subscribed, but for some reason do not receive any notifications.)
Again, I think if we want to keep things as they are, we should create some more structure around that issue and how to respond to its notifications.
3 Likes
Supply chain attacks.
How do we reduce the risk that someone sneaks in something malicious? (Note: I’m not looking for perfect solutions here, I’m perfectly aware that requiring PRs is not a panacea. But it is a step in the right direction, as others have said.)
Let’s try to play out a couple scenarios:
In the current state of affairs, a malicious actor trying to sneak in some code can:
- commit directly to master, hoping nobody will notice (or notice too late)
- create a PR and self-merge it, counting on the huge level of PR churn
- create a PR, and conspire with someone who merges the PR (should raise less suspicions than self-merging)
- obfuscate the malicious bit, using either a PR or with direct push
Switching to mandatory PRs eliminates the possibility of 1 - which seems like a net win in my book.
Does it make the others more likely? I don’t think so.
So, assuming both no direct pushes and even no self-merge, the attacker compromises a committer’s account, notices impossibility to push, and now needs to create a new Github account to submit a PR to be merged by this committer account? Does it even count as a speed bump?
(Obviously an attacker planning an attack would have a few non-committer accounts with a track record of trivial contributions to «submit» and «report test results» for the change, but that would be a high-touch attack)
We should definitely test this out.
This is heartening (and, really, THANK YOU!). Is anyone else doing the same? (I, for one, am subscribed, but for some reason do not receive any notifications.)
Have you finished reading my statement? Because I then mention that my definition of misuse is empty.
(I do react if something looks broken to me — so far it is GitHub being broken again, not commits)
Again, I think if we want to keep things as they are, we should create some more structure around that issue and how to respond to its notifications.
I think what we should do includes more consensus-building on implications of considered threat models, and less checkbox-ticking.
2 Likes
Oh wow, the onboarding procedure is really rough at the moment. I just received commit access (
) and was greeted with a flood of emails. Turns out that GitHub automatically subscribed me to everything happening on multiple repositories, including nixpkgs itself. Even worse, while going back to the usual “participating and mentions”, all the notification subscriptions I had on issues where I didn’t directly participate got lost 
(well, they probably already were lost at the point where I got subscribed to everything, but that doesn’t change the situation).
4 Likes
Yes, I have finished reading your statement. And interpreted as meaning that you would react if you saw suspicious behaviour - for which I thanked you. Doesn’t seem to have had the effect I hoped for, apparently.
Anyway, I rest my case. I’ve read through (part of) the comments on RFC79, and there seems to be a bunch of legitimate objections to just switching branch protection on.
I’ll leave the task of figuring out a solution to this problem to someone more motivated.
Peace!
The two improvements I’d like to see, but don’t have time to do:
-
No direct pushes to master. For this one we can just revisit the old RFC and now that we have a public log of direct pushes we can if we can really pull it off.
-
A bot to merge PRs based on a file similar to CODEOWNERs, matching username to paths the user can maintain (this is similar model to the kernel).
If someone wants to work on either of those two you get all my karma points 
7 Likes