-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/espanso: fix wayland problems due to missing capabilities #328890
base: master
Are you sure you want to change the base?
Conversation
Thanks for your work on this! This adds a considerable amount of complexity, and I'm a little concerned about how it will hold up over time (as context, espanso is having some growing pains and may be having some significant refactoring in the near future). As far as I can tell, As an alternative, it seems that we could just move the service to run as root (and likely implement some of the numerous systemd-level protections to keep services well confined) and have a considerably simpler service module that -- AFAICT -- has effectively the same security concerns, but would have the benefit of keeping its systemd "jail" even as the package is updated over time. Assuming that espanso could (in theory) read / write / change any file on the entire system before lowering its effective privileges, what do you see as the advantage of keeping this a user service instead of system-level / root? |
Thanks for the reply! :) I totally see the concerns regarding the complexity and that running this as a system-level service might be the more maintainable solution (I am not too familiar with the additional systemd level protections). My main motivation for this route (in contrast to using it as a privileged system service) was to stay as close to the intended installation of espanso as possible. And, from my understanding of the difference between system/user level services (which might be incomplete/wrong), I tend to avoid running system level services for tasks used by probably only a few users. In particular, a text-expander seems like something that is very user-specific and into which a user should have to explicitly opt-in. If this is run as a system service, users might be unaware of the fact that there is a service running that could, in principle, capture their keystrokes. |
I think your points are fair but also might miss my point:
From what I understand, setting these capabilities in effect gives this service the ability to capture and modify all content on the device -- including those same keystrokes, so I don't think it makes a difference. With these capabilities you are basically running espanso as One thing I'm unsure of is WRT multiple user-level installs; the espanso config usually goes somewhere in Other ideas perhaps worth looking into (you may have already done this): the
|
I think concerning the possibility to abuse CAP_DAC_OVERRIDE, we're on the same page. I absolutely agree that giving the user service those capabilities means it could exploit the unrestricted access to do more or less what it wants. The benefit of CAP_DAC_OVERRIDE is over
The difference that I see is that on a multi-user system, the system service would still be running in the background even for users that do not use espanso. As a user service, it is a true opt-in and a user not knowing espanso would not have to wonder whether espanso (or any of its dependencies) is trustworthy enough to have it listing on keystrokes. Even if a user knows and wants to use espanso, they might not want to have it running as a service but decide to run it when needed via
I indeed stumbled over these but decided against them since I still think running this as a user service is more desirable. But I agree that if one wants to run espanso as a system service, this is the way to go. About your concerns wrt multiple user-level installs: If espanso is run as a user service by systemd, that should be fine ( I guess one could also completely decouple the Anyway, I totally get it if you think the maintenance burden of this might be too large to justify the possibility of running espanso manually or as a pure user-level service. I'd still try to polish this a little bit (feedback is welcome!) in case someone else wants to use this in their own nixpkgs fork. |
6d7317a
to
eefb7da
Compare
@n8henrie FYI: I did refactor this quite a bit. It is now simply a single, new module At the least, this should make it much easier if anyone wants to use this, even if this is not added to nixpkgs. |
I've been thinking about this a fair amount but haven't come up with any better solutions. And to be frank, you seem much more knowledgeable than me about much of the details -- part of my hesitation is my poor familiarity with C and the C wrapper (simple as it may be). Part of my wonders -- wouldn't it make sense for the wrapped package to be the default espanso on Linux? It seems like the If espanso without the wrapper doesn't work, wouldn't it be best to make the wrapped espanso the only option? |
Actually, the |
I'm not too sure myself whether this is the right solution, but so far it is the best I could come up with to get it to work in user-space. Concerning the C wrapper: I stumbled upon this only recently, but it has been used in nixpkgs at other places, especially for binary drivers that sometimes contain hardcoded firmware file locations (see e.g., PR #260715). I'm using a similar wrapping library in another PR (#326272), also for a binary driver. But given that this PR involves granting extra capabilities, it would clearly be good if someone more experienced with such things could take a look at the wrapper library and maybe comment whether this is a reasonable approach. I wonder whether maybe @robryk might want to take a look at this. Quite some commits on Concerning making the wrapped, capability-enabled espanso the default on Linux: In principle I would agree. One possible problem: I cannot see an implementation as a package only. The wrapped binary must know the Then one should include a comment in the description of |
Initial comment after a quick skim: why doesn't the capability get inherited by the child process? (I don't remember the inheritance rules by heart, and they are somewhat weird given that they're trying to handle a very weird model, but the wrappers execs the actual process and that execs ends up preserving capabilities, so I would expect the next one to also do so.) Concrete questions: do you know that this doesn't "just work" with wrappers? |
@robryk Thanks for taking a look and the quick response! I did indeed try to simply use the wrapper, without any success. That said, I'm using Linux only occasionally and am not too familiar with the capability framework, so it is very well possible that I simply did overlook something. Specifically, I tried the following wrapper for espanso under wayland:
If I start espanso with this via Before I did this implementation, I had a quick look at the capability inheritance rules in the man page and also at how these are transformed during a fork (under "Transformation of capabilities during execve()" in the man page). I admit that I might very well be misunderstanding something, but here is what I think might be the problem:
Again, take this with a grain of salt, since I neither have much experience with the linux capabilities nor with Rust (so, I might be misreading what espanso is doing). |
The capsets in the wrappers are a bit of a lie anyway (due to the weirdnesses I mentioned earlier it's impossible to simulate filecaps on the target file with a wrapper), so you're getting more than you asked for usually anyway. I'll check whether the story you're painting is correct later, but it both sounds plausible and would explain the issue. Give me some time to think about the least weird way to deal with this. |
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
9474876
to
55e1e44
Compare
Just updated the PR draft, doing some cleanup and incorporating points from several discussions with @n8henrie (in particular this comment in the related PR #339594 that uses an alternative approach to fix the same issue). The wrapper is now only used for |
55e1e44
to
c751dbd
Compare
981db6d
to
c822ea7
Compare
BTW: I'm not sure whether a module-generated overlay to override a package is a good idea, especially if enabled by default. It was just an idea to have this automatically pick up by anything using Instead of enabling this by default, it seems to make most sense to have it disabled by default but enable it in the NixOS Espanso module whenever that is enabled. Unfortunately, we cannot enable this from the HM Espanso module (I think), so HM users would have to enable it manually in their NixOS system configuration. |
6902c97
to
67b9308
Compare
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 |
67b9308
to
74650d7
Compare
I removed the draft status of this PR and did some minor cleanups in the description and code. In particular, the module is now not enabled by default. @n8henrie Did you have time to think about this and/or your approach in #339594? I thought again a bit about the general issue during the last couple of days but couldn't come up with another, more elegant way that keeps Espanso's monitoring capabilities. But still, it is probably completely fine to use #339594 instead if we don't want the extra monitoring capabilities. Would be nice if we could get a fix for espanso-wayland (either this or #339594) into the 24.11 NixOS release 🙂 (if possible, not sure about the time frame…). |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/home-managers-espanso-module-does-not-create-config-directory/51170/14 |
74650d7
to
7a36827
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/using-espanso-on-wayland-nixos/55268/2 |
7a36827
to
22e99a9
Compare
I think this is looking better and more palatable over time. I still wonder if a more general fix could be implemented at the @pitkling thanks for all the effort you've put into this and for your excellent communication. |
@n8henrie: It's still on my todo-list to investigate Some thoughts/questions we should probably still discuss (even though they are not necessarily part of this PR):
|
22e99a9
to
3361df7
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4910 |
Now I have, totally forgot about that thread before 😅. @robryk: Just pinging you since you took already a quick glance at this some time back. If you have time to review this and give your opinion would be great, but no worries if not. |
Description of changes
This PR fixes #249364
(which I assume is actually not only Wayland-specific). When enabled viaprograms.espanso.capdacoverride.enable = true
, it injects an overlay that overridespkgs.espanso-wayland
with a package that has the DAC_OVERRIDE capability.With this in place, espanso can be used under Wayland (both in NixOS and Home Manager) via the standard way:
There is also a
programs.espanso.capdacoverride.package
option that allows to set which espanso-wayland is used as a base for the DAC-OVERRIDE-enabled package version. For example, if you pulled in espanso-wayland from nixpkgs-unstable under pkgs.unstable.espanso-wayland), you can set:If this module is added, one should probably add a note to the
services.espanso
module mentioning that this must be enabled for Wayland (or it won't work). Alternatively, one could detect automatically whether the user wants to use an espanso-wayland package and activate this module automatically.Detailed Description
Under Wayland, Espanso requires the DAC_OVERRIDE capability (see the documentation). NixOS does not support capabilities in the Nix store but instead provides a framework to create wrapper binaries with suitable capabilities. While the wrapper does some work to maintain those capabilities across forks, this is not enough in the case of Espanso, which drops the extra capabilities early on and relies on file capabilities to regain those capabilities when forking a worker process (via
/proc/self/exe
). Unfortunately, the symlink under/proc/self/exe
does not point to the capability-enabled wrapper but to the original binary in the nix store.Thus, this PR wraps the original espanso-wayland package definition and injects a wrapper library that intercepts the
readlink
-call and – if it looks up/proc/self/exe
– returns the capability-enabled wrapper. Sinceespanso-wayland
is broken without those capabilities, the wrapped espanso-wayland package is then overlayed (unless explicitly disabled viaprograms.espanso.capdacoverride.enable = false
) overpkgs.espanso-wayland
(making the originalpkgs.espanso_wayland
available underpkgs._espanso-wayland-orig
).Note that the updated package definition must be created on-the-fly during evaluation of the system configuration, since the path to the capability-enabled wrapper depends on the
security.wrapperDir
.This has been working
pretty wellflawlessly on my system for severalweeksmonths. It seems currently the only workaround for #249364 that keeps all of Espanso's features. For example, there is also #339594, but with it we lose Espanso's self-monitoring (which is used, e.g., for automatically re-loading the configuration when it changes). Happy about any feedback (especially from the corresponding maintainers @kimat @pyrox0 @n8henrie @numkem) to improve this if you think this is a viable way forward.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.