On Sun, Feb 09, 2020 at 10:03:14PM +0800, Zhang Bo wrote:
> virAdmServerUpdateTlsFiles:
> @flags specifies how to update server cert/key in tls service.
> Two modes are currently supported: append mode and clear mode, means
> whether to clear the original cert then add the new one, or just append
> to the original one.
> ---
> include/libvirt/libvirt-admin.h | 14 ++++++++++++++
> src/admin/admin_server.c | 7 +------
> src/admin/libvirt-admin.c | 7 ++++++-
> src/rpc/virnetserver.c | 17 +++++++++++++----
> src/rpc/virnetserver.h | 3 ++-
> src/rpc/virnettlscontext.c | 7 +++++--
> src/rpc/virnettlscontext.h | 3 ++-
> 7 files changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> index 6e38261129..dfdd81ae83 100644
> --- a/include/libvirt/libvirt-admin.h
> +++ b/include/libvirt/libvirt-admin.h
> @@ -392,6 +392,20 @@ int virAdmClientClose(virAdmClientPtr client, unsigned int flags);
>
> # define VIR_SERVER_CLIENTS_UNAUTH_CURRENT "nclients_unauth"
>
> +typedef enum {
> + /* free old credentials and then set new tls context.
> + */
> + VIR_TLS_UPDATE_CLEAR = 0,
> +
> + /* do not clear original certificates and keys.
> + */
> + VIR_TLS_UPDATE_APPEND = 1,
I don't think we should we supporting this operation. In order to achieve
reliability of the TLS reload, we need to re-create the credentials object
and swap out the original credentails on success. This precludes updating
the original credentials....
> @@ -1165,7 +1166,9 @@ int virNetTLSContextReload(virNetTLSContextPtr ctxt,
> }
>
> if (filetypes & VIR_TLS_FILE_TYPE_SERVER_CERT) {
> - gnutls_certificate_free_keys(ctxt->x509cred);
> + if (flags == VIR_TLS_UPDATE_CLEAR)
> + gnutls_certificate_free_keys(ctxt->x509cred);
> +
> if (virNetTLSContextSetCertAndKey(ctxt, cert, key, false))
> goto cleanup;
...because this code still has the dangerous behaviour that it leaves the
server without any valid cert/key on failure.
The second bad thing is that the state in-memory does not match the
state on disk. This puts the old & new cert+key in memory, but only
the new cert+key is on disk. So if libvirtd is restarted for any
reason you'll get a change in certs again. The reload operation
should just be updating libvirtd's in-memory state to exactly match
the on-disk state, in the same way that a restart does.
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 :|
