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

On 02.05.2012 13:44, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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.