Am 15/11/2023 um 14:28 schrieb Stefan Sterz:
> On 15.11.23 13:23, Markus Frank wrote:
>> Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/"
>> to allow hyphen in ldap attribute names for pve & pmg.
>>
>> Signed-off-by: Markus Frank <m.fr...@proxmox.com>
>> ---
>> There does not seem to be a regex for LDAP attributes in pbs.
>> Should a regex be added for this?
>>
>
> we recently moved away from using regex for validating a LDAP
> configuration, for two reasons:
>
> 1. turns out finding a regex that validates all possible valid LDAP DNs
> is pretty hard and fixing this often comes along with breaking older
> setups
> 2. even a valid *looking* DN may not actual work against a real LDAP
> server.
>
> so instead we now actually try to query an LDAP server with the provided
> config and see if that returns anything. thus, users are more likely to
> get what they want, and we don't have to validate for every possible case.
>
> i guess there could be an argument why a regex here makes sense.
> however, i'd imagine it's a little odd for users that we are stricter
> here than we are with DNs.
thanks for your input (didn't see your reply before sending mine)
>
>> src/PVE/JSONSchema.pm | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>> index 49e0d7a..ef58b62 100644
>> --- a/src/PVE/JSONSchema.pm
>> +++ b/src/PVE/JSONSchema.pm
>> @@ -408,7 +408,7 @@ PVE::JSONSchema::register_format('ldap-simple-attr',
>> \&verify_ldap_simple_attr);
>> sub verify_ldap_simple_attr {
>> my ($attr, $noerr) = @_;
>>
>> - if ($attr =~ m/^[a-zA-Z0-9]+$/) {
>> + if ($attr =~ m/^[a-zA-Z0-9\-]+$/) {
>
> if i'm not mistaken, this regex should try to filter an `AttributeValue`
> [1]. in case we do stick with this regex approach here, you may want to
> relax this even further, as per the standard:
>
>> If that UTF-8-encoded Unicode string does not have any of the
>> following characters that need escaping, then that string can be used
>> as the string representation
>> of the value.
>>
>> - a space (' ' U+0020) or number sign ('#' U+0023) occurring at
>> the beginning of the string;
>>
>> - a space (' ' U+0020) character occurring at the end of the
>> string;
>>
>> - one of the characters '"', '+', ',', ';', '<', '>', or '\'
>> (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C,
>> respectively);
>>
>> - the null (U+0000) character.
>>
Ack, so I was wrong, the format might still make sense albeit checking
for above cases would then indeed better, something along the lines of:
if ($attr !~ /(?:^(?:\s|#))|["+,;<>\0\\]|(?:\s$)/) {
return $attr;
}
If we leave that regex out completely we should ensure that we don't get
any tainting issues.
The format could move to PVE::Auth::LDAP too, FWIW, but that's a different
story.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
