| Message ID | 1444649126-13863-1-git-send-email-cfergeau@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 ...
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
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
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; }