All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""'
@ 2011-01-31 20:43 Anthony Liguori
  2011-01-31 21:32 ` Anthony Liguori
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Anthony Liguori @ 2011-01-31 20:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: Neil Wilson, Gerd Hoffman, Anthony Liguori

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-01-31 20:43 [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""' Anthony Liguori
@ 2011-01-31 21:32 ` Anthony Liguori
  2011-02-03 16:29 ` [Qemu-devel] " Daniel P. Berrange
  2011-02-14 10:57 ` Gerd Hoffmann
  2 siblings, 0 replies; 14+ messages in thread
From: Anthony Liguori @ 2011-01-31 21:32 UTC (permalink / raw)
  Cc: Neil Wilson, qemu-devel, Gerd Hoffman

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-01-31 20:43 [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""' Anthony Liguori
  2011-01-31 21:32 ` Anthony Liguori
@ 2011-02-03 16:29 ` Daniel P. Berrange
  2011-02-03 16:35   ` Anthony Liguori
  2011-02-14 10:57 ` Gerd Hoffmann
  2 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrange @ 2011-02-03 16:29 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, qemu-devel, 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 <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

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 :|

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-03 16:29 ` [Qemu-devel] " Daniel P. Berrange
@ 2011-02-03 16:35   ` Anthony Liguori
  2011-02-03 17:02     ` Daniel P. Berrange
  0 siblings, 1 reply; 14+ messages in thread
From: Anthony Liguori @ 2011-02-03 16:35 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Anthony Liguori, Gerd Hoffman, Neil Wilson, qemu-devel

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
>    

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-03 16:35   ` Anthony Liguori
@ 2011-02-03 17:02     ` Daniel P. Berrange
  2011-02-03 17:16       ` Anthony Liguori
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel P. Berrange @ 2011-02-03 17:02 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Anthony Liguori, Gerd Hoffman, Neil Wilson, qemu-devel

On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
> 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.

Doesn't Gerd's 'set_password' command already replace the functionality
of the 'change vnc' command. So we should likely declare 'change vnc'
as deprecated in 0.14 and remove it in 0.16

Regards,
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 :|

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-03 17:02     ` Daniel P. Berrange
@ 2011-02-03 17:16       ` Anthony Liguori
  2011-02-04  8:56         ` Markus Armbruster
  0 siblings, 1 reply; 14+ messages in thread
From: Anthony Liguori @ 2011-02-03 17:16 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Neil Wilson, Gerd Hoffman, qemu-devel

On 02/03/2011 11:02 AM, Daniel P. Berrange wrote:
> On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
>    
>> 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.
>>      
> Doesn't Gerd's 'set_password' command already replace the functionality
> of the 'change vnc' command. So we should likely declare 'change vnc'
> as deprecated in 0.14 and remove it in 0.16
>    

Yup.  But it doesn't let you disable login.  Since that was a feature of 
'change vnc', I think we need to provide a proper interface to do this.

Likewise, we need a new interface for changing the block device.  The 
way password setting is handled is fubar right now.

I've got some new commands documented in a git tree if you're interested.

http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-03 17:16       ` Anthony Liguori
@ 2011-02-04  8:56         ` Markus Armbruster
  0 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-02-04  8:56 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, Gerd Hoffman, qemu-devel

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/03/2011 11:02 AM, Daniel P. Berrange wrote:
>> On Thu, Feb 03, 2011 at 10:35:51AM -0600, Anthony Liguori wrote:
>>    
>>> 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.
>>>      
>> Doesn't Gerd's 'set_password' command already replace the functionality
>> of the 'change vnc' command. So we should likely declare 'change vnc'
>> as deprecated in 0.14 and remove it in 0.16
>>    
>
> Yup.  But it doesn't let you disable login.  Since that was a feature
> of 'change vnc', I think we need to provide a proper interface to do
> this.
>
> Likewise, we need a new interface for changing the block device.  The
> way password setting is handled is fubar right now.

Not just that.

> I've got some new commands documented in a git tree if you're interested.
>
> http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json

The command to insert media into an empty block backend needs to support
same range of media options as the command to create a block backend
with media (that command is currently caught within drive_add,
struggling to get out).

My efforts to provide exactly that got bogged down last year by
protracted design discussions on the finer points of its QMP interface,
and the need to clean up and fix N+1 things before I can even start
attacking the goal problem.  Fortunately, much of the preliminary work
got in.  Unfortunately, I haven't been able to work on the rest, lately
:(

Media change is just a convenience command fusing eject and insert.  For
even more convenience, we can make it inherit option defaults from the
media being replaced.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-01-31 20:43 [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""' Anthony Liguori
  2011-01-31 21:32 ` Anthony Liguori
  2011-02-03 16:29 ` [Qemu-devel] " Daniel P. Berrange
@ 2011-02-14 10:57 ` Gerd Hoffmann
  2011-02-14 12:10   ` Anthony Liguori
  2 siblings, 1 reply; 14+ messages in thread
From: Gerd Hoffmann @ 2011-02-14 10:57 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, qemu-devel

On 01/31/11 21:43, 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.

Hmm, now about simply never ever changing vs->auth?

cheers,
   Gerd

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 10:57 ` Gerd Hoffmann
@ 2011-02-14 12:10   ` Anthony Liguori
  2011-02-14 12:24     ` Daniel P. Berrange
  2011-02-14 13:56     ` Gerd Hoffmann
  0 siblings, 2 replies; 14+ messages in thread
From: Anthony Liguori @ 2011-02-14 12:10 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Neil Wilson, qemu-devel

On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> On 01/31/11 21:43, 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.
>
> Hmm, now about simply never ever changing vs->auth?

If auth is none and you do a vnc change password "" then if we don't set 
vs->auth to vnc, it won't have the desired effect.  I really dislike the 
semantics of this command but that was a past mistake.

Regards,

Anthony Liguori

>
> cheers,
>   Gerd
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 12:10   ` Anthony Liguori
@ 2011-02-14 12:24     ` Daniel P. Berrange
  2011-02-14 14:14       ` Anthony Liguori
  2011-02-14 13:56     ` Gerd Hoffmann
  1 sibling, 1 reply; 14+ messages in thread
From: Daniel P. Berrange @ 2011-02-14 12:24 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, Gerd Hoffmann, qemu-devel

On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> >On 01/31/11 21:43, 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.
> >
> >Hmm, now about simply never ever changing vs->auth?
> 
> If auth is none and you do a vnc change password "" then if we don't
> set vs->auth to vnc, it won't have the desired effect.  I really
> dislike the semantics of this command but that was a past mistake.

Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
So we really do want the change password command to leave 'vs->auth'
alone completely - just change the password string, with no side effects
on auth methods. If an app intends to use the change password command
it will have already launched QEMU with neccessary -vnc flags to set the
desired vs->auth and vs->subauth methods.

Regards,
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 :|

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 12:10   ` Anthony Liguori
  2011-02-14 12:24     ` Daniel P. Berrange
@ 2011-02-14 13:56     ` Gerd Hoffmann
  2011-02-14 14:16       ` Anthony Liguori
  1 sibling, 1 reply; 14+ messages in thread
From: Gerd Hoffmann @ 2011-02-14 13:56 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, qemu-devel

On 02/14/11 13:10, Anthony Liguori wrote:
> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>> On 01/31/11 21:43, 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.
>>
>> Hmm, now about simply never ever changing vs->auth?
>
> If auth is none and you do a vnc change password "" then if we don't set
> vs->auth to vnc, it won't have the desired effect.

If you want a password-protected vnc session you should better 
explicitly say so using '-vnc :0,password', otherwise you'll have a 
window (between qemu start and setting the password) where vnc clients 
can connect without a password.

Going from "none" to "vnc" automagically when setting a password 
encourages this insecure way to enable password protection.  IMHO we 
should stop doing this.  There are backward compatibility issues though 
as qemu did this for quite some time ...

Going from "vnc" to "none" automagically when setting a empty password 
is a no-go from a security point of view, especially as older qemu 
versions did *not* do that.

I don't think we'll need a monitor command to switch authentication 
methods on the fly.  YMMV.

cheers,
   Gerd

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 12:24     ` Daniel P. Berrange
@ 2011-02-14 14:14       ` Anthony Liguori
  2011-02-14 14:47         ` Daniel P. Berrange
  0 siblings, 1 reply; 14+ messages in thread
From: Anthony Liguori @ 2011-02-14 14:14 UTC (permalink / raw)
  To: Daniel P. Berrange; +Cc: Neil Wilson, Gerd Hoffmann, qemu-devel

On 02/14/2011 06:24 AM, Daniel P. Berrange wrote:
> On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
>    
>> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>>      
>>> On 01/31/11 21:43, 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.
>>>>          
>>> Hmm, now about simply never ever changing vs->auth?
>>>        
>> If auth is none and you do a vnc change password "" then if we don't
>> set vs->auth to vnc, it won't have the desired effect.  I really
>> dislike the semantics of this command but that was a past mistake.
>>      
> Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
>    

But this is the semantics of the command.  I agree it's stupid but a 
security flaw is a regression and this is not a regression.

This is why the set-password command no longer does any of this nonsense.

> If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
> and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
> So we really do want the change password command to leave 'vs->auth'
> alone completely - just change the password string, with no side effects
> on auth methods. If an app intends to use the change password command
> it will have already launched QEMU with neccessary -vnc flags to set the
> desired vs->auth and vs->subauth methods.
>    

I think I see how this could work but I'm not sure it's worth doing.  
I'd rather just leave the (bad) semantics of this command alone and 
deprecate the interface.

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 13:56     ` Gerd Hoffmann
@ 2011-02-14 14:16       ` Anthony Liguori
  0 siblings, 0 replies; 14+ messages in thread
From: Anthony Liguori @ 2011-02-14 14:16 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Neil Wilson, qemu-devel

On 02/14/2011 07:56 AM, Gerd Hoffmann wrote:
> On 02/14/11 13:10, Anthony Liguori wrote:
>> On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
>>> On 01/31/11 21:43, 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.
>>>
>>> Hmm, now about simply never ever changing vs->auth?
>>
>> If auth is none and you do a vnc change password "" then if we don't set
>> vs->auth to vnc, it won't have the desired effect.
>
> If you want a password-protected vnc session you should better 
> explicitly say so using '-vnc :0,password', otherwise you'll have a 
> window (between qemu start and setting the password) where vnc clients 
> can connect without a password.
>
> Going from "none" to "vnc" automagically when setting a password 
> encourages this insecure way to enable password protection.  IMHO we 
> should stop doing this.  There are backward compatibility issues 
> though as qemu did this for quite some time ...

Yeah, change vnc is deprecated, set-password has sane semantics.

> Going from "vnc" to "none" automagically when setting a empty password 
> is a no-go from a security point of view, especially as older qemu 
> versions did *not* do that. ]

