public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
@ 2023-12-20 14:37 Filip Schauer
  2023-12-21  9:42 ` Lukas Wagner
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Filip Schauer @ 2023-12-20 14:37 UTC (permalink / raw)
  To: pve-devel

Decode non-ASCII character when syncing user attributes, since those
will be encoded later on. Without this fix the attributes where encoded
twice, resulting in cases such as 'ü' turning into 'ü'.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Auth/LDAP.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
index b958f2b..5e7a30c 100755
--- a/src/PVE/Auth/LDAP.pm
+++ b/src/PVE/Auth/LDAP.pm
@@ -301,7 +301,7 @@ sub get_users {
 
 	foreach my $attr (keys %$user_attributes) {
 	    if (my $ours = $ldap_attribute_map->{$attr}) {
-		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
+		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);
 	    }
 	}
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
  2023-12-20 14:37 [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes Filip Schauer
@ 2023-12-21  9:42 ` Lukas Wagner
  2023-12-21  9:46 ` Christoph Heiss
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2023-12-21  9:42 UTC (permalink / raw)
  To: Proxmox VE development discussion, Filip Schauer

Hi, thanks for tackling this!

On 12/20/23 15:37, Filip Schauer wrote:
> Decode non-ASCII character when syncing user attributes, since those
> will be encoded later on. Without this fix the attributes where encoded
> twice, resulting in cases such as 'ü' turning into 'ü'.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>   src/PVE/Auth/LDAP.pm | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index b958f2b..5e7a30c 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -301,7 +301,7 @@ sub get_users {
>   
>   	foreach my $attr (keys %$user_attributes) {
>   	    if (my $ours = $ldap_attribute_map->{$attr}) {
> -		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
> +		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);
>   	    }
>   	}
>   

Gave this a quick test.
Set up a glauth LDAP server, added some unicode symbols to the 'sn' LDAP 
attribute, configured sync_attributes lastname=sn in domains.cfg and
tested the sync.

Tested-by: Lukas Wagner <l.wagner@proxmox.com>


-- 
- Lukas




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
  2023-12-20 14:37 [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes Filip Schauer
  2023-12-21  9:42 ` Lukas Wagner
