[systemd-devel] Looking for comments on this patch.

[Lennart Poettering]

On Tue, 24.07.12 15:45, Daniel J Walsh (dwalsh at redhat.com) wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> The goal of this patch is to add the ability for systemd to verify that
> SELinux policy allows the calling process to do the specified action.
> Start/Stop/Service

Looks mostly OK.

> This is expanded upon in this Feature Page Article.
> 
> https://fedoraproject.org/wiki/Features/SELinuxSystemdAccessControl
> 
> I am having a hard time testing this, and any suggestions welcomed.

My usual way to test these things is installing this into the system you
work on (i.e. by just running "make install"), but instead of rebooting
every time (and thus risking ending up in something that doesn't work),
just booting the main system in qemu using a throw-away snapshot. I use
a shell script like the following for this:

<snip>
#!/bin/sh
sudo sync
sudo /bin/sh -c 'echo 3 > /proc/sys/vm/drop_caches'
sudo umount /
sudo modprobe kvm-intel
exec sudo qemu-kvm -smp 2 -m 512 -snapshot /dev/sda
</snip>

The first three lines are basically just a way to make the kernel flush
everything to disk. The file system will still appear dirty to the
booting system, but fsck will not find anything.

> +                if (!(u = manager_get_unit(m, name))) {
> +                        dbus_set_error(&error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s is not loaded.", name);
> +                        return bus_send_error_reply(connection, message, &error, -ENOENT);
> +                }

Some coding style nitpicks:

We nowadays try to avoid writing this:

if ((foo = bar()) { 
        ...

and instead write this:

foo = bar();
if (foo) {
        ...

We haven't always done that, but all new code should really follow the
new style.

> +
> +                if (u->fragment_path) {
> +                        r = selinux_access_check(connection, message, m, &error, perm, u->fragment_path);
> +                } else {
> +                        r = selinux_access_check(connection, message, m, &error, perm, u->source_path);
> +                }

Also, please drop the { }  in this case.

> +static const char *unit_methods[][2] = {{ "DisableUnitFiles", "disable" },
> +                                        { "EnableUnitFiles", "enable" },
> +                                        { "GetUnit", "status" },
> +                                        { "GetUnitByPID", "status" },
> +                                        { "GetUnitFileState",  "status" },
> +                                        { "Kill", "stop" },
> +                                        { "KillUnit", "stop" },
> +                                        { "LinkUnitFiles", "enable" },
> +                                        { "ListUnits", "status" },
> +                                        { "LoadUnit", "status" },
> +                                        { "MaskUnitFiles", "disable" },
> +                                        { "PresetUnitFiles", "enable" },
> +                                        { "ReenableUnitFiles", "enable" },
> +                                        { "Reexecute", "start" },
> +                                        { "Reload", "reload" },
> +                                        { "ReloadOrRestart", "start" },
> +                                        { "ReloadOrRestartUnit", "start" },
> +                                        { "ReloadOrTryRestart", "start" },
> +                                        { "ReloadOrTryRestartUnit", "start" },
> +                                        { "ReloadUnit", "reload" },
> +                                        { "ResetFailed", "stop" },
> +                                        { "ResetFailedUnit", "stop" },
> +                                        { "Restart", "start" },
> +                                        { "RestartUnit", "start" },
> +                                        { "Start", "start" },
> +                                        { "StartUnit", "start" },
> +                                        { "StartUnitReplace", "start" },
> +                                        { "Stop", "stop" },
> +                                        { "StopUnit", "stop" },
> +                                        { "TryRestart", "start" },
> +                                        { "TryRestartUnit", "start" },
> +                                        { "UnmaskUnitFiles", "enable" },
> +                                        { NULL, NULL }

Hmm, ideally, we'd have a gperf table for this... That said, we
currently don't use any gperf when looking up what to do on dbus calls,
so don't bother to fix this now.

Please change "const char *" to "const char * const" so that the array
itself is const too, not just the strings. 

> +        va_start(ap, fmt);
> +#ifdef HAVE_AUDIT
> +        if (audit_fd >= 0)
> +        {
> +                char buf[PATH_MAX*2];

LINE_MAX sounds like a better choice here, no?


> +        for (i = 0; unit_methods[i][0]; i++) {
> +                if (strcmp(method, unit_methods[i][0]) == 0) {
> +                        *perm = unit_methods[i][1];
> +                        *require_unit = 1;
> +                        break;
> +                }
> +        }

We prefer streq(a, b) over strcmp(a, b) == 0, but that's just nitpicking....

> -int label_init(const char *prefix);
> +int label_init(const char *prefixes[]);
>  void label_finish(void);

This bit would be better in a separate, second patch I think.

Otherwise looks good!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.