On Thu, Jul 18, 2013 at 12:18:31PM -0400, dwalsh@xxxxxxxxxx wrote:
> From: Dan Walsh <dwalsh@xxxxxxxxxx>
>
> 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.
> As root you should be able to join arbitrary containers, but when this tool is
> run by a normal user it will only join the "users" container.
> ---
> .gitignore | 1 +
> po/POTFILES.in | 1 +
> tools/Makefile.am | 34 ++++-
> tools/login_shell.conf | 12 ++
Lets call this virt-login-shell.conf too
> tools/virt-login-shell.c | 340 +++++++++++++++++++++++++++++++++++++++++++++
> tools/virt-login-shell.pod | 53 +++++++
> 6 files changed, 440 insertions(+), 1 deletion(-)
> create mode 100755 tools/login_shell.conf
> create mode 100644 tools/virt-login-shell.c
> create mode 100644 tools/virt-login-shell.pod
libvirt.spec.in and mingw-libvirt.spec.in will need updates
to list this new binary + man page + config file.
> +virt_login_shell_SOURCES = \
> + login-shell.conf \
> + virt-login-shell.c
> +
> +virt_login_shell_LDFLAGS = $(COVERAGE_LDFLAGS)
> +virt_login_shell_LDADD = \
> + $(STATIC_BINARIES) \
> + $(PIE_LDFLAGS) \
> + $(RELRO_LDFLAGS) \
> + ../src/libvirt.la \
> + ../src/libvirt-lxc.la \
> + ../src/libvirt-qemu.la \
Strictly speaking there's no need to link to libvirt-qemu.la
> diff --git a/tools/login_shell.conf b/tools/login_shell.conf
> new file mode 100755
> index 0000000..23fee1a
> --- /dev/null
> +++ b/tools/login_shell.conf
> @@ -0,0 +1,12 @@
> +# 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
> +# container. Shell commands must be a list of commands/options separated by
> +# comma and delimited by square brackets. For example:
> +# shell = [ "/bin/sh", "--login" ]
> +#
> +shell=[ "/bin/sh", "--login" ]
Normally we leave defaults fully commented out, so the default
is present in the code.
Hmm, so if this shell is going to be install setuid, then we ought
to probably ensure that in a default install, it is completely
disabled.
As it stands, if someone happens to create a container whose
name happens to match that of a local user account, the default
out of the box config would allow the shell to run.
With libvirtd auth, we have a config option which is a whitelist
of allowed users, with glob matching. We should probably do the
same with this.
Have a line:
# allow_users = [...list of globs..]
so eg to allow fred & joe only
allow_users = ["fred", "joe"]
To allow any user named with 'dan', 'daniel', 'danielle', etc
allow_users = ["dan*"]
To allow any user at all
allow_users = ["*"]
disallow all users
allow_users = []
and make sure that if allow_users is not set, then it is
equivalent to [] - ie forbid all. This way we are secure
by default.
> +#include <config.h>
> +#include "conf/domain_conf.h"
I don't see any code which requires this header.
> +#include "virkeyfile.h"
> +#include "virconf.h"
You don't need both of these. virkeyfile is .ini style files,
while virconf is .py style files. From the example, it looks
like you chose .py style config files. So remove virkeyfile.h
> +#include "virfile.h"
> +
> +#include <stdarg.h>
Move this to be with the other system headers
> +#include <libvirt/libvirt.h>
No need for this, since it is pulled in recursively.
> +#define VIR_FROM_THIS VIR_FROM_NONE
This #define should be after end of all includes
> +#include "configmake.h"
> +#include "internal.h"
> +#include "virerror.h"
> +#include "virstring.h"
> +#include "viralloc.h"
> +#include "vircommand.h"
> +
> +#include <stdio.h>
> +#include <errno.h>
> +#include <pwd.h>
> +#include <stdlib.h>
> +#include <selinux/selinux.h>
> +
> +#include <sys/wait.h>
> +#include <cap-ng.h>
> +
> +static int nfdlist = 0;
> +static int *fdlist = NULL;
> +
> +static void fini(virConnectPtr conn, virDomainPtr dom) {
As a naming convention can you use
virLoginShellXXXX
for all methods in this file, with the exception of 'main' of
course.
> + int i;
s/int/size_t/
make syntax-check should flag this & similar problems
> + if (nfdlist > 0) {
> + for (i=0; i < nfdlist; i++)
> + VIR_FORCE_CLOSE(fdlist[i]);
> + VIR_FREE(fdlist);
> + nfdlist = 0;
> + }
> + if (conn)
> + virConnectClose(conn);
> + if (dom)
> + virDomainFree(dom);
Nit-pick, reverse the order, so you free the domain before
the connection
> +}
> +
> +static capng_select_t cap_set = CAPNG_SELECT_CAPS;
> +
> +/**
> + * This function will drop all capabilities.
> + */
> +static int drop_caps(struct passwd *pw)
> +{
> +
> + if (capng_have_capabilities(cap_set) == CAPNG_NONE)
> + return 0;
> + capng_setpid(getpid());
> + capng_clear(cap_set);
> + if (virSetUIDGID(pw->pw_uid, pw->pw_gid) < 0)
> + return -1;
> + return capng_apply(CAPNG_SELECT_CAPS);
> +}
I think you can just just this entire method and then
just update the caller to use virSetUIDGIDWithCaps()
> +static int reset_environment(struct passwd *pw)
Method naming convention as ealier
> +{
> + char *lang_env = getenv("LANG");
> + char *lang=NULL;
Include whitespace around '='
> + int ret = 0;
Convention is to initialize to '-1' and only set to
0, once everything has succeeeded.
> +
> + if (lang_env) {
> + if (!VIR_STRDUP(lang_env, lang)) {
THe args are the wrong way around - you're replacing
'lang_env' with a dup of 'lang' here.
> + virReportOOMError();
No need for this anymore - we now report that automatically
> + return -1;
> + }
> + }
> +
> + if (clearenv() < 0) {
> + fprintf(stderr, _("Unable to clear environment: %m"));
> + goto out;
> + }
> +
> + ret |= setenv("HOME", pw->pw_dir, 1);
> + ret |= setenv("USER", pw->pw_name, 1);
> + ret |= setenv("PATH", ":/bin:/usr/bin", 1);
> + if (lang) {
> + ret |= setenv("LANG", lang, 1);
Change all these to
if (setenv(...) < 0)
goto cleanup;
> + VIR_FREE(lang);
> + }
Add
ret = 0;
> +out:
s/out/cleanup/ for our naming conventions
> + VIR_FREE(lang);
> + if (ret) {
s/ret/ret < 0/
> + fprintf(stderr, _("Unable to set environment: %m"));
> + return ret;
No need for 'return ret' here, since the next line does that
already
> + }
> + return ret;
> +}
> +
> +static char **get_shell_argv(void) {
Method naming convention
> + int i;
> + char** shargv;
Use a space before the '**' rather than after it.
> + virConfPtr conf = NULL;
> + virConfValuePtr p;
> + const char *login_shell_path = SYSCONFDIR "/libvirt/login_shell.conf";
> + if (!(conf = virConfReadFile(login_shell_path, 0))) {
> + fprintf(stderr, _("Failed to read %s: %m\n"), login_shell_path);
> + return NULL;
> + }
> + p = virConfGetValue(conf, "shell");
> + if (!p) {
> + fprintf(stderr, _("Failed to read shell from %s: %m\n"), login_shell_path);
> + goto err;
> + }
> + 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"));
> + goto err;
> + }
> + }
> +
> + if (VIR_ALLOC_N(shargv, len + 1) < 0) {
> + fprintf(stderr, _("out of memory"));
> + goto err;
> + }
> + for (i = 0, pp = p->list; pp; i++, pp = pp->next) {
> + if (VIR_STRDUP(shargv[i], pp->str) < 0) {
> + fprintf(stderr, _("out of memory"));
> + goto err;
> + }
> + }
> + shargv[len] = NULL;
> + }
> + goto cleanup;
> +err:
> + for (i = 0; shargv[i]; i++)
> + VIR_FREE(shargv[i]);
> + VIR_FREE(shargv);
> + shargv=NULL;
> +cleanup:
Merge the err & cleanup labels into just one 'cleanup' label
and make the for() condition be 'shargv && shargv[i]' to make
it rebust.
> + virConfFree(conf);
> + return shargv;
> +}
> +
> +int
> +main(int argc, char **argv) {
> + int i;
s/int/size_t/
> + struct passwd *pw = NULL;
> + struct passwd pwbuf;
> + pid_t cpid;
> + pid_t ccpid;
> + long val = sysconf(_SC_GETPW_R_SIZE_MAX);
> + size_t strbuflen = val;
> + char *strbuf = NULL;
> + int ret = EXIT_FAILURE;
> + int status;
> + int status2;
> + uid_t uid = getuid();
> + int optOffset = 1;
> + char** shargv;
Space before the '**', rather than after it.
> + virSecurityModelPtr secmodel = NULL;
> + virSecurityLabelPtr seclabel = NULL;
> + virDomainPtr dom = NULL;
Too many spaces before var name
> + virConnectPtr conn = NULL;
> + if (!setlocale(LC_ALL, "")) {
> + perror("setlocale");
> + /* failure to setup locale is not fatal */
> + }
> + if (!bindtextdomain(PACKAGE, LOCALEDIR)) {
> + perror("bindtextdomain");
> + return EXIT_FAILURE;
> + }
> + if (!textdomain(PACKAGE)) {
> + perror("textdomain");
> + return EXIT_FAILURE;
> + }
> +
> + if (!(shargv = get_shell_argv()))
> + return EXIT_FAILURE;
> +
> + /* sysconf is a hint; if it fails, fall back to a reasonable size */
> + if (val < 0)
> + strbuflen = 1024;
> +
> + if (VIR_ALLOC_N(strbuf, strbuflen) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if (argc > 1) {
> + if (uid != 0) {
> + fprintf(stderr, _("Root is required to select a container to join: %m\n"));
> + return EXIT_FAILURE;
> + }
> +
> + while (getpwnam_r(argv[1], &pwbuf, strbuf, strbuflen, &pw) == ERANGE) {
> + if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> +
> + optOffset++;
> + } else {
> + while ((getpwuid_r(uid, &pwbuf, strbuf, strbuflen,
> + &pw)) == ERANGE) {
> + if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> + }
> + if (!pw) {
> + fprintf(stderr, _("Passwd record does not exist: %m\n"));
> + return EXIT_FAILURE;
> + }
Don't call getpwnam/uid at all, since they are very hard to
call & handle errors correctly with. Use the virGetUserName and
virGetUserID functions instead.
> + if ((uid == 0) && (reset_environment(pw)))
> + return EXIT_FAILURE;
> +
> + conn = virConnectOpen("lxc:///");
> + if (!conn) {
> + fprintf(stderr, _("Unable to connect to lxc:///: %m\n"));
> + return EXIT_FAILURE;
> + }
> +
> + dom = virDomainLookupByName(conn, pw->pw_name);
> + if (!dom) {
> + fprintf(stderr, _("Container %s does not exist: %m\n"), pw->pw_name);
> + return EXIT_FAILURE;
> + }
Best practice is to have a cleanup: block for the main() function
where you free all memory/resources, even on error conditions. This
lets us run virt-login-shell under valgrind and validate that it
doesn't leak.
> +
> + if (! virDomainIsActive(dom) && virDomainCreate(dom)) {
> + virErrorPtr last_error;
> + last_error = virGetLastError();
> + if (last_error->code != VIR_ERR_OPERATION_INVALID) {
> + fprintf(stderr,_("Can't create %s container: %s\n"), pw->pw_name, virGetLastErrorMessage());
> + return EXIT_FAILURE;
> + }
> + }
> +
> + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) {
> + fprintf(stderr,_("Can't open %s namespace: %m\n"), pw->pw_name);
> + goto err;
> + }
> +
> + if (VIR_ALLOC(secmodel) < 0) {
> + fprintf(stderr, _("Failed to allocate security model: %m\n"));
> + goto err;
> + }
> + if (VIR_ALLOC(seclabel) < 0) {
> + fprintf(stderr, _("Failed to allocate security label: %m\n"));
> + goto err;
> + }
> + if (virNodeGetSecurityModel(conn, secmodel) < 0)
> + goto err;
> + if (virDomainGetSecurityLabel(dom, seclabel) < 0)
> + goto err;
> +
> + if (virFork(&cpid) < 0)
> + goto err;
> +
> + if (cpid == 0) {
> + /* Fork once because we don't want to affect
> + * virt-login-shell's namespace itself
> + */
> + if (virSetUIDGID(0, 0) < 0) {
> + fprintf(stderr, _("Unable to setresuid: %m\n"));
> + 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;
> + }
> +
> + drop_caps(pw);
> +
> + if (virFork(&ccpid) < 0)
> + return EXIT_FAILURE;
> +
> + if (ccpid == 0) {
> + if (chdir(pw->pw_dir) < 0) {
> + fprintf(stderr, _("Unable chdir(%s): %m\n"), pw->pw_dir);
> + return EXIT_FAILURE;
> + }
> + if (execv(shargv[0], (char *const*) shargv) < 0) {
> + fprintf(stderr, _("Unable exec shell %s: %m\n"), shargv[0]);
> + return EXIT_FAILURE;
> + }
> + }
> + wait(&status2);
> + return EXIT_SUCCESS;
> + }
> + wait(&status);
Use virProcessWait() instead of wait(), particularly since you're
not dealing with errors at all.
> + ret = EXIT_SUCCESS;
> + goto cleanup;
> +err:
> + ret = EXIT_FAILURE;
> +cleanup:
> + fini(conn, dom);
> + for (i = 0; shargv[i]; i++)
> + VIR_FREE(shargv[i]);
> + VIR_FREE(shargv);
> + VIR_FREE(strbuf);
> + VIR_FREE(seclabel);
> + VIR_FREE(secmodel);
> + return ret;
> +}
> diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod
> new file mode 100644
> index 0000000..f32eed0
> --- /dev/null
> +++ b/tools/virt-login-shell.pod
> @@ -0,0 +1,53 @@
> +=head1 NAME
> +
> +virt-login-shell - tool to execute a shell within a container matching the users name
> +
> +=head1 SYNOPSIS
> +
> +B<virt-login-shell>
> +
> +B<virt-login-shell> USERNAME
> +
> +=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. If
> +virt-sandbox-shell is run by root, you can specify an alternate username
> +to join the container. Normal users will get added to a container that matches their username, if it exists.
> +
> +The basic structure of most virsh usage is:
> +
> + virt-login-shell
> +
> +By default, virt-login-shell will execute the /bin/sh program for the user. Modify the /etc/libvirt/login_shell.conf file to change the default shell.
> +
Ought to have a section "=head1 CONFIG" where we descfribe the config
file settings really.
> +=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
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list