<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 3, 2016 at 6:15 PM, Guido Günther <span dir="ltr"><<a href="mailto:agx@sigxcpu.org" target="_blank">agx@sigxcpu.org</a>></span> wrote:</div><div class="gmail_quote"><br></div><div class="gmail_quote">Thanks for your feedback Guido!</div><div class="gmail_quote"><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:<br>
> When parsing labels virt-aa-helper does no more pass<br>
> VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the<br>
> changes of a89f05ba. For those it had to switch from<br>
<br>
</span>I wouldn't call it mitigate. It was rather a fix.<span class="gmail-"><br></span></blockquote><div> </div><div>True, I really meant no offense nor wanted to degrade the fix by wording it badly.</div><div>I reworded the commit message locally so that the next re-submission (if needed) will call it a fix correctly.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id<br>
> (ctl->def->id) as it is part of the socket path now which is needed for the<br>
> aa profile.<br>
><br>
> But that turned out to break non apparmor seclabels as well as apparmor<br>
> seclabels in xmls without labels.<br>
<br>
</span>While I understand the "non apparmor seclabel" part I don't understand<br>
what you mean by "apparmor seclabels in xmls without labels".</blockquote><div><br></div><div><div>Yeah what I meant with "apparmor seclabels in xmls without labels" was misleading.</div><div>The whole reproduction of the case started very shaky for me which I think has slipped into the description of this paragraph.</div><div>What I wanted to make clear is this:</div><div><br></div><div>If you are in the error case e.g. after adding:<br></div><div>  <seclabel type='dynamic' model='dac' relabel='yes'/><br></div><div>Note: Remember the error is now "... cannot load AppArmor profile ..." pointing to apparmor<br></div><div><br></div><div>That neither adding an "apparmor seclabel without label" ...</div><div><div>   <seclabel type='dynamic' model='apparmor' relabel='yes'/><br></div><div><br></div><div>Nor adding "apparmor seclabel with label" ...</div></div><div><div>  <seclabel type='dynamic' model='apparmor' relabel='yes'></div><div>    <label>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</label></div><div>    <imagelabel>libvirt-a4b7c764-988b-4a88-a614-5399b745ca94</imagelabel></div><div>  </seclabel></div></div><div><br></div><div>... will get you going again.</div><div><br></div><div>So while the message "...cannot load AppArmor profile..." is pointing to apparmor, it actually is an error parsing the dac seclabel.<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Debian<br>
based distros e.g. virtinst creates all VMs without seclabels and we let<br>
the dac and apparmor security drivers do the work. I.e. this gets added<br>
to the domains live xml upon start.<br></blockquote><div><br></div><div>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.</div><div>Once shut down all content of the seclabel is gone in the dumpxml.</div><div><br></div><div>That is the reason why in the common cases there is no issue to be seen.</div><div>There is nothing in the xml to be parsed and the drivers add their stuff.</div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">> In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore<br>
> virSecurityLabelDefParseXML insists on finding labels. Cases:<br>
> - non-apparmor seclabel - virt-aa-helper breaks<br>
> - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks<br>
> This was not spotted due to labels getting dynamically created on<br>
> definition.<br>
<br>
</span>As far as I understand it dynamic labels get created on domain start not<br>
domain definition (and are updated on e.g. device plug).</blockquote><div><br></div><div>Yes I agree.</div><div>If I define an xml without a label the drivers add their stuff.</div><div>On live (dumpxml) I then see it completely with all labels just as you posted.</div><div>And once I shut down the guest and dumpxml it again it does not contain the seclabel anymore.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> So "define, start, stop" works. But "define, edit (add label), start"<br>
> does not.<br>
<br>
</span>What do you add during the edit step. Can you provide an example?<br></blockquote><div><br></div><div>As shown above add a dac seclabel without label/imagelabel child elements:</div><div>  <seclabel type='dynamic' model='dac' relabel='yes'/><br></div><div> </div><div>[...]</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><br>
> Testcase with virt-aa-helper on xml file:<br>
>  virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml<br>
>    virt-aa-helper: error: could not parse XML<br>
>    virt-aa-helper: error: could not get VM definition<br>
> (That should have printed a valid apparmor profile)<br>
<br>
</span>/usr/lib/libvirt/virt-aa-<wbr>helper -d -r -p 0 -u libvirt-a9287b6e-ca06-42fe-<wbr>b1a2-06830752843a < /etc/libvirt/qemu/wheezy.xml<br>
virt-aa-helper:<br>
/etc/apparmor.d/libvirt/<wbr>libvirt-a9287b6e-ca06-42fe-<wbr>b1a2-06830752843a.files<br>
virt-aa-helper:<br>
  "/var/log/libvirt/**/wheezy.<wbr>log" w,<br></blockquote><div>[...]</div><div><br></div><div>Yep that is how the good case.</div><div>As outlined that is either without seclabel in the xml (default) or with fully complete seclabels (+imagelabel +label).</div><div>But as soon as there is the simple dac seclabel added it should show the issue.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I don't understand why VIR_DOMAIN_DEF_PARSE_SKIP_<wbr>VALIDATE is not sufficient.<br></blockquote><div><br></div><div>As far as I understand the reason is that It is not a part of the validation that fails.</div><div>The flag VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE does not skip the parsing of the seclabels, but that is where it fails already.</div><div>In the function virSecurityLabelDefParseXML the only flag that skips the insisting on labels being present always was VIR_DOMAIN_DEF_PARSE_INACTIVE.</div><div><br></div><div>Now I understand that you had to drop VIR_DOMAIN_DEF_PARSE_INACTIVE for the fix in dfbc9a83.</div><div>But that caused the regression that "<seclabel type='dynamic' model='dac' relabel='yes'/>" can no more be added explicitly.</div><div>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.</div><div><br></div></div>
</div></div>