Re: [libvirt] [PATCH 40/41] remote: switch to connect to per-driver daemons by default
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 :|