[libvirt] [PATCH] Updated to deal with specifying user IDs to that do not map to usernames
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. It compiles but is as yet untested. --- src/util/virutil.c | 69 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index d80d994..ae95237 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -790,26 +790,57 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir) if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) goto cleanup; } - if (rc != 0) { - virReportSystemError(rc, - _("Failed to find user record for uid '%u'"), - (unsigned int) uid); - goto cleanup; - } else if (pw == NULL) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to find user record for uid '%u'"), - (unsigned int) uid); - goto cleanup; - } - if (name && VIR_STRDUP(*name, pw->pw_name) < 0) - goto cleanup; - if (group) - *group = pw->pw_gid; - if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) { - if (name) - VIR_FREE(*name); - goto cleanup; + if (rc != 0 || pw == NULL) { + /* + * If the user does not exist or its data is not present, return + * a created username. + */ + VIR_FREE(strbuf); + + strbuflen = 128; + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + return(-1); + } + + /* + * Fake user home directory: / + */ + if (dir) { + if (VIR_STRDUP(*dir, "/") < 0) { + goto cleanup; + } + } + + /* + * Fake user GID: Same as UID + */ + if (group) { + *group = (gid_t) uid; + } + + /* + * Fake user name: Same as UID (in string) + */ + snprintf(strbuf, strbuflen, "%llu", (unsigned long long) uid); + + if (name && VIR_STRDUP(*name, strbuf) < 0) { + if (dir) { + VIR_FREE(*dir); + } + goto cleanup; + } + } else { + if (name && VIR_STRDUP(*name, pw->pw_name) < 0) + goto cleanup; + if (group) + *group = pw->pw_gid; + if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) { + if (name) + VIR_FREE(*name); + goto cleanup; + } } ret = 0; -- 2.7.4
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. https://libvirt.org/formatdomain.html#seclabel
It compiles but is as yet untested.
--- src/util/virutil.c | 69 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 19 deletions(-)
NACK to this patch Peter
diff --git a/src/util/virutil.c b/src/util/virutil.c index d80d994..ae95237 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -790,26 +790,57 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir) if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) goto cleanup; } - if (rc != 0) { - virReportSystemError(rc, - _("Failed to find user record for uid '%u'"), - (unsigned int) uid); - goto cleanup; - } else if (pw == NULL) { - virReportError(VIR_ERR_SYSTEM_ERROR, - _("Failed to find user record for uid '%u'"), - (unsigned int) uid); - goto cleanup; - }
- if (name && VIR_STRDUP(*name, pw->pw_name) < 0) - goto cleanup; - if (group) - *group = pw->pw_gid; - if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) { - if (name) - VIR_FREE(*name); - goto cleanup; + if (rc != 0 || pw == NULL) { + /* + * If the user does not exist or its data is not present, return + * a created username. + */ + VIR_FREE(strbuf); + + strbuflen = 128; + + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { + return(-1); + } + + /* + * Fake user home directory: / + */ + if (dir) { + if (VIR_STRDUP(*dir, "/") < 0) { + goto cleanup; + } + }
Erm no. We should not do this. Not at this level.
+ + /* + * Fake user GID: Same as UID + */ + if (group) { + *group = (gid_t) uid; + } + + /* + * Fake user name: Same as UID (in string) + */ + snprintf(strbuf, strbuflen, "%llu", (unsigned long long) uid); + + if (name && VIR_STRDUP(*name, strbuf) < 0) { + if (dir) { + VIR_FREE(*dir); + } + goto cleanup; + } + } else { + if (name && VIR_STRDUP(*name, pw->pw_name) < 0) + goto cleanup; + if (group) + *group = pw->pw_gid; + if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) { + if (name) + VIR_FREE(*name); + goto cleanup; + } }
ret = 0; -- 2.7.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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. Peter
On Tue, Jun 07, 2016 at 05:15:54PM +0200, 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.
Yep, if all we're doing is trying to get supplementary groups, then lack of an /etc/passwd or /etc/group entry really just semantically means zero supplementary groups are needed. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Jun 07, 2016 at 04:16:46PM +0100, Daniel P. Berrange wrote:
On Tue, Jun 07, 2016 at 05:15:54PM +0200, 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.
Yep, if all we're doing is trying to get supplementary groups, then lack of an /etc/passwd or /etc/group entry really just semantically means zero supplementary groups are needed.
Roy, perhaps test if the following change makes it work for you: diff --git a/src/util/virutil.c b/src/util/virutil.c index d80d994..20a426d 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1109,8 +1109,11 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list) if (uid == (uid_t)-1) return 0; - if (virGetUserEnt(uid, &user, &primary, NULL) < 0) - return -1; + if (virGetUserEnt(uid, &user, &primary, NULL) < 0) { + VIR_INFO("No passwd entry for uid %llu, setting empty group list" + (unsigned long long)uid); + return 0; + } ret = mgetgroups(user, primary, list); if (ret < 0) { Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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:
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
On Mon, Jun 06, 2016 at 14:25:23 -0500, Roy Keene wrote: 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.
On 06/07/2016 11:31 AM, Laine Stump wrote:
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:
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
On Mon, Jun 06, 2016 at 14:25:23 -0500, Roy Keene wrote: 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.
Ignore that - it's a lot of effort for nothing. What Dan says by definition gives correct results.
On Tue, Jun 07, 2016 at 11:31:02AM -0400, Laine Stump wrote:
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:
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
On Mon, Jun 06, 2016 at 14:25:23 -0500, Roy Keene wrote: 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.
I don't think that's going to work as you have a chicken nand egg scenario. ie getgroups() is just reporting the list of supplementary groups set by a previous call to initgroups(). It is the inigroups call that we need to make and why we're lookiing up the grouplist beforehand. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Peter Krempa -
Roy Keene