Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

2014-03-04 Thread Daniel P. Berrange 
On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=981729
 
 This config tunable allows users to determine the maximum number of
 accepted but yet not authenticated users.
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
  daemon/libvirtd-config.c|  1 +
  daemon/libvirtd-config.h|  1 +
  daemon/libvirtd.aug |  1 +
  daemon/libvirtd.c   |  1 +
  daemon/libvirtd.conf|  4 
  daemon/test_libvirtd.aug.in |  1 +
  src/locking/lock_daemon.c   |  3 +--
  src/lxc/lxc_controller.c|  2 +-
  src/rpc/virnetserver.c  | 52 
 +++--
  src/rpc/virnetserver.h  |  1 +
  10 files changed, 62 insertions(+), 5 deletions(-)
 
 diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
 index c816fda..04482c5 100644
 --- a/daemon/libvirtd-config.c
 +++ b/daemon/libvirtd-config.c
 @@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
  GET_CONF_INT(conf, filename, max_workers);
  GET_CONF_INT(conf, filename, max_clients);
  GET_CONF_INT(conf, filename, max_queued_clients);
 +GET_CONF_INT(conf, filename, max_anonymous_clients);
  
  GET_CONF_INT(conf, filename, prio_workers);
  

You need a 'data-max_anonymous_clients = 20' initialization somewher
in this file, otherwise it'll default to 0.

Also, don't we want to increase 'max_clients' to something much
larger like 5000 now that we rely on max_anonymous_clients to
protect against DOS attack.

 diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
 index 073c178..880f46a 100644
 --- a/daemon/libvirtd.conf
 +++ b/daemon/libvirtd.conf
 @@ -263,6 +263,10 @@
  # connection succeeds.
  #max_queued_clients = 1000
  
 +# The maximum length of queue of accepted but not yet not
 +# authenticated clients. The default value is zero, meaning
 +# the feature is disabled.
 +#max_anonymous_clients = 20

same not about increasing max_clients value here.

 diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
 index e047751..054ece2 100644
 --- a/src/locking/lock_daemon.c
 +++ b/src/locking/lock_daemon.c
 @@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool 
 privileged)
  }
  
  if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
 -   -1, 0,
 -   false, NULL,
 +   0, -1, 0, false, NULL,
 virLockDaemonClientNew,
 virLockDaemonClientPreExecRestart,
 virLockDaemonClientFree,

We need to support max_anonymous_clients in the lock daemon config file
too and increase its max clients to something huge too. Each VM started
corresponds to an open client with the lock daemon, so we need that
value to be quite high.

 @@ -457,6 +468,7 @@ virNetServerPtr 
 virNetServerNewPostExecRestart(virJSONValuePtr object,
  unsigned int max_workers;
  unsigned int priority_workers;
  unsigned int max_clients;
 +unsigned int nclients_unauth_max;
  unsigned int keepaliveInterval;
  unsigned int keepaliveCount;
  bool keepaliveRequired;
 @@ -482,6 +494,11 @@ virNetServerPtr 
 virNetServerNewPostExecRestart(virJSONValuePtr object,
 _(Missing max_clients data in JSON document));
  goto error;
  }
 +if (virJSONValueObjectGetNumberUint(object, nclients_unauth_max, 
 nclients_unauth_max)  0) {
 +virReportError(VIR_ERR_INTERNAL_ERROR, %s,
 +   _(Missing nclients_unauth_max data in JSON 
 document));

We can't raise an error here - that'll cause failure upon RPM upgrade
from version which didn't have this setting. Need to set some kind of
sensible default value upon upgrade.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

2014-03-04 Thread Michal Privoznik 

On 04.03.2014 12:56, Daniel P. Berrange wrote:

On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=981729

This config tunable allows users to determine the maximum number of
accepted but yet not authenticated users.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
  daemon/libvirtd-config.c|  1 +
  daemon/libvirtd-config.h|  1 +
  daemon/libvirtd.aug |  1 +
  daemon/libvirtd.c   |  1 +
  daemon/libvirtd.conf|  4 
  daemon/test_libvirtd.aug.in |  1 +
  src/locking/lock_daemon.c   |  3 +--
  src/lxc/lxc_controller.c|  2 +-
  src/rpc/virnetserver.c  | 52 +++--
  src/rpc/virnetserver.h  |  1 +
  10 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index c816fda..04482c5 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
  GET_CONF_INT(conf, filename, max_workers);
  GET_CONF_INT(conf, filename, max_clients);
  GET_CONF_INT(conf, filename, max_queued_clients);
+GET_CONF_INT(conf, filename, max_anonymous_clients);

  GET_CONF_INT(conf, filename, prio_workers);



You need a 'data-max_anonymous_clients = 20' initialization somewher
in this file, otherwise it'll default to 0.


And I think we want to leave it that way. The value of zero means the 
feature is disabled. That is, libvirt won't take any actions if count of 
anonymous clients exceed  certain value. It will still take an action 
though if the number of total clients (both auth and unauth) exceeds 
max_clients. The action is stop accept()-ing new clients. Trying to 
invent a bright formula to compute the correct value may lead to us 
adapting to a certain use case while forgetting about other. And we've 
been there before (remember 'Set reasonable RSS limit on domain 
startup'?). If a specific management application using libvirt requires 
these values it should set it explicitly in the config file rather than 
relying on libvirt defaults (which would change as libvirt adapts to 
different management application).


What we can do is change the commented default value in config file. Is 
that what you had in mind in the first place?




Also, don't we want to increase 'max_clients' to something much
larger like 5000 now that we rely on max_anonymous_clients to
protect against DOS attack.


diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index 073c178..880f46a 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -263,6 +263,10 @@
  # connection succeeds.
  #max_queued_clients = 1000

+# The maximum length of queue of accepted but not yet not
+# authenticated clients. The default value is zero, meaning
+# the feature is disabled.
+#max_anonymous_clients = 20


same not about increasing max_clients value here.


diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index e047751..054ece2 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config, bool 
privileged)
  }

  if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
