[libvirt] [PATCH] conf: Fix parsing of seclabels without model
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@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 224aec5..5316b59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3102,22 +3102,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, def->baselabel = p; } - /* Only parse model, if static labelling, or a base - * label is set, or doing active XML - */ - if (def->type == VIR_DOMAIN_SECLABEL_STATIC || - def->baselabel || - (!(flags & VIR_DOMAIN_XML_INACTIVE) && - def->type != VIR_DOMAIN_SECLABEL_NONE)) { - - p = virXPathStringLimit("string(./@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL && def->type != VIR_DOMAIN_SECLABEL_NONE) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - } - def->model = p; - } + /* Always parse model */ + p = virXPathStringLimit("string(./@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + def->model = p; return def; @@ -3129,10 +3117,12 @@ error: static int virSecurityLabelDefsParseXML(virDomainDefPtr def, xmlXPathContextPtr ctxt, + virCapsPtr caps, unsigned int flags) { - int i = 0, n; + int i, j, n; xmlNodePtr *list = NULL, saved_node; + virCapsHostPtr host = &caps->host; /* Check args and save context */ if (def == NULL || ctxt == NULL) @@ -3159,14 +3149,26 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, ctxt->node = saved_node; VIR_FREE(list); - /* Checking missing model information - * when there is more than one seclabel */ - if (n > 1) { - for(; n; n--) { - if (def->seclabels[n - 1]->model == NULL) { + /* Check missing model information */ + for (i = j = 0; i < n; i++) { + /* If model is missing, try to assign it based on driver's + * capabilities. + */ + if (def->seclabels[i]->model == NULL) { + /* Check if there's any host's security model that wasn't + * assigned yet. + */ + if (j >= host->nsecModels) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing security model " - "when using multiple labels")); + _("missing security model and " + "it can't be assigned based on " + "host's capabilities")); + goto error; + } + /* Copy model from host. */ + def->seclabels[i]->model = strdup(host->secModels[j++].model); + if (def->seclabels[i]->model == NULL) { + virReportOOMError(); goto error; } } @@ -3175,8 +3177,8 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, error: ctxt->node = saved_node; - for (; i > 0; i--) { - virSecurityLabelDefFree(def->seclabels[i - 1]); + for (i = 0; i < n; i++) { + virSecurityLabelDefFree(def->seclabels[i]); } VIR_FREE(def->seclabels); def->nseclabels = 0; @@ -8166,7 +8168,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, /* analysis of security label, done early even though we format it * late, so devices can refer to this for defaults */ - if (virSecurityLabelDefsParseXML(def, ctxt, flags) == -1) + if (virSecurityLabelDefsParseXML(def, ctxt, caps, flags) == -1) goto error; /* Extract domain memory */ -- 1.7.12
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@linux.vnet.ibm.com> --- 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). Jirka
On 08/30/2012 02:12 PM, 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@linux.vnet.ibm.com> --- 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.
I don't agree. If you save a domain using the latest libvirt and using an earlier version (I used v0.9.10) and then check the XML included in each save file, you'll see something similar to this for the latest libvirt version: ... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>unconfined_u:system_r:svirt_t:s0:c323,c995</label> <imagelabel>unconfined_u:object_r:svirt_image_t:s0:c323,c995</imagelabel> </seclabel> <seclabel type='dynamic' model='dac' relabel='yes'> <label>0:0</label> <imagelabel>0:0</imagelabel> </seclabel> </domain> And this for v0.9.10: ... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>system_u:system_r:svirt_t:s0:c175,c437</label> <imagelabel>system_u:object_r:svirt_image_t:s0:c175,c437</imagelabel> </seclabel> </domain> The biggest difference is the seclabel for DAC.
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).
Not sure if I understood you point. I can't find a scenario that an older libvirtd will try to parse a XML generated by an earlier libvirtd version. I think that this will only happen if you save a guest, downgrade libvirt and then restore the guest.
Jirka
On Thu, Aug 30, 2012 at 15:01:34 -0300, Marcelo Cerri wrote:
On 08/30/2012 02:12 PM, 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@linux.vnet.ibm.com> --- 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.
I don't agree. If you save a domain using the latest libvirt and using an earlier version (I used v0.9.10) and then check the XML included in each save file, you'll see something similar to this for the latest libvirt version:
... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>unconfined_u:system_r:svirt_t:s0:c323,c995</label>
<imagelabel>unconfined_u:object_r:svirt_image_t:s0:c323,c995</imagelabel> </seclabel> <seclabel type='dynamic' model='dac' relabel='yes'> <label>0:0</label> <imagelabel>0:0</imagelabel> </seclabel> </domain>
And this for v0.9.10:
... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>system_u:system_r:svirt_t:s0:c175,c437</label> <imagelabel>system_u:object_r:svirt_image_t:s0:c175,c437</imagelabel> </seclabel> </domain>
The biggest difference is the seclabel for DAC.
Exactly. But while latest libvirt can happily parse the XML generated with 0.9.13 (or older), it will fail to load the XML the latest libvirt itself generated. Thus, if the generated XML is missing something and you would need to guess that info when parsing the XML, it's the formatting code that needs to be fixed to output what is needed by the parsing code. But that's mostly commenting the code that I don't quite understand why it is needed; the code that fills in missing seclabel models, while all seclabel elements in both XMLs contain model attributes. Jirka
On Thu, Aug 30, 2012 at 21:32:32 +0200, Jiri Denemark wrote:
On Thu, Aug 30, 2012 at 15:01:34 -0300, Marcelo Cerri wrote:
On 08/30/2012 02:12 PM, 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@linux.vnet.ibm.com> --- 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.
I don't agree. If you save a domain using the latest libvirt and using an earlier version (I used v0.9.10) and then check the XML included in each save file, you'll see something similar to this for the latest libvirt version:
... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>unconfined_u:system_r:svirt_t:s0:c323,c995</label>
<imagelabel>unconfined_u:object_r:svirt_image_t:s0:c323,c995</imagelabel> </seclabel> <seclabel type='dynamic' model='dac' relabel='yes'> <label>0:0</label> <imagelabel>0:0</imagelabel> </seclabel> </domain>
And this for v0.9.10:
... </devices> <seclabel type='dynamic' model='selinux' relabel='yes'> <label>system_u:system_r:svirt_t:s0:c175,c437</label> <imagelabel>system_u:object_r:svirt_image_t:s0:c175,c437</imagelabel> </seclabel> </domain>
The biggest difference is the seclabel for DAC.
Exactly. But while latest libvirt can happily parse the XML generated with 0.9.13 (or older), it will fail to load the XML the latest libvirt itself generated. Thus, if the generated XML is missing something and you would need to guess that info when parsing the XML, it's the formatting code that needs to be fixed to output what is needed by the parsing code. But that's mostly commenting the code that I don't quite understand why it is needed; the code that fills in missing seclabel models, while all seclabel elements in both XMLs contain model attributes.
OK, now I think I understand the issue. The problem is that in certain cases, where model is not required for compatibility reasons, we would just ignore it instead of parsing it anyway. Thus we end up in a situation that should never happen because model is always required when multiple seclabels are used. With this understanding, I commented on your original patch. Jirka
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@linux.vnet.ibm.com> --- 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. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@linux.vnet.ibm.com> --- 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.
Daniel
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@linux.vnet.ibm.com> --- 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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/30/2012 03:20 PM, Daniel P. Berrange wrote:
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@linux.vnet.ibm.com> --- 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.
Ok. But in which scenario would this happen? It doesn't seem to make sense to save a guest with an earlier libvirt version and restore it in an older libvirt.
Daniel
On Thu, Aug 30, 2012 at 03:31:05PM -0300, Marcelo Cerri wrote:
On 08/30/2012 03:20 PM, Daniel P. Berrange wrote:
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@linux.vnet.ibm.com> --- 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.
Ok. But in which scenario would this happen? It doesn't seem to make sense to save a guest with an earlier libvirt version and restore it in an older libvirt.
I wish that was the case, but unfortunately people do want todo exactly that :-( More particularly for live-migration betweeen different releases of RHEL, but save+restore too. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Aug 30, 2012 at 12:11:18 -0700, Daniel P. Berrange wrote:
On Thu, Aug 30, 2012 at 03:31:05PM -0300, Marcelo Cerri wrote:
On 08/30/2012 03:20 PM, Daniel P. Berrange wrote:
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.
Ok. But in which scenario would this happen? It doesn't seem to make sense to save a guest with an earlier libvirt version and restore it in an older libvirt.
I wish that was the case, but unfortunately people do want todo exactly that :-( More particularly for live-migration betweeen different releases of RHEL, but save+restore too.
Right, people like to upgrade their clusters incrementally and still be able to live-migrate domains between any two nodes of the cluster (of course, except for the ones being upgraded) rather than having to split nodes in two groups and have only uni-directional migration between nodes that do not belong to the same group. Obviously, this needs to work only for domains that do not explicitly use any feature that was introduced by the new libvirt. Jirka
On 08/30/2012 04:11 PM, Daniel P. Berrange wrote:
I wish that was the case, but unfortunately people do want todo exactly that :-( More particularly for live-migration betweeen different releases of RHEL, but save+restore too.
Ok. But this still doesn't make this patch invalid. The problem regarding save+restore occurs even if you use the same libvirt version to save and restore a guest, and it'll occur even if we exclude the <seclabel> for model=dac from the save file. I can write another patch excluding the <seclabel> for model=dac if this is really needed. Do you think that it should be removed only from the save file or it also should be removed from dumpxml output for example?
Daniel
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@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 224aec5..5316b59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3102,22 +3102,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, def->baselabel = p; }
- /* Only parse model, if static labelling, or a base - * label is set, or doing active XML - */ - if (def->type == VIR_DOMAIN_SECLABEL_STATIC || - def->baselabel || - (!(flags & VIR_DOMAIN_XML_INACTIVE) && - def->type != VIR_DOMAIN_SECLABEL_NONE)) { - - p = virXPathStringLimit("string(./@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL && def->type != VIR_DOMAIN_SECLABEL_NONE) { - virReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - } - def->model = p; - } + /* Always parse model */ + p = virXPathStringLimit("string(./@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + def->model = p;
return def;
This hunk looks correct.
@@ -3129,10 +3117,12 @@ error: static int virSecurityLabelDefsParseXML(virDomainDefPtr def, xmlXPathContextPtr ctxt, + virCapsPtr caps, unsigned int flags) { - int i = 0, n; + int i, j, n; xmlNodePtr *list = NULL, saved_node; + virCapsHostPtr host = &caps->host;
/* Check args and save context */ if (def == NULL || ctxt == NULL) @@ -3159,14 +3149,26 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, ctxt->node = saved_node; VIR_FREE(list);
- /* Checking missing model information - * when there is more than one seclabel */ - if (n > 1) { - for(; n; n--) { - if (def->seclabels[n - 1]->model == NULL) { + /* Check missing model information */ + for (i = j = 0; i < n; i++) { + /* If model is missing, try to assign it based on driver's + * capabilities. + */ + if (def->seclabels[i]->model == NULL) { + /* Check if there's any host's security model that wasn't + * assigned yet. + */ + if (j >= host->nsecModels) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing security model " - "when using multiple labels")); + _("missing security model and " + "it can't be assigned based on " + "host's capabilities")); + goto error; + } + /* Copy model from host. */ + def->seclabels[i]->model = strdup(host->secModels[j++].model); + if (def->seclabels[i]->model == NULL) { + virReportOOMError(); goto error; } }
But this seems wrong. The only case when model can be missing is when there is just one seclabel defined and either type is none or type is dynamic, baselabel is not defined and INACTIVE flags is set. This is the only case in which we need to guess what model was used and we should be able to just use the first secModel for that. That is the code is not incorrect but relaxes the requirements too much. We should require model to be present in all cases except for the one case needed for backward compatibility. Jirka
On 08/30/2012 06:42 PM, Jiri Denemark wrote:
But this seems wrong. The only case when model can be missing is when there is just one seclabel defined and either type is none or type is dynamic, baselabel is not defined and INACTIVE flags is set. This is the only case in which we need to guess what model was used and we should be able to just use the first secModel for that. That is the code is not incorrect but relaxes the requirements too much. We should require model to be present in all cases except for the one case needed for backward compatibility.
Ok, no problem. I'll provide a new version of this patch that is more restricted when assigning a model and another patch suppressing seclabel for DAC when it is not explicitly defined for a guest.
Jirka
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Marcelo Cerri