GNOME Bugzilla – Bug 491462
[PATCH] PolicyKit support
Last modified: 2012-12-08 00:34:18 UTC
Hi, I've been working on adding PolicyKit support to gnome-system-monitor. Right now it depends on a soon-to-be-released PolicyKit-gnome 0.7 http://hal.freedesktop.org/docs/PolicyKit-gnome/ For more information about PolicyKit, see http://hal.freedesktop.org/docs/PolicyKit/ (Availability: PolicyKit right now ships at least with Fedora, SUSE and Gentoo. I talked to the Debian utopia maintainers a few weeks ago and was told they are in the process of getting it into Debian.) This patch uses a system bus mechanism to carry out the privileged operation of killing and renicing. If you have D-Bus 1.1.2 or later it will get activated on demand and exit after 30 secs of inactivity. For earlier D-Bus versions, you need to start it by hand (or an initscript). However, most distributions now ship D-Bus 1.1.2... Some screenshots http://people.freedesktop.org/~david/gnome-system-monitor-polkit-1.png http://people.freedesktop.org/~david/gnome-system-monitor-polkit-2.png Also, this patch is preliminary; I want to update it once 0.7 is out.. the main thing that doesn't work well right now is - when multiple processes are selected. That should be easy to fix with a small enhancement to PolKitGnomeAction. Other ideas for improvements (that I'm willing to work on if it gets upstream) - the mechanism is a lot of boiler plate code mostly due to how dbus-glib right now works. It could be rewritten if Python if you want - it should be easy to add support for viewing open files and smaps for privileged processes; the mechanism would just return a string I'm also going to suggest PolicyKit and PolicyKit-gnome as blessed dependencies for GNOME 2.22. My goal is to get PolicyKit-gnome into the Desktop for 2.24. Will post a link to the thread once I've proposed it. Anyway, attaching the patch. Comments/flames/concerns/thoughts both about the approach and the patch itself is very welcome!
Created attachment 98122 [details] [review] preliminary patch There's also a buildsystem bug in this patch; sometimes I've found that I had to go in and manually cd src make gnome-system-monitor-mechanism-client-glue.h $ diffstat g-s-m-polkit-1.patch configure.in | 28 + po/POTFILES.in | 1 src/Makefile.am | 44 ++ src/callbacks.cpp | 10 src/gnome-system-monitor-mechanism.c | 619 +++++++++++++++++++++++++++++++++ src/gnome-system-monitor-mechanism.xml | 15 src/gnome-system-monitor.policy.in | 46 ++ src/interface.cpp | 89 ++++ src/procactions.cpp | 58 ++- src/procdialogs.cpp | 91 ++++ src/procman.cpp | 33 + src/procman.h | 17 12 files changed, 1021 insertions(+), 30 deletions(-)
Here's the proposal thread http://mail.gnome.org/archives/desktop-devel-list/2007-October/msg00237.html with some more background information...
No offense, but adding 1000 lines in order to do the same thing looks wrong ...
(In reply to comment #3) > No offense, but adding 1000 lines As noted most of this (619 lines according to diffstat) is GObject boilerplate; also note that I asked if you preferred this being written in another language; e.g. in Python this can be done in 20-30 lines. The actual patch to the source code amounts to 10+89+58+91+33+17 = 298 changed lines. Is that really too bad? > in order to do the same thing looks wrong ... It's not really the "same thing" as gksu. Notably one new feature added is visual feedback whether authentication is needed. Then there's all the other things; e.g. PolicyKit is an authorization framework; in the future such authorizations can be managed centrally etc. etc.
This is totally a good thing to do - using PolicyKit I can set a central policy as an admin and then applications just do the right thing - I don't have to start giving users sudo or the root password. I'm using PolicyKit in PackageKit and it rocks. Richard.
Created attachment 101436 [details] [review] updated patch Here's an updated patch now that PolicyKit 0.7 and PolicyKit-gnome 0.7 have been out for some time. This is also now shipping in Fedora's development branch: http://koji.fedoraproject.org/koji/taskinfo?taskID=305939 http://cvs.fedoraproject.org/viewcvs/devel/gnome-system-monitor/ Thanks for considering this patch.
I'd love to see this patch get in, as it would allow me to change the priority on processes without getting a permission denied error. Benoit, how about it? After all PolicyKit is going to be the way forward for a number of major distributions.
As long as policykit is not a blessed dependency, it's a no-go for me.
(In reply to comment #8) > As long as policykit is not a blessed dependency, it's a no-go for me. I think you've got confused from the thread on d-d-l. It's fine to have an optional dependency on PolicyKit, intlclock has that. Does that clear it up? Anyway, it would be nice with some more concrete feedback. Do you think PolicyKit is a stupid thing? Thanks.
I don't want to merge yet another gksu-like optional dependency. We already have 2. I've asked GNOME to bless one of them and it was rejected because none was "good enough". I'm done with optional dependencies. So I wait. I keep reading the patch and some polkit documentation, and i'm still not comfortable with adding 1000 lines where gksudo is 3 lines. I'm not comfortable with creating program to be run as root where i trust kill and renice. And the fact that you added a killtimer scares me.
Created attachment 102872 [details] [review] updated patch Patch updated to 2.21.5
Created attachment 136220 [details] [review] additional patch to update to the PolicyKit 1 api
Any chance of getting an updated patch for this? The previous arguments no longer seem valid.
Just for the record, if I had to redo this today (more than three years later), I'd just write some wrapper scripts and use pkexec [1] (which is part of PolicyKit to make things like this a lot easier). That way you can bypass the system daemon D-Bus mechanism entirely which would greatly simplify the patch. [1] : http://hal.freedesktop.org/docs/polkit/pkexec.1.html
Thanks David, Good to know once I (or someone else, preferably) get around to it.
hi, has there been any progress on this?
Created attachment 225335 [details] [review] added pkexec backend for running renice/kill commands as root I have added pkexec as an additional method for trying to run kill/renice as root, it seems to work fine, however I would appreciate a review for it.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
(In reply to comment #17) > Created an attachment (id=225335) [details] [review] > added pkexec backend for running renice/kill commands as root > > I have added pkexec as an additional method for trying to run kill/renice as > root, it seems to work fine, however I would appreciate a review for it. Couple of quick comments here + <annotate key="org.freedesktop.policykit.exec.path">/usr/bin/renice</annotate> What happens if I ship a program called, say, mate-system-monitor that also ships a polkit action for /usr/bin/renice but it calls the action org.gnome.mate-system-monitor.renice ? Answer: it's not defined, it could pick one at random (this is what happens today, I think) or none of them... So it would be better to avoid collisions there:... to avoid collisions, you could use a wrapper script that just uses the exec builtin of sh(1), e.g. something like (untested): /usr/libexec/gnome-system-monitor-renice: #!/bin/sh exec /usr/bin/renice "$@" and something like this <annotate key="org.freedesktop.policykit.exec.path">/bin/sh</annotate> <annotate key="org.freedesktop.policykit.exec.argv1">/usr/libexec/gnome-system-monitor-renice</annotate> But please be careful to require a new enough polkit version that supports exec.argv1! + <annotate key="org.freedesktop.policykit.exec.allow_gui">FALSE</annotate> I *think* this does what you want but since the docs explicitly says These two variables will be retained if the org.freedesktop.policykit.exec.allow_gui annotation on an action is set to a nonempty value and, strictly speaking, since "FALSE" is non-empty this does the opposite of what you intended. So I'd just not set that annotation at all.
Or better/safer would be this: /usr/libexec/gnome-system-monitor-renice: #!/usr/bin/pkexec /bin/sh exec /usr/bin/renice "$@" and your application simply calls /usr/libexec/gnome-system-monitor-renice with the "pkexec" in front of it.
Thanks for the suggestions, I'm working on implementing them, however I have used @libexecdir@ as the target to install the binaries into, and have no idea how to reference it from the code (as I have to call it from the code). Could you help me with this? I'm not really experienced with autotools.
Created attachment 225609 [details] [review] Patch for proposed changes This patch should be applied on top of the "added pkexec backend ..." and contains the changes suggested by David Zeuthen. @David: could you please take a look and let me know if you have any other suggestions?
I'm surprised that <annotate key="org.freedesktop.policykit.exec.path">@pkglibexecdir@/gnome-system-monitor-kill</annotate> works... did you test it with that? I'd imagine it would need to be <annotate key="org.freedesktop.policykit.exec.path">/bin/sh</annotate> <annotate key="org.freedesktop.policykit.exec.argv1">@pkglibexecdir@/gnome-system-monitor-kill</annotate> as mentioned in comment 19. I would also add a check for polkit >= 0.106 as argv1 isn't available until that version.
(In reply to comment #23) > as mentioned in comment 19. I would also add a check for polkit >= 0.106 as > argv1 isn't available until that version. Sorry, I meant 0.107.
Yes, I did test it and it works this way. What's the point of moving it to /bin/sh + args if that has another requirement? Please also note that checking policykit is not so easy, as we don't want a build-time dependency, thus we only do run-time check for the existence of pkexec (just like gksudo checks the existence of libgksudo.so and gnomesu the existence of libgnomesu)
(In reply to comment #25) > Yes, I did test it and it works this way. Doesn't work for me and I doubt it works for you - do you really get the correct authentication dialog for your action? I don't think so, I think you get the standard "Authentication is needed to run /bin/sh as the super user" one. > What's the point of moving it to > /bin/sh + args if that has another requirement? The point is that it works (comment 19). > Please also note that checking > policykit is not so easy, as we don't want a build-time dependency, thus we > only do run-time check for the existence of pkexec (just like gksudo checks the > existence of libgksudo.so and gnomesu the existence of libgnomesu) I realize that. The good news is that it's not your problem - it's a distributor problem. Your task is to make sure you properly communicate to distros that they need polkit 0.107 or later in order for this to work. So e.g. mention it at the end of configure output and in NEWS.
(In reply to comment #26) > (In reply to comment #25) > > Yes, I did test it and it works this way. > > Doesn't work for me and I doubt it works for you - do you really get the > correct authentication dialog for your action? I don't think so, I think you > get the standard "Authentication is needed to run /bin/sh as the super user" > one. No, I don't get the standard dialog, I get the correct dialog with the text from the policy file. See the upcoming screenshot. > > > What's the point of moving it to > > /bin/sh + args if that has another requirement? > > The point is that it works (comment 19). I have tried that and it did not work for me. See the upcoming screenshot. And after a quick look I have found the problem: I only have policykit-1 version 0.104 (this ships with the latest daily Ubuntu, debian sid ships 105, I have found with google that mageia ships 106). What os do you test it on? I'll try to get the debian/ubuntu polkit package updated, and see if that makes a difference. Could that be that older versions (like 104 for me) work without argv1 by specifying the script in exec.path, but newer versions don't? > I realize that. The good news is that it's not your problem - it's a > distributor problem. Your task is to make sure you properly communicate to > distros that they need polkit 0.106 or later in order for this to work. So e.g. > mention it at the end of configure output and in NEWS.
Created attachment 225648 [details] Executing gnome-system-monitor-kill Screenshot with exec.path set to gnome-system-monitor-kill
Created attachment 225649 [details] Executing /bin/sh with argv1 gnome-system-monitor-kill
Yeah, only 0.107 and newer supports argv1 - please test with that version. Thanks! (I'm using F18 to test)
@David: I am still thinking of a solution to support both pkexec>=0.107 with argv1 and pkexec<0.107 somehow: * have two actions for each command (system-monitor-kill and system-monitor-renice) in the same policy file (with different ID's), * one with exec.argv, * another with only exec.path set to the command path In case policykit finds such a policy file, how would it find which action to use?
Reopening to find the best solution to define the policykit actions.
One thing you can do that works on both old and new polkit versions is this: write a C program and install it as /usr/libexec/gnome-system-monitor-renice that simply just uses the execvp(3) C function to invoke the renice(1) command .... something (untested) like execvp ("renice", argv); should work (remember that pkexec(1) will sanitize the environment so there's no chance of the user fooling you into running /home/user/bin/renice as root). Then you just do pkexec /usr/libexec/gnome-system-monitor-renice <options> from gnome-system-monitor itself and the action you'd need is <annotate key="org.freedesktop.policykit.exec.path">@pkglibexecdir@/gnome system-monitor-renice</annotate> This way you don't depend on new features introduced in polkit 0.107. (Yes, it's a little weird being forced to do it in C instead of shell but that's just how things work.)
Created attachment 230928 [details] [review] Proposed patch with compatible pkexec helpers Proposing a patch with separate helper applications (instead of shell scripts) executed with pkexec. @David Zeuthen: could you help me with a small review here, I'm pretty stuck, and can't find why: execvp works fine with "renice", but with kill I am getting the following error: "skill: "gsm-kill" is not support For more details see skill(1)" Where gsm-kill is my helper application calling execvp("kill", {"-s", "SIGNAL", "PID"}). Tried using /bin/kill instead of kill, but it did not help.
*** Bug 605711 has been marked as a duplicate of this bug. ***
(In reply to comment #34) > Created an attachment (id=230928) [details] [review] > Proposed patch with compatible pkexec helpers > > Proposing a patch with separate helper applications (instead of shell scripts) > executed with pkexec. > > @David Zeuthen: could you help me with a small review here, I'm pretty stuck, > and can't find why: execvp works fine with "renice", but with kill I am getting > the following error: > "skill: "gsm-kill" is not support > For more details see skill(1)" > Where gsm-kill is my helper application calling execvp("kill", {"-s", "SIGNAL", > "PID"}). > Tried using /bin/kill instead of kill, but it did not help. My guess is that kill(1) examines argv[0]. Yes, looks like it: http://git.kernel.org/?p=utils/util-linux/util-linux.git;a=blob;f=misc-utils/kill.c;h=6abfd24a11942d8e9daa596cf023911e744193e7;hb=HEAD#l158 Also, AFAICT, execvp(2) excepts the argv passed to it to be NULL terminated but you are not guaranteed this is the case for the argv passed to main() (with standard Linux/glibc, I'm pretty sure it is though). I would try something like this gchar **argv_modified; argv_modified = g_new0 (gchar *, argc + 1); memcpy (argv_modified, argv, argc * sizeof (char*)); argv_modified[0] = PROGRAM; and then pass argv_modified to execvp(2).
Created attachment 230961 [details] [review] Proposed patch with pkexec helper binaries instead of shell scripts Thanks David, that helped, so the patch seems to be complete now. I will try again to install F18 (last time I have tried anaconda did crash all the time) and test on F18 too to see if it works correctly with the latest policykit too (on Ubuntu I have 0.104) and if it works, I will commit it.