On Sun, Feb 09, 2020 at 10:03:12PM +0800, Zhang Bo wrote:
> Add an API to update server's tls context before admin method can be
> introduced.
> ---
> include/libvirt/libvirt-admin.h | 8 ++++
> src/libvirt_remote.syms | 1 +
> src/rpc/virnetserver.c | 72 +++++++++++++++++++++++++++++++++
> src/rpc/virnetserver.h | 3 ++
> src/rpc/virnetserverclient.c | 4 ++
> src/rpc/virnettlscontext.c | 41 +++++++++++++++++++
> src/rpc/virnettlscontext.h | 2 +
> 7 files changed, 131 insertions(+)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index abf2792926..3edc044490 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,14 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +/* tls related filetype flags. */
> +typedef enum {
> + VIR_TLS_FILE_TYPE_CA_CERT = (1U << 0),
> + VIR_TLS_FILE_TYPE_CA_CRL = (1U << 1),
> + VIR_TLS_FILE_TYPE_SERVER_CERT = (1U << 2),
> + VIR_TLS_FILE_TYPE_SERVER_KEY = (1U << 3),
> +} virServerTlsFiletype;
[snip]
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 12811bed78..8baa6a15b2 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -1139,6 +1139,47 @@ void virNetTLSContextDispose(void *obj)
> gnutls_certificate_free_credentials(ctxt->x509cred);
> }
>
> +int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> + unsigned int filetypes)
> +{
> + int ret = -1;
> + char *cacert = NULL;
> + char *cacrl = NULL;
> + char *cert = NULL;
> + char *key = NULL;
> +
> + virObjectLock(ctxt);
> +
> + if (virNetTLSContextLocateCredentials(NULL, false, true,
> + &cacert, &cacrl, &cert, &key) < 0)
> + goto cleanup;
> +
> + if (filetypes & VIR_TLS_FILE_TYPE_CA_CERT) {
> + if (virNetTLSContextSetCACert(ctxt, cacert, false))
> + goto cleanup;
> + }
> +
> + if (filetypes & VIR_TLS_FILE_TYPE_CA_CRL) {
> + if (virNetTLSContextSetCACRL(ctxt, cacrl, false))
> + goto cleanup;
> + }
> +
> + if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> + gnutls_certificate_free_keys(ctxt->x509cred);
> + if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> + goto cleanup;
> + }
I'm not convinced we should be doing a selective update of only
a subset of files here.
I feel like an oneline update should always have the exact same
result as you would get by doing a restart of libvirtd.
Consider if you update the CA cert, CA CRL and Server Cert
on disk, but then tell libvirtd to only reload Server Cert.
The state of libvirtd is now out of sync with state on disk,
and when libvirtd gets restarted due to a RPM software upgrade
later, its will load different content again.
This can lead to hard to diagnose problems, or delayed discovery
of problems.
The second is is that we're modifying the existing "x509cred"
object in virNetTLSContext.
gnutls_certificate_free_keys(ctxt->x509cred);
if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
goto cleanup;
Consider if virNetTLSContextSetCertAndKey() here fails - now libvirtd
is missing the original TLS cert/key, and also missing the new TLS
cert/key.
When we're reloading certs, I think we need to create an entirely
new gnutls_certificate_credentials_t object, and populate it from
all the files on disk.
Only once that is succesful, should we then replace the "x509cred"
object in the virNetTLSContext. This gives us an atomic update
path, so we're guaranteed to always have valid credentials
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
