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

[Anthony Liguori <anthony@codemonkey.ws>]

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<neil@aldur.co.uk>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> 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
>