Re: [libvirt] [PATCH] Updated to deal with specifying user IDs to that do not map to usernames

On 06/07/2016 11:15 AM, Peter Krempa wrote: > On Tue, Jun 07, 2016 at 15:50:38 +0100, Daniel Berrange wrote: >> On Tue, Jun 07, 2016 at 08:24:14AM +0200, Peter Krempa wrote: >>> On Mon, Jun 06, 2016 at 14:25:23 -0500, Roy Keene wrote: >>>> Patch to libvirt master to avoid failing when a user ID is specified, >>>> e.g. for <seclabel type='dac'>, that does not map to a user name. >>>> >>>> This is useful if you want to run each VM as a separate user and not >>>> bother creating an /etc/passwd entry for each UID. >>> For this use case you shall prefix the name with a +. Please refer to >>> the documentation on seclabels. >> Empirically that does not currently work: >> >> # virsh dumpxml serial | grep --after 2 '<seclabel' >> <seclabel type='static' model='dac' relabel='no'> >> <label>+21421:+12421421</label> >> </seclabel> >> >> # virsh start serial >> error: Failed to start domain serial >> error: Cannot access storage file >> '/var/lib/libvirt/images/demo.qcow2' (as uid:21421, gid:12421421): >> Success >> >> # ls -al /var/lib/libvirt/images/demo.qcow2 >> -rw-r--r--. 1 21421 12421421 197120 Apr 30 2015 >> /var/lib/libvirt/images/demo.qcow2 >> >> >> Looking at the libvirtd logs we see >> >> 2016-06-07 14:49:13.724+0000: 13490: debug : >> qemuDomainCheckDiskPresence:3954 : Checking for disk presence >> 2016-06-07 14:49:16.551+0000: 13490: error : virGetUserEnt:801 : >> Failed to find user record for uid '21421' >> 2016-06-07 14:49:16.551+0000: 13490: error : >> virStorageFileGetMetadataRecurse:3114 : Cannot access storage file >> '/var/lib/libvirt/images/demo.qcow2' (as uid:21421, gid:12421421): >> Success >> >> >> So even though the QEMU driver has honoured the '+' syntax, some of the >> things QEMU is calling appears to be trying to resolve the UID back >> into >> a user password record and failing. > Indeed. This is caused by calling virFileAccessibleAs which calls > virGetGroupList so that it can add all groups for the given UID which is > very strange. The group list is then set after forking at attempting to > check file presence. I hate root squashed NFS. > > I guess we could just skip reporting the error if we can't get the > group list in that case and just set the provided numerical UID/GID and > try it that way. Without taking the time to wind through the code - are you saying that we're attempting to get the grouplist for a different uid while running as root, then doing a fork/seteuid, and then adding the groups that we learned pre-fork? If that's the case, then I think the same results can be achieved by getting the grouplist *after* seteuid(). It looks like getgroups() gets the groups for the current uid, while mgetgroups() gets the groups for a specified username, so getgroups() would avoid the need for a username.