[libvirt] [PATCH v3 4/4] util: Make failure to get supplementary group list for a uid non-fatal

[Peter Krempa]

On Mon, Jun 20, 2016 at 08:10:10 -0400, John Ferlan wrote:
> 
> 
> On 06/17/2016 09:44 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
> > supplementary groups for the given uid. Since a uid without entry in
> > /etc/passwd (or other user database) will not have any supplementary
> > groups we can treat the failure to obtain them as such.
> > 
> > This patch modifies virGetGroupList to not report the error for missing
> > users and makes it return an empty list or just the group specified in
> > @gid.
> > 
> > All callers will grand less permissions in case of failure and thus this
> > change is safe.
> 
> "grand less"?  did you mean "gain less"?

I've explained it a bit more:

All callers will grant less permissions to a user in case of failure of
this function and thus this change is safe.

> ACK series (just typed out loud below)
> 
> John
> > ---
> >  src/util/virutil.c | 31 ++++++++++++++++---------------
> >  1 file changed, 16 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > index 392091b..60da17b 100644
> > --- a/src/util/virutil.c
> > +++ b/src/util/virutil.c

[...]

> > +        }
> >      }
> 
> Playing the "what if" game here.  If for some reason uid == -1 OR
> virGetUserEnt fails, then
> 
> gid - still to be determined
> ret = 0
> 
> > 
> >      if (gid != (gid_t)-1) {
> > 
> 
> If we enter this "if", then the for loop will exit immediately and the
> VIR_APPEND_ELEMENT will append that gid onto *list which is IIRC what we
> want. Then ret will be set to 1 (i) and we return. Otherwise if gid ==
> -1, then we return 0, which is still fine.

Yep I wanted to achieve exactly this logic as it makes most sense IMO.

I've pushed this now. Thanks.

Peter