Re: [libvirt] [PATCH v2 1/2] virNetServer: Introduce unauth clients counter

4 Mar 2014

On Mon, Mar 03, 2014 at 06:22:43PM +0100, Michal Privoznik wrote:
...
The counter gets incremented on each unauthenticated client added to the
server and decremented whenever the client authenticates.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 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);
I'm finding this API naming rather confusing in reviewing the patch.
The name suggests it is operating on the virNetServerClientPtr
instance. I think it might also be clearer to have separate methods
for add/dec. eg perhaps

  virNetServerTrackPendingAuth(srv);
  virNetServerTrackCompletedAuth(srv);

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

    

Re: [libvirt] [PATCH v2 1/2] virNetServer: Introduce unauth clients counter

Daniel P. Berrange