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

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

Thanks for the review!

On Wed, Mar 15, 2023 at 10:54:38AM +0100, Dominik Csapak wrote:
> 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 ;)
Good point, I'll do that if/when I spin a v2 and for further patchsets!
I will also include the main points from below, to really make things clear.

>
> also i'm missing a bit the rationale for how the regex was chosen, besides
> that it works in some conditions
Ack, I should have elaborated on that in the commit.

Basically, I took the current regex and what characters are allowed in
attribute values (see patch #2). From that, constructing the character
class for the not-allowed characters (and conversely, the quoted version
of that to allow such special characters) and further the whole regex
was rather simple. The latter was based on the previous one.

So although it looks a bit like a mess, it's a rather simple regex if
you look at it this way.

>
> further comment inline
>
> On 1/31/23 13:50, Christoph Heiss wrote:
> > Signed-off-by: Christoph Heiss <c.heiss@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
Thing is, that would have not worked previously anyway. "Worked" in that
sense that any sensible LDAP server would have failed to parse or
outright rejected such DNs anyway, but could be configured using the
API/UI.

Picking up on your example, "<" and ">" are both not allowed (at least
unquoted) in DN attribute values - see the docs patch again. But using
them properly quoted (e.g. foo="<",bar=">") worked before as does it
with the patch.

I just tested this exact example (using an unpatched PVE) against a
(somewhat current, v2.5.13 as available in bullseye-backports) slapd
server for the sake of it - it fails when performing the search with
"invalid DN" - as expected.

> 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)
Following up on the above, I'd hope no user has such configuration. And
if so, that user has to be using a completely bonkers LDAP
server/implementation.

In conclusion, I don't see how this could break existing setups. But I
do see your point - breaking someones existing setup is a no-go. In that
case I would just hold onto this patchset for the next major release.

>
> > +
> >   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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
>
>