Re: LDAP where DN does not include UID attribute

On Fri, Sep 18, 2009 at 02:24, Robert Fleming <fleminra(at)gmail(dot)com> wrote:
> On Thu, Sep 17, 2009 at 11:15 AM, Magnus Hagander <magnus(at)hagander(dot)net>
> wrote:
>>
>> On Thu, Sep 17, 2009 at 18:02, Robert Fleming <fleminra(at)gmail(dot)com> wrote:
>> > Following a discussion on the pgsql-admin list
>> > <http://archives.postgresql.org/pgsql-admin/2009-09/msg00075.php>, I
>> > have
>> > created a patch to (optionally) allow PostgreSQL to do a LDAP search to
>> > determine the user's DN (as is done in Apache, MediaWiki, Bugzilla, et
>> > al.)
>> > instead of building the DN from a prefix and suffix.
>> > This is necessary for schemas where the login attribute is not in the
>> > DN,
>> > such as is described here
>> > <http://www.ldapman.org/articles/intro_to_ldap.html#individual> (look
>> > for
>> > "name-based").  This patch is against PostgreSQL 8.4.0 from Debian
>> > Lenny-backports.  If this would be a welcome addition, I can port it
>> > forward
>> > to the latest from postgresql.org.
>> > Thanks in advance for your feedback.
>>
>> This sounds like a very useful feature, and one that I can then remove
>> from my personal TODO list without having to do much work :-)
>>
>> A couple of comments:
>>
>> First of all, please read up on the PostgreSQL coding style, or at
>> least look at the code around yours. This doesn't look anything like
>> our standards.
>
> Sorry, the 8.1 manual said all contributions go through pgindent so I didn't
> spend too much time on that.  I see that the 8.4 manual clarifies that
> pgindent won't get run till release time.  In any case, I've re-formatted
> the patch using the Emacs settings from the 8.1 manual (why are they gone
> from the 8.4 manual)?  It seems like most of the rest of the Coding
> Conventions have to do with error reporting.  Do please let me know if
> there's something else I can do.
>>
>> Second, this appears to require an anonymous bind to the directory,
>> which is something we should not encourage people to enable on their
>> LDAP servers. I think we need to also take parameters with a DN and a
>> password to bind with in order to do the search, and then re-bind as
>> the user when found.
>
> The new patch implements the initial bind using new configuration parameters
> "ldapbinddn" and "ldapbindpasswd".  I've also added a "ldapsearchattribute"
> just to be complete.

I've finally had the time to look at this patch some more, for the
current commitfest - sorry about the delay.

Other than minor style changes (make the indentation be the same as
the code around it), I noticed one fairly large issue.

The way that LDAP is re-initialized both breaks Windows (in the error
reporting part) and not as obvious but even more importantly, it
breaks TLS. If you enable TLS for LDAP it will only be used for the
search, not for the actual password check - which really removes the
point of it :-)

I assume we need the second ldap_init() because we want to do a new
ldap_simple_bind()? In that case, we realliy need to re-initialize the
whole connection, including TLS.

I also notice that it's hardcoded to retrieve the "uid" attribute,
while the one being searched for can be configured. ISTM it's better
to set both of these to the hba->ldapsearchattribute value - so we
won't fail if "uid" does not exist.

Also, as I'm sure you're aware there are no documentation updates included.

I'll be happy to work on this to get it ready for commit, or do you
want to do the updates?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/