Re: [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""'

[Anthony Liguori]

On 01/31/2011 02:43 PM, 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.
>
> 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.
>
> 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.
>
> 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;
> +        }
>    

Missing return 0;

However, since there is a check in the password handlign in VNC for 
empty password too, this still ended up working as expected.  I've 
removed that additional check locally and then added the return 0; such 
that I could confirm the functionality again.

Regards,

Anthony Liguori

> +    }
> +
>       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;
>   }
>