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 <[email protected]> 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]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
