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

Re: [libvirt] [PATCH 03/12] Remove hack using existance of an 'identity' string to disable auth



On Wed, May 02, 2012 at 05:23:01PM +0200, Michal Privoznik wrote:
> On 02.05.2012 13:44, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange redhat com>
> > 
> > Currently the server determines whether authentication of clients
> > is complete, by checking whether an identity is set. This patch
> > removes that lame hack and replaces it with an explicit method
> > for changing the client auth code
> > 
> > * daemon/remote.c: Update for new APis
> > * src/libvirt_private.syms, src/rpc/virnetserverclient.c,
> >   src/rpc/virnetserverclient.h: Remove virNetServerClientGetIdentity
> >   and virNetServerClientSetIdentity, adding a new method
> >   virNetServerClientSetAuth.
> > ---
> >  daemon/remote.c              |   14 +++++++-------
> >  src/libvirt_private.syms     |    2 +-
> >  src/rpc/virnetserverclient.c |   36 ++++++++----------------------------
> >  src/rpc/virnetserverclient.h |    5 +----
> >  4 files changed, 17 insertions(+), 40 deletions(-)
> > 
> > diff --git a/daemon/remote.c b/daemon/remote.c
> > index 16a8a05..0bf58d3 100644
> > --- a/daemon/remote.c
> > +++ b/daemon/remote.c
> > @@ -2137,10 +2137,12 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED,
> >                  goto cleanup;
> >              }
> >              VIR_INFO("Bypass polkit auth for privileged client %s", ident);
> > -            if (virNetServerClientSetIdentity(client, ident) < 0)
> > +            if (virNetServerClientSetIdentity(client, ident) < 0) {
> >                  virResetLastError();
> > -            else
> > +            } else {
> > +                virNetServerClientSetAuth(client, 0);
> >                  auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
> > +            }
> >              VIR_FREE(ident);
> >          }
> >      }
> > @@ -2279,9 +2281,7 @@ remoteSASLFinish(virNetServerClientPtr client)
> >      if (!virNetSASLContextCheckIdentity(saslCtxt, identity))
> >          return -2;
> >  
> > -    if (virNetServerClientSetIdentity(client, identity) < 0)
> > -        goto error;
> > -
> > +    virNetServerClientSetAuth(client, 0);
> >      virNetServerClientSetSASLSession(client, priv->sasl);
> >  
> >      VIR_DEBUG("Authentication successful %d", virNetServerClientGetFD(client));
> > @@ -2613,7 +2613,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server ATTRIBUTE_UNUSED,
> >               action, (long long) callerPid, callerUid);
> >      ret->complete = 1;
> >  
> > -    virNetServerClientSetIdentity(client, ident);
> > +    virNetServerClientSetAuth(client, 0);
> >      virMutexUnlock(&priv->lock);
> >      virCommandFree(cmd);
> >      VIR_FREE(pkout);
> > @@ -2767,8 +2767,8 @@ remoteDispatchAuthPolkit(virNetServerPtr server,
> >               action, (long long) callerPid, callerUid,
> >               polkit_result_to_string_representation(pkresult));
> >      ret->complete = 1;
> > -    virNetServerClientSetIdentity(client, ident);
> >  
> > +    virNetServerClientSetAuth(client, 0);
> >      virMutexUnlock(&priv->lock);
> >      VIR_FREE(ident);
> >      return 0;
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index d4038b2..391c977 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1384,8 +1384,8 @@ virNetServerClientRef;
> >  virNetServerClientRemoteAddrString;
> >  virNetServerClientRemoveFilter;
> >  virNetServerClientSendMessage;
> > +virNetServerClientSetAuth;
> >  virNetServerClientSetCloseHook;
> > -virNetServerClientSetIdentity;
> >  virNetServerClientSetPrivateData;
> >  virNetServerClientStartKeepAlive;
> >  
> > diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> > index 67600fd..81dbb32 100644
> > --- a/src/rpc/virnetserverclient.c
> > +++ b/src/rpc/virnetserverclient.c
> > @@ -67,7 +67,6 @@ struct _virNetServerClient
> >      virNetSocketPtr sock;
> >      int auth;
> >      bool readonly;
> > -    char *identity;
> >      virNetTLSContextPtr tlsCtxt;
> >      virNetTLSSessionPtr tls;
> >  #if HAVE_SASL
> > @@ -408,6 +407,13 @@ int virNetServerClientGetAuth(virNetServerClientPtr client)
> >      return auth;
> >  }
> >  
> > +void virNetServerClientSetAuth(virNetServerClientPtr client, int auth)
> > +{
> > +    virNetServerClientLock(client);
> > +    client->auth = auth;
> > +    virNetServerClientUnlock(client);
> > +}
> > +
> >  bool virNetServerClientGetReadonly(virNetServerClientPtr client)
> >  {
> >      bool readonly;
> > @@ -492,31 +498,6 @@ void virNetServerClientSetSASLSession(virNetServerClientPtr client,
> >  #endif
> >  
> >  
> > -int virNetServerClientSetIdentity(virNetServerClientPtr client,
> > -                                  const char *identity)
> > -{
> > -    int ret = -1;
> > -    virNetServerClientLock(client);
> > -    if (!(client->identity = strdup(identity))) {
> > -        virReportOOMError();
> > -        goto error;
> > -    }
> > -    ret = 0;
> > -
> > -error:
> > -    virNetServerClientUnlock(client);
> > -    return ret;
> > -}
> > -
> > -const char *virNetServerClientGetIdentity(virNetServerClientPtr client)
> > -{
> > -    const char *identity;
> > -    virNetServerClientLock(client);
> > -    identity = client->identity;
> > -    virNetServerClientLock(client);
> > -    return identity;
> > -}
> > -
> >  void virNetServerClientSetPrivateData(virNetServerClientPtr client,
> >                                        void *opaque,
> >                                        virNetServerClientFreeFunc ff)
> > @@ -600,7 +581,6 @@ void virNetServerClientFree(virNetServerClientPtr client)
> >          client->privateDataFreeFunc)
> >          client->privateDataFreeFunc(client->privateData);
> >  
> > -    VIR_FREE(client->identity);
> >  #if HAVE_SASL
> >      virNetSASLSessionFree(client->sasl);
> >  #endif
> > @@ -1130,7 +1110,7 @@ bool virNetServerClientNeedAuth(virNetServerClientPtr client)
> >  {
> >      bool need = false;
> >      virNetServerClientLock(client);
> > -    if (client->auth && !client->identity)
> > +    if (client->auth)
> >          need = true;
> >      virNetServerClientUnlock(client);
> >      return need;
> > diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> > index 154a160..633e9e1 100644
> > --- a/src/rpc/virnetserverclient.h
> > +++ b/src/rpc/virnetserverclient.h
> > @@ -52,6 +52,7 @@ void virNetServerClientRemoveFilter(virNetServerClientPtr client,
> >                                      int filterID);
> >  
> >  int virNetServerClientGetAuth(virNetServerClientPtr client);
> > +void virNetServerClientSetAuth(virNetServerClientPtr client, int auth);
> >  bool virNetServerClientGetReadonly(virNetServerClientPtr client);
> >  
> >  bool virNetServerClientHasTLSSession(virNetServerClientPtr client);
> > @@ -66,10 +67,6 @@ int virNetServerClientGetFD(virNetServerClientPtr client);
> >  
> >  bool virNetServerClientIsSecure(virNetServerClientPtr client);
> >  
> > -int virNetServerClientSetIdentity(virNetServerClientPtr client,
> > -                                  const char *identity);
> > -const char *virNetServerClientGetIdentity(virNetServerClientPtr client);
> > -
> >  int virNetServerClientGetUNIXIdentity(virNetServerClientPtr client,
> >                                        uid_t *uid, gid_t *gid, pid_t *pid);
> >  
> 
> Okay, I see your point. However why are you removing
> virNetServerClientSetIdentity() if we are still using it (it could be
> seen even from patch context)?
> 
> ACK to the idea, though.

This was a rebase mistake. The following extra chunk was lost:

diff --git a/daemon/remote.c b/daemon/remote.c
index 0bf58d3..df9053b 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -2137,12 +2137,8 @@ remoteDispatchAuthList(virNetServerPtr server ATTRIBUTE_UNUSED,
                 goto cleanup;
             }
             VIR_INFO("Bypass polkit auth for privileged client %s", ident);
-            if (virNetServerClientSetIdentity(client, ident) < 0) {
-                virResetLastError();
-            } else {
-                virNetServerClientSetAuth(client, 0);
-                auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
-            }
+            virNetServerClientSetAuth(client, 0);
+            auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
             VIR_FREE(ident);
         }
     }


which removes the last use of this API


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


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