Yup, this was the reason for the CVE.

> I don't think we'll need a monitor command to switch authentication 
> methods on the fly.  YMMV.

Actually, I do, but that's far off on my radar screen.

One thing we're noticing is that QEMU is not terribly forgiving in a 
hosting environment.  If you make a mistake configuring something (like 
use the wrong auth type), you're only recourse is restarting the guest 
with the new options.  Restarting a guest of a paying customer == 
unhappy customer.

The more we can change without restarting a guest the better IMHO.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""'
  2011-02-14 14:14       ` Anthony Liguori
@ 2011-02-14 14:47         ` Daniel P. Berrange
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel P. Berrange @ 2011-02-14 14:47 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Neil Wilson, Gerd Hoffmann, qemu-devel

On Mon, Feb 14, 2011 at 08:14:07AM -0600, Anthony Liguori wrote:
> On 02/14/2011 06:24 AM, Daniel P. Berrange wrote:
> >On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote:
> >>On 02/14/2011 04:57 AM, Gerd Hoffmann wrote:
> >>>On 01/31/11 21:43, 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.
> >>>Hmm, now about simply never ever changing vs->auth?
> >>If auth is none and you do a vnc change password "" then if we don't
> >>set vs->auth to vnc, it won't have the desired effect.  I really
> >>dislike the semantics of this command but that was a past mistake.
> >Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw.
> 
> But this is the semantics of the command.  I agree it's stupid but a
> security flaw is a regression and this is not a regression.
> 
> This is why the set-password command no longer does any of this nonsense.
> 
> >If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT
> >and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme.
> >So we really do want the change password command to leave 'vs->auth'
> >alone completely - just change the password string, with no side effects
> >on auth methods. If an app intends to use the change password command
> >it will have already launched QEMU with neccessary -vnc flags to set the
> >desired vs->auth and vs->subauth methods.
> 
> I think I see how this could work but I'm not sure it's worth doing.
> I'd rather just leave the (bad) semantics of this command alone and
> deprecate the interface.

