[Qemu-devel] [PATCH 0/3] SPICE/VNC: Allow ticketing on the fly

[Qemu-devel] [PATCH 0/3] SPICE/VNC: Allow ticketing on the fly

[Michal Privoznik]

Currently, if the ticketing (password) is not set at the command
line, there's no way how to set it afterwards. Or vice versa -
disable previously set password. I think it's worth allowing
users to do that. The use case may be a teacher, who wants to
share a graphical session with students, but has to set up the
environment firstly. So he starts with a password, and then
remove it and let students in.

Michal Privoznik (3):
  qapi-schema: Make @password in set_password optional
  spice: Implement set_password without password
  vnc: Implement set_password without password

 hmp-commands.hx  |  2 +-
 hmp.c            |  3 ++-
 qapi-schema.json |  8 +++++---
 qmp-commands.hx  |  2 +-
 qmp.c            |  6 +++++-
 ui/spice-core.c  | 15 +++++++++++++--
 ui/vnc.c         | 18 ++++++++++++++----
 ui/vnc.h         |  1 +
 8 files changed, 42 insertions(+), 13 deletions(-)

-- 
2.0.5

[Qemu-devel] [PATCH 1/3] qapi-schema: Make @password in set_password optional

[Michal Privoznik]

So, imagine you've started a guest with ticketing enabled. You've set
some password to access your SPICE/VNC session. However, later you
want to give the access to somebody else's and therefore disable the
ticketing. Come on, be imaginative! Currently, there's no way how to
achieve this. And while there are two possible ways to fulfill the
goal: 1) invent new monitor command to disable ticketing, or 2) let
@password argument to 'set_password' monitor command be optional, I'm
choosing the latter. It's easier to implement, after all.

