On 07/20/2013 05:46 AM, dwalsh@redhat.com wrote:

From: Dan Walsh <dwalsh@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. ---

I'm afraid this needs another version, and thus I'm reluctant to take it into 1.1.1 if we can't get it fixed quickly.

+++ b/mingw-libvirt.spec.in

@@ -246,9 +249,11 @@ rm -rf $RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh %files -n mingw64-libvirt %dir %{mingw64_sysconfdir}/libvirt/ %config(noreplace) %{mingw64_sysconfdir}/libvirt/libvirt.conf +%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf

This probably needs to be %{mingw64_sysconfdir}. For that matter, does virt-login-shell even make sense on mingw, or is it a Linux-only concept where the mingw .spec file should be ensuring that it is not compiled nor installed?

%{mingw64_bindir}/libvirt-0.dll %{mingw64_bindir}/virsh.exe +%{mingw64_bindir}/virt-login-shell.exe

In other words, is virt-login-shell.exe even capable of doing anything useful on mingw?

+++ b/src/libvirt_private.syms @@ -2026,6 +2026,7 @@ virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; virGetUserDirectory; +virGetUserDirectoryByUID;

This looks like an independently useful function; it would be worth splitting this patch into a series where patch 1 adds the new function, and patch 2 takes advantage of it, rather than mixing it all into one patch.

+char *virGetUserDirectoryByUID(uid_t uid) +{ + char *ret; + virGetUserEnt(uid, NULL, NULL, &ret); + return ret; +}

Is it worth rewriting the existing: char *virGetUserDirectory(void) { return virGetUserDirectoryByUID(geteuid()); } Doing so would give us instant code coverage of the new function.

bin_SCRIPTS = virt-xml-validate virt-pki-validate -bin_PROGRAMS = virsh virt-host-validate +bin_PROGRAMS = virsh virt-host-validate virt-login-shell

Since the .spec file is installing this file as setuid, shouldn't 'make install' attempt to do so likewise (that is, do more than just the normal reliance on automake's bin_PROGRAMS, which doesn't set special mode bits)? Or is setuid something rare enough that only spec files need to be requesting it? I'm thinking about the case of someone that does './configure --prefix=$HOME' with the intent of installing local tools - will virt-login-shell be usable by such a user?

+++ b/tools/virt-login-shell.c @@ -0,0 +1,312 @@ +/* + * virt-login-shell.c: a shell to connect to a container + *

+#include <config.h> +#include "virconf.h" +#include "virutil.h" +#include "virfile.h" +#include "virprocess.h" +#include "configmake.h" +#include "virstring.h" +#include "viralloc.h" +#include "vircommand.h" + +#include <stdarg.h> +#include <stdio.h> +#include <errno.h> +#include <stdlib.h> +#include <fnmatch.h>

We tend to list system headers before "" headers; but it shouldn't be a problem that you did it the other way around.

+#define VIR_FROM_THIS VIR_FROM_NONE + +static ssize_t nfdlist = 0; +static int *fdlist = NULL;

static variables are already 0-initialized without needing an explicit initializer; while gcc is smart enough to optimize explicit zero optimization into using the bss, not all compilers are that good.

+ +static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) {

unnecessary doubled space Style nit: we prefer: static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) {

+ size_t i;

Style nit: we prefer blank line between local variable declarations and expressions.

+ for (i=0; i < nfdlist; i++)

Style nit: we prefer spaces around '='.

+ VIR_FORCE_CLOSE(fdlist[i]); + VIR_FREE(fdlist);

This looks like leak prevention (good so that tools like valgrind don't complain)...

+ nfdlist = 0;

...whereas this may be dead code - if we are not supposed to use anything after virLoginShellFini() has been called (because we are about to exit or exec), then cleaning things back to a zero-initialized state is wasted cycles. On the other hand, being defensive against reuse can't hurt, and as this will be setuid, paranoia is okay.

+ if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn); +} + +static int virLoginShellAllowedUser(virConfPtr conf, + uid_t uid, + gid_t gid, + char *name) {

Do you modify 'name'? If not, add 'const'. Style nit: 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) { + errno = EPERM; + fprintf(stderr, _("%s is not allowed to connect a container: %m\n"), name);

%m is not POSIX; it is not safe to use here. Also, why do you need to use fprintf instead of more typical virReportSystemError(), which already provides strerror() as part of the logged message?

+ goto cleanup; + } + if (p && p->type == VIR_CONF_LIST) { + size_t len; + virConfValuePtr pp; + + /* Calc length and check items */ + for (len = 0, pp = p->list; pp; len++, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + fprintf(stderr, _("shell must be a list of strings\n")); + goto cleanup; + } else { + if (fnmatch(pp->str, name, 0) == 0) { + ret = 0; + goto cleanup; + } + if (pp->str[0] == '%') { + ptr=&pp->str[1];

Style nit: space around '='

+ if (!ptr) + continue;

So if the user passes "%" as one of their strings in the list, we just ignore it - but only after checking it against fnmatch? I'm not following why % is allowed, or what it does. [\me reads the .conf file] Oh, %engineers is a way to match all users within the 'engineers' group. But if there is a %, then I think you DON'T want to fnmatch() right away.

+ if (virGetGroupList(uid, gid, &groups) < 0) { + fprintf(stderr, _("Unable to get group list for %s: %m\n"), name);

%m is unacceptable. virGetGroupList() already reported its own error message with virReportError, so why are you duplicating the error message here with fprintf? Shouldn't we instead be setting up error logging to use virError set to stderr, to take advantage of all the helper routines we call that already log their own errors?

+ goto cleanup; + } + for (i=0; groups[i]; i++) {

space around '='

+ if (!(gname = virGetGroupName(groups[i]))) + continue; + if (fnmatch(ptr, gname, 0) == 0) { + ret = 0; + goto cleanup; + } + VIR_FREE(gname); gname=NULL;

'gname=NULL;' is spurious; VIR_FREE(gname) already sets gname to NULL.

+ } + } + } + }

