2:10 p.m.
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
2:10 p.m.
New subject: [libvirt] [PATCH 1/2] seclabel: Do not output relabel attribute for type 'none'
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
2:31 p.m.
New subject: [libvirt] [PATCH 1/2] seclabel: Do not output relabel attribute for type 'none'
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:06 a.m.
New subject: [libvirt] [PATCH 1/2] seclabel: Do not output relabel attribute for type 'none'
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
2:10 p.m.
New subject: [libvirt] [PATCH 2/2] security: Driver 'none' cannot create confined guests
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
2:39 p.m.
New subject: [libvirt] [PATCH 2/2] security: Driver 'none' cannot create confined guests
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:14 p.m.
New subject: [libvirt] [PATCH 2/2] security: Driver 'none' cannot create confined guests
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
5:29 a.m.
New subject: [libvirt] [PATCH 2/2] security: Driver 'none' cannot create confined guests
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
4671
days inactive
4672
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Jiri Denemark