Обсуждение: [HACKERS] More flexible LDAP auth search filters?

Hi hackers,

A customer asked how to use pg_hba.conf LDAP search+bind
authentication to restrict logins to users in one of a small number of
groups.  ldapsearchattribute only lets you make filters like
"(foo=username)", so it couldn't be done.  Is there any reason we
should allow a more general kind of search filter constructions?

A post on planet.postgresql.org today reminded me that a colleague had
asked me to post this POC patch here for discussion.  It allows custom
filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
might be to take a filter pattern with "%USERNAME%" or whatever in it.
There's an existing precedent for the prefix and suffix approach, but
on the other hand a pattern approach would allow filters where the
username is inserted more than once.

Motivating example:

  ldapsearchprefix="(&(cn="
  ldapsearchsuffix = ")(|(memberof=cn=Paris DBA
Team)(memberof=cn=Tokyo DBA Team))"

Note that with this patch ldapsearchattribute=cn is equivalent to:

  ldasearchprefix="(cn="
  ldapsearchsuffix=")"

Perhaps there are better ways to organise your LDAP servers so that
this sort of thing isn't necessary.  I don't know.  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 Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> A post on planet.postgresql.org today reminded me that a colleague had
>> asked me to post this POC patch here for discussion.  It allows custom
>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
>> There's an existing precedent for the prefix and suffix approach, but
>> on the other hand a pattern approach would allow filters where the
>> username is inserted more than once.
>
>
> Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
> you could have something like:
>
> ldapsearchattribute="uid"
> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"
>
> We could then always to substitution of the kind:
> (&(attr=<uid>)(<filter>))
>
> which would in this case give:
> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
>
>
> Basically we'd always AND together the username lookup with the additional
> filter.

Ok, so we have 3 ideas put forward:

