[Qemu-devel] [PATCH] let management expire vnc password

[Qemu-devel] [PATCH] let management expire vnc password

[Dan Kenigsberg]

After a client connects to vnc server, management may wish to expire the
vnc password, so that an attacker has less time to break into the vm.
---
 console.h |    2 +-
 monitor.c |    5 ++++-
 vnc.c     |    5 +++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/console.h b/console.h
index 9615f56..13e7314 100644
--- a/console.h
+++ b/console.h
@@ -321,7 +321,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
 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_password(DisplayState *ds, const char *password, int expired);
 void do_info_vnc(Monitor *mon);
 char *vnc_display_local_addr(DisplayState *ds);
 
diff --git a/monitor.c b/monitor.c
index 167041e..2d3dc40 100644
--- a/monitor.c
+++ b/monitor.c
@@ -497,7 +497,7 @@ static void do_change_block(Monitor *mon, const char *device,
 static void change_vnc_password_cb(Monitor *mon, const char *password,
                                    void *opaque)
 {
-    if (vnc_display_password(NULL, password) < 0)
+    if (vnc_display_password(NULL, password, (int)opaque) < 0)
         monitor_printf(mon, "could not set VNC server password\n");
 
     monitor_read_command(mon, 1);
@@ -515,6 +515,9 @@ static void do_change_vnc(Monitor *mon, const char *target, const char *arg)
         } else {
             monitor_read_password(mon, change_vnc_password_cb, NULL);
         }
+    } else if (strcmp(target, "expire_passwd") == 0 ||
+               strcmp(target, "expire_password") == 0) {
+        change_vnc_password_cb(mon, NULL, (void *)1);
     } else {
         if (vnc_display_open(NULL, target) < 0)
             monitor_printf(mon, "could not start VNC server on %s\n", target);
diff --git a/vnc.c b/vnc.c
index 5eaef6a..a002973 100644
--- a/vnc.c
+++ b/vnc.c
@@ -2259,7 +2259,7 @@ void vnc_display_close(DisplayState *ds)
 #endif
 }
 
-int vnc_display_password(DisplayState *ds, const char *password)
+int vnc_display_password(DisplayState *ds, const char *password, int expired)
 {
     VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
 
@@ -2278,7 +2278,8 @@ int vnc_display_password(DisplayState *ds, const char *password)
             vs->auth = VNC_AUTH_VNC;
         }
     } else {
-        vs->auth = VNC_AUTH_NONE;
+        if (!expired)
+            vs->auth = VNC_AUTH_NONE;
     }
 
     return 0;
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Anthony Liguori]

Dan Kenigsberg wrote:
> After a client connects to vnc server, management may wish to expire the
> vnc password, so that an attacker has less time to break into the vm.
>   

I don't understand what the use-case for this is.

You want to basically lock out any new clients?  Can't you just set the 
password to something random?  I really don't understand why you would 
want to do this.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Dan Kenigsberg]

On Wed, Sep 30, 2009 at 08:49:28AM -0500, Anthony Liguori wrote:
> Dan Kenigsberg wrote:
>> After a client connects to vnc server, management may wish to expire the
>> vnc password, so that an attacker has less time to break into the vm.
>>   
>
> I don't understand what the use-case for this is.
>
> You want to basically lock out any new clients?  Can't you just set the  
> password to something random?

Yes, and actually that's what we currently do. But having a random
password still opens a crack for guessing it.

> I really don't understand why you would  
> want to do this.
>
> Regards,
>
> Anthony Liguori

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Anthony Liguori]

Dan Kenigsberg wrote:
> On Wed, Sep 30, 2009 at 08:49:28AM -0500, Anthony Liguori wrote:
>   
>> Dan Kenigsberg wrote:
>>     
>>> After a client connects to vnc server, management may wish to expire the
>>> vnc password, so that an attacker has less time to break into the vm.
>>>   
>>>       
>> I don't understand what the use-case for this is.
>>
>> You want to basically lock out any new clients?  Can't you just set the  
>> password to something random?
>>     
>
> Yes, and actually that's what we currently do. But having a random
> password still opens a crack for guessing it.
>   

Is the requirement, prevent future clients from connecting to the vnc 
server?  Essentially, disabling the vnc server?

Could we do something more direct like add a 'vnc off' monitor command?  
The nice thing about this approach is that we could add a flag to 
disconnect all connected clients since someone else wanted that feature 
in the past.

Can you explain the rationale for doing this though in a management 
tool?  I'd like to better understand what sort of policy you're trying 
to enforce.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Dan Kenigsberg]

