[PATCH 0/4] ui: add support for 'secret' object to provide VNC/SPICE passwords

[PATCH 0/4] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Daniel P. Berrangé]

This fixes a long standing limitation of the VNC/SPICE code which was
unable to securely accept passswords on the CLI, instead requiring use
of separate monitor commands after startup.

This takes the opportunity to also remove previously deprecated ACL
functionality from VNC.

Daniel P. Berrangé (4):
  ui: introduce "password-secret" option for VNC servers
  ui: introduce "password-secret" option for SPICE server
  ui: deprecate "password" option for SPICE server
  ui, monitor: remove deprecated VNC ACL option and HMP commands

 docs/system/deprecated.rst       |  24 ++--
 docs/system/removed-features.rst |  13 +++
 hmp-commands.hx                  |  76 -------------
 monitor/misc.c                   | 187 -------------------------------
 qemu-options.hx                  |  17 ++-
 ui/spice-core.c                  |  32 +++++-
 ui/vnc.c                         |  61 ++++------
 7 files changed, 88 insertions(+), 322 deletions(-)

-- 
2.29.2

[PATCH 1/4] ui: introduce "password-secret" option for VNC servers

[Daniel P. Berrangé]

Currently when using VNC the "password" flag turns on password based
authentication. The actual password has to be provided separately via
the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --vnc localhost:0,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx |  5 +++++
 ui/vnc.c        | 23 ++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 6c34c7050f..893d0f500b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2164,6 +2164,11 @@ SRST
         time to allow <protocol> password to expire immediately or never
         expire.
 
+    ``password-secret=<secret-id>``
+        Require that password based authentication is used for client
+        connections, using the password provided by the ``secret``
+        object identified by ``secret-id``.
+
     ``tls-creds=ID``
         Provides the ID of a set of TLS credentials to use to secure the
         VNC server. They will apply to both the normal VNC server socket
diff --git a/ui/vnc.c b/ui/vnc.c
index 16bb3be770..77e07ac351 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -48,6 +48,7 @@
 #include "crypto/tlscredsanon.h"
 #include "crypto/tlscredsx509.h"
 #include "crypto/random.h"
+#include "crypto/secret_common.h"
 #include "qom/object_interfaces.h"
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
@@ -3469,6 +3470,9 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "reverse",
             .type = QEMU_OPT_BOOL,
@@ -3941,6 +3945,7 @@ void vnc_display_open(const char *id, Error **errp)
     int lock_key_sync = 1;
     int key_delay_ms;
     const char *audiodev;
+    const char *passwordSecret;
 
     if (!vd) {
         error_setg(errp, "VNC display not active");
@@ -3958,7 +3963,23 @@ void vnc_display_open(const char *id, Error **errp)
         goto fail;
     }
 