1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
filter (as implemented by POC patch)
2.  Optionally AND ldapsearchfilter with the existing
ldapsearchattribute-based filter (Magnus's proposal)
3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
username (my other suggestion)

The main argument for approach 1 is that it follows the style of the
bind-only mode.

With idea 2, I wonder if there are some more general kinds of things
that people might want to do that that wouldn't be possible because it
has to include (attribute=user)... perhaps something involving a
substring or other transformation functions (but I'm no LDAP expert,
that may not make sense).

With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
don't know if any such multiple-mention filters would ever make sense
in a sane LDAP configuration.

Any other views from LDAP-users?

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

On 16/07/17 00:08, Thomas Munro wrote:

> On Fri, Jul 14, 2017 at 11:04 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Thu, Jul 13, 2017 at 9:31 AM, Thomas Munro
>> <thomas.munro@enterprisedb.com> wrote:
>>> A post on planet.postgresql.org today reminded me that a colleague had
>>> asked me to post this POC patch here for discussion.  It allows custom
>>> filters with ldapsearchprefix and ldapsearchsuffix.  Another approach
>>> might be to take a filter pattern with "%USERNAME%" or whatever in it.
>>> There's an existing precedent for the prefix and suffix approach, but
>>> on the other hand a pattern approach would allow filters where the
>>> username is inserted more than once.
>>
>>
>> Do we even need prefix/suffix? If we just make it "ldapsearchpattern", then
>> you could have something like:
>>
>> ldapsearchattribute="uid"
>> ldapsearchfilter="|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)"
>>
>> We could then always to substitution of the kind:
>> (&(attr=<uid>)(<filter>))
>>
>> which would in this case give:
>> (&(uid=mha)(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team)))
>>
>>
>> Basically we'd always AND together the username lookup with the additional
>> filter.
> 
> Ok, so we have 3 ideas put forward:
> 
> 1.  Wrap username with ldapsearchprefix ldapsearchsuffix to build
> filter (as implemented by POC patch)
> 2.  Optionally AND ldapsearchfilter with the existing
> ldapsearchattribute-based filter (Magnus's proposal)
> 3.  Pattern-based ldapsearchfilter so that %USER% is replaced with
> username (my other suggestion)
> 
> The main argument for approach 1 is that it follows the style of the
> bind-only mode.
> 
> With idea 2, I wonder if there are some more general kinds of things
> that people might want to do that that wouldn't be possible because it
> has to include (attribute=user)... perhaps something involving a
> substring or other transformation functions (but I'm no LDAP expert,
> that may not make sense).
> 
> With idea 3 it would allow "(|(foo=%USER%)(bar=%USER%))", though I
> don't know if any such multiple-mention filters would ever make sense
> in a sane LDAP configuration.
> 
> Any other views from LDAP-users?

I've spent quite a bit of time integrating various bits of
non-PostgreSQL software to LDAP and in my experience option 3 tends to
be the standard.

Generally you find that you will be given the option to set the
attribute for the default search filter of the form
"(attribute=username)" which defaults to uid for UNIX-type systems and
sAMAccountName for AD. However there is always the ability to specify a
custom filter where the user is substituted via e.g. %u to cover all the
other use-cases.

As an example, I don't know if anyone would actually do this with
PostgreSQL but I've been asked on multiple occasions to configure
software so that users should be allowed to log in with either their
email address or username which is easily done with a custom LDAP filter
like "(|(mail=%u)(uid=%u))".


ATB,

Mark.

Magnus, all,

* Magnus Hagander (magnus@hagander.net) wrote:
> (FWIW, a workaround I've applied more than once to this in AD environments
> (where kerberos for one reason or other can't be done, sorry Stephen) is to
> set up a RADIUS server and use that one as a "middle man". But it would be
> much better if we could do it natively)

I'd suggest that we try to understand why Kerberos couldn't be used in
that environment.  I suspect in at least some cases what users would
like is the ability to do Kerberos auth but then have LDAP checked to
see if a given user (who has now auth'd through Kerberos) is allowed to
connect.  We don't currently have any way to do that, but if we were
looking for things to do, that's what I'd suggest working on rather than
adding more to our LDAP auth system and implying by doing so that it's
reasonable to use.

I find it particularly disappointing to see recommendations for using
LDAP auth, particularly in AD environments, that don't even mention
Kerberos or bother to explain how using LDAP sends the user's PW to the
server in cleartext.

Thanks!

Stephen

On Mon, Jul 17, 2017 at 5:58 AM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>> Any other views from LDAP-users?
>
> I've spent quite a bit of time integrating various bits of
> non-PostgreSQL software to LDAP and in my experience option 3 tends to
> be the standard.
>
> Generally you find that you will be given the option to set the
> attribute for the default search filter of the form
> "(attribute=username)" which defaults to uid for UNIX-type systems and
> sAMAccountName for AD. However there is always the ability to specify a
> custom filter where the user is substituted via e.g. %u to cover all the
> other use-cases.

Cool.  Here is a new version of the patch updated to do it exactly
like that.  I tested it against OpenLDAP.

> As an example, I don't know if anyone would actually do this with
> PostgreSQL but I've been asked on multiple occasions to configure
> software so that users should be allowed to log in with either their
> email address or username which is easily done with a custom LDAP filter
> like "(|(mail=%u)(uid=%u))".

Thank you very much for this feedback and example, which I used in the
documentation in the patch.  I see similar examples in the
documentation for other things on the web.

I'll leave it up to Magnus and Stephen to duke it out over whether we
want to encourage LDAP usage, extend documentation to warn about
cleartext passwords with certain LDAP implementations or
configurations, etc etc.  I'll add this patch to the commitfest and
get some popcorn.

-- 
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 16/07/17 23:26, Thomas Munro wrote:

> Thank you very much for this feedback and example, which I used in the
> documentation in the patch.  I see similar examples in the
> documentation for other things on the web.
> 
> I'll leave it up to Magnus and Stephen to duke it out over whether we
> want to encourage LDAP usage, extend documentation to warn about
> cleartext passwords with certain LDAP implementations or
> configurations, etc etc.  I'll add this patch to the commitfest and
> get some popcorn.

If it helps, we normally recommend that clients use ldaps for both AD
and UNIX environments, although this can be trickier from an
administrative perspective in AD environments because it can require
changes to the Windows firewall and certificate installation.

Whilst OpenLDAP will support ldap+starttls you can end up with some
clients with starttls either disabled or misconfigured sending plaintext
passwords over the wire regardless, so it's generally easiest to
firewall ldap port 389 at the edge of the trusted VLAN so that only
ldaps port 636 connections make it out onto the untrusted network
hosting the local AD/OpenLDAP server.


ATB,

Mark.

On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> ldap-search-filters-v2.patch

Gah, it would help if I could spell "occurrences" correctly.  Fixed in
the attached.

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

Mark,

* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote:
> On 16/07/17 23:26, Thomas Munro wrote:
> > Thank you very much for this feedback and example, which I used in the
> > documentation in the patch.  I see similar examples in the
> > documentation for other things on the web.
> >
> > I'll leave it up to Magnus and Stephen to duke it out over whether we
> > want to encourage LDAP usage, extend documentation to warn about
> > cleartext passwords with certain LDAP implementations or
> > configurations, etc etc.  I'll add this patch to the commitfest and
> > get some popcorn.
>
> If it helps, we normally recommend that clients use ldaps for both AD
> and UNIX environments, although this can be trickier from an
> administrative perspective in AD environments because it can require
> changes to the Windows firewall and certificate installation.

LDAPS is better than straight LDAP, of course, but it still doesn't
address the issue that the password is sent to the server, which both
SCRAM and Kerberos do and is why AD environments use Kerberos for
authentication, and why everything in an AD environment also should use
Kerberos.

Using Kerberos should also avoid the need to hack the Windows firewall
or deal with certificate installation.  In an AD environment, it's
actually pretty straight-forward to add a PG server too.  Further, in my
experience at least, there's been other changes recommended by Microsoft
that prevent using LDAP for auth because it's insecure.

Thanks!

Stephen

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:
> On Sun, Jul 16, 2017 at 11:05 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > I'd suggest that we try to understand why Kerberos couldn't be used in
> > that environment.  I suspect in at least some cases what users would
> > like is the ability to do Kerberos auth but then have LDAP checked to
> > see if a given user (who has now auth'd through Kerberos) is allowed to
> > connect.  We don't currently have any way to do that, but if we were
> > looking for things to do, that's what I'd suggest working on rather than
> > adding more to our LDAP auth system and implying by doing so that it's
> > reasonable to use.
> >
> > I find it particularly disappointing to see recommendations for using
> > LDAP auth, particularly in AD environments, that don't even mention
> > Kerberos or bother to explain how using LDAP sends the user's PW to the
> > server in cleartext.
>
> You do realize, I'm sure, that there are many LDAP servers out there that
> are not AD, and that do not come with a Kerberos server attached to them...

I am aware that some exist, I've even contributed to their development
and packaging, but that doesn't make it a good idea to use them for
authentication.

> I agree that Kerberos is usually the better choice *if it's available*.

Which is the case in an AD environment..

> It's several orders of magnitude more complicated to set up though, and
> there are many environments that have ldap but don't have Kerberos.

Frankly, I simply don't agree with this.

> Refusing to improve LDAP for the users who have no choice seems like a very
> unfriendly thing to do.

I'm fine with improving LDAP in general, but, as I tried to point out,
having a way to make it easier to integrate PG into an AD environment
would be better.  It's not my intent to stop this patch but rather to
point out the issues with LDAP auth that far too frequently are not
properly understood.

> (And you can actually reasonably solve the case of
> kerberos-for-auth-ldap-for-priv by syncing the groups into postgres roles)

Yes, but sync'ing roles is a bit of a pain and it'd be nice if we could
avoid it, or perhaps make it easier.

Thanks!

Stephen

On 17/07/17 00:14, Stephen Frost wrote:

>> If it helps, we normally recommend that clients use ldaps for both AD
>> and UNIX environments, although this can be trickier from an
>> administrative perspective in AD environments because it can require
>> changes to the Windows firewall and certificate installation.
> 
> LDAPS is better than straight LDAP, of course, but it still doesn't
> address the issue that the password is sent to the server, which both
> SCRAM and Kerberos do and is why AD environments use Kerberos for
> authentication, and why everything in an AD environment also should use
> Kerberos.
> 
> Using Kerberos should also avoid the need to hack the Windows firewall
> or deal with certificate installation.  In an AD environment, it's
> actually pretty straight-forward to add a PG server too.  Further, in my
> experience at least, there's been other changes recommended by Microsoft
> that prevent using LDAP for auth because it's insecure.

Oh sure - I'm not questioning that Kerberos is a far better choice in
pure AD environments, it's just that I spend the majority of my time in
mixed-mode environments where Windows is very much in the minority.

In my experience LDAP is often implemented badly; for example the
majority of software still uses simple binds (i.e. plain logins) rather
than using SASL binds which support a whole range of better
authentication methods (e.g. GSSAPI, and even DIGEST-MD5 has been
mandatory for v3 and is implemented on AD).

And yes, while better authentication mechanisms do exist, I find that
all too often most software packages claim LDAP support rather than
Kerberos, and even then it is often limited to LDAP simple binds without
ldaps support.


ATB,

Mark.

On 17/07/17 13:09, Magnus Hagander wrote:

Hi Magnus,

Great to hear from you! It has definitely been a while...

>     Generally you find that you will be given the option to set the
>     attribute for the default search filter of the form
>     "(attribute=username)" which defaults to uid for UNIX-type systems and
>     sAMAccountName for AD. However there is always the ability to specify a
>     custom filter where the user is substituted via e.g. %u to cover all the
>     other use-cases.
> 
> Right, but that's something we do already today. It just defaults to
> uid, but it's easy to change. 

Yes, I think that bit is fine as long as the default can be overridden.
There's always a choice as to whether the defaults favour a POSIX-based
LDAP or AD environment but that happens with all installations so it's
not a big deal.

>     As an example, I don't know if anyone would actually do this with
>     PostgreSQL but I've been asked on multiple occasions to configure
>     software so that users should be allowed to log in with either their
>     email address or username which is easily done with a custom LDAP filter
>     like "(|(mail=%u)(uid=%u))".
> 
> 
> How would that actually work, though? Given the same user in ldap could
> now potentially match multiple different users in PostgreSQL. Would you
> then create two accounts for the user, one with the uid as name and one
> with email as name? Wouldn't that actually cause more issues than it solves?

Normally what happens for search+bind is that you execute the custom
filter with substitutions in order to find the entry for your bind DN,
but you also request the value of ldapsearchattribute (or equivalent) at
the same time. Say for example you had an entry like this:

dn: uid=mha,dc=users,dc=hagander,dc=net
uid: mha
mail: magnus@hagander.net

Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
"uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
email address.

If the bind is successful then the current user identity should be set
to the value of the ldapsearchattribute retrieved from the bind DN
entry, which with the default of "uid" would be "mha". Hence you end up
with the same user role "mha" regardless of whether a uid or email
address was entered.

In terms of matching multiple users, all LDAP authentication routines
I've seen will fail if more than one DN matches the search filter, so
this nicely handles the cases where either the custom filter is
incorrect or more than one entry is accidentally matched in the directory.


ATB,

Mark.

On 17/07/17 18:08, Magnus Hagander wrote:

> On Mon, Jul 17, 2017 at 6:47 PM, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk <mailto:mark.cave-ayland@ilande.co.uk>>
> wrote: 
>     Great to hear from you! It has definitely been a while...
> 
> Indeed. You should spend more time on these lists :P

Well I do get the emails, unfortunately it's trying to find the time to
read them all ;)

>     > How would that actually work, though? Given the same user in ldap could
>     > now potentially match multiple different users in PostgreSQL. Would you
>     > then create two accounts for the user, one with the uid as name and one
>     > with email as name? Wouldn't that actually cause more issues than it solves?
> 
>     Normally what happens for search+bind is that you execute the custom
>     filter with substitutions in order to find the entry for your bind DN,
>     but you also request the value of ldapsearchattribute (or equivalent) at
>     the same time. Say for example you had an entry like this:
> 
>     dn: uid=mha,dc=users,dc=hagander,dc=net
>     uid: mha
>     mail: magnus@hagander.net <mailto:magnus@hagander.net>
> 
>     Using the filter "(|(mail=%u)(uid=%u))" would locate the same bind DN
>     "uid=mha,dc=users,dc=hagander,dc=net" with either one of your uid or
>     email address.
> 
>     If the bind is successful then the current user identity should be set
>     to the value of the ldapsearchattribute retrieved from the bind DN
>     entry, which with the default of "uid" would be "mha". Hence you end up
>     with the same user role "mha" regardless of whether a uid or email
>     address was entered.
> 
> 
> Right. This is the part that doesn't work for PostgreSQL. Because we
> have already specified the username -- it goes in the startup packet in
> order to pick the correct row from pg_hba.conf.

I don't think that's necessarily going to be an issue here because if
you're specifying a custom LDAP filter then there's a very good chance
that you're delegating access control to information held in the
directory anyway, e.g.
 (&(memberOf=cn=pgusers,dc=groups,dc=hagander,dc=net)(uid=%u))
 ((&(uid=%u)(|(uid=mha)(uid=mark)(uid=thomas)))

In the mail example above when you're using more than one attribute, I
think it's fair enough to simply say in the documentation you must set
user to "all" in pg_hba.conf since it is impossible to know which
attribute is being used to identify the user role until after
authentication.

I should add that personally I don't recommend such setups where the
user can log in using more than one identifier, but there are clients
out there who absolutely will insist on it (think internal vs. external
users) so if the LDAP support is being updated then it's worth exploring
to see if these cases can be supported.

> I guess in theory we could treat it like Kerberos or another one of the
> systems where we get the username from an external entity. But then
> you'd still have to specify the mapping again in pg_ident.conf, and it
> would be a pretty strong break of backwards compatibility.

(goes and glances at the code)

Is there no reason why you couldn't just overwrite port->user_name based
upon ldapsearchattribute at the end of CheckLDAPAuth?

And if this were only enabled when a custom filter were specified then
it shouldn't cause any backwards compatibility issues being a new feature?

>     In terms of matching multiple users, all LDAP authentication routines
>     I've seen will fail if more than one DN matches the search filter, so
>     this nicely handles the cases where either the custom filter is
>     incorrect or more than one entry is accidentally matched in the
>     directory.
> 
> So do we, in the current implementation. But it's a lot less likely to
> happen in the current implementation, since we do a single equals lookup.

Great, that's absolutely fine :)


ATB,

Mark.

On Sun, Jul 16, 2017 at 7:23 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> Refusing to improve LDAP for the users who have no choice seems like a very
>> unfriendly thing to do.
>
> I'm fine with improving LDAP in general, but, as I tried to point out,
> having a way to make it easier to integrate PG into an AD environment
> would be better.  It's not my intent to stop this patch but rather to
> point out the issues with LDAP auth that far too frequently are not
> properly understood.

Then it's off-topic for this thread.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 7/16/17 19:09, Thomas Munro wrote:
> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> ldap-search-filters-v2.patch
> 
> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
> the attached.

Please also add the corresponding support for specifying search filters
in LDAP URLs.  See RFC 4516 for the format and
https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
need to grab LDAPURLDesc.lud_filter and use it.

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

On Wed, Aug 2, 2017 at 5:36 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 7/16/17 19:09, Thomas Munro wrote:
>> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
>> <thomas.munro@enterprisedb.com> wrote:
>>> ldap-search-filters-v2.patch
>>
>> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
>> the attached.
>
> Please also add the corresponding support for specifying search filters
> in LDAP URLs.  See RFC 4516 for the format and
> https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
> need to grab LDAPURLDesc.lud_filter and use it.

Good idea.  Yes, it seems to be that simple.  Here's a version like
that.  Here's an example of how it looks in pg_hba.conf:

host   all             all  127.0.0.1/32        ldap
ldapurl="ldap://localhost/ou=people1,dc=my-domain,dc=com??sub?(cn=%25u)"

Maybe we could choose a better token than %u for user name, since it
has to be escaped when included in a URL like that, but on the other
hand there seems to be wide precedent for %u in other software.

-- 
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 01/08/17 23:17, Thomas Munro wrote:

> On Wed, Aug 2, 2017 at 5:36 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 7/16/17 19:09, Thomas Munro wrote:
>>> On Mon, Jul 17, 2017 at 10:26 AM, Thomas Munro
>>> <thomas.munro@enterprisedb.com> wrote:
>>>> ldap-search-filters-v2.patch
>>>
>>> Gah, it would help if I could spell "occurrences" correctly.  Fixed in
>>> the attached.
>>
>> Please also add the corresponding support for specifying search filters
>> in LDAP URLs.  See RFC 4516 for the format and
>> https://linux.die.net/man/3/ldap_url_parse for the API.  You might just
>> need to grab LDAPURLDesc.lud_filter and use it.
> 
> Good idea.  Yes, it seems to be that simple.  Here's a version like
> that.  Here's an example of how it looks in pg_hba.conf:
> 
> host   all             all  127.0.0.1/32        ldap
> ldapurl="ldap://localhost/ou=people1,dc=my-domain,dc=com??sub?(cn=%25u)"
> 
> Maybe we could choose a better token than %u for user name, since it
> has to be escaped when included in a URL like that, but on the other
> hand there seems to be wide precedent for %u in other software.

Yeah, mostly I only ever see ldapurls used programatically, i.e. the
configuration allows you to set the various fields separately and then
the software generates the URL with the correct encoding itself. But if
it's documented that's not a reason to reject the patch as I definitely
see it as an improvement.

As I mentioned previously in the thread, the main barrier preventing
people from using LDAP is that the role cannot be generated from other
attributes in the directory. In a lot of real-life cases I see, that
would be enough to discount PostgreSQL's LDAP authentication completely.


ATB,

Mark.

A couple of comments on this patch.  I have attached a "fixup" patch on
top of your v4 that should address them.

- I think the bracketing of the LDAP URL synopsis is wrong.

- I have dropped the sentence that LDAP URL extensions are not
supported.  That sentence was written mainly to point out that filters
are not supported, which they are now.  There is nothing beyond filters
and extensions left in LDAP URLs, AFAIK.

- We have previously used the ldapsearchattribute a bit strangely.  We
use it as both the search filter and as the attribute to return from the
search, but we don't actually care about the returned attribute (we only
use the DN (which is not a real attribute)).  That hasn't been a real
problem, but now if you specify a filter, as the code stands it will
always request the "uid" attribute, which won't work if there is no such
attribute.  I have found that there is an official way to request "no
attribute", so I have fixed the code to do that.

- I was wondering whether we need a way to escape the % (%%?) but it
doesn't seem worth bothering.

I have been looking around the Internet how this functionality compares
to other LDAP authentication systems.

I didn't see the origin of the %u idea in this thread, but perhaps it
came from Dovecot.  But there it's a general configuration file syntax,
nothing specific to LDAP.  In nginx you use %(username), which again is
general configuration file syntax.  I'm OK with using %u.

The original LDAP URL design was adapted from Apache
(https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
 They combine the attribute filter and the general filter if both are
specified, but I think they got that a bit wrong.  The attribute field
in the URL is actually not supposed to be a filter but a return field,
which is also the confusion mentioned above.

The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
specify a search attribute and a general filter and combines them.

Neither of these allows substituting the user name into the search filter.

I think there would be a case to be made to allow the searchattribute
and the searchfilter both be specified.  One typical use would be to use
the first one to locate the user and the second one to "filter" out
certain things, which I think is the approach the PAM and Apache modules
take.  This wouldn't work well, however, if you use the %u placeholder,
because then you'd have to explicitly unset ldapsearchattribute, which
would be annoying.  So maybe not.

Please check my patch.  I think it's ready to go then.

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

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

For additional entertainment I have written a test suite for this LDAP
authentication functionality.  It's not quite robust enough to be run by
default, because it needs a full OpenLDAP installation, but it's been
very helpful for reviewing this patch.  Here it is.

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

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

On 08/09/17 16:33, Peter Eisentraut wrote:

> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
> 
> - I think the bracketing of the LDAP URL synopsis is wrong.
> 
> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.
> 
> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.
> 
> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
> 
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
> 
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
> 
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
> 
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
> 
> Neither of these allows substituting the user name into the search filter.

Great work! Having installed quite a few LDAP-based authentication
setups in the past, I can promise you that pam_ldap is the last tool I
would consider for the job so please don't hold to this as being the
gold standard(!).

My weapon of choice for LDAP deployments on POSIX-based systems is
Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
which is far more flexible than pam_ldap and fixes a large number of
bugs, including the tendency for pam_ldap to hang infinitely if it can't
contact its LDAP server.

Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
pam_authz_search - this is exactly the type of filters I would end up
deploying onto servers. This happens a lot in large organisations
whereby getting group memberships updated in the main directory can take
days/weeks whereas someone with root access to the server itself can
hard-code an authentication list of users and/or groups in an LDAP
filter in just a few minutes.


ATB,

Mark.


-- 
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, Sep 9, 2017 at 3:36 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> For additional entertainment I have written a test suite for this LDAP
> authentication functionality.  It's not quite robust enough to be run by
> default, because it needs a full OpenLDAP installation, but it's been
> very helpful for reviewing this patch.  Here it is.

Very nice!

+if ($^O eq 'darwin')
+{
+       $slapd = '/usr/local/opt/openldap/libexec/slapd';
+       $ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}

I'm guessing this is the MacPorts location, and someone from that
other tribe that uses Brew can eventually post a patch to make this
look in more places.

+my $ldap_port = int(rand() * 16384) + 49152;

Hmm.  I guess ldapi (Unix domain sockets) would be less roulette-like,
but require client side support too.

Here's a change I needed to make to run this here.  It seems that to
use "database mdb" I'd need to add a config line to tell it the path
to load back_mdb.so from.  I could have done, but I noticed that if I
tell it to use raw ldif files instead it's happy.  Does this still
work for you on the systems you tested?

-- 
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, Sep 9, 2017 at 3:33 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
>
> - I think the bracketing of the LDAP URL synopsis is wrong.

+1

> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.

+1

> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.

Ah.  Good.

> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
>
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
>
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
>
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
>
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
>
> Neither of these allows substituting the user name into the search filter.
>
> I think there would be a case to be made to allow the searchattribute
> and the searchfilter both be specified.  One typical use would be to use
> the first one to locate the user and the second one to "filter" out
> certain things, which I think is the approach the PAM and Apache modules
> take.  This wouldn't work well, however, if you use the %u placeholder,
> because then you'd have to explicitly unset ldapsearchattribute, which
> would be annoying.  So maybe not.

I like this way, because it doesn't leave the user wondering how the
filter is constructed.  It's either the user's filter using %u
placeholders or a system-built one, but not a combination where you
have to wonder whether it's an implicit AND, OR or something else...

> Please check my patch.  I think it's ready to go then.

+1 from me to all your changes except this one:

- buffer_size += user_name_size;
+ buffer_size += user_name_size - 2;

The algorithm is: start with buffer_size = 0; add user_name_size if
you see "%u" but otherwise just add one per character; finally add one
for the terminator.  There is no reason to subtract 2, since we didn't
count the "%u" characters.  Consider a username of "x" and a search
filter of "%u": your change would underflow buffer_size.

Here's a patch with your fixup squashed (except for the above-noted thing).

Thanks for all your work on this.

-- 
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 9/8/17 13:24, Mark Cave-Ayland wrote:
> My weapon of choice for LDAP deployments on POSIX-based systems is
> Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
> which is far more flexible than pam_ldap and fixes a large number of
> bugs, including the tendency for pam_ldap to hang infinitely if it can't
> contact its LDAP server.
> 
> Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
> pam_authz_search - this is exactly the type of filters I would end up
> deploying onto servers. This happens a lot in large organisations
> whereby getting group memberships updated in the main directory can take
> days/weeks whereas someone with root access to the server itself can
> hard-code an authentication list of users and/or groups in an LDAP
> filter in just a few minutes.

Thomas, would you consider using the placeholder syntax described at
<https://arthurdejong.org/nss-pam-ldapd/nslcd.conf.5> under
pam_authz_search?

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


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

On 9/8/17 21:31, Thomas Munro wrote:
> +if ($^O eq 'darwin')
> +{
> +       $slapd = '/usr/local/opt/openldap/libexec/slapd';
> +       $ldap_schema_dir = '/usr/local/etc/openldap/schema';
> +}
> 
> I'm guessing this is the MacPorts location, and someone from that
> other tribe that uses Brew can eventually post a patch to make this
> look in more places.

Or the other way around :)

> +my $ldap_port = int(rand() * 16384) + 49152;
> 
> Hmm.  I guess ldapi (Unix domain sockets) would be less roulette-like,
> but require client side support too.

Yeah, issue similar to the SSL tests.  The above formula is what
PostgresNode uses.

> Here's a change I needed to make to run this here.  It seems that to
> use "database mdb" I'd need to add a config line to tell it the path
> to load back_mdb.so from.  I could have done, but I noticed that if I
> tell it to use raw ldif files instead it's happy.  Does this still
> work for you on the systems you tested?

Yes, that seems like a better choice.

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


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

On Tue, Sep 12, 2017 at 7:21 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/8/17 13:24, Mark Cave-Ayland wrote:
>> My weapon of choice for LDAP deployments on POSIX-based systems is
>> Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
>> which is far more flexible than pam_ldap and fixes a large number of
>> bugs, including the tendency for pam_ldap to hang infinitely if it can't
>> contact its LDAP server.
>>
>> Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
>> pam_authz_search - this is exactly the type of filters I would end up
>> deploying onto servers. This happens a lot in large organisations
>> whereby getting group memberships updated in the main directory can take
>> days/weeks whereas someone with root access to the server itself can
>> hard-code an authentication list of users and/or groups in an LDAP
>> filter in just a few minutes.
>
> Thomas, would you consider using the placeholder syntax described at
> <https://arthurdejong.org/nss-pam-ldapd/nslcd.conf.5> under
> pam_authz_search?

Sounds good.  Here it is with $username.  It's nice not to have to
escape any characters in URLs.  I suppose more keywords could be added
in follow-up patches if someone thinks that would be useful
($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
changed it to use StringInfo.  Here also are your test patches tweaked
slightly: 0002 just adds FreeBSD support as per previous fixup and
0003 changes to $username.

-- 
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 9/11/17 23:58, Thomas Munro wrote:
> Sounds good.  Here it is with $username.  It's nice not to have to
> escape any characters in URLs.  I suppose more keywords could be added
> in follow-up patches if someone thinks that would be useful
> ($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
> changed it to use StringInfo.  Here also are your test patches tweaked
> slightly: 0002 just adds FreeBSD support as per previous fixup and
> 0003 changes to $username.

Committed the feature patch.

Any further thoughts on the test suite?  Otherwise I'll commit it as we
have it, for manual use.

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


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

On Wed, Sep 13, 2017 at 1:55 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/11/17 23:58, Thomas Munro wrote:
>> Sounds good.  Here it is with $username.  It's nice not to have to
>> escape any characters in URLs.  I suppose more keywords could be added
>> in follow-up patches if someone thinks that would be useful
>> ($hostname, $dbname, ...?).  I got sick of that buffer sizing code and
>> changed it to use StringInfo.  Here also are your test patches tweaked
>> slightly: 0002 just adds FreeBSD support as per previous fixup and
>> 0003 changes to $username.
>
> Committed the feature patch.

Thanks!

> Any further thoughts on the test suite?  Otherwise I'll commit it as we
> have it, for manual use.

I wonder if there is a reasonable way to indicate or determine whether
you have slapd installed so that check-world could run this test...

-- 
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 Wed, Sep 13, 2017 at 8:04 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> I wonder if there is a reasonable way to indicate or determine whether
> you have slapd installed so that check-world could run this test...

Module::Install's requires_external_bin is one:
http://search.cpan.org/~ether/Module-Install-1.18/lib/Module/Install.pod#requires_external_bin
But the bar to add a new module dependency is high.

Another trick that you could use is to attempt to run it, see if it is
present by checking for 127 sounds fragile though, for Windows
particularly, and otherwise skip the test.
-- 
Michael


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

On 9/12/17 19:04, Thomas Munro wrote:
>> Any further thoughts on the test suite?  Otherwise I'll commit it as we
>> have it, for manual use.

done

> I wonder if there is a reasonable way to indicate or determine whether
> you have slapd installed so that check-world could run this test...

The other problem is the open TCP/IP socket, so we can't really run it
by default anyway, like the SSL tests.

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


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