From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51505 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pl23y-000692-IK for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:29:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pl23s-0005Xr-B2 for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:29:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54468) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pl23s-0005X0-4C for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:29:36 -0500 Date: Thu, 3 Feb 2011 16:29:21 +0000 From: "Daniel P. Berrange" Message-ID: <20110203162921.GJ19545@redhat.com> References: <1296506599-7126-1-git-send-email-aliguori@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1296506599-7126-1-git-send-email-aliguori@us.ibm.com> Subject: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""' Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Neil Wilson , qemu-devel@nongnu.org, Gerd Hoffman On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote: > commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the > change vnc password command that changed the behavior of setting the VNC > password to an empty string from disabling login to disabling authentication. > > This commit refactors the code to eliminate this overloaded semantics in > vnc_display_password and instead introduces the vnc_display_disable_login. The > monitor implementation then determines the behavior of an empty or missing > string. Personally I think this is a little overkill & just reverting the original patch was fine, but from a functional POV your patch produces the same results, so I won't argue. > Recently, a set_password command was added that allows both the Spice and VNC > password to be set. This command has not shown up in a release yet so the > behavior is not yet defined. > > This patch proposes that an empty password be treated as an empty password with > no special handling. For specifically disabling login, I believe a new command > should be introduced instead of overloading semantics. Agreed, if some mgmt app does need to change this kind of thin on the fly, they'll likely want more than just a toggle between AUTH_NONE/AUTH_VNC too. eg There's AUTH_SASL, which is the only VNC auth scheme with any real security, and the psuedo auth schemes for providing the TLS encryption/certificate support. > I'm not sure how Spice handles this but I would recommend that we have Spice > and VNC have consistent semantics here for the 0.14.0 release. Sounds like a very good idea. > Reported-by: Neil Wilson > Signed-off-by: Anthony Liguori > > diff --git a/console.h b/console.h > index 3157330..f4e4741 100644 > --- a/console.h > +++ b/console.h > @@ -369,6 +369,7 @@ void vnc_display_init(DisplayState *ds); > void vnc_display_close(DisplayState *ds); > int vnc_display_open(DisplayState *ds, const char *display); > int vnc_display_password(DisplayState *ds, const char *password); > +int vnc_display_disable_login(DisplayState *ds); > int vnc_display_pw_expire(DisplayState *ds, time_t expires); > void do_info_vnc_print(Monitor *mon, const QObject *data); > void do_info_vnc(Monitor *mon, QObject **ret_data); > diff --git a/monitor.c b/monitor.c > index c5f54f4..24ed971 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -1018,6 +1018,13 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data) > > static int change_vnc_password(const char *password) > { > + if (!password || !password[0]) { > + if (vnc_display_disable_login(NULL)) { > + qerror_report(QERR_SET_PASSWD_FAILED); > + return -1; > + } > + } > + > if (vnc_display_password(NULL, password) < 0) { > qerror_report(QERR_SET_PASSWD_FAILED); > return -1; > @@ -1117,6 +1124,8 @@ static int set_password(Monitor *mon, const QDict *qdict, QObject **ret_data) > qerror_report(QERR_INVALID_PARAMETER, "connected"); > return -1; > } > + /* Note that setting an empty password will not disable login through > + * this interface. */ > rc = vnc_display_password(NULL, password); > if (rc != 0) { > qerror_report(QERR_SET_PASSWD_FAILED); > diff --git a/ui/vnc.c b/ui/vnc.c > index 495d6d6..73e7ffa 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -2484,6 +2484,24 @@ void vnc_display_close(DisplayState *ds) > #endif > } > > +int vnc_display_disable_login(DisplayState *ds) > +{ > + VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display; > + > + if (!vs) { > + return -1; > + } > + > + if (vs->password) { > + qemu_free(vs->password); > + } > + > + vs->password = NULL; > + vs->auth = VNC_AUTH_VNC; > + > + return 0; > +} > + > int vnc_display_password(DisplayState *ds, const char *password) > { > VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display; > @@ -2492,19 +2510,18 @@ int vnc_display_password(DisplayState *ds, const char *password) > return -1; > } > > + if (!password) { > + /* This is not the intention of this interface but err on the side > + of being safe */ > + return vnc_display_disable_login(ds); > + } > + > if (vs->password) { > qemu_free(vs->password); > vs->password = NULL; > } > - if (password && password[0]) { > - if (!(vs->password = qemu_strdup(password))) > - return -1; > - if (vs->auth == VNC_AUTH_NONE) { > - vs->auth = VNC_AUTH_VNC; > - } > - } else { > - vs->auth = VNC_AUTH_NONE; > - } > + vs->password = qemu_strdup(password); > + vs->auth = VNC_AUTH_VNC; > > return 0; > } Looks good, assuming the addition of the missing 'return 0' you already mentioned 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 :|