On 08/02/2013 09:22 AM, Daniel J Walsh wrote:
> Subject: [PATCH] virt-login-shell joins users into lxc container.
>
> 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.
> ---
> .gitignore | 1 +
> libvirt.spec.in | 3 +
> po/POTFILES.in | 1 +
> tools/Makefile.am | 30 +++-
> tools/virt-login-shell.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
> tools/virt-login-shell.conf | 26 ++++
> tools/virt-login-shell.pod | 62 ++++++++
> 7 files changed, 472 insertions(+), 1 deletion(-)
> create mode 100644 tools/virt-login-shell.c
> create mode 100644 tools/virt-login-shell.conf
> create mode 100644 tools/virt-login-shell.pod
Already committed, but the following items are worth cleaning up in a
followup patch:
> @@ -128,6 +133,24 @@ virt_host_validate_CFLAGS = \
> $(COVERAGE_CFLAGS) \
> $(NULL)
>
> +virt_login_shell_SOURCES = \
> + virt-login-shell.conf \
.conf files should not be part of _SOURCES; it is already correctly
listed under conf_DATA, so I think you can just delete this line.
> +
> +static ssize_t nfdlist = 0;
> +static int *fdlist = NULL;
Static variables are automatically 0-initialized without needing
something explicit. gcc can optimize this into .bss, but not all
compilers do that.
> +static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf";
> +
> +static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom)
Unintentional 2 spaces.
> +static int virLoginShellAllowedUser(virConfPtr conf,
> + const char *name,
> + gid_t *groups)
> +{
> + /*
> + 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)
This conditional is dead code (ptr is always non-NULL at this point).
Did you mean to check if (!*ptr) instead? And if so, you should
probably warn that "%" is an invalid string, rather than silently
ignoring it.
> + }
> + virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"), name, conf_file);
Awkward grammar; I'd suggest s/as an/in/:
"%s not listed in allowed_users in %s"
> +
> +static char **virLoginShellGetShellArgv(virConfPtr conf)
> +{
> + size_t i;
> + char **shargv=NULL;
> + virConfValuePtr p;
> +
> + p = virConfGetValue(conf, "shell");
> + if (!p)
> + return virStringSplit("/bin/sh -l", " ", 3);
> +
> + if (p && p->type == VIR_CONF_LIST) {
Dead conditional on the left of &&; you only get here if p is non-NULL.
> +
> + 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;
Dead assignment; VIR_ALLOC_N guaranteed that shargv[len] starts life as
NULL.
> +static void
> +usage(void)
> +{
> + fprintf(stdout, _("\n"
> + "%s is a privileged program that allows non root users \n"
Outputting trailing whitespace to stdout is evil.
> + "specified in %s to join a Linux container \n"
> + "with a matching user name and launch a shell. \n"
Two more instances.
> +
> + struct option opt[] = {
> + {"help", no_argument, NULL, 'h'},
> + {NULL, 0, NULL, 0}
I still think all programs should have a --version option.
> +
> + /* The only option we support is help
> + */
> + while ((arg = getopt_long(argc, argv, "h", opt, &longindex)) != -1) {
> + switch (arg) {
> + case 'h':
> + usage();
> + exit(EXIT_SUCCESS);
> + break;
> + }
> + }
That may be so, but the way you used getopt_long, you will print a
message and then plow onwards anyways. It is better to have a case '?'
that calls usage(); exit(EXIT_FAILURE);, so that if we later add a new
option, and a user calls this program without knowing whether the
version they are using was built before or after the addition of the new
option, then trying the new option will gracefully fail instead of
printing a warning and then behaving as if the option had not been
specified...
> +
> + if (argc > optind) {
> + virReportSystemError(EINVAL, _("%s takes no options"), progname);
> + errno = EINVAL;
> + goto cleanup;
...especially since that's the behavior you chose for unknown non-options.
> +
> + if (!virDomainIsActive(dom) && virDomainCreate(dom)) {
> + virErrorPtr last_error;
> + last_error = virGetLastError();
> + if (last_error->code != VIR_ERR_OPERATION_INVALID) {
> + virReportSystemError(last_error->code,_("Can't create %s container: %s"), name, virGetLastErrorMessage());
Missing a space after ','.
Long line; please wrap to 80 columns.
> + 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;
This ends up being status 255; but it is more typical to use status 127
on execv() failure.
> +++ 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
s/setuid/a setuid/
> +an LXC container that matches the users name. If the container is not
s/users/user's/
> +running virt-login-shell will attempt to start the container.
s/running/running,/
> +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
> +configured in /etc/libvirt/virt-login-shell.conf.
grammar:
s/if it exists. And/if it exists, and if/
> +
> +By default no users are allowed to user virt-login-shell, if you want to allow
s/to user/to use/
> +certain users to use virt-login-shell, you need to modify the allowed_users variable in /etc/libvirt/virt-login-shell.conf.
Long line; please wrap to 80 columns. (Didn't I make some of these
comments against your earlier version? I'm not fond of having review
comments ignored, only to have to make them again)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list