[libvirt] [PATCH v2 0/2] extend virGetUserID and virGetGroupID

This patch series moves the logic for parsing users and groups in a similar way to coreutils' chown from security_dac.c to util.c, as suggested by Eric Blake. This change has two majors side effects: 1. Some error messages that were issued when security_dac.c tried to parse an ID as a name are no longer issued. 2. The keys `user` and `group` in qemu.conf can now be defined in the same way that in DAC security labels. Peter Krempa's fix for correctly handling errors returned by getpwnam_r and getgrnam_r is squashed into this patch series. Marcelo Cerri (2): util: extend virGetUserID and virGetGroupID to support names and IDs security: update user and group parsing in security_dac.c src/security/security_dac.c | 45 +++----------- src/util/util.c | 143 ++++++++++++++++++++++++++++++++------------ 2 files changed, 112 insertions(+), 76 deletions(-) -- 1.7.12

This patch updates virGetUserID and virGetGroupID to be able to parse a user or group name in a similar way to coreutils' chown. This means that a numeric value with a leading plus sign is always parsed as an ID, otherwise the functions try to parse the input first as a user or group name and if this fails they try to parse it as an ID. This patch includes Peter Krempa's changes to correctly handle errors returned by getpwnam_r and getgrnam_r. --- src/util/util.c | 143 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 104 insertions(+), 39 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 43fdaf1..348c388 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2495,16 +2495,19 @@ char *virGetGroupName(gid_t gid) return virGetGroupEnt(gid); } - -int virGetUserID(const char *name, - uid_t *uid) +/* Search in the password database for a user id that matches the user name + * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found. + */ +static int +virGetUserIDByName(const char *name, uid_t *uid) { - char *strbuf; + char *strbuf = NULL; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1; /* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2512,48 +2515,81 @@ int virGetUserID(const char *name, if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || pw == NULL) { - virReportSystemError(rc, - _("Failed to find user record for name '%s'"), + + if (rc != 0) { + virReportSystemError(rc, _("Failed to get user record for name '%s'"), name); - VIR_FREE(strbuf); - return -1; + goto cleanup; + } + + if (!pw) { + VIR_DEBUG("User record for user '%s' does not exist", name); + ret = 1; + goto cleanup; } *uid = pw->pw_uid; + ret = 0; +cleanup: VIR_FREE(strbuf); + return ret; +} + +/* Try to match a user id based on `user`. The default behavior is to parse + * `user` first as a user name and then as a user id. However if `user` + * contains a leading '+', the rest of the string is always parsed as a uid. + * + * Returns 0 on success and -1 otherwise. + */ +int +virGetUserID(const char *user, uid_t *uid) +{ + unsigned int uint_uid; + + if (*user == '+') { + user++; + } else { + int rc = virGetUserIDByName(user, uid); + if (rc <= 0) + return rc; + } + + if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 || + ((uid_t) uint_uid) != uint_uid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + user); + return -1; + } + + *uid = uint_uid; + return 0; } - -int virGetGroupID(const char *name, - gid_t *gid) +/* Search in the group database for a group id that matches the group name + * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found. + */ +static int +virGetGroupIDByName(const char *name, gid_t *gid) { - char *strbuf; + char *strbuf = NULL; struct group grbuf; struct group *gr = NULL; long val = sysconf(_SC_GETGR_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1; /* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2561,39 +2597,68 @@ int virGetGroupID(const char *name, if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || gr == NULL) { - virReportSystemError(rc, - _("Failed to find group record for name '%s'"), + + if (rc != 0) { + virReportSystemError(rc, _("Failed to get group record for name '%s'"), name); - VIR_FREE(strbuf); - return -1; + goto cleanup; + } + + if (!gr) { + VIR_DEBUG("Group record for group '%s' does not exist", name); + ret = 1; + goto cleanup; } *gid = gr->gr_gid; + ret = 0; +cleanup: VIR_FREE(strbuf); + return ret; +} + +/* Try to match a group id based on `group`. The default behavior is to parse + * `group` first as a group name and then as a group id. However if `group` + * contains a leading '+', the rest of the string is always parsed as a guid. + * + * Returns 0 on success and -1 otherwise. + */ +int +virGetGroupID(const char *group, gid_t *gid) +{ + unsigned int uint_gid; + + if (*group == '+') { + group++; + } else { + int rc = virGetGroupIDByName(group, gid); + if (rc <= 0) + return rc; + } + + if (virStrToLong_ui(group, NULL, 10, &uint_gid) < 0 || + ((gid_t) uint_gid) != uint_gid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse group '%s'"), + group); + return -1; + } + + *gid = uint_gid; + return 0; } - /* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of * that uid. return 0 on success, -1 on failure (the original system error -- 1.7.12

The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 45 ++++++++------------------------------------- 1 file changed, 8 insertions(+), 37 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a427e9d..22edba2 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -69,8 +69,8 @@ static int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) { int rc = -1; - unsigned int theuid; - unsigned int thegid; + uid_t theuid; + gid_t thegid; char *tmp_label = NULL; char *sep = NULL; char *owner = NULL; @@ -94,41 +94,12 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) owner = tmp_label; group = sep + 1; - /* Parse owner */ - if (*owner == '+') { - if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid uid \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } - } else { - if (virGetUserID(owner, &theuid) < 0 && - virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } - } - - /* Parse group */ - if (*group == '+') { - if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid gid \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } - } else { - if (virGetGroupID(group, &thegid) < 0 && - virStrToLong_ui(group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid group \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } - } + /* Parse owner and group, error message is defined by + * virGetUserID or virGetGroupID. + */ + if (virGetUserID(owner, &theuid) < 0 || + virGetGroupID(group, &thegid) < 0) + goto cleanup; if (uidPtr) *uidPtr = theuid; -- 1.7.12

On 10/08/2012 02:37 PM, Marcelo Cerri wrote:
This patch series moves the logic for parsing users and groups in a similar way to coreutils' chown from security_dac.c to util.c, as suggested by Eric Blake.
This change has two majors side effects:
1. Some error messages that were issued when security_dac.c tried to parse an ID as a name are no longer issued. 2. The keys `user` and `group` in qemu.conf can now be defined in the same way that in DAC security labels.
Peter Krempa's fix for correctly handling errors returned by getpwnam_r and getgrnam_r is squashed into this patch series.
Marcelo Cerri (2): util: extend virGetUserID and virGetGroupID to support names and IDs security: update user and group parsing in security_dac.c
src/security/security_dac.c | 45 +++----------- src/util/util.c | 143 ++++++++++++++++++++++++++++++++------------ 2 files changed, 112 insertions(+), 76 deletions(-)
ACK series, and pushed. However, I wonder if we should prepare a followup patch to src/qemu/qemu.conf, documenting the new semantics of the 'user' and 'group' config items for accepting numeric ids via leading '+' in the string. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Mon, Oct 08, 2012 at 03:23:41PM -0600, Eric Blake wrote:
On 10/08/2012 02:37 PM, Marcelo Cerri wrote:
This patch series moves the logic for parsing users and groups in a similar way to coreutils' chown from security_dac.c to util.c, as suggested by Eric Blake.
This change has two majors side effects:
1. Some error messages that were issued when security_dac.c tried to parse an ID as a name are no longer issued. 2. The keys `user` and `group` in qemu.conf can now be defined in the same way that in DAC security labels.
Peter Krempa's fix for correctly handling errors returned by getpwnam_r and getgrnam_r is squashed into this patch series.
Marcelo Cerri (2): util: extend virGetUserID and virGetGroupID to support names and IDs security: update user and group parsing in security_dac.c
src/security/security_dac.c | 45 +++----------- src/util/util.c | 143 ++++++++++++++++++++++++++++++++------------ 2 files changed, 112 insertions(+), 76 deletions(-)
ACK series, and pushed. However, I wonder if we should prepare a followup patch to src/qemu/qemu.conf, documenting the new semantics of the 'user' and 'group' config items for accepting numeric ids via leading '+' in the string.
I agree. I've just sent a patch as you suggested: https://www.redhat.com/archives/libvir-list/2012-October/msg00342.html
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Marcelo Cerri