On Thu, Jun 11, 2009 at 05:49:55PM +0200, Daniel Veillard wrote:
> On Wed, Jun 03, 2009 at 05:31:13PM +0100, Daniel P. Berrange wrote:
> > There are currently far too many cases where a correct URI returns an
> > generic 'failed to connect to hypervisor' error message. This gives the
> > user no idea why they could not connect. Of particular importance is
> > that they cannot distinguish a correct URI + plus a driver problem, vs
> > incorrect URI. Consider the following examples
> [...]
> > The core problem here is that far too many places are using the return
> > code VIR_DRV_OPEN_DECLINED instead of VIR_DRV_OPEN_ERROR. The former
> > provides no way to give any error info to the user. With this patch
> > I have taken the view that a driver must *only* ever use the return
> > code VIR_DRV_OPEN_DECLINED when:
> >
> > - Auto-probe of a driver URI, and this driver is not active
> > - Explicit URI with 'server' specified
> > - URI scheme does not match the driver
>
> okay
>
> > The remote driver is special in that it *must* accept all URIs given to
>
> As long as we check first it's a correct URI...
Well the URI is parsed by libxml already so its good enough for the
remote driver. It'll just pass it onto the libvirtd daemon, where a
real driver will accept or decline it. So the remote driver doesn't
need todo any validation itself really.
> > static virDrvOpenStatus openvzOpen(virConnectPtr conn,
> > virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> > int flags ATTRIBUTE_UNUSED)
> > {
> > struct openvz_driver *driver;
> > - if (!openvzProbe())
> > - return VIR_DRV_OPEN_DECLINED;
> >
> > if (conn->uri == NULL) {
> > + if (!virFileExists("/proc/vz"))
> > + return VIR_DRV_OPEN_DECLINED;
> > +
> > + if (access("/proc/vz", W_OK) < 0)
> > + return VIR_DRV_OPEN_DECLINED;
> > +
>
> Okay I was confused at first about dropping geteuid() == 0 check but
> it's a more specific check,
And geteuid() will soon be wrong when the libvirtd daemon runs as
non-root, but with appropriate capabilities to access /proc/vz.
> > enum virDrvOpenRemoteFlags {
> > VIR_DRV_OPEN_REMOTE_RO = (1 << 0),
> > - VIR_DRV_OPEN_REMOTE_UNIX = (1 << 1),
> > - VIR_DRV_OPEN_REMOTE_USER = (1 << 2),
> > - VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 3),
> > -};
> > -
> > -/* What transport? */
> > -enum {
> > - trans_tls,
> > - trans_unix,
> > - trans_ssh,
> > - trans_ext,
> > - trans_tcp,
> > -} transport;
> > -
> > -
> > + VIR_DRV_OPEN_REMOTE_USER = (1 << 1), /* Use the per-user socket path */
> > + VIR_DRV_OPEN_REMOTE_AUTOSTART = (1 << 2), /* Autostart a per-user daemon */
> > +};
> > +
> > +
> > +/*
> > + * URIs that this driver needs to handle:
> > + *
> > + * The easy answer:
> > + * - Everything that no one else has yet claimed, but nothing if
> > + * we're inside the libvirtd daemon
> > + *
> > + * The hard answer:
> > + * - Plain paths (///var/lib/xen/xend-socket) -> UNIX domain socket
> > + * - xxx://servername/ -> TLS connection
> > + * - xxx+tls://servername/ -> TLS connection
> > + * - xxx+tls:/// -> TLS connection to localhost
> > + * - xxx+tcp://servername/ -> TCP connection
> > + * - xxx+tcp:/// -> TCP connection to localhost
> > + * - xxx+unix:/// -> UNIX domain socket
> > + * - xxx:/// -> UNIX domain socket
> > + */
> > static int
> > doRemoteOpen (virConnectPtr conn,
> > struct private_data *priv,
> > @@ -328,37 +335,51 @@ doRemoteOpen (virConnectPtr conn,
> > {
> > int wakeupFD[2] = { -1, -1 };
> > char *transport_str = NULL;
> > + enum {
> > + trans_tls,
> > + trans_unix,
> > + trans_ssh,
> > + trans_ext,
> > + trans_tcp,
> > + } transport;
>
> hum ... I would have expected this to remain global somehow, but
> thinking about it, fine :-)
Its not entirely clear at first glance, but the original code was
declaring a global variable 'transport' with an anonymous enum.
And this global variable was used from the 'doRemoteOpen' method.
This is totally & utterly unsafe, it should always have been a
local variable :-)
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list