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
