[Libguestfs] [PATCH v2 2/2] New API: get-sockdir
Richard W.M. Jones
rjones at redhat.com
Tue Feb 9 12:31:13 UTC 2016
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 at 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 at 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/
More information about the Libguestfs
mailing list