[Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used

[Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used

[Christophe Fergeau]

Before commit b1ea7b79e1, it was possible to start with -spice
disable-ticketing, and then use the "set_password spice" command to
enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
possible as qemu_spice_set_ticket() will return an error unless the
'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so
the attempt to set password fails.

This change of behaviour caused a bug in oVirt
https://gerrit.ovirt.org/#/c/44842/

This commit allows to call qemu_spice_set_ticket() when 'auth' is "none"
and changes 'auth' to "spice" when this happens.

Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
Changes since v2:
  - Added mention of the oVirt bug which was caused by the change of behaviour

 ui/spice-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 4da3042..3b20c6c 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -882,6 +882,10 @@ 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, "none") == 0) {
+        /* Allow to set a password when started with 'disable-ticketing' */
+        auth = "spice";
+    }
     if (strcmp(auth, "spice") != 0) {
         return -1;
     }
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used

[Gerd Hoffmann]

On Mo, 2015-10-12 at 13:25 +0200, Christophe Fergeau wrote:
> Before commit b1ea7b79e1, it was possible to start with -spice
> disable-ticketing, and then use the "set_password spice" command to
> enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
> possible as qemu_spice_set_ticket() will return an error unless the
> 'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so
> the attempt to set password fails.

Huh?  And this actually worked?  i.e. spice_server_set_ticket() has an
effect after spice_server_set_noauth() was called?

> This change of behaviour caused a bug in oVirt
> https://gerrit.ovirt.org/#/c/44842/

Hmm, I'd say fix this in ovirt then [1].

If you want run with spice authentication, then say so when starting
qemu.  Switching authentication methods as side-effect of setting the
password is asking for trouble.  We had that with vnc.  We finally got
rid of it a while ago.  I don't feel like opening that can of worms
again.

Also it encourages bad security practice.  If you turn on password auth
as side effect of setting the password there is a window where one can
access the virtual machine without a password, which probably is not
what you want.

If there is an actual use case where switching authentication methods at
runtime is needed we can discuss that.  But we'll be doing that as
explicit monitor command, not as side-effect of something else.

cheers,
  Gerd

[1]  You have to do that anyway.  We had three qemu releases (2.1 to
     2.3) with that behavior ...

Re: [Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used

[Christophe Fergeau]

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

Hey Gerd,

On Mon, Oct 12, 2015 at 03:43:51PM +0200, Gerd Hoffmann wrote:
> On Mo, 2015-10-12 at 13:25 +0200, Christophe Fergeau wrote:
> > Before commit b1ea7b79e1, it was possible to start with -spice
> > disable-ticketing, and then use the "set_password spice" command to
> > enable ticketing with SPICE. Since commit b1ea7b79e1 this is no longer
> > possible as qemu_spice_set_ticket() will return an error unless the
> > 'auth' type is "spice". When ticketing is disabled, 'auth' is "none" so
> > the attempt to set password fails.
> 
> Huh?  And this actually worked?  i.e. spice_server_set_ticket() has an
> effect after spice_server_set_noauth() was called?

Yes, this works on the spice server side.

> 
> > This change of behaviour caused a bug in oVirt
> > https://gerrit.ovirt.org/#/c/44842/
> 
> Hmm, I'd say fix this in ovirt then [1].

Fair enough (I believe this is already fixed in newer versions).

> 
> If you want run with spice authentication, then say so when starting
> qemu.  Switching authentication methods as side-effect of setting the
> password is asking for trouble.  We had that with vnc.  We finally got
> rid of it a while ago.  I don't feel like opening that can of worms
> again.
> 
> Also it encourages bad security practice.  If you turn on password auth
> as side effect of setting the password there is a window where one can
> access the virtual machine without a password, which probably is not
> what you want.
> 
> If there is an actual use case where switching authentication methods at
> runtime is needed we can discuss that.  But we'll be doing that as
> explicit monitor command, not as side-effect of something else.

Yeah, I'm not saying this patch is a good idea. However, there was a
silent change (by reading the commit log which introduced it, this
change is not mentioned at all, so may have been unintentional) in
behaviour in QEMU, and this change causes breakage in existing apps
using QEMU. This patch only attempts to fix this regression, I'm not
saying one behaviour or the other is better.

Christophe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v3] spice: Allow to set password even if disable-ticketing was used

[Gerd Hoffmann]

  Hi,

> > If there is an actual use case where switching authentication methods at
> > runtime is needed we can discuss that.  But we'll be doing that as
> > explicit monitor command, not as side-effect of something else.
> 
> Yeah, I'm not saying this patch is a good idea. However, there was a
> silent change (by reading the commit log which introduced it, this
> change is not mentioned at all, so may have been unintentional)

Well, sort of.  The check was mainly added to avoid set_ticket being
called with sasl auth being active.  And as setting the ticket only
makes sense with spice auth being active (not knowing set_ticket can
switch auth mode) the check added verifies we are in spice auth mode.

> in
> behaviour in QEMU, and this change causes breakage in existing apps
> using QEMU.

Undocumented behavior.

> This patch only attempts to fix this regression, I'm not
> saying one behaviour or the other is better.

Fair enough.  I still prefer to keep the current behavior.

cheers,
  Gerd