Policy change: pushing to protected branches is now blocked

https://github.com/NixOS/nixpkgs/issues/249117

30 Likes

Should this be under announcements? It might have more visibility there.

5 Likes

Thanks for pushing this through!

And @zimbatm as well!

4 Likes

Now moved to announcements

5 Likes

This wasn’t the case before? :open_mouth: Glad this security risk is patched

2 Likes

How was it a security risk? The same people can now create a pull request and immediately self-merge it.

EDIT: I mean, perhaps the RFC or some other place should state those security improvements, if significant.

5 Likes

The RFC does: https://github.com/tweag/rfcs/blob/709c8979ece291291ff12da8e206cb212a14652e/rfcs/0156-no-direct-pushes.md#motivation

2 Likes

I still don’t really see “security risk” improvement in there, I’m afraid, but I suppose I’m nitpicking about details of that formulation.

I agree that it will make somewhat better visible who pushed a change, which could dissuade potential malicious pushes/merges. Though there’s a catch that GitHub often misattributes who merged a pull request. (It happened to me many times that push-merge by me gets marked as merged by someone else who pushed around similar time. I reported it to support at least twice over the years, but clearly they don’t care enough.)

3 Likes

And to be clear, I do agree with this change in protection, at least from what I know.

3 Likes

At the very least it helps prevent embarrassing mistakes, so easy to accidentally push to the wrong branch sometimes… And I suppose accidental pushes could at least in theory result in a security mess (“oops, I just accidentally disabled everybody’s firewalls while mucking about on a feature branch trying to make the default nftables, my bad”).

2 Likes

I don’t see this change as a security improvement on its own, but it unblocks many security improvements that can’t happen otherwise (e.g. enforcing a 2-person rule for code getting merged to nixpkgs, and in the future requiring CI which can be used to ensure security critical properties stay upheld and prevent mistakes).

7 Likes

s/security/safety/ I guess

2 Likes

What branches are affected?

The master and release-* branches that are consumed by Hydra.

Were the nixos-* branches already protected in some way?

2 Likes

Yes those branches can only be pushed to by the channel updater scripts (@zimbatm confirmed this when I asked the same question)

5 Likes

To be honest, given the strong focus on PRs in nixpkgs, I was surprised to learn that this wasn’t already the case (I was even shocked to learn that there were up to 58 commits not associated with PRs in the last year!).

2 Likes

Given that it is very clear a majority of those are only in the list because GitHub fails at data consistency (the majority of commits not associated with a PR… are signed by GitHub as web merges of PRs, but these are not counted in the 58), you have an option to believe all 58 are GitHub glitches, not direct pushes! Especially if this is a more comfortable thought. (You do not have an option to believe GitHub has no glitches, sorry)

3 Likes

I believe those are non-false positives direct pushes, collected by @Infinisil manually.

At least, I can say for sure that my commits there are direct pushes.

1 Like

The first step is to ask GitHub what it considers direct pushes. Out of between a hundred and a couple hundred, @Infinisil has looked at the commits and discarded obvious false positives (more than half). Thanks for the confirmation that not all the remaining cases are false positives!

Nit that I’ve run into: it’s impossible to push multiple merges at once. Even if pushing the exact same commit hashes succeeds when done one by one. It’s certainly not an issue. Otherwise everything works fine for me and I’m glad that I’m not getting those spurious direct-push alerts anymore.

5 Likes