Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserver.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h index 151bac8b38aa..6a79d15370e5 100644 --- a/src/rpc/virnetserver.h +++ b/src/rpc/virnetserver.h @@ -65,9 +65,6 @@ int virNetServerAddService(virNetServerPtr srv, virNetServerServicePtr svc, const char *mdnsEntryName); -int virNetServerAddClient(virNetServerPtr srv, - virNetServerClientPtr client); - int virNetServerAddProgram(virNetServerPtr srv, virNetServerProgramPtr prog); -- 2.13.4

Direct leak of 104 byte(s) in 1 object(s) allocated from: #0 0x7f904bfbe12b (/lib64/liblsan.so.0+0xe12b) #1 0x7f904ba0ad67 in virAlloc ../../src/util/viralloc.c:144 #2 0x7f904bbc11a4 in virNetMessageNew ../../src/rpc/virnetmessage.c:42 #3 0x7f904bbb8e77 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:392 #4 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440 #5 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55 #6 0x403bed in virTestRun ../../tests/testutils.c:180 #7 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146 #8 0x404c80 in virTestMain ../../tests/testutils.c:1119 #9 0x4030d5 in main ../../tests/virnetserverclienttest.c:152 #10 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889) Indirect leak of 4 byte(s) in 1 object(s) allocated from: #0 0x7f904bfbe12b (/lib64/liblsan.so.0+0xe12b) #1 0x7f904ba0adc7 in virAllocN ../../src/util/viralloc.c:191 #2 0x7f904bbb8ec7 in virNetServerClientNewInternal ../../src/rpc/virnetserverclient.c:395 #3 0x7f904bbb9921 in virNetServerClientNew ../../src/rpc/virnetserverclient.c:440 #4 0x402ce5 in testIdentity ../../tests/virnetserverclienttest.c:55 #5 0x403bed in virTestRun ../../tests/testutils.c:180 #6 0x402c1e in mymain ../../tests/virnetserverclienttest.c:146 #7 0x404c80 in virTestMain ../../tests/testutils.c:1119 #8 0x4030d5 in main ../../tests/virnetserverclienttest.c:152 #9 0x7f9047f7f889 in __libc_start_main (/lib64/libc.so.6+0x20889) SUMMARY: LeakSanitizer: 108 byte(s) leaked in 2 allocation(s). Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserverclient.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 6e086b7b4e2b..b454a3ff6992 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -948,6 +948,9 @@ void virNetServerClientDispose(void *obj) virObjectUnref(client->tlsCtxt); #endif virObjectUnref(client->sock); + + virNetMessageFree(client->rx); + virNetMessageFree(client->tx); } -- 2.13.4

On Fri, Dec 15, 2017 at 01:14 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: Yes. Will add an patch for this. Should I remove this patch then? -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- daemon/remote.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index c2111ae378a2..f769bb762d5b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3263,7 +3263,7 @@ remoteDispatchAuthList(virNetServerPtr server, (long long) callerPid, (int) callerUid) < 0) goto cleanup; VIR_INFO("Bypass polkit auth for privileged client %s", ident); - virNetServerClientSetAuth(client, 0); + virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); virNetServerTrackCompletedAuth(server); auth = VIR_NET_SERVER_SERVICE_AUTH_NONE; VIR_FREE(ident); @@ -3409,7 +3409,7 @@ remoteSASLFinish(virNetServerPtr server, if (!(clnt_identity = virNetServerClientGetIdentity(client))) goto error; - virNetServerClientSetAuth(client, 0); + virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); virNetServerTrackCompletedAuth(server); virNetServerClientSetSASLSession(client, priv->sasl); virIdentitySetSASLUserName(clnt_identity, identity); @@ -3733,7 +3733,7 @@ remoteDispatchAuthPolkit(virNetServerPtr server, action, (long long) callerPid, callerUid); ret->complete = 1; - virNetServerClientSetAuth(client, 0); + virNetServerClientSetAuth(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); virNetServerTrackCompletedAuth(server); virMutexUnlock(&priv->lock); -- 2.13.4

