Re: [pve-devel] [PATCH common] fix #5034 ldap attribute regex

[Christoph Heiss <c.heiss@proxmox.com>]

On Wed, Nov 15, 2023 at 02:30:06PM +0100, Thomas Lamprecht wrote:
>
> Am 15/11/2023 um 13:23 schrieb Markus Frank:
> > Change regex from "m/^[a-zA-Z0-9]+$/" to "m/^[a-zA-Z0-9\-]+$/"
> > to allow hyphen in ldap attribute names for pve & pmg.
> > [..]
> > --- 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\-]+$/) {
>
> Pre-existing, but shouldn't the regex actually be?
>
> $attr =~ m/^[a-zA-Z][a-zA-Z0-9\-]*$/
>
> I.e., start with a letter and then be any of letter, digit or hyphen (minus).
>
> CCing Christoph, you did a bit more LDAP stuff recently - opinions?

Sorry for the late reply, just saw this now.

I'd definitely agree with Stefan here, that moving away from regex's for
validating LDAP DNs/attributes/etc is the right way, instead of
continuously having to fix them up.
Even if we try to follow the RFCs as closely as possible, that does
unfortunaly still not really guarantee that it is indeed valid and will
be _accepted by the server_.

Just doing a basic sanity check (e.g. not empty, no spaces) and then
querying the actual LDAP server whether that accepts it or not would be
IMHO preferable. Plus, it's less work on our side in the long run.

PBS does it no differently too, so I'd go the same way here too. Being
overly strict does not help anyone, especially with LDAP, which does a
lot of weird things.