[libvirt] [PATCH 0/3] Couple of seclabels improvements
Yes, the first two patches basically revert 693eac388f1759d. Only the last one fixes a real problem. Michal Privoznik (3): virSecurityLabelDef: substitute 'norelabel' with 'relabel' virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel' conf: Disallow <seclabel type='none' relabel='yes'/> src/conf/domain_conf.c | 40 +++++++++++++++++++++++----------------- src/security/security_apparmor.c | 10 +++++----- src/security/security_dac.c | 22 +++++++++++----------- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 32 ++++++++++++++------------------ src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 4 ++-- 7 files changed, 57 insertions(+), 55 deletions(-) -- 1.8.5.5
This negation in names of boolean variables is driving me insane. The code is much more readable if we drop the 'no-' prefix. Well, at least for me. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++---------- src/security/security_apparmor.c | 10 +++++----- src/security/security_dac.c | 14 +++++++------- src/security/security_manager.c | 2 +- src/security/security_selinux.c | 24 ++++++++++-------------- src/util/virseclabel.h | 2 +- 6 files changed, 34 insertions(+), 38 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b80e7cf..f6743e5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4576,9 +4576,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); if (p != NULL) { if (STREQ(p, "yes")) { - def->norelabel = false; + def->relabel = true; } else if (STREQ(p, "no")) { - def->norelabel = true; + def->relabel = false; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), p); @@ -4587,13 +4587,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } VIR_FREE(p); if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC && - def->norelabel) { + !def->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dynamic label type must use resource relabeling")); goto error; } if (def->type == VIR_DOMAIN_SECLABEL_NONE && - !def->norelabel) { + def->relabel) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("resource relabeling is not compatible with 'none' label type")); goto error; @@ -4601,9 +4601,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } else { if (def->type == VIR_DOMAIN_SECLABEL_STATIC || def->type == VIR_DOMAIN_SECLABEL_NONE) - def->norelabel = true; + def->relabel = false; else - def->norelabel = false; + def->relabel = true; } /* Always parse model */ @@ -4635,7 +4635,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, } /* Only parse imagelabel, if requested live XML with relabeling */ - if (!def->norelabel && + if (def->relabel && (!(flags & VIR_DOMAIN_XML_INACTIVE) && def->type != VIR_DOMAIN_SECLABEL_NONE)) { p = virXPathStringLimit("string(./imagelabel[1])", @@ -4793,7 +4793,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, } /* Can't use overrides if top-level doesn't allow relabeling. */ - if (vmDef && vmDef->norelabel) { + if (vmDef && !vmDef->relabel) { virReportError(VIR_ERR_XML_ERROR, "%s", _("label overrides require relabeling to be " "enabled at the domain level")); @@ -14708,14 +14708,14 @@ virSecurityLabelDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, " relabel='%s'", - def->norelabel ? "no" : "yes"); + def->relabel ? "yes" : "no"); if (def->label || def->imagelabel || def->baselabel) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); virBufferEscapeString(buf, "<label>%s</label>\n", def->label); - if (!def->norelabel) + if (def->relabel) virBufferEscapeString(buf, "<imagelabel>%s</imagelabel>\n", def->imagelabel); if (def->type == VIR_DOMAIN_SECLABEL_DYNAMIC) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 1e2a38b..9603c78 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -281,7 +281,7 @@ reload_profile(virSecurityManagerPtr mgr, if (!secdef) return rc; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if ((profile_name = get_profile_name(def)) == NULL) @@ -481,7 +481,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; /* Reload the profile if stdin_path is specified. Note that @@ -718,7 +718,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, if (!(secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME))) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if (secdef->imagelabel) { @@ -805,7 +805,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) @@ -904,7 +904,7 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, if (!secdef) return -1; - if (secdef->norelabel) + if (!secdef->relabel) return 0; return reload_profile(mgr, def, NULL, false); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 26cd615..665cbc9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -307,7 +307,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (secdef && secdef->norelabel) + if (secdef && !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -369,7 +369,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (secdef && secdef->norelabel) + if (secdef && !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -477,7 +477,7 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr, cbdata.manager = mgr; cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (cbdata.secdef && cbdata.secdef->norelabel) + if (cbdata.secdef && !cbdata.secdef->relabel) return 0; switch ((virDomainHostdevSubsysType) dev->source.subsys.type) { @@ -601,7 +601,7 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) @@ -881,7 +881,7 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; VIR_DEBUG("Restoring security label on %s migrated=%d", @@ -955,7 +955,7 @@ virSecurityDACSetSecurityAllLabel(virSecurityManagerPtr mgr, secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - if (!priv->dynamicOwnership || (secdef && secdef->norelabel)) + if (!priv->dynamicOwnership || (secdef && !secdef->relabel)) return 0; for (i = 0; i < def->ndisks; i++) { @@ -1157,7 +1157,7 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr, return rc; } - if (!seclabel->norelabel && !seclabel->imagelabel && + if (seclabel->relabel && !seclabel->imagelabel && VIR_STRDUP(seclabel->imagelabel, seclabel->label) < 0) { VIR_FREE(seclabel->label); return rc; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 16bec5c..8a45e04 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -616,7 +616,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; } else { seclabel->type = VIR_DOMAIN_SECLABEL_NONE; - seclabel->norelabel = true; + seclabel->relabel = false; } } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e06c003..8d4a9aa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1130,7 +1130,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (seclabel->norelabel || (disk_seclabel && disk_seclabel->norelabel)) + if (!seclabel->relabel || (disk_seclabel && disk_seclabel->norelabel)) return 0; /* If labelskip is true and there are no backing files, then we @@ -1202,7 +1202,7 @@ virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr, return 0; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; disk_seclabel = virStorageSourceGetSecurityLabelDef(src, @@ -1456,7 +1456,7 @@ virSecuritySELinuxSetSecurityHostdevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; switch (dev->mode) { @@ -1641,7 +1641,7 @@ virSecuritySELinuxRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; switch (dev->mode) { @@ -1670,7 +1670,7 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || seclabel->norelabel) + if (!seclabel || !seclabel->relabel) return 0; if (dev) @@ -1741,7 +1741,7 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, int ret = -1; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!seclabel || seclabel->norelabel) + if (!seclabel || !seclabel->relabel) return 0; if (dev) @@ -1866,10 +1866,8 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, VIR_DEBUG("Restoring security label on %s", def->name); secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return 0; - if (secdef->norelabel || data->skipAllLabel) + if (!secdef || !secdef->relabel || data->skipAllLabel) return 0; if (def->tpm) { @@ -1956,7 +1954,7 @@ virSecuritySELinuxSetSavedStateLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; return virSecuritySELinuxSetFilecon(savefile, secdef->imagelabel); @@ -1971,7 +1969,7 @@ virSecuritySELinuxRestoreSavedStateLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (!secdef || secdef->norelabel) + if (!secdef || !secdef->relabel) return 0; return virSecuritySELinuxRestoreSecurityFileLabel(mgr, savefile); @@ -2245,10 +2243,8 @@ virSecuritySELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, virSecurityLabelDefPtr secdef; secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); - if (secdef == NULL) - return 0; - if (secdef->norelabel || data->skipAllLabel) + if (!secdef || !secdef->relabel || data->skipAllLabel) return 0; for (i = 0; i < def->ndisks; i++) { diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index b90d212..8d671fd 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -40,7 +40,7 @@ struct _virSecurityLabelDef { char *imagelabel; /* security image label string */ char *baselabel; /* base name of label string */ int type; /* virDomainSeclabelType */ - bool norelabel; + bool relabel; /* should try labeling attempts? */ bool implicit; /* true if seclabel is auto-added */ }; -- 1.8.5.5
Similarly to the previous commit, boolean variables should not start with 'no-' prefix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 12 ++++++------ src/security/security_dac.c | 8 ++++---- src/security/security_selinux.c | 10 +++++----- src/util/virseclabel.c | 2 +- src/util/virseclabel.h | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f6743e5..f75c0cb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4803,9 +4803,9 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, relabel = virXMLPropString(list[i], "relabel"); if (relabel != NULL) { if (STREQ(relabel, "yes")) { - seclabels[i]->norelabel = false; + seclabels[i]->relabel = true; } else if (STREQ(relabel, "no")) { - seclabels[i]->norelabel = true; + seclabels[i]->relabel = false; } else { virReportError(VIR_ERR_XML_ERROR, _("invalid security relabel value %s"), @@ -4815,7 +4815,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, } VIR_FREE(relabel); } else { - seclabels[i]->norelabel = false; + seclabels[i]->relabel = true; } /* labelskip is only parsed on live images */ @@ -4830,7 +4830,7 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, VIR_SECURITY_LABEL_BUFLEN-1, ctxt); seclabels[i]->label = label; - if (label && seclabels[i]->norelabel) { + if (label && !seclabels[i]->relabel) { virReportError(VIR_ERR_XML_ERROR, _("Cannot specify a label if relabelling is " "turned off. model=%s"), @@ -14736,7 +14736,7 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf, { /* For offline output, skip elements that allow labels but have no * label specified (possible if labelskip was ignored on input). */ - if ((flags & VIR_DOMAIN_XML_INACTIVE) && !def->label && !def->norelabel) + if ((flags & VIR_DOMAIN_XML_INACTIVE) && !def->label && def->relabel) return; virBufferAddLit(buf, "<seclabel"); @@ -14747,7 +14747,7 @@ virSecurityDeviceLabelDefFormat(virBufferPtr buf, if (def->labelskip) virBufferAddLit(buf, " labelskip='yes'"); else - virBufferAsprintf(buf, " relabel='%s'", def->norelabel ? "no" : "yes"); + virBufferAsprintf(buf, " relabel='%s'", def->relabel ? "yes" : "no"); if (def->label) { virBufferAddLit(buf, ">\n"); diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 665cbc9..4d2a9d6 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -312,7 +312,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; if (disk_seclabel && disk_seclabel->label) { @@ -374,7 +374,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; /* If we have a shared FS and are doing migration, we must not change @@ -703,7 +703,7 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_DAC_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; if (chr_seclabel && chr_seclabel->label) { @@ -772,7 +772,7 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_DAC_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; switch ((virDomainChrType) dev_source->type) { diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 8d4a9aa..a0e89b7 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1130,7 +1130,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (!seclabel->relabel || (disk_seclabel && disk_seclabel->norelabel)) + if (!seclabel->relabel || (disk_seclabel && !disk_seclabel->relabel)) return 0; /* If labelskip is true and there are no backing files, then we @@ -1208,10 +1208,10 @@ virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr, disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); - if (disk_seclabel && disk_seclabel->norelabel) + if (disk_seclabel && !disk_seclabel->relabel) return 0; - if (disk_seclabel && !disk_seclabel->norelabel && disk_seclabel->label) { + if (disk_seclabel && disk_seclabel->relabel && disk_seclabel->label) { ret = virSecuritySELinuxSetFilecon(src->path, disk_seclabel->label); } else if (first) { if (src->shared) { @@ -1677,7 +1677,7 @@ virSecuritySELinuxSetSecurityChardevLabel(virDomainDefPtr def, chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; if (chr_seclabel) @@ -1747,7 +1747,7 @@ virSecuritySELinuxRestoreSecurityChardevLabel(virSecurityManagerPtr mgr, if (dev) chr_seclabel = virDomainChrDefGetSecurityLabelDef(dev, SECURITY_SELINUX_NAME); - if (chr_seclabel && chr_seclabel->norelabel) + if (chr_seclabel && !chr_seclabel->relabel) return 0; switch (dev_source->type) { diff --git a/src/util/virseclabel.c b/src/util/virseclabel.c index 8f07de3..88c1fc3 100644 --- a/src/util/virseclabel.c +++ b/src/util/virseclabel.c @@ -90,7 +90,7 @@ virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) if (VIR_ALLOC(ret) < 0) return NULL; - ret->norelabel = src->norelabel; + ret->relabel = src->relabel; ret->labelskip = src->labelskip; if (VIR_STRDUP(ret->model, src->model) < 0 || diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 8d671fd..7eebd60 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -51,7 +51,7 @@ typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; struct _virSecurityDeviceLabelDef { char *model; char *label; /* image label string */ - bool norelabel; /* true to skip label attempts */ + bool relabel; /* should try labeling attempts? */ bool labelskip; /* live-only; true if skipping failed label attempt */ }; -- 1.8.5.5
https://bugzilla.redhat.com/show_bug.cgi?id=1113860 The combination of type='none' and relabel='yes' makes no sense as 'none' type basically means relabel='no'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f75c0cb..4215565 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4614,8 +4614,14 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* For the model 'none' none of the following labels is going to be * present. Hence, return now. */ - if (STREQ_NULLABLE(def->model, "none")) + if (STREQ_NULLABLE(def->model, "none")) { + if (def->relabel) { + virReportError(VIR_ERR_XML_DETAIL, "%s", + _("model 'none' does not allow relabeling")); + goto error; + } return def; + } /* Only parse label, if using static labels, or * if the 'live' VM XML is requested -- 1.8.5.5
participants (1)
-
Michal Privoznik