[RFC 0172] Mergebot #172
[RFC 0172] Mergebot #172
Conversation
|
I'd like to nominate for shepard team. |
| - **maintainer_update:** Currently, this strategy allows package maintainers to merge updates from r-ryantm, triggered by mention of the bot. | ||
|
|
||
| Proposed strategies are: | ||
|
|
||
| - **automerge_updates:** Merges PRs from r-ryantm automatically if ofBorg checks pass, triggered by PR creation or mention. We strongly recommend to have at least one passthrough test. | ||
| - **automerge_backport:** Automatically merges backport PRs if ofBorg checks pass, triggered by PR creation or mention. | ||
| - **full_consensus:** Requires approval from all maintainers before merging. The bot will notify which maintainers' approvals are pending, triggered by PR approval. | ||
| - **single_maintainer:** Allows a single maintainer's approval to trigger a merge, facilitated by a mention of the bot. |
There was a problem hiding this comment.
These need to be specified more clearly. From just these it sounds like only maintainer_update and automerge_updates are limited to @r-ryantm as author, while all the others don't restrict the author.
There was a problem hiding this comment.
yes the idea is to not restrict the author for the other strategies.
There was a problem hiding this comment.
Oh that is very easy to miss, you have to read between the lines to even notice it. I don't think it should be done like this and would definitely need to be justified. Honestly this RFC should primarily be just about the lifting of that restriction, because it's the only truly controversial part.
There was a problem hiding this comment.
ah, okay, I thought that was pretty clear. But yeah, that's the part worth discussing the most.
There was a problem hiding this comment.
I added now a sentence to the summary to make the goal clearer 4727de4 (#172). Is that good enough? or should be emphasize it more?
|
|
||
| - **Security Risks:** There's a potential for malicious actors to manipulate the merge bot to authorize merges improperly. A mitigation for these pull requests could be running validity checks (e.g file size limits, valid nix files, ..) | ||
| - **Reliability Concerns:** A malfunction in the merge bot could disrupt numerous contributors' workflows. | ||
| - **Code Quality** As no review is done by seasoned commiters, the code quality could drop. We want to face that by holding monthly meetings and review the merged PRs to see if the code quality drops and improving on the bot in that case. |
There was a problem hiding this comment.
Maybe some public audit log could be useful for this purpose.
There was a problem hiding this comment.
maybe we can use the github api to show every invocation of the mergebot. Or maybe we should collect all of them in the mergebot directly and show them on a webpage?
There was a problem hiding this comment.
Showing them on a webpage or so could be handy, I think. It makes it easy to quickly review some of the actions taken.
There was a problem hiding this comment.
It would be very useful to do that in a way that it does not query the Github API live, but stores the audit log in some way independent of the nixpkgs repo.
In the xz backdoor situation, Github took down the xz repo, which had the effect that people were hindered investigating the compromise.
If the merge bot is abused, it would be great to be able to have those logs even if the Github live API is not available for this (just saving the audit log somewhere else in some reasonably frequent interval would be enough to do that).
| Proposed strategies are: | ||
|
|
||
| - **automerge_updates:** Merges PRs from r-ryantm automatically if ofBorg checks pass, triggered by PR creation or mention. We strongly recommend to have at least one passthrough test. | ||
| - **automerge_backport:** Automatically merges backport PRs if ofBorg checks pass, triggered by PR creation or mention. |
There was a problem hiding this comment.
I think backports could often use an extra pair of eyes. However, they're also often pretty neglected...
There was a problem hiding this comment.
They should still follow the criteria for what is acceptable to backport. I think that requires a person to check.
https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#changes-acceptable-for-releases
There was a problem hiding this comment.
for certain packages it seems to be fine? like security relevant leave packages could always auto backport
rfcs/0172-mergebot.md
Outdated
|
|
||
|  | ||
|
|
||
| The current `nixpkgs` contribution model relies heavily on a group of *committers* (approximately 200 at the time of writing), who possess exclusive rights to merge PRs. This model has remained unchanged since its inception and poses limitations on community contributions and maintainer workload. Implementing granular merge permissions will enable a broader contribution base and lessen the burden on existing committers by empowering package maintainers with more control over their respective packages. |
There was a problem hiding this comment.
Not sure if it should land in this RFC, but I think a hybrid model is ultimately the most suitable. Larger changes of course sometimes require people that have a good overview, and limiting that by a lot seems likely to cause a power struggle or fiefdoms, I'd expect. It makes sense to me to touch upon that lightly, but maybe it just makes it harder to get this rfc accepted. At least consider writing a bit about (possible) social effects.
There was a problem hiding this comment.
this section is not about limiting existing commiters, but empowering maintainers to do more actions directly with the bot. Without commiter assistance required and not giving maintainers global merge access.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixpkgs-merge-bot-testing-and-plan/39824/13 |
|
|
||
| Proposed strategies are: | ||
|
|
||
| - **automerge_updates:** Merges PRs from r-ryantm automatically if ofBorg checks pass, triggered by PR creation or mention. We strongly recommend to have at least one passthrough test. |
There was a problem hiding this comment.
Would it make sense to fail the automerge_updates strategy by design if no passthrough tests exist? In which case would automerge + no passthrough test be ok?
There was a problem hiding this comment.
I think this is something we can decide by a case by case basis? like if there is no passthrough test defined, why was the strategy enabled for this package?
There was a problem hiding this comment.
like if there is no passthrough test defined, why was the strategy enabled for this package?
That's exactly my point - is there any case where this would be ok? If not, then making that strategy fail by design if no passthrough test is defined seems like a good choice to avoid this kind of mistake happening.
Given that this only ever automerges r-ryantm updates, I'm ok with the current design, but I think being stricter here helps avoid mistakes and gives an incentive for package maintainers to make Nixpkgs more automated by contributing passthrough tests (this is a good thing IMO).
There was a problem hiding this comment.
I think there are certain packages where a delayed update would be more of a liability than an automerge. Like maybe the package is just some icon theme where there really is no testing needed?
There was a problem hiding this comment.
Sometimes the tests of the package itself are exhaustive enough too. So I think it makes sense to not require anything extra per say.
|
|
||
| ## Drawbacks | ||
|
|
||
| - **Security Risks:** There's a potential for malicious actors to manipulate the merge bot to authorize merges improperly. A mitigation for these pull requests could be running validity checks (e.g file size limits, valid nix files, ..) |
There was a problem hiding this comment.
I think this risk should require more careful consideration of how merges get done from authors other than r-ryantm. My initial proposal is:
- Add some kind of "hold" period (at least 24h) where the mergebot keeps the PR in "will merge" state to let other people look at the changes before they get pushed.
- Make it easy for people to browse through PRs in this state.
- In case potentially malicious changes are detected (or in case any other action is needed to stop the bot from merging), closing a PR, marking it as draft or something similar should make the bot forget about the "will merge" state.
The main concern is "this will make merges take longer!". My view is that the mergebot helps us increase throughput of changes merged, which also has the effect of decreasing latency (they're somewhat connected), and it's ok to have the latency at a minimum of 24h (due to the hold period) because the throughput is still increased and changes are still automatically getting merged.
There was a problem hiding this comment.
we can create a label which the bot tags the PR with and then would do a merge after the holding period. But I think that also conflicts with other peoples workflow. Maybe this is something we want to enable on a per package basis? but not sure how to expose the interface to configure something like that without making it complex
There was a problem hiding this comment.
Can you elaborate on how this conflicts with existing workflows? From my perspective, this mergebot only creates entirely new workflows (non-committers being able to merge changes), so there's no way this can create a conflict.
If a committer wants to get the change merged straight away without waiting for the holding period, they can just merge themselves without the help of the bot, which is the same as the current workflow.
There was a problem hiding this comment.
Ah, I didn't mean existing workflows, but what people told me they would expect by the mergebot. So it wouldn't disrupt current operations but would be a point where opinions will differ :)
There was a problem hiding this comment.
Security will almost always be a trade-off with convenience. I think adding this holding period is a good approach to mitigate the risk that the RFC points out, while not being too complicated to implement. The suggestion of file size limits and valid nix files doesn't seem to be as flexible and cover as many cases. I'd like to get more opinions on this, hoping more people end up commenting.
There was a problem hiding this comment.
hmm, depending on how the bot is manipulated the attacker could just merge the PR directly? So it would be better to have a review log of all the actions the bot has taken instead of relying on a waiting period which could have no effect in an attack?
There was a problem hiding this comment.
Can you elaborate on what attack vector you have in mind, and what bot you're talking about (mergebot or r-ryantm)? I think if there's some case where the mergebot can be manipulated to merge the PR directly when it shouldn't, it should definitely be addressed!
I had the following assumption during all my other comments: r-ryantm's PRs are "safe". There are ways that code upstream can be malicious, but this is a problem outside r-ryantm. r-ryantm itself introducing malicious changes that don't come from upstream is possible, but not something this RFC should worry about.
As for other people introducing malicious changes:
- Currently there's no way those changes get merged automatically or if a maintainer approves them, only when a committer merges them.
- Since there is an existing process to vet new committers, I see this as the baseline for vetting new changes that get merged into Nixpkgs.
- With the new strategies, those changes may be merged by maintainers who aren't committers. This is the issue called out in the RFC.
- So we should find a way to vet those changes. My opinion is that the current proposal (checking file sizes and valid nix files) is not enough vetting: very inflexible, doesn't cover as many cases.
- A holding period of at least 24h ensures that people can vet those changes in a way roughly similar to how new committers are vetted, thus roughly maintaining the baseline.
- So we should find a way to vet those changes. My opinion is that the current proposal (checking file sizes and valid nix files) is not enough vetting: very inflexible, doesn't cover as many cases.
So it would be better to have a review log of all the actions the bot has taken
I don't think that is better, but a complementary approach, since one concerns changes before they're merged and the other concerns them after they're merged, and the actual merging is what's the issue here.
instead of relying on a waiting period which could have no effect in an attack
I think most people would agree that delaying a possible attack by at least 24h seems like a pretty good mitigation. Unless you have something to back your claim, I think it is weak.
Having said all of that, the strongest argument that I can see against the holding period is this:
I think in practice it's unlikely that a recently-committed malicious change will be used right away, since it usually takes at least some hours for the change to propagate to a channel that most people will be using. Therefore this would already serve as a "holding period". However...
In case something malicious gets committed, I see two approaches to "fix" it:
- Rewrite git history to remove the commit completely: better solution to ensure the malicious change doesn't propagate further and is never stored anywhere in other people's machines. But practically impossible to carry this out.
- Revert the commit: less preferable solution, since the malicious changes are still in git history.
Given this, I'd say anything we can do to prevent this from even happening is good.
Not to mention that relying on hydra taking some time to propagate changes to a channel is definitely a very weak mitigation to a potential security issue.
There was a problem hiding this comment.
I'm not against the holding period, it just doesn't fix the security issue addressed in this part of the RFC. What you describe is another issue, that of maintainers merging malicious changes with this bot. What the issue describes is, for example, someone faking the PR in a way that the bot thinks it's a r-ryantm PR (with only pkgs/by-name changes) and merging it therefore directly.
If maintainers merge malicious code, a 24h period could help against that, but then someone would have to check those PRs in the 24h period. The impact also shouldn't be that in big in that case. since the strategy has to be explicitly enabled for a package and if we see malicious action, we either would disable that strategy again on that package, remove the malicious actor from maintainers (and probably all of nixpkgs?) or both. Also we probably would enable maintainer strategies only on low impact leaf packages.
There was a problem hiding this comment.
I've scraped multiple attempts at a comment on this because none actually described what feels off about holding period.
but then someone would have to check those PRs in the 24h period
I think this is the crux of the issue. If 99% PRs are totally benign and get merged with maintainer approval, that already means people are going to learn to ignore those, because there's nothing to see, really.
In case of malicious maintainer, they'll have to make it look benign, possibly collude with upstream, at which point a curious person actively has to go look in the package changelog within the holding period. Even that is sure enough to fail because of the sheer number of times I've seen commit message with 'fix'.
In short, I do not think a holding period is going to stop a malicious maintainer. It will be de-facto annoyance to the rest of the folks who are just waiting for the period to be over.
What the mergebot is intended to do in this RFC, IMO does not get any help from a holding period, whether 1h or 1w.
Just my 2c
There was a problem hiding this comment.
I'm not against the holding period, it just doesn't fix the security issue addressed in this part of the RFC. [...] What the issue describes is, for example, someone faking the PR in a way that the bot thinks it's a r-ryantm PR (with only pkgs/by-name changes) and merging it therefore directly.
Yes, my interpretation of that part of the RFC wasn't the interpretation you intended. I think calling out the issue you describe doesn't make sense. It's essentially saying "there could be a bug in the code". Obviously the code will try to be as robust as possible :)
Regarding the holding period: I still think it has value, but my motivation to argue for it is gone. My last breath: everyone who will benefit from the non-r-ryantm merging strategies already waits a long time in their PRs. I don't have the data (and would love if someone else had), but from anecdotes all over, it already takes over 24h for people's PRs to get merged (which is what I argued before - reducing the >24h wait to ~24h will still make it fast enough and increase throughput). This bot isn't replacing committers, it's increasing merging throughput. Saying that this will be an annoyance given that this won't make any existing PR workflow slower is silly.
|
Previous proposal on this topic #50. |
|
I'd like to nominate for shepard team. |
|
There is also a less common situation where automatic merge may be possible: maintainers updating their own information in |
I'd say that it's fine to leave that out of scope for now. |
|
|
||
| ### Future extensions | ||
|
|
||
| For now we propose that new merge strategies or significant changes to current ones go through their own RFCs. The mergebot team is deciding what is deemed significat in that case. |
There was a problem hiding this comment.
Maybe add that even minor changes of interpretations of the existing strategies should give some amount of notice (repository issues/thread on the dominant discussion platform) before going live?
There was a problem hiding this comment.
yeah, probably new releases on the mergebot repository would be a good idea for this?
There was a problem hiding this comment.
I mention Discourse because it reduces the need to consciously subscribe to yet another thing elsewhere to get notifications.
Another thing is — I think a tracking issue+heads-up thread when the development is starting is better because you can promise to give ten days for feedback with less slowdown (development and testing and preparations for deployment are not just an instant).
Also if a change is not universally perceived as small, when the discussion is known to have time to happen we get a better chance to avoid high-tension discussions.
|
|
||
| ## Unresolved Questions | ||
|
|
||
| *To be completed* |
There was a problem hiding this comment.
If a rev-dep maintainer thinks that the dep's mergebot policy is too lax — do we recommend having a pin-like copy with a different policy, or tightening the policy for the main package?
There was a problem hiding this comment.
I would suggest tightening it for the main package? like all people depending on a package should be fine with the strategy enabled? and duplicating a package seems like overhead
There was a problem hiding this comment.
I agree that this is probably the best trade-off, at least in the beginning, but then it needs to be spelled out in the written policy, as it basically says potentially many people have the right to unilaterally deprive a maintainer of a useful tool.
MMesch
added
the
status: open for nominations
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-04-02/42643/1 |
|
I'd like to self-nominate to be a shepherd. |
|
I'm also nominating myself as a shepherd |
|
alrighty, I'm not sure about the exact process. But we have @0x4A6F @JulienMalka @Mic92 and @infinisil who nominated as shepherd, @Mic92 told me he would be ok with lead. So I guess the next step is to find a suitable date for our first call. If anybody else also wants to join up, please mention it here :) |
|
While we have enough shepherds, until the RFC steering committee officially announces the shepherds, nominations are still open. But that's not a blocker for setting up a call already :) |
|
https://meet.dgnum.eu/rfc172-403705 time to find a date :) |
|
Ok. It looks like the 16:00 UTC, 19.04.2024 is the best date for us for a first meeting. |
| **Shepherd Team:** `[To be nominated and accepted by the RFC Steering Committee]` | ||
| **Shepherd Leader:** `[To be appointed by the RFC Steering Committee]` |
There was a problem hiding this comment.
| **Shepherd Team:** `[To be nominated and accepted by the RFC Steering Committee]` | |
| **Shepherd Leader:** `[To be appointed by the RFC Steering Committee]` | |
| **Shepherd Team:** 0x4A6F, Mic92, JulienMalka, infinisil | |
| **Shepherd Leader:** Mic92 |
Thank you for your nominations!
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-04-16/43512/1 |
RFC meeting summary on the 23.04.2024Attendees: @Mic92 @Lassulus @Scriptkiddi @a-kenji @0xA46F @infinisil Key Topics Discussed:
Decisions and Actions:
Full meeting notes# RFC172 meeting notes2024-04-19 16:00 UTCAttendees: @Mic92 @Lassulus @Scriptkiddi @a-kenji @0xA46F @infinisil next meeting friday in 2 weeks, meetings are aimed fortnightly
Domen (1)
|
| - **automerge_updates:** Merges PRs from r-ryantm automatically if ofBorg checks pass, triggered by PR creation or mention. We strongly recommend to have at least one passthrough test. | ||
| - **automerge_backport:** Automatically merges backport PRs if ofBorg checks pass, triggered by PR creation or mention. | ||
| - **full_consensus:** Requires approval from all maintainers before merging. The bot will notify which maintainers' approvals are pending, triggered by PR approval. | ||
| - **single_maintainer:** Allows a single maintainer's approval to trigger a merge, facilitated by a mention of the bot. |
There was a problem hiding this comment.
Not sure how to feel about this. Wouldn't single_maintainer make it incredibly easy to introduce less-than-desirable code into that maintainer's packages? Currently, committer access is very hard to obtain and requires a good track record that inspires trust (and rightfully so).
Comparatively, to become a maintainer, you have to "just" get a package accepted into nixpkgs. Once that's done, what's to stop anyone from slipping in a bad change unnoticed, due to the huge number of PRs?
There was a problem hiding this comment.
well the maintainer has to introduce that code into his own package. So not sure what the attack vector would be here. like the maintainer probably maintains that package alone (otherwise there would be probably a different merge strategy) and probably introduced it to nixpkgs, so slipping in code is possible through other a bit more obfuscated means. In my opinion the attack vector from outdated packages is far bigger than the ones from maintainers backdooring the packages they maintain.
There was a problem hiding this comment.
Worst case scenario I could think of from the top off my head is someone submits a package for a new crypto wallet, then once it's adopted by a number of users, "secretly" (in a way that no one notices, anyway) introduces code that exposes private keys or siphons off funds. Something similar has happened before in the snap store, so it's not a very far-fetched scenario.
There was a problem hiding this comment.
We discussed the requirements to become a package maintainer as well. They will probably also need to prove some track record. But their scope will be also more limited than full committers, so they can actual less damage than us having to give out commit rights.
There was a problem hiding this comment.
Note that we currently have no policy in place to prevent people from adding themselves to maintainer entries, even if they make no other meaningful changes.
There was a problem hiding this comment.
Sure. This policies we need is what this RFC is about.
There was a problem hiding this comment.
We discussed the requirements to become a package maintainer as well. They will probably also need to prove some track record.
I think such "vetted maintainers" should be in addition to the type of maintainers we have now, not in replacement.
It should stay possible to be a random driveby contributor, as those are also very useful
GetPsyched
added
status: in discussion
and removed
status: open for nominations
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-05-14/45414/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-05-28/46113/1 |
| We propose augmenting package meta fields with a new attribute set that specifies a list of merge strategies. | ||
| The mergebot will execute all listed strategies in parallel and will proceed with the merge upon one successful strategy, contingent on passing ofborg checks as well. | ||
|
|
||
|  |
There was a problem hiding this comment.
I would like for this section/diagram to discuss/handle the situation of the PR contents changing, e.g. if the owner of the PR force-pushes.
Currently the diagram is written under the assumption that the PR contents don't change at all after PR is created.
- If there's no way that this can happen, e.g. because all PRs must be made by
r-ryantm, then that assumption should at least be stated explicitly (note also that might change in the future, mayber-ryantmgets a feature to update PRs automatically to the latest upstream version). - But even better would be if there was explicit transitions in the diagram for what happens if the PR contents change.
- It should also be discussed somehwere whether it's even technically possible to notice PR content changs in a race-free fashion.
In general, the potential threat should be acknowledged that the merge bot makes a decision on a PR state, and then a malicious PR author force-pushes a malicious change to trick the bot to merge something else.
There was a problem hiding this comment.
The current implementation takes the current revision of the PR at the point in time when the merge is triggered.
So for example: someone creates a PR, someones triggers the merge action, then after the merge action is triggered but before the merge is done, the author force pushes to the PR again with some malicious changes. In that case the merge would fail and buildbot would report that (github has this feature, that you can specify a revision in a merge action)
|
|
||
| ### Limitation to `pkgs/by-name` Directory | ||
|
|
||
| To ensure accuracy in processing PRs, only those within the `pkgs/by-name` directory will be considered, due to the challenge of verifying if a PR solely affects the intended files. |
There was a problem hiding this comment.
Should be stated somewhere:
Does the nix language guarantee that a limitation to a certain file (or subtree) prevents me from overriding packages somewhere else in the evaluated package attribute tree?
Or could a malicious users pull tricks to write magic trickery ala
{ pkgs, lib, ...}:
(some Nix builtin that allows injection into the final global attrset here, or some nixpkgs magic attributes that nixpkgs interprets)
(mkDerivation
# ...
)There was a problem hiding this comment.
afaik pkgs/by-name limits exactly that. so there are no side effects on other packages.
|
Is a review bot in scope for this as well? |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-06-10/46817/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-06-24/47589/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-07-08/48678/1 |
|
As far as I know, the hardest package to review is the desktop application. Unluckily, it is the most important problem that the RFC want to solve. Because they updates too quickly and makes nixpkgs too many PRs. However, I do not think we should provide VNC to maintainers because the desktop applcation is always with many features like audio and so on. The easy VNC can't solve this problem. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-08-05/50170/1 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/rfcsc-meeting-2024-08-19/50831/1 |
|
drafting this until we have time gain (post NCA), in september or october :) |
Rendered