[pkg-apparmor] apparmor.service - get upstream and Debian (mostly?) in sync?

[John Johansen]

On 11/5/23 10:14, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 25. Oktober 2023, 11:42:40 CET schrieb intrigeri:
>> Yep, for historical context, IIRC both Debian and OpenSUSE initially
>> had their own downstream systemd unit, and then you've upstreamed your
>> (OpenSUSE) version, which was a good first step. Since then, nobody
>> ensured that upstreamed version was suitable for Debian as well, so
>> here we are still shipping our own. In 2018 I did lots of work to
>> make the Debian version closer to upstream's but I did not go as far
>> as fully converging.
>>
>> I won't have time to work on this any time soon myself, but here's
>> a suggestion to anyone who will:
> 
> I hope you at least have some time to give feedback ;-)
> 
> I already got the Documentation=... upstream, and the remaining diff is:
> 
> --- upstream
> +++ Debian
>   [Unit]
>   Description=Load AppArmor profiles
>   DefaultDependencies=no
>   Before=sysinit.target
> +After=local-fs.target
> 
> I'd prefer to not use After=local-fs.target, see below.
> 
>   After=systemd-journald-audit.socket
> 
> 
> 
> -# profile cache: /var/cache/apparmor/ and /usr/share/apparmor/cache/
> -After=var.mount var-cache.mount usr.mount usr-share.mount
> +RequiresMountsFor=/var/cache/apparmor
> 
> I guess switching upstream to RequiresMountsFor should be easy.
> 
> The most interesting question is if we should keep the dependency on
> /usr/share/apparmor for the precompiled cache.
> 
no. There is a reason systemd requires the early boot cache to be in
/etc/apparmor/earlypolicy

and the general consensus when this was done, was it was a mistake
to move the cache into /usr/share/apparmor. That the FHS didn't
properly cover binary policy and there was a proposal to amend it.

> FYI: I have disabled the precompiled cache in the Tumbleweed packages
> again, because it caused interesting[tm] problems - the parser only
> looks at the timestamps, which meant "old" local additions (older than
> the precompiled cache) were missed.
> 
yeah, we are working on a fix. Its scheduled for the 4.0 release
but being an addition may force us to push the release from Dec
to January

> 
> +AssertPathIsReadWrite=/sys/kernel/security/apparmor/.load
> 
> I'd be somewhat surprised if apparmor gets started before /sys/ is
> mounted, and we already have ConditionSecurity=apparmor in the next
> line.
> So - do you think this is really needed?
> 
it depends on how its booted. systemd knows that /sys/ is needed for
the early profile load (this is before other processes are started)
and should handle it. None systemd systems, I haven't kept up with

>   ConditionSecurity=apparmor
>   Documentation=man:apparmor(7)
>   Documentation=https://gitlab.com/apparmor/apparmor/wikis/home/
> 
> 
> +# Don't start this unit on the Ubuntu Live CD
> +ConditionPathExists=!/rofs/etc/apparmor.d
> +
> +# Don't start this unit on the Debian Live CD when using overlayfs
> +ConditionPathExists=!/run/live/overlay/work
> 
> These lines might be candidates for a drop-in shipped as
> /usr/lib/systemd/system/apparmor.service.d/debian.conf in Debian and
> Ubuntu.
> 
> 
>   [Install]
> -WantedBy=multi-user.target
> +WantedBy=sysinit.target
> 
> This (together with "After=local-fs.target") is probably the most
> interesting diff.
> 
> My goal is to load AppArmor profiles as early as possible, therefore I'd
> prefer to keep sysinit.target. Can I convince you to also use it, and to
> drop "After=local-fs.target"?
> (If it helps: the dependencies in the upstream apparmor.service always
> worked in openSUSE.)
> 

use the early boot cache. Currently this requires manual setup and
syncing but, the regular apparmor unit will try to reload when it
gets to run, so even if the early boot cache is out of date, updated
policy will get loaded. The kernel will dedup profiles that haven't
changed.

https://gitlab.com/apparmor/apparmor/-/wikis/AppArmorInSystemd


> 
> Feedback welcome ;-)
> 
> 
> Regards,
> 
> Christian Boltz