On Thu, Jul 18, 2013 at 05:25:56PM -0400, dwalsh(a)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 :|