[libvirt] [PATCH 0/2] Don't lose running domains configured with no seclabel
It's not exactly obvious but these two patches fix quite an ugly bug affecting setups without any useful security driver (i.e., either explicitly or implicitly using driver 'none'). When a domain is defined without any <seclabel> element in its XML and started by libvirt, an incorrect <seclabel> element is put into its runtime XML configuration which causes such domain to disappear from libvirt when libvirtd is restarted. Without these patches, the incorrect element is <seclabel type='dynamic' relabel='yes'/> after applying patch 2/2, the element is <seclabel type='none' relabel='yes'/> which is still wrong and after applying both of these patches, correct element <seclabel type='none'/> is placed into the runtime XML configuration. Jiri Denemark (2): seclabel: Do not output relabel attribute for type 'none' security: Driver 'none' cannot create confined guests src/conf/domain_conf.c | 9 +++++---- src/security/security_manager.c | 20 ++++++++++++++++++++ .../qemuxml2argv-seclabel-none.xml | 2 +- tests/seclabeltest.c | 2 +- 4 files changed, 27 insertions(+), 6 deletions(-) -- 1.7.8.4
Security label type 'none' requires relabel to be set to 'no' so there's no reason to output this extra attribute. Moreover, since relabel is internally stored in a negative from (norelabel), the default value for relabel would be 'yes' in case there is no <seclabel> element in domain configuration. In case VIR_DOMAIN_SECLABEL_DEFAULT turns into VIR_DOMAIN_SECLABEL_NONE, we would incorrectly output relabel='yes' for seclabel type 'none'. --- src/conf/domain_conf.c | 9 +++++---- .../qemuxml2argv-seclabel-none.xml | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6949ece..81836e5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9948,16 +9948,17 @@ virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def) virBufferAsprintf(buf, "<seclabel type='%s'", sectype); - virBufferEscapeString(buf, " model='%s'", def->model); - - virBufferAsprintf(buf, " relabel='%s'", - def->norelabel ? "no" : "yes"); if (def->type == VIR_DOMAIN_SECLABEL_NONE) { virBufferAddLit(buf, "/>\n"); return; } + virBufferEscapeString(buf, " model='%s'", def->model); + + virBufferAsprintf(buf, " relabel='%s'", + def->norelabel ? "no" : "yes"); + if (def->label || def->imagelabel || def->baselabel) { virBufferAddLit(buf, ">\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-none.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-none.xml index 1ef97ce..9def692 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-none.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-none.xml @@ -22,5 +22,5 @@ <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> - <seclabel type='none' relabel='no'/> + <seclabel type='none'/> </domain> -- 1.7.8.4
On 02/07/2012 01:10 PM, Jiri Denemark wrote:
Security label type 'none' requires relabel to be set to 'no' so there's no reason to output this extra attribute. Moreover, since relabel is internally stored in a negative from (norelabel), the default value for relabel would be 'yes' in case there is no <seclabel> element in domain configuration. In case VIR_DOMAIN_SECLABEL_DEFAULT turns into VIR_DOMAIN_SECLABEL_NONE, we would incorrectly output relabel='yes' for seclabel type 'none'. --- src/conf/domain_conf.c | 9 +++++---- .../qemuxml2argv-seclabel-none.xml | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Feb 07, 2012 at 13:31:39 -0700, Eric Blake wrote:
On 02/07/2012 01:10 PM, Jiri Denemark wrote:
Security label type 'none' requires relabel to be set to 'no' so there's no reason to output this extra attribute. Moreover, since relabel is internally stored in a negative from (norelabel), the default value for relabel would be 'yes' in case there is no <seclabel> element in domain configuration. In case VIR_DOMAIN_SECLABEL_DEFAULT turns into VIR_DOMAIN_SECLABEL_NONE, we would incorrectly output relabel='yes' for seclabel type 'none'. --- src/conf/domain_conf.c | 9 +++++---- .../qemuxml2argv-seclabel-none.xml | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-)
ACK.
Pushed, thanks. Jirka
In case the caller specifies that confined guests are required but the security driver turns out to be 'none', we should return an error since this driver clearly cannot meet that requirement. As a result of this error, libvirtd fails to start when the host admin explicitly sets confined guests are required but there is no security driver available. Since security driver 'none' cannot create confined guests, we override default confined setting so that hypervisor drivers do not thing they should create confined guests. --- src/security/security_manager.c | 20 ++++++++++++++++++++ tests/seclabeltest.c | 2 +- 2 files changed, 21 insertions(+), 1 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index d0bafae..0a43458 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -115,6 +115,26 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, if (!drv) return NULL; + /* driver "none" needs some special handling of *Confined bools */ + if (STREQ(drv->name, "none")) { + if (requireConfined) { + virSecurityReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Security driver \"none\" cannot create confined guests")); + return NULL; + } + + if (defaultConfined) { + if (name != NULL) { + VIR_WARN("Configured security driver \"none\" disables default" + " policy to create confined guests"); + } else { + VIR_DEBUG("Auto-probed security driver is \"none\";" + " confined guests will not be created"); + } + defaultConfined = false; + } + } + return virSecurityManagerNewDriver(drv, allowDiskFormatProbing, defaultConfined, diff --git a/tests/seclabeltest.c b/tests/seclabeltest.c index 1898c3e..fca76b9 100644 --- a/tests/seclabeltest.c +++ b/tests/seclabeltest.c @@ -13,7 +13,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) virSecurityManagerPtr mgr; const char *doi, *model; - mgr = virSecurityManagerNew(NULL, false, true, true); + mgr = virSecurityManagerNew(NULL, false, true, false); if (mgr == NULL) { fprintf (stderr, "Failed to start security driver"); exit (-1); -- 1.7.8.4
On 02/07/2012 01:10 PM, Jiri Denemark wrote:
In case the caller specifies that confined guests are required but the security driver turns out to be 'none', we should return an error since this driver clearly cannot meet that requirement. As a result of this error, libvirtd fails to start when the host admin explicitly sets confined guests are required but there is no security driver available.
Since security driver 'none' cannot create confined guests, we override default confined setting so that hypervisor drivers do not thing they
s/thing/think/
should create confined guests. --- src/security/security_manager.c | 20 ++++++++++++++++++++ tests/seclabeltest.c | 2 +- 2 files changed, 21 insertions(+), 1 deletions(-)
ACK that this fixes the issue, but I'm wondering whether we should move the logic that rejects requireConfig out of security_manager.c and into security_nop.c:virSecurityDriverOpenNop(). That is, the special casing is a property of the 'none' security manager. Is it worth a v2 patch that moves the error messages in that manner?
+++ b/tests/seclabeltest.c @@ -13,7 +13,7 @@ main (int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED) virSecurityManagerPtr mgr; const char *doi, *model;
- mgr = virSecurityManagerNew(NULL, false, true, true); + mgr = virSecurityManagerNew(NULL, false, true, false);
And here's a classic example that proves Laine's point that any interface with more than one bool parameter is hard to read (you have to check the implementation), compared to consolidating those into a flags argument. But no need to change the signature for this particular patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Feb 07, 2012 at 13:39:17 -0700, Eric Blake wrote:
On 02/07/2012 01:10 PM, Jiri Denemark wrote:
In case the caller specifies that confined guests are required but the security driver turns out to be 'none', we should return an error since this driver clearly cannot meet that requirement. As a result of this error, libvirtd fails to start when the host admin explicitly sets confined guests are required but there is no security driver available.
Since security driver 'none' cannot create confined guests, we override default confined setting so that hypervisor drivers do not thing they
s/thing/think/
should create confined guests. --- src/security/security_manager.c | 20 ++++++++++++++++++++ tests/seclabeltest.c | 2 +- 2 files changed, 21 insertions(+), 1 deletions(-)
ACK that this fixes the issue, but I'm wondering whether we should move the logic that rejects requireConfig out of security_manager.c and into security_nop.c:virSecurityDriverOpenNop(). That is, the special casing is a property of the 'none' security manager. Is it worth a v2 patch that moves the error messages in that manner?
That was my first thought but both defaultConfined and requireConfined bools are currently a property of security manager (not driver) and drivers have no access inside virSecurityManager. Moreover, virSecurityDriverOpenNop doesn't know whether it was requested explicitly or it was just the only available security driver when there was no explicit configuration. So while it is certainly doable I don't think it's worth all the mess it would require. Jirka
On Tue, Feb 07, 2012 at 13:39:17 -0700, Eric Blake wrote:
On 02/07/2012 01:10 PM, Jiri Denemark wrote:
In case the caller specifies that confined guests are required but the security driver turns out to be 'none', we should return an error since this driver clearly cannot meet that requirement. As a result of this error, libvirtd fails to start when the host admin explicitly sets confined guests are required but there is no security driver available.
Since security driver 'none' cannot create confined guests, we override default confined setting so that hypervisor drivers do not thing they
s/thing/think/
Oops, I mistakenly pushed this without fixing the typo.
should create confined guests. --- src/security/security_manager.c | 20 ++++++++++++++++++++ tests/seclabeltest.c | 2 +- 2 files changed, 21 insertions(+), 1 deletions(-)
ACK that this fixes the issue, but I'm wondering whether we should move the logic that rejects requireConfig out of security_manager.c and into security_nop.c:virSecurityDriverOpenNop(). That is, the special casing is a property of the 'none' security manager. Is it worth a v2 patch that moves the error messages in that manner?
I went ahead and pushed this version (see my other email for reasons). We can refactor the whole thing later if we feel like it's a good idea. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark