[pkg-apparmor] Bug#796589: Bug#796589: apparmor: Has init script in runlevel S but no matching service file

[intrigeri]

Hi,

thanks a lot for this review!

Felix Geyer wrote (26 Aug 2015 18:00:16 GMT) :
> On 26.08.2015 17:45, intrigeri wrote:
>> here are my initial notes and (incomplete) drafts, partly inspired by
>> OpenSuSe's unit.

>> sys-kernel-security.mount
>> =========================
[...]
> systemd mounts securityfs [1] automatically so this shouldn't be needed.

Cool, thanks!

>> apparmor-load-policy.service
>> ============================

> Unless we want to rename it for every init system we need to keep the name apparmor.

Right ⇒ done.

>> [Unit]
>> Description=Load AppArmor profiles
>> 
>> DefaultDependencies=no
>> 
>> # Load policy before bringing up the first network interface,
>> # to be able to confine processes that access the network early,
>> # such as dhclient:
>> Wants=network-pre.target
>> Before=network-pre.target
>> 
>> # ... however, let's not add an exagerated Before=basic.target
>> # or Before=sysinit.target, meant to ensure that the policy for basic system
>> # services is applied: in most case that's not needed, and it is prone
>> # to introducing dependency loops
>> # (https://wiki.debian.org/Teams/pkg-systemd/rcSMigration).
>> # Instead, basic system services that should be confined with AppArmor
>> # should add an After=apparmor.service, just like it's done already e.g.
>> # by networking.service (Debian -specific) and libvirtd.service.

> Is network-pre.target really enough?
> You'd basically have to add After=apparmor.service to every daemon that doesn't depend on
> network.target.

I ended up simply adding Before=sysinit.target (see my upcoming reply
to Felipe for details), which should address this concern, right?

>> After=local-fs.target systemd-journald-audit.socket

> Why is systemd-journald-audit.socket needed given it's
> socket-activated?

This came straight from openSUSE. I've dropped it.

>> RequiresMountsFor=/sys/kernel/security

> I think we can expect it to always be there except for containers which are already
> covered below.

OK, I've dropped it.

>> # XXX: do we need to do anything at shutdown?
>> # If yes, then add Conflicts=shutdown.target and Before=shutdown.target,
>> # or we won't be gracefully stopped on shutdown due to DefaultDependencies=no.

> No, we don't need to do anything on shutdown.
> That's also how the init script is set up right now.

So I did nothing special and thus the service should not be stopped
at shutdown.

>> ConditionSecurity=apparmor
>> ConditionPathIsReadWrite=/sys/kernel/security/apparmor/.load

> I'd rather have the unit fail if the file is not writable instead of covering it
> in a blocked state.

Assuming this comment was only about ConditionPathIsReadWrite= —
agreed, I've turned it into:

AssertPathIsReadWrite=/sys/kernel/security/apparmor/.load

Cheers,
--
intrigeri