1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 00/14] Fix race on srv->nclients_unauth and some other changes
  4. View patch

[libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed

Marc Hartmayer posted 14 patches 7 years, 5 months ago
There is a newer version of this series
[libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by Marc Hartmayer 7 years, 5 months ago 
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(-)

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
Re: [libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by John Ferlan 7 years, 5 months ago 

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.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by Marc Hartmayer 7 years, 4 months ago 
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by John Ferlan 7 years, 5 months ago 

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

or in one ugly C line ;-)

need = !(client->auth == VIR_NET_SERVER_SERVICE_AUTH_NONE);

or

need = client->auth != VIR_NET_SERVER_SERVICE_AUTH_NONE;

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

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by Daniel P. Berrange 7 years, 5 months ago 
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.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/14] rpc: Be more precise in which cases the authentication is needed
Posted by Marc Hartmayer 7 years, 4 months ago 
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


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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.