-   -1, 0,
-   false, NULL,
+   0, -1, 0, false, NULL,
 virLockDaemonClientNew,
 virLockDaemonClientPreExecRestart,
 virLockDaemonClientFree,


We need to support max_anonymous_clients in the lock daemon config file
too and increase its max clients to something huge too. Each VM started
corresponds to an open client with the lock daemon, so we need that
value to be quite high.


@@ -457,6 +468,7 @@ virNetServerPtr 
virNetServerNewPostExecRestart(virJSONValuePtr object,
  unsigned int max_workers;
  unsigned int priority_workers;
  unsigned int max_clients;
+unsigned int nclients_unauth_max;
  unsigned int keepaliveInterval;
  unsigned int keepaliveCount;
  bool keepaliveRequired;
@@ -482,6 +494,11 @@ virNetServerPtr 
virNetServerNewPostExecRestart(virJSONValuePtr object,
 _(Missing max_clients data in JSON document));
  goto error;
  }
+if (virJSONValueObjectGetNumberUint(object, nclients_unauth_max, 
nclients_unauth_max)  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(Missing nclients_unauth_max data in JSON document));


We can't raise an error here - that'll cause failure upon RPM upgrade
from version which didn't have this setting. Need to set some kind of
sensible default value upon upgrade.


The sensible default is zero in my opinion. If management application 
wants to override this, they still can set the value in the config and 
then softly reset

Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

2014-03-04 Thread Daniel P. Berrange 
On Tue, Mar 04, 2014 at 04:46:22PM +0100, Michal Privoznik wrote:
 On 04.03.2014 12:56, Daniel P. Berrange wrote:
 On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=981729
 
 This config tunable allows users to determine the maximum number of
 accepted but yet not authenticated users.
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
   daemon/libvirtd-config.c|  1 +
   daemon/libvirtd-config.h|  1 +
   daemon/libvirtd.aug |  1 +
   daemon/libvirtd.c   |  1 +
   daemon/libvirtd.conf|  4 
   daemon/test_libvirtd.aug.in |  1 +
   src/locking/lock_daemon.c   |  3 +--
   src/lxc/lxc_controller.c|  2 +-
   src/rpc/virnetserver.c  | 52 
  +++--
   src/rpc/virnetserver.h  |  1 +
   10 files changed, 62 insertions(+), 5 deletions(-)
 
 diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
 index c816fda..04482c5 100644
 --- a/daemon/libvirtd-config.c
 +++ b/daemon/libvirtd-config.c
 @@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
   GET_CONF_INT(conf, filename, max_workers);
   GET_CONF_INT(conf, filename, max_clients);
   GET_CONF_INT(conf, filename, max_queued_clients);
 +GET_CONF_INT(conf, filename, max_anonymous_clients);
 
   GET_CONF_INT(conf, filename, prio_workers);
 
 
 You need a 'data-max_anonymous_clients = 20' initialization somewher
 in this file, otherwise it'll default to 0.
 
 And I think we want to leave it that way. The value of zero means
 the feature is disabled. That is, libvirt won't take any actions if
 count of anonymous clients exceed  certain value. It will still take
 an action though if the number of total clients (both auth and
 unauth) exceeds max_clients. The action is stop accept()-ing new
 clients. Trying to invent a bright formula to compute the correct
 value may lead to us adapting to a certain use case while forgetting
 about other. And we've been there before (remember 'Set reasonable
 RSS limit on domain startup'?). If a specific management application
 using libvirt requires these values it should set it explicitly in
 the config file rather than relying on libvirt defaults (which would
 change as libvirt adapts to different management application).
 
 What we can do is change the commented default value in config file.
 Is that what you had in mind in the first place?

