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 | Resend email |
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
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 |