Bug 63312

Created attachment 77667 [details] [review]
Add support for ConditionSecurity=apparmor, update man page

Whitespace wasn't expanded-tabs in the previous patch. Fixed now.

any particular reason for glob_exists()? The passed path is not a glob, so access(F_OK) sounds sufficient?

(In reply to comment #2)
> any particular reason for glob_exists()? The passed path is not a glob, so
> access(F_OK) sounds sufficient?

I tried to use systemd util functions, but access(path, F_OK) works the same.

Hmm, so, the current implementation of the SELinux check not only checks whether SELinux is compiled into the kernel, but also if it is turned on during runtime. I wonder if we should have the same for AppArmor? Is there a nice way to check whether AppArmor is actually turned on?

(Also, as a side note, we currently load SELinux, IMA and SMACK policies from early PID 1, so that they are applied before the first process is started. Do we want the same for AppArmor?)

(In reply to comment #4)
> Hmm, so, the current implementation of the SELinux check not only checks
> whether SELinux is compiled into the kernel, but also if it is turned on
> during runtime. 

That directory only exists if AppArmor is loaded *and* turned on, so the check is sufficient. This can be verified by booting with security=none and running `aa-status`. The module is loaded, but the apparmor tree inside securityfs isn't.

> (Also, as a side note, we currently load SELinux, IMA and SMACK policies
> from early PID 1, so that they are applied before the first process is
> started. Do we want the same for AppArmor?)

Right now we're using a .service file with DefaultDependenices=no, Before=basic.target, WantedBy=sysinit.target which works fine for us because everything that we confine is running in basic.target.

However, eventually it would indeed be nice to have systemd load AA profiles with PID 1.

(In reply to comment #5)
> However, eventually it would indeed be nice to have systemd load AA profiles
> with PID 1.

I should note here that I speak as a distributor/user of AppArmor, and not a developer, so there might be details that I am missing which would make this undesirable.

Created attachment 78627 [details] [review]
Add support for ConditionSecurity=apparmor, update man page

The attached patch implements access(F_OK), and has an updated commit message.

I think early loading of profiles is a separate issue? Is there anything else that needs to be done here?

Thanks! Applied!

Hmm, I just noticed this patch:

https://code.launchpad.net/~mdeslaur/upstart/apparmor-support/+merge/164169

It contains a different check for AppArmor. Basically something like this:

/sys/module/apparmor/parameters/enabled == 'Y'

I'd prefer if we could change our code to do the same, given that the Ubuntu guys are guys are upstream for apparmor.

Any chance you could rework the condition check? Most likely you can just use:

static bool apparmor_enabled(void) {
        _cleanup_free_ char *p = NULL;

        r = read_one_line_file("/sys/module/apparmor/parameters/enabled", &p);
        if (r < 0)
                return false;

        return parse_boolean(p) > 0;
}

But I can't test this, so I am kinda counting on you to check if this works and provide a patch?

I'll test this and get back to you ASAP.

Created attachment 79566 [details] [review]
Use the same test as upstart for apparmor

The attached patch works for me.

Applied in http://cgit.freedesktop.org/systemd/systemd/commit/?id=cb0edd735c40f.