[Libvir] [PATCH] Remote 3/8: Client-side

[Daniel P. Berrange]

On Sat, May 05, 2007 at 12:17:44PM +0100, Richard W.M. Jones wrote:
> Richard W.M. Jones wrote:
> >3 Client-side
> >-------------
> >
> >A src/remote_internal.c
> >A src/remote_internal.h
> >M src/driver.h
> >M src/libvirt.c
> 
> This is the code which serialises requests on the client side.  First up 
> is a small patch against driver.h and libvirt.c which just declares and 
> registers the new driver.
> 
> Secondly, the driver itself (remote_internal.h / remote_internal.c) 
> which I will describe in more detail.
> 
> The header file is small and I think non-controversial.  It just 
> declares the remoteRegister function and a handful of constants like 
> paths and port numbers.

What sort of info is currently stored in the  $sysconfdir/libvirtd.conf
file ? It seems to be referred to by both the client & server. Although
there will be options common to both the client & server I think it may
be worth having them refer to separate files (cf ssh_config, sshd_config).
ACAICT there's no compelling need to have them refer to same file since the
client & server will be on different machines anyway. The separation 
could be useful if there turns out to be a reasonble to make libvirtd.conf
mode 0600.

On the subject of 

>  /* GnuTLS functions used by remoteOpen. */
>  #define CAFILE "demoCA/cacert.pem" /* XXX */
>  #define KEY_FILE "127001key.pem" /* XXX */
>  #define CERT_FILE "127001cert.pem" /* XXX */

We've got to think about best way to provide TLS parameters. Bearing in mind
that people may want to port to Mozilla NSS in the future I think it is 
probably best to not provide formal APIs for these options at this time, and
instead pick them up from the client's config file. As previously discussed
there's no clear standard for location of TLS certs, so I reckon we just 
default to something like

   tls_ca_cert = "/etc/pks/tls/ca-cert.pem"
   tls_secret = "/etc/pks/tls/libvirt-key.pem"
   tls_cert = "/etc/pks/tls/libvirt-cert.pem"

This actually does suggest we need separate libvirt.conf & libvirtd.conf
because obviously the server needs to point to its own key/cert & also
keep a whitelist of client keys.

The client might actually need a more flexible config - allowing it to
present different keys, per host. We could do something based on a
placeholder, with a default fallback

   tls_ca_cert = "/etc/pks/tls/ca-cert.pem"

   tls_default_secret = "/etc/pks/tls/libvirt-key.pem"
   tls_default_cert = "/etc/pks/tls/libvirt-cert.pem"

   tls_host_cert = "/etc/pks/tls/libvirt-%h-key.pem"
   tls_host_cert = "/etc/pks/tls/libvirt-%h-cert.pem"

Since we're using client certs for authentication to start with, we ought
to actually make it possible to keep the certs in per-user home dirs rather
than globally readable. eg $HOME/.pki/tls/libvirt-key.pem  and mode 0600

> The driver itself (remote_internal.c) consists of the following parts, 
> arranged here in the same order that they appear in the file:
> 
> (1) remoteOpen and associated, GnuTLS initialisation
> 
> This function and associated functions deals with parsing the remote 
> URI, deciding which transport to use, deciding the URI to forward to the 
> remote end, creating the transport (including initialising GnuTLS, 
> forking ssh, ...), and verifying the server's certificate.

Wrt to this snippet in negotiate_gnutls_on_connection()

>    const int cert_type_priority[3] = {
>        GNUTLS_CRT_X509,
>        GNUTLS_CRT_OPENPGP,
>        0
>    };

Do we have support in the client/server for doing whatever checks are needed
for the OpenPGP style 'certs'. If not we should probably comment out the
OpenGPG option for now.
 
> (2) The driver functions, remoteClose, remoteType, etc. etc.
> (3) The network driver functions, remoteNetworkOpen, etc. etc.
> 
> For each function we serialise the parameters as an XDR remote_foo_args 
> object, use the generic "call" function to dispatch the call, then 
> deserialise the XDR remote_foo_ret return value.  Most of the heavy 
> lifting for the RPC is done in the "call" function itself ...

This all loooks good to me. Only thought I had was perhaps that

      GET_PRIVATE (conn, -1);

is slightly more readable as

      struct private_data *priv = GET_PRIVATE (conn, -1);

ie, to make it clear to the readable what local variable is being
provided, while still hiding away all the tedious error checking /
assertion logic.

> (4) The RPC implementation (function "call"), error handling, 
> serialisation of virDomainPtr and virNetworkPtr.

In the 'call' method:

>    /* Get a unique serial number for this message. */
>    /* XXX This is supposed to be atomic over threads, but I don't know
>     * if it is.
>     */
>    static sig_atomic_t counter = 1;
>    sig_atomic_t serial = counter++;

Serial numbers only need to be unique within the scope of a single
client <-> server socket connection. So could we just store the
counter in the virConectPtr private data struct to avoid the threading
question here



> 
> ... which also deals with making error handling transparent.

> static void
> error (virConnectPtr conn, virErrorNumber code, const char *info)
> {
>     const char *errmsg;
>     /* XXX I don't think this is correct use of virErrorMsg. */
>    errmsg = __virErrorMsg (code, info);


This looks consistent with the way other drivers are using __virErrorMsg
at least. Anything in particular you thought was wrong ?




Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|