[libvirt] [PATCH] fix parsing security labels from virt-aa-helper

[Christian Ehrhardt]

On Thu, Nov 3, 2016 at 6:15 PM, Guido Günther <agx at sigxcpu.org> wrote:

Thanks for your feedback Guido!

On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:
> > When parsing labels virt-aa-helper does no more pass
> > VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the
> > changes of a89f05ba. For those it had to switch from
>
> I wouldn't call it mitigate. It was rather a fix.
>

True, I really meant no offense nor wanted to degrade the fix by wording it
badly.
I reworded the commit message locally so that the next re-submission (if
needed) will call it a fix correctly.

> VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id
> > (ctl->def->id) as it is part of the socket path now which is needed for
> the
> > aa profile.
> >
> > But that turned out to break non apparmor seclabels as well as apparmor
> > seclabels in xmls without labels.
>
> While I understand the "non apparmor seclabel" part I don't understand
> what you mean by "apparmor seclabels in xmls without labels".


Yeah what I meant with "apparmor seclabels in xmls without labels" was
misleading.
The whole reproduction of the case started very shaky for me which I think
has slipped into the description of this paragraph.
What I wanted to make clear is this:

If you are in the error case e.g. after adding:
  <seclabel type='dynamic' model='dac' relabel='yes'/>
Note: Remember the error is now "... cannot load AppArmor profile ..."
pointing to apparmor

That neither adding an "apparmor seclabel without label" ...
   <seclabel type='dynamic' model='apparmor' relabel='yes'/>

Nor adding "apparmor seclabel with label" ...
  <seclabel type='dynamic' model='apparmor' relabel='yes'>
    <label>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</label>
    <imagelabel>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</imagelabel>
  </seclabel>

... will get you going again.

So while the message "...cannot load AppArmor profile..." is pointing to
apparmor, it actually is an error parsing the dac seclabel.


> On Debian
> based distros e.g. virtinst creates all VMs without seclabels and we let
> the dac and apparmor security drivers do the work. I.e. this gets added
> to the domains live xml upon start.
>

That works the same for my environment - if nothing is in the xml the
security drivers add their stuff as needed and it can be seen in the active
domain dumpxml with label and imagelabel set.
Once shut down all content of the seclabel is gone in the dumpxml.

That is the reason why in the common cases there is no issue to be seen.
There is nothing in the xml to be parsed and the drivers add their stuff.
But IMHO it is valid to add the seclabel to the xml explicitly and in that
case the parsing as called from virt-aa-helper fails.

> In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore
> > virSecurityLabelDefParseXML insists on finding labels. Cases:
> > - non-apparmor seclabel - virt-aa-helper breaks
> > - apparmor seclabel without labels on a defined domain - virt-aa-helper
> breaks
> > This was not spotted due to labels getting dynamically created on
> > definition.
>
> As far as I understand it dynamic labels get created on domain start not
> domain definition (and are updated on e.g. device plug).


Yes I agree.
If I define an xml without a label the drivers add their stuff.
On live (dumpxml) I then see it completely with all labels just as you
posted.
And once I shut down the guest and dumpxml it again it does not contain the
seclabel anymore.


> > So "define, start, stop" works. But "define, edit (add label), start"
> > does not.
>
> What do you add during the edit step. Can you provide an example?
>

As shown above add a dac seclabel without label/imagelabel child elements:
  <seclabel type='dynamic' model='dac' relabel='yes'/>

[...]

>
> > Testcase with virt-aa-helper on xml file:
> >  virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml
> >    virt-aa-helper: error: could not parse XML
> >    virt-aa-helper: error: could not get VM definition
> > (That should have printed a valid apparmor profile)
>
> /usr/lib/libvirt/virt-aa-helper -d -r -p 0 -u libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a
> < /etc/libvirt/qemu/wheezy.xml
> virt-aa-helper:
> /etc/apparmor.d/libvirt/libvirt-a9287b6e-ca06-42fe-b1a2-06830752843a.files
> virt-aa-helper:
>   "/var/log/libvirt/**/wheezy.log" w,
>
[...]

Yep that is how the good case.
As outlined that is either without seclabel in the xml (default) or with
fully complete seclabels (+imagelabel +label).
But as soon as there is the simple dac seclabel added it should show the
issue.

I don't understand why VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE is not sufficient.
>

As far as I understand the reason is that It is not a part of the
validation that fails.
The flag VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE does not skip the parsing of
the seclabels, but that is where it fails already.
In the function virSecurityLabelDefParseXML the only flag that skips the
insisting on labels being present always was VIR_DOMAIN_DEF_PARSE_INACTIVE.

Now I understand that you had to drop VIR_DOMAIN_DEF_PARSE_INACTIVE for the
fix in dfbc9a83.
But that caused the regression that "<seclabel type='dynamic' model='dac'
relabel='yes'/>" can no more be added explicitly.
So since we now have this orthogonal need to not set
VIR_DOMAIN_DEF_PARSE_INACTIVE but at the same time need to "skip insisting
on labels being set" I added the new flag to imply the latter when called
from virt-aa-helper.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161104/80af2523/attachment-0001.htm>