The RNG for <seclabel> was too strict - if it was present, then it had to have sub-elements, even if those didn't make sense for the given attributes. Also, we didn't have any tests of <seclabel> parsing or XML output. In this patch, I added more parsing tests than output tests (since the output populates and/or reorders fields not present in certain inputs). Making the RNG reliable is a precursor to using <seclabel> variants in more places in the XML in later patches. See also: http://berrange.com/posts/2011/09/29/two-small-improvements-to-svirt-guest-c... * docs/schemas/domaincommon.rng (seclabel): Tighten rules. * tests/qemuxml2argvtest.c (mymain): New tests. * tests/qemuxml2xmltest.c (mymain): Likewise. * tests/qemuxml2argvdata/qemuxml2argv-seclabel-*.*: New files. --- docs/schemas/domaincommon.rng | 88 ++++++++++++++------ .../qemuxml2argv-seclabel-dynamic-baselabel.args | 4 + .../qemuxml2argv-seclabel-dynamic-baselabel.xml | 28 ++++++ .../qemuxml2argv-seclabel-dynamic.args | 4 + .../qemuxml2argv-seclabel-dynamic.xml | 26 ++++++ .../qemuxml2argv-seclabel-static-relabel.args | 4 + .../qemuxml2argv-seclabel-static-relabel.xml | 29 +++++++ .../qemuxml2argv-seclabel-static.args | 4 + .../qemuxml2argv-seclabel-static.xml | 28 ++++++ tests/qemuxml2argvtest.c | 5 + tests/qemuxml2xmltest.c | 3 + 11 files changed, 199 insertions(+), 24 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 553a6f0..dd76f91 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -50,30 +50,70 @@ </define> <define name="seclabel"> <element name="seclabel"> - <attribute name="model"> - <text/> - </attribute> - <attribute name="type"> - <choice> - <value>dynamic</value> - <value>static</value> - </choice> - </attribute> - <attribute name="relabel"> - <choice> - <value>yes</value> - <value>no</value> - </choice> - </attribute> - <element name="label"> - <text/> - </element> - <element name="imagelabel"> - <text/> - </element> - <element name="baselabel"> - <text/> - </element> + <optional> + <attribute name='model'> + <text/> + </attribute> + </optional> + <choice> + <group> + <!-- with dynamic label (default), relabel must be yes, baselabel + is optional, and label and imagelabel are output-only --> + <optional> + <attribute name='type'> + <value>dynamic</value> + </attribute> + </optional> + <optional> + <attribute name='relabel'> + <value>yes</value> + </attribute> + </optional> + <interleave> + <optional> + <element name='label'> + <text/> + </element> + </optional> + <optional> + <element name='imagelabel'> + <text/> + </element> + </optional> + <optional> + <element name='baselabel'> + <text/> + </element> + </optional> + </interleave> + </group> + <group> + <!-- with static label, relabel can be either format (default + no), label is required, imagelabel is output-only, and no + baselabel is present --> + <attribute name='type'> + <value>static</value> + </attribute> + <optional> + <attribute name='relabel'> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <interleave> + <element name='label'> + <text/> + </element> + <optional> + <element name='imagelabel'> + <text/> + </element> + </optional> + </interleave> + </group> + </choice> </element> </define> <define name="hvs"> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.args b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.args new file mode 100644 index 0000000..651793d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -monitor unix:/tmp/test-monitor,\ +server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ +none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.xml new file mode 100644 index 0000000..fea0eb7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-baselabel.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='selinux' relabel='yes'> + <baselabel>system_u:system_r:svirt_custom_t:s0</baselabel> + </seclabel> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.args b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.args new file mode 100644 index 0000000..651793d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -monitor unix:/tmp/test-monitor,\ +server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ +none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.xml new file mode 100644 index 0000000..096c766 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic.xml @@ -0,0 +1,26 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' relabel='yes'/> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.args b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.args new file mode 100644 index 0000000..651793d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -monitor unix:/tmp/test-monitor,\ +server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ +none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.xml new file mode 100644 index 0000000..3b2ad04 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-relabel.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='static' model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + <imagelabel>system_u:system_r:svirt_custom_t:s0:c192,c392</imagelabel> + </seclabel> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.args b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.args new file mode 100644 index 0000000..651793d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.args @@ -0,0 +1,4 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -monitor unix:/tmp/test-monitor,\ +server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial \ +none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.xml new file mode 100644 index 0000000..416bd86 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-static.xml @@ -0,0 +1,28 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='static' model='selinux' relabel='no'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + </seclabel> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e1221eb..18e8941 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -660,6 +660,11 @@ mymain(void) QEMU_CAPS_CHARDEV, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_NO_SHUTDOWN); + DO_TEST("seclabel-dynamic", false, QEMU_CAPS_NAME); + DO_TEST("seclabel-dynamic-baselabel", false, QEMU_CAPS_NAME); + DO_TEST("seclabel-static", false, QEMU_CAPS_NAME); + DO_TEST("seclabel-static-relabel", false, QEMU_CAPS_NAME); + free(driver.stateDir); virCapabilitiesFree(driver.caps); free(map); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 35bfdce..e4b99c4 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -194,6 +194,9 @@ mymain(void) DO_TEST("usb-redir"); DO_TEST("blkdeviotune"); + DO_TEST("seclabel-dynamic-baselabel"); + DO_TEST("seclabel-static"); + /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); DO_TEST_DIFFERENT("channel-virtio-auto"); -- 1.7.7.4

