9:04 a.m.
diff to v1:
- rework the 3rd patch
- introduce one more bugfix
Michal Privoznik (4):
virSecurityLabelDef: substitute 'norelabel' with 'relabel'
virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel'
conf: Always format seclabel's model
conf: Don't allow multiple seclabels for same model
src/conf/domain_conf.c | 67 ++++++++++++----------
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 +-
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 +++++++++
.../qemuxml2argv-seclabel-multiple.xml | 40 +++++++++++++
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmltest.c | 1 +
11 files changed, 142 insertions(+), 67 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
--
1.8.5.5
9:04 a.m.
New subject: [libvirt] [PATCH v2 1/4] virSecurityLabelDef: substitute 'norelabel' with 'relabel'
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(a)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 fa76eb4..8384b5d 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
10:02 a.m.
New subject: [libvirt] [PATCH v2 1/4] virSecurityLabelDef: substitute 'norelabel' with 'relabel'
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
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(a)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/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;
seclabel->relabel = true;
is needed here now, since the code was relying on norelabel being false by
default to enable relabeling (and I agree with your comment about readability
now :))
The new default also affects the other caller of virSecurityLabelDefNew:
In qemuProcessAttach where we generate a new label:
if (seclabeldef == NULL) {
if (!(seclabeldef = virSecurityLabelDefNew(model)))
goto error;
seclabelgen = true;
}
I'd set relabel to true here, to make this commit a no-op.
} else {
seclabel->type = VIR_DOMAIN_SECLABEL_NONE;
- seclabel->norelabel = true;
+ seclabel->relabel = false;
}
}
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? */
I can't parse that. How about "whether we relabel files", or just leaving
it
without a comment?
ACK with the two callers of virSecurityLabelDefNew fixed.
Jan
11:42 a.m.
New subject: [libvirt] [PATCH v2 1/4] virSecurityLabelDef: substitute 'norelabel' with 'relabel'
On 07/10/2014 09:02 AM, Ján Tomko wrote:
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
> 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(a)redhat.com>
> ---
> +++ b/src/security/security_manager.c
> @@ -616,7 +616,7 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
> seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
seclabel->relabel = true;
is needed here now, since the code was relying on norelabel being false by
default to enable relabeling (and I agree with your comment about readability
now :))
The new default also affects the other caller of virSecurityLabelDefNew:
In qemuProcessAttach where we generate a new label:
if (seclabeldef == NULL) {
if (!(seclabeldef = virSecurityLabelDefNew(model)))
goto error;
seclabelgen = true;
}
I'd set relabel to true here, to make this commit a no-op.
Actually, that would argue that virSecurityLabelDefNew() (and any other
allocation function) should set the relabel=true as PART of the
allocation. Any time that we rely on a default state, if the state is
all 0 then VIR_ALLOC is okay, and if the default state requires
non-zero, then all clients should use an allocation function and the
allocation function should take care of the default.
For example, see the commit pair bc3f5f1 (where I ensured that all
clients allocate via a common function) and c123ef7 (where I used that
common allocation function to initialize a non-zero member, rather than
having to touch each caller).
> +++ 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? */
I can't parse that. How about "whether we relabel files", or just leaving
it
without a comment?
or even "true (default) for allowing relabels"
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 2/4] virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel'
Similarly to the previous commit, boolean variables should not start
with 'no-' prefix.
Signed-off-by: Michal Privoznik <mprivozn(a)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 8384b5d..eccecd4 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
10:06 a.m.
New subject: [libvirt] [PATCH v2 2/4] virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel'
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
Similarly to the previous commit, boolean variables should not start
with 'no-' prefix.
Signed-off-by: Michal Privoznik <mprivozn(a)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/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? */
Same issue as in the first patch.
bool labelskip; /* live-only; true if skipping failed label
attempt */
};
ACK
Jan
9:04 a.m.
New subject: [libvirt] [PATCH v2 3/4] conf: Always format seclabel's model
https://bugzilla.redhat.com/show_bug.cgi?id=1113860
We've always done that. Well, until 990e46c45. Point is, if we don't
format model, we may lose a domain on libvirtd restart. If the
seclabel is implicit however, we should skip it's formatting.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 17 ++++++-------
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 ++++++++++++++++++++++
tests/qemuxml2xmltest.c | 1 +
3 files changed, 36 insertions(+), 10 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index eccecd4..c730d37 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14677,8 +14677,7 @@ virDomainEventActionDefFormat(virBufferPtr buf,
static void
virSecurityLabelDefFormat(virBufferPtr buf,
- virSecurityLabelDefPtr def,
- unsigned flags)
+ virSecurityLabelDefPtr def)
{
const char *sectype = virDomainSeclabelTypeToString(def->type);
@@ -14688,19 +14687,17 @@ virSecurityLabelDefFormat(virBufferPtr buf,
if (def->type == VIR_DOMAIN_SECLABEL_DEFAULT)
return;
- /* To avoid backward compatibility issues, suppress DAC labels that are
- * automatically generated.
+ /* To avoid backward compatibility issues, suppress DAC and 'none' labels
+ * that are automatically generated.
*/
- if (STREQ_NULLABLE(def->model, "dac") && def->implicit)
+ if ((STREQ_NULLABLE(def->model, "dac") ||
+ STREQ_NULLABLE(def->model, "none")) && def->implicit)
return;
virBufferAsprintf(buf, "<seclabel type='%s'",
sectype);
- /* When generating state XML do include the model */
- if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS ||
- STRNEQ_NULLABLE(def->model, "none"))
- virBufferEscapeString(buf, " model='%s'", def->model);
+ virBufferEscapeString(buf, " model='%s'", def->model);
if (def->type == VIR_DOMAIN_SECLABEL_NONE) {
virBufferAddLit(buf, "/>\n");
@@ -17910,7 +17907,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferAddLit(buf, "</devices>\n");
for (n = 0; n < def->nseclabels; n++)
- virSecurityLabelDefFormat(buf, def->seclabels[n], flags);
+ virSecurityLabelDefFormat(buf, def->seclabels[n]);
if (def->namespaceData && def->ns.format) {
if ((def->ns.format)(buf, def->namespaceData) < 0)
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
new file mode 100644
index 0000000..7949f99
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
@@ -0,0 +1,28 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' 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'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <memballoon model='virtio'/>
+ </devices>
+ <seclabel type='dynamic' model='none' relabel='yes'/>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 26e3cad..9f919de 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -307,6 +307,7 @@ mymain(void)
DO_TEST_FULL("seclabel-static-labelskip", false, WHEN_ACTIVE);
DO_TEST("seclabel-none");
DO_TEST("seclabel-dac-none");
+ DO_TEST("seclabel-dynamic-none");
DO_TEST("numad-static-vcpu-no-numatune");
DO_TEST("disk-scsi-lun-passthrough-sgio");
--
1.8.5.5
11:09 a.m.
New subject: [libvirt] [PATCH v2 3/4] conf: Always format seclabel's model
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1113860
We've always done that. Well, until 990e46c45. Point is, if we don't
format model, we may lose a domain on libvirtd restart. If the
seclabel is implicit however, we should skip it's formatting.
It seems we only generate a seclabel with model="none" with
type="none".
Specifying other types (or the relabel attribute) with model="none" doesn't
make much sense.
Could we instead error out on other types than "none" when model is
"none", or
(if we can't) silently correct it to type "none"?
Also, formatting model="none" in the status XML shouldn't be needed after
that
since we only generate it with type="none" and we would reject other types.
Jan
9:04 a.m.
New subject: [libvirt] [PATCH v2 4/4] conf: Don't allow multiple seclabels for same model
https://bugzilla.redhat.com/show_bug.cgi?id=1066894
With current code it's possible to have for instance:
virsh dumpxml mydomain | grep seclabel
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
what doesn't make any sense. We should reject the XML in the config
parsing phase.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 18 ++++++++--
.../qemuxml2argv-seclabel-multiple.xml | 40 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 56 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c730d37..ace3ddf 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4668,7 +4668,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
virCapsPtr caps,
unsigned int flags)
{
- size_t i = 0;
+ size_t i = 0, j;
int n;
xmlNodePtr *list = NULL, saved_node;
virCapsHostPtr host = &caps->host;
@@ -4689,10 +4689,22 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
/* Parse each "seclabel" tag */
for (i = 0; i < n; i++) {
+ virSecurityLabelDefPtr seclabel;
+
ctxt->node = list[i];
- def->seclabels[i] = virSecurityLabelDefParseXML(ctxt, flags);
- if (def->seclabels[i] == NULL)
+ if (!(seclabel = virSecurityLabelDefParseXML(ctxt, flags)))
goto error;
+
+ for (j = 0; j < i; j++) {
+ if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("seclablel for model %s is already
provided"),
+ seclabel->model);
+ goto error;
+ }
+ }
+
+ def->seclabels[i] = seclabel;
}
def->nseclabels = n;
ctxt->node = saved_node;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
new file mode 100644
index 0000000..bd6fd15
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
@@ -0,0 +1,40 @@
+<domain type='qemu' id='1'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' 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 model='selinux' labelskip='yes'/>
+ </source>
+ <backingStore/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <memballoon model='virtio'/>
+ </devices>
+ <seclabel type='none' relabel='no'/>
+ <seclabel type='dynamic' model='dac' relabel='yes'/>
+ <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>
+ <seclabel type='static' model='selinux' relabel='yes'>
+ <label>system_u:system_r:svirt_custom_t:s0:c192,c393</label>
+ <imagelabel>system_u:system_r:svirt_custom_t:s0:c192,c393</imagelabel>
+ </seclabel>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index bbc0fb7..a841adb 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1223,6 +1223,7 @@ mymain(void)
DO_TEST("seclabel-static-labelskip", QEMU_CAPS_NAME);
DO_TEST("seclabel-none", QEMU_CAPS_NAME);
DO_TEST("seclabel-dac-none", QEMU_CAPS_NAME);
+ DO_TEST_PARSE_ERROR("seclabel-multiple", QEMU_CAPS_NAME);
DO_TEST("pseries-basic",
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
--
1.8.5.5
11:16 a.m.
New subject: [libvirt] [PATCH v2 4/4] conf: Don't allow multiple seclabels for same model
On 07/10/2014 04:04 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1066894
With current code it's possible to have for instance:
virsh dumpxml mydomain | grep seclabel
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
<seclabel type='dynamic' model='selinux' relabel='yes'/>
what doesn't make any sense. We should reject the XML in the config
s/what/which/
parsing phase.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 18 ++++++++--
.../qemuxml2argv-seclabel-multiple.xml | 40 ++++++++++++++++++++++
tests/qemuxml2argvtest.c | 1 +
3 files changed, 56 insertions(+), 3 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
@@ -4689,10 +4689,22 @@ virSecurityLabelDefsParseXML(virDomainDefPtr
def,
/* Parse each "seclabel" tag */
for (i = 0; i < n; i++) {
+ virSecurityLabelDefPtr seclabel;
+
ctxt->node = list[i];
- def->seclabels[i] = virSecurityLabelDefParseXML(ctxt, flags);
- if (def->seclabels[i] == NULL)
+ if (!(seclabel = virSecurityLabelDefParseXML(ctxt, flags)))
goto error;
+
+ for (j = 0; j < i; j++) {
+ if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("seclablel for model %s is already
provided"),
+ seclabel->model);
virSecurityLabelDefFree(seclabel);
+ goto error;
+ }
+ }
+
+ def->seclabels[i] = seclabel;
}
def->nseclabels = n;
ctxt->node = saved_node;
ACK with the leak fixed.
Jan
6:01 a.m.
On 07/10/2014 10:04 AM, Michal Privoznik wrote:
diff to v1:
- rework the 3rd patch
- introduce one more bugfix
Michal Privoznik (4):
virSecurityLabelDef: substitute 'norelabel' with 'relabel'
virSecurityDeviceLabelDef: substitute 'norelabel' with 'relabel'
conf: Always format seclabel's model
conf: Don't allow multiple seclabels for same model
src/conf/domain_conf.c | 67 ++++++++++++----------
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 +-
.../qemuxml2argv-seclabel-dynamic-none.xml | 28 +++++++++
.../qemuxml2argv-seclabel-multiple.xml | 40 +++++++++++++
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmltest.c | 1 +
11 files changed, 142 insertions(+), 67 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-none.xml
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-multiple.xml
There's a Coverity issue from these patches - it looks like perhaps
patch 1&2 were combined when submitted into commit id '13adf1b' which has:
virSecurityLabelDefPtr
virSecurityLabelDefNew(const char *model)
{
virSecurityLabelDefPtr seclabel = NULL;
if (VIR_ALLOC(seclabel) < 0 ||
VIR_STRDUP(seclabel->model, model) < 0) {
virSecurityLabelDefFree(seclabel);
seclabel = NULL;
}
+ seclabel->relabel = true;
+
return seclabel;
}
See the problem at all? It's a FORWARD_NULL on 'seclabel'.
John
3781
days inactive
3782
days old
10 comments
4 participants
participants (4)
-
Eric Blake -
John Ferlan -
Ján Tomko -
Michal Privoznik