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

intrigeri+libvirt@boum.org posted 1 patch 5 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180609192902.20986-1-intrigeri+libvirt@boum.org
Test syntax-check passed
examples/apparmor/usr.lib.libvirt.virt-aa-helper | 1 +
1 file changed, 1 insertion(+)
[libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by intrigeri+libvirt@boum.org 5 years, 10 months ago
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/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,
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Michal Prívozník 5 years, 10 months ago
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/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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Christian Ehrhardt 5 years, 10 months ago
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@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{,.*} 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Michal Prívozník 5 years, 10 months ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Christian Ehrhardt 5 years, 10 months ago
On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn@redhat.com>
wrote:

> 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?
>

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.



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

We had the real name discussion before, but at least the S-O-B as agreed
last time should be added.

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 :-)



> Michal
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Michal Prívozník 5 years, 10 months ago
On 06/11/2018 08:43 AM, Christian Ehrhardt wrote:
> On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn@redhat.com>
> 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@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.
>>
> 
> 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. Perhaps we could have a
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Christian Ehrhardt 5 years, 10 months ago
On Mon, Jun 11, 2018 at 9:41 AM, Michal Prívozník <mprivozn@redhat.com>
wrote:

> On 06/11/2018 08:43 AM, Christian Ehrhardt wrote:
> > On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn@redhat.com>
> > 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@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.
> >>
> >
> > 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.


There are optional incldues coming as a new feature in apparmor.
I have it on my todo list but have to wait a bit until that level of
apparmor is more widely available.
I have thought about that for user driven per guest overrides so far.
Allowing something like a conf.d directory might be possible - need to
evaluate that.

I added (mostly a note to myself) on
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1745114

For now on this patch here we are waiting for the S-O-B resubmit of
intrigeri then you can consider it acked-by by me and push it.

Perhaps we could have a
> 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
>



-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by intrigeri 5 years, 10 months ago
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@redhat.com>
>> 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,
-- 
intrigeri

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Michal Privoznik 5 years, 10 months ago
On 06/11/2018 02:01 PM, intrigeri wrote:
> 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@redhat.com>
>>> 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,
> 

Ah, okay. I've ACKed and pushed your patch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by intrigeri 5 years, 10 months ago
Christian Ehrhardt:
> On Mon, Jun 11, 2018 at 8:12 AM, Michal Prívozník <mprivozn@redhat.com>
> wrote:
>> 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.

No Debian stable release has had AppArmor enabled by default yet,
which I think explains why nobody noticed this problem there so far.

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

Here's an attached patch with S-O-B added. Sorry I did not keep
up-to-date with the contribution guidelines update, I'm not
contributing that often and only to a tiny part of libvirt, so I only
skim over what's happening on the mailing list.

> 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 :-)

Indeed. Perhaps next step is to check if the same paths are used on
other major distros?

Cheers,
-- 
intrigeri

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] AppArmor: allow virt-aa-helper read access to Nova's qcow backing files.
Posted by Michal Prívozník 5 years, 10 months ago
On 06/11/2018 01:50 PM, intrigeri wrote:
> <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@boum.org>
> 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@boum.org>

I'm sorry but as stated earlier, this should be your real name not a
nickname. Also, we don't really thread next versions of a patch under
the previous ones.

Michal

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