Re: [libvirt] [PATCH] conf: Fix parsing of seclabels without model

On Thu, Aug 30, 2012 at 03:17:09PM -0300, Marcelo Cerri wrote: > On 08/30/2012 03:03 PM, Daniel P. Berrange wrote: >> On Thu, Aug 30, 2012 at 07:12:26PM +0200, Jiri Denemark wrote: >>> On Thu, Aug 30, 2012 at 13:19:31 -0300, Marcelo Cerri wrote: >>>> With this patch libvirt tries to assign a model to seclabels when model >>>> is missing. Libvirt will look up at host's capabilities and assign a >>>> model in order to each seclabel that doesn't have a model assigned. >>>> >>>> This patch fixes: >>>> >>>> 1. The problem with existing guests that have a seclabel defined in its XML. >>>> 2. A XML parse error when a guest is restored. >>>> >>>> Signed-off-by: Marcelo Cerri <mhcerri(a)linux.vnet.ibm.com&gt; >>>> --- >>>> src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++------------------------ >>>> 1 file changed, 29 insertions(+), 27 deletions(-) >>> >>> I think this is trying to fix the issue at a wrong place. It's not that XML >>> generated by older libvirtd is not correctly parsed by current libvirtd. The >>> problem is that *current* libvirtd creates an XML that it cannot parse back. >>> Thus we should rather fix the code that formats the XML. >>> >>> On that front, I'm concerned about migration compatibility of this new >>> security driver code. If we just blindly emit <seclabel type='dynamic' >>> model='dac' relabel='yes'> element into the XML, I'm pretty sure an older >>> libvirtd will complain about it even though the element was not used to do >>> anything special that would be done anyway (that is, if labels are the default >>> qemu_user:qemu_group). >> >> Yes, we should not auto-add a <seclabel> for model=dac unless we have >> configured it to auto-assign a private uid:gid pair per guest. If it is >> operating in the mode where it just uses a fixed uid:gid pair we should >> not emit the seclabel. >> > > Can you explain which problem this auto-added <seclabel> for > model=dac can create? I really can see a migration compatibility > issue with it. When a <seclabel> for model=selinux is not defined > for a guest, and SELinux driver is in use, a <seclabel> is also > auto-added to this guest. An old libvirtd (ie < 0.10.0) already knows how to parse & accept a <seclabel> for model=selinux. It will reject a <seclabel> which has model=dac, if that is the first <seclabe> element present. (it will of course ignore the 2nd/3rd/etc <seclabel> element, since it only expected one to exist). So if model=dac is added as the second <seclabel> back compat is ok. If the selinux/apparmour security drivers are disabled though, the <seclabel> with model=dac will be the first & only element. This will confuse old libvirtd.

Daniel