This seems generally useful especially for when you often run nix run nixpkgs#<random-command-here> commands without lock files just like how cooldowns protect you against uvx and npx
I wonder if we can natively set something like this up with Hydra though
At least on NixOS, that wouldn’t be much of an issue (at least by default), since nixpkgs indirect flakeref is pinned to a store path of a revision that the system has been built against.
Waiting for the jobset to be completed is already a bit of cooldown.
The way forward
With the currently brewing supply chain crisis, I hope the NixOS project changes this policy and moves to require peer review, despite the obvious negative impact on merge throughput, at least in the short term. This will mitigate one of the largest current security gaps in the project.
I think that the rule should only apply if the package is a dependency for more than x (100, 200 etc.) other packages.
From the linked article:
Nixpkgs’s hundreds of maintainers are all empowered to merge their own PRs with absolutely no review, and this privilege isn’t reserved solely for emergency situations—it’s used with such regularity by said maintainers that it could be described as standard operating procedure.
This seems wrong. Maintainers are anyone who wants to add themselves to be pinged to review PR relating to a packages I think vs. committers are GitHub - NixOS/nixpkgs-committers: Tracking of changes to Nixpkgs committers · GitHub with requirements very, very loosely guidelined in What is expected from a candidate when applying to the committers list · Issue #63 · NixOS/nixpkgs-committers · GitHub. I don’t know what organizaitonal or cultural guiderails exist around whether or not committers regularly merge without review.
I think this is meant in regard to ryantm and maintainers being able to use the merge-bot to merge. So if you control an upstream package, you could theoretically get a bad version in quite as quickly as ryantm picks it up
It’s generally frowned upon for committers to merge their own PRs, especially without any review. But it does happen and is sometimes justified.
I’m not aware of any “organizational” guardrails in place, but in extreme circumstances the nixpkgs-core team could decide to remove a contributor’s commit-access.
Non-committer maintainers can also merge some PRs, generally PRs related exclusively to packages they maintain, that were opened by a committer or the update bot. See CI README: Nixpkgs Merge Bot.
While I’m irked about DetSys trying to profit off all the work going into Nixpkgs, while at the same time complaining about its security without making any effort to improve it, another person complaining doesn’t help anybody. So instead I’ll try to make this a constructive rant 
While Nixpkgs security situation is not as ideal as it could be, and probably never will be on the level of what any well-funded commercial entity could provide, I want to highlight various recently completed and ongoing efforts to improve the status quo, which could probably also use more support:
- Nixpkgs security tracker (source): Tracking of CVE’s in Nixpkgs’ packages, being worked on by @fricklerhandwerk, @florentc, @erethon and others, guided by feedback from the security team, funded via Tweag
- Various potential security improvements: Prompted by @emily
- One of which, org-wide enforcement of secure 2FA, was just enabled
- Security-review team: Reviewing core packages for Nixpkgs-level PR (non-upstream) vulnerabilities, with work round-robin distributed among the members @balsoft (lead), @Infinisil, @andir and @pyrox
- GitHub - NixOS/nixpkgs-committers: Tracking of changes to Nixpkgs committers · GitHub - More Nixpkgs commit access transparency and automatic removal after inactivity
Furthermore, here’s a brainstorm of other ideas to improve Nixpkgs security from two angles:
Against the threat of compromised Nixpkgs committers self-merging (just “require peer review” as suggested by the blog post would only cause frustration):
- Automatically merge PRs for packages whose update scripts would’ve produced the same result
- This paves the way towards blocking all self-merges, because that’s a large class of them
- Block self-merges, but request reviews when it happens:
- Request the author to review (not necessarily approve) another PR whose author wanted to self-merge
- Request a random committer to review the authors PRs
- Fund an available committer to security-review PRs that would be self-merged
- Limit self-merges to:
- Core packages, e.g. by rebuild count, or when the security review team is a maintainer
- A smaller more vetted group of committers
Reactive approaches:
- Set up an easily accessible log of self-merges in Nixpkgs. Before we enabled the requirement for PRs, we had the same for direct pushes to master
- Just mirror what DetSys does: Offer a channel that mirrors the channels with a week delay, and improve documentation to recommend that for more security-critical use cases
- Introduce a channels-meta repository that contains continuously updated metadata history about all channels, ironically like @grahamc’s https://channels.nix.gsc.io/, but with hashes so they can be
fetchTarballed using Nix. Using this, it’s easy to define arbitrary cooldown schemes using Nix itself, as well as cooldown overrides for individual packages. Nice side effect: You’ll be able to access and process all historic versions of Nixpkgs using Nix itself!
Of course, none of these will happen unless either a volunteer steps up, or somebody gets funded for it. And I know that DetSys is funding their employees to upstream some of their work (appreciated!), though it doesn’t seem like they’re accounting for the fact that upstreaming takes much more time than it would otherwise, in particular for more significant changes.
The good (old) news is that the new Nix project governance structure is working better than ever before, so that if you have a great concrete proposal for a major improvement, you can get it approved (or rejected, no promises!) within a reasonable time frame instead of never. If you’re not sure how, see the org repo.
Furthermore, if there’s both somebody interested in doing any of this work, and one or more companies wanting to fund that work, the NixOS Foundation can act as an independent party to handle the legal/financial bits. We’re working to make this easier, but for now interested parties (companies or contributors) can reach out to foundation@nixos.org
Sure, and the same would happen if a committer merged it. Committers generally aren’t doing security checks on upstream code that’s already referred to in nixpkgs, particularly not for leaf packages. This was already known as a possible vector.
Cooldowns are a new and remarkably effective technique for avoiding supply chain attacks.
Wow, didn’t realize I was onto some cutting-edge innovation back in the day, when I did this unique thing of not updating everything immediately. If only I had known!
Joking aside, it’s neither new, nor is it avoiding the attacks, it’s just having a taste tester eating the meal before you do and seeing what happens. That is a burden on those brave souls and a risk to them. I’m very much with the first halve of Sylvan sentence here:
…I’m irked about DetSys trying to profit off all the work going into Nixpkgs, while at the same time complaining about its security without making any effort to improve it…
Anyway, honest question regarding:
just “require peer review” as suggested by the blog post would only cause frustration
Would it really? Because it’s very common in other large organizations. For nixpkgs, urgent stuff is usually done by people who live and breath nixpkgs and and have shortwires to folks that do, too, and that, if push really comes to shove, can review on a moment’s notice.—
Not that it would help, because a token 
by itself does nothing for security and plenty of security professionals have approved things that turned out to be vulnerabilities and got egg on their faces. It just seems odd to me, that a single review by a single person would be too much to ask.
Sure with some social circles it can be a bit easier, but 99% of my PRs are merged by Sandro, and it usually still takes several days after they are approved by mutliple people and have been added to the distress thread.
I think nixpkgs’ scale is so far removed from any other “large organization”, especially when compared to its active committer base, that this is a significant burden. People are already complaining that security patches aren’t pushed quickly enough.
Besides, from my experience with other “large organizations”, peer-review is usually seen as the biggest evil by every person trying to make changes to the product. I have nearly monthly discussions about how we can solve the “peer-review overhead”, and have been for my entire career. We’re holding onto it, because we believe it is valuable (and I am one of the primary proponents), but we’re nowhere near nixpkgs scale.
The answer IMO would be reducing the scale first. It needs to be feasible to do meaningful review before you can introduce these kinds of checks.
Well, yeah, if we’re asking for peer-review we’re asking for actual review, not whatever most organizations pass off for it to tick the security theatre box.
Adding pointless processes just so detsys stops pointing fingers at us (which they won’t, their entire business model is spreading FUD, they’ll start complaining about other things - 2/100+ remains a really small number to bribe or manipulate) seems unproductive. Harmful, even.
Even then, this kind of pointless process still requires prodding someone who might be on the other side of the planet. IME even pointless review usually adds 2-3 days between patch creation and merge.
Which would be fine, if nixpkgs wasn’t so unbelievably large.
Never thought I’d see the day that detsys advertised themselves as the Manjaro of NixOS 
I think we’ve all seen why that doesn’t work.