[libvirt] [PATCH] virt-aa-helper: handle more disk images

[intrigeri]

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