On 09/20/12 23:07, Marcelo Cerri wrote:
The DAC driver is missing parsing of group and user names for DAC labels
and currently just parses uid and gid. This patch extends it to support
names, so the following security label definition is now valid:
<seclabel type='static' model='dac' relabel='yes'>
<label>qemu:qemu</label>
<imagelabel>qemu:qemu</imagelabel>
</seclabel>
When it tries to parse an owner or a group, it first tries to resolve it as
a name, if it fails or it's an invalid user/group name then it tries to
parse it as an UID or GID. A leading '+' can also be used for both owner and
group to force it to be parsed as IDs, so the following example is also
valid:
<seclabel type='static' model='dac' relabel='yes'>
<label>+101:+101</label>
<imagelabel>+101:+101</imagelabel>
</seclabel>
This ensures that UID 101 and GUI 101 will be used instead of an user or
group named "101".
---
src/security/security_dac.c | 62 +++++++++++++++++++++++++++++++++++++++------
1 file changed, 54 insertions(+), 8 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 5f30f0f..00cbb8a 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -68,26 +68,72 @@ void
virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr,
static
int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr)
{
+ int rc = -1;
unsigned int theuid;
unsigned int thegid;
- char *endptr = NULL;
+ char *tmp_label = NULL;
+ char *sep = NULL;
+ char *owner = NULL;
+ char *group = NULL;
if (label == NULL)
- return -1;
+ goto cleanup;
"label" is guaranteed to be non-null here. Removing this condition will
prevent you from inventing a new error message for this while changing
to proper error reporting (see below.).
- if (virStrToLong_ui(label, &endptr, 10, &theuid) ||
- endptr == NULL || *endptr != ':') {
- return -1;
+ tmp_label = strdup(label);
+ if (tmp_label == NULL) {
+ virReportOOMError();
+ goto cleanup;
}
- if (virStrToLong_ui(endptr + 1, NULL, 10, &thegid))
- return -1;
+ /* Split label */
+ sep = strchr(tmp_label, ':');
+ if (sep == NULL) {
+ VIR_DEBUG("Missgin separator ':' in DAC label \"%s\"", label);
s/Missgin/Missing/
Also now when this function has a lot of more detail to report about the
problems with the security label, I'd change it back to reporting a
proper error and removing the error in the function that calls this that
would mas errors from here.
I think a good error code here would be VIR_ERR_INVALID_ARG.
+ goto cleanup;
+ }
+ *sep = '\0';
+ owner = tmp_label;
+ group = sep + 1;
+
+ /* Parse owner */
+ if (*owner == '+') {
+ if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) {
+ VIR_DEBUG("Invalid uid \"%s\" in DAC label \"%s\"", owner, label);
Also this is a nice proper error message.
+ goto cleanup;
+ }
+ } else {
+ if (virGetUserID(owner, &theuid) < 0 &&
+ virStrToLong_ui(owner, NULL, 10, &theuid) < 0) {
+ VIR_DEBUG("Invalid owner \"%s\" in DAC label \"%s\"", owner,
label);
Here too.
+ goto cleanup;
+ }
+ }
+
+ /* Parse group */
+ if (*group == '+') {
+ if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) {
+ VIR_DEBUG("Invalid gid \"%s\" in DAC label \"%s\"", group, label);
Here.
+ goto cleanup;
+ }
+ } else {
+ if (virGetGroupID(group, &thegid) < 0 &&
+ virStrToLong_ui(group, NULL, 10, &thegid) < 0) {
+ VIR_DEBUG("Invalid group \"%s\" in DAC label \"%s\"", group,
label);
And here :)
+ goto cleanup;
+ }
+ }
if (uidPtr)
*uidPtr = theuid;
if (gidPtr)
*gidPtr = thegid;
- return 0;
+
+ rc = 0;
+
+cleanup:
+ VIR_FREE(tmp_label);
+
+ return rc;
}
/* returns 1 if label isn't found, 0 on success, -1 on error */
When changing the the error messages as I suggest, you will have to
squash in the attached patch to avoid masking the errors. Otherwise the
code looks fine and I'm looking forward to review v3.
Peter
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 1268f13..8e72aff 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -153,12 +153,8 @@ int virSecurityDACParseIds(virDomainDefPtr def, uid_t
*uidPtr, gid_t *gidPtr)
return 1;
}
- if (seclabel->label && parseIds(seclabel->label, &uid, &gid)) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("failed to parse DAC seclabel '%s' for domain '%s'"),
- seclabel->label, def->name);
+ if (parseIds(seclabel->label, &uid, &gid) < 0)
return -1;
- }
if (uidPtr)
*uidPtr = uid;
@@ -218,13 +214,8 @@ int virSecurityDACParseImageIds(virDomainDefPtr def,
return 1;
}
- if (seclabel->imagelabel
- && parseIds(seclabel->imagelabel, &uid, &gid)) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("failed to parse DAC imagelabel '%s' for domain
'%s'"),
- seclabel->imagelabel, def->name);
+ if (parseIds(seclabel->imagelabel, &uid, &gid) < 0)
return -1;
- }
if (uidPtr)
*uidPtr = uid;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
