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

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

Comment inline.

On Wed, Mar 15, 2023 at 12:41:39PM +0100, Dominik Csapak wrote:
> On 3/15/23 12:17, Christoph Heiss wrote:
> > Thanks for the review!
> >
> > [..]
> > > >
> > > > 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.
>
>
> ok i mistook the 'reserved' characters as reserved by us, not ldap.
> in such a case, when there is an external format/etc. please
> include a reference on where to find these restrictions
> (e.g. a link to an rfc)
For context; see RFC 2253 [0], section 4. Interestingly, this document
was obsoleted by RFC 4514 [1] in 2006, which only mentions this in
section 2.4 ("Converting an AttributeValue from ASN.1 to a String") and
and appendix A ("Presentation Issues").

But the first one seems to be the "authoritive" document on this matter,
at least looking at some other docs about LDAP DNs (RedHat, Microsoft, ..).

I will include that too in the commit next time.

[0] https://www.ietf.org/rfc/rfc2253.txt
[1] https://docs.ldap.com/specs/rfc4514.txt

>
> if my example and all that could have been configured but
> would now be invalid are not valid ldap syntax anyway, i think
> we can get more strict and "break" someones config
> (as you said, shouldn't have worked anyway)
> or how do you see that @thomas?
>
> (maybe there are some weirdly configured ldap instances out there?)
>
> [..]
>
>