Re: More flexible LDAP auth search filters?

On Sat, Sep 9, 2017 at 3:33 AM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> A couple of comments on this patch. I have attached a "fixup" patch on
> top of your v4 that should address them.
>
> - I think the bracketing of the LDAP URL synopsis is wrong.

+1

> - I have dropped the sentence that LDAP URL extensions are not
> supported. That sentence was written mainly to point out that filters
> are not supported, which they are now. There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.

+1

> - We have previously used the ldapsearchattribute a bit strangely. We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)). That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute. I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.

Ah. Good.

> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
>
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
>
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot. But there it's a general configuration file syntax,
> nothing specific to LDAP. In nginx you use %(username), which again is
> general configuration file syntax. I'm OK with using %u.
>
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
> They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong. The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
>
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
>
> Neither of these allows substituting the user name into the search filter.
>
> I think there would be a case to be made to allow the searchattribute
> and the searchfilter both be specified. One typical use would be to use
> the first one to locate the user and the second one to "filter" out
> certain things, which I think is the approach the PAM and Apache modules
> take. This wouldn't work well, however, if you use the %u placeholder,
> because then you'd have to explicitly unset ldapsearchattribute, which
> would be annoying. So maybe not.

I like this way, because it doesn't leave the user wondering how the
filter is constructed. It's either the user's filter using %u
placeholders or a system-built one, but not a combination where you
have to wonder whether it's an implicit AND, OR or something else...

> Please check my patch. I think it's ready to go then.

+1 from me to all your changes except this one:

- buffer_size += user_name_size;
+ buffer_size += user_name_size - 2;

The algorithm is: start with buffer_size = 0; add user_name_size if
you see "%u" but otherwise just add one per character; finally add one
for the terminator. There is no reason to subtract 2, since we didn't
count the "%u" characters. Consider a username of "x" and a search
filter of "%u": your change would underflow buffer_size.

Here's a patch with your fixup squashed (except for the above-noted thing).

Thanks for all your work on this.

--
Thomas Munro
http://www.enterprisedb.com