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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]