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

[Osier Yang]

于 2011年01月24日 19:52, 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'

okay, make sense, will update.

>
>> +                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'.

yes, will update.
>
>> +            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
>

oh, yeah, checking $HOME/.pki for non-root user will be better, will
update.

Thanks
Osier