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

[intrigeri]

Hi,

here are my initial notes and (incomplete) drafts, partly inspired by
OpenSuSe's unit. I think we'll need at least two units. That's kind of
blocked by the ongoing discussion on /usr and click-specific
bits, though.

sys-kernel-security.mount
=========================

[Unit]
Description=Security File System
Documentation=XXX
DefaultDependencies=no
ConditionPathExists=/sys/kernel/security

[Mount]
What=none
Where=/sys/kernel/security
Type=securityfs

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

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

After=local-fs.target systemd-journald-audit.socket
RequiresMountsFor=/sys/kernel/security

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

ConditionSecurity=apparmor
ConditionPathIsReadWrite=/sys/kernel/security/apparmor/.load
ConditionVirtualization=!container
# do not perform start/stop/reload actions when running from the Ubuntu liveCD:
ConditionPathExists=!/rofs/etc/apparmor.d

Documentation=man:apparmor(7)
Documentation=http://wiki.apparmor.net/

[Service]
Type=oneshot
ExecStart=XXX
ExecReload=XXX
ExecRestart=XXX
ExecStop=XXX
RemainAfterExit=yes

# XXX: if we do anything else than trivially loading policy in ExecStart=,
# then we may need to add RequiresMountsFor=/usr (see the corresponding discussion
# on Debian#782700); hopefully it won't be the case, as it would almost
# certainly break the "/usr on a remote filesystem" use case, e.g. because
# of our Before=network-pre.target.

[Install]
WantedBy=multi-user.target


Cheers,
--
intrigeri

[Felix Geyer]

Hi,

On 26.08.2015 17:45, intrigeri wrote:
> Hi,
>
> here are my initial notes and (incomplete) drafts, partly inspired by
> OpenSuSe's unit. I think we'll need at least two units. That's kind of
> blocked by the ongoing discussion on /usr and click-specific
> bits, though.
>
> sys-kernel-security.mount
> =========================
>
> [Unit]
> Description=Security File System
> Documentation=XXX
> DefaultDependencies=no
> ConditionPathExists=/sys/kernel/security
>
> [Mount]
> What=none
> Where=/sys/kernel/security
> Type=securityfs

systemd mounts securityfs [1] automatically so this shouldn't be needed.

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

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

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

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

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

> RequiresMountsFor=/sys/kernel/security

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

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

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

> ConditionVirtualization=!container
> # do not perform start/stop/reload actions when running from the Ubuntu liveCD:
> ConditionPathExists=!/rofs/etc/apparmor.d
>
> Documentation=man:apparmor(7)
> Documentation=http://wiki.apparmor.net/
>
> [Service]
> Type=oneshot
> ExecStart=XXX
> ExecReload=XXX
> ExecRestart=XXX
> ExecStop=XXX

There is no ExecRestart, systemd translates restart to stop/start.
That makes it a bit challenging to have a well-defined reload/restart actions.

Currently we do this in the init script:
- start: load all profiles
- stop: clear cache
- reload/restart: clear cache, load profiles, unload obsolete profiles

Why does it clear the cache? Is the cache invalidation known to be problematic?

Imho ideally we'd do this:
- start/reload: load all profiles, unload obsolete
- stop: nop

Iirc detecting/unloading obsolete profiles is a bit expensive so we might not want to do
that on boot though.


Ultimately we need another tool (apparmorctl?) that provides these actions and some
more so it can be called from the systemd unit.
For example we need users to be able to trigger teardown and clear-cache.

Cheers,
Felix


[1] https://github.com/systemd/systemd/blob/master/src/core/mount-setup.c#L78

[Seth Arnold]

On Wed, Aug 26, 2015 at 08:00:16PM +0200, Felix Geyer wrote:
> > [Service]
> > Type=oneshot
> > ExecStart=XXX
> > ExecReload=XXX
> > ExecRestart=XXX
> > ExecStop=XXX
>
> There is no ExecRestart, systemd translates restart to stop/start.
> That makes it a bit challenging to have a well-defined reload/restart actions.
>
> Currently we do this in the init script:
> - start: load all profiles
> - stop: clear cache
> - reload/restart: clear cache, load profiles, unload obsolete profiles
>
> Why does it clear the cache? Is the cache invalidation known to be problematic?

I think "stop clears cache" was a bandaid to fix a cache invalidation
issue. Since it was so painful, it also had bandaids to try to keep stop
from running all that often. Last time I looked into this I was driven
to insanity -- especially since it wasn't clear what was historical
holdover from ancient civilzations and what was actually necessary today.

> Imho ideally we'd do this:
> - start/reload: load all profiles, unload obsolete
> - stop: nop
>
> Iirc detecting/unloading obsolete profiles is a bit expensive so we might not want to do
> that on boot though.

I don't really like the "unload obsolete" idea much: "the system" policies
are in /etc/apparmor.d/. Ubuntu touch / snappy has entire second set
of policies for "apps" / "snaps" stored in /var/ somewhere. Perhaps
other services exist that generate profiles on the fly or store them
somewhere else and unloading them just because they don't also exist in
/etc/apparmor.d/ feels like a mistake.

Darix mentioned that systemd units can be parameterized; could this
functionality be used to load profiles from multiple directories? Does it
buy us anything?

Thanks

[intrigeri]

Hi,

FYI I can't commit to work on this again any time soon.

Seth recently posted on this topic elsewhere:
https://bugs.launchpad.net/apparmor/+bug/1503762/comments/2

Cheers,
--
intrigeri

[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