On Tue, Feb 09, 2016 at 11:51:57AM +0100, Pino Toscano wrote:
On Monday 08 February 2016 19:34:10 Richard W.M. Jones wrote:
On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote:
Introduce a new read-only API to get a path where to store temporary sockets: this is different from tmpdir, as we need short paths for sockets (due to sockaddr_un::sun_path), and it is either XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname to create sockets in that location.
Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for debugging.
As you saw, there were a few problems with this patch. However I also found something more fundamental.
On machines where XDG_RUNTIME_DIR is set to /run/user/$UID, it fails badly when run as root:
Original error from libvirt: internal error: process exited while connecting to monitor: 2016-02-08T19:17:42.375986Z qemu-system-x86_64: -chardev socket,id=charserial0,path=/run/user/0/libguestfsdittS9/console.sock: Failed to connect socket: Permission denied [code=1 int1=-1]
This is because libvirt runs the appliance as qemu.qemu, which cannot access /run/user/0 (mode 0700).
This is the default configuration when accessing a remote machine using `ssh root@remote virt-tool ...'
This is not due to the work itself, but to the limitations coming from other parties involved (libvirt in this case).
I think we should drop all references to XDG_RUNTIME_DIR (as I noted before, I don't have a high regard for freedesktop pseudo-standards, which I believe are just a way for some people to "policy launder" junk into Linux).
There isn't any needed to scrap everything at the first issue, there actually is a way to make this work better.
Also, not having high regards does not automatically mean that things are obnoxious, not that there isn't any middle ground possible.
The attached patch does that. Note the get-sockdir function now returns the hard-coded value "/tmp", which may or may not be a good idea.
NACK for me.
Attached there is a (IMHO) better approach to the issue found with libvirt: since only root needs particular changes, then limit them only for this user. After all, we all recommend using libguestfs and its tools as normal user as much as possible, right? So we shouldn't make the common user case worse only for non-recommended use cases.
Thanks, -- Pino Toscano
From 772f649e595d202bdb67f05aeb62157c1104be89 Mon Sep 17 00:00:00 2001 From: Pino Toscano <ptoscano@redhat.com> Date: Tue, 9 Feb 2016 11:36:23 +0100 Subject: [PATCH] lib: fix sockdir for root
When running as root libvirt will launch qemu as qemu.qemu, which will not be able to read inside the socket directory in case it is set as XDG_RUNTIME_DIR under /run/user/0.
Since normal users don't need particular extra access permissions for their sockdirs, restrict /tmp as only possible sockdir for root, changing the permissions only for this user (and making this operation fatal).
Fixes commit 55202a4d49a101392148d79cb2e1591428db2681 and commit 7453952d2456272624f154082e4fc4244af8e507. --- src/launch.c | 6 ------ src/tmpdirs.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/launch.c b/src/launch.c index 9e9960a..9273c58 100644 --- a/src/launch.c +++ b/src/launch.c @@ -428,12 +428,6 @@ guestfs_int_create_socketname (guestfs_h *g, const char *filename, if (guestfs_int_lazy_make_sockdir (g) == -1) return -1;
- /* Allow qemu (which may be running as qemu.qemu) to read the socket - * temporary directory. (RHBZ#610880). - */ - if (chmod (g->sockdir, 0755) == -1) - warning (g, "chmod: %s: %m (ignored)", g->sockdir); - if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { error (g, _("socket path too long: %s/%s"), g->sockdir, filename); return -1; diff --git a/src/tmpdirs.c b/src/tmpdirs.c index e66ab9c..76bf1c5 100644 --- a/src/tmpdirs.c +++ b/src/tmpdirs.c @@ -23,6 +23,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <libintl.h> +#include <unistd.h>
#include "ignore-value.h"
@@ -130,11 +131,20 @@ char * guestfs_impl_get_sockdir (guestfs_h *g) { const char *str; + uid_t euid = geteuid ();
- if (g->env_runtimedir) - str = g->env_runtimedir; - else + if (euid == 0) { + /* Use /tmp exclusively for root, as otherwise qemu (running as + * qemu.qemu when launched by libvirt) will not be able to access + * the directory. + */ str = "/tmp"; + } else { + if (g->env_runtimedir) + str = g->env_runtimedir; + else + str = "/tmp"; + }
return safe_strdup (g, str); } @@ -168,7 +178,24 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g) int guestfs_int_lazy_make_sockdir (guestfs_h *g) { - return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + int ret; + uid_t euid = geteuid (); + + ret = lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + if (ret == -1) + return ret; + + if (euid == 0) { + /* Allow qemu (which may be running as qemu.qemu) to read the socket + * temporary directory. (RHBZ#610880). + */ + if (chmod (g->sockdir, 0755) == -1) { + perrorf (g, "chmod: %s", g->sockdir); + return -1; + } + } + + return ret; }
/* Recursively remove a temporary directory. If removal fails, just
ACK. Since the chmod in this case has been folded into the guestfs_int_lazy_make_sockdir function, there should probably be a followup commit to do the same for guestfs_int_lazy_make_tmpdir. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/