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