The idea behind, how this will work, is: if user issues the command
without the password field, it means they want to disable the
ticketing. Any subsequent call to the call with password field filled
in, will enable the ticketing again.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 hmp-commands.hx  | 2 +-
 hmp.c            | 3 ++-
 qapi-schema.json | 8 +++++---
 qmp-commands.hx  | 2 +-
 qmp.c            | 6 +++++-
 5 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e37bc8b..91dffb2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1576,7 +1576,7 @@ ETEXI
 
     {
         .name       = "set_password",
-        .args_type  = "protocol:s,password:s,connected:s?",
+        .args_type  = "protocol:s,password:s?,connected:s?",
         .params     = "protocol password action-if-connected",
         .help       = "set spice/vnc password",
         .mhandler.cmd = hmp_set_password,
diff --git a/hmp.c b/hmp.c
index b47f331..ce0d19e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1147,7 +1147,8 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
     const char *connected = qdict_get_try_str(qdict, "connected");
     Error *err = NULL;
 
-    qmp_set_password(protocol, password, !!connected, connected, &err);
+    qmp_set_password(protocol, !!password, password,
+                     !!connected, connected, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..a0d9b38 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1561,12 +1561,14 @@
 ##
 # @set_password:
 #
-# Sets the password of a remote display session.
+# Sets (and enables) the password of a remote display session.
+# Or, optionally since 2.2.1, if no @password is passed, disables
+# password for remote display.
 #
 # @protocol: `vnc' to modify the VNC server password
 #            `spice' to modify the Spice server password
 #
-# @password: the new password
+# @password: #optional the new password
 #
 # @connected: #optional how to handle existing clients when changing the
 #                       password.  If nothing is specified, defaults to `keep'
@@ -1580,7 +1582,7 @@
 # Since: 0.14.0
 ##
 { 'command': 'set_password',
-  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
+  'data': {'protocol': 'str', '*password': 'str', '*connected': 'str'} }
 
 ##
 # @expire_password:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..e926a4e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1753,7 +1753,7 @@ EQMP
 
     {
         .name       = "set_password",
-        .args_type  = "protocol:s,password:s,connected:s?",
+        .args_type  = "protocol:s,password:s?,connected:s?",
         .mhandler.cmd_new = qmp_marshal_input_set_password,
     },
 
diff --git a/qmp.c b/qmp.c
index 6b2c4be..4f741f9 100644
--- a/qmp.c
+++ b/qmp.c
@@ -276,13 +276,17 @@ out:
     return 0;
 }
 
-void qmp_set_password(const char *protocol, const char *password,
+void qmp_set_password(const char *protocol,
+                      bool has_password, const char *password,
                       bool has_connected, const char *connected, Error **errp)
 {
     int disconnect_if_connected = 0;
     int fail_if_connected = 0;
     int rc;
 
+    if (!has_password)
+        password = NULL;
+
     if (has_connected) {
         if (strcmp(connected, "fail") == 0) {
             fail_if_connected = 1;
-- 
2.0.5

Re: [Qemu-devel] [PATCH 1/3] qapi-schema: Make @password in set_password optional

[Daniel P. Berrange]

On Tue, Feb 17, 2015 at 05:40:45PM +0100, Michal Privoznik wrote:
> So, imagine you've started a guest with ticketing enabled. You've set
> some password to access your SPICE/VNC session. However, later you
> want to give the access to somebody else's and therefore disable the
> ticketing. Come on, be imaginative! Currently, there's no way how to
> achieve this. And while there are two possible ways to fulfill the
> goal: 1) invent new monitor command to disable ticketing, or 2) let
> @password argument to 'set_password' monitor command be optional, I'm
> choosing the latter. It's easier to implement, after all.
> 
> The idea behind, how this will work, is: if user issues the command
> without the password field, it means they want to disable the
> ticketing. Any subsequent call to the call with password field filled
> in, will enable the ticketing again.

When password auth is enabled with VNC, the use of a NULL / empty string
password is explicitly intended to block access to the VNC server, by
causing the password auth to always return failure. Overloading the
'set_password' command such that a missing password changes the auth
scheme in use is a really surprising and bad side effect.

If we want to have the ability to change the authentication protocol
used for VNC/SPICE, then lets add a proper command for this. ie
create a 'set_graphics_auth' command to change auth protocol. This
is really better for VNC anyway, as there are far more possible auth
schemes than just password or no-password, and overloading the
'set_password' command can't handle that.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1/3] qapi-schema: Make @password in set_password optional

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2078 bytes --]

On 02/17/2015 09:53 AM, Daniel P. Berrange wrote:
> On Tue, Feb 17, 2015 at 05:40:45PM +0100, Michal Privoznik wrote:
>> So, imagine you've started a guest with ticketing enabled. You've set
>> some password to access your SPICE/VNC session. However, later you
>> want to give the access to somebody else's and therefore disable the
>> ticketing. Come on, be imaginative! Currently, there's no way how to
>> achieve this. And while there are two possible ways to fulfill the
>> goal: 1) invent new monitor command to disable ticketing, or 2) let
>> @password argument to 'set_password' monitor command be optional, I'm
>> choosing the latter. It's easier to implement, after all.
>>
>> The idea behind, how this will work, is: if user issues the command
>> without the password field, it means they want to disable the
>> ticketing. Any subsequent call to the call with password field filled
>> in, will enable the ticketing again.
> 
> When password auth is enabled with VNC, the use of a NULL / empty string
> password is explicitly intended to block access to the VNC server, by
> causing the password auth to always return failure. Overloading the
> 'set_password' command such that a missing password changes the auth
> scheme in use is a really surprising and bad side effect.
> 
> If we want to have the ability to change the authentication protocol
> used for VNC/SPICE, then lets add a proper command for this. ie
> create a 'set_graphics_auth' command to change auth protocol. This
> is really better for VNC anyway, as there are far more possible auth
> schemes than just password or no-password, and overloading the
> 'set_password' command can't handle that.

Agreed about the need for a new command; another rationale is that
making an argument optional is NOT discoverable without introspection or
painful probing, but adding a new command IS easily discovered via the
existing query commands that list all commands.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/3] spice: Implement set_password without password

[Michal Privoznik]

This is quite easy. SPICE has a function to disable ticketing.
So, if no password was provided, call the function, and if a
password was provided, call another function to actually set it.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 ui/spice-core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index c8f7f18..50e1939 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -895,13 +895,24 @@ static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
 int qemu_spice_set_passwd(const char *passwd,
                           bool fail_if_conn, bool disconnect_if_conn)
 {
-    if (strcmp(auth, "spice") != 0) {
+    if (strcmp(auth, "spice") != 0 && strcmp(auth, "none") != 0) {
         return -1;
     }
 
     g_free(auth_passwd);
     auth_passwd = g_strdup(passwd);
-    return qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn);
+
+    if (!passwd) {
+        if (spice_server_set_noauth(spice_server) < 0)
+            return -1;
+        auth = "none";
+    } else {
+        if (qemu_spice_set_ticket(fail_if_conn, disconnect_if_conn) < 0)
+            return -1;
+        auth = "spice";
+    }
+
+    return 0;
 }
 
 int qemu_spice_set_pw_expire(time_t expires)
-- 
2.0.5

[Qemu-devel] [PATCH 3/3] vnc: Implement set_password without password

[Michal Privoznik]

Well, this is a bit difficult since VNC has many ways of
authentication. So, if no password was provided, the current
authentication method is held in a variable (oldauth) and the
method is set to VNC_AUTH_NONE to mark it explicitly that no
password is required. However, as soon as the password is
provided, the old method is restored. But, what happens if
there's no old method? What if no authentication method was
provided at the command line? Well, in that case the VNC_AUTH_VNC
is set, which boils down to the same state as if the password was
put onto command line.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 ui/vnc.c | 18 ++++++++++++++----
 ui/vnc.h |  1 +
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 02552ee..c6886ac 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3161,6 +3161,7 @@ void vnc_display_init(const char *id)
 
     QTAILQ_INIT(&vs->clients);
     vs->expires = TIME_MAX;
+    vs->oldauth = VNC_AUTH_NONE;
 
     if (keyboard_layout) {
         trace_vnc_key_map_init(keyboard_layout);
@@ -3214,10 +3215,19 @@ int vnc_display_password(const char *id, const char *password)
     if (!vs) {
         return -EINVAL;
     }
-    if (vs->auth == VNC_AUTH_NONE) {
-        error_printf_unless_qmp("If you want use passwords please enable "
-                                "password auth using '-vnc ${dpy},password'.");
-        return -EINVAL;
+
+    if (password) {
+        if (vs->auth == VNC_AUTH_NONE && vs->oldauth == VNC_AUTH_NONE) {
+            vs->auth = VNC_AUTH_VNC;
+        } else if (vs->oldauth != VNC_AUTH_NONE) {
+            vs->auth = vs->oldauth;
+            vs->oldauth = VNC_AUTH_NONE;
+        }
+    } else {
+        if (vs->auth != VNC_AUTH_NONE) {
+            vs->oldauth = vs->auth;
+            vs->auth = VNC_AUTH_NONE;
+        }
     }
 
     g_free(vs->password);
diff --git a/ui/vnc.h b/ui/vnc.h
index 5e2b1a5..ec4a566 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -180,6 +180,7 @@ struct VncDisplay
     char *password;
     time_t expires;
     int auth;
+    int oldauth;    /* Used to store value of auth, when temporary disabled */
     bool lossy;
     bool non_adaptive;
 #ifdef CONFIG_VNC_TLS
-- 
2.0.5

Re: [Qemu-devel] [PATCH 0/3] SPICE/VNC: Allow ticketing on the fly

[Gerd Hoffmann]

On Di, 2015-02-17 at 17:40 +0100, Michal Privoznik wrote:
> Currently, if the ticketing (password) is not set at the command
> line, there's no way how to set it afterwards. Or vice versa -
> disable previously set password. I think it's worth allowing
> users to do that. The use case may be a teacher, who wants to
> share a graphical session with students, but has to set up the
> environment firstly. So he starts with a password, and then
> remove it and let students in.

No.  Changing auth scheme as side effect of setting the password is
not going to happen.  It's calling for trouble security-wise.

If anything, then we might add an explicit set-authscheme, although I
suspect for your use case it would be more useful to somehow enforce
view-only access for the students.

cheers,
  Gerd