ui: add proper error reporting for password changes

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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/include/ui/console.h b/include/ui/console.h
index 98feaa58bdd1ad91f175a7a9294eefe6f1f1e03a..3677a9d334d9cd5ed51e63f70af3feb604ee6de0 100644 (file)
--- 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 1f22d557ea2fb9b82a4c8dd677d9fead2e22798d..072efa0c8348583acc8ae5870ef573d2c5f0eac5 100644 (file)
--- 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 f1dd1597cf18fd2275d895738a682e444d97de1b..097f858ca1667cefeaff612aa8bc746358903930 100755 (executable)
--- a/tests/functional/generic/test_vnc.py
+++ b/tests/functional/generic/test_vnc.py
@@ -48,7 +48,7 @@ class Vnc(QemuSystemTest):
         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 @@ class Vnc(QemuSystemTest):
         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 ee13ecc4a530e8c678dd372226f56c00f2daa530..ef1c00134fa4cf450f1abdb0e9342dc65c20a9cb 100644 (file)
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -757,7 +757,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);
     }
     if (qemu_opt_get_bool(opts, "sasl", 0)) {
         if (spice_server_set_sasl(spice_server, 1) == -1) {
@@ -920,8 +920,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;
 
@@ -935,26 +937,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 32223358722c768593e5ccf5cac9769252bf164d..7651c85885f943d92cd0a424c4a77e6d0180bbf8 100644 (file)
--- 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();
 }
 
 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 b49b6361520ba31d5365e632bee3fe16102ddba4..1173c82cf7f5ebeda9e5354463b463b9b1baa582 100644 (file)
--- 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
 

diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c
index a96bc862363c23a37dc1152e6ed094e082686f1f..5de9bf9d70cb042af600a52fbc0b0abb1646c885 100644 (file)
--- 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();
 };

diff --git a/ui/vnc.c b/ui/vnc.c
index daf5b01d34211be3fed543a714210ac5b1a6899a..8bf14cf9a728691fd182ea1b0f0b49c713a04e5f 100644 (file)
--- 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;
     }