Re: [PATCH] vnc: add qmp to support change authz

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Zihao Chang <changzihao1@huawei.com>
Cc: oscar.zhangbo@huawei.com, kraxel@redhat.com,
	qemu-devel@nongnu.org, xiexiangyou@huawei.com, armbru@redhat.com
Subject: Re: [PATCH] vnc: add qmp to support change authz
Date: Thu, 7 Jan 2021 16:18:30 +0000	[thread overview]
Message-ID: <20210107161830.GE1029501@redhat.com> (raw)
In-Reply-To: <20210107124657.650-1-changzihao1@huawei.com>

On Thu, Jan 07, 2021 at 08:46:57PM +0800, Zihao Chang wrote:
> This patch add a new qmp 'change-vnc-authz' to support change the
> tls/sasl authz of vm. If index='', unset tlsauthzid/sasl.authzid
> {
>    "execute":"change-vnc-authz",
>    "arguments":{
>       "index":"object-authz-id",
>       "type":"tls/sasl"
>    }
> }

I'm not a fan of adding adhoc new commands for specific properties.

It feels like that is just repeating the design mistakes we made
for years with the CLI where we added countless -set-this-feature-too
args.  We realized this mistake and now have the general purpose
"-display" arg.

If we want to support changing properties on the fly in QMP, then
I think we should do a general purpose "display_update" QMP
command, to replace the existing 'set_pasword', 'expire_password',
'change-vnc-password' commands, and also enable this authz change.

It can also enable changing the TLS credentials object, and the
authentication scheme, and be extensible to any number of other
VNC properties we might like to make configurable in the future.
Similarly it would then be extensible for SPICE and other graphics
backends.

> 
> Signed-off-by: Zihao Chang <changzihao1@huawei.com>
> ---
>  include/ui/console.h |  3 +++
>  monitor/qmp-cmds.c   | 10 ++++++++++
>  qapi/ui.json         | 16 ++++++++++++++++
>  ui/vnc.c             | 32 ++++++++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 5dd21976a3..6b85546105 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -441,6 +441,9 @@ 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);
> +#ifdef CONFIG_VNC_SASL
> +int vnc_change_authz(const char *id, const char *type, const char *index);
> +#endif
>  
>  /* 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..085aeb9bec 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -289,6 +289,16 @@ static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
>  }
>  #endif /* !CONFIG_VNC */
>  
> +#ifdef CONFIG_VNC_SASL
> +void qmp_change_vnc_authz(const char *type, const char *index, Error **errp)
> +{
> +    if (vnc_change_authz(NULL, type, index) < 0) {
> +        error_setg(errp, "Could not set authz, type:%s, index:%s",
> +                   type, index);
> +    }
> +}
> +#endif
> +
>  void qmp_change(const char *device, const char *target,
>                  bool has_arg, const char *arg, Error **errp)
>  {
> diff --git a/qapi/ui.json b/qapi/ui.json
> index d08d72b439..37ddeabbd2 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1179,3 +1179,19 @@
>  ##
>  { 'command': 'query-display-options',
>    'returns': 'DisplayOptions' }
> +
> +##
> +# @change-vnc-authz:
> +#
> +# Change the VNC server authz.
> +#
> +# @type:  the new authz type to use with VNC authentication
> +# @index:  the new authz object index to use with VNC authentication
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'command': 'change-vnc-authz',
> +  'data': { 'type' : 'str',
> +            'index': 'str'},
> +  'if': 'defined(CONFIG_VNC_SASL)' }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 7452ac7df2..f0809290a8 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3276,6 +3276,38 @@ int vnc_display_password(const char *id, const char *password)
>      return 0;
>  }
>  
> +#ifdef CONFIG_VNC_SASL
> +int vnc_change_authz(const char *id, const char *type, const char *index)
> +{
> +    VncDisplay *vd = vnc_display_find(id);
> +
> +    if (!vd) {
> +        return -EINVAL;
> +    }
> +
> +    if (strcmp(type, "sasl") == 0) {
> +        g_free(vd->sasl.authzid);
> +        vd->sasl.authzid = NULL;
> +
> +        if (strcmp(index, "") != 0) {
> +            vd->sasl.authzid = g_strdup(index);
> +        }
> +    } else if (strcmp(type, "tls") == 0) {
> +        g_free(vd->tlsauthzid);
> +        vd->tlsauthzid = NULL;
> +
> +        if (strcmp(index, "") != 0) {
> +            vd->tlsauthzid = g_strdup(index);
> +        }
> +    } else {
> +        error_printf_unless_qmp("unsupport authz type: %s", type);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +#endif
> +
>  int vnc_display_pw_expire(const char *id, time_t expires)
>  {
>      VncDisplay *vd = vnc_display_find(id);
> -- 
> 2.23.0
> 
> 

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