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

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@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@Osier client]# pwd > /tmp/pki/client > [root@Osier client]# ls > cacert.pem clientcert.pem client.info clientkey.pem > > [root@Osier client]# mv cacert.pem .. > mv: overwrite `../cacert.pem'? y > [root@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