On Wed, Sep 30, 2009 at 09:43:13AM -0500, Anthony Liguori wrote:
> Dan Kenigsberg wrote:
>> On Wed, Sep 30, 2009 at 08:49:28AM -0500, Anthony Liguori wrote:
>>   
>>> Dan Kenigsberg wrote:
>>>     
>>>> After a client connects to vnc server, management may wish to expire the
>>>> vnc password, so that an attacker has less time to break into the vm.
>>>>         
>>> I don't understand what the use-case for this is.
>>>
>>> You want to basically lock out any new clients?  Can't you just set 
>>> the  password to something random?
>>>     
>>
>> Yes, and actually that's what we currently do. But having a random
>> password still opens a crack for guessing it.
>>   
>
> Is the requirement, prevent future clients from connecting to the vnc  
> server?  Essentially, disabling the vnc server?
>
> Could we do something more direct like add a 'vnc off' monitor command?   
> The nice thing about this approach is that we could add a flag to  
> disconnect all connected clients since someone else wanted that feature  
> in the past.
>

We would like to prevent future connection, but not to disconnect
existing ones.

> Can you explain the rationale for doing this though in a management  
> tool?  I'd like to better understand what sort of policy you're trying  
> to enforce.

The rationale is central management of access to virtual machines.

Normally, no vnc access to VMs is allowed. A user with enough
credentials may request the management tool for a short-lived
"ticket" to connect to a VM. If the user uses it, great. But after the
ticket expires, no further connections are allowed.

Regards,

Dan.

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Anthony Liguori]

Dan Kenigsberg wrote:
> The rationale is central management of access to virtual machines.
>
> Normally, no vnc access to VMs is allowed. A user with enough
> credentials may request the management tool for a short-lived
> "ticket" to connect to a VM. If the user uses it, great. But after the
> ticket expires, no further connections are allowed.
>   

Couldn't you implement the same feature with an IP tables rule (prevent 
new connections from being established)?

I'm not convinced this functionality is very useful generally so I think 
I'd prefer not to merge it.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Daniel P. Berrange]

On Wed, Sep 30, 2009 at 04:03:20PM -0500, Anthony Liguori wrote:
> Dan Kenigsberg wrote:
> >The rationale is central management of access to virtual machines.
> >
> >Normally, no vnc access to VMs is allowed. A user with enough
> >credentials may request the management tool for a short-lived
> >"ticket" to connect to a VM. If the user uses it, great. But after the
> >ticket expires, no further connections are allowed.
> >  
> 
> Couldn't you implement the same feature with an IP tables rule (prevent 
> new connections from being established)?
> 
> I'm not convinced this functionality is very useful generally so I think 
> I'd prefer not to merge it.

I think it is a pretty valid use case, though I don't like the proposed
implementation. In essence it is implementing one-time-passwords instead
of multi-use passwords and both of those are reasonable concepts. Having
to implement one-time passwords using multi-use passwords + iptables is
a really bad, over complicated hack, particularly considering how trivial
this is todo in QEMU.

In terms of impl though, rather than having separate a 'expire_password'
command, I think  it would be preferrable to have alternative syntax for
setting initial credentials 

    change vnc passwd     (for multi-use passwords)
    change vnc otp        (for single-use passwords)

Or, extend the existing 'change vnc passwd' command to allow optional
flags as a 4th argument.

     change vnc passwd otp

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Anthony Liguori]

Daniel P. Berrange wrote:
> I think it is a pretty valid use case, though I don't like the proposed
> implementation. In essence it is implementing one-time-passwords instead
> of multi-use passwords and both of those are reasonable concepts. Having
> to implement one-time passwords using multi-use passwords + iptables is
> a really bad, over complicated hack, particularly considering how trivial
> this is todo in QEMU.
>
> In terms of impl though, rather than having separate a 'expire_password'
> command, I think  it would be preferrable to have alternative syntax for
> setting initial credentials 
>
>     change vnc passwd     (for multi-use passwords)
>     change vnc otp        (for single-use passwords)
>
> Or, extend the existing 'change vnc passwd' command to allow optional
> flags as a 4th argument.
>
>      change vnc passwd otp
>   

A one time password does make a bit more sense to me but I wonder if 
that still satisfies the use case.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] let management expire vnc password

[Jamie Lokier]

Anthony Liguori wrote:
> A one time password does make a bit more sense to me but I wonder if 
> that still satisfies the use case.

The way I read it, the use case required a one time password with an
expiry time.  Expiry is easily implemented by the management tool if
there's a way to disable new connections.

-- Jamie