[pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Filip Schauer]

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

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
Changes since v1:
* Do not try to URI unescape the user attributes, since we do that later
  in PVE::AccessControl::parse_user_config anyways.

 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..06177db 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} = Encode::decode('utf8', $user_attributes->{$attr}->[0]);
 	    }
 	}
 
-- 
2.39.2

Re: [pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Fiona Ebner]

Am 09.01.24 um 14:35 schrieb Filip Schauer:
> UTF8 decode non-ASCII characters when syncing user attributes, since
> those will be encoded later on. Without this fix the attributes were
> encoded twice, resulting in cases such as 'ü' turning into 'ü'.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>

Is this already fixed by avoiding the accidental re-encoding as UTF-8,
i.e. the following patch:
https://lists.proxmox.com/pipermail/pve-devel/2024-January/061270.html
?

Re: [pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Filip Schauer]

I just tried your patch, but it did not fix this specific issue.

On 09/01/2024 14:38, Fiona Ebner wrote:
> Am 09.01.24 um 14:35 schrieb Filip Schauer:
>> UTF8 decode non-ASCII characters when syncing user attributes, since
>> those will be encoded later on. Without this fix the attributes were
>> encoded twice, resulting in cases such as 'ü' turning into 'ü'.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> Is this already fixed by avoiding the accidental re-encoding as UTF-8,
> i.e. the following patch:
> https://lists.proxmox.com/pipermail/pve-devel/2024-January/061270.html
> ?

Re: [pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Thomas Lamprecht]

Am 09/01/2024 um 14:35 schrieb Filip Schauer:
> UTF8 decode non-ASCII characters when syncing user attributes, since
> those will be encoded later on. Without this fix the attributes were
> encoded twice, resulting in cases such as 'ü' turning into 'ü'.
> 
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
> Changes since v1:
> * Do not try to URI unescape the user attributes, since we do that later
>   in PVE::AccessControl::parse_user_config anyways.
> 
>  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..06177db 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} = Encode::decode('utf8', $user_attributes->{$attr}->[0]);
>  	    }
>  	}
>  

this would need a rebase, oh, and would be great if the original testers
could reconfirm the v2 approach of doing utf-8 decoding only.

Re: [pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Fiona Ebner]

Am 28.02.24 um 15:41 schrieb Thomas Lamprecht:
> Am 09/01/2024 um 14:35 schrieb Filip Schauer:
>> UTF8 decode non-ASCII characters when syncing user attributes, since
>> those will be encoded later on. Without this fix the attributes were
>> encoded twice, resulting in cases such as 'ü' turning into 'ü'.
>>
>> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
>> ---
>> Changes since v1:
>> * Do not try to URI unescape the user attributes, since we do that later
>>   in PVE::AccessControl::parse_user_config anyways.
>>
>>  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..06177db 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} = Encode::decode('utf8', $user_attributes->{$attr}->[0]);

Note: missing use Encode; at the beginning of the file.

>>  	    }
>>  	}
>>  
> 
> this would need a rebase, oh, and would be great if the original testers
> could reconfirm the v2 approach of doing utf-8 decoding only.
> 

Gave it a quick test and fixes issues with special characters for me.
Don't forget to also use the latest master of pve-cluster, otherwise
writing the user config will still do the wrong thing [0]! Both are
needed to fix the issue here. I'm just wondering if we are guaranteed
that the LDAP server sends UTF-8 encoded data?

[0]:
https://git.proxmox.com/?p=pve-cluster.git;a=commit;h=2e276ccd9beb2004ddd72396b2a9b72a288771d8

Re: [pve-devel] [PATCH v2 access-control] fix #5136: ldap: Decode non-ASCII characters in attributes

[Stoiko Ivanov]

On Wed, 28 Feb 2024 16:00:48 +0100
Fiona Ebner <f.ebner@proxmox.com> wrote:

> Am 28.02.24 um 15:41 schrieb Thomas Lamprecht:
> > Am 09/01/2024 um 14:35 schrieb Filip Schauer:  
> >> UTF8 decode non-ASCII characters when syncing user attributes, since
> >> those will be encoded later on. Without this fix the attributes were
> >> encoded twice, resulting in cases such as 'ü' turning into 'ü'.
> >>
> >> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> >> ---
> >> Changes since v1:
> >> * Do not try to URI unescape the user attributes, since we do that later
> >>   in PVE::AccessControl::parse_user_config anyways.
> >>
> >>  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..06177db 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} = Encode::decode('utf8', $user_attributes->{$attr}->[0]);  
> 
> Note: missing use Encode; at the beginning of the file.
> 
> >>  	    }
> >>  	}
> >>    
> > 
> > this would need a rebase, oh, and would be great if the original testers
> > could reconfirm the v2 approach of doing utf-8 decoding only.
> >   
> 
> Gave it a quick test and fixes issues with special characters for me.
> Don't forget to also use the latest master of pve-cluster, otherwise
> writing the user config will still do the wrong thing [0]! Both are
> needed to fix the issue here. I'm just wondering if we are guaranteed
> that the LDAP server sends UTF-8 encoded data?
sadly (or luckily) not too much experience with validity of LDAP data out
in the wild. Quickly searched online and went through the rfc-chain until
there was not Link to "Obsoleted by" anymore (and then going through all
RFC indexed there [0]:
The (~18 year old) standard indicates that strings used should be UTF-8
encoded:
https://datatracker.ietf.org/doc/html/rfc4511#section-4.1.2
(and pointed out the (by now probably not significant difference between
unicode and ISO10646 - see [1]).

However, probably with any protocol that has been around for 30+ years -
guarantees are hard to come by:
https://datatracker.ietf.org/doc/html/rfc4512#section-7.2

anyways - iiuc we can just skip the syncing of the attribute in this part?
- if we add a warning to the log it sounds ok to me (but I only very
  quickly skimmed through what the code does)


[0] https://datatracker.ietf.org/doc/html/rfc4510
[1] https://www.unicode.org/versions/Unicode15.0.0/appC.pdf
> 
> [0]:
> https://git.proxmox.com/?p=pve-cluster.git;a=commit;h=2e276ccd9beb2004ddd72396b2a9b72a288771d8
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel