[pve-devel] [PATCH access-control 1/2] ldap: Allow quoted values for DN attribute values

[Dominik Csapak]

hi,

so high level comment:
i'd write most of what you wrote in the cover letter here in the commit message,
makes it much more convenient to find it only via git ;)

also i'm missing a bit the rationale for how the regex was chosen, besides
that it works in some conditions

further comment inline

On 1/31/23 13:50, Christoph Heiss wrote:
> Signed-off-by: Christoph Heiss <c.heiss at proxmox.com>
> ---
>   src/PVE/Auth/LDAP.pm | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/Auth/LDAP.pm b/src/PVE/Auth/LDAP.pm
> index 4792586..4d771e7 100755
> --- a/src/PVE/Auth/LDAP.pm
> +++ b/src/PVE/Auth/LDAP.pm
> @@ -10,6 +10,8 @@ use PVE::Tools;
> 
>   use base qw(PVE::Auth::Plugin);
> 
> +our $dn_regex = qr!\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+)(,\s*\w+=("[\w ,+/<>;=]+"|[^ ,+"/<>;=]+))*!;

are you sure you did not make it more strict than what is allowed?

e.g. if i had 'foo=<,bar=>' that would have previously worked, but now is forbidden AFAICS
while we can make such changes, we should only do so on major releases where it's a breaking
change, preferably with a workaround and/or script where we can rewrite/warn the user
that it's not valid syntax

OTOH, most users probably won't notice since they did not use such 'strange' values

the problem here is that possibly working configs are not valid anymore
(for logins it's problematic, depending on how the admins log in)

> +
>   sub type {
>       return 'ldap';
>   }
> @@ -19,7 +21,7 @@ sub properties {
>   	base_dn => {
>   	    description => "LDAP base domain name",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> @@ -33,7 +35,7 @@ sub properties {
>   	bind_dn => {
>   	    description => "LDAP bind domain name",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> @@ -91,7 +93,7 @@ sub properties {
>   	    description => "LDAP base domain name for group sync. If not set, the"
>   		." base_dn will be used.",
>   	    type => 'string',
> -	    pattern => '\w+=[^,]+(,\s*\w+=[^,]+)*',
> +	    pattern => $dn_regex,
>   	    optional => 1,
>   	    maxLength => 256,
>   	},
> --
> 2.34.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>