On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- 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);

Makes me curious why virNetServerClientClose isn't doing the Free for the rx/tx. Oh, I see, the test program isn't calling it. I think this is the right idea, but the issue is in the test program doing the Unref before calling virNetServerClientClose. If you see how other tests/code handle this, the client usually gets added to the net server [n]clients list (virNetServerAddClient) and later when netserver is disposed the nclients is perused and calls virNetServerClientClose prior to the Unref as well for each client. So if you make the right call in tests/virnetserverclienttest.c, then you have my Reviewed-by: John Ferlan <jferlan@redhat.com> John

}

On 12/21/2017 06:32 AM, Marc Hartmayer wrote:

On Fri, Dec 15, 2017 at 01:14 PM +0100, John Ferlan <jferlan@redhat.com> wrote:

...

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

...

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- 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);

Makes me curious why virNetServerClientClose isn't doing the Free for the rx/tx. Oh, I see, the test program isn't calling it.

I think this is the right idea, but the issue is in the test program doing the Unref before calling virNetServerClientClose.

Yes. Will add an patch for this. Should I remove this patch then?

Yes probably should have been explicit - the R-B below was conditional on altering this patch to call virNetServerClientClose in the test program instead of in virNetServerClientDispose. John

...

If you see how other tests/code handle this, the client usually gets added to the net server [n]clients list (virNetServerAddClient) and later when netserver is disposed the nclients is perused and calls virNetServerClientClose prior to the Unref as well for each client.

So if you make the right call in tests/virnetserverclienttest.c, then you have my

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

...

}

-- 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 Fri, Dec 15, 2017 at 01:37 PM +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:

On Tue, Dec 12, 2017 at 12:36:27PM +0100, Marc Hartmayer wrote:

...

Additionally, use a whitelist model to decide whether authentication is needed or not.

Is this actually fixing any real problem, if so please document what the problem is.

It 'tells' the developer what we’re doing here. At first glance, it wasn’t obvious to me how the authentication process works in libvirt. This change doesn’t hurt someone, but at least it should be easier for other developer to understand. Additionally, the default return value is changing to True (=> client is not authenticated). So if something may change at the authentication process (for example the value for the enums - I know it’s unlikely to happen here…) we’re sure that the user isn’t authenticated by accident.

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 ?

...

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- 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

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

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

-- 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 Fri, Dec 15, 2017 at 02:45 PM +0100, John Ferlan <jferlan@redhat.com> wrote:

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

...

Additionally, use a whitelist model to decide whether authentication is needed or not.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/rpc/virnetserverclient.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

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.

Yep makes sense. Will change it.

Hazards of not peeking forward by me.

John

...

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; }

-- 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 Tue, Dec 12, 2017 at 12:36:30PM +0100, Marc Hartmayer wrote:

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- 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; +}

This just seems to be functionally identical to the existing virNetServerClientNeedAuth method you're refactoring. The only difference is whether ther mutex is held or not. Giving it a completely different name is just confusing in this case. Our normal practice is to just name the method the same, but add a "Locked" suffix to indicate that the caller must already hold the lock.

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

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

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 Fri, Dec 15, 2017 at 02:16 PM +0100, John Ferlan <jferlan@redhat.com> wrote:

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

...

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/rpc/virnetserverclient.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)

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.

The first time I read the code it was very strange to me that a user is authenticated when the authentication method was set to none. This was also the reason why I added this function - I tried to make it easier to understand this code part. But if you think it’s self-explanatory enough to test for none, then of course I can replace it :) Thanks for reviewing.

In particular I'm thinking of that auth_pending checking where there's no "client".

This then just becomes "Introduce virNetServerClientNeedAuthLocked"

John

...

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; }

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

-- 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 12/12/2017 06:36 AM, Marc Hartmayer wrote:

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- 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(-)

I think if you split out the virNetServerClientIsClosedLocked into its own patch before/after the virNetServerClientNeedAuthLocked, then perhaps patch 7 and 9 are combined. It's just a patch ordering and trying not to do too much in one patch.

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)

May as well go with the multi line here too bool virNetServer... John

{ - 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,

On Fri, Dec 15, 2017 at 03:26 PM +0100, John Ferlan <jferlan@redhat.com> wrote:

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

...

Introduce a function which marks the client as authenticated and also it tracks on the server that the authentication for this client has been completed. Afterwords it will check for the limits of the server.

After using this new function the function virNetServerTrackCompletedAuth was superfluous and is therefore removed. In addition, it is not very common that a '{{function}}' (virNetServerTrackCompletedAuth) does more than just the locking compared to '{{function}}Locked' (virNetServerTrackCompletedAuthLocked).

virNetServerTrackPendingAuth was already superfluous and therefore it's also removed.

So essentially you're combining virNetServerClientSetAuth and virNetServerTrackCompletedAuth into one new function and needed to rename the virNetServerClientSetAuth to the Locked variety.

Yes. I’ll rewrite the commit message.

...

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- daemon/remote.c | 9 +++------ src/libvirt_remote.syms | 5 ++--- src/rpc/virnetserver.c | 40 ++++++++++++++++++++++------------------ src/rpc/virnetserver.h | 3 +-- src/rpc/virnetserverclient.c | 5 ++--- src/rpc/virnetserverclient.h | 2 +- 6 files changed, 31 insertions(+), 33 deletions(-)

Reviewed-by: John Ferlan <jferlan@redhat.com>

Thanks.

John

one nit below...

[...]

...

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index d03bd3e91905..72105cd9318f 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -737,6 +737,28 @@ int virNetServerSetTLSContext(virNetServerPtr srv, #endif

+/** + * virNetServerSetClientAuthenticated: + * @srv: server must be unlocked + * @client: client must be unlocked + * + * Mark @client as authenticated and tracks on @srv that the + * authentication of this @client has been completed. Also it checks + * the limits of @srv. + */ +void +virNetServerSetClientAuthenticated(virNetServerPtr srv, virNetServerClientPtr client)

One line for each argument...

...

+{ + virObjectLock(srv); + virObjectLock(client); + virNetServerClientSetAuthLocked(client, VIR_NET_SERVER_SERVICE_AUTH_NONE); + virNetServerTrackCompletedAuthLocked(srv); + virNetServerCheckLimits(srv); + virObjectUnlock(client); + virObjectUnlock(srv); +} + +

[...]

-- 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 12/12/2017 06:36 AM, Marc Hartmayer wrote:

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@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- 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(-)

Ah... the patch I've been waiting for ;-)... What caused me to initially look at this series is because I have bz related to some possible sort of timing problem with TLS authentication, so I figured reviewing a series related to this code would help me understand the flow a bit better. In any case, I think perhaps this can be split into two patches - one that just sets/clears the auth_pending and then what's leftover as the fix.

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.

If the client authentication was pending, clear that pending and update the server tracking.

+ */ +static void +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)

One line for each argument.

+{ + 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 */

Some may say a superfluous comment... I'd probably say make it one line /* Update server authentication tracking */

+ 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)) ^^^^^^ Why check twice? I'm not clear on the need for this check. The setting of auth_pending is based on @auth in the caller or perhaps the value of auth_pending already. If there's some sort of check to be made perhaps it's in ExecRestart.

