From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46343 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pl2A0-000197-FT for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:35:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pl29y-0007Um-UV for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:35:56 -0500 Received: from mail-yi0-f45.google.com ([209.85.218.45]:48936) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pl29y-0007Uh-Qo for qemu-devel@nongnu.org; Thu, 03 Feb 2011 11:35:54 -0500 Received: by yie21 with SMTP id 21so620511yie.4 for ; Thu, 03 Feb 2011 08:35:54 -0800 (PST) Message-ID: <4D4AD967.9000107@codemonkey.ws> Date: Thu, 03 Feb 2011 10:35:51 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""' References: <1296506599-7126-1-git-send-email-aliguori@us.ibm.com> <20110203162921.GJ19545@redhat.com> In-Reply-To: <20110203162921.GJ19545@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Anthony Liguori , Gerd Hoffman , Neil Wilson , qemu-devel@nongnu.org On 02/03/2011 10:29 AM, Daniel P. Berrange wrote: > 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. > For 0.15, I'd like to introduce a new set of commands such that we don't multiplex the change command anymore. This refactoring lays the ground work for that. For instance, if you created a block device with the name 'vnc', you'd get very unexpected results! Multiplexing based on special values on top of existing commands is pretty evil. >> 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 > Yup. Thanks. Regards, Anthony Liguori > Daniel >