Skip to content
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

Jump to bottom

gnome3.gnome-settings-daemon: fix #14168 #57620

Merged
merged 1 commit into from Apr 30, 2019
Merged

gnome3.gnome-settings-daemon: fix #14168 #57620

merged 1 commit into from Apr 30, 2019

Conversation

worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Mar 14, 2019 •
edited

Motivation for this change

Originally from #54584 then got moved to the 3.32 pr.

However GitHub hides long discussions in prs that get really long, like the one's for major GNOME3 updates. So putting bugfixes in there, where discussion is important to be easily accessible, isn't a good idea.

This is blocked by #57027 because it depends on code added
in that release.

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.

@GrahamcOfBorg GrahamcOfBorg added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 8.has: module (update) labels Mar 14, 2019
hedning
hedning reviewed Mar 14, 2019
View reviewed changes
nixos/modules/services/desktops/gnome3/gnome-settings-daemon.nix Outdated
@@ -40,6 +40,8 @@ in

services.udev.packages = [ cfg.package ];

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

Choose a reason for hiding this comment

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

I'm getting this when running the wrapper:

❯ gsd-backlight-helper
This program can only be used by the root user

So we're hitting this for some reason: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/blob/master/plugins/power/gsd-backlight-helper.c#L65

Copy link
Member

Choose a reason for hiding this comment

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

Is this resolved with the latest version of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this alone for a bit because we just need to get 3.32 on master first.
Then debugging would be quick.

Though I'm not sure why we're getting https://gitlab.gnome.org/GNOME/gnome-settings-daemon/blob/master/plugins/power/gsd-backlight-helper.c#L65

worldofpeace added a commit to worldofpeace/nixpkgs that referenced this pull request Mar 15, 2019
@worldofpeace
pantheon.elementary-settings-daemon: bypass polkit by using an suid w…
b30f6fa 
…rapper

Same as NixOS#57620
@jtojnar jtojnar mentioned this pull request Apr 5, 2019
Closed
@jtojnar jtojnar added this to In Progress in GNOME Apr 5, 2019
@worldofpeace
Copy link
Contributor Author

So I've looked and I don't know why this isn't working.
I locally added setuid = true; and user, group to root, and with tests getuid () still returned 1000 for me.

Think something like worldofpeace@c744359 would work?

@hedning
Copy link
Contributor

hedning commented Apr 22, 2019

Yeah, seems like getuid() simply returns the actual user id (as oppsosed to geteuid() which returns the effective id) so we'd have to patch gsd for it to be used with setuid.

Seeing that the problem is a new polkit policy file, but an old gsd application (#14168 (comment)) your fix should hopefully work. Seems sane to me 👍

@worldofpeace
Copy link
Contributor Author

Yeah, seems like getuid() simply returns the actual user id (as oppsosed to geteuid() which returns the effective id) so we'd have to patch gsd for it to be used with setuid.

Strangely enough the effective id would only be 0 if I removed the layer of wrapping from wrapGAppsHook
(i think).

Seeing that the problem is a new polkit policy file, but an old gsd application (#14168 (comment)) your fix should hopefully work. Seems sane to me +1

Right, think I'll go with that since we'll still be able to use polkit.

@worldofpeace worldofpeace changed the title gnome3.gnome-settings-daemon: bypass polkit by using an suid wrapper gnome3.gnome-settings-daemon: fix #14168 Apr 22, 2019
hedning
hedning approved these changes Apr 22, 2019
View reviewed changes
Copy link
Contributor

@hedning hedning left a comment

Choose a reason for hiding this comment

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

Haven't tested, but LGTM

jtojnar
jtojnar reviewed Apr 22, 2019
View reviewed changes
pkgs/desktops/gnome-3/core/gnome-settings-daemon/default.nix Outdated
# So the polkit policy can reference /run/current-system/sw/bin/gsd-backlight-helper
postFixup = ''
mkdir $out/bin
ln -s $out/libexec/gsd-backlight-helper $out/bin/gsd-backlight-helper
Copy link
Contributor

@jtojnar jtojnar Apr 22, 2019 •
edited

Choose a reason for hiding this comment

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

Can we use some other path that is guaranteed to be linked but not in the PATH?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a subdirectory of /bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing bin/gnome-settings-daemon.

Copy link
Contributor

@hedning hedning Apr 23, 2019 •
edited

Choose a reason for hiding this comment

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

Could pathsToLink = [ "/libexec/gsd-backlight-helper" ]; in gnome-settings-daemon.nix work? Seems somewhat cleaner to me, though not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thought of that too. It'd be cleaner but I don't see it happening soon where gnome-settings-daemon wouldn't be in environment.systemPackages. (thusly needing that).

@ofborg ofborg bot requested review from jtojnar and hedning April 22, 2019 20:29
@worldofpeace
gnome3.gnome-settings-daemon: fix NixOS#14168
f6e9229 
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.

By patching references to the backlight helper to
/run/current-system/sw/bin/gsd-backlight-helper
the polkit policy will not undergo a change.
@worldofpeace worldofpeace merged commit 44bf322 into NixOS:master Apr 30, 2019
GNOME automation moved this from In Progress to Done Apr 30, 2019
@worldofpeace worldofpeace deleted the gnome-brightness-fix branch April 30, 2019 17:14
worldofpeace added a commit that referenced this pull request May 5, 2019
@worldofpeace
pantheon.elementary-settings-daemon: fix #14168
bb4f35b 
Is the same fix as in #57620 just for Pantheon's settings daemon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil Awaiting requested review from infinisil

@jtojnar jtojnar Awaiting requested review from jtojnar

@hedning hedning Awaiting requested review from hedning

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 11.by: package-maintainer
Projects
GNOME
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@worldofpeace @hedning @jtojnar @infinisil @GrahamcOfBorg