[systemd-devel] Work on adding polkit support to systemd1

[Lennart Poettering]

On Wed, 06.08.14 13:23, Stef Walter (stefw at redhat.com) wrote:

> I've done initial work on adding polkit support to systemd1 DBus
> methods. You can see it here:
> 
> https://github.com/stefwalter/systemd/commits/polkit-systemd1
> 
> Basic rules:
> 
>  * Read access for everyone
> 
>  * Methods that modifies running unit state is controlled by a polkit
>    action: org.freedesktop.systemd1.manage-units
> 
>  * Methods that modifies unit state files is controlled by a polkit
>    action: org.freedesktop.systemd1.manage-unit-files
> 
>  * Many methods are only callable by root callers, like: Poweroff()
>    Kexec() etc...
> 
>  * Job.Cancel() and Manager.CancelJob() are callable by the caller(s)
>    that started the job.
> 
>  * Setting properties is only possible by root callers.
> 
> The way that each callback in sd-bus has to handle verification seems a
> bit risky to me. So I've only opened up the specific interfaces I
> touched in the DBus policy file.
> 
> Eventually the DBus policy file would go away, but hopefully sd-bus will
> have a less risky way of verifying callers at that point.
> 
> I need to work on testing this. Will send a patch set when I'm done.
> 
> I'd be happy to add documentation here when we're done:
> 
> http://www.freedesktop.org/wiki/Software/systemd/dbus/

THis looks pretty good. A few comments:

The first three patches look totally OK.

Regarding the fourth: please don't bind any checks against the UID,
check for CAP_SYS_ADMIN instead, we make it equally easy.

Why is verify_root_sync() invoked that often? I mean, for these cases
the dbus1 policy should not grant access anyway, so we don't really need
to check for permissions here again. I'd really prefer if on dbus1 would
use the dbus1 policy for as much access control work as useful, and only
do it on our own if we really need to.

reload (and probably also reexecute) should probably hook into polkit
too, with a new action? it sounds useful enough for people to invoke it
frequently.

Note that on kdbus access control works differently than on dbus1: it's
the client's responsibility to implement access control. To make this
easy our sd-bus library actually allows encoding in the method vtable
which calls are accessible for unpriviliged processes. That's what the
SD_BUS_VTABLE_UNPRIVILEGED flag in the object vtables is for (grep for
it). The flag (or its absence) has no effect on dbus1 daemons at all,
and only matters for kdbus. To make sure your changes also work
correctly and as intended on kdbus you need to add the flag to all
methods that you are now opening up via polkit. Because otherwise method
calls won't even get that far.

Or in other words: everything that is opened up in the dbus1 policy also
needs to be opened up with SD_BUS_VTABLE_UNPRIVILIGED in the object
vtables...

Sometiems you added spurious newlines to the patches, please don't.

I'd prefer if the dbus1 policy would precisely list the method calls
that are now opened up...

Looks like really good stuff I must say!

Lennart

-- 
Lennart Poettering, Red Hat