Re: [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.

[Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>]

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".

On Sun, Jun 10, 2018 at 9:08 AM, Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:

On 06/09/2018 09:29 PM, intrigeri+libvirt@xxxxxxxx wrote:
> From: intrigeri <intrigeri+libvirt@xxxxxxxx>
>
> 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
> ---
>  examples/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> index 6869685c05..e32402a904 100644
> --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper
> @@ -50,6 +50,7 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper {
>    @{HOME}/** r,
>    /var/lib/libvirt/images/ r,
>    /var/lib/libvirt/images/** r,
> +  /var/lib/nova/instances/_base/* r
>    /{media,mnt,opt,srv}/** r,
>    # For virt-sandbox
>    /{,var/}run/libvirt/**/[sv]d[a-z] r,
>

I am not convinced this is correct fix. This would fix only some use
cases where base image is under /var/lib/nova/.. path. The root cause
seems to be virt-aa-helper not putting backing store into the profile
but looking into the code it does. So we might need to debug
virt-aa-helper adding disks with backing chain instead of blindly
allowing some path.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

--

Christian Ehrhardt

Software Engineer, Ubuntu Server

Canonical Ltd

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list