Re: [PATCH] vnc: add qmp to support reload vnc tls certificates

[Daniel P . Berrangé]

On Mon, Jan 04, 2021 at 03:11:28PM +0800, Zihao Chang wrote:
> QEMU loads vnc tls certificates only when vm is started. This patch
> provides a new qmp command to reload vnc tls certificates without
> restart vnc-server/VM.
> 
> Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> ---
>  include/ui/console.h |  1 +
>  monitor/qmp-cmds.c   |  7 +++++++
>  qapi/ui.json         | 17 +++++++++++++++++
>  ui/vnc.c             | 20 ++++++++++++++++++++
>  4 files changed, 45 insertions(+)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5dd21976a3..f05140b662 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -441,6 +441,7 @@ int vnc_display_password(const char *id, const char 
> *password);
>  int vnc_display_pw_expire(const char *id, time_t expires);
>  QemuOpts *vnc_parse(const char *str, Error **errp);
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> +int vnc_display_reload_cert(const char *id,  Error **errp);
>  
>  /* input.c */
>  int index_from_key(const char *key, size_t key_length);
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 34f7e75b7b..90bd08c8ed 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -287,6 +287,13 @@ static void qmp_change_vnc(const char *target, bool 
> has_arg, const char *arg,
>          qmp_change_vnc_listen(target, errp);
>      }
>  }
> +
> +void qmp_reload_vnc_cert(Error **errp)
> +{
> +    if (vnc_display_reload_cert(NULL, errp) < 0) {
> +        error_setg(errp, "Reload vnc tls cert failed");

This error message is entirely useless at helping diagnose what
failed. We need to propagate real error messages from the root
cause.

> +    }
> +}
>  #endif /* !CONFIG_VNC */
>  
>  void qmp_change(const char *device, const char *target,
> diff --git a/qapi/ui.json b/qapi/ui.json
> index d08d72b439..bc3ffdb20f 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1179,3 +1179,20 @@
>  ##
>  { 'command': 'query-display-options',
>    'returns': 'DisplayOptions' }
> +
> +##
> +# @reload-vnc-cert:
> +#
> +# Reload certificates for vnc.
> +#
> +# Returns: nothing
> +#
> +# Since: 5.2
> +#
> +# Example:
> +#
> +# -> { "execute": "reload-vnc-cert" }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'reload-vnc-cert' }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7452ac7df2..b0cfbcf47c 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -582,6 +582,26 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp)
>      return prev;
>  }
>  
> +int vnc_display_reload_cert(const char *id, Error **errp)
> +{
> +    VncDisplay *vd = vnc_display_find(id);
> +
> +    if (!vd) {
> +        return -EINVAL;
> +    }
> +
> +    if (!vd->tlscreds) {
> +        error_printf_unless_qmp("If you want use vnc tls  please enable "
> +                                "vnc tls using '-vnc 
> tls-creds=${tls-obj-id}'.\n");
> +        return -EPERM;

You're passing in a 'Error' object, so use that and don't retyrn errnos

> +    }
> +
> +    object_property_set_bool(OBJECT(vd->tlscreds), "loaded", false, NULL);
> +    object_property_set_bool(OBJECT(vd->tlscreds), "loaded", true, NULL);

This is ignoring all errors which is not at all acceptable. It means on
failure to load the new certs, we're left with a broken creds object
which callers will not expect.

We need to be able to do this in a safe way such that we carry on using
the original TLS certs if loading the new ones fails. This can't be done
using the 'loaded' property- we need an explicit API to reload things.

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