[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vn
From: |
Daniel P. Berrange |
Subject: |
[Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""' |
Date: |
Thu, 3 Feb 2011 16:29:21 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> Signed-off-by: Anthony Liguori <address@hidden>
>
> 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 :|
- [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""',
Daniel P. Berrange <=