If you make it blindly set 'vs->auth = VNC' then you haven't fully
fixed the security flawed, because you will be downgrading from
'vencrypt+vnc' auth to just 'vnc' which loose all your data encryption
capabilities. Just reverting the previous commit 52c18be9e99dabe295321153fda7fce9f76647ac
fully addresses the problems leaving the command side-effect free
as it was originally designed & implemented.

Regards,
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 :|

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-02-14 14:47 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-31 20:43 [Qemu-devel] [PATCH] vnc: Fix password expiration through 'change vnc ""' Anthony Liguori
2011-01-31 21:32 ` Anthony Liguori
2011-02-03 16:29 ` [Qemu-devel] " Daniel P. Berrange
2011-02-03 16:35   ` Anthony Liguori
2011-02-03 17:02     ` Daniel P. Berrange
2011-02-03 17:16       ` Anthony Liguori
2011-02-04  8:56         ` Markus Armbruster
2011-02-14 10:57 ` Gerd Hoffmann
2011-02-14 12:10   ` Anthony Liguori
2011-02-14 12:24     ` Daniel P. Berrange
2011-02-14 14:14       ` Anthony Liguori
2011-02-14 14:47         ` Daniel P. Berrange
2011-02-14 13:56     ` Gerd Hoffmann
2011-02-14 14:16       ` Anthony Liguori

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.