-    password = qemu_opt_get_bool(opts, "password", false);
+
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        if (qemu_opt_get(opts, "password")) {
+            error_setg(errp,
+                       "'password' flag is redundant with 'password-secret'");
+            goto fail;
+        }
+        vd->password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                     errp);
+        if (!vd->password) {
+            goto fail;
+        }
+        password = true;
+    } else {
+        password = qemu_opt_get_bool(opts, "password", false);
+    }
     if (password) {
         if (fips_get_state()) {
             error_setg(errp,
-- 
2.29.2

[PATCH 2/4] ui: introduce "password-secret" option for SPICE server

[Daniel P. Berrangé]

Currently when using SPICE the "password" option provides the password
in plain text on the command line. This is insecure as it is visible
to all processes on the host. As an alternative, the password can be
provided separately via the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --spice port=5901,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx |  8 ++++++--
 ui/spice-core.c | 28 ++++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 893d0f500b..ff4ef3b708 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1898,7 +1898,7 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,tls-ciphers=<list>]\n"
     "       [,tls-channel=[main|display|cursor|inputs|record|playback]]\n"
     "       [,plaintext-channel=[main|display|cursor|inputs|record|playback]]\n"
-    "       [,sasl][,password=<secret>][,disable-ticketing]\n"
+    "       [,sasl][,password=<string>][,password-secret=<secret-id>][,disable-ticketing]\n"
     "       [,image-compression=[auto_glz|auto_lz|quic|glz|lz|off]]\n"
     "       [,jpeg-wan-compression=[auto|never|always]]\n"
     "       [,zlib-glz-wan-compression=[auto|never|always]]\n"
@@ -1923,9 +1923,13 @@ SRST
     ``ipv4``; \ ``ipv6``; \ ``unix``
         Force using the specified IP version.
 
-    ``password=<secret>``
+    ``password=<string>``
         Set the password you need to authenticate.
 
+    ``password-secret=<secret-id>``
+        Set the ID of the ``secret`` object containing the password
+        you need to authenticate.
+
     ``sasl``
         Require that the client use SASL to authenticate with the spice.
         The exact choice of authentication method used is controlled
diff --git a/ui/spice-core.c b/ui/spice-core.c
index beee932f55..353848b244 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -34,6 +34,7 @@
 #include "qapi/qapi-events-ui.h"
 #include "qemu/notify.h"
 #include "qemu/option.h"
+#include "crypto/secret_common.h"
 #include "migration/misc.h"
 #include "hw/pci/pci_bus.h"
 #include "ui/spice-display.h"
@@ -415,6 +416,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "disable-ticketing",
             .type = QEMU_OPT_BOOL,
@@ -636,7 +640,9 @@ void qemu_spice_display_init_done(void)
 static void qemu_spice_init(void)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
-    const char *password, *str, *x509_dir, *addr,
+    char *password = NULL;
+    const char *passwordSecret;
+    const char *str, *x509_dir, *addr,
         *x509_key_password = NULL,
         *x509_dh_file = NULL,
         *tls_ciphers = NULL;
@@ -663,7 +669,24 @@ static void qemu_spice_init(void)
         error_report("spice tls-port is out of range");
         exit(1);
     }
-    password = qemu_opt_get(opts, "password");
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        Error *local_err = NULL;
+        if (qemu_opt_get(opts, "password")) {
+            error_report("'password' option is mutually exclusive with "
+                         "'password-secret'");
+            exit(1);
+        }
+        password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                 &local_err);
+        if (!password) {
+            error_report_err(local_err);
+            exit(1);
+        }
+    } else {
+        str = qemu_opt_get(opts, "password");
+        password = g_strdup(str);
+    }
 
     if (tls_port) {
         x509_dir = qemu_opt_get(opts, "x509-dir");
@@ -809,6 +832,7 @@ static void qemu_spice_init(void)
     g_free(x509_key_file);
     g_free(x509_cert_file);
     g_free(x509_cacert_file);
+    g_free(password);
 
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
-- 
2.29.2

[PATCH 3/4] ui: deprecate "password" option for SPICE server

[Daniel P. Berrangé]

With the new "password-secret" option, there is no reason to use the old
inecure "password" option with -spice, so it can be deprecated.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/system/deprecated.rst | 8 ++++++++
 qemu-options.hx            | 4 ++++
 ui/spice-core.c            | 4 ++++
 3 files changed, 16 insertions(+)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e..57ff9f47cc 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,14 @@ library enabled as a cryptography provider.
 Neither the ``nettle`` library, or the built-in cryptography provider are
 supported on FIPS enabled hosts.
 
+``-spice password=string`` (since 6.0)
+''''''''''''''''''''''''''''''''''''''
+
+This option is insecure because the SPICE password remains visible in
+the process listing. This is replaced by the new ``password-secret``
+option which lets the password be securely provided on the command
+line using a ``secret`` object instance.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/qemu-options.hx b/qemu-options.hx
index ff4ef3b708..4833bd59cf 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1926,6 +1926,10 @@ SRST
     ``password=<string>``
         Set the password you need to authenticate.
 
+        This option is deprecated and insecure because it leaves the
+        password visible in the process listing. Use ``password-secret``
+        instead.
+
     ``password-secret=<secret-id>``
         Set the ID of the ``secret`` object containing the password
         you need to authenticate.
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 353848b244..5e00e31457 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -685,6 +685,10 @@ static void qemu_spice_init(void)
         }
     } else {
         str = qemu_opt_get(opts, "password");
+        if (str) {
+            warn_report("'password' option is deprecated and insecure, "
+                        "use 'password-secret' instead");
+        }
         password = g_strdup(str);
     }
 
-- 
2.29.2

[PATCH 4/4] ui, monitor: remove deprecated VNC ACL option and HMP commands

[Daniel P. Berrangé]

The VNC ACL concept has been replaced by the pluggable "authz" framework
which does not use monitor commands.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/system/deprecated.rst       |  16 ---
 docs/system/removed-features.rst |  13 +++
 hmp-commands.hx                  |  76 -------------
 monitor/misc.c                   | 187 -------------------------------
 ui/vnc.c                         |  38 -------
 5 files changed, 13 insertions(+), 317 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 57ff9f47cc..beed4b4f02 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -37,12 +37,6 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
-``-vnc acl`` (since 4.0.0)
-''''''''''''''''''''''''''
-
-The ``acl`` option to the ``-vnc`` argument has been replaced
-by the ``tls-authz`` and ``sasl-authz`` options.
-
 ``QEMU_AUDIO_`` environment variables and ``-audio-help`` (since 4.0)
 '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
@@ -262,16 +256,6 @@ Use the more generic commands ``block-export-add`` and ``block-export-del``
 instead.  As part of this deprecation, where ``nbd-server-add`` used a
 single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
 
-Human Monitor Protocol (HMP) commands
--------------------------------------
-
-``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, ``acl_remove`` (since 4.0.0)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
-
-The ``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, and
-``acl_remove`` commands are deprecated with no replacement. Authorization
-for VNC should be performed using the pluggable QAuthZ objects.
-
 System emulator CPUS
 --------------------
 
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index c8481cafbd..0424b9a89d 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -38,6 +38,12 @@ or ``-display default,show-cursor=on`` instead.
 QEMU 5.0 introduced an alternative syntax to specify the size of the translation
 block cache, ``-accel tcg,tb-size=``.
 
