Обсуждение: [HACKERS] LDAPS

Hi hackers,

I've run into a few requests for $SUBJECT in the field.  I understand
that this is a bit controversial: LDAP + StartTLS (what we already
support) is better than LDAPS because it's a proper standard, and LDAP
auth in general is not as good as some other authentication methods
that we should be encouraging.  I don't actually have a strong view on
whether we should support it myself, but I took a swing at it to see
if there would be any technical obstacles.  I did not find any.  That
said, I've only tested the attached lightly on FreeBSD + OpenLDAP and
don't know if it'll work elsewhere.  Thoughts?

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Nov 4, 2017 at 2:05 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> That
> said, I've only tested the attached lightly on FreeBSD + OpenLDAP and
> don't know if it'll work elsewhere.

Oops, that version's TAP test was a little too dependent on my
system's ldap.conf file.  Here's a version that sets the LDAPCONF env
var to fix that.

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Nov 4, 2017 at 2:05 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> I've only tested the attached lightly on FreeBSD + OpenLDAP and
> don't know if it'll work elsewhere.

While rebasing this on top of a nearby changes, I looked into how
portable it is.  The previous version unconditionally used
ldap_initialize() instead of ldap_init() in order to be able to pass
in ldap or ldaps.  According to the man pages on my system:

       At this time, ldap_open() and ldap_init() are deprecated in favor of
       ldap_initialize(), essentially because the latter allows to specify a
       schema in the URI and it explicitly returns an error code.

But:

1.  It looks like ldap_initialize() arrived in OpenLDAP 2.4 (2007),
which means that it won't work with RHEL5's OpenLDAP 2.3.  That's a
vintage still found in the build farm.  This new version of the patch
has a configure test so it can fall back to ldap_init(), dropping
ldaps support.  That is possibly also necessary for other
implementations.

2.  Windows doesn't have ldap_initialize(), but it has
ldap_sslinit()[1] which adds an SSL boolean argument.  I've included
(but not tested) code for that.  I would need a Windows + LDAP savvy
person to help test that.  I'm not sure if it should also do an
LDAP_OPT_SSL check to see if the server forced the connection back to
plaintext as shown in the Microsoft docs[2], or if that should be
considered OK, or it should be an option.

BTW, Stephen Layland posted a patch for ldaps years ago[3].  It must
have worked some other way though, because he mentions RHEL 4 and
OpenLDAP 2.2/2.3.  Unfortunately the patch wasn't attached and the
referenced webserver has disappeared from the intertubes.

I've added this to the January Commitfest.

[1] https://msdn.microsoft.com/en-us/library/aa366996(v=vs.85).aspx
[2] https://msdn.microsoft.com/en-us/library/aa366105(v=vs.85).aspx
[3] https://www.postgresql.org/message-id/20080426010240.GS5734@68k.org

-- 
Thomas Munro
http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

This patch looks reasonable to me.  I have also seen occasional requests
for this in the field.

If someone could test this on Windows, I think we could move ahead with it.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/26/17 15:53, Peter Eisentraut wrote:
> This patch looks reasonable to me.  I have also seen occasional requests
> for this in the field.
> 
> If someone could test this on Windows, I think we could move ahead with it.

A small point on the test changes.  You change the test under
"diagnostic message", but I'm not sure why.  Do the changes invalidate
the existing test?

We should probably also add another "note" call to introduce the LDAPS
tests section.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 3, 2018 at 5:31 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 12/26/17 15:53, Peter Eisentraut wrote:
>> This patch looks reasonable to me.  I have also seen occasional requests
>> for this in the field.
>>
>> If someone could test this on Windows, I think we could move ahead with it.

Thanks for looking at this.

> A small point on the test changes.  You change the test under
> "diagnostic message", but I'm not sure why.  Do the changes invalidate
> the existing test?

Yeah.  In master, I was relying on the server rejecting ldaptls=1
requests due to lack of configured certificate in order to generate a
diagnostic message.  Now that there is a certificate, I needed to find
another way to get requests rejected with a diagnostic message.  I
have added a brief note to the commit message about this.

> We should probably also add another "note" call to introduce the LDAPS
> tests section.

I realised that I should probably also include a new test for
ldaptls=1, so that we can see that both ways of doing TLS are working.
I added that test, and added a "note" to label the whole section as
"TLS".  Please see attached.

-- 
Thomas Munro
http://www.enterprisedb.com

On 1/2/18 14:56, Thomas Munro wrote:
>> A small point on the test changes.  You change the test under
>> "diagnostic message", but I'm not sure why.  Do the changes invalidate
>> the existing test?
> 
> Yeah.  In master, I was relying on the server rejecting ldaptls=1
> requests due to lack of configured certificate in order to generate a
> diagnostic message.  Now that there is a certificate, I needed to find
> another way to get requests rejected with a diagnostic message.  I
> have added a brief note to the commit message about this.
> 
>> We should probably also add another "note" call to introduce the LDAPS
>> tests section.
> 
> I realised that I should probably also include a new test for
> ldaptls=1, so that we can see that both ways of doing TLS are working.
> I added that test, and added a "note" to label the whole section as
> "TLS".  Please see attached.

Committed.

I added a test case for combining LDAPS with StartTLS.  The OpenLDAP
library sensibly rejects that, so we don't need to do anything ourselves
to prevent that.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services