7:51 a.m.
Couple of things happening here:
1) add a missing case to our polkit checks
2) modernize XML parsing and formatting, so that the code is prepared
for a new feature I'm working on.
Michal Prívozník (4):
viraccessdriverpolkit: Add missing vtpm case
secret_conf: Simplify calling of virSecretDefParseUsage()
virSecretDef: Convert 'usage_type' field to proper enum type
secret_conf: Modernize XML parsing & formatting
src/access/viraccessdriverpolkit.c | 17 ++++++
src/conf/secret_conf.c | 92 ++++++++++++++----------------
src/conf/secret_conf.h | 2 +-
3 files changed, 60 insertions(+), 51 deletions(-)
--
2.43.0
7:51 a.m.
New subject: [PATCH 1/4] viraccessdriverpolkit: Add missing vtpm case
When adding vtpm virSecret usage type (in v5.6.0-rc1~61) we
forgot to update polkit access check. This limited user's ability
to match secrets in their rules. Add missing case into switch in
virAccessDriverPolkitCheckSecret().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/access/viraccessdriverpolkit.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c
index c7ff81ee86..b78049583c 100644
--- a/src/access/viraccessdriverpolkit.c
+++ b/src/access/viraccessdriverpolkit.c
@@ -390,6 +390,19 @@ virAccessDriverPolkitCheckSecret(virAccessManager *manager,
virAccessPermSecretTypeToString(perm),
attrs);
} break;
+ case VIR_SECRET_USAGE_TYPE_VTPM: {
+ const char *attrs[] = {
+ "connect_driver", driverName,
+ "secret_uuid", uuidstr,
+ "secret_usage_name", secret->usage_id,
+ NULL,
+ };
+
+ return virAccessDriverPolkitCheck(manager,
+ "secret",
+ virAccessPermSecretTypeToString(perm),
+ attrs);
+ } break;
}
}
--
2.43.0
7:51 a.m.
New subject: [PATCH 2/4] secret_conf: Simplify calling of virSecretDefParseUsage()
The virSecretDefParseUsage() function is called conditionally.
Call it unconditionally and keep pointer to the <usage/> node as
it'll come handy soon.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/secret_conf.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 7463185eb1..d8ce772d3b 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -49,10 +49,17 @@ static int
virSecretDefParseUsage(xmlXPathContextPtr ctxt,
virSecretDef *def)
{
+ xmlNodePtr node = NULL;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt)
g_autofree char *type_str = NULL;
int type;
- type_str = virXPathString("string(./usage/@type)", ctxt);
+ if (!(node = virXPathNode("./usage", ctxt)))
+ return 0;
+
+ ctxt->node = node;
+
+ type_str = virXMLPropString(node, "type");
if (type_str == NULL) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("unknown secret usage type"));
@@ -70,7 +77,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
break;
case VIR_SECRET_USAGE_TYPE_VOLUME:
- def->usage_id = virXPathString("string(./usage/volume)", ctxt);
+ def->usage_id = virXPathString("string(./volume)", ctxt);
if (!def->usage_id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("volume usage specified, but volume path is
missing"));
@@ -79,7 +86,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
break;
case VIR_SECRET_USAGE_TYPE_CEPH:
- def->usage_id = virXPathString("string(./usage/name)", ctxt);
+ def->usage_id = virXPathString("string(./name)", ctxt);
if (!def->usage_id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Ceph usage specified, but name is missing"));
@@ -88,7 +95,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
break;
case VIR_SECRET_USAGE_TYPE_ISCSI:
- def->usage_id = virXPathString("string(./usage/target)", ctxt);
+ def->usage_id = virXPathString("string(./target)", ctxt);
if (!def->usage_id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("iSCSI usage specified, but target is missing"));
@@ -97,7 +104,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
break;
case VIR_SECRET_USAGE_TYPE_TLS:
- def->usage_id = virXPathString("string(./usage/name)", ctxt);
+ def->usage_id = virXPathString("string(./name)", ctxt);
if (!def->usage_id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("TLS usage specified, but name is missing"));
@@ -106,7 +113,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
break;
case VIR_SECRET_USAGE_TYPE_VTPM:
- def->usage_id = virXPathString("string(./usage/name)", ctxt);
+ def->usage_id = virXPathString("string(./name)", ctxt);
if (!def->usage_id) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("vTPM usage specified, but name is missing"));
@@ -166,8 +173,8 @@ virSecretParseXML(xmlXPathContext *ctxt)
}
def->description = virXPathString("string(./description)", ctxt);
- if (virXPathNode("./usage", ctxt) != NULL
- && virSecretDefParseUsage(ctxt, def) < 0)
+
+ if (virSecretDefParseUsage(ctxt, def) < 0)
return NULL;
return g_steal_pointer(&def);
--
2.43.0
7:51 a.m.
New subject: [PATCH 3/4] virSecretDef: Convert 'usage_type' field to proper enum type
Convert the field and adjust the XML parsers to use
virXMLPropEnum().
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/access/viraccessdriverpolkit.c | 4 ++++
src/conf/secret_conf.c | 24 ++++++++----------------
src/conf/secret_conf.h | 2 +-
3 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/src/access/viraccessdriverpolkit.c b/src/access/viraccessdriverpolkit.c
index b78049583c..83381183a5 100644
--- a/src/access/viraccessdriverpolkit.c
+++ b/src/access/viraccessdriverpolkit.c
@@ -403,6 +403,10 @@ virAccessDriverPolkitCheckSecret(virAccessManager *manager,
virAccessPermSecretTypeToString(perm),
attrs);
} break;
+
+ case VIR_SECRET_USAGE_TYPE_LAST:
+ virReportEnumRangeError(virSecretUsageType, secret->usage_type);
+ return -1;
}
}
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index d8ce772d3b..c6c8c30ba9 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -51,27 +51,19 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
{
xmlNodePtr node = NULL;
VIR_XPATH_NODE_AUTORESTORE(ctxt)
- g_autofree char *type_str = NULL;
- int type;
if (!(node = virXPathNode("./usage", ctxt)))
return 0;
ctxt->node = node;
- type_str = virXMLPropString(node, "type");
- if (type_str == NULL) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("unknown secret usage type"));
+ if (virXMLPropEnum(node, "type",
+ virSecretUsageTypeFromString,
+ VIR_XML_PROP_REQUIRED,
+ &def->usage_type) < 0) {
return -1;
}
- type = virSecretUsageTypeFromString(type_str);
- if (type < 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown secret usage type %1$s"), type_str);
- return -1;
- }
- def->usage_type = type;
+
switch (def->usage_type) {
case VIR_SECRET_USAGE_TYPE_NONE:
break;
@@ -121,6 +113,7 @@ virSecretDefParseUsage(xmlXPathContextPtr ctxt,
}
break;
+ case VIR_SECRET_USAGE_TYPE_LAST:
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected secret usage type %1$d"),
@@ -238,9 +231,8 @@ virSecretDefFormatUsage(virBuffer *buf,
break;
default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected secret usage type %1$d"),
- def->usage_type);
+ case VIR_SECRET_USAGE_TYPE_LAST:
+ virReportEnumRangeError(virSecretUsageType, def->usage_type);
return -1;
}
virBufferAdjustIndent(buf, -2);
diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h
index dee98899ac..8f8f47933a 100644
--- a/src/conf/secret_conf.h
+++ b/src/conf/secret_conf.h
@@ -28,7 +28,7 @@ struct _virSecretDef {
bool isprivate;
unsigned char uuid[VIR_UUID_BUFLEN];
char *description; /* May be NULL */
- int usage_type; /* virSecretUsageType */
+ virSecretUsageType usage_type;
char *usage_id; /* May be NULL */
};
--
2.43.0
7:51 a.m.
New subject: [PATCH 4/4] secret_conf: Modernize XML parsing & formatting
Our virSecret XML is still parsed and formatted using old way
(e.g. virXPathString() + virXXXTypeFromString() combo, or
formatting elements using plain virBufferAsprintf() instead of
virXMLFormatElement()). Modernize the code as it'll make it
easier for future expansion.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/secret_conf.c | 47 ++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 27 deletions(-)
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index c6c8c30ba9..966536599e 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -195,39 +195,34 @@ static int
virSecretDefFormatUsage(virBuffer *buf,
const virSecretDef *def)
{
- const char *type;
+ g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+ g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+
+ virBufferAsprintf(&attrBuf, " type='%s'",
+ virSecretUsageTypeToString(def->usage_type));
- type = virSecretUsageTypeToString(def->usage_type);
- if (type == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected secret usage type %1$d"),
- def->usage_type);
- return -1;
- }
- virBufferAsprintf(buf, "<usage type='%s'>\n", type);
- virBufferAdjustIndent(buf, 2);
switch (def->usage_type) {
case VIR_SECRET_USAGE_TYPE_NONE:
break;
case VIR_SECRET_USAGE_TYPE_VOLUME:
- virBufferEscapeString(buf, "<volume>%s</volume>\n",
def->usage_id);
+ virBufferEscapeString(&childBuf,
"<volume>%s</volume>\n", def->usage_id);
break;
case VIR_SECRET_USAGE_TYPE_CEPH:
- virBufferEscapeString(buf, "<name>%s</name>\n",
def->usage_id);
+ virBufferEscapeString(&childBuf, "<name>%s</name>\n",
def->usage_id);
break;
case VIR_SECRET_USAGE_TYPE_ISCSI:
- virBufferEscapeString(buf, "<target>%s</target>\n",
def->usage_id);
+ virBufferEscapeString(&childBuf,
"<target>%s</target>\n", def->usage_id);
break;
case VIR_SECRET_USAGE_TYPE_TLS:
- virBufferEscapeString(buf, "<name>%s</name>\n",
def->usage_id);
+ virBufferEscapeString(&childBuf, "<name>%s</name>\n",
def->usage_id);
break;
case VIR_SECRET_USAGE_TYPE_VTPM:
- virBufferEscapeString(buf, "<name>%s</name>\n",
def->usage_id);
+ virBufferEscapeString(&childBuf, "<name>%s</name>\n",
def->usage_id);
break;
default:
@@ -235,8 +230,8 @@ virSecretDefFormatUsage(virBuffer *buf,
virReportEnumRangeError(virSecretUsageType, def->usage_type);
return -1;
}
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</usage>\n");
+
+ virXMLFormatElement(buf, "usage", &attrBuf, &childBuf);
return 0;
}
@@ -245,25 +240,23 @@ char *
virSecretDefFormat(const virSecretDef *def)
{
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- const unsigned char *uuid;
+ g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+ g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(&buf);
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virBufferAsprintf(&buf, "<secret ephemeral='%s'
private='%s'>\n",
+ virBufferAsprintf(&attrBuf, " ephemeral='%s'
private='%s'",
def->isephemeral ? "yes" : "no",
def->isprivate ? "yes" : "no");
- uuid = def->uuid;
- virUUIDFormat(uuid, uuidstr);
- virBufferAdjustIndent(&buf, 2);
- virBufferEscapeString(&buf, "<uuid>%s</uuid>\n", uuidstr);
+ virUUIDFormat(def->uuid, uuidstr);
+ virBufferEscapeString(&childBuf, "<uuid>%s</uuid>\n",
uuidstr);
if (def->description != NULL)
- virBufferEscapeString(&buf,
"<description>%s</description>\n",
+ virBufferEscapeString(&childBuf,
"<description>%s</description>\n",
def->description);
if (def->usage_type != VIR_SECRET_USAGE_TYPE_NONE &&
- virSecretDefFormatUsage(&buf, def) < 0)
+ virSecretDefFormatUsage(&childBuf, def) < 0)
return NULL;
- virBufferAdjustIndent(&buf, -2);
- virBufferAddLit(&buf, "</secret>\n");
+ virXMLFormatElement(&buf, "secret", &attrBuf, &childBuf);
return virBufferContentAndReset(&buf);
}
--
2.43.0
8:13 a.m.
On a Tuesday in 2024, Michal Privoznik wrote:
Couple of things happening here:
1) add a missing case to our polkit checks
2) modernize XML parsing and formatting, so that the code is prepared
for a new feature I'm working on.
Michal Prívozník (4):
viraccessdriverpolkit: Add missing vtpm case
secret_conf: Simplify calling of virSecretDefParseUsage()
virSecretDef: Convert 'usage_type' field to proper enum type
secret_conf: Modernize XML parsing & formatting
src/access/viraccessdriverpolkit.c | 17 ++++++
src/conf/secret_conf.c | 92 ++++++++++++++----------------
src/conf/secret_conf.h | 2 +-
3 files changed, 60 insertions(+), 51 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
318
days inactive
318
days old
5 comments
2 participants
participants (2)
-
Ján Tomko -
Michal Privoznik