Also, if we returned NULL here without some sort of error, then couldn't we get the libvirt failed for some reason error message which is always not fun to track down.

+ 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);

I think it's much easier for readability to have result be auth != VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even easier to find all instances of AUTH_NONE.

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);

same here - since not found in .json, then set based on auth != AUTH_NONE would be just clearer I think.

+ } 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)

One line per argument John

+{ + 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": {

On 12/21/2017 06:29 AM, Marc Hartmayer wrote:

On Fri, Dec 15, 2017 at 07:16 PM +0100, John Ferlan <jferlan@redhat.com> wrote:

...

On 12/12/2017 06:36 AM, Marc Hartmayer wrote:

...

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.

In any case, I think perhaps this can be split into two patches - one that just sets/clears the auth_pending and then what's leftover as the fix.

I’m not sure it’s necessary as the correct usage of auth_pending is already the fix. But let’s see then.

OK. I understand. I assume by this point I was juggling in my head a few of the various adjustments so I figured that by adding the flag and separating the logic between just the set/clear flag and the reason it was added would be easier to review, but then again that would make the set/clear patch essentially meaningless.

...

...

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.

If the client authentication was pending, clear that pending and update the server tracking.

Changed it.

...

...

+ */ +static void +virNetServerSetClientAuthCompletedLocked(virNetServerPtr srv, virNetServerClientPtr client)

One line for each argument.

Changed.

...

...

+{ + 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 */

Some may say a superfluous comment... I'd probably say make it one line

/* Update server authentication tracking */

Changed.

...

...

+ 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)) ^^^^^^ Why check twice? I'm not clear on the need for this check. The setting of auth_pending is based on @auth in the caller or perhaps the value of auth_pending already. If there's some sort of check to be made perhaps it's in ExecRestart.

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.

I think this is perhaps why I made the separate the patches comment. I was starting to overflow my heap.

...

Also, if we returned NULL here without some sort of error, then couldn't we get the libvirt failed for some reason error message which is always not fun to track down.

Does it makes sense to leave the check here (and additionally add it to ExecRestart) and throw an error message here?

Adding a check, a message, and having a NULL return to guard against some future change passing the wrong parameters perhaps means that future patch submitter (and eventual reviewer) didn't clearly understand the purpose of the function... Still this is a *NewInternal function - I would hope passing correct arguments wouldn't be problem for it.

...

...

+ 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);

I think it's much easier for readability to have result be auth != VIR_NET_SERVER_SERVICE_AUTH_NONE instead of this call... Perhaps even easier to find all instances of AUTH_NONE.

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

By this time I wasn't sure what was going to happen from Dan's patch 8 comment and the virNetServerClientNeedAuth[Locked] functions vs. the name chosen (ImpliesAuthenticated) that was AFAICT trying to convey what you discovered. Whether the comparison is hidden behind some call or a direct comparison I guess just becomes an implementation detail. I use cscope a lot to find things, so searching on NONE is perhaps easier that searching on NONE, finding some API that returning true/false based off of NONE, and then searching on callers for that too. OTOH, it's cleaner to have a helper <sigh>. Shorter answer, your call. If you want the helper, that's fine. John

...

...

VIR_DEBUG("sock=%p auth=%d tls=%p", sock, auth, #ifdef WITH_GNUTLS @@ -454,7 +464,7 @@ virNetServerClientPtr virNetServerClientNew(unsigned long long id,

[…snip…]

...

...

@@ -1545,6 +1569,22 @@ virNetServerClientNeedAuth(virNetServerClientPtr client) }

+/* The caller must hold the lock for @client */ +void +virNetServerClientSetAuthPendingLocked(virNetServerClientPtr client, bool auth_pending)

One line per argument

Changed.

...

John

...

+{ + client->auth_pending = auth_pending;

[…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 12/12/2017 06:36 AM, Marc Hartmayer wrote:

'Squash' virNetServerClientNeedAuthLocked into virNetServerClientNeedAuth and remove virNetServerClientNeedAuthLocked as it's not longer needed.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com> --- src/libvirt_remote.syms | 1 - src/rpc/virnetserverclient.c | 10 +--------- src/rpc/virnetserverclient.h | 1 - 3 files changed, 1 insertion(+), 11 deletions(-)

I have a feeling this may change given the comments already - so I'll stop here. John

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);