Libguestfs - Feb 2016 - Re: [PATCH v2 2/2] New API: get-sockdir

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 ...'

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).

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.

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/

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

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/