@ 2023-12-21  9:46 ` Christoph Heiss
  2023-12-21 10:03 ` Lukas Wagner
  2024-01-08  9:26 ` Wolfgang Bumiller
  3 siblings, 0 replies; 6+ messages in thread
From: Christoph Heiss @ 2023-12-21  9:46 UTC (permalink / raw)
  To: Filip Schauer; +Cc: Proxmox VE development discussion


Tested it using Windows Server 2022 and Samba 4.19.2 on Linux, with
both LDAP and AD realms.

Fixes the problem after a re-sync, LGTM.

Tested-by: Christoph Heiss <c.heiss@proxmox.com>

On Wed, Dec 20, 2023 at 03:37:03PM +0100, Filip Schauer wrote:
> Decode non-ASCII character when syncing user attributes, since those
> will be encoded later on. Without this fix the attributes where encoded
> twice, resulting in cases such as 'ü' turning into 'ü'.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Auth/LDAP.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index b958f2b..5e7a30c 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -301,7 +301,7 @@ sub get_users {
>
>  	foreach my $attr (keys %$user_attributes) {
>  	    if (my $ours = $ldap_attribute_map->{$attr}) {
> -		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
> +		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);
>  	    }
>  	}
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
  2023-12-20 14:37 [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes Filip Schauer
  2023-12-21  9:42 ` Lukas Wagner
  2023-12-21  9:46 ` Christoph Heiss
@ 2023-12-21 10:03 ` Lukas Wagner
  2024-01-08  9:26 ` Wolfgang Bumiller
  3 siblings, 0 replies; 6+ messages in thread
From: Lukas Wagner @ 2023-12-21 10:03 UTC (permalink / raw)
  To: Proxmox VE development discussion, Filip Schauer



On 12/20/23 15:37, Filip Schauer wrote:
> Decode non-ASCII character when syncing user attributes, since those
> will be encoded later on. Without this fix the attributes where encoded
> twice, resulting in cases such as 'ü' turning into 'ü'.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>   src/PVE/Auth/LDAP.pm | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index b958f2b..5e7a30c 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -301,7 +301,7 @@ sub get_users {
>   
>   	foreach my $attr (keys %$user_attributes) {
>   	    if (my $ours = $ldap_attribute_map->{$attr}) {
> -		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
> +		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);
>   	    }
>   	}
>   

For the record, the reporter of the issue also confirmed that the fix 
works for them:

https://bugzilla.proxmox.com/show_bug.cgi?id=5136

-- 
- Lukas




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
  2023-12-20 14:37 [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes Filip Schauer
                   ` (2 preceding siblings ...)
  2023-12-21 10:03 ` Lukas Wagner
@ 2024-01-08  9:26 ` Wolfgang Bumiller
  2024-01-09 13:36   ` Filip Schauer
  3 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2024-01-08  9:26 UTC (permalink / raw)
  To: Filip Schauer; +Cc: pve-devel

On Wed, Dec 20, 2023 at 03:37:03PM +0100, Filip Schauer wrote:
> Decode non-ASCII character when syncing user attributes, since those

decode *how*?

> will be encoded later on. Without this fix the attributes where encoded
> twice, resulting in cases such as 'ü' turning into 'ü'.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/PVE/Auth/LDAP.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index b958f2b..5e7a30c 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -301,7 +301,7 @@ sub get_users {
>  
>  	foreach my $attr (keys %$user_attributes) {
>  	    if (my $ours = $ldap_attribute_map->{$attr}) {
> -		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
> +		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);

This does 2 things: URI unescaping and utf-8 decoding.
Does the former make sense?

Given that 'decode_text' is a way too generic name in a module with yet
another way too generic name "tools", I'd argue against its use in
general and would prefer to call the *actual* decode function right
there so you can see what's going on.

>  	    }
>  	}
>  
> -- 
> 2.39.2




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes
  2024-01-08  9:26 ` Wolfgang Bumiller
@ 2024-01-09 13:36   ` Filip Schauer
  0 siblings, 0 replies; 6+ messages in thread
From: Filip Schauer @ 2024-01-09 13:36 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pve-devel

Patch v2 is available:

https://lists.proxmox.com/pipermail/pve-devel/2024-January/061273.html

On 08/01/2024 10:26, Wolfgang Bumiller wrote:
> On Wed, Dec 20, 2023 at 03:37:03PM +0100, Filip Schauer wrote:
>> Decode non-ASCII character when syncing user attributes, since those
> decode *how*?
>
>> will be encoded later on. Without this fix the attributes where encoded
>> twice, resulting in cases such as 'ü' turning into 'ü'.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>>   src/PVE/Auth/LDAP.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
>> index b958f2b..5e7a30c 100755
>> --- a/src/PVE/Auth/LDAP.pm
>> +++ b/src/PVE/Auth/LDAP.pm
>> @@ -301,7 +301,7 @@ sub get_users {
>>   
>>   	foreach my $attr (keys %$user_attributes) {
>>   	    if (my $ours = $ldap_attribute_map->{$attr}) {
>> -		$ret->{$username}->{$ours} = $user_attributes->{$attr}->[0];
>> +		$ret->{$username}->{$ours} = PVE::Tools::decode_text($user_attributes->{$attr}->[0]);
> This does 2 things: URI unescaping and utf-8 decoding.
> Does the former make sense?
>
> Given that 'decode_text' is a way too generic name in a module with yet
> another way too generic name "tools", I'd argue against its use in
> general and would prefer to call the *actual* decode function right
> there so you can see what's going on.
>
>>   	    }
>>   	}
>>   
>> -- 
>> 2.39.2




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-01-09 13:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-20 14:37 [pve-devel] [PATCH access-control] fix #5136: ldap: Decode non-ASCII characters in attributes Filip Schauer
2023-12-21  9:42 ` Lukas Wagner
2023-12-21  9:46 ` Christoph Heiss
2023-12-21 10:03 ` Lukas Wagner
2024-01-08  9:26 ` Wolfgang Bumiller
2024-01-09 13:36   ` Filip Schauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal