On Mon, Mar 11, 2013 at 02:26:48PM +0800, Gao feng wrote:
> This patch introduces new helper function
> virLXCControllerSetupUserns, in this function,
> we set the files uid_map and gid_map of process
> libvirt_lxc.
>
> lxcContainerSetUserns is used for creating cred for
> tasks running in container. Since after setuid/setgid,
> we may be a new user. This patch calls lxcContainerSetUserns
> at first to make sure the new created files belong to
> right user.
>
> Signed-off-by: Gao feng <gaofeng@xxxxxxxxxxxxxx>
> ---
> src/lxc/lxc_container.c | 55 ++++++++++++++++++++++++++++++----------
> src/lxc/lxc_controller.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 107 insertions(+), 14 deletions(-)
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 1d7bc1e..5c66ae3 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -329,6 +329,29 @@ int lxcContainerWaitForContinue(int control)
>
>
> /**
> + * lxcContainerSetUserns:
> + *
> + * This function calls setuid and setgid to create proper
> + * cred for tasks running in container.
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int lxcContainerSetUserns(virDomainDefPtr def)
> +{
> + if (def->os.userns != VIR_DOMAIN_USER_NS_ENABLED)
> + return 0;
> +
> + if (virSetUIDGID(def->os.uidmap.first,
> + def->os.gidmap.first) < 0) {
> + virReportSystemError(errno, "%s",
> + _("setuid or setgid failed"));
> + return -1;
> + }
> +
> + return 0;
> +}
I don't see why we should force the init process to have the first
UID in the map. The init process should always start as uid:gid 0:0
regardless of mapping IMHO. If we want a capability to start the
init process as a different uid:gid, then that should involve separate
XML configuration.
> @@ -2221,6 +2244,24 @@ static int lxcContainerChild(void *data)
> }
> }
>
> + if (!virFileExists(vmDef->os.init)) {
> + virReportSystemError(errno,
> + _("cannot find init path '%s' relative to container root"),
> + vmDef->os.init);
> + goto cleanup;
> + }
> +
> + /* Wait for interface devices to show up */
> + if (lxcContainerWaitForContinue(argv->monitor) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Failed to read the container continue message"));
> + goto cleanup;
> + }
> + VIR_DEBUG("Received container continue message");
I don't really see why you're moving these lines - they are unrelated
to user namespaces.
> +
> + if (lxcContainerSetUserns(vmDef) < 0)
> + goto cleanup;
> +
> VIR_DEBUG("Container TTY path: %s", ttyPath);
>
> ttyfd = open(ttyPath, O_RDWR|O_NOCTTY);
> @@ -2236,20 +2277,6 @@ static int lxcContainerChild(void *data)
> argv->securityDriver) < 0)
> goto cleanup;
>
> - if (!virFileExists(vmDef->os.init)) {
> - virReportSystemError(errno,
> - _("cannot find init path '%s' relative to container root"),
> - vmDef->os.init);
> - goto cleanup;
> - }
> -
> - /* Wait for interface devices to show up */
> - if (lxcContainerWaitForContinue(argv->monitor) < 0) {
> - virReportSystemError(errno, "%s",
> - _("Failed to read the container continue message"));
> - goto cleanup;
> - }
> - VIR_DEBUG("Received container continue message");
>
> /* rename and enable interfaces */
> if (lxcContainerRenameAndEnableInterfaces(!!(vmDef->features &
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 15aa334..f17142f 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -1028,6 +1028,69 @@ cleanup2:
> }
>
>
> +/**
> + * virLXCControllerSetupUserns
> + *
> + * Set proc files for user namespace
> + *
> + * Returns 0 on success or -1 in case of error
> + */
> +static int virLXCControllerSetupUserns(virLXCControllerPtr ctrl)
> +{
> + char *uid_map = NULL;
> + char *gid_map = NULL;
> + char *uidmap_value = NULL;
> + char *gidmap_value = NULL;
> + int ret = -1;
> +
> + if (ctrl->def->os.userns != VIR_DOMAIN_USER_NS_ENABLED)
> + return 0;
> +
> + if (virAsprintf(&uid_map, "/proc/%d/uid_map", ctrl->initpid) < 0)
> + goto cleanup;
> +
> + if (virAsprintf(&gid_map, "/proc/%d/gid_map", ctrl->initpid) < 0)
> + goto cleanup;
> +
> + if (virAsprintf(&uidmap_value, "%u %u %u",
> + ctrl->def->os.uidmap.first,
> + ctrl->def->os.uidmap.low_first,
> + ctrl->def->os.uidmap.count) < 0)
> + goto cleanup;
> +
> + if (virAsprintf(&gidmap_value, "%u %u %u",
> + ctrl->def->os.gidmap.first,
> + ctrl->def->os.gidmap.low_first,
> + ctrl->def->os.gidmap.count) < 0)
> + goto cleanup;
> +
> + if (virFileWriteStr(uid_map, uidmap_value, 0) < 0) {
> + if (errno == -ENOENT)
> + virReportSystemError(errno,
> + _("%s doesn't exist, please disable userns"),
> + uid_map);
> + virReportSystemError(errno, _("unable write to %s"), uid_map);
> + goto cleanup;
> + }
> +
> + if (virFileWriteStr(gid_map, gidmap_value, 0) < 0) {
> + if (errno == -ENOENT)
> + virReportSystemError(errno,
> + _("%s doesn't exist, please disable userns"),
> + gid_map);
> + virReportSystemError(errno, _("unable write to %s"), gid_map);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +cleanup:
> + VIR_FREE(uidmap_value);
> + VIR_FREE(gidmap_value);
> + VIR_FREE(uid_map);
> + VIR_FREE(gid_map);
> + return ret;
> +}
I think we should have one method can write to either mapping file,
and that should be invoked once to setup UID mapping and once to
setup GID mapping. This would remove all the duplication in this
method. Also we need to be able to write multiple lines to the
mapping files in one go.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list