[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virt-login-shell joins users into lxc container.



On Thu, Jul 18, 2013 at 05:25:56PM -0400, dwalsh redhat com wrote:
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index e0e0004..34e3594 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1751,6 +1751,7 @@ fi
>  %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd.qemu
>      %endif
>      %if %{with_lxc}
> +%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf

The Makefile.am for this is not conditional on WITH_LXC, so
you shoyuld have this outside the %{with_lxc} block.

>  %config(noreplace) %{_sysconfdir}/libvirt/lxc.conf
>  %config(noreplace) %{_sysconfdir}/logrotate.d/libvirtd.lxc
>      %endif
> @@ -1830,6 +1831,8 @@ fi
>  
>      %if %{with_lxc}
>  %attr(0755, root, root) %{_libexecdir}/libvirt_lxc
> +%attr(4755, root, root) %{_bindir}/virt-login-shell
> +%{_mandir}/man1/virt-login-shell.1*

Same for these two.

> diff --git a/tools/Makefile.am b/tools/Makefile.am
> index 644a86d..b4bed0b 100644
> --- a/tools/Makefile.am
> +++ b/tools/Makefile.am
> +virt_login_shell_LDFLAGS = $(COVERAGE_LDFLAGS)
> +virt_login_shell_LDADD =					\
> +		$(STATIC_BINARIES)				\
> +		$(PIE_LDFLAGS)					\
> +		$(RELRO_LDFLAGS) \
> +		../src/libvirt.la				\
> +		../src/libvirt-lxc.la				\
> +		../gnulib/lib/libgnu.la				\
> +		$(LIBXML_LIBS)

Can remove LIBXML_LIBS

> +
> +virt_login_shell_CFLAGS =					\
> +		$(WARN_CFLAGS)					\
> +		$(PIE_CFLAGS)					\
> +		$(COVERAGE_CFLAGS)				\
> +		$(LIBXML_CFLAGS)				\
> +		$(READLINE_CFLAGS)

Can remove LIBXML & READLINE here.

> diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
> new file mode 100644
> index 0000000..2528199
> --- /dev/null
> +++ b/tools/virt-login-shell.c
> @@ -0,0 +1,310 @@

> +#include <selinux/selinux.h>

I don't think you need this, since all selinux stuff is hidden
inside libvirt APIs.

> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
> +static int nfdlist = 0;

s/int/size_t/

> +static int *fdlist = NULL;
> +
> +static void virLoginShellFini(virConnectPtr conn, virDomainPtr  dom) {
> +    size_t i;
> +    if (nfdlist > 0) {

No need to have this  if (nfdlist > 0) since the next for() condition
already handles that, and VIR_FREE(NULL) is safe.

> +        for (i=0; i < nfdlist; i++)
> +            VIR_FORCE_CLOSE(fdlist[i]);
> +        VIR_FREE(fdlist);
> +        nfdlist = 0;
> +    }
> +    if (dom)
> +        virDomainFree(dom);
> +    if (conn)
> +        virConnectClose(conn);
> +}
> +
> +static int virLoginShellAllowedUser(virConfPtr conf, 
> +                                    uid_t uid, 

Some trailing whitespace at the end of the line - make syntax-check
will vcalidate this.

> +                                    gid_t gid,
> +                                    char *name) {
> +    virConfValuePtr p;
> +    int ret = 0;

Again, normal practice is to initialize 'ret = -1' - ie expect
failure

> +    char *ptr = NULL;
> +    size_t i;
> +    gid_t *groups = NULL;
> +
> +    p = virConfGetValue(conf, "allowed_users");
> +    if (!p) {
> +        errno = EPERM;
> +        fprintf(stderr, _("%s is not allowed to connect a container: %m\n"), name);
> +        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;
> +            } else {
> +                if (STREQ(pp->str, "*") || STREQ(pp->str, name)) {
> +                    ret = 0;
> +                    goto cleanup;
> +                }

You don't wnat to be using STREQ for this. Instead us fnmatch()
which does proper glob matching.

> +                if (pp->str[0] == '%') {
> +                    ptr=&pp->str[1];
> +                    gid_t groupid;
> +                    if (virGetGroupID(ptr, &groupid) < 0) {
> +                        fprintf(stderr, _("(%s) is not a valid group\n"), ptr);
> +                        continue;
> +                    }
> +                    if (virGetGroupList(uid, gid, &groups) < 0) 
> +                        goto err;
> +                    for (i=0; groups[i]; i++) {
> +                        if (groups[i] == groupid) {
> +                            ret = 0;
> +                            goto cleanup;
> +                        }
> +                    }

We want to have glob matching support on group names too, so you
can't just match on groupid. You'll need to turn the groupid into
a groupname, and then use fnmatch() again.

> +                }
> +            }
> +        }
> +    }
> +    errno = EPERM;
> +    fprintf(stderr, _("%s is not allowed to connect a container: %m\n"), name);
> +err:

