After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 491462 - [PATCH] PolicyKit support
[PATCH] PolicyKit support
Status: RESOLVED FIXED
Product: system-monitor
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Robert Roth
System-monitor maintainers
: 605711 (view as bug list)
Depends on:
Blocks: 685294
 
 
Reported: 2007-10-29 17:29 UTC by David Zeuthen (not reading bugmail)
Modified: 2012-12-08 00:34 UTC
See Also:
GNOME target: ---
GNOME version: 3.7/3.8


Attachments
preliminary patch (49.71 KB, patch)
2007-10-29 17:31 UTC, David Zeuthen (not reading bugmail)
none Details | Review
updated patch (52.94 KB, patch)
2007-12-21 21:13 UTC, David Zeuthen (not reading bugmail)
reviewed Details | Review
updated patch (55.37 KB, patch)
2008-01-15 01:32 UTC, Matthias Clasen
none Details | Review
additional patch to update to the PolicyKit 1 api (29.96 KB, patch)
2009-06-09 16:41 UTC, Matthias Clasen
none Details | Review
added pkexec backend for running renice/kill commands as root (6.06 KB, patch)
2012-09-28 13:40 UTC, Robert Roth
none Details | Review
Patch for proposed changes (7.25 KB, patch)
2012-10-02 18:30 UTC, Robert Roth
none Details | Review
Executing gnome-system-monitor-kill (322.56 KB, image/png)
2012-10-03 03:39 UTC, Robert Roth
  Details
Executing /bin/sh with argv1 gnome-system-monitor-kill (335.03 KB, image/png)
2012-10-03 03:40 UTC, Robert Roth
  Details
Proposed patch with compatible pkexec helpers (8.13 KB, patch)
2012-12-06 22:13 UTC, Robert Roth
none Details | Review
Proposed patch with pkexec helper binaries instead of shell scripts (8.37 KB, patch)
2012-12-07 10:53 UTC, Robert Roth
none Details | Review

Description David Zeuthen (not reading bugmail) 2007-10-29 17:29:52 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!
Comment 1 David Zeuthen (not reading bugmail) 2007-10-29 17:31:50 UTC
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(-)
Comment 2 David Zeuthen (not reading bugmail) 2007-10-29 18:26:36 UTC
Here's the proposal thread

 http://mail.gnome.org/archives/desktop-devel-list/2007-October/msg00237.html

with some more background information...
Comment 3 Benoît Dejean 2007-10-29 19:14:16 UTC
No offense, but adding 1000 lines in order to do the same thing looks wrong ...
Comment 4 David Zeuthen (not reading bugmail) 2007-10-29 21:03:24 UTC
(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.
Comment 5 Richard Hughes 2007-11-02 18:45:27 UTC
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.
Comment 6 David Zeuthen (not reading bugmail) 2007-12-21 21:13:38 UTC
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.
Comment 7 Karl Lattimer 2008-01-03 14:17:35 UTC
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.
Comment 8 Benoît Dejean 2008-01-03 20:05:49 UTC
As long as policykit is not a blessed dependency, it's a no-go for me.
Comment 9 David Zeuthen (not reading bugmail) 2008-01-10 18:15:56 UTC
(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.
Comment 10 Benoît Dejean 2008-01-11 12:04:27 UTC
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.
Comment 11 Matthias Clasen 2008-01-15 01:32:46 UTC
Created attachment 102872 [details] [review]
updated patch

Patch updated to 2.21.5
Comment 12 Matthias Clasen 2009-06-09 16:41:08 UTC
Created attachment 136220 [details] [review]
additional patch to update to the PolicyKit 1 api
Comment 13 Chris Kühl 2011-02-26 21:59:02 UTC
Any chance of getting an updated patch for this? The previous arguments no longer seem valid.
Comment 14 David Zeuthen (not reading bugmail) 2011-02-26 22:28:04 UTC
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
Comment 15 Chris Kühl 2011-02-28 13:52:59 UTC
Thanks David, Good to know once I (or someone else, preferably) get around to it.
Comment 16 Michael Biebl 2011-09-29 11:06:09 UTC
hi, has there been any progress on this?
Comment 17 Robert Roth 2012-09-28 13:40:50 UTC
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.
Comment 18 Robert Roth 2012-10-01 00:10:15 UTC
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.
Comment 19 David Zeuthen (not reading bugmail) 2012-10-02 15:53:32 UTC
(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.
Comment 20 David Zeuthen (not reading bugmail) 2012-10-02 16:12:37 UTC
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.
Comment 21 Robert Roth 2012-10-02 17:51:39 UTC
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.
Comment 22 Robert Roth 2012-10-02 18:30:17 UTC
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?
Comment 23 David Zeuthen (not reading bugmail) 2012-10-02 18:36:52 UTC
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.
Comment 24 David Zeuthen (not reading bugmail) 2012-10-02 18:37:46 UTC
(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.
Comment 25 Robert Roth 2012-10-02 19:48:54 UTC
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)
Comment 26 David Zeuthen (not reading bugmail) 2012-10-02 21:02:47 UTC
(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.
Comment 27 Robert Roth 2012-10-03 03:38:25 UTC
(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.
Comment 28 Robert Roth 2012-10-03 03:39:27 UTC
Created attachment 225648 [details]
Executing gnome-system-monitor-kill

Screenshot with exec.path set to gnome-system-monitor-kill
Comment 29 Robert Roth 2012-10-03 03:40:21 UTC
Created attachment 225649 [details]
Executing /bin/sh with argv1 gnome-system-monitor-kill
Comment 30 David Zeuthen (not reading bugmail) 2012-10-03 13:23:10 UTC
Yeah, only 0.107 and newer supports argv1 - please test with that version. Thanks! (I'm using F18 to test)
Comment 31 Robert Roth 2012-10-05 09:50:00 UTC
@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?
Comment 32 Robert Roth 2012-10-05 09:50:43 UTC
Reopening to find the best solution to define the policykit actions.
Comment 33 David Zeuthen (not reading bugmail) 2012-10-05 13:51:03 UTC
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.)
Comment 34 Robert Roth 2012-12-06 22:13:22 UTC
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.
Comment 35 Robert Roth 2012-12-06 22:35:49 UTC
*** Bug 605711 has been marked as a duplicate of this bug. ***
Comment 36 David Zeuthen (not reading bugmail) 2012-12-07 03:53:14 UTC
(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).
Comment 37 Robert Roth 2012-12-07 10:53:17 UTC
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.
Comment 38 Robert Roth 2012-12-08 00:34:18 UTC
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.