The intent behind this was to get better out of the box behaviour
for client limits.

Currently with max_clients==20, we are very limited out of the
box. People frequently hit the max clients limit, eg due to having
many 'virsh console' sessions open at once.

We can't just raise max_clients because that opens up a denial
of service from unauthenticated clients.

The idea behind tracking unauthenticated clients separately
was that we can have a low value for max_anonymous_clients
to prevent the denial of service possibility. We can then
safely have a very high value for max_clients to cope with
the users who want to open many connections.

 The sensible default is zero in my opinion. If management
 application wants to override this, they still can set the value in
 the config and then softly reset libvirtd (by softly I mean sending
 it signal to reload the config).

A default value of zero means this feature has achieved nothing to
improve our out of the box behaviour, which is what's really hurting
us from users POV.

Thus IMHO our default should be max_anonymous_clients=20 and max_clients=5000


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

2014-03-04 Thread Michal Privoznik 

On 04.03.2014 16:46, Michal Privoznik wrote:

On 04.03.2014 12:56, Daniel P. Berrange wrote:

On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=981729

This config tunable allows users to determine the maximum number of
accepted but yet not authenticated users.

Signed-off-by: Michal Privoznik mpriv...@redhat.com
---
  daemon/libvirtd-config.c|  1 +
  daemon/libvirtd-config.h|  1 +
  daemon/libvirtd.aug |  1 +
  daemon/libvirtd.c   |  1 +
  daemon/libvirtd.conf|  4 
  daemon/test_libvirtd.aug.in |  1 +
  src/locking/lock_daemon.c   |  3 +--
  src/lxc/lxc_controller.c|  2 +-
  src/rpc/virnetserver.c  | 52
+++--
  src/rpc/virnetserver.h  |  1 +
  10 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index c816fda..04482c5 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
  GET_CONF_INT(conf, filename, max_workers);
  GET_CONF_INT(conf, filename, max_clients);
  GET_CONF_INT(conf, filename, max_queued_clients);
+GET_CONF_INT(conf, filename, max_anonymous_clients);

  GET_CONF_INT(conf, filename, prio_workers);



You need a 'data-max_anonymous_clients = 20' initialization somewher
in this file, otherwise it'll default to 0.


And I think we want to leave it that way. The value of zero means the
feature is disabled. That is, libvirt won't take any actions if count of
anonymous clients exceed  certain value. It will still take an action
though if the number of total clients (both auth and unauth) exceeds
max_clients. The action is stop accept()-ing new clients. Trying to
invent a bright formula to compute the correct value may lead to us
adapting to a certain use case while forgetting about other. And we've
been there before (remember 'Set reasonable RSS limit on domain
startup'?). If a specific management application using libvirt requires
these values it should set it explicitly in the config file rather than
relying on libvirt defaults (which would change as libvirt adapts to
different management application).

What we can do is change the commented default value in config file. Is
that what you had in mind in the first place?



Also, don't we want to increase 'max_clients' to something much
larger like 5000 now that we rely on max_anonymous_clients to
protect against DOS attack.


diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index 073c178..880f46a 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -263,6 +263,10 @@
  # connection succeeds.
  #max_queued_clients = 1000

+# The maximum length of queue of accepted but not yet not
+# authenticated clients. The default value is zero, meaning
+# the feature is disabled.
+#max_anonymous_clients = 20


same not about increasing max_clients value here.


diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
index e047751..054ece2 100644
--- a/src/locking/lock_daemon.c
+++ b/src/locking/lock_daemon.c
@@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config,
bool privileged)
  }

  if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
-   -1, 0,
-   false, NULL,
+   0, -1, 0, false, NULL,
 virLockDaemonClientNew,

virLockDaemonClientPreExecRestart,
 virLockDaemonClientFree,


We need to support max_anonymous_clients in the lock daemon config file
too and increase its max clients to something huge too. Each VM started
corresponds to an open client with the lock daemon, so we need that
value to be quite high.


I forgot to reply to this block. But my argumentation stays the same 
here. Moreover, there's no auth or unauth users in virlockd. I mean, all 
users are unauth by default and virlockd has no authentication 
mechanisms to make them authenticate. So either we will pass 0 here and 
let the rest of code deal with it as if the feature is disabled or pass 
config-max_clients which results in the same behavior in the end.





@@ -457,6 +468,7 @@ virNetServerPtr
virNetServerNewPostExecRestart(virJSONValuePtr object,
  unsigned int max_workers;
  unsigned int priority_workers;
  unsigned int max_clients;
+unsigned int nclients_unauth_max;
  unsigned int keepaliveInterval;
  unsigned int keepaliveCount;
  bool keepaliveRequired;
@@ -482,6 +494,11 @@ virNetServerPtr
virNetServerNewPostExecRestart(virJSONValuePtr object,
 _(Missing max_clients data in JSON
document));
  goto error;
  }