Memory leak. You have an outer loop that can potentially iterate over multiple user tokens, and a call to virGetGroupList that allocates groups once per user token starting with %, but nothing that frees groups between iterations.

+ } + errno = EPERM;

You set errno...

+ fprintf(stderr, _("%s is not allowed to connect a container: %m\n"), name);

...then promptly nuke it with an fprintf(). If you want errno to be in a known state after this function returns failure, then you must set it after anything else that might alter errno.

+cleanup: + VIR_FREE(gname); + VIR_FREE(groups); + return ret; +} + +static char **virLoginShellGetShellArgv(virConfPtr conf) {

static char ** virLoginShellGetShellArgv(virConfPtr conf) {

+ size_t i; + char **shargv; + virConfValuePtr p; + p = virConfGetValue(conf, "shell"); + if (!p) + return virStringSplit("/bin/sh --login", " ", 3);

Use the POSIX spelling s/--login/-l/. After all, on debian, people install dash as /bin/sh, but dash doesn't like long options. Just because /bin/sh is bash on Fedora doesn't mean it is safe to abuse that fact.

+ + if (p && p->type == VIR_CONF_LIST) { + size_t len; + virConfValuePtr pp; + + /* Calc length and check items */ + for (len = 0, pp = p->list; pp; len++, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + fprintf(stderr, _("shell must be a list of strings\n")); + goto error; + } + }

I'm wondering if we should have a helper function that makes it easier to parse a list of strings out of a conf file, instead of having to open-code our own iterator (this is the second time you've done a string-list iteration in this patch alone).

+ + if (VIR_ALLOC_N(shargv, len + 1) < 0) + goto error; + for (i = 0, pp = p->list; pp; i++, pp = pp->next) { + if (VIR_STRDUP(shargv[i], pp->str) < 0) + goto error; + } + shargv[len] = NULL; + } + return shargv; +error: + virStringFreeList(shargv); + return NULL; +} + +int +main(int argc, char **argv) {

main(int argc, char **argv) {

+ virConfPtr conf = NULL; + const char *login_shell_path = SYSCONFDIR "/libvirt/virt-login-shell.conf"; + pid_t cpid; + int ret = EXIT_FAILURE; + int status; + int status2; + uid_t uid = getuid(); + gid_t gid = getgid();

Maybe I'm asking a stupid question, but why getuid() instead of geteuid()? Ditto for getegid().

+ char *name; + char **shargv = NULL; + virSecurityModelPtr secmodel = NULL; + virSecurityLabelPtr seclabel = NULL; + virDomainPtr dom = NULL; + virConnectPtr conn = NULL; + char *homedir = NULL; + if (!setlocale(LC_ALL, "")) { + perror("setlocale"); + /* failure to setup locale is not fatal */ + } + if (!bindtextdomain(PACKAGE, LOCALEDIR)) { + perror("bindtextdomain"); + return ret; + } + if (!textdomain(PACKAGE)) { + perror("textdomain"); + return ret; + } + + if (uid == 0) { + errno = EPERM; + fprintf(stderr, _("%s must be run by non root users: %m\n"), argv[0]);

%m is bad.

+ goto cleanup; + }

By placing this here, we give this failure message even for innocent usage like 'virt-login-shell --help' done as root. Would it make more sense to defer user enforcement until after option parsing, so that even root can at least probe the version of this software instead of being blindly rejected?

+ + if (argc > 1) { + errno = EINVAL; + fprintf(stderr, _("%s takes no options: %m\n"), argv[0]);

%m is bad.

+ goto cleanup; + }

Evil. All good programs should support --help and --program at a minimum - especially setuid programs in order to explain why they are setuid.

+ + name = virGetUserName(uid); + if (!name) { + fprintf(stderr, _("Failed to get username for %d: %m\n"), uid);

%m - and back to my question about why you aren't initializing the existing virError framework to use that to print to stderr instead of repeating it yourself. Remember, virGetUserName() does NOT guarantee a sane errno value on failure.

+ goto cleanup; + } + homedir = virGetUserDirectoryByUID(uid); + if (!homedir) { + fprintf(stderr, _("Can't read %s home directory: %m\n"), name);

%m, and virGetUserDirectoryByUID() does NOT guarantee a sane errno value on failure.

+ goto cleanup; + } + + if (!(conf = virConfReadFile(login_shell_path, 0))) { + fprintf(stderr, _("Failed to read %s: %m\n"), login_shell_path);

%m

+ goto cleanup; + } + if (virLoginShellAllowedUser(conf, uid, gid, name) < 0) + goto cleanup; + + if (!(shargv = virLoginShellGetShellArgv(conf))) + goto cleanup; + + conn = virConnectOpen("lxc:///");

This hard-codes us to LXC. We should allow a -c option, similar to other tools in the virt-stack, to let the user specify an alternate connection name. virConnectOpen(NULL) seems more future-proof, in case we ever change our default container hypervisor away from lxc:/// over to some other URI.

+ if (!conn) { + fprintf(stderr, _("Unable to connect to lxc:///: %m\n"));

%m

+ goto cleanup; + } + + dom = virDomainLookupByName(conn, name); + if (!dom) { + fprintf(stderr, _("Container %s does not exist: %m\n"), name);

%m

+ goto cleanup; + } + + if (! virDomainIsActive(dom) && virDomainCreate(dom)) {

Style nit: no space after '!'

+ virErrorPtr last_error; + last_error = virGetLastError(); + if (last_error->code != VIR_ERR_OPERATION_INVALID) { + fprintf(stderr,_("Can't create %s container: %s\n"), name, virGetLastErrorMessage());

Long line; wrap to stay within 80 columns.

+ goto cleanup; + } + } + + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) {

Ah, we ARE hard-coding ourselves to lxc, by relying on an API that can only be successful when talking to lxc.

+ fprintf(stderr,_("Can't open %s namespace: %m\n"), name);

%m

+ goto cleanup; + } + + if (VIR_ALLOC(secmodel) < 0) + goto cleanup; + if (VIR_ALLOC(seclabel) < 0) + goto cleanup; + if (virNodeGetSecurityModel(conn, secmodel) < 0) + goto cleanup; + if (virDomainGetSecurityLabel(dom, seclabel) < 0) + goto cleanup; + + 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, 0, 0) < 0) {

virSetUIDGID(0, 0, NULL, 0) - I don't like the use of '0' when NULL is intended.

+ fprintf(stderr, _("Unable to setresuid: %m\n"));

%m - and you are neither guaranteed that errno is reliable on failure, nor that the failure of virSetUIDGID was caused by setresuid.

+ return EXIT_FAILURE; + } + + if (virDomainLxcEnterSecurityLabel(secmodel, + seclabel, + NULL, + 0) < 0) + return EXIT_FAILURE;

