Re: [libvirt] [PATCH] virt-aa-helper: handle more disk images
Hi,
Cedric Bosdonnat:
On Tue, 2017-12-12 at 15:01 +0100, intrigeri wrote:
> Cédric Bosdonnat:
> > This commit helps users allowing access to their images by adding their
> > own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper.
> > […]
> > profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
> > #include <abstractions/base>
> > + #include <local/usr.lib.libvirt.virt-aa-helper>
>
> The packaging helper we use in Debian adds exactly the same line at
> the *end* of the profile (and actually, at the end of almost every
> AppArmor profile included in Debian and derivatives); I don't know why
> it's added at the end and not at the beginning. I suspect Jamie will
> know better.
>
> If there's no strong reason to add this line in the beginning of the
> profile, I suggest we add it at the end instead, so we avoid changing
> behaviour subtly once this gets merged upstream and we drop the
> Debian-specific line.
>
> Other than this, ACK from me on the proposed profile modifications.
I'm perfectly fine in having that include at the end of the
profile. I'll
push with that change.
Thanks! This will allow us to remove half of
apparmor_profiles_local_include.patch we carry in Debian.
Now, two follow-up questions:
1. Doing the same for usr.sbin.libvirtd?
The apparmor_profiles_local_include.patch Debian patch does the same
for usr.sbin.libvirtd:
diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd
index fa4ebb3..5505bf6 100644
--- a/examples/apparmor/usr.sbin.libvirtd
+++ b/examples/apparmor/usr.sbin.libvirtd
@@ -90,4 +90,7 @@
/usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix,
}
+
+ # Site-specific additions and overrides. See local/README for details.
+ #include <local/usr.sbin.libvirtd>
}
Cédric and others, what about upstreaming this as well?
2. Impact on packaging for distros that were already managing this
local file themselves & differently
… i.e. I guess mostly Debian and derivatives, that use dh_apparmor.
If I got the build system changes right,
local/usr.lib.libvirt.virt-aa-helper will be created at installation
time so in practice it'll be shipped by distro packages.
I *think* it's not a problem with dh_apparmor because the generated
postinst scripts only manage that file if it does not exist yet (so it
should be a no-op if dpkg has already installed it), and similarly the
code for purging in postrm should work just fine if dpkg has already
deleted it.
But local/usr.lib.libvirt.virt-aa-helper becomes a conffile, which
previously it was not managed by dpkg. I don't know how this is
handled by dpkg. I suspect it might be easier to comment out:
INSTALL_DATA_LOCAL += install-apparmor-local
UNINSTALL_LOCAL += uninstall-apparmor-local
… and keep the current behaviour, for consistency with how all other
local/* override files are handled in Debian/Ubuntu.
Guido, Ubuntu packagers, what do you think?
Cheers,
--
intrigeri