nixos/espanso: add required capabilities for wayland #339594
Conversation
On Wayland, Espanso depends on `cap_dac_override+p` for the EVDEV backend. Specifically, this capability is required by the `worker` thread, which is forked from the main espanso process when run by the usual means (`espanso start` or `espanso daemon`). Espanso (responsibly) drops capabilities as soon as possible, prior to forking the worker process. Unfortunately, `security.wrappers` sets the capabilities in such a way that the forked process does not pick up these capabilities (due to `/proc/self/exe` pointing to the original espanso binary, which does *not* have these capabilities). By running `worker` directly from the capability-enabled wrapper, the worker thread is able to run without issue, and Espanso runs as expected on wayland. - NixOS#249364 - NixOS#328890 - https://espanso.org/docs/install/linux - fixes NixOS#249364
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
|
Continuing the discussion from #249364 (especially this comment) here, to avoid spamming that issue unnecessarily. First of all, thanks for digging further into this approach! It's definitely worth pursuing and might be a good alternative to PR #328890. From your issue description, I get it that the CAP_DAC_OVERRIDE requirement of espanso is a wayland-only thing? Is that documented somewhere? Just asking, since in that case I'll update my draft PR #328890 accordingly. (quotes from #249364, this comment)
True enough, there won't be problems until we update the derivation. I'm mostly worried about upstream changes in the backend that we are unaware of and that do not cause build problems. E.g., the espanso team might keep the worker subcommand but add additional features relying on some monitor process managing the worker. We would not spot those problems on a derivation update but they could cause problems much later that might be difficult to figure out.
Good point, that definitely helps. Still, there must be more to it, right? Otherwise, why would the espanso team not make use of this themselves in their reference systemd service?
That's true, it's not possible to avoid such issues. I'm just worried that this approach is more prone to these problems since (a) it relies on the seemingly not-well documented workings of an internal subcommand and (b) we might not even spot problems until much later when updating the derivation (as I mentioned above). It seems to me that the approach via the wrapper library from #328890 (which definitely has its own problems!) has the benefit on not relying on internals (at least not much; I don't think it's likely that the espanso team changes process forking in a way that is not captures by the wrapper library) and (b) if there are upstream changes breaking that approach, we should realize this very quickly (not necessarily at build time, but the process will most likely not work at all, as it is currently the case at least on wayland). Btw., I will sometime soon (currently somewhat busy) create a issue for discussing whether such problems can be handled directly by security.wrappers, as you suggested here. But I think we should implement a fix for #249364 (whether this PR or #328890) anyways, since I think even if security.wrappers can handle this, it will take some time to implement such a change. |
I think so -- I linked to the linux installation instructions above, capabilities are only mentioned in the wayland section.
Certainly possible, but I think you might be over-worried here. I am involved in espanso development and intend to become more involved over time. Similarly, I'm listed as a maintainer of the espanso package and module; with any luck I'll be able to help watch for issues here going forward. Can you elaborate on a worst-case scenario in your mind? I want to make sure we're speaking the same language; in my mind, you're suggesting that:
Is that more-or-less the risks that concern you? As a hobbyist self-taught programmer, sometimes I am completely blind to the concerns that professionals assume I am taking into account. * perhaps we could consider adding a nixos test for espanso?
I might be misunderstanding -- their reference service does leverage systemd's automatic service restarts: Also -- for context -- espanso's creator and primary dev @federico-terzi has done an amazing job but has openly noted that he is not going to be able to focus on espanso for the time being (espanso/espanso#1742); @AucaCoyan and others have stepped up, but issues like espanso/espanso#1953, espanso/espanso#1963, and many others show that there is still a lot of work to be done to even get development back on track. I only bring this up to point out that they are an extremely welcoming community that is hungry for additional contributors; I still think that an upstream patch should be considered if we can come up with a strategy that would ameliorate nix issues and be a win (or net even) for the espanso team. They've showed a lot of flexibility working with 3rd party package management in the past (espanso/espanso#615) and are generally very nice people. |
Ahh, you're right. I'll update my PR draft accordingly.
Yes, that's basically it. In general I think it's more robust to stick to openly documented interfaces, to keep maintenance low. That's also why I still think the approach from #328890 requires probably lower maintenance of the NixOS module code (major concern for #328890) than the approach here. But given that you're involved in espanso's development definitely weakens those arguments.
Yes, but they're running |
Trying to test to confirm (on an Arch host running i3 over VNC, with espanso built from source), espanso runs and seems to try to expand without any additional capabilities. Unfortunately it succeeds at deleting the trigger text but the replacement text doesn't appear; I suspect this is an issue with the clipboard and VNC. It is definitely different than the "crash at launch" that I get on Wayland when trying to run without capabilities. Also, regarding your PR -- IMO users installing espanso with
Did you mean to put the same PR for both of those links? Assuming so -- that's interesting, I still worry that the complexity of that approach would make it far more difficult (for me! though perhaps not for you or hypothetical maintainers) to debug / maintain when something eventually goes wrong. Even if it isn't the ultimate cause of a bug, when a bug arises this code should be considered as part of the machinery that is under examination -- and I don't think I could do that well. I am reasonably comfortable with nix and rust, which is why I thought I could help on the respective teams, but I never learned the C family! That said, your explanatory comments in the code are wonderful and give me a fighting chance at understanding the approach you've taken.
Correct -- and this is part of my point. Both NixOS and Home-Manager modules are currently using So whichever way we go, just by using nix, I think we're already pretty far off the well documented path. It seems that @KenMacD has been using the |
I just did update #328890. It is now its own small module (BTW: I also just tested myself that running
I absolutely get your reservation, and if you don't feel those changes are worth the hassle, that's fine :). If it helps, I'd be willing to maintain that part if wanted. BUT I should say that I have never written a single line of Rust code, so not sure how helpful I can be with issues related to Espanso internals. But yes, I think it's lower maintenance (I meant PR #328890 in both cases). I think it is pretty orthogonal to any changes to the espanso package and module. I cannot see many upstream changes that would cause hiccups with the wrapper; the most likely breaking change would be that Espanso no longer uses the libc-based fork, but then the wrapper just behaves as the original. Also, one can easily disable the wrapper completely via Note that the way #328890 is implemented now does neither touch the Espanso package definition nor the Espanso module code directly. The way I wrap the Espanso package in
Ah, I wasn't aware that |
| }; | ||
| config = | ||
| let | ||
| wayland = cfg.package == pkgs.espanso-wayland; |
There was a problem hiding this comment.
It might be better to do something like
| wayland = cfg.package == pkgs.espanso-wayland; | |
| wayland = builtins.elem "wayland" cfg.package.buildFeatures; |
Otherwise this will probably not work if one pulls an espanso-wayland package from another nixpkgs branch (e.g., if one runs nixos-2405 but pulls espanso-wayland from nixpkgs-unstable).
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/espanso-daemon-problem/35309/27 |
|
Closing in favor of #328890 |
On Wayland, Espanso depends on
cap_dac_override+pfor the EVDEVbackend. Specifically, this capability is required by the
workerthread, which is forked from the main espanso process when run by the
usual means (
espanso startorespanso daemon).Espanso (responsibly) drops capabilities as soon as possible, prior
to forking the worker process. Unfortunately,
security.wrapperssetsthe capabilities in such a way that the forked process does not pick
up these capabilities (due to
/proc/self/exepointing to the originalespanso binary, which does not have these capabilities).
By running
workerdirectly from the capability-enabled wrapper,the worker thread is able to run without issue, and Espanso runs as
expected on wayland.
espanso-wayland: service start failure #249364
nixos/espanso: fix wayland problems due to missing capabilities #328890
https://espanso.org/docs/install/linux
fixes espanso-wayland: service start failure #249364
Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.