What, no error message on why you exited? Again, I think you need to wire up virError handling to report to stderr by default.

+ + 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; + + ret = virSetUIDGIDWithCaps(uid, gid, groups, ngroups, 0, 0);

Since you aren't setting caps, why not use the simpler virSetUIDGID()?

+ VIR_FREE(groups); + if (ret < 0) + return EXIT_FAILURE; + + if (virFork(&ccpid) < 0) + return EXIT_FAILURE; + + if (ccpid == 0) { + if (chdir(homedir) < 0) { + fprintf(stderr, _("Unable chdir(%s): %m\n"), homedir);

%m

+ return EXIT_FAILURE; + } + if (execv(shargv[0], (char *const*) shargv) < 0) { + fprintf(stderr, _("Unable exec shell %s: %m\n"), shargv[0]);

%m

+ return EXIT_FAILURE;

Generally, you want to exit with status 126 or 127 after execv() failure, not 1; although I'm not sure it matters much here.

+ } + } + return virProcessWait(ccpid, &status2); + } + ret = virProcessWait(cpid, &status); + +cleanup: + virConfFree(conf); + virLoginShellFini(conn, dom); + virStringFreeList(shargv); + VIR_FREE(name); + VIR_FREE(homedir); + VIR_FREE(seclabel); + VIR_FREE(secmodel); + return ret; +} diff --git a/tools/virt-login-shell.conf b/tools/virt-login-shell.conf new file mode 100755 index 0000000..107424a --- /dev/null +++ b/tools/virt-login-shell.conf @@ -0,0 +1,24 @@ +# Master configuration file for the virt-login-shell program. +# All settings described here are optional - if omitted, sensible +# defaults are used. + +# By default, virt-login-shell will connect you to a container running +# with the /bin/sh program. Modify the shell variable if you want your +# users to run a different shell or a setup containe when joining a

