[apparmor] [PATCH] Native systemd support

Wed Oct 19 22:53:41 UTC 2016

Hello,

quick review - but I'd welcome if someone else also comments on this. 
I know the biggest pitfalls, but I'm far from being a systemd expert ;-)

Am Mittwoch, 19. Oktober 2016, 10:45:53 CEST schrieb Goldwyn Rodrigues:
> This patch implements native systemd support for apparmor. This
> is performed and tested on opensuse 42.1. I think we can keep
> rc.apparmor.suse for a bit more time until we decide to
> fully retire it.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.com>

This patch looks much better than your submit request ;-)

Thanks for working on this!

> --- a/parser/Makefile
> +++ b/parser/Makefile
> @@ -314,11 +314,12 @@
> 
>  .PHONY: install-suse
>  install-suse:

It's probably a good idea to move the sytemd stuff to the general 
"install" section (or maybe "install-systemd" if we want to stay 
flexible) instead of keeping it suse-specific.

> -	install -m 755 -d $(DESTDIR)/etc/init.d
> -	install -m 755 rc.apparmor.$(subst install-,,$(@))
> $(DESTDIR)/etc/init.d/boot.apparmor -	install -m 755 -d
> $(DESTDIR)/sbin
> -	ln -sf /etc/init.d/boot.apparmor $(DESTDIR)/sbin/rcapparmor
> -	ln -sf rcapparmor $(DESTDIR)/sbin/rcsubdomain

We migth want to keep "rcapparmor", but as symlink to /usr/sbin/service
(that's something I can also do in the spec because it's (open)SUSE-
specific)

"rcsubdomain" is terribly old, and I doubt if someone still uses it, so 
dropping it sounds OK. (Besides that, it wouldn't work if we symlink it 
to /usr/sbin/service anyway.)

> +	install -m 755 -d $(DESTDIR)/usr/lib/systemd/system/
> +	install -m 755 -d $(DESTDIR)/usr/lib/systemd/scripts/
> +	install -m 0444 apparmor.service $(DESTDIR)/usr/lib/systemd/system
> +	install -m 0755 apparmor_start.sh $(DESTDIR)/usr/lib/systemd/scripts
> +	install -m 0755 apparmor_stop.sh $(DESTDIR)/usr/lib/systemd/scripts
> +	install -m 0755 apparmor_reload.sh
> $(DESTDIR)/usr/lib/systemd/scripts

No objections to that path, but we already have /usr/share/apparmor/. 
Would storing the scripts there make sence, or is it a bad idea?

> --- /dev/null
> +++ b/parser/apparmor.service
> @@ -0,0 +1,16 @@
> +[Unit]
> +Description=Load AppArmor profiles
> +DefaultDependencies=no
> +Before=sysinit.target
> +After=systemd-journald-audit.socket
> +ConditionSecurity=apparmor
> +
> +[Service]
> +Type=oneshot
> +ExecStart=/usr/lib/systemd/scripts/apparmor_start.sh
> +ExecReload=/usr/lib/systemd/scripts/apparmor_reload.sh
> +ExecStop=/usr/lib/systemd/scripts/apparmor_stop.sh
> +RemainAfterExit=yes
> +
> +[Install]
> +WantedBy=multi-user.target

Looks mostly like the current openSUSE apparmor.service (which is known 
to work), with Exec* adjusted to your new scripts.

FYI: You aren't the only one who started working on apparmor.service. 
Several other distributions have their own variants of it, see 
- https://bugs.launchpad.net/apparmor/+bug/1503762/
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=796589
- https://bugs.gentoo.org/show_bug.cgi?id=555388
- https://aur.archlinux.org/packages/apparmor/ (git clone to get the 
  files)

I don't know the state of each of them, and it shouldn't stop us from 
getting apparmor.service into the upstream release. Nevertheless it 
might be a good idea to have a look at each of them, especially when it 
comes to the dependencies (for example, the Debian maintainer thinks 
After=systemd-journald-audit.socket is superfluous and dropped it). 
Ideally we can agree on an apparmor.service that all distributions can 
and will use ;-)

> --- /dev/null
> +++ b/parser/apparmor_reload.sh
> @@ -0,0 +1,4 @@
> +#!/bin/bash
> +
> +/usr/lib/systemd/scripts/apparmor_stop.sh

That's a very bad idea - unloading the profiles means you remove the 
AppArmor confinement from all running processes, and...

> +/sbin/apparmor_parser -r /etc/apparmor.d

... afterwards the profiles get loaded again, but will NOT be applied to 
running processes (= leaving them unconfined!)

It's already bad enough if someone accidently uses "systemctl restart 
apparmor" or "rcapparmor restart" (which gets mapped to stop, then 
start) [1], but adding the same mistake to the reload script would be a 
bad idea ;-)

Only profiles that no longer exist in /etc/apparmor.d/ should be unloaded 
on reload. Have a look at /lib/apparmor/rc.apparmor.functions and the 
current initscript for details.

> --- /dev/null
> +++ b/parser/apparmor_start.sh
> @@ -0,0 +1,4 @@
> +#!/bin/bash
> +/sbin/apparmor_parser -r /etc/apparmor.d

Looks good.

> --- /dev/null
> +++ b/parser/apparmor_stop.sh
> @@ -0,0 +1,20 @@
> +#!/bin/bash
> +SECURITYFS=/sys/kernel/security
> +APPARMOR_MOUNTPOINT=$SECURITYFS/apparmor
> +
> +if [ ! -w "$APPARMOR_MOUNTPOINT/.remove" ] ; then
> +	exit 1
> +fi
> +
> +PROFILES=`sed -e "s/ (\(enforce\|complain\))$//"
> $APPARMOR_MOUNTPOINT/profiles` +
> +retval=0
> +for profile in $PROFILES; do
> +	echo -n "$profile" > $APPARMOR_MOUNTPOINT/.remove
> +	rc=$?
> +	if [ ${rc} -ne 0 ]; then
> +		retval=${rc}
> +	fi
> +done
> +exit $retval

Looks good at the first view, but I'll have to compare it with the 
existing scripts to be sure.

Regards,

Christian Boltz

[1] If you know any way to convince the systemd devs to add an 
    ExecRestart= option that can overrite the "stop, then start" default 
    behaviour, please do that. I already tried without success :-(

-- 
Du willst von SATA (alt, 7.200 RPM) jetzt gleich auf SSDs (!) im RAID50
(!!!!)?
Meinst Du nicht, Papierflieger und Überschalljet gäbe es noch ein paar
sinnvolle filligranere Abstufungen? [Peer Heinlein in postfixbuch-users]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20161020/4a13fc10/attachment.pgp>