+if (virJSONValueObjectGetNumberUint(object,
nclients_unauth_max, nclients_unauth_max)  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+

Re: [libvirt] [PATCH v2 2/2] daemon: Introduce max_anonymous_clients

2014-03-04 Thread Daniel P. Berrange 
On Tue, Mar 04, 2014 at 04:56:24PM +0100, Michal Privoznik wrote:
 On 04.03.2014 16:46, Michal Privoznik wrote:
 On 04.03.2014 12:56, Daniel P. Berrange wrote:
 On Mon, Mar 03, 2014 at 06:22:44PM +0100, Michal Privoznik wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=981729
 
 This config tunable allows users to determine the maximum number of
 accepted but yet not authenticated users.
 
 Signed-off-by: Michal Privoznik mpriv...@redhat.com
 ---
   daemon/libvirtd-config.c|  1 +
   daemon/libvirtd-config.h|  1 +
   daemon/libvirtd.aug |  1 +
   daemon/libvirtd.c   |  1 +
   daemon/libvirtd.conf|  4 
   daemon/test_libvirtd.aug.in |  1 +
   src/locking/lock_daemon.c   |  3 +--
   src/lxc/lxc_controller.c|  2 +-
   src/rpc/virnetserver.c  | 52
 +++--
   src/rpc/virnetserver.h  |  1 +
   10 files changed, 62 insertions(+), 5 deletions(-)
 
 diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
 index c816fda..04482c5 100644
 --- a/daemon/libvirtd-config.c
 +++ b/daemon/libvirtd-config.c
 @@ -415,6 +415,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
   GET_CONF_INT(conf, filename, max_workers);
   GET_CONF_INT(conf, filename, max_clients);
   GET_CONF_INT(conf, filename, max_queued_clients);
 +GET_CONF_INT(conf, filename, max_anonymous_clients);
 
   GET_CONF_INT(conf, filename, prio_workers);
 
 
 You need a 'data-max_anonymous_clients = 20' initialization somewher
 in this file, otherwise it'll default to 0.
 
 And I think we want to leave it that way. The value of zero means the
 feature is disabled. That is, libvirt won't take any actions if count of
 anonymous clients exceed  certain value. It will still take an action
 though if the number of total clients (both auth and unauth) exceeds
 max_clients. The action is stop accept()-ing new clients. Trying to
 invent a bright formula to compute the correct value may lead to us
 adapting to a certain use case while forgetting about other. And we've
 been there before (remember 'Set reasonable RSS limit on domain
 startup'?). If a specific management application using libvirt requires
 these values it should set it explicitly in the config file rather than
 relying on libvirt defaults (which would change as libvirt adapts to
 different management application).
 
 What we can do is change the commented default value in config file. Is
 that what you had in mind in the first place?
 
 
 Also, don't we want to increase 'max_clients' to something much
 larger like 5000 now that we rely on max_anonymous_clients to
 protect against DOS attack.
 
 diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
 index 073c178..880f46a 100644
 --- a/daemon/libvirtd.conf
 +++ b/daemon/libvirtd.conf
 @@ -263,6 +263,10 @@
   # connection succeeds.
   #max_queued_clients = 1000
 
 +# The maximum length of queue of accepted but not yet not
 +# authenticated clients. The default value is zero, meaning
 +# the feature is disabled.
 +#max_anonymous_clients = 20
 
 same not about increasing max_clients value here.
 
 diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c
 index e047751..054ece2 100644
 --- a/src/locking/lock_daemon.c
 +++ b/src/locking/lock_daemon.c
 @@ -145,8 +145,7 @@ virLockDaemonNew(virLockDaemonConfigPtr config,
 bool privileged)
   }
 
   if (!(lockd-srv = virNetServerNew(1, 1, 0, config-max_clients,
 -   -1, 0,
 -   false, NULL,
 +   0, -1, 0, false, NULL,
  virLockDaemonClientNew,
 
 virLockDaemonClientPreExecRestart,
  virLockDaemonClientFree,
 
 We need to support max_anonymous_clients in the lock daemon config file
 too and increase its max clients to something huge too. Each VM started
 corresponds to an open client with the lock daemon, so we need that
 value to be quite high.
 
 I forgot to reply to this block. But my argumentation stays the same
 here. Moreover, there's no auth or unauth users in virlockd. I mean,
 all users are unauth by default and virlockd has no authentication
 mechanisms to make them authenticate. So either we will pass 0 here
 and let the rest of code deal with it as if the feature is disabled
 or pass config-max_clients which results in the same behavior in
 the end.

I forgot we didn't do auth here. We immediately check the UID of
the client and drop them if not root, so we'd actually be perfectly
safe to increase  max_clients for the lock daemon already.

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

--