[libvirt] [v1] remote: Add extra parameter client_pki_dir for URI

[Daniel P. Berrange]

On Sun, Jan 23, 2011 at 07:24:05PM +0800, Osier Yang wrote:
> This new parameter allows user specifies directory in which the client
> cerficate, client key, CA certificate reside (the directory path must
> be absolute), instead of hardcoding it in remote driver. But the hardcoded
> locations are still kept, and attempt to use them if the required files
> can not be found in the location user specified. e.g.
> 
> [root at Osier client]# virsh -c qemu+tls://$hostname/system?client_pki_dir\
> > =/tmp/pki/client
> Welcome to virsh, the virtualization interactive terminal.
> 
> Type:  'help' for help with commands
>        'quit' to quit
> 
> virsh # quit
> 
> [root at Osier client]# pwd
> /tmp/pki/client
> [root at Osier client]# ls
> cacert.pem  clientcert.pem  client.info  clientkey.pem
> 
> [root at Osier client]# mv cacert.pem ..
> mv: overwrite `../cacert.pem'? y
> [root at Osier client]# virsh -c qemu+tls://10.66.93.111/system?client_pki_dir\
> > =/tmp/pki/client
> 19:11:23.844: 7589: warning : initialize_gnutls:1186 : /tmp/pki/client/cacert.pem
>   doesn't exist, try to use: /etc/pki/CA/cacert.pem
> Welcome to virsh, the virtualization interactive terminal.
> 
> Type:  'help' for help with commands
>        'quit' to quit
> 
> virsh #
> 
> * src/remote/remote_driver.c
> ---
>  src/remote/remote_driver.c |   87 ++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 75 insertions(+), 12 deletions(-)
> 
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index ea119c6..7053e45 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -268,7 +268,7 @@ void remoteDomainEventQueueFlush(int timer, void *opaque);
>  static char *get_transport_from_scheme (char *scheme);
> 
>  /* GnuTLS functions used by remoteOpen. */
> -static int initialize_gnutls(void);
> +static int initialize_gnutls(char *client_pki_dir);
>  static gnutls_session_t negotiate_gnutls_on_connection (virConnectPtr conn, struct private_data *priv, int no_verify);
> 
>  #ifdef WITH_LIBVIRTD
> @@ -430,6 +430,7 @@ doRemoteOpen (virConnectPtr conn,
>      char *port = NULL, *authtype = NULL, *username = NULL;
>      int no_verify = 0, no_tty = 0;
>      char **cmd_argv = NULL;
> +    char *client_pki_dir = NULL;
> 
>      /* Return code from this function, and the private data. */
>      int retcode = VIR_DRV_OPEN_ERROR;
> @@ -509,9 +510,14 @@ doRemoteOpen (virConnectPtr conn,
>                      priv->debugLog = stdout;
>                  else
>                      priv->debugLog = stderr;
> -            } else
> +            } else if (STRCASEEQ(var->name, "client_pki_dir")) {

'client' is redundant here, and it'd be nice to avoid 'dir' in the
name, since if we ever have to use NSS, it might point to a file
rather than a dir. So it would be preferable to call this 'pkipath'

> +                client_pki_dir = strdup(var->value);
> +                if (!client_pki_dir) goto out_of_memory;
> +                var->ignore = 1;
> +            } else {
>                  DEBUG("passing through variable '%s' ('%s') to remote end",
>                        var->name, var->value);
> +            }
>          }

> @@ -1166,18 +1176,64 @@ initialize_gnutls(void)
>          return -1;
>      }
> 
> +    if (client_pki_dir) {
> +        if((virAsprintf(&libvirt_cacert, "%s/%s", client_pki_dir,
> +                        "cacert.pem")) < 0)
> +             goto out_of_memory;
> +
> +        if (!virFileExists(libvirt_cacert)) {
> +            VIR_WARN(_("%s doesn't exist, try to use: %s"),
> +                     libvirt_cacert, LIBVIRT_CACERT);
> +
> +            libvirt_cacert = strdup(LIBVIRT_CACERT);

This just leaked the memory previously assigned to 'libvirt_cacert'.

> +            if (!libvirt_cacert) goto out_of_memory;
> +        }
> +
> +        if((virAsprintf(&libvirt_clientkey, "%s/%s", client_pki_dir,
> +                        "clientkey.pem")) < 0)
> +             goto out_of_memory;
> +
> +        if (!virFileExists(libvirt_clientkey)) {
> +            VIR_WARN(_("%s doesn't exist, try to use: %s"),
> +                     libvirt_clientkey, LIBVIRT_CLIENTKEY);
> +
> +            libvirt_clientkey = strdup(LIBVIRT_CLIENTKEY);
> +            if (!libvirt_clientkey) goto out_of_memory;
> +        }
> +
> +        if((virAsprintf(&libvirt_clientcert, "%s/%s", client_pki_dir,
> +                        "clientcert.pem")) < 0)
> +             goto out_of_memory;
> 
> -    if (check_cert_file("CA certificate", LIBVIRT_CACERT) < 0)
> +        if (!virFileExists(libvirt_clientcert)) {
> +            VIR_WARN(_("%s doesn't exist, try to use: %s"),
> +                     libvirt_clientcert, LIBVIRT_CLIENTCERT);
> +
> +            libvirt_clientcert = strdup(LIBVIRT_CLIENTCERT);
> +            if (!libvirt_clientcert) goto out_of_memory;
> +        }
> +    } else {
> +        libvirt_cacert = strdup(LIBVIRT_CACERT);
> +        if (!libvirt_cacert) goto out_of_memory;
> +
> +        libvirt_clientkey = strdup(LIBVIRT_CLIENTKEY);
> +        if (!libvirt_cacert) goto out_of_memory;
> +
> +        libvirt_clientcert = strdup(LIBVIRT_CLIENTCERT);
> +        if (!libvirt_cacert) goto out_of_memory;
> +    }

I think I would structure this code somewhat differently. We currently
have a global location for PKI. This patch lets the user set a custom dir
per URI. I think we also want to check $HOME/.pki for non-root users.

Thus I would structure it:

  - If  pkipath URI parameter is set, use that. If a file does
    not exist raise a fatal error, no fallback

  - Try $HOME/.pki if non root. If the files don't exist, or if
    root, then use /etc/pki

Daniel