[PATCH] vnc: add qmp to support change authz

[PATCH] vnc: add qmp to support change authz

[Zihao Chang]

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"
   }
}

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

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

[Gerd Hoffmann]

  Hi,

> 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"
>    }
> }

> +##
> +# @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)' }

type should be an enum I think.

Also index should be an int (and possibly an optional argument so you
can just not specify index to unset).

take care,
  Gerd

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

[Zihao Chang]

Hi,

On 2021/1/8 0:04, Gerd Hoffmann wrote:
>   Hi,
> 
>> 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"
>>    }
>> }
> 
>> +##
>> +# @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)' }
> 
> type should be an enum I think.
It is a good idea to set type to enum, I will fix it in the next version.

> Also index should be an int (and possibly an optional argument so you
> can just not specify index to unset).
> 
Index is the id of authz object(myauthz, user can set it as they wanted), may not use int.
-object authz-simple,id=myauthz,identity=test1

> take care,
>   Gerd
> 
> .
> 
BTW, Daniel P . Berrangé suggests that all property set QMP commands should be integrated
as an general purpose "display_update" QMP command. That's really a good idea. This may
take longer time to design and code. Will we not think about adding new property set QMP
until the general purpose "display_update" QMP is ready？

https://lists.nongnu.org/archive/html/qemu-devel/2021-01/msg00941.html


Thanks，
Zihao

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

[Gerd Hoffmann]

> >> +{ 'command': 'change-vnc-authz',
> >> +  'data': { 'type' : 'str',
> >> +            'index': 'str'},
> >> +  'if': 'defined(CONFIG_VNC_SASL)' }
> > 
> > type should be an enum I think.
> It is a good idea to set type to enum, I will fix it in the next version.
> 
> > Also index should be an int (and possibly an optional argument so you
> > can just not specify index to unset).
> > 
> Index is the id of authz object(myauthz, user can set it as they wanted), may not use int.
> -object authz-simple,id=myauthz,identity=test1

Name it "auth-id" then?  "index" sounds like a number ...

> BTW, Daniel P . Berrangé suggests that all property set QMP commands should be integrated
> as an general purpose "display_update" QMP command. That's really a good idea.

Makes sense indeed.

take care,
  Gerd

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

[Daniel P. Berrangé]

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