You don't need this err: label if you initialize ret == -1;

> +    ret = -1;

and change this to 'ret = 0'

> +cleanup:
> +    VIR_FREE(groups);
> +    return ret;
> +}
> +
> +static char **virLoginShellGetShellArgv(virConfPtr conf) {
> +    size_t i;
> +    char **shargv;
> +    virConfValuePtr p;
> +    p = virConfGetValue(conf, "shell");
> +    if (!p)
> +        return virStringSplit("/bin/sh --login", " ", 3);
> +
> +    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;
> +    }
> +    return shargv;
> +err:

s/err/error/

> +    virStringFreeList(shargv);
> +    return NULL;
> +}
> +
> +int
> +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();
> +    char *name;
> +    char **shargv = NULL;
> +    virSecurityModelPtr secmodel = NULL;
> +    virSecurityLabelPtr seclabel = NULL;
> +    virDomainPtr dom = NULL;
> +    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 (uid == 0) {
> +        errno = EPERM;
> +        fprintf(stderr, _("%s must be run by non root users: %m\n"), argv[0]);
> +        goto err;
> +    }
> +
> +    if (argc > 1) {
> +        errno = EINVAL;
> +        fprintf(stderr, _("%s takes no options: %m\n"), argv[0]);
> +        goto err;
> +    } 

More trailing whitespace

> +
> +    name = virGetUserName(uid);
> +
> +    if (!(conf = virConfReadFile(login_shell_path, 0))) {
> +        fprintf(stderr, _("Failed to read %s: %m\n"), login_shell_path);
> +        goto err;
> +    }
> +    if (virLoginShellAllowedUser(conf, uid, gid, name) < 0)
> +        goto err;
> +
> +    if (!(shargv = virLoginShellGetShellArgv(conf)))
> +        goto err;
> +     

Trailing whitespace.

> +    conn = virConnectOpen("lxc:///");
> +    if (!conn) {
> +        fprintf(stderr, _("Unable to connect to lxc:///: %m\n"));
> +        goto err;
> +    }
> +
> +    dom = virDomainLookupByName(conn, name);
> +    if (!dom) {
> +        fprintf(stderr, _("Container %s does not exist: %m\n"), name);
> +        goto err;
> +    }
> +
> +    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"), name, virGetLastErrorMessage());
> +            goto err;
> +        }
> +    }
> +
> +    if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) {
> +        fprintf(stderr,_("Can't open %s namespace: %m\n"), 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) {
> +        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) {
> +            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;
> +        }
> +
> +        if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
> +            return -1;
> +
> +        ret = virSetUIDGIDWithCaps(uid, gid, groups, ngroups, 0, 0);
> +        VIR_FREE(groups);
> +        if (ret < 0)
> +            return -1;
> +
> +        if (virFork(&ccpid) < 0)
> +            return EXIT_FAILURE;
> +
> +        if (ccpid == 0) {
> +            char *homedir = virGetUserDirectory();

You'll need to call virGetUserDirectory() before any fork(), since it
calls code which is not async-signal safe.

> +            if (chdir(homedir) < 0) {
> +                fprintf(stderr, _("Unable chdir(%s): %m\n"), homedir);
> +                return EXIT_FAILURE;
> +            }
> +            if (execv(shargv[0], (char *const*) shargv) < 0) {
> +                fprintf(stderr, _("Unable exec shell %s: %m\n"), shargv[0]);
> +                return EXIT_FAILURE;
> +            }
> +        }
> +        return virProcessWait(ccpid, &status2);
> +    }
> +    ret = virProcessWait(cpid, &status);

Hmm, looking at this again, I'm wondering you need to fork()
at all. In virsh we do the double-fork dance, because virsh
is an interactive shell & we don't want to affect other parts
of virsh.

This login shell though is different - its only job is to run
inside the namespace. So can't the main process just enter
the namespace directly ?

> +    goto cleanup;
> +err:
> +    ret = EXIT_FAILURE;

You're pre-initializing ret to EXIT_FAILURE. So remove the
separate 'err' label and make everything jump to 'cleanup'
intead. Then just have a 'ret = EXIT_SUCCESS'

> +cleanup:
> +    virConfFree(conf);
> +    virLoginShellFini(conn, dom);
> +    virStringFreeList(shargv);
> +    VIR_FREE(seclabel);
> +    VIR_FREE(secmodel);
> +    return ret;
> +}

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]