The counter gets incremented on each unauthenticated client added to the server and decremented whenever the client authenticates. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- daemon/remote.c | 21 +++++++++++++-------- src/rpc/virnetserver.c | 36 +++++++++++++++++++++++++++++++++--- src/rpc/virnetserver.h | 2 ++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 932f65f..b70d298 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2619,7 +2619,7 @@ cleanup: /*-------------------------------------------------------------*/ static int -remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, +remoteDispatchAuthList(virNetServerPtr server, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -2649,6 +2649,7 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; VIR_INFO("Bypass polkit auth for privileged client %s", ident); virNetServerClientSetAuth(client, 0); + virNetServerClientAuth(server, false); auth = VIR_NET_SERVER_SERVICE_AUTH_NONE; VIR_FREE(ident); } @@ -2764,7 +2765,8 @@ authfail: * Returns 0 if ok, -1 on error, -2 if rejected */ static int -remoteSASLFinish(virNetServerClientPtr client) +remoteSASLFinish(virNetServerPtr server, + virNetServerClientPtr client) { const char *identity; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -2789,6 +2791,7 @@ remoteSASLFinish(virNetServerClientPtr client) return -2; virNetServerClientSetAuth(client, 0); + virNetServerClientAuth(server, false); virNetServerClientSetSASLSession(client, priv->sasl); VIR_DEBUG("Authentication successful %d", virNetServerClientGetFD(client)); @@ -2810,7 +2813,7 @@ error: * This starts the SASL authentication negotiation. */ static int -remoteDispatchAuthSaslStart(virNetServerPtr server ATTRIBUTE_UNUSED, +remoteDispatchAuthSaslStart(virNetServerPtr server, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -2868,7 +2871,7 @@ remoteDispatchAuthSaslStart(virNetServerPtr server ATTRIBUTE_UNUSED, ret->complete = 0; } else { /* Check username whitelist ACL */ - if ((err = remoteSASLFinish(client)) < 0) { + if ((err = remoteSASLFinish(server, client)) < 0) { if (err == -2) goto authdeny; else @@ -2908,7 +2911,7 @@ error: static int -remoteDispatchAuthSaslStep(virNetServerPtr server ATTRIBUTE_UNUSED, +remoteDispatchAuthSaslStep(virNetServerPtr server, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -2966,7 +2969,7 @@ remoteDispatchAuthSaslStep(virNetServerPtr server ATTRIBUTE_UNUSED, ret->complete = 0; } else { /* Check username whitelist ACL */ - if ((err = remoteSASLFinish(client)) < 0) { + if ((err = remoteSASLFinish(server, client)) < 0) { if (err == -2) goto authdeny; else @@ -3051,7 +3054,7 @@ remoteDispatchAuthSaslStep(virNetServerPtr server ATTRIBUTE_UNUSED, #if WITH_POLKIT1 static int -remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, +remoteDispatchAuthPolkit(virNetServerPtr server, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -3143,6 +3146,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, ret->complete = 1; virNetServerClientSetAuth(client, 0); + virNetServerClientAuth(server, false); virMutexUnlock(&priv->lock); virCommandFree(cmd); VIR_FREE(pkout); @@ -3183,7 +3187,7 @@ authdeny: } #elif WITH_POLKIT0 static int -remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, +remoteDispatchAuthPolkit(virNetServerPtr server, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, virNetMessageErrorPtr rerr, @@ -3298,6 +3302,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED, ret->complete = 1; virNetServerClientSetAuth(client, 0); + virNetServerClientAuth(server, false); virMutexUnlock(&priv->lock); VIR_FREE(ident); return 0; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index f70e260..6b3f5f0 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -88,9 +88,10 @@ struct _virNetServer { size_t nprograms; virNetServerProgramPtr *programs; - size_t nclients; - size_t nclients_max; - virNetServerClientPtr *clients; + size_t nclients; /* Current clients count */ + virNetServerClientPtr *clients; /* Clients */ + size_t nclients_max; /* Max allowed clients count */ + size_t nclients_unauth; /* Unauthenticated clients count */ int keepaliveInterval; unsigned int keepaliveCount; @@ -118,6 +119,8 @@ static virClassPtr virNetServerClass; static void virNetServerDispose(void *obj); static void virNetServerUpdateServicesLocked(virNetServerPtr srv, bool enabled); +static size_t virNetServerClientAuthLocked(virNetServerPtr srv, + bool need_auth); static int virNetServerOnceInit(void) { @@ -273,6 +276,9 @@ static int virNetServerAddClient(virNetServerPtr srv, srv->clients[srv->nclients-1] = client; virObjectRef(client); + if (virNetServerClientNeedAuth(client)) + virNetServerClientAuthLocked(srv, true); + if (srv->nclients == srv->nclients_max) { /* Temporarily stop accepting new clients */ VIR_DEBUG("Temporarily suspending services due to max_clients"); @@ -1140,6 +1146,9 @@ void virNetServerRun(virNetServerPtr srv) srv->nclients = 0; } + if (virNetServerClientNeedAuth(client)) + virNetServerClientAuthLocked(srv, false); + /* Enable services if we can accept a new client. * The new client can be accepted if we are at the limit. */ if (srv->nclients == srv->nclients_max - 1) { @@ -1236,3 +1245,24 @@ bool virNetServerKeepAliveRequired(virNetServerPtr srv) virObjectUnlock(srv); return required; } + +static size_t +virNetServerClientAuthLocked(virNetServerPtr srv, + bool need_auth) +{ + if (need_auth) + srv->nclients_unauth++; + else + srv->nclients_unauth--; + return srv->nclients_unauth; +} + +size_t virNetServerClientAuth(virNetServerPtr srv, + bool need_auth) +{ + size_t ret; + virObjectLock(srv); + ret = virNetServerClientAuthLocked(srv, need_auth); + virObjectUnlock(srv); + return ret; +} diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 1a85c02..703a733 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -97,4 +97,6 @@ void virNetServerClose(virNetServerPtr srv); bool virNetServerKeepAliveRequired(virNetServerPtr srv); +size_t virNetServerClientAuth(virNetServerPtr srv, + bool need_auth); #endif -- 1.9.0

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 <mprivozn(a)redhat.com&gt; --- 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); diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h index a24d5d2..66dc80b 100644 --- a/daemon/libvirtd-config.h +++ b/daemon/libvirtd-config.h @@ -64,6 +64,7 @@ struct daemonConfig { int max_workers; int max_clients; int max_queued_clients; + int max_anonymous_clients; int prio_workers; diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug index 70fce5c..5a0807c 100644 --- a/daemon/libvirtd.aug +++ b/daemon/libvirtd.aug @@ -57,6 +57,7 @@ module Libvirtd = | int_entry "max_workers" | int_entry "max_clients" | int_entry "max_queued_clients" + | int_entry "max_anonymous_clients" | int_entry "max_requests" | int_entry "max_client_requests" | int_entry "prio_workers" diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index b27c6fd..6e4c06f 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1376,6 +1376,7 @@ int main(int argc, char **argv) { config->max_workers, config->prio_workers, config->max_clients, + config->max_anonymous_clients, config->keepalive_interval, config->keepalive_count, !!config->keepalive_required, 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 # The minimum limit sets the number of workers to start up # initially. If the number of active clients exceeds this, diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in index a7e8515..b03451c 100644 --- a/daemon/test_libvirtd.aug.in +++ b/daemon/test_libvirtd.aug.in @@ -36,6 +36,7 @@ module Test_libvirtd = } { "max_clients" = "20" } { "max_queued_clients" = "1000" } + { "max_anonymous_clients" = "20" } { "min_workers" = "5" } { "max_workers" = "20" } { "prio_workers" = "5" } 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, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 5ca960f..dcb8264 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -736,7 +736,7 @@ static int virLXCControllerSetupServer(virLXCControllerPtr ctrl) LXC_STATE_DIR, ctrl->name) < 0) return -1; - if (!(ctrl->server = virNetServerNew(0, 0, 0, 1, + if (!(ctrl->server = virNetServerNew(0, 0, 0, 1, 1, -1, 0, false, NULL, virLXCControllerClientPrivateNew, diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 6b3f5f0..c4e8dbc 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -92,6 +92,7 @@ struct _virNetServer { virNetServerClientPtr *clients; /* Clients */ size_t nclients_max; /* Max allowed clients count */ size_t nclients_unauth; /* Unauthenticated clients count */ + size_t nclients_unauth_max; /* Max allowed unauth clients count */ int keepaliveInterval; unsigned int keepaliveCount; @@ -279,6 +280,14 @@ static int virNetServerAddClient(virNetServerPtr srv, if (virNetServerClientNeedAuth(client)) virNetServerClientAuthLocked(srv, true); + if (srv->nclients_unauth_max && + srv->nclients_unauth == srv->nclients_unauth_max) { + /* Temporarily stop accepting new clients */ + VIR_DEBUG("Temporarily suspending services " + "due to max_anonymous_clients"); + virNetServerUpdateServicesLocked(srv, false); + } + if (srv->nclients == srv->nclients_max) { /* Temporarily stop accepting new clients */ VIR_DEBUG("Temporarily suspending services due to max_clients"); @@ -362,6 +371,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, size_t max_workers, size_t priority_workers, size_t max_clients, + size_t max_anonymous_clients, int keepaliveInterval, unsigned int keepaliveCount, bool keepaliveRequired, @@ -388,6 +398,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, goto error; srv->nclients_max = max_clients; + srv->nclients_unauth_max = max_anonymous_clients; srv->keepaliveInterval = keepaliveInterval; srv->keepaliveCount = keepaliveCount; srv->keepaliveRequired = keepaliveRequired; @@ -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")); + goto error; + } if (virJSONValueObjectGetNumberUint(object, "keepaliveInterval", &keepaliveInterval) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing keepaliveInterval data in JSON document")); @@ -507,6 +524,7 @@ virNetServerPtr virNetServerNewPostExecRestart(virJSONValuePtr object, if (!(srv = virNetServerNew(min_workers, max_clients, priority_workers, max_clients, + nclients_unauth_max, keepaliveInterval, keepaliveCount, keepaliveRequired, mdnsGroupName, clientPrivNew, clientPrivPreExecRestart, @@ -625,6 +643,12 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv) _("Cannot set max_clients data in JSON document")); goto error; } + if (virJSONValueObjectAppendNumberUint(object, "nclients_unauth_max", + srv->nclients_unauth_max) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot set nclients_unauth_max data in JSON document")); + goto error; + } if (virJSONValueObjectAppendNumberUint(object, "keepaliveInterval", srv->keepaliveInterval) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot set keepaliveInterval data in JSON document")); @@ -1150,8 +1174,17 @@ void virNetServerRun(virNetServerPtr srv) virNetServerClientAuthLocked(srv, false); /* Enable services if we can accept a new client. - * The new client can be accepted if we are at the limit. */ - if (srv->nclients == srv->nclients_max - 1) { + * The new client can be accepted if both max_clients and + * max_anonymous_clients wouldn't get overcommitted by + * accepting it. */ + VIR_DEBUG("Considering re-enabling services: " + "nclients=%zu nclients_max=%zu " + "nclients_unauth=%zu nclients_unauth_max=%zu", + srv->nclients, srv->nclients_max, + srv->nclients_unauth, srv->nclients_unauth_max); + if (srv->nclients < srv->nclients_max && + (!srv->nclients_unauth_max || + srv->nclients_unauth < srv->nclients_unauth_max)) { /* Now it makes sense to accept() a new client. */ VIR_DEBUG("Re-enabling services"); virNetServerUpdateServicesLocked(srv, true); @@ -1262,7 +1295,22 @@ size_t virNetServerClientAuth(virNetServerPtr srv, { size_t ret; virObjectLock(srv); + ret = virNetServerClientAuthLocked(srv, need_auth); + + VIR_DEBUG("Considering re-enabling services: " + "nclients=%zu nclients_max=%zu " + "nclients_unauth=%zu nclients_unauth_max=%zu", + srv->nclients, srv->nclients_max, + srv->nclients_unauth, srv->nclients_unauth_max); + if (srv->nclients < srv->nclients_max && + (!srv->nclients_unauth_max || + srv->nclients_unauth < srv->nclients_unauth_max)) { + /* Now it makes sense to accept() a new client. */ + VIR_DEBUG("Re-enabling services"); + virNetServerUpdateServicesLocked(srv, true); + } + virObjectUnlock(srv); return ret; } diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 703a733..4b165c3 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -39,6 +39,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, size_t max_workers, size_t priority_workers, size_t max_clients, + size_t max_anonymous_clients, int keepaliveInterval, unsigned int keepaliveCount, bool keepaliveRequired, -- 1.9.0

On 04.03.2014 12:56, Daniel P. Berrange wrote: 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 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). Michal

On 04.03.2014 16:46, Michal Privoznik wrote: 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.