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