nixos/polkit: modernize, make pkexec wrapper opt-in#530106
Conversation
Review dismissed automatically
3f10524 to
d9aab67
Compare
|
Actually sure looks like Plasma doesn't need it: https://github.com/search?q=org%3AKDE%20pkexec&type=code (but the installer image still does, to launch Calamares) |
thefossguy
left a comment
There was a problem hiding this comment.
For the COSMIC module:
- enablePkexecWrapper = true;
+ # pkexec requires setting values of at least `XDG_RUNTIME_DIR` and `WAYLAND_DISPLAY`
+ # and these values are not sourced automatically on wayland-only sessions
+ # this can only be handled manually, AFAIK, like so:
+ # `env XDG_RUNTIME_DIR=$XDG_RUNTIME_DIR WAYLAND_DISPLAY=$WAYLAND_DISPLAY pkexec $args`
+ # xwayland is the default, and a wayland-only session is an opt-in
+ # therefore, if the user has opted out of xwayland, let the user explicitly handle this
+ enablePkexecWrapper = lib.mkDefault cfg.xwayland.enable;The module now enables polkit, which run0 requires to faciliate elevation. This warrants guarding the config by an opt-in enable toggle. For the options that existed prior to the enable toggle we now assert that users need to opt into the module for them to have an effect.
Calls pkexec in src/grd-ctl.c.
The run0 module now enables polkit and properly reflects the intent behind the `enableRun0Elevation` option.
We concluded this is fine, because we don't require elevation while switching generations. Co-Authored-By: r-vdp <ramses@well-founded.dev> Co-Authored-By: Grimmauld <Grimmauld@grimmauld.de>
|
nixpkgs/nixos/tests/lomiri.nix Lines 670 to 672 in a04cdfd So turns out lomiri expects Edit: oops, seems i was a second later than the push with that |
LordGrimmauld
left a comment
There was a problem hiding this comment.
Alright. Ran the tests, the breaks i ran into all have either been fixed or were caused at some earlier time outside this PR. I agree with the approach and the motivation.
I do expect this to break some peoples system. I think this is acceptable and necessary in this case. I am happy with this PR, though i am not quite sure how trigger-happy i should be with the merge button here. First time i knowingly am approving something i expect will break quite some setups.
We got a go-ahead from cosmic, gnome, plasma afaict. Not sure who else we would need/want to wait for.
|
Some more potential users:
|
|
I noticed that login in COSMIC was timing out. We have NixOS VM tests that handle session login (via the first-party greeter) and autologin. If in future a similar problem is encountered, I suggest you try to build all 4 On that note, did you figure out why the login was timing out? |
|
|
thefossguy
left a comment
There was a problem hiding this comment.
Sorry about the noise. I ran the NixOS VM tests on this PR and they pass. The above review results are of this PR.
I have an open PR to refactor the tests and add a new test for polkit using pkexec. Created a new PR (#530915) with your changes on top of my changes and they pass there as well.
LordGrimmauld
left a comment
There was a problem hiding this comment.
Alright, lets do it. Some things probably will break, but considering this change is necessary and fixing the fallout is trivial, i see no good reason to delay this.
The motivation here is that enabling polkit is required for run0, but at that point you don't want pkexec unless you need to.
I grepped for every service that touches
security.polkitand if it's package source mentioned pkexec it got the opt in commit.Is this a follow-up to #156858? You tell me!
cc @K900 for Plasma, because I have no idea where to grep for pkexec.
thanks @theCapypara for providing some info for GNOME already.
Tests run:
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.