Re: LDAP where DN does not include UID attribute

On Sun, Nov 29, 2009 at 13:05, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> 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?

Here's a patch with my work so far. Major points missing is the
sanitizing of the username and the docs.

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