Skip to content

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

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
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
+33 −0

Conversation

worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Mar 14, 2019 •
edited
Loading

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.

Sorry, something went wrong.

@worldofpeace worldofpeace requested a review from infinisil as a code owner March 14, 2019 02:19
@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 Mar 14, 2019
@worldofpeace worldofpeace added the 2.status: work-in-progress This PR isn't done label Mar 14, 2019
@GrahamcOfBorg GrahamcOfBorg added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 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 worldofpeace force-pushed the gnome-brightness-fix branch from bd67df9 to 4b0ebf1 Compare March 15, 2019 20:32
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
@GrahamcOfBorg GrahamcOfBorg requested a review from hedning March 15, 2019 20:45
@jtojnar jtojnar mentioned this pull request Apr 5, 2019
Closed
@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 force-pushed the gnome-brightness-fix branch from 4b0ebf1 to a034c40 Compare April 22, 2019 19:57
@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

@ofborg ofborg bot removed 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 Apr 22, 2019
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
Member

@jtojnar jtojnar Apr 22, 2019 •
edited
Loading

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
Member

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
Loading

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 force-pushed the gnome-brightness-fix branch from a034c40 to f6e9229 Compare April 22, 2019 20:46
@worldofpeace worldofpeace removed the 2.status: work-in-progress This PR isn't done label Apr 24, 2019
@worldofpeace worldofpeace merged commit 44bf322 into NixOS:master 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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
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

@jtojnar jtojnar

@hedning hedning

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
GNOME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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