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

[Stef Walter]

On 13.08.2014 20:27, Lennart Poettering wrote:
> 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:

Thanks for the review. Worked on this a bit more.

I might drop off the face of the earth for a couple weeks. In case I do,
I thought I'd update my public branch. But if I'm around, I'll test and
prepare a patch set early next week.

>> https://github.com/stefwalter/systemd/commits/polkit-systemd1

Updated this branch ^^

<snip>

>> 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.

Your comments below address what I was worried about here ^^

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

Done.

> 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.

Done.

> 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.

New action is called: org.freedesktop.systemd1.reload-daemon

> 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...

Makes sense ... but out of interest, why don't you just rely on this for
dbus1 as well? It seems that maintaining these two distinct access
control methods is risky.

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

Removed a couple extra newlines. I hope I found them all.

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

Done.

One other thing is that it seems like the ofs.Manager.LoadUnit() DBus
method call is wide open for anyone to call. Is this intentional? I
don't know all the implications, but wanted to highlight it.

Stef