Re: DNS SRV support for LDAP authentication

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: DNS SRV support for LDAP authentication
Date: 2019-02-01 11:48:15
Message-ID: 07897B8D-2667-490D-86C6-D46CB215224B@yesql.se
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> On 25 Sep 2018, at 04:09, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:

> Some people like to use DNS SRV records to advertise LDAP servers on
> their network. Microsoft Active Directory is usually (always?) set up
> that way. Here is a patch to allow our LDAP auth module to support
> that kind of discovery. It copies the convention of the OpenLDAP
> command line tools: if you give it a URL that has no hostname, it'll
> try to extract a domain name from the bind DN, and then ask your DNS
> server for a SRV record for LDAP-over-TCP at that domain. The
> OpenLDAP version of libldap.so exports the magic to do that, so the
> patch is very small (but the infrastructure set-up to test it is a bit
> of a schlep, see below). I'll add this to the next Commitfest.

Sounds like a reasonable feature.

> Testing instructions for (paths and commands given for FreeBSD, adjust
> as appropriate):

Trying this quickly on macOS while at a conference didn’t yield much success,
will do another attempt when I’m on a more reliable connection.

> This is a first draft. Not tested much yet. I wonder if
> HAVE_LDAP_INITIALIZE is a reasonable way to detact OpenLDAP. The
> documentation was written in about 7 seconds so probably needs work.
> There is probably a Windowsy way to do this too but I didn't look into
> that.

Reading through the patch, and related OpenLDAP code, this seems like a good
approach. A few small comments:

+ If <productname>PostgreSQL</productname> was compiled with OpenLDAP as

Should OpenLDAP be wrapped in <productname> tags as well? If so, there is
another “bare” instance in client-auth.sgml which perhaps can be wrapped into
this patch while at it.

+ ereport(LOG,
+ (errmsg("could not look up a hostlist for %s",
+ domain)));

Should this be \”%s\”?

+ new_uris = psprintf("%s%s%s://%s:%d",

While this construction isn't introduced in this patch, would it not make sense
to convert uris to StringInfo instead to improve readability?

+ /* Step over this hostname and any spaces. */

Nitpicking on a moved hunk, but single-line comments shouldn’t end in a period
I believe.

cheers ./daniel

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2019-02-01 11:52:49 Re: [HACKERS] [PATCH] Generic type subscripting
Previous Message Alvaro Herrera 2019-02-01 11:45:53 Re: [HACKERS] [PATCH] Generic type subscripting