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(a)redhat.com&gt; > wrote: > >> On 06/09/2018 09:29 PM, intrigeri+libvirt(a)boum.org wrote: >>> From: intrigeri <intrigeri+libvirt(a)boum.org&gt; >>> >>> 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

On 06/11/2018 08:43 AM, Christian Ehrhardt wrote: > On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn(a)redhat.com&gt; > wrote: > >> On 06/11/2018 07:34 AM, Christian Ehrhardt wrote: >>> >>> <snip/> >> 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? >> > > We didn't Ubuntu had this as downstream Delta as long as I can remember - I > guess only now someone drives Nova in Debian to that point. > And all that have done further have done local overrides. Ah, that explains it. > >>> >>> On Sun, Jun 10, 2018 at 9:08 AM, Michal Prívozník <mprivozn(a)redhat.com > >>> wrote: >>> >>>> On 06/09/2018 09:29 PM, intrigeri+libvirt(a)boum.org wrote: >>>>> From: intrigeri <intrigeri+libvirt(a)boum.org&gt; >>>>> >>>>> 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. >> > > We had the real name discussion before, but at least the S-O-B as agreed > last time should be added. To my recollection even the usage of real name is a must (although we don't have any means to verify that). Anyway, the point being we don't allow any nicknames, TLAs (three letter acronyms), and so on. IANAL, but I guess the reason for that is to protect libvirt from any future lawsuits. > > And I'd ask for an opinion on the "other" paths I listed - I can only > recommend adding as much as we can commonly agree to be useful. > To avoid coming back every few months adding another such line :-) Well, yes we can add them. However, at that point I guess we might want to revisit virt-aa-helper design? I mean, it's nice that we have two step permission building, but then we might end up chasing changing consumers of libvirt. So I guess if we want to preserve the two step process we should encourage libvirt consumers to write their own rules and have virt-aa-helper match against that. Of course, libvirt would still provide some sensible default so that it works out of box, but in this case OpenStack would provide an additional file to enable access to /var/lib/nova/... (which is a private path to Nova and could be considered internal implementation of Nova and therefore libvirt should not care). Also, whenever there's a patch changing the path in Nova code base it can be coupled with apparmor profile adjustment. I'm not sure if there's a way to achieve this.

separate directory and all other projects would store their profiles there and libvirt would just include them all (among with the defaults file described above). Then virt-aa-helper would generate its own rules in second step. Michal

Hi, Michal Prívozník: > On 06/11/2018 08:43 AM, Christian Ehrhardt wrote: >> On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn(a)redhat.com&gt; >> wrote: >>> Also, it would be nice if you can use your real name. >> >> We had the real name discussion before, but at least the S-O-B as agreed >> last time should be added. > To my recollection even the usage of real name is a must (although we > don't have any means to verify that). Anyway, the point being we don't > allow any nicknames, TLAs (three letter acronyms), and so on. IANAL, but > I guess the reason for that is to protect libvirt from any future lawsuits. FTR here are the relevant bits of the previous discussion about this topic: https://www.redhat.com/archives/libvir-list/2016-December/msg00841.html https://www.redhat.com/archives/libvir-list/2016-December/msg00856.html Then the conclusion of that discussion has been applied consistently (although implicitly): 4 commits of mine have been applied to Git since Daniel wrote that this was a valid exception, and nobody raised this topic again until today. Cheers,

<snip/> 0001-AppArmor-allow-virt-aa-helper-read-access-to-Nova-s-.patch >From f4ea2da3ddeb275b3bd08b33ebe858dd6f7f274f Mon Sep 17 00:00:00 2001 From: intrigeri <intrigeri+libvirt(a)boum.org&gt; Date: Sat, 9 Jun 2018 19:26:26 +0000 Subject: [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files. 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/affe96668a4c64ef380ff1c71b4caec17039080e': Permission denied The other instance disk images are already covered by the existing rule: /**/disk{,.*} r Signed-off-by: intrigeri <intrigeri(a)boum.org&gt;