[libvirt] [PATCH 2/2] virt-login-shell joins users into lxc container.

[Daniel P. Berrange]

On Tue, Jul 30, 2013 at 03:55:45PM -0400, dwalsh at redhat.com wrote:
> From: Dan Walsh <dwalsh at redhat.com>
> 
> Openshift wants to have their gears stuck into a container when they login
> to the system.  virt-login-shell will join a running gear with the username of
> the person running it, or attempt to start the container if it is not running.
> (Currently containers do not exist if they are not running, so I can not test
> this feature. But the code is there).
> 
> This tool needs to be setuid since joining a container (nsjoin) requires privs.
> The root user is not allowed to execute this command. When this tool is
> run by a normal user it will only join the "users" container.
> 
> Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
> are allowed to join containers using this tool. By default no users are allowed.
> ---
> +static ssize_t nfdlist = 0;
> +static int *fdlist = NULL;
> +static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf";
> +
> +static void virLoginShellFini(virConnectPtr conn, virDomainPtr  dom)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < nfdlist; i++)
> +	VIR_FORCE_CLOSE(fdlist[i]);
> +    VIR_FREE(fdlist);
> +    nfdlist = 0;
> +    if (dom)
> +	virDomainFree(dom);
> +    if (conn)
> +	virConnectClose(conn);

There are lots of tabs used for indent here and later in this
file. Please check with 'make syntax-check'.

> +static int virLoginShellAllowedUser(virConfPtr conf,
> +				    uid_t uid,
> +				    gid_t gid,
> +				    const char *name)
> +{
> +    virConfValuePtr p;
> +    int ret = -1;
> +    char *ptr = NULL;
> +    size_t i;
> +    gid_t *groups = NULL;
> +    char *gname = NULL;
> +
> +    p = virConfGetValue(conf, "allowed_users");
> +    if (p && p->type == VIR_CONF_LIST) {
> +	virConfValuePtr pp;
> +
> +	/* Calc length and check items */
> +	for (pp = p->list; pp; pp = pp->next) {
> +	    if (pp->type != VIR_CONF_STRING) {
> +		virReportSystemError(EINVAL, "%s", _("shell must be a list of strings\n"));
> +		goto cleanup;
> +	    } else {
> +		/*
> +		   If string begins with a % this indicates a linux group.
> +		   Check to see if the user is in the Linux Group.
> +		*/
> +		if (pp->str[0] == '%') {
> +		    ptr = &pp->str[1];
> +		    if (!ptr)
> +			continue;
> +		    if (virGetGroupList(uid, gid, &groups) < 0) {
> +			goto cleanup;
> +		    }
> +		    for (i = 0; groups[i]; i++) {
> +			if (!(gname = virGetGroupName(groups[i])))
> +			    continue;
> +			if (fnmatch(ptr, gname, 0) == 0) {
> +			    ret = 0;
> +			    goto cleanup;
> +			}
> +			VIR_FREE(gname);
> +		    }
> +		    VIR_FREE(groups);
> +		    continue;
> +		}
> +		if (fnmatch(pp->str, name, 0) == 0) {
> +		    ret = 0;
> +		    goto cleanup;
> +		}
> +	    }
> +	}
> +    }
> +    virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"), name, conf_file);
> +    errno = EPERM;

No need to set errno if you using virReportSystemError().

> +cleanup:
> +    VIR_FREE(gname);
> +    VIR_FREE(groups);
> +    return ret;
> +}
> +

> +int
> +main(int argc, char **argv)
> +{

> +
> +    struct option opt[] = {
> +	{"help", no_argument, NULL, 'h'},
> +	{NULL, 0, NULL, 0}
> +    };
> +    virReportSystemError(1, "%s %d %s", "Test1", argc, argv[0]);

Accidental debug line left in I presume

> +    if (virInitialize() < 0) {
> +	fprintf(stderr, _("Failed to initialize libvirt Error Handling"));
> +	return EXIT_FAILURE;
> +    }
> +    virReportSystemError(EINVAL, "%s", "Test2");

And here

> +
> +    if (virErrorInitialize() < 0) {
> +	fprintf(stderr, _("Failed to initialize libvirt Error Handling"));
> +	return EXIT_FAILURE;
> +    }

Actually no need to call virErrorInitialize() - virInitialize
takes care of that for you

> +    if (uid == 0) {
> +	virReportSystemError(EPERM, _("%s must be run by non root users"), progname);
> +
> +	errno = EPERM;

No need to set errno.


> +    if (virFork(&cpid) < 0)
> +	goto cleanup;
> +
> +    if (cpid == 0) {
> +	gid_t *groups = NULL;
> +	int ngroups;
> +	pid_t ccpid;
> +
> +	/* Fork once because we don't want to affect
> +	 * virt-login-shell's namespace itself
> +	 */
> +	if (virSetUIDGID(0, 0, NULL, 0) < 0)
> +	    return EXIT_FAILURE;
> +
> +	if (virDomainLxcEnterSecurityLabel(secmodel,
> +					   seclabel,
> +					   NULL,
> +					   0) < 0)
> +	    return EXIT_FAILURE;
> +
> +	if (nfdlist > 0) {
> +	    if (virDomainLxcEnterNamespace(dom,
> +					   nfdlist,
> +					   fdlist,
> +					   NULL,
> +					   NULL,
> +					   0) < 0)
> +		return EXIT_FAILURE;
> +	}
> +
> +	if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
> +	    return EXIT_FAILURE;

virGetGroupList isn't safe to call after fork(). Need to
move it earlier in this function.

> +
> +	ret = virSetUIDGID(uid, gid, groups, ngroups);
> +	VIR_FREE(groups);
> +	if (ret < 0)
> +	    return EXIT_FAILURE;
> +
> +	if (virFork(&ccpid) < 0)
> +	    return EXIT_FAILURE;
> +
> +	if (ccpid == 0) {
> +	    if (chdir(homedir) < 0) {
> +		virReportSystemError(errno, _("Unable chdir(%s)"), homedir);
> +		return EXIT_FAILURE;
> +	    }
> +	    if (execv(shargv[0], (char *const*) shargv) < 0) {
> +		virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]);
> +		return -errno;
> +	    }
> +	}
> +	return virProcessWait(ccpid, &status2);
> +    }
> +    ret = virProcessWait(cpid, &status);
> +

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 :|