Re: [PATCH 4/6] admin: support server cert update mode

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 :|





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux