Re: DNS SRV support for LDAP authentication - Mailing list pgsql-hackers

On Sat, Feb 16, 2019 at 10:57 PM Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> Yeah.  This coding is ugly and StringInfo would be much nicer.
> Thinking about that made me realise that the proposed SRV case should
> also handle multiple SRV records by building a multi-URL string too
> (instead of just taking the first one).  I will make it so.

Done, in the attached.  Reviewing your comments again, from the top:

On Sat, Feb 2, 2019 at 12:48 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> +     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.

Fixed.

> +               ereport(LOG,
> +                       (errmsg("could not look up a hostlist for %s",
> +                               domain)));
>
> Should this be \”%s\”?

Yep, fixed.

> +               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?

Agreed, fixed.

> +               /* Step over this hostname and any spaces. */
>
> Nitpicking on a moved hunk, but single-line comments shouldn’t end in a period
> I believe.

Huh. And yet they are sentences.

tmunro@dogmatix $ git grep '/\* [A-Za-z].*\. \*/' | wc -l
    5607
tmunro@dogmatix $ git grep '/\* [A-Za-z].*[a-z] \*/' | wc -l
   59500

Yep, you win!

I also fixed a bug where some error messages could pass a NULL pointer
for %s when we don't have a server name.

I also added a hint to the error message you get if it can't find DNS
SRV records, so that if you accidentally activate this feature by
forgetting to set the server name, it'll remind you that you could do
that:

 LOG:  LDAP authentication could not find DNS SRV records for "example.net"
 HINT:  Set an LDAP server name explicitly.

Unfortunately, no feedback from MS Active Directory users has been
forthcoming, but I guess that might take a beta release.  See below
for new more complete instructions for testing this with an open
source stack (now that I know there is a lazy way to stand up an LDAP
server using the TAP test stuff, I've adjusted the instructions to
work with that).

I'd like to commit this soon.

Some random things I noticed that I am not fixing in this patch but
wanted to mention:  I don't like the asymmetry initStringInfo(si),
pfree(si->data).  I don't like si->data as a way to get a C string
from a StringInfo.  There are a couple of references to StringBuffer
that surely mean StringInfo in comments.

=== How to test ===

1.  Start up an LDAP server that has a user test1/secret1 under
dc=example,dc=net (it runs in the background and you can stop it with
SIGINT):

$ make -C src/test/ldap check
$ /usr/local/libexec/slapd -f src/test/ldap/tmp_check/slapd.conf -h
ldap://127.0.0.1:5555

2.  Start up a BIND daemon that has multiple SRV records for LDAP at
example.com:

$ tail -4 /usr/local/etc/namedb/named.conf
zone "example.net" {
        type master;
        file "/usr/local/etc/namedb/master/example.net";
};
$ cat /usr/local/etc/namedb/master/example.net
$TTL    10
@       IN      SOA     ns.example.net. admin.example.net. (
                              2         ; Serial
                         604800         ; Refresh
                          86400         ; Retry
                        2419200         ; Expire
                         604800 )       ; Negative Cache TTL
        IN      NS      ns.example.net.
ns.example.net. IN A 127.0.0.1
example.net. IN A 127.0.0.1
ldap1.example.net. IN A 127.0.0.1
ldap2.example.net. IN A 127.0.0.1
_ldap._tcp.example.net. IN SRV 0 0 5555 ldap1
_ldap._tcp.example.net. IN SRV 1 0 5555 ldap2

3.  Tell your OS to talk to that DNS server (and, erm, keep what you
had here so you can restore it later):

$ cat /etc/resolv.conf
nameserver 127.0.0.1

4.  Check that standard DNS and LDAP tools can find their way to your
LDAP servers via these breadcrumbs:

$ host -t srv _ldap._tcp.example.net
_ldap._tcp.example.net has SRV record 0 0 5555 ldap1.example.net.
_ldap._tcp.example.net has SRV record 1 0 5555 ldap2.example.net.

$ ldapsearch -H 'ldap:///dc%3Dexample%2Cdc%3Dnet' -b 'dc=example,dc=net'

5.  Tell PostgreSQL to use SRV records in pg_hba.conf using either of
these styles:

host all test1 127.0.0.1/32 ldap basedn="dc=example,dc=net"
host all test1 127.0.0.1/32 ldap ldapurl="ldap:///dc=example,dc=net?uid?sub"

6.  Check that you now log in as test1/secret1:

$ psql -h 127.0.0.1 postgres test1

--
Thomas Munro
https://enterprisedb.com