+``-vnc acl`` (removed in 6.0)
+'''''''''''''''''''''''''''''
+
+The ``acl`` option to the ``-vnc`` argument has been replaced
+by the ``tls-authz`` and ``sasl-authz`` options.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
@@ -79,6 +85,13 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
 No replacement.  The ``change vnc password`` and ``change DEVICE MEDIUM``
 commands are not affected.
 
+``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, ``acl_remove`` (removed in 6.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+The ``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, and
+``acl_remove`` commands were removed with no replacement. Authorization
+for VNC should be performed using the pluggable QAuthZ objects.
+
 Guest Emulator ISAs
 -------------------
 
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..b500b8526d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1433,82 +1433,6 @@ SRST
   Change watchdog action.
 ERST
 
-    {
-        .name       = "acl_show",
-        .args_type  = "aclname:s",
-        .params     = "aclname",
-        .help       = "list rules in the access control list",
-        .cmd        = hmp_acl_show,
-    },
-
-SRST
-``acl_show`` *aclname*
-  List all the matching rules in the access control list, and the default
-  policy. There are currently two named access control lists,
-  *vnc.x509dname* and *vnc.username* matching on the x509 client
-  certificate distinguished name, and SASL username respectively.
-ERST
-
-    {
-        .name       = "acl_policy",
-        .args_type  = "aclname:s,policy:s",
-        .params     = "aclname allow|deny",
-        .help       = "set default access control list policy",
-        .cmd        = hmp_acl_policy,
-    },
-
-SRST
-``acl_policy`` *aclname* ``allow|deny``
-  Set the default access control list policy, used in the event that
-  none of the explicit rules match. The default policy at startup is
-  always ``deny``.
-ERST
-
-    {
-        .name       = "acl_add",
-        .args_type  = "aclname:s,match:s,policy:s,index:i?",
-        .params     = "aclname match allow|deny [index]",
-        .help       = "add a match rule to the access control list",
-        .cmd        = hmp_acl_add,
-    },
-
-SRST
-``acl_add`` *aclname* *match* ``allow|deny`` [*index*]
-  Add a match rule to the access control list, allowing or denying access.
-  The match will normally be an exact username or x509 distinguished name,
-  but can optionally include wildcard globs. eg ``*@EXAMPLE.COM`` to
-  allow all users in the ``EXAMPLE.COM`` kerberos realm. The match will
-  normally be appended to the end of the ACL, but can be inserted
-  earlier in the list if the optional *index* parameter is supplied.
-ERST
-
-    {
-        .name       = "acl_remove",
-        .args_type  = "aclname:s,match:s",
-        .params     = "aclname match",
-        .help       = "remove a match rule from the access control list",
-        .cmd        = hmp_acl_remove,
-    },
-
-SRST
-``acl_remove`` *aclname* *match*
-  Remove the specified match rule from the access control list.
-ERST
-
-    {
-        .name       = "acl_reset",
-        .args_type  = "aclname:s",
-        .params     = "aclname",
-        .help       = "reset the access control list",
-        .cmd        = hmp_acl_reset,
-    },
-
-SRST
-``acl_reset`` *aclname*
-  Remove all matches from the access control list, and set the default
-  policy back to ``deny``.
-ERST
-
     {
         .name       = "nbd_server_start",
         .args_type  = "all:-a,writable:-w,uri:s",
diff --git a/monitor/misc.c b/monitor/misc.c
index a7650ed747..d9ed2bacef 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1045,193 +1045,6 @@ static void hmp_wavcapture(Monitor *mon, const QDict *qdict)
     QLIST_INSERT_HEAD (&capture_head, s, entries);
 }
 
-static QAuthZList *find_auth(Monitor *mon, const char *name)
-{
-    Object *obj;
-    Object *container;
-
-    container = object_get_objects_root();
-    obj = object_resolve_path_component(container, name);
-    if (!obj) {
-        monitor_printf(mon, "acl: unknown list '%s'\n", name);
-        return NULL;
-    }
-
-    return QAUTHZ_LIST(obj);
-}
-
-static bool warn_acl;
-static void hmp_warn_acl(void)
-{
-    if (warn_acl) {
-        return;
-    }
-    error_report("The acl_show, acl_reset, acl_policy, acl_add, acl_remove "
-                 "commands are deprecated with no replacement. Authorization "
-                 "for VNC should be performed using the pluggable QAuthZ "
-                 "objects");
-    warn_acl = true;
-}
-
-static void hmp_acl_show(Monitor *mon, const QDict *qdict)
-{
-    const char *aclname = qdict_get_str(qdict, "aclname");
-    QAuthZList *auth = find_auth(mon, aclname);
-    QAuthZListRuleList *rules;
-    size_t i = 0;
-
-    hmp_warn_acl();
-
-    if (!auth) {
-        return;
-    }
-
-    monitor_printf(mon, "policy: %s\n",
-                   QAuthZListPolicy_str(auth->policy));
-
-    rules = auth->rules;
-    while (rules) {
-        QAuthZListRule *rule = rules->value;
-        i++;
-        monitor_printf(mon, "%zu: %s %s\n", i,
-                       QAuthZListPolicy_str(rule->policy),
-                       rule->match);
-        rules = rules->next;
-    }
-}
-
-static void hmp_acl_reset(Monitor *mon, const QDict *qdict)
-{
-    const char *aclname = qdict_get_str(qdict, "aclname");
-    QAuthZList *auth = find_auth(mon, aclname);
-
-    hmp_warn_acl();
-
-    if (!auth) {
-        return;
-    }
-
-    auth->policy = QAUTHZ_LIST_POLICY_DENY;
-    qapi_free_QAuthZListRuleList(auth->rules);
-    auth->rules = NULL;
-    monitor_printf(mon, "acl: removed all rules\n");
-}
-
-static void hmp_acl_policy(Monitor *mon, const QDict *qdict)
-{
-    const char *aclname = qdict_get_str(qdict, "aclname");
-    const char *policy = qdict_get_str(qdict, "policy");
-    QAuthZList *auth = find_auth(mon, aclname);
-    int val;
-    Error *err = NULL;
-
-    hmp_warn_acl();
-
-    if (!auth) {
-        return;
-    }
-
-    val = qapi_enum_parse(&QAuthZListPolicy_lookup,
-                          policy,
-                          QAUTHZ_LIST_POLICY_DENY,
-                          &err);
-    if (err) {
-        error_free(err);
-        monitor_printf(mon, "acl: unknown policy '%s', "
-                       "expected 'deny' or 'allow'\n", policy);
-    } else {
-        auth->policy = val;
-        if (auth->policy == QAUTHZ_LIST_POLICY_ALLOW) {
-            monitor_printf(mon, "acl: policy set to 'allow'\n");
-        } else {
-            monitor_printf(mon, "acl: policy set to 'deny'\n");
-        }
-    }
-}
-
-static QAuthZListFormat hmp_acl_get_format(const char *match)
-{
-    if (strchr(match, '*')) {
-        return QAUTHZ_LIST_FORMAT_GLOB;
-    } else {
-        return QAUTHZ_LIST_FORMAT_EXACT;
-    }
-}
-
-static void hmp_acl_add(Monitor *mon, const QDict *qdict)
-{
-    const char *aclname = qdict_get_str(qdict, "aclname");
-    const char *match = qdict_get_str(qdict, "match");
-    const char *policystr = qdict_get_str(qdict, "policy");
-    int has_index = qdict_haskey(qdict, "index");
-    int index = qdict_get_try_int(qdict, "index", -1);
-    QAuthZList *auth = find_auth(mon, aclname);
-    Error *err = NULL;
-    QAuthZListPolicy policy;
-    QAuthZListFormat format;
-    size_t i = 0;
-
-    hmp_warn_acl();
-
-    if (!auth) {
-        return;
-    }
-
-    policy = qapi_enum_parse(&QAuthZListPolicy_lookup,
-                             policystr,
-                             QAUTHZ_LIST_POLICY_DENY,
-                             &err);
-    if (err) {
-        error_free(err);
-        monitor_printf(mon, "acl: unknown policy '%s', "
-                       "expected 'deny' or 'allow'\n", policystr);
-        return;
-    }
-
-    format = hmp_acl_get_format(match);
-
-    if (has_index && index == 0) {
-        monitor_printf(mon, "acl: unable to add acl entry\n");
-        return;
-    }
-
-    if (has_index) {
-        i = qauthz_list_insert_rule(auth, match, policy,
-                                    format, index - 1, &err);
-    } else {
-        i = qauthz_list_append_rule(auth, match, policy,
-                                    format, &err);
-    }
-    if (err) {
-        monitor_printf(mon, "acl: unable to add rule: %s",
-                       error_get_pretty(err));
-        error_free(err);
-    } else {
-        monitor_printf(mon, "acl: added rule at position %zu\n", i + 1);
-    }
-}
-
-static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
-{
-    const char *aclname = qdict_get_str(qdict, "aclname");
-    const char *match = qdict_get_str(qdict, "match");
-    QAuthZList *auth = find_auth(mon, aclname);
-    ssize_t i = 0;
-
-    hmp_warn_acl();
-
-    if (!auth) {
-        return;
-    }
-
-    i = qauthz_list_delete_rule(auth, match);
-    if (i >= 0) {
-        monitor_printf(mon, "acl: removed rule at position %zu\n", i + 1);
-    } else {
-        monitor_printf(mon, "acl: no matching acl entry\n");
-    }
-}
-
 void qmp_getfd(const char *fdname, Error **errp)
 {
     Monitor *cur_mon = monitor_cur();
diff --git a/ui/vnc.c b/ui/vnc.c
index 77e07ac351..5aea2652d4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3485,9 +3485,6 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "sasl",
             .type = QEMU_OPT_BOOL,
-        },{
-            .name = "acl",
-            .type = QEMU_OPT_BOOL,
         },{
             .name = "tls-authz",
             .type = QEMU_OPT_STRING,
@@ -3939,7 +3936,6 @@ void vnc_display_open(const char *id, Error **errp)
     bool reverse = false;
     const char *credid;
     bool sasl = false;
-    int acl = 0;
     const char *tlsauthz;
     const char *saslauthz;
     int lock_key_sync = 1;
@@ -4031,29 +4027,13 @@ void vnc_display_open(const char *id, Error **errp)
             goto fail;
         }
     }
-    if (qemu_opt_get(opts, "acl")) {
-        error_report("The 'acl' option to -vnc is deprecated. "
-                     "Please use the 'tls-authz' and 'sasl-authz' "
-                     "options instead");
-    }
-    acl = qemu_opt_get_bool(opts, "acl", false);
     tlsauthz = qemu_opt_get(opts, "tls-authz");
-    if (acl && tlsauthz) {
-        error_setg(errp, "'acl' option is mutually exclusive with the "
-                   "'tls-authz' option");
-        goto fail;
-    }
     if (tlsauthz && !vd->tlscreds) {
         error_setg(errp, "'tls-authz' provided but TLS is not enabled");
         goto fail;
     }
 
     saslauthz = qemu_opt_get(opts, "sasl-authz");
-    if (acl && saslauthz) {
-        error_setg(errp, "'acl' option is mutually exclusive with the "
-                   "'sasl-authz' option");
-        goto fail;
-    }
     if (saslauthz && !sasl) {
         error_setg(errp, "'sasl-authz' provided but SASL auth is not enabled");
         goto fail;
@@ -4091,29 +4071,11 @@ void vnc_display_open(const char *id, Error **errp)
 
     if (tlsauthz) {
         vd->tlsauthzid = g_strdup(tlsauthz);
-    } else if (acl) {
-        if (strcmp(vd->id, "default") == 0) {
-            vd->tlsauthzid = g_strdup("vnc.x509dname");
-        } else {
-            vd->tlsauthzid = g_strdup_printf("vnc.%s.x509dname", vd->id);
-        }
-        vd->tlsauthz = QAUTHZ(qauthz_list_new(vd->tlsauthzid,
-                                              QAUTHZ_LIST_POLICY_DENY,
-                                              &error_abort));
     }
 #ifdef CONFIG_VNC_SASL
     if (sasl) {
         if (saslauthz) {
             vd->sasl.authzid = g_strdup(saslauthz);
-        } else if (acl) {
-            if (strcmp(vd->id, "default") == 0) {
-                vd->sasl.authzid = g_strdup("vnc.username");
-            } else {
-                vd->sasl.authzid = g_strdup_printf("vnc.%s.username", vd->id);
-            }
-            vd->sasl.authz = QAUTHZ(qauthz_list_new(vd->sasl.authzid,
-                                                    QAUTHZ_LIST_POLICY_DENY,
-                                                    &error_abort));
         }
     }
 #endif
-- 
2.29.2

Re: [PATCH 4/4] ui, monitor: remove deprecated VNC ACL option and HMP commands

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> The VNC ACL concept has been replaced by the pluggable "authz" framework
> which does not use monitor commands.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

This looks OK to me, so:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

however, can you explicitly add an example of the qauthz syntax; while
you say you should use QAuthZ, nothing in docs/ describes it/uses that
name.

Dave

> ---
>  docs/system/deprecated.rst       |  16 ---
>  docs/system/removed-features.rst |  13 +++
>  hmp-commands.hx                  |  76 -------------
>  monitor/misc.c                   | 187 -------------------------------
>  ui/vnc.c                         |  38 -------
>  5 files changed, 13 insertions(+), 317 deletions(-)
> 
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 57ff9f47cc..beed4b4f02 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/deprecated.rst
> @@ -37,12 +37,6 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> -``-vnc acl`` (since 4.0.0)
> -''''''''''''''''''''''''''
> -
> -The ``acl`` option to the ``-vnc`` argument has been replaced
> -by the ``tls-authz`` and ``sasl-authz`` options.
> -
>  ``QEMU_AUDIO_`` environment variables and ``-audio-help`` (since 4.0)
>  '''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
>  
> @@ -262,16 +256,6 @@ Use the more generic commands ``block-export-add`` and ``block-export-del``
>  instead.  As part of this deprecation, where ``nbd-server-add`` used a
>  single ``bitmap``, the new ``block-export-add`` uses a list of ``bitmaps``.
>  
> -Human Monitor Protocol (HMP) commands
> --------------------------------------
> -
> -``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, ``acl_remove`` (since 4.0.0)
> -''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> -
> -The ``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, and
> -``acl_remove`` commands are deprecated with no replacement. Authorization
> -for VNC should be performed using the pluggable QAuthZ objects.
> -
>  System emulator CPUS
>  --------------------
>  
> diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
> index c8481cafbd..0424b9a89d 100644
> --- a/docs/system/removed-features.rst
> +++ b/docs/system/removed-features.rst
> @@ -38,6 +38,12 @@ or ``-display default,show-cursor=on`` instead.
>  QEMU 5.0 introduced an alternative syntax to specify the size of the translation
>  block cache, ``-accel tcg,tb-size=``.
>  
> +``-vnc acl`` (removed in 6.0)
> +'''''''''''''''''''''''''''''
> +
> +The ``acl`` option to the ``-vnc`` argument has been replaced
> +by the ``tls-authz`` and ``sasl-authz`` options.
> +
>  QEMU Machine Protocol (QMP) commands
>  ------------------------------------
>  
> @@ -79,6 +85,13 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
>  No replacement.  The ``change vnc password`` and ``change DEVICE MEDIUM``
>  commands are not affected.
>  
> +``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, ``acl_remove`` (removed in 6.0)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +The ``acl_show``, ``acl_reset``, ``acl_policy``, ``acl_add``, and
> +``acl_remove`` commands were removed with no replacement. Authorization
> +for VNC should be performed using the pluggable QAuthZ objects.
> +
>  Guest Emulator ISAs
>  -------------------
>  
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d4001f9c5d..b500b8526d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1433,82 +1433,6 @@ SRST
>    Change watchdog action.
>  ERST
>  
> -    {
> -        .name       = "acl_show",
> -        .args_type  = "aclname:s",
> -        .params     = "aclname",
> -        .help       = "list rules in the access control list",
> -        .cmd        = hmp_acl_show,
> -    },
> -
> -SRST
> -``acl_show`` *aclname*
> -  List all the matching rules in the access control list, and the default
> -  policy. There are currently two named access control lists,
> -  *vnc.x509dname* and *vnc.username* matching on the x509 client
> -  certificate distinguished name, and SASL username respectively.
> -ERST
> -
> -    {
> -        .name       = "acl_policy",
> -        .args_type  = "aclname:s,policy:s",
> -        .params     = "aclname allow|deny",
> -        .help       = "set default access control list policy",
> -        .cmd        = hmp_acl_policy,
> -    },
> -
> -SRST
> -``acl_policy`` *aclname* ``allow|deny``
> -  Set the default access control list policy, used in the event that
> -  none of the explicit rules match. The default policy at startup is
> -  always ``deny``.
> -ERST
> -
> -    {
> -        .name       = "acl_add",
> -        .args_type  = "aclname:s,match:s,policy:s,index:i?",
> -        .params     = "aclname match allow|deny [index]",
> -        .help       = "add a match rule to the access control list",
> -        .cmd        = hmp_acl_add,
> -    },
> -
> -SRST
> -``acl_add`` *aclname* *match* ``allow|deny`` [*index*]
> -  Add a match rule to the access control list, allowing or denying access.
> -  The match will normally be an exact username or x509 distinguished name,
> -  but can optionally include wildcard globs. eg ``*@EXAMPLE.COM`` to
> -  allow all users in the ``EXAMPLE.COM`` kerberos realm. The match will
> -  normally be appended to the end of the ACL, but can be inserted
> -  earlier in the list if the optional *index* parameter is supplied.
> -ERST
> -
> -    {
> -        .name       = "acl_remove",
> -        .args_type  = "aclname:s,match:s",
> -        .params     = "aclname match",
> -        .help       = "remove a match rule from the access control list",
> -        .cmd        = hmp_acl_remove,
> -    },
> -
> -SRST
> -``acl_remove`` *aclname* *match*
> -  Remove the specified match rule from the access control list.
> -ERST
> -
> -    {
> -        .name       = "acl_reset",
> -        .args_type  = "aclname:s",
> -        .params     = "aclname",
> -        .help       = "reset the access control list",
> -        .cmd        = hmp_acl_reset,
> -    },
> -
> -SRST
> -``acl_reset`` *aclname*
> -  Remove all matches from the access control list, and set the default
> -  policy back to ``deny``.
> -ERST
> -
>      {
>          .name       = "nbd_server_start",
>          .args_type  = "all:-a,writable:-w,uri:s",
> diff --git a/monitor/misc.c b/monitor/misc.c
> index a7650ed747..d9ed2bacef 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -1045,193 +1045,6 @@ static void hmp_wavcapture(Monitor *mon, const QDict *qdict)
>      QLIST_INSERT_HEAD (&capture_head, s, entries);
>  }
>  
> -static QAuthZList *find_auth(Monitor *mon, const char *name)
> -{
> -    Object *obj;
> -    Object *container;
> -
> -    container = object_get_objects_root();
> -    obj = object_resolve_path_component(container, name);
> -    if (!obj) {
> -        monitor_printf(mon, "acl: unknown list '%s'\n", name);
> -        return NULL;
> -    }
> -
> -    return QAUTHZ_LIST(obj);
> -}
> -
> -static bool warn_acl;
> -static void hmp_warn_acl(void)
> -{
> -    if (warn_acl) {
> -        return;
> -    }
> -    error_report("The acl_show, acl_reset, acl_policy, acl_add, acl_remove "
> -                 "commands are deprecated with no replacement. Authorization "
> -                 "for VNC should be performed using the pluggable QAuthZ "
> -                 "objects");
> -    warn_acl = true;
> -}
> -
> -static void hmp_acl_show(Monitor *mon, const QDict *qdict)
> -{
> -    const char *aclname = qdict_get_str(qdict, "aclname");
> -    QAuthZList *auth = find_auth(mon, aclname);
> -    QAuthZListRuleList *rules;
> -    size_t i = 0;
> -
> -    hmp_warn_acl();
> -
> -    if (!auth) {
> -        return;
> -    }
> -
> -    monitor_printf(mon, "policy: %s\n",
> -                   QAuthZListPolicy_str(auth->policy));
> -
> -    rules = auth->rules;
> -    while (rules) {
> -        QAuthZListRule *rule = rules->value;
> -        i++;
> -        monitor_printf(mon, "%zu: %s %s\n", i,
> -                       QAuthZListPolicy_str(rule->policy),
> -                       rule->match);
> -        rules = rules->next;
> -    }
> -}
> -
> -static void hmp_acl_reset(Monitor *mon, const QDict *qdict)
> -{
> -    const char *aclname = qdict_get_str(qdict, "aclname");
> -    QAuthZList *auth = find_auth(mon, aclname);
> -
> -    hmp_warn_acl();
> -
> -    if (!auth) {
> -        return;
> -    }
> -
> -    auth->policy = QAUTHZ_LIST_POLICY_DENY;
> -    qapi_free_QAuthZListRuleList(auth->rules);
> -    auth->rules = NULL;
> -    monitor_printf(mon, "acl: removed all rules\n");
> -}
> -
> -static void hmp_acl_policy(Monitor *mon, const QDict *qdict)
> -{
> -    const char *aclname = qdict_get_str(qdict, "aclname");
> -    const char *policy = qdict_get_str(qdict, "policy");
> -    QAuthZList *auth = find_auth(mon, aclname);
> -    int val;
> -    Error *err = NULL;
> -
> -    hmp_warn_acl();
> -
> -    if (!auth) {
> -        return;
> -    }
> -
> -    val = qapi_enum_parse(&QAuthZListPolicy_lookup,
> -                          policy,
> -                          QAUTHZ_LIST_POLICY_DENY,
> -                          &err);
> -    if (err) {
> -        error_free(err);
> -        monitor_printf(mon, "acl: unknown policy '%s', "
> -                       "expected 'deny' or 'allow'\n", policy);
> -    } else {
> -        auth->policy = val;
> -        if (auth->policy == QAUTHZ_LIST_POLICY_ALLOW) {
> -            monitor_printf(mon, "acl: policy set to 'allow'\n");
> -        } else {
> -            monitor_printf(mon, "acl: policy set to 'deny'\n");
> -        }
> -    }
> -}
> -
> -static QAuthZListFormat hmp_acl_get_format(const char *match)
> -{
> -    if (strchr(match, '*')) {
> -        return QAUTHZ_LIST_FORMAT_GLOB;
> -    } else {
> -        return QAUTHZ_LIST_FORMAT_EXACT;
> -    }
> -}
> -
> -static void hmp_acl_add(Monitor *mon, const QDict *qdict)
> -{
> -    const char *aclname = qdict_get_str(qdict, "aclname");
> -    const char *match = qdict_get_str(qdict, "match");
> -    const char *policystr = qdict_get_str(qdict, "policy");
> -    int has_index = qdict_haskey(qdict, "index");
> -    int index = qdict_get_try_int(qdict, "index", -1);
> -    QAuthZList *auth = find_auth(mon, aclname);
> -    Error *err = NULL;
> -    QAuthZListPolicy policy;
> -    QAuthZListFormat format;
> -    size_t i = 0;
> -
> -    hmp_warn_acl();
> -
> -    if (!auth) {
> -        return;
> -    }
> -
> -    policy = qapi_enum_parse(&QAuthZListPolicy_lookup,
> -                             policystr,
> -                             QAUTHZ_LIST_POLICY_DENY,
> -                             &err);
> -    if (err) {
> -        error_free(err);
> -        monitor_printf(mon, "acl: unknown policy '%s', "
> -                       "expected 'deny' or 'allow'\n", policystr);
> -        return;
> -    }
> -
> -    format = hmp_acl_get_format(match);
> -
> -    if (has_index && index == 0) {
> -        monitor_printf(mon, "acl: unable to add acl entry\n");
> -        return;
> -    }
> -
> -    if (has_index) {
> -        i = qauthz_list_insert_rule(auth, match, policy,
> -                                    format, index - 1, &err);
> -    } else {
> -        i = qauthz_list_append_rule(auth, match, policy,
> -                                    format, &err);
> -    }
> -    if (err) {
> -        monitor_printf(mon, "acl: unable to add rule: %s",
> -                       error_get_pretty(err));
> -        error_free(err);
> -    } else {
> -        monitor_printf(mon, "acl: added rule at position %zu\n", i + 1);
> -    }
> -}
> -
> -static void hmp_acl_remove(Monitor *mon, const QDict *qdict)
> -{
> -    const char *aclname = qdict_get_str(qdict, "aclname");
> -    const char *match = qdict_get_str(qdict, "match");
> -    QAuthZList *auth = find_auth(mon, aclname);
> -    ssize_t i = 0;
> -
> -    hmp_warn_acl();
> -
> -    if (!auth) {
> -        return;
> -    }
> -
> -    i = qauthz_list_delete_rule(auth, match);
> -    if (i >= 0) {
> -        monitor_printf(mon, "acl: removed rule at position %zu\n", i + 1);
> -    } else {
> -        monitor_printf(mon, "acl: no matching acl entry\n");
> -    }
> -}
> -
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
>      Monitor *cur_mon = monitor_cur();
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 77e07ac351..5aea2652d4 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3485,9 +3485,6 @@ static QemuOptsList qemu_vnc_opts = {
>          },{
>              .name = "sasl",
>              .type = QEMU_OPT_BOOL,
> -        },{
> -            .name = "acl",
> -            .type = QEMU_OPT_BOOL,
>          },{
>              .name = "tls-authz",
>              .type = QEMU_OPT_STRING,
> @@ -3939,7 +3936,6 @@ void vnc_display_open(const char *id, Error **errp)
>      bool reverse = false;
>      const char *credid;
>      bool sasl = false;
> -    int acl = 0;
>      const char *tlsauthz;
>      const char *saslauthz;
>      int lock_key_sync = 1;
> @@ -4031,29 +4027,13 @@ void vnc_display_open(const char *id, Error **errp)
>              goto fail;
>          }
>      }
> -    if (qemu_opt_get(opts, "acl")) {
> -        error_report("The 'acl' option to -vnc is deprecated. "
> -                     "Please use the 'tls-authz' and 'sasl-authz' "
> -                     "options instead");
> -    }
> -    acl = qemu_opt_get_bool(opts, "acl", false);
>      tlsauthz = qemu_opt_get(opts, "tls-authz");
> -    if (acl && tlsauthz) {
> -        error_setg(errp, "'acl' option is mutually exclusive with the "
> -                   "'tls-authz' option");
> -        goto fail;
> -    }
>      if (tlsauthz && !vd->tlscreds) {
>          error_setg(errp, "'tls-authz' provided but TLS is not enabled");
>          goto fail;
>      }
>  
>      saslauthz = qemu_opt_get(opts, "sasl-authz");
> -    if (acl && saslauthz) {
> -        error_setg(errp, "'acl' option is mutually exclusive with the "
> -                   "'sasl-authz' option");
> -        goto fail;
> -    }
>      if (saslauthz && !sasl) {
>          error_setg(errp, "'sasl-authz' provided but SASL auth is not enabled");
>          goto fail;
> @@ -4091,29 +4071,11 @@ void vnc_display_open(const char *id, Error **errp)
>  
>      if (tlsauthz) {
>          vd->tlsauthzid = g_strdup(tlsauthz);
> -    } else if (acl) {
> -        if (strcmp(vd->id, "default") == 0) {
> -            vd->tlsauthzid = g_strdup("vnc.x509dname");
> -        } else {
> -            vd->tlsauthzid = g_strdup_printf("vnc.%s.x509dname", vd->id);
> -        }
> -        vd->tlsauthz = QAUTHZ(qauthz_list_new(vd->tlsauthzid,
> -                                              QAUTHZ_LIST_POLICY_DENY,
> -                                              &error_abort));
>      }
>  #ifdef CONFIG_VNC_SASL
>      if (sasl) {
>          if (saslauthz) {
>              vd->sasl.authzid = g_strdup(saslauthz);
> -        } else if (acl) {
> -            if (strcmp(vd->id, "default") == 0) {
> -                vd->sasl.authzid = g_strdup("vnc.username");
> -            } else {
> -                vd->sasl.authzid = g_strdup_printf("vnc.%s.username", vd->id);
> -            }
> -            vd->sasl.authz = QAUTHZ(qauthz_list_new(vd->sasl.authzid,
> -                                                    QAUTHZ_LIST_POLICY_DENY,
> -                                                    &error_abort));
>          }
>      }
>  #endif
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/4] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Daniel P. Berrangé]

Ping

On Fri, Feb 19, 2021 at 06:45:52PM +0000, Daniel P. Berrangé wrote:
> This fixes a long standing limitation of the VNC/SPICE code which was
> unable to securely accept passswords on the CLI, instead requiring use
> of separate monitor commands after startup.
> 
> This takes the opportunity to also remove previously deprecated ACL
> functionality from VNC.
> 
> Daniel P. Berrangé (4):
>   ui: introduce "password-secret" option for VNC servers
>   ui: introduce "password-secret" option for SPICE server
>   ui: deprecate "password" option for SPICE server
>   ui, monitor: remove deprecated VNC ACL option and HMP commands
> 
>  docs/system/deprecated.rst       |  24 ++--
>  docs/system/removed-features.rst |  13 +++
>  hmp-commands.hx                  |  76 -------------
>  monitor/misc.c                   | 187 -------------------------------
>  qemu-options.hx                  |  17 ++-
>  ui/spice-core.c                  |  32 +++++-
>  ui/vnc.c                         |  61 ++++------
>  7 files changed, 88 insertions(+), 322 deletions(-)
> 
> -- 
> 2.29.2
> 

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

Re: [PATCH 0/4] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Gerd Hoffmann]

On Thu, Mar 11, 2021 at 10:37:45AM +0000, Daniel P. Berrangé wrote:
> Ping

Looks good but doesn't apply cleanly, can you rebase?

(current ui queue is gitlab.com/kraxel/qemu queue/ui, there are no
spice/vnc changes queued so it probably doesn't make a difference
compared to latest master)

take care,
  Gerd

Re: [PATCH 0/4] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Daniel P. Berrangé]

On Thu, Mar 11, 2021 at 12:13:42PM +0100, Gerd Hoffmann wrote:
> On Thu, Mar 11, 2021 at 10:37:45AM +0000, Daniel P. Berrangé wrote:
> > Ping
> 
> Looks good but doesn't apply cleanly, can you rebase?
> 
> (current ui queue is gitlab.com/kraxel/qemu queue/ui, there are no
> spice/vnc changes queued so it probably doesn't make a difference
> compared to latest master)

Sure, will rebase to master and check it also applies to this queue
cleanly.


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