s/containe/container/

+# container. Shell commands must be a list of commands/options separated by +# comma and delimited by square brackets. Defaults to: /bin/sh --login.

s/--login/-l/

+# Modify and uncomment the following to modify the login shell. +# shell = [ "/bin/sh", "--login" ]

s/--login/-l/

+ +# allowed_users specifies the user name of all users that are allowed to execute +# virt-login-shell. You can specify the users as a comma separated list of usernames. +# The variable support glob syntaxt, if you specify no names

s/syntaxt/syntax/

+# (default) then no one except root is allowed to use this executable.

Umm, but you just coded it so that the executable always fails when run as root.

+# to allow fred and joe only +# allowed_users = ["fred", "joe"] +# To allow all users within a specific group prefix the group name with %, etc +# allowed_users = ["%engineers"] +# to allow all users specify the following +# allowed_users = [ "*" ] +# disallow all users +# allowed_users = [] diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod new file mode 100644 index 0000000..0cd35cf --- /dev/null +++ b/tools/virt-login-shell.pod @@ -0,0 +1,62 @@ +=head1 NAME + +virt-login-shell - tool to execute a shell within a container matching the users name + +=head1 SYNOPSIS + +B<virt-login-shell> + +=head1 DESCRIPTION + +The B<virt-login-shell> program is setuid shell that is used to join +an LXC container that matches the users name. If the container is not +running virt-login-shell will attempt to start the container. +virt-sandbox-shell is not allowed to be run by root. Normal users will get +added to a container that matches their username, if it exists. And they are

s/exists. And they are/exists, and as/

+configured in /etc/libvirt/virt-login-shell.conf. + +The basic structure of most virt-login-shell usage is: + + virt-login-shell + +=head1 CONFIG + +By default, virt-login-shell will execute the /bin/sh program for the user. +You can modify this behaviour by defining the shell variable in /etc/libvirt/virt-login-shell.conf. + +eg. shell = [ "/bin/ksh", "--login"] + +By default no users are allowed to user virt-login-shell, if you want to allow +certain users to use virt-login-shell, you need to modify the allowed_users variable in /etc/libvirt/virt-login-shell.conf.

Long line; wrap to keep in 80 columns.

+ +eg. allowed_users = [ "tom", "dick", "harry" ] + +=head1 BUGS + +Report any bugs discovered to the libvirt community via the mailing +list C<http://libvirt.org/contact.html> or bug tracker C<http://libvirt.org/bugs.html>. +Alternatively report bugs to your software distributor / vendor. + +=head1 AUTHORS + + Please refer to the AUTHORS file distributed with libvirt. + + Daniel Walsh <dwalsh at redhat dot com> + +=head1 COPYRIGHT + +Copyright (C) 2013 Red Hat, Inc., and the authors listed in the +libvirt AUTHORS file. + +=head1 LICENSE + +virt-login-shell is distributed under the terms of the GNU LGPL v2+. +This is free software; see the source for copying conditions. There +is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR +PURPOSE + +=head1 SEE ALSO + +L<virsh(1)>, L<http://www.libvirt.org/> + +=cut

-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org