On Thu, Jul 18, 2013 at 05:25:56PM -0400, dwalsh@xxxxxxxxxx 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list