On 13.08.2014 20:27, Lennart Poettering wrote: > On Wed, 06.08.14 13:23, Stef Walter (st...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
