On 06/11/2018 07:34 AM, Christian Ehrhardt wrote:
> Hi Intrigeri and Michal,
> IMHO this is the right path as it is stage one of the two stage process to
> allow images for guests with apparmor.
>
> Stage 1:
> Virt-aa-helper has to be allowed to access the paths to evaluate images for
> some attributes and e.g. backing chains.
> Due to that virt-aa-helper needs access to the paths we expect the images
> in.
> Further more uncommon paths shall be handled by a local include #include
> <local/usr.lib.libvirt.virt-aa-helper> where a user can add his special
> cases.
>
> Stage 2:
> virt-aa-helper generated the guest apparmor profile, and in there will be
> entries for all the used image files of a backing chain.
> The guest can work due to the file allowing this.
>
> With virt-aa-helper not able to read those paths in stage1 some entries
> might be missing for stage2.
> IMHO here the suggestion is just to open up stage1 a bit to get it working.
>
> But if that is right and we are considering the change, the we should go
> further and add more.
> As a background, I had the proposed rule in Ubuntu for quite a while,
> together with a bunch of other common (for us) paths.
> I always considered them a downstream decision as nova could have been
> somewhere else for other downstreams.
> The same applies to paths for other tools.
>
> In fact you'll see in the rules that there is some history to this with not
> only nova, but also eucalyptus, then the ubuntu specific uvtool as well as
> two snap specific paths which actually would be useful everywhere.
>
> +  # nova base images (LP: #907269)
> +  /var/lib/nova/images/** r,
> +  /var/lib/nova/instances/_base/** r,
> +  # nova snapshots (LP: #1244694)
> +  /var/lib/nova/instances/snapshots/** r,
> +  # nova base/snapshot files in snapped nova (LP: #1644507)
> +  /var/snap/nova-hypervisor/common/instances/_base/** r,
> +  /var/snap/nova-hypervisor/common/instances/snapshots/** r,
> +  # eucalyptus (LP: #564914)
> +  /var/lib/eucalyptus/instances/**/disk* r,
> +  # eucalyptus loader (LP: #637544)
> +  /var/lib/eucalyptus/instances/**/loader* r,
> +  # for uvtool
> +  /var/lib/uvtool/libvirt/images/** r,
> +  # for multipass
> +  /var/snap/multipass/common/data/multipassd/vault/instances/** r
>
> Since this is only opening up the paths for virt-aa-helper and not the
> guest it is rather safe.
> And I always likes it more to explicitly list the paths we want instead of
> the almost too global /**/disk{,.*} rule.
>
> So I'm +0.75, lacking the final 0.25 only for the reason of considering it
> a downstream thing so far.
> OTOH if we can agree on the paths I'd totally love to see these paths
> upstream, but then I'd prefer to add like all the paths I referred and not
> only "the one just found".

Thank you for your exhaustive explanation. You've convinced me that it's
safe to merge this patch. However, what I still don't quite understand
is: Nova uses that path for ages, doesn't it? How come we've hit the bug
only now?

>
> On Sun, Jun 10, 2018 at 9:08 AM, Michal Prívozník <mprivozn@redhat.com>
> wrote:
>
>> On 06/09/2018 09:29 PM, intrigeri+libvirt@boum.org wrote:
>>> From: intrigeri <intrigeri+libvirt@boum.org>
>>>
>>> As reported on https://bugs.debian.org/892431, without this rule, when
>> launching
>>> a QEMU KVM instance, an error occurs immediately upon launching the QEMU
>>> process such as:
>>>
>>>   Could not open backing file: Could not open
>>>   '/var/lib/nova/instances/_base/affe96668a4c64ef380ff1c71b4cae
>> c17039080e':
>>>   Permission denied
>>>
>>> The other instance disk images are already covered by the existing rule:
>>>
>>>   /**/disk{,.*}

Oh, I can't merge the patch as-is because it is missing S-O-B line which
is required (https://libvirt.org/hacking.html). Also, it would be nice
if you can use your real name.

Michal