* 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