Re: [PATCH v6 14/27] ui: add proper error reporting for password changes

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Neither the VNC or SPICE code for password changes provides error
> reporting at source, leading the callers to report a largely useless
> generic error message.
>
> Fixing this removes one of the two remaining needs for the undesirable
> error_printf_unless_qmp() method.
>
> While fixing this the error message hint is improved to recommend the
> 'password-secret' option which allows securely passing a password at
> startup.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/ui/console.h                 |  2 +-
>  include/ui/qemu-spice-module.h       |  3 ++-
>  tests/functional/generic/test_vnc.py |  4 ++--
>  ui/spice-core.c                      | 25 ++++++++++++++++++-------
>  ui/spice-module.c                    |  7 ++++---
>  ui/ui-qmp-cmds.c                     | 19 ++++++-------------
>  ui/vnc-stubs.c                       |  6 +++---
>  ui/vnc.c                             | 10 +++++++---
>  8 files changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 98feaa58bd..3677a9d334 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -457,7 +457,7 @@ void qemu_display_help(void);
>  void vnc_display_init(const char *id, Error **errp);
>  void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
> -int vnc_display_password(const char *id, const char *password);
> +int vnc_display_password(const char *id, const char *password, Error **errp);
>  int vnc_display_pw_expire(const char *id, time_t expires);
>  void vnc_parse(const char *str);
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> diff --git a/include/ui/qemu-spice-module.h b/include/ui/qemu-spice-module.h
> index 1f22d557ea..072efa0c83 100644
> --- a/include/ui/qemu-spice-module.h
> +++ b/include/ui/qemu-spice-module.h
> @@ -29,7 +29,8 @@ struct QemuSpiceOps {
>      void (*display_init)(void);
>      int (*migrate_info)(const char *h, int p, int t, const char *s);
>      int (*set_passwd)(const char *passwd,
> -                      bool fail_if_connected, bool disconnect_if_connected);
> +                      bool fail_if_connected, bool disconnect_if_connected,
> +                      Error **errp);
>      int (*set_pw_expire)(time_t expires);
>      int (*display_add_client)(int csock, int skipauth, int tls);
>  #ifdef CONFIG_SPICE
> diff --git a/tests/functional/generic/test_vnc.py b/tests/functional/generic/test_vnc.py
> index f1dd1597cf..097f858ca1 100755
> --- a/tests/functional/generic/test_vnc.py
> +++ b/tests/functional/generic/test_vnc.py
> @@ -48,7 +48,7 @@ def test_no_vnc_change_password(self):
>          self.assertEqual(set_password_response['error']['class'],
>                           'GenericError')
>          self.assertEqual(set_password_response['error']['desc'],
> -                         'Could not set password')
> +                         'No VNC display is present');
>  
>      def launch_guarded(self):
>          try:
> @@ -73,7 +73,7 @@ def test_change_password_requires_a_password(self):
>          self.assertEqual(set_password_response['error']['class'],
>                           'GenericError')
>          self.assertEqual(set_password_response['error']['desc'],
> -                         'Could not set password')
> +                         'VNC password authentication is disabled')
>  
>      def test_change_password(self):
>          self.set_machine('none')
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 8a6050f4ae..cdcec34f67 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -756,7 +756,7 @@ static void qemu_spice_init(void)
>                               tls_ciphers);
>      }
>      if (password) {
> -        qemu_spice.set_passwd(password, false, false);
> +        qemu_spice.set_passwd(password, false, false, NULL);

qemu_spice.set_passwd is qemu_spice_set_passwd().  It's converted to
Error below.  That conversion doesn't replace error reporting, it only
adds.  Therefore, passing NULL does not lose error reporting here.
Good.

However, why is ignoring errors okay here?  Not this patch's fault, of
course.

>      }
>      if (qemu_opt_get_bool(opts, "sasl", 0)) {
>          if (spice_server_set_sasl(spice_server, 1) == -1) {
               error_report("spice: failed to enable sasl");
               exit(1);

For what it's worth, we treat this error as fatal.

           }
           auth = "sasl";
       }
> @@ -919,8 +919,10 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
>      return qemu_spice_add_interface(&qxlin->base);
>  }
>  
> -static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
> +static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn,
> +                                 Error **errp)
>  {
> +    int ret;
>      time_t lifetime, now = time(NULL);
>      char *passwd;
>  
> @@ -934,26 +936,35 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
>          passwd = NULL;
>          lifetime = 1;
>      }
> -    return spice_server_set_ticket(spice_server, passwd, lifetime,
> -                                   fail_if_conn, disconnect_if_conn);
> +    ret = spice_server_set_ticket(spice_server, passwd, lifetime,
> +                                  fail_if_conn, disconnect_if_conn);
> +    if (ret < 0) {
> +        error_setg(errp, "Unable to set SPICE server ticket");
> +        return -1;
> +    }
> +    return 0;
>  }
>  
>  static int qemu_spice_set_passwd(const char *passwd,
> -                                 bool fail_if_conn, bool disconnect_if_conn)
> +                                 bool fail_if_conn, bool disconnect_if_conn,
> +                                 Error **errp)
>  {
>      if (strcmp(auth, "spice") != 0) {
> +        error_setg(errp, "SPICE authentication is disabled");
> +        error_append_hint(errp,
> +                          "To enable it use '-spice ...,password-secret=ID'");
>          return -1;
>      }
>  
>      g_free(auth_passwd);
>      auth_passwd = g_strdup(passwd);
> -    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
> +    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn, errp);
>  }
>  
>  static int qemu_spice_set_pw_expire(time_t expires)
>  {
>      auth_expires = expires;
> -    return qemu_spice_set_ticket(false, false);
> +    return qemu_spice_set_ticket(false, false, NULL);
>  }
>  
>  static int qemu_spice_display_add_client(int csock, int skipauth, int tls)
> diff --git a/ui/spice-module.c b/ui/spice-module.c
> index 3222335872..7651c85885 100644
> --- a/ui/spice-module.c
> +++ b/ui/spice-module.c
> @@ -45,14 +45,15 @@ static int qemu_spice_migrate_info_stub(const char *h, int p, int t,
>  
>  static int qemu_spice_set_passwd_stub(const char *passwd,
>                                        bool fail_if_connected,
> -                                      bool disconnect_if_connected)
> +                                      bool disconnect_if_connected,
> +                                      Error **errp)
>  {
> -    return -1;
> +    g_assert_not_reached();
>  }
>  
>  static int qemu_spice_set_pw_expire_stub(time_t expires)
>  {
> -    return -1;
> +    g_assert_not_reached();
>  }
>  

Makes the stubs' "just to keep the linker happy" nature obvious.  I like
it.

> static int qemu_spice_display_add_client_stub(int csock, int skipauth,
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index b49b636152..1173c82cf7 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -31,15 +31,14 @@
>  
>  void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>  {
> -    int rc;
> -
>      if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
>          if (!qemu_using_spice(errp)) {
>              return;
>          }
> -        rc = qemu_spice.set_passwd(opts->password,
> -                opts->connected == SET_PASSWORD_ACTION_FAIL,
> -                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
> +        qemu_spice.set_passwd(opts->password,
> +                              opts->connected == SET_PASSWORD_ACTION_FAIL,
> +                              opts->connected == SET_PASSWORD_ACTION_DISCONNECT,
> +                              errp);
>      } else {
>          assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
>          if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
> @@ -52,11 +51,7 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>           * Note that setting an empty password will not disable login
>           * through this interface.
>           */
> -        rc = vnc_display_password(opts->u.vnc.display, opts->password);
> -    }
> -
> -    if (rc != 0) {
> -        error_setg(errp, "Could not set password");
> +        vnc_display_password(opts->u.vnc.display, opts->password, errp);
>      }
>  }
>  
> @@ -107,9 +102,7 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>  #ifdef CONFIG_VNC
>  void qmp_change_vnc_password(const char *password, Error **errp)
>  {
> -    if (vnc_display_password(NULL, password) < 0) {
> -        error_setg(errp, "Could not set password");
> -    }
> +    vnc_display_password(NULL, password, errp);
>  }
>  #endif
>  

The QMP commands become simpler.  Nice!

> diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
> index a96bc86236..5de9bf9d70 100644
> --- a/ui/vnc-stubs.c
> +++ b/ui/vnc-stubs.c
> @@ -2,11 +2,11 @@
>  #include "ui/console.h"
>  #include "qapi/error.h"
>  
> -int vnc_display_password(const char *id, const char *password)
> +int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
> -    return -ENODEV;
> +    g_assert_not_reached();
>  }
>  int vnc_display_pw_expire(const char *id, time_t expires)
>  {
> -    return -ENODEV;
> +    g_assert_not_reached();
>  };

Like it.

> diff --git a/ui/vnc.c b/ui/vnc.c
> index a61a4f937d..833e0e2e68 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3526,16 +3526,20 @@ static void vnc_display_close(VncDisplay *vd)
>  #endif
>  }
>  
> -int vnc_display_password(const char *id, const char *password)
> +int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
>      VncDisplay *vd = vnc_display_find(id);
>  
>      if (!vd) {
> +        error_setg(errp, "No VNC display is present");
> +        error_append_hint(errp,
> +                          "To enable it, use '-vnc ...'");
>          return -EINVAL;
>      }
>      if (vd->auth == VNC_AUTH_NONE) {
> -        error_printf_unless_qmp("If you want use passwords please enable "
> -                                "password auth using '-vnc ${dpy},password'.\n");
> +        error_setg(errp, "VNC password authentication is disabled");
> +        error_append_hint(errp,
> +                          "To enable it, use '-vnc ...,password-secret=ID'");
>          return -EINVAL;
>      }

The only issue I found is not this patch's problem, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>