$subj: "supplementary"
On 06/15/2016 11:58 AM, Peter Krempa wrote:
> Since introduction of the DAC security driver we've documented that
> seclabels with a leading + can be used with numerical uid. This would
> not work though with the rest of libvirt if the uid was not actually
> used in the system as we'd fail when trying to get a list of
> suplementary groups for the given uid. Since a uid without entry in
ditto
> /etc/passwd (or other user database) will not have any suppolementary
ditto
> groups we can treat the failure to obtain them as such.
>
> This patch modifies virGetGroupList to not report the error of missing
> user and tweaks callers to treat the missing list as having 0
> supplementary groups.
"and tweaks callers that then won't need the supplementary list to
ignore the failure."
>
> The only place reporting errors is virt-login-shell as it's used to
> determine whether the given user is allowed to access the shell.
> ---
> Although it was ACKed I'm reposting a rebased version including the fixes and
> fixed merge conflicts. I'd like to hear feedback from the reporter of this issue.
>
> CC: Roy Keene <rkeene@xxxxxxxxxxxxxxx>
>
> src/security/security_dac.c | 13 +++++++------
> src/util/vircommand.c | 4 +++-
> src/util/virfile.c | 28 ++++++++++++++++------------
> src/util/virutil.c | 27 +++++++++++++++++----------
> tools/virt-login-shell.c | 6 +++++-
> 5 files changed, 48 insertions(+), 30 deletions(-)
>
You did mix those "quiet" changes into the same patch...
Similarly you could just modify virGetGroupList to add a quiet argument
where quiet (or whatever it is) means don't return an error. Just a
thought...
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 442ce70..9dec201 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -266,14 +266,15 @@ static int
> virSecurityDACPreFork(virSecurityManagerPtr mgr)
> {
> virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> - int ngroups;
>
> VIR_FREE(priv->groups);
> - priv->ngroups = 0;
> - if ((ngroups = virGetGroupList(priv->user, priv->group,
> - &priv->groups)) < 0)
> - return -1;
> - priv->ngroups = ngroups;
> +
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> + if ((priv->ngroups = virGetGroupList(priv->user, priv->group,
> + &priv->groups)) < 0)
> + priv->ngroups = 0;
> +
> return 0;
> }
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index f5bd7af..58af06a 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -554,8 +554,10 @@ virExec(virCommandPtr cmd)
> childerr = null;
> }
>
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
> - goto cleanup;
> + ngroups = 0;
>
> pid = virFork();
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 9d460b9..4298ec5 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1978,9 +1978,10 @@ virFileAccessibleAs(const char *path, int mode,
> gid == getegid())
> return access(path, mode);
>
> - ngroups = virGetGroupList(uid, gid, &groups);
> - if (ngroups < 0)
> - return -1;
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> + if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
Since mgetgroups() returns -1 on failure, this could be masking
something different... It wasn't clear why the comparison to -1 in
some/most cases, while others were < 0...
> + ngroups = 0;
>
> pid = virFork();
>
> @@ -2104,9 +2105,10 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
> * following dance avoids problems caused by root-squashing
> * NFS servers. */
>
> - ngroups = virGetGroupList(uid, gid, &groups);
> - if (ngroups < 0)
> - return -errno;
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> + if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
> + ngroups = 0;
>
> if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
> ret = -errno;
> @@ -2407,9 +2409,10 @@ virFileRemove(const char *path,
> if (gid == (gid_t) -1)
> gid = getegid();
>
> - ngroups = virGetGroupList(uid, gid, &groups);
> - if (ngroups < 0)
> - return -errno;
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> + if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
> + ngroups = 0;
>
> pid = virFork();
>
> @@ -2583,9 +2586,10 @@ virDirCreate(const char *path,
> if (gid == (gid_t) -1)
> gid = getegid();
>
> - ngroups = virGetGroupList(uid, gid, &groups);
> - if (ngroups < 0)
> - return -errno;
> + /* ignore a possible problem in getting supplementary groups just assume
> + * we have none and continue with uid/gid only */
> + if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
> + ngroups = 0;
>
> pid = virFork();
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index ff58054..c11278f 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -755,9 +755,10 @@ virGetUserDirectory(void)
>
> #ifdef HAVE_GETPWUID_R
> /* Look up fields from the user database for the given user. On
> - * error, set errno, report the error, and return -1. */
> + * error, set errno, report the error if not instructed otherwise via @quiet,
> + * and return -1. */
> static int
> -virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell)
> +virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell, bool quiet)
> {
> char *strbuf;
> struct passwd pwbuf;
> @@ -792,12 +793,19 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell)
> if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
> goto cleanup;
> }
> +
> if (rc != 0) {
> + if (quiet)
> + goto cleanup;
> +
> virReportSystemError(rc,
> _("Failed to find user record for uid '%u'"),
> (unsigned int) uid);
> goto cleanup;
> } else if (pw == NULL) {
> + if (quiet)
> + goto cleanup;
> +
> virReportError(VIR_ERR_SYSTEM_ERROR,
> _("Failed to find user record for uid '%u'"),
> (unsigned int) uid);
> @@ -882,7 +890,7 @@ char *
> virGetUserDirectoryByUID(uid_t uid)
> {
> char *ret;
> - virGetUserEnt(uid, NULL, NULL, &ret, NULL);
> + virGetUserEnt(uid, NULL, NULL, &ret, NULL, false);
> return ret;
> }
>
> @@ -890,7 +898,7 @@ virGetUserDirectoryByUID(uid_t uid)
> char *virGetUserShell(uid_t uid)
> {
> char *ret;
> - virGetUserEnt(uid, NULL, NULL, NULL, &ret);
> + virGetUserEnt(uid, NULL, NULL, NULL, &ret, false);
> return ret;
> }
>
> @@ -940,7 +948,7 @@ char *virGetUserRuntimeDirectory(void)
> char *virGetUserName(uid_t uid)
> {
> char *ret;
> - virGetUserEnt(uid, &ret, NULL, NULL, NULL);
> + virGetUserEnt(uid, &ret, NULL, NULL, NULL, false);
> return ret;
> }
>
> @@ -1113,8 +1121,9 @@ virGetGroupID(const char *group, gid_t *gid)
> /* Compute the list of primary and supplementary groups associated
> * with @uid, and including @gid in the list (unless it is -1),
> * storing a malloc'd result into @list. Return the size of the list
> - * on success, or -1 on failure with error reported and errno set. May
> - * not be called between fork and exec. */
> + * on success, or -1 on failure with no error reported as this usually isn't
> + * a fatal problem for callers. errno is set on error. May not be called
> + * between fork and exec. */
> int
> virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
> {
> @@ -1126,14 +1135,12 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
> if (uid == (uid_t)-1)
> return 0;
>
> - if (virGetUserEnt(uid, &user, &primary, NULL, NULL) < 0)
> + if (virGetUserEnt(uid, &user, &primary, NULL, NULL, true) < 0)
> return -1;
Maybe this should return -2; To allow the callers to distinguish?
-2 means no user
-1 means some error getting group list
0-n is the count of entries on the group list.
>
> ret = mgetgroups(user, primary, list);
Once we get here we know "user"/uid_t exists on this host, right? The
"user" will not be NULL, we'll have some sort of primary, and with any
luck get a list. If getting the list fails, then I would think we'd want
to trap that. I was only half following the IRC discussion though
> if (ret < 0) {
> sa_assert(!*list);
> - virReportSystemError(errno,
> - _("cannot get group list for '%s'"), user);
So removing this could be masking a different problem...
John
BTW: Using the most recent coverity, I tried removing that sa_assert and
coverity didn't complain... Still on a todo list I have somewhere - go
through all those sa_asserts (sigh).
> goto cleanup;
> }
>
> diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
> index 38fcb9e..2ad6634 100644
> --- a/tools/virt-login-shell.c
> +++ b/tools/virt-login-shell.c
> @@ -303,8 +303,12 @@ main(int argc, char **argv)
> if (!(conf = virConfReadFile(login_shell_path, 0)))
> goto cleanup;
>
> - if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
> + if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to query supplementary group list for uid '%u'"),
> + (unsigned int) uid);
> goto cleanup;
> + }
>
> if (virLoginShellAllowedUser(conf, name, groups) < 0)
> goto cleanup;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list