Skip to content

gnome3.gnome-settings-daemon: bypass polkit by using an suid wrapper #54584

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

gnome3.gnome-settings-daemon: bypass polkit by using an suid wrapper #54584

wants to merge 2 commits into from
+51 −2

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

This is intended to fix #14168 untli we have a better solution from
upstream GNOME.

cc @jameysharp @hedning @jtojnar

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@worldofpeace worldofpeace changed the title gnome3.gnome-settings-daemon: bypass polkit by using an suid wrappe gnome3.gnome-settings-daemon: bypass polkit by using an suid wrapper Jan 25, 2019
@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 25, 2019
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Jan 25, 2019
@worldofpeace
Copy link
Contributor Author

worldofpeace commented Jan 25, 2019 •
edited
Loading

🤔 Is pruning the polkit service away needed so it's not in the environment?

@jtojnar
Copy link
Member

jtojnar commented Jan 25, 2019

🤔 Is pruning the polkit service away needed so it's not in the environment?

What do you mean?

@worldofpeace
Copy link
Contributor Author

thinking Is pruning the polkit service away needed so it's not in the environment?

What do you mean?

Is removing $out/share/polkit-1 needed?

@jtojnar
Copy link
Member

jtojnar commented Jan 25, 2019

I do not think removing it is necessary – single unused field in some hashmap will not cause any problem.

@worldofpeace
gnome3.gnome-settings-daemon: bypass polkit by using an suid wrapper
a9776a1 
This is intended to fix NixOS#14168 untli we have a better solution from
upstream GNOME.

The issue in brief is caused by the fact that gnome-settings-daemon is
non-restartable and if the policy definitions change in any way
it won't propagate to the running system because of that.

Also take note that we're abusing a debugging envar
`GSD_BACKLIGHT_HELPER` as it was never intended
for use outside testing.
@worldofpeace
Copy link
Contributor Author

Did the thing, also built and ran a vm.

This probably needs to be tested on a running machine though.

@GrahamcOfBorg GrahamcOfBorg requested a review from jtojnar January 26, 2019 02:19
@jameysharp
Copy link
Contributor

Looks like a sensible pair of commits to me! I have not tested it yet though and I could well have missed important details.

I have this vague feeling that the entire preFixup hook should only happen when the NixOS module is what's using the gsd package, since otherwise there's no /run/wrappers copy of the helper. I'm not sure if Hydra would build and cache the wrapped version then though? Either way, I wouldn't block merging this PR over that question.

@jtojnar
Copy link
Member

jtojnar commented Jan 26, 2019

We could create the wrapper in a separate derivation like we do with plug-in wrappers (e.g. gimp/wrapper.nix). But that would not really help, as the module is not what is running it, gnome-session is.

@worldofpeace
Copy link
Contributor Author

Looks like a sensible pair of commits to me! I have not tested it yet though and I could well have missed important details.

I have this vague feeling that the entire preFixup hook should only happen when the NixOS module is what's using the gsd package, since otherwise there's no /run/wrappers copy of the helper. I'm not sure if Hydra would build and cache the wrapped version then though? Either way, I wouldn't block merging this PR over that question.

I'll try to test this later today. I've abused debugging features like this in the past and it generally goes well.

@grahamc
Copy link
Member

grahamc commented Jan 26, 2019 •
edited
Loading

@GrahamcOfBorg test gnome3

grahamc
grahamc reviewed Jan 26, 2019
View reviewed changes
nixos/modules/services/desktops/gnome3/gnome-settings-daemon.nix

security.wrappers.gsd-backlight-helper.source = "${pkgs.gnome3.gnome-settings-daemon}/libexec/gsd-backlight-helper";

nixpkgs.overlays = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably don't want to be setting overlays in NixOS modules.

Copy link
Contributor Author

@worldofpeace worldofpeace Jan 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same thing, but @jtojnar thought it would be an improvement.
Not sure if hydra will cache it and I don't have any details to support against it.

@worldofpeace
Copy link
Contributor Author

@GrahamcOfBorg build nixosTests.gnome3

@worldofpeace
Copy link
Contributor Author

So I just realized that the code that allows us to do this in is 3.31.
I could just patch this feature into the latest release.

This was referenced Mar 1, 2019
Merged
Merged
@worldofpeace
Copy link
Contributor Author

Is part of #57027

@worldofpeace worldofpeace mentioned this pull request Mar 14, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc

@infinisil infinisil

@hedning hedning

@jtojnar jtojnar

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rebuild causes sudo to be required to adjust screen brightness
5 participants
@worldofpeace @jtojnar @jameysharp @grahamc @GrahamcOfBorg