[libvirt] [PATCH 40/41] remote: switch to connect to per-driver daemons by default

[Daniel P. Berrangé]

On Mon, Jul 29, 2019 at 02:32:31PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:03 +0100, Daniel P. Berrangé wrote:
> [...]
> > If connecting to a remote host over any kind of ssh tunnel, for now we
> > must assume only the legacy socket exists. A future patch will introduce
> > a netcat replacement that is tailored for libvirt to make remote
> > tunnelling easier.
> > 
> > The configure arg '--with-remote-default-mode=legacy|direct' allows
> > packagers to set a default at build time. If not given, it will default
> > to direct mode.
> > 
> > In RPM builds this is overriden, because before we can default to the
> > new daemons, we must get SELinux policy written & the timeframe for that
> > is unclear at this stage.


> > @@ -758,21 +776,126 @@ remoteGetUNIXSocket(remoteDriverTransport transport,
> >          if (!(userdir = virGetUserRuntimeDirectory()))
> >              return NULL;
> >  
> > -        if (virAsprintf(&sockname,
> > -                        "%s/" LIBVIRTD_USER_UNIX_SOCKET, userdir) < 0)
> > +        if (virAsprintf(&sockname, "%s/%s-sock",
> > +                        userdir, sock_prefix) < 0)
> 
> I kinda just noticed, but don't we support R/O connections in
> session mode?

The client app is required to be the same user ID as the daemon.
As such there's no meaningful security separation between the
two from a DAC pov, so R/O socket was deemed to be a waste of
time.

If you had SELinux strictly locking things down it could be
considered slightly more secure, but no one has ever cared
enough to enable it.


> > +        if (!direct_sock_name) {
> > +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> > +                           _("Cannot use direct socket mode if no URI is set"));
> > +            return NULL;
> > +        }
> 
> Is the error message accurate? We should be way past making sure we
> have a URI to work with by now.

We'll only hit  direct_sock_name == NULL, if driver == NULL.

We'll only hit driver == NULL, if the original URI was NULL.


> > +#ifndef WIN32
> > +static const char *
> > +remoteGetDaemonPathEnv(void)
> > +{
> > +    /* We prefer a VIRTD_PATH env var to use for all daemons,
> > +     * but if it is not set we will fallback to LIBVIRTD_PATH
> > +     * for previous behaviour
> > +     */
> > +    if (virGetEnvBlockSUID("VIRTD_PATH") != NULL) {
> > +        return "VIRTD_PATH";
> > +    } else {
> > +        return "LIBVIRTD_PATH";
> > +    }
> > +}
> > +#endif /* WIN32 */
> 
> I don't think this function needs to be guarded by 'ifndef WIN32':
> we already do so at the call site, and AFAICT there's nothing in the
> helper itself that warrants compiling it out on Windows.

It is a static function, so will trigger an unused  function warning.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|