A future patch will parse and output <seclabel> in more than one location in a <domain> xml; make it easier to reuse code. * src/conf/domain_conf.c (virSecurityLabelDefFree): Rename... (virSecurityLabelDefClear): ...and make static. (virSecurityLabelDefParseXML): Alter signature. (virDomainDefParseXML, virDomainDefFree): Adjust callers. (virDomainDefFormatInternal): Split output... (virSecurityLabelDefFormat): ...into new helper. --- src/conf/domain_conf.c | 118 ++++++++++++++++++++++++++--------------------- 1 files changed, 65 insertions(+), 53 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2897b4a..2379c81 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1326,14 +1326,13 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) VIR_FREE(def); } -void virSecurityLabelDefFree(virDomainDefPtr def); - -void virSecurityLabelDefFree(virDomainDefPtr def) +static void +virSecurityLabelDefClear(virSecurityLabelDefPtr def) { - VIR_FREE(def->seclabel.model); - VIR_FREE(def->seclabel.label); - VIR_FREE(def->seclabel.imagelabel); - VIR_FREE(def->seclabel.baselabel); + VIR_FREE(def->model); + VIR_FREE(def->label); + VIR_FREE(def->imagelabel); + VIR_FREE(def->baselabel); } static void @@ -1467,7 +1466,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainMemballoonDefFree(def->memballoon); - virSecurityLabelDefFree(def); + virSecurityLabelDefClear(&def->seclabel); virCPUDefFree(def->cpu); @@ -6212,7 +6211,7 @@ static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, } static int -virSecurityLabelDefParseXML(const virDomainDefPtr def, +virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -6228,9 +6227,9 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security type")); goto error; } - def->seclabel.type = virDomainSeclabelTypeFromString(p); + def->type = virDomainSeclabelTypeFromString(p); VIR_FREE(p); - if (def->seclabel.type < 0) { + if (def->type < 0) { virDomainReportError(VIR_ERR_XML_ERROR, "%s", _("invalid security type")); goto error; @@ -6239,9 +6238,9 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) { if (STREQ(p, "yes")) { - def->seclabel.norelabel = false; + def->norelabel = false; } else if (STREQ(p, "no")) { - def->seclabel.norelabel = true; + def->norelabel = true; } else { virDomainReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), p); @@ -6249,23 +6248,23 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, goto error; } VIR_FREE(p); - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && - def->seclabel.norelabel) { + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && + def->norelabel) { virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dynamic label type must use resource relabeling")); goto error; } } else { - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) - def->seclabel.norelabel = true; + if (def->type == VIR_DOMAIN_SECLABEL_STATIC) + def->norelabel = true; else - def->seclabel.norelabel = false; + def->norelabel = false; } /* Only parse label, if using static labels, or * if the 'live' VM XML is requested */ - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || !(flags & VIR_DOMAIN_XML_INACTIVE)) { p = virXPathStringLimit("string(./seclabel/label[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -6275,11 +6274,11 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, goto error; } - def->seclabel.label = p; + def->label = p; } /* Only parse imagelabel, if requested live XML with relabeling */ - if (!def->seclabel.norelabel && + if (!def->norelabel && !(flags & VIR_DOMAIN_XML_INACTIVE)) { p = virXPathStringLimit("string(./seclabel/imagelabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); @@ -6288,22 +6287,22 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("security imagelabel is missing")); goto error; } - def->seclabel.imagelabel = p; + def->imagelabel = p; } /* Only parse baselabel, for dynamic label */ - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { p = virXPathStringLimit("string(./seclabel/baselabel[1])", VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) - def->seclabel.baselabel = p; + def->baselabel = p; } /* Only parse model, if static labelling, or a base * label is set, or doing active XML */ - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC || - def->seclabel.baselabel || + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || + def->baselabel || !(flags & VIR_DOMAIN_XML_INACTIVE)) { p = virXPathStringLimit("string(./seclabel/@model)", VIR_SECURITY_MODEL_BUFLEN-1, ctxt); @@ -6312,13 +6311,13 @@ virSecurityLabelDefParseXML(const virDomainDefPtr def, "%s", _("missing security model")); goto error; } - def->seclabel.model = p; + def->model = p; } return 0; error: - virSecurityLabelDefFree(def); + virSecurityLabelDefClear(def); return -1; } @@ -7939,7 +7938,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, VIR_FREE(nodes); /* analysis of security label */ - if (virSecurityLabelDefParseXML(def, ctxt, flags) == -1) + if (virSecurityLabelDefParseXML(&def->seclabel, ctxt, flags) == -1) goto error; if ((node = virXPathNode("./cpu[1]", ctxt)) != NULL) { @@ -9739,6 +9738,40 @@ virDomainLifecycleDefFormat(virBufferPtr buf, static int +virSecurityLabelDefFormat(virBufferPtr buf, virSecurityLabelDefPtr def, + unsigned int flags) +{ + const char *sectype = virDomainSeclabelTypeToString(def->type); + int ret = -1; + + if (!sectype) + goto cleanup; + + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && + !def->baselabel && + (flags & VIR_DOMAIN_XML_INACTIVE)) { + /* This is the default for inactive xml, so nothing to output. */ + } else { + virBufferAsprintf(buf, "<seclabel type='%s' model='%s' relabel='%s'>\n", + sectype, def->model, + def->norelabel ? "no" : "yes"); + virBufferEscapeString(buf, " <label>%s</label>\n", + def->label); + if (!def->norelabel) + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", + def->imagelabel); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", + def->baselabel); + virBufferAddLit(buf, "</seclabel>\n"); + } + ret = 0; +cleanup: + return ret; +} + + +static int virDomainLeaseDefFormat(virBufferPtr buf, virDomainLeaseDefPtr def) { @@ -11679,31 +11712,10 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, " </devices>\n"); if (def->seclabel.model) { - const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); - if (!sectype) + virBufferAdjustIndent(buf, 2); + if (virSecurityLabelDefFormat(buf, &def->seclabel, flags) < 0) goto cleanup; - - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC && - !def->seclabel.baselabel && - (flags & VIR_DOMAIN_XML_INACTIVE)) { - /* This is the default for inactive xml, so nothing to output. */ - } else { - virBufferAsprintf(buf, " <seclabel type='%s' model='%s' " - "relabel='%s'>\n", - sectype, def->seclabel.model, - def->seclabel.norelabel ? "no" : "yes"); - virBufferEscapeString(buf, " <label>%s</label>\n", - def->seclabel.label); - if (!def->seclabel.norelabel) - virBufferEscapeString(buf, - " <imagelabel>%s</imagelabel>\n", - def->seclabel.imagelabel); - if (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) - virBufferEscapeString(buf, - " <baselabel>%s</baselabel>\n", - def->seclabel.baselabel); - virBufferAddLit(buf, " </seclabel>\n"); - } + virBufferAdjustIndent(buf, -2); } if (def->namespaceData && def->ns.format) { -- 1.7.7.4

Pure code motion; no semantic change. * src/conf/domain_conf.h (virDomainSeclabelType) (virSecurityLabelDefPtr): Declare earlier. * src/conf/domain_conf.c (virSecurityLabelDefClear) (virSecurityLabelDefParseXML): Move earlier. (virDomainDefParseXML): Move seclabel parsing earlier. --- src/conf/domain_conf.c | 250 ++++++++++++++++++++++++------------------------ src/conf/domain_conf.h | 38 ++++---- 2 files changed, 145 insertions(+), 143 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2379c81..41db117 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -788,6 +788,15 @@ virDomainGraphicsListenDefClear(virDomainGraphicsListenDefPtr def) return; } +static void +virSecurityLabelDefClear(virSecurityLabelDefPtr def) +{ + VIR_FREE(def->model); + VIR_FREE(def->label); + VIR_FREE(def->imagelabel); + VIR_FREE(def->baselabel); +} + void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) { int ii; @@ -1327,15 +1336,6 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) } static void -virSecurityLabelDefClear(virSecurityLabelDefPtr def) -{ - VIR_FREE(def->model); - VIR_FREE(def->label); - VIR_FREE(def->imagelabel); - VIR_FREE(def->baselabel); -} - -static void virDomainClockDefClear(virDomainClockDefPtr def) { if (def->offset == VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE) @@ -2517,6 +2517,117 @@ virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def) return 0; } +static int +virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, + xmlXPathContextPtr ctxt, + unsigned int flags) +{ + char *p; + + if (virXPathNode("./seclabel", ctxt) == NULL) + return 0; + + p = virXPathStringLimit("string(./seclabel/@type)", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing security type")); + goto error; + } + def->type = virDomainSeclabelTypeFromString(p); + VIR_FREE(p); + if (def->type < 0) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("invalid security type")); + goto error; + } + p = virXPathStringLimit("string(./seclabel/@relabel)", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p != NULL) { + if (STREQ(p, "yes")) { + def->norelabel = false; + } else if (STREQ(p, "no")) { + def->norelabel = true; + } else { + virDomainReportError(VIR_ERR_XML_ERROR, + _("invalid security relabel value %s"), p); + VIR_FREE(p); + goto error; + } + VIR_FREE(p); + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && + def->norelabel) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("dynamic label type must use resource relabeling")); + goto error; + } + } else { + if (def->type == VIR_DOMAIN_SECLABEL_STATIC) + def->norelabel = true; + else + def->norelabel = false; + } + + /* Only parse label, if using static labels, or + * if the 'live' VM XML is requested + */ + if (def->type == VIR_DOMAIN_SECLABEL_STATIC || + !(flags & VIR_DOMAIN_XML_INACTIVE)) { + p = virXPathStringLimit("string(./seclabel/label[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("security label is missing")); + goto error; + } + + def->label = p; + } + + /* Only parse imagelabel, if requested live XML with relabeling */ + if (!def->norelabel && + !(flags & VIR_DOMAIN_XML_INACTIVE)) { + p = virXPathStringLimit("string(./seclabel/imagelabel[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("security imagelabel is missing")); + goto error; + } + def->imagelabel = p; + } + + /* Only parse baselabel, for dynamic label */ + if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { + p = virXPathStringLimit("string(./seclabel/baselabel[1])", + VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + if (p != NULL) + 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)) { + p = virXPathStringLimit("string(./seclabel/@model)", + VIR_SECURITY_MODEL_BUFLEN-1, ctxt); + if (p == NULL) { + virDomainReportError(VIR_ERR_XML_ERROR, + "%s", _("missing security model")); + goto error; + } + def->model = p; + } + + return 0; + +error: + virSecurityLabelDefClear(def); + return -1; +} + /* Parse the XML definition for a lease */ static virDomainLeaseDefPtr @@ -6210,117 +6321,6 @@ static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, return 0; } -static int -virSecurityLabelDefParseXML(virSecurityLabelDefPtr def, - xmlXPathContextPtr ctxt, - unsigned int flags) -{ - char *p; - - if (virXPathNode("./seclabel", ctxt) == NULL) - return 0; - - p = virXPathStringLimit("string(./seclabel/@type)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security type")); - goto error; - } - def->type = virDomainSeclabelTypeFromString(p); - VIR_FREE(p); - if (def->type < 0) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("invalid security type")); - goto error; - } - p = virXPathStringLimit("string(./seclabel/@relabel)", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p != NULL) { - if (STREQ(p, "yes")) { - def->norelabel = false; - } else if (STREQ(p, "no")) { - def->norelabel = true; - } else { - virDomainReportError(VIR_ERR_XML_ERROR, - _("invalid security relabel value %s"), p); - VIR_FREE(p); - goto error; - } - VIR_FREE(p); - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && - def->norelabel) { - virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", _("dynamic label type must use resource relabeling")); - goto error; - } - } else { - if (def->type == VIR_DOMAIN_SECLABEL_STATIC) - def->norelabel = true; - else - def->norelabel = false; - } - - /* Only parse label, if using static labels, or - * if the 'live' VM XML is requested - */ - if (def->type == VIR_DOMAIN_SECLABEL_STATIC || - !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit("string(./seclabel/label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("security label is missing")); - goto error; - } - - def->label = p; - } - - /* Only parse imagelabel, if requested live XML with relabeling */ - if (!def->norelabel && - !(flags & VIR_DOMAIN_XML_INACTIVE)) { - p = virXPathStringLimit("string(./seclabel/imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("security imagelabel is missing")); - goto error; - } - def->imagelabel = p; - } - - /* Only parse baselabel, for dynamic label */ - if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - p = virXPathStringLimit("string(./seclabel/baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (p != NULL) - 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)) { - p = virXPathStringLimit("string(./seclabel/@model)", - VIR_SECURITY_MODEL_BUFLEN-1, ctxt); - if (p == NULL) { - virDomainReportError(VIR_ERR_XML_ERROR, - "%s", _("missing security model")); - goto error; - } - def->model = p; - } - - return 0; - -error: - virSecurityLabelDefClear(def); - return -1; -} - virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, const virDomainDefPtr def, const char *xmlStr, @@ -7030,6 +7030,11 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, /* Extract documentation if present */ def->description = virXPathString("string(./description[1])", ctxt); + /* analysis of security label, done early even though we format it + * late, so devices can refer to this for defaults */ + if (virSecurityLabelDefParseXML(&def->seclabel, ctxt, flags) == -1) + goto error; + /* Extract domain memory */ if (virXPathULong("string(./memory[1])", ctxt, &def->mem.max_balloon) < 0) { @@ -7937,10 +7942,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } VIR_FREE(nodes); - /* analysis of security label */ - if (virSecurityLabelDefParseXML(&def->seclabel, ctxt, flags) == -1) - goto error; - + /* analysis of cpu handling */ if ((node = virXPathNode("./cpu[1]", ctxt)) != NULL) { xmlNodePtr oldnode = ctxt->node; ctxt->node = node; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1f6e442..7c5946f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -161,6 +161,25 @@ struct _virDomainDeviceInfo { } master; }; +enum virDomainSeclabelType { + VIR_DOMAIN_SECLABEL_DYNAMIC, + VIR_DOMAIN_SECLABEL_STATIC, + + VIR_DOMAIN_SECLABEL_LAST, +}; + +/* Security configuration for domain */ +typedef struct _virSecurityLabelDef virSecurityLabelDef; +typedef virSecurityLabelDef *virSecurityLabelDefPtr; +struct _virSecurityLabelDef { + char *model; /* name of security model */ + char *label; /* security label string */ + char *imagelabel; /* security image label string */ + char *baselabel; /* base name of label string */ + int type; /* virDomainSeclabelType */ + bool norelabel; +}; + typedef struct _virDomainHostdevOrigStates virDomainHostdevOrigStates; typedef virDomainHostdevOrigStates *virDomainHostdevOrigStatesPtr; struct _virDomainHostdevOrigStates { @@ -1238,25 +1257,6 @@ struct _virDomainOSDef { virDomainBIOSDef bios; }; -enum virDomainSeclabelType { - VIR_DOMAIN_SECLABEL_DYNAMIC, - VIR_DOMAIN_SECLABEL_STATIC, - - VIR_DOMAIN_SECLABEL_LAST, -}; - -/* Security configuration for domain */ -typedef struct _virSecurityLabelDef virSecurityLabelDef; -typedef virSecurityLabelDef *virSecurityLabelDefPtr; -struct _virSecurityLabelDef { - char *model; /* name of security model */ - char *label; /* security label string */ - char *imagelabel; /* security image label string */ - char *baselabel; /* base name of label string */ - int type; /* virDomainSeclabelType */ - bool norelabel; -}; - enum virDomainTimerNameType { VIR_DOMAIN_TIMER_NAME_PLATFORM = 0, VIR_DOMAIN_TIMER_NAME_PIT, -- 1.7.7.4

When doing security relabeling, there are cases where a per-file override might be appropriate. For example, with a static label and relabeling, it might be appropriate to skip relabeling on a particular disk, where the backing file lives on NFS that lacks the ability to track labeling. Or with dynamic labeling, it might be appropriate to use a custom (non-dynamic) label for a disk specifically intended to be shared across domains. The new XML resembles the top-level <seclabel>, but with fewer options (basically relabel='no', or <label>text</label>): <domain ...> ... <devices> <disk type='file' device='disk'> <source file='/path/to/image1'> <seclabel relabel='no'/> <!-- override for just this disk --> </source> ... </disk> <disk type='file' device='disk'> <source file='/path/to/image1'> <seclabel relabel='yes'> <!-- override for just this disk --> <label>system_u:object_r:shared_content_t:s0</label> </seclabel> </source> ... </disk> ... </devices> <seclabel type='dynamic' model='selinux'> <baselabel>text</baselabel> <!-- used for all devices without override --> </seclabel> </domain> This patch only introduces the XML and documentation; future patches will actually parse and make use of it. The intent is that we can further extend things as needed, adding a per-device <seclabel> in more places (such as the source of a console device), and possibly allowing a <baselabel> instead of <label> for labeling where we want to reuse the cNNN,cNNN pair of a dynamically labeled domain but a different base label. First suggested by Daniel P. Berrange here: https://www.redhat.com/archives/libvir-list/2011-December/msg00258.html * docs/schemas/domaincommon.rng (devSeclabel): New define. (disk): Use it. * docs/formatdomain.html.in (elementsDisks, seclabel): Document the new XML. * tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml: New test, to validate RNG. --- docs/formatdomain.html.in | 29 ++++++++++++-- docs/schemas/domaincommon.rng | 29 +++++++++++++- .../qemuxml2argv-seclabel-dynamic-override.xml | 40 ++++++++++++++++++++ 3 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 06181b1..d468299 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -947,7 +947,9 @@ <devices> <disk type='file' snapshot='external'> <driver name="tap" type="aio" cache="default"/> - <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'/> + <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> + <seclabel relabel='no'/> + </source> <target dev='hda' bus='ide'/> <iotune> <total_bytes_sec>10000000</total_bytes_sec> @@ -1023,7 +1025,11 @@ path to the file holding the disk. If the disk <code>type</code> is "block", then the <code>dev</code> attribute specifies the path to the host device to serve as - the disk. If the disk <code>type</code> is "dir", then the + the disk. With both "file" and "block", an optional + sub-element <code>seclabel</code>, <a href="#seclabel">described + below</a> (and <span class="since">since 0.9.9</span>), can be + used to override the domain security labeling policy for just + that source file. If the disk <code>type</code> is "dir", then the <code>dir</code> attribute specifies the fully-qualified path to the directory to use as the disk. If the disk <code>type</code> is "network", then the <code>protocol</code> attribute specifies @@ -1031,7 +1037,7 @@ are "nbd", "rbd", and "sheepdog". If the <code>protocol</code> attribute is "rbd" or "sheepdog", an additional attribute <code>name</code> is mandatory to specify which - image to be used. When the disk <code>type</code> is + image will be used. When the disk <code>type</code> is "network", the <code>source</code> may have zero or more <code>host</code> sub-elements used to specify the hosts to connect. @@ -3372,11 +3378,11 @@ qemu-kvm -net nic,model=? /dev/null With static label assignment, by default, the administrator or application must ensure labels are set correctly on any resources, however, automatic relabeling can be enabled - if desired + if desired. </p> <p> - Valid input XML configurations for the security label + Valid input XML configurations for the top-level security label are: </p> @@ -3435,6 +3441,19 @@ qemu-kvm -net nic,model=? /dev/null </dd> </dl> + <p>When relabeling is in effect, it is also possible to fine-tune + the labeling done for specific source file names, by either + disabling the labeling (useful if the file lives on NFS or other + file system that lacks security labeling) or requesting an + alternate label (useful when a management application creates a + special label to allow sharing of some, but not all, resources + between domains), <span class="since">since 0.9.9</span>. When + a <code>seclabel</code> element is attached to a specific path + rather than the top-level domain assignment, only the + attribute <code>relabel</code> or the + sub-element <code>label</code> are supported. + </p> + <h2><a name="examples">Example configs</a></h2> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index dd76f91..7a8f7f4 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -116,6 +116,27 @@ </choice> </element> </define> + <define name="devSeclabel"> + <element name="seclabel"> + <!-- A per-device seclabel override is more limited, either + relabel=no or a <label> must be present. --> + <choice> + <attribute name='relabel'> + <value>no</value> + </attribute> + <group> + <optional> + <attribute name='relabel'> + <value>yes</value> + </attribute> + </optional> + <element name='label'> + <text/> + </element> + </group> + </choice> + </element> + </define> <define name="hvs"> <attribute name="type"> <choice> @@ -795,7 +816,9 @@ <optional> <ref name="startupPolicy"/> </optional> - <empty/> + <optional> + <ref name='devSeclabel'/> + </optional> </element> </optional> <ref name="diskspec"/> @@ -811,7 +834,9 @@ <attribute name="dev"> <ref name="absFilePath"/> </attribute> - <empty/> + <optional> + <ref name='devSeclabel'/> + </optional> </element> </optional> <ref name="diskspec"/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml new file mode 100644 index 0000000..19b1cbb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'> + <seclabel relabel='no'/> + </source> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'> + <seclabel relabel='yes'> + <label>system_u:system_r:public_content_t:s0</label> + </seclabel> + </source> + <target dev='hdb' bus='ide'/> + <readonly/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='dynamic' model='selinux' relabel='yes'> + <baselabel>system_u:system_r:svirt_custom_t:s0</baselabel> + </seclabel> +</domain> -- 1.7.7.4

This wires up the XML changes in the previous patch to let SELinux labeling honor user overrides, as well as affecting the live XML configuration in one case where the user didn't specify anything in the offline XML. I noticed that the logs contained messages like this: 2011-12-05 23:32:40.382+0000: 26569: warning : SELinuxRestoreSecurityFileLabel:533 : cannot lookup default selinux label for /nfs/libvirt/images/dom.img for all my domain images living on NFS. But if we would just remember that on domain creation that we were unable to set a SELinux label (due to NFSv3 lacking labels, or NFSv4 not being configured to expose attributes), then we could avoid wasting the time trying to clear the label on domain shutdown. This in turn is one less point of NFS failure, especially since there have been documented cases of virDomainDestroy hanging during an attempted operation on a failed NFS connection. * src/security/security_selinux.c (SELinuxSetFilecon): Move guts... (SELinuxSetFileconHelper): ...to new function. (SELinuxSetFileconOptional): New function. (SELinuxSetSecurityFileLabel): Honor override label, and remember if labeling failed. (SELinuxRestoreSecurityImageLabelInt): Skip relabeling based on override. --- Tested on a system with NFS, and /etc/libvirt/qemu.conf having dynamic_ownership=0; without this patch, killing NFS via 'iptables -I INPUT -s $server -j DROP' while a guest was running, then trying 'virsh destroy guest', would hang, and lock out attempts to use virsh from other terminals. After this patch, 'virsh destroy guest' succeeded without hanging. Also tested that custom labels are honored. src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++--------- 1 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 6ef61c7..cdc28ad 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -394,8 +394,11 @@ SELinuxGetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +/* Attempt to change the label of PATH to TCON. If OPTIONAL is true, + * return 1 if labelling was not possible. Otherwise, require a label + * change, and return 0 for success, -1 for failure. */ static int -SELinuxSetFilecon(const char *path, char *tcon) +SELinuxSetFileconHelper(const char *path, char *tcon, bool optional) { security_context_t econ; @@ -408,7 +411,7 @@ SELinuxSetFilecon(const char *path, char *tcon) if (STREQ(tcon, econ)) { freecon(econ); /* It's alright, there's nothing to change anyway. */ - return 0; + return optional ? 1 : 0; } freecon(econ); } @@ -440,12 +443,26 @@ SELinuxSetFilecon(const char *path, char *tcon) VIR_INFO("Setting security context '%s' on '%s' not supported", tcon, path); } + if (optional) + return 1; } } return 0; } static int +SELinuxSetFileconOptional(const char *path, char *tcon) +{ + return SELinuxSetFileconHelper(path, tcon, true); +} + +static int +SELinuxSetFilecon(const char *path, char *tcon) +{ + return SELinuxSetFileconHelper(path, tcon, false); +} + +static int SELinuxFSetFilecon(int fd, char *tcon) { security_context_t econ; @@ -549,7 +566,7 @@ SELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - if (secdef->norelabel) + if (secdef->norelabel || (disk->seclabel && disk->seclabel->norelabel)) return 0; /* Don't restore labels on readoly/shared disks, because @@ -604,23 +621,35 @@ SELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, const virSecurityLabelDefPtr secdef = opaque; int ret; - if (depth == 0) { + if (disk->seclabel && disk->seclabel->norelabel) + return 0; + + if (disk->seclabel && !disk->seclabel->norelabel && + disk->seclabel->label) { + ret = SELinuxSetFilecon(path, disk->seclabel->label); + } else if (depth == 0) { if (disk->shared) { - ret = SELinuxSetFilecon(path, default_image_context); + ret = SELinuxSetFileconOptional(path, default_image_context); } else if (disk->readonly) { - ret = SELinuxSetFilecon(path, default_content_context); + ret = SELinuxSetFileconOptional(path, default_content_context); } else if (secdef->imagelabel) { - ret = SELinuxSetFilecon(path, secdef->imagelabel); + ret = SELinuxSetFileconOptional(path, secdef->imagelabel); } else { ret = 0; } } else { - ret = SELinuxSetFilecon(path, default_content_context); + ret = SELinuxSetFileconOptional(path, default_content_context); + } + if (ret == 1 && !disk->seclabel) { + /* If we failed to set a label, but virt_use_nfs let us + * proceed anyway, then we don't need to relabel later. */ + if (VIR_ALLOC(disk->seclabel) < 0) { + virReportOOMError(); + return -1; + } + disk->seclabel->norelabel = true; + ret = 0; } - if (ret < 0 && - virStorageFileIsSharedFSType(path, - VIR_STORAGE_FILE_SHFS_NFS) == 1) - ret = 0; return ret; } -- 1.7.7.4