On Thu, Nov 3, 2016 at 6:15 PM, Guido Günther <agx@xxxxxxxxxxx> 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.
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
