-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
nixos/espanso: provide required capabilities for espanso-wayland #423931
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
Conversation
|
Thanks for taking another try at this, @n8henrie. The changes look good to me, but I currently don't have access to a NixOS Linux system (not for a few weeks), so I won't be able to test this for the moment. Would be great to finally have an espanso-wayland that works out of the box :). |
|
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/5662 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/2503 |
|
gentle ping, in case this slipped away |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/espanso-daemon-problem/35309/28 |
|
Haven't had much luck getting reviews in the months since this PR was created. Pinging maintainers from the home-assistant module. Do any of you have access to a wayland system to test and possibly review? Would love to get espanso working on wayland by default! |
bobvanderlinden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a try. I haven't been using espanso actively for a while, but it was already working for me on Hyprland with with the home-manager module. I do indeed see:
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] EVDEV backend is being used, but without enabling linux capabilities.
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [INFO] monitoring the status of the daemon process
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] Although you CAN run espanso EVDEV backend as root, it's not recommended due
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] to security reasons. Espanso supports linux capabilities to limit the attack surface
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] area by only leveraging on the CAP_DAC_OVERRIDE capability (needed to work with
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] /dev/input/* devices to detect and inject text) and disabling it as soon as the
Oct 20 23:40:16 nac44250 espanso[29696]: 23:40:16 [worker(29696)] [WARN] initial setup is completed.
I can confirm that when I have in configuration.nix:
services.espanso = {
enable = true;
package = pkgs.espanso-wayland;
};
Now the log shows: using EVDEVInjector without the errors.
So to me this PR seems like an improvement already. It avoids the errors. Merging makes sense.
I am wondering: is this only usable for espanso-wayland? Why not always create the wrapper, regardless of package == espanso-wayland?
|
Thanks for the review!
Interesting -- I'll need to look again, I only use the hm module on Darwin, but I don't see why it would work through HM; I would expect Wayland to fail there as well due to the lack of capabilities etc. As to only applying the wrapper on Wayland -- that was just because they're only required on Wayland. Figured X11 users might not want the small incremental security cost of an extra capabilities-enabled binary on their system if it didn't give them any extra functionality. |
|
@bobvanderlinden it is definitely failing for me with Arch on wayland (sway) with a home-manager-only setup. Still working on getting Hyprland to run -- darn nvidia GPU issues. |
|
Integration tests added in #454248 -- once this PR is merged, the wayland test passes. |
| patchPhase = | ||
| old.patchPhase or "" | ||
| + '' | ||
| substituteInPlace espanso/src/cli/daemon/mod.rs \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be moved inside the package definition, behind a flag (I lack the creativity for a proper name, but something like inSecurityWrapper). Also, use postPatch instead of patchPhase for such substitutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better, the flag could be the path to the exe, e.g.
{ ..., securityWrapperPath ? null }:
stdenv.mkDerivation {
...
postPatch = lib.optionalString securityWrapperPath != "" ''
substituteInPlace espanso/src/cli/daemon/mod.rs \
--replace-fail \
'std::env::current_exe().expect("unable to obtain espanso executable location");' \
'std::ffi::OsString::from("${securityWrapperPath}");'
'';
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niklaskorz Maybe a little more discussion on this one -- I specifically did not include this in the package because the package itself is also run on non-NixOS linuxes and should not have this applied in that case. In my mind, this workaround should only be applied through the NixOS module (as that is the only case in which the security wrapper script will exist).
Does that make sense? Do you think there's another path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I suppose the securityWrapperPath should be assumed to be null in those cases, so it shouldn't affect them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my suggestion is mainly meant to make it easier to fix the patch on updates, having it all in one place.
92b309e to
3427bc6
Compare
|
Migrating the new nixostests to this PR, since this should establish Wayland as working. Will close #454248 when / if this is merged. |
33cf7d2 to
3868462
Compare
3868462 to
e647f9d
Compare
|
Looks like you accidentally included two unrelated commits with the last rebase. |
|
Ugh, sorry and thanks. |
|
Working on changing the package definition as suggested. Trying to test locally before I re-push. |
8732792 to
c92c38c
Compare
|
For some reason the nixosTests aren't working from my aarch64-darwin machine anymore. Will need to do a little more debugging once I get home to a linux machine -- hopefully this evening. |
c92c38c to
b526cc2
Compare
af6c070 to
f02e2e6
Compare
f02e2e6 to
dff1dee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests pass, changes look good. Might be preferable to fetchpatch the package fix instead but I'll leave that up to you.
69802ab to
6267196
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/6 |
On wayland, espanso relies on capabilities for its worker process to function.
Unfortunately it forks itself using
current_exe()to find the binary, which resolves to the capability-lacking binary in the store instead of a wrapped binary provided bysecurity.wrappers.Extensive discussion of the problem and alternative solutions in #328890.
As compared to that PR, which has not made much progress, this is a dramatically simpler approach that seems to work well.
Happy to incorporate constructive criticism, as I am not terribly familiar with
security.wrappersand suspect there may be better ways to e.g. find the wrapped espanso, test forpkgs.espanso-wayland, etc.Fixes #249364
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.