Add typedef for the anonymous enum used for the authentication methods and remove the default case. This allows the usage of the type in a switch statement and taking advantage of the compilers feature to detect uncovered cases. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- daemon/remote.c | 4 +--- src/rpc/virnetserverservice.h | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index f769bb762d5b..6f78f17178f7 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3274,7 +3274,7 @@ remoteDispatchAuthList(virNetServerPtr server, if (VIR_ALLOC_N(ret->types.types_val, ret->types.types_len) < 0) goto cleanup; - switch (auth) { + switch ((virNetServerServiceAuthMethods) auth) { case VIR_NET_SERVER_SERVICE_AUTH_NONE: ret->types.types_val[0] = REMOTE_AUTH_NONE; break; @@ -3284,8 +3284,6 @@ remoteDispatchAuthList(virNetServerPtr server, case VIR_NET_SERVER_SERVICE_AUTH_SASL: ret->types.types_val[0] = REMOTE_AUTH_SASL; break; - default: - ret->types.types_val[0] = REMOTE_AUTH_NONE; } rv = 0; diff --git a/src/rpc/virnetserverservice.h b/src/rpc/virnetserverservice.h index 9afa0b13dbc6..5d8c583db2e2 100644 --- a/src/rpc/virnetserverservice.h +++ b/src/rpc/virnetserverservice.h @@ -27,11 +27,11 @@ # include "virnetserverprogram.h" # include "virobject.h" -enum { +typedef enum { VIR_NET_SERVER_SERVICE_AUTH_NONE = 0, VIR_NET_SERVER_SERVICE_AUTH_SASL, VIR_NET_SERVER_SERVICE_AUTH_POLKIT, -}; +} virNetServerServiceAuthMethods; typedef int (*virNetServerServiceDispatchFunc)(virNetServerServicePtr svc, virNetSocketPtr sock, -- 2.13.4

Additionally, use a whitelist model to decide whether authentication is needed or not. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserverclient.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index b454a3ff6992..0ee299e2d6ec 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1512,10 +1512,10 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool virNetServerClientNeedAuth(virNetServerClientPtr client) { - bool need = false; + bool need = true; virObjectLock(client); - if (client->auth) - need = true; + if (client->auth == VIR_NET_SERVER_SERVICE_AUTH_NONE) + need = false; virObjectUnlock(client); return need; } -- 2.13.4

On Tue, Dec 12, 2017 at 12:36:27PM +0100, Marc Hartmayer wrote: Is this actually fixing any real problem, if so please document what the problem is. AFAICT, this is mostly just a case of painting the bikeshed a different colour, as both old & new code seem to have the same result in all cases ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 12/12/2017 06:36 AM, Marc Hartmayer wrote: Considering later patches... Why not introduce the Locked version here which just returns (client->auth == VIR_NET_SERVER_SERVICE_AUTH_NONE)? and of course alter the commit message to say Introduce *Locked. Hazards of not peeking forward by me. John

This makes the code more efficient. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserverprogram.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 556c91605f90..1e8dfc2f1128 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -400,8 +400,8 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, /* If client is marked as needing auth, don't allow any RPC ops * which are except for authentication ones */ - if (virNetServerClientNeedAuth(client) && - dispatcher->needAuth) { + if (dispatcher->needAuth && + virNetServerClientNeedAuth(client)) { /* Explicitly *NOT* calling remoteDispatchAuthError() because we want back-compatibility with libvirt clients which don't support the VIR_ERR_AUTH_FAILED error code */ -- 2.13.4

The lock for @client must not only be held for the duration of checking whether the client wants to close, but also for as long as we're closing the client. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 3 ++- src/rpc/virnetserver.c | 12 ++++++++---- src/rpc/virnetserverclient.c | 25 ++++++++++++++----------- src/rpc/virnetserverclient.h | 3 ++- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 61c20d530bc8..3ce5694b781d 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -125,6 +125,7 @@ virNetServerUpdateServices; # rpc/virnetserverclient.h virNetServerClientAddFilter; virNetServerClientClose; +virNetServerClientCloseLocked; virNetServerClientDelayedClose; virNetServerClientGetAuth; virNetServerClientGetFD; @@ -154,7 +155,7 @@ virNetServerClientSetAuth; virNetServerClientSetCloseHook; virNetServerClientSetDispatcher; virNetServerClientStartKeepAlive; -virNetServerClientWantClose; +virNetServerClientWantCloseLocked; # rpc/virnetservermdns.h diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 2b76daab5566..dde9b73fc250 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -846,6 +846,7 @@ void virNetServerProcessClients(virNetServerPtr srv) { size_t i; + virNetServerClientPtr client; virObjectLock(srv); @@ -854,11 +855,14 @@ virNetServerProcessClients(virNetServerPtr srv) /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL * if srv->nclients is non-zero. */ sa_assert(srv->clients); - if (virNetServerClientWantClose(srv->clients[i])) - virNetServerClientClose(srv->clients[i]); - if (virNetServerClientIsClosed(srv->clients[i])) { - virNetServerClientPtr client = srv->clients[i]; + client = srv->clients[i]; + virObjectLock(client); + if (virNetServerClientWantCloseLocked(client)) + virNetServerClientCloseLocked(client); + virObjectUnlock(client); + + if (virNetServerClientIsClosed(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); if (virNetServerClientNeedAuth(client)) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 0ee299e2d6ec..96fd1e6d15c2 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -962,17 +962,15 @@ void virNetServerClientDispose(void *obj) * Full free of the client is done later in a safe point * where it can be guaranteed it is no longer in use */ -void virNetServerClientClose(virNetServerClientPtr client) +void +virNetServerClientCloseLocked(virNetServerClientPtr client) { virNetServerClientCloseFunc cf; virKeepAlivePtr ka; - virObjectLock(client); VIR_DEBUG("client=%p", client); - if (!client->sock) { - virObjectUnlock(client); + if (!client->sock) return; - } if (client->keepalive) { virKeepAliveStop(client->keepalive); @@ -1023,7 +1021,14 @@ void virNetServerClientClose(virNetServerClientPtr client) virObjectUnref(client->sock); client->sock = NULL; } +} + +void +virNetServerClientClose(virNetServerClientPtr client) +{ + virObjectLock(client); + virNetServerClientCloseLocked(client); virObjectUnlock(client); } @@ -1051,13 +1056,11 @@ void virNetServerClientImmediateClose(virNetServerClientPtr client) virObjectUnlock(client); } -bool virNetServerClientWantClose(virNetServerClientPtr client) + +/* @client needs to be locked by the caller */ +bool virNetServerClientWantCloseLocked(virNetServerClientPtr client) { - bool wantClose; - virObjectLock(client); - wantClose = client->wantClose; - virObjectUnlock(client); - return wantClose; + return client->wantClose; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index e45c78882ef7..60ad0f9ed326 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -123,11 +123,12 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client, virNetServerClientDispatchFunc func, void *opaque); void virNetServerClientClose(virNetServerClientPtr client); +void virNetServerClientCloseLocked(virNetServerClientPtr client); bool virNetServerClientIsClosed(virNetServerClientPtr client); void virNetServerClientDelayedClose(virNetServerClientPtr client); void virNetServerClientImmediateClose(virNetServerClientPtr client); -bool virNetServerClientWantClose(virNetServerClientPtr client); +bool virNetServerClientWantCloseLocked(virNetServerClientPtr client); int virNetServerClientInit(virNetServerClientPtr client); -- 2.13.4

Add virNetServerClientAuthMethodImpliesAuthenticated() for deciding whether a authentication method implies that a client is automatically authenticated or not. Use this new function in virNetServerClientNeedAuth(). Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/rpc/virnetserverclient.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 96fd1e6d15c2..616b6fe115e5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -354,6 +354,23 @@ static void virNetServerClientSockTimerFunc(int timer, } +/** + * virNetServerClientAuthMethodImpliesAuthenticated: + * @auth: authentication method to check + * + * Check if the passed authentication method implies that a client is + * automatically authenticated. + * + * Returns true if @auth implies that a client is automatically + * authenticated, otherwise false. + */ +static bool +virNetServerClientAuthMethodImpliesAuthenticated(int auth) +{ + return auth == VIR_NET_SERVER_SERVICE_AUTH_NONE; +} + + static virNetServerClientPtr virNetServerClientNewInternal(unsigned long long id, virNetSocketPtr sock, @@ -1515,10 +1532,9 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool virNetServerClientNeedAuth(virNetServerClientPtr client) { - bool need = true; + bool need; virObjectLock(client); - if (client->auth == VIR_NET_SERVER_SERVICE_AUTH_NONE) - need = false; + need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); virObjectUnlock(client); return need; } -- 2.13.4

On 12/12/2017 06:36 AM, Marc Hartmayer wrote: I see Daniel has been looking too - and I think if you extract parts of the subsequent patch into this patch with the *Locked name then perhaps there'd be less difference in the subsequent patch. In later patches where virNetServerClientAuthMethodImpliesAuthenticated is used in other parts of the code - I see no reason why we couldn't compare directly to VIR_NET_SERVER_SERVICE_AUTH_NONE. In particular I'm thinking of that auth_pending checking where there's no "client". This then just becomes "Introduce virNetServerClientNeedAuthLocked" John

Merge critical sections for virNetServerProcessClients and take the lock of @client before we're calling virNetServerClientNeedAuth. This is done in preparation for upcoming patches and in addition it is more efficient not to release a lock which is reacquired immediately afterwards. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 3 ++- src/rpc/virnetserver.c | 12 ++++++++---- src/rpc/virnetserverclient.c | 22 ++++++++++++++-------- src/rpc/virnetserverclient.h | 3 ++- 4 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 3ce5694b781d..efdf912ec563 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -139,11 +139,12 @@ virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; virNetServerClientInitKeepAlive; -virNetServerClientIsClosed; +virNetServerClientIsClosedLocked; virNetServerClientIsLocal; virNetServerClientIsSecure; virNetServerClientLocalAddrStringSASL; virNetServerClientNeedAuth; +virNetServerClientNeedAuthLocked; virNetServerClientNew; virNetServerClientNewPostExecRestart; virNetServerClientPreExecRestart; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index dde9b73fc250..d03bd3e91905 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -285,8 +285,10 @@ int virNetServerAddClient(virNetServerPtr srv, goto error; srv->clients[srv->nclients-1] = virObjectRef(client); - if (virNetServerClientNeedAuth(client)) + virObjectLock(client); + if (virNetServerClientNeedAuthLocked(client)) virNetServerTrackPendingAuthLocked(srv); + virObjectUnlock(client); virNetServerCheckLimits(srv); @@ -860,13 +862,13 @@ virNetServerProcessClients(virNetServerPtr srv) virObjectLock(client); if (virNetServerClientWantCloseLocked(client)) virNetServerClientCloseLocked(client); - virObjectUnlock(client); - if (virNetServerClientIsClosed(client)) { + if (virNetServerClientIsClosedLocked(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); - if (virNetServerClientNeedAuth(client)) + if (virNetServerClientNeedAuthLocked(client)) virNetServerTrackCompletedAuthLocked(srv); + virObjectUnlock(client); virNetServerCheckLimits(srv); @@ -875,6 +877,8 @@ virNetServerProcessClients(virNetServerPtr srv) virObjectLock(srv); goto reprocess; + } else { + virObjectUnlock(client); } } diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 616b6fe115e5..3101b555a90d 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1050,13 +1050,10 @@ virNetServerClientClose(virNetServerClientPtr client) } -bool virNetServerClientIsClosed(virNetServerClientPtr client) +/* @client needs to be locked by the caller */ +bool virNetServerClientIsClosedLocked(virNetServerClientPtr client) { - bool closed; - virObjectLock(client); - closed = client->sock == NULL ? true : false; - virObjectUnlock(client); - return closed; + return client->sock == NULL ? true : false; } void virNetServerClientDelayedClose(virNetServerClientPtr client) @@ -1530,11 +1527,20 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, } -bool virNetServerClientNeedAuth(virNetServerClientPtr client) +/* The caller must hold the lock for @client */ +bool +virNetServerClientNeedAuthLocked(virNetServerClientPtr client) +{ + return !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); +} + + +bool +virNetServerClientNeedAuth(virNetServerClientPtr client) { bool need; virObjectLock(client); - need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); + need = virNetServerClientNeedAuthLocked(client); virObjectUnlock(client); return need; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 60ad0f9ed326..9ec18588a858 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -124,7 +124,7 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client, void *opaque); void virNetServerClientClose(virNetServerClientPtr client); void virNetServerClientCloseLocked(virNetServerClientPtr client); -bool virNetServerClientIsClosed(virNetServerClientPtr client); +bool virNetServerClientIsClosedLocked(virNetServerClientPtr client); void virNetServerClientDelayedClose(virNetServerClientPtr client); void virNetServerClientImmediateClose(virNetServerClientPtr client); @@ -147,6 +147,7 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, virNetMessagePtr msg); bool virNetServerClientNeedAuth(virNetServerClientPtr client); +bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client); int virNetServerClientGetTransport(virNetServerClientPtr client); int virNetServerClientGetInfo(virNetServerClientPtr client, -- 2.13.4

On 12/12/2017 06:36 AM, Marc Hartmayer wrote: So essentially you're combining virNetServerClientSetAuth and virNetServerTrackCompletedAuth into one new function and needed to rename the virNetServerClientSetAuth to the Locked variety. Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John one nit below... [...] One line for each argument... [...]

There is a race between virNetServerProcessClients (main thread) and remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker thread) that can lead to decrementing srv->nclients_unauth when it's zero. Since virNetServerCheckLimits relies on the value srv->nclients_unauth the underrun causes libvirtd to stop accepting new connections forever. Example race scenario (assuming libvirtd is using policykit and the client is privileged): 1. The client calls the RPC remoteDispatchAuthList => remoteDispatchAuthList is executed on a worker thread (Thread T1). We're assuming now the execution stops for some time before the line 'virNetServerClientSetAuth(client, 0)' 2. The client closes the connection irregularly. This causes the event loop to wake up and virNetServerProcessClient to be called (on the main thread T0). During the virNetServerProcessClients the srv lock is hold. The condition virNetServerClientNeedAuth(client) will be checked and as the authentication is not finished right now virNetServerTrackCompletedAuthLocked(srv) will be called => --srv->nclients_unauth => 0 3. The Thread T1 continues, marks the client as authenticated, and calls virNetServerTrackCompletedAuthLocked(srv) => --srv->nclients_unauth => --0 => wrap around as nclient_unauth is unsigned 4. virNetServerCheckLimits(srv) will disable the services forever To fix it, add an auth_pending field to the client struct so that it is now possible to determine if the authentication process has already been handled for this client. Setting the authentication method to none for the client in virNetServerProcessClients is not a proper way to indicate that the counter has been decremented, as this would imply that the client is authenticated. Additionally, adjust the existing test cases for this new field. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 2 + src/rpc/virnetserver.c | 28 ++++++++++++-- src/rpc/virnetserverclient.c | 44 +++++++++++++++++++++- src/rpc/virnetserverclient.h | 2 + .../virnetdaemondata/output-data-admin-nomdns.json | 4 ++ .../output-data-admin-server-names.json | 4 ++ .../virnetdaemondata/output-data-anon-clients.json | 2 + tests/virnetdaemondata/output-data-client-ids.json | 2 + .../output-data-client-timestamp.json | 2 + .../output-data-initial-nomdns.json | 2 + tests/virnetdaemondata/output-data-initial.json | 2 + .../output-data-no-keepalive-required.json | 4 ++ 12 files changed, 92 insertions(+), 6 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 7fcae970484d..cef2794e1122 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -138,6 +138,7 @@ virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; virNetServerClientInitKeepAlive; +virNetServerClientIsAuthPendingLocked; virNetServerClientIsClosedLocked; virNetServerClientIsLocal; virNetServerClientIsSecure; @@ -152,6 +153,7 @@ virNetServerClientRemoteAddrStringURI; virNetServerClientRemoveFilter; virNetServerClientSendMessage; virNetServerClientSetAuthLocked; +virNetServerClientSetAuthPendingLocked; virNetServerClientSetCloseHook; virNetServerClientSetDispatcher; virNetServerClientStartKeepAlive; diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 72105cd9318f..6dd673ff3b23 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -286,7 +286,7 @@ int virNetServerAddClient(virNetServerPtr srv, srv->clients[srv->nclients-1] = virObjectRef(client); virObjectLock(client); - if (virNetServerClientNeedAuthLocked(client)) + if (virNetServerClientIsAuthPendingLocked(client)) virNetServerTrackPendingAuthLocked(srv); virObjectUnlock(client); @@ -738,6 +738,25 @@ int virNetServerSetTLSContext(virNetServerPtr srv, /** + * virNetServerSetClientAuthCompletedLocked: + * @srv: server must be locked by the caller + * @client: client must be locked by the caller + * + * Sets on the client that the authentication is no longer pending and + * tracks on @srv that the authentication of this @client has been + * completed. + */ +static void +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client) +{ + if (virNetServerClientIsAuthPendingLocked(client)) { + virNetServerClientSetAuthPendingLocked(client, false); + virNetServerTrackCompletedAuthLocked(srv); + } +} + + +/** * virNetServerSetClientAuthenticated: * @srv: server must be unlocked * @client: client must be unlocked @@ -752,7 +771,7 @@ virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr cl virObjectLock(srv); virObjectLock(client); virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); - virNetServerTrackCompletedAuthLocked(srv); + virNetServerSetClientAuthCompletedLocked(srv, client); virNetServerCheckLimits(srv); virObjectUnlock(client); virObjectUnlock(srv); @@ -870,8 +889,9 @@ virNetServerProcessClients(virNetServerPtr srv) if (virNetServerClientIsClosedLocked(client)) { VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients); - if (virNetServerClientNeedAuthLocked(client)) - virNetServerTrackCompletedAuthLocked(srv); + /* Mark the authentication for this client as no longer + * pending */ + virNetServerSetClientAuthCompletedLocked(srv, client); virObjectUnlock(client); virNetServerCheckLimits(srv); diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 6adc3ec3ec01..5224bc7de1d5 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -71,6 +71,7 @@ struct _virNetServerClient bool delayedClose; virNetSocketPtr sock; int auth; + bool auth_pending; bool readonly; #if WITH_GNUTLS virNetTLSContextPtr tlsCtxt; @@ -375,6 +376,7 @@ static virNetServerClientPtr virNetServerClientNewInternal(unsigned long long id, virNetSocketPtr sock, int auth, + bool auth_pending, #ifdef WITH_GNUTLS virNetTLSContextPtr tls, #endif @@ -384,6 +386,12 @@ virNetServerClientNewInternal(unsigned long long id, { virNetServerClientPtr client; + /* If the used authentication method implies that the new client + * is automatically authenticated, the authentication cannot be + * pending */ + if (auth_pending && virNetServerClientAuthMethodImpliesAuthenticated(auth)) + return NULL; + if (virNetServerClientInitialize() < 0) return NULL; @@ -393,6 +401,7 @@ virNetServerClientNewInternal(unsigned long long id, client->id = id; client->sock = virObjectRef(sock); client->auth = auth; + client->auth_pending = auth_pending; client->readonly = readonly; #ifdef WITH_GNUTLS client->tlsCtxt = virObjectRef(tls); @@ -440,6 +449,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id, { virNetServerClientPtr client; time_t now; + bool auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth); VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, #ifdef WITH_GNUTLS @@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id, return NULL; } - if (!(client = virNetServerClientNewInternal(id, sock, auth, + if (!(client = virNetServerClientNewInternal(id, sock, auth, auth_pending, #ifdef WITH_GNUTLS tls, #endif @@ -486,7 +496,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec virNetServerClientPtr client = NULL; virNetSocketPtr sock; int auth; - bool readonly; + bool readonly, auth_pending; unsigned int nrequests_max; unsigned long long id; long long timestamp; @@ -496,6 +506,17 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec _("Missing auth field in JSON state document")); return NULL; } + + if (!virJSONValueObjectHasKey(object, "auth_pending")) { + auth_pending = !virNetServerClientAuthMethodImpliesAuthenticated(auth); + } else { + if (virJSONValueObjectGetBoolean(object, "auth_pending", &auth_pending) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed auth_pending field in JSON state document")); + return NULL; + } + } + if (virJSONValueObjectGetBoolean(object, "readonly", &readonly) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing readonly field in JSON state document")); @@ -544,6 +565,7 @@ virNetServerClientPtr virNetServerClientNewPostExecRestart(virJSONValuePtr objec if (!(client = virNetServerClientNewInternal(id, sock, auth, + auth_pending, #ifdef WITH_GNUTLS NULL, #endif @@ -592,6 +614,8 @@ virJSONValuePtr virNetServerClientPreExecRestart(virNetServerClientPtr client) if (virJSONValueObjectAppendNumberInt(object, "auth", client->auth) < 0) goto error; + if (virJSONValueObjectAppendBoolean(object, "auth_pending", client->auth_pending) < 0) + goto error; if (virJSONValueObjectAppendBoolean(object, "readonly", client->readonly) < 0) goto error; if (virJSONValueObjectAppendNumberUint(object, "nrequests_max", client->nrequests_max) < 0) @@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client) } +/* The caller must hold the lock for @client */ +void +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending) +{ + client->auth_pending = auth_pending; +} + + +/* The caller must hold the lock for @client */ +bool +virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client) +{ + return client->auth_pending; +} + + static void virNetServerClientKeepAliveDeadCB(void *opaque) { diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 19f20b808284..f608682dc600 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -148,6 +148,8 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool virNetServerClientNeedAuth(virNetServerClientPtr client); bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client); +bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client); +void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending); int virNetServerClientGetTransport(virNetServerClientPtr client); int virNetServerClientGetInfo(virNetServerClientPtr client, diff --git a/tests/virnetdaemondata/output-data-admin-nomdns.json b/tests/virnetdaemondata/output-data-admin-nomdns.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-admin-nomdns.json +++ b/tests/virnetdaemondata/output-data-admin-nomdns.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-admin-server-names.json b/tests/virnetdaemondata/output-data-admin-server-names.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-admin-server-names.json +++ b/tests/virnetdaemondata/output-data-admin-server-names.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-anon-clients.json b/tests/virnetdaemondata/output-data-anon-clients.json index 9f1c63504a1b..cb6005bfe68c 100644 --- a/tests/virnetdaemondata/output-data-anon-clients.json +++ b/tests/virnetdaemondata/output-data-anon-clients.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-client-ids.json b/tests/virnetdaemondata/output-data-client-ids.json index 43c61cc46ec7..2b1663d4f8c6 100644 --- a/tests/virnetdaemondata/output-data-client-ids.json +++ b/tests/virnetdaemondata/output-data-client-ids.json @@ -41,6 +41,7 @@ { "id": 2, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 3, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-client-timestamp.json b/tests/virnetdaemondata/output-data-client-timestamp.json index b706c147ed0e..e8c8e9a991d9 100644 --- a/tests/virnetdaemondata/output-data-client-timestamp.json +++ b/tests/virnetdaemondata/output-data-client-timestamp.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "conn_time": 1234567890, @@ -54,6 +55,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "conn_time": 1234567890, diff --git a/tests/virnetdaemondata/output-data-initial-nomdns.json b/tests/virnetdaemondata/output-data-initial-nomdns.json index ca6b99553754..167aef8d295e 100644 --- a/tests/virnetdaemondata/output-data-initial-nomdns.json +++ b/tests/virnetdaemondata/output-data-initial-nomdns.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-initial.json b/tests/virnetdaemondata/output-data-initial.json index a8df6336c316..3281e868d7aa 100644 --- a/tests/virnetdaemondata/output-data-initial.json +++ b/tests/virnetdaemondata/output-data-initial.json @@ -42,6 +42,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -54,6 +55,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { diff --git a/tests/virnetdaemondata/output-data-no-keepalive-required.json b/tests/virnetdaemondata/output-data-no-keepalive-required.json index fc960f02cee7..d6d02163e282 100644 --- a/tests/virnetdaemondata/output-data-no-keepalive-required.json +++ b/tests/virnetdaemondata/output-data-no-keepalive-required.json @@ -41,6 +41,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -53,6 +54,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { @@ -105,6 +107,7 @@ { "id": 1, "auth": 1, + "auth_pending": true, "readonly": true, "nrequests_max": 15, "sock": { @@ -117,6 +120,7 @@ { "id": 2, "auth": 2, + "auth_pending": true, "readonly": true, "nrequests_max": 66, "sock": { -- 2.13.4

On Fri, Dec 15, 2017 at 07:16 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: I’m not sure it’s necessary as the correct usage of auth_pending is already the fix. But let’s see then. Changed it. Changed. Changed. Normally I would prefer an assert here… The intent behind this guard was that it should prevent the wrong usage of this function. But I’ll move the check. Does it makes sense to leave the check here (and additionally add it to ExecRestart) and throw an error message here? This function/condition is used in four places. But sure, if you’re still saying it’s easier to read and to understand that a user is authenticated (and therefore the authentication is no longer pending) if auth_method == VIR_NET_SERVER_SERVICE_AUTH_NONE then I’ll replace this function at all places :) […snip…] Changed. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On Thu, Dec 21, 2017 at 01:18 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: […snip…] Yep. That’s why I like asserts… (for functions used internally only). You can 'tell' other developers what are the preconditions/postconditions and help them to understand the code flow. Sorry for the off topic… Short answer, I’ll move the check. Me too. I’ll think about it. […snip] -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

'Squash' virNetServerClientNeedAuthLocked into virNetServerClientNeedAuth and remove virNetServerClientNeedAuthLocked as it's not longer needed. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 1 - src/rpc/virnetserverclient.c | 10 +--------- src/rpc/virnetserverclient.h | 1 - 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index cef2794e1122..10583dc241ad 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -144,7 +144,6 @@ virNetServerClientIsLocal; virNetServerClientIsSecure; virNetServerClientLocalAddrStringSASL; virNetServerClientNeedAuth; -virNetServerClientNeedAuthLocked; virNetServerClientNew; virNetServerClientNewPostExecRestart; virNetServerClientPreExecRestart; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 5224bc7de1d5..81dfc41a5967 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1550,20 +1550,12 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, } -/* The caller must hold the lock for @client */ -bool -virNetServerClientNeedAuthLocked(virNetServerClientPtr client) -{ - return !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); -} - - bool virNetServerClientNeedAuth(virNetServerClientPtr client) { bool need; virObjectLock(client); - need = virNetServerClientNeedAuthLocked(client); + need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); virObjectUnlock(client); return need; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index f608682dc600..6f20ab4c06a0 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -147,7 +147,6 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, virNetMessagePtr msg); bool virNetServerClientNeedAuth(virNetServerClientPtr client); -bool virNetServerClientNeedAuthLocked(virNetServerClientPtr client); bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client); void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending); -- 2.13.4

Replace virNetServerClientNeedAuth with virNetServerClientIsAuthenticated because it makes it clearer what it means. Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; Reviewed-by: Stefan Zimmermann <stzi(a)linux.vnet.ibm.com&gt; --- src/libvirt_remote.syms | 2 +- src/rpc/virnetserverclient.c | 8 ++++---- src/rpc/virnetserverclient.h | 2 +- src/rpc/virnetserverprogram.c | 7 +++---- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms index 10583dc241ad..3ba1e3f33243 100644 --- a/src/libvirt_remote.syms +++ b/src/libvirt_remote.syms @@ -138,12 +138,12 @@ virNetServerClientGetUNIXIdentity; virNetServerClientImmediateClose; virNetServerClientInit; virNetServerClientInitKeepAlive; +virNetServerClientIsAuthenticated; virNetServerClientIsAuthPendingLocked; virNetServerClientIsClosedLocked; virNetServerClientIsLocal; virNetServerClientIsSecure; virNetServerClientLocalAddrStringSASL; -virNetServerClientNeedAuth; virNetServerClientNew; virNetServerClientNewPostExecRestart; virNetServerClientPreExecRestart; diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index 81dfc41a5967..24994bc471b8 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -1551,13 +1551,13 @@ int virNetServerClientSendMessage(virNetServerClientPtr client, bool -virNetServerClientNeedAuth(virNetServerClientPtr client) +virNetServerClientIsAuthenticated(virNetServerClientPtr client) { - bool need; + bool authenticated; virObjectLock(client); - need = !virNetServerClientAuthMethodImpliesAuthenticated(client->auth); + authenticated = virNetServerClientAuthMethodImpliesAuthenticated(client->auth); virObjectUnlock(client); - return need; + return authenticated; } diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h index 6f20ab4c06a0..66ef5cd68235 100644 --- a/src/rpc/virnetserverclient.h +++ b/src/rpc/virnetserverclient.h @@ -146,7 +146,7 @@ const char *virNetServerClientRemoteAddrStringURI(virNetServerClientPtr client); int virNetServerClientSendMessage(virNetServerClientPtr client, virNetMessagePtr msg); -bool virNetServerClientNeedAuth(virNetServerClientPtr client); +bool virNetServerClientIsAuthenticated(virNetServerClientPtr client); bool virNetServerClientIsAuthPendingLocked(virNetServerClientPtr client); void virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending); diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 1e8dfc2f1128..557651ffbd91 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -397,11 +397,10 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, goto error; } - /* If client is marked as needing auth, don't allow any RPC ops - * which are except for authentication ones - */ + /* If the client is not authenticated, don't allow any RPC ops + * which are except for authentication ones */ if (dispatcher->needAuth && - virNetServerClientNeedAuth(client)) { + !virNetServerClientIsAuthenticated(client)) { /* Explicitly *NOT* calling remoteDispatchAuthError() because we want back-compatibility with libvirt clients which don't support the VIR_ERR_AUTH_FAILED error code */ -- 2.13.4