Обсуждение: [PATCH] Support pg_ident mapping for LDAP

Hello,

There was a brief discussion [1] back in February on allowing user
mapping for LDAP, in order to open up some more complex authorization
logic (and slightly reduce the need for LDAP-to-Postgres user
synchronization). Attached is an implementation of this that separates
the LDAP authentication and authorization identities, and lets the
client control the former with an `ldapuser` connection option or its
associated PGLDAPUSER envvar.

This isn't as useful as, say, authorization based on LDAP attributes or
group membership, but it seems like a necessary step towards that
feature, since we'll need to separate authn and authz anyway. This
provides some feature parity with other auth methods like gss, and it
solves the "let anyone who can authenticate against LDAP connect as X
role" use case trivially.

There is precedent for allowing the DBA to choose whether to map a bare
username or the "full" identity expansion -- see for example
include_realm=1 for gss and clientname=DN for cert -- so I added an
ldap_map_dn=1 option which can be used to map the whole DN. (I'm not
entirely convinced that it's useful, but maybe there are some
deployments that put authorization information into the LDAP topology,
like "everyone in this particular subtree is an admin".) Unlike
include_realm, this is only allowed with an explicit map. I don't
anticipate people using a full DN as a database username, and I'm
worried that doing that without normalization could cause some major
confusion and/or security problems.

When using a newer client with an older server, the new `ldapuser`
option will cause a connection failure. For the case where PGUSER and
PGLDAPUSER are identical, that failure is technically unnecessary, and
I briefly considered stripping the `ldapuser` option from the
connection string in that case so that we could have wider
compatibility. I now think that's a bad idea, because suddenly
introducing a hard connection failure (or new success) just because
your PGLDAPUSER variable changed would be a major support hazard. The
TODO is still in the code to remind me to have the conversation.

WDYT?

--Jacob

[1] https://www.postgresql.org/message-id/94f6b945f9ca8cabe2b9d2a38ec19dca6f90a083.camel%40vmware.com

On Tue, 2021-08-31 at 19:39 +0000, Jacob Champion wrote:
> Hello,
> 
> There was a brief discussion [1] back in February on allowing user
> mapping for LDAP, in order to open up some more complex authorization
> logic (and slightly reduce the need for LDAP-to-Postgres user
> synchronization). Attached is an implementation of this that separates
> the LDAP authentication and authorization identities, and lets the
> client control the former with an `ldapuser` connection option or its
> associated PGLDAPUSER envvar.

The cfbot found a failure in postgres_fdw, which I completely neglected
in my design. I think the desired functionality should be to allow the
ldapuser connection option during CREATE USER MAPPING but not CREATE
SERVER. I'll have a v2 up today to fix that.

--Jacob

On Wed, 2021-09-01 at 15:42 +0000, Jacob Champion wrote:
> The cfbot found a failure in postgres_fdw, which I completely neglected
> in my design. I think the desired functionality should be to allow the
> ldapuser connection option during CREATE USER MAPPING but not CREATE
> SERVER.

Fixed in v2, attached.

--Jacob

On Wed, 2021-09-01 at 12:59 -0700, Zhihong Yu wrote:
> +       if (strcmp(val, "1") == 0)
> +           hbaline->ldap_map_dn = true;
> +       else
> +           hbaline->ldap_map_dn = false;
> 
> The above can be shortened as:
> 
>   hbaline->ldap_map_dn = strcmp(val, "1") == 0;

I usually prefer simplifying those conditionals, too, but in this case
I think it'd be a pretty big departure from the existing style. See for
example the handling of include_realm and compat_realm just after this
hunk.

--Jacob

On Wed, 2021-09-01 at 14:20 -0700, Zhihong Yu wrote:
> I looked at v2-Allow-user-name-mapping-with-LDAP.patch
> and src/backend/postmaster/postmaster.c in master branch but didn't
> find what you mentioned.

This hunk is in src/backend/libpq/hba.c, in the parse_hba_auth_opt()
function. The code there uses the less concise form throughout, as far
as I can see.

--Jacob

On Wed, Sep 1, 2021 at 8:43 PM Jacob Champion <pchampion@vmware.com> wrote:
>
> On Wed, 2021-09-01 at 15:42 +0000, Jacob Champion wrote:
> > The cfbot found a failure in postgres_fdw, which I completely neglected
> > in my design. I think the desired functionality should be to allow the
> > ldapuser connection option during CREATE USER MAPPING but not CREATE
> > SERVER.
>
> Fixed in v2, attached.

A couple of quick comments from a quick look-over:

I'm a bit hesitant about the ldapuser libpq parameter. Do we really
want to limit ourselves to just ldap, if we allow this? I mean, why
not allow say radius or pam to also specify a different username for
the external system? If we want to do that, now or in the future, we
should have a much more generic parameter name, something like
authuser?

Why do we actually need ldap_map_dn? Shouldn't this just be what
happens if you specify map= on an ldap connection?

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

On Tue, 2021-09-28 at 15:38 +0200, Magnus Hagander wrote:
> I'm a bit hesitant about the ldapuser libpq parameter. Do we really
> want to limit ourselves to just ldap, if we allow this? I mean, why
> not allow say radius or pam to also specify a different username for
> the external system? If we want to do that, now or in the future, we
> should have a much more generic parameter name, something like
> authuser?

I'd be on board with a more general option name.

So from the perspective of a SASL exchange, PGUSER would be the
authorization identity, and PGAUTHUSER, say, would be the
authentication identity. Is "auth" a clear enough prefix that users and
devs will understand what the difference is between the two?

         | authn             authz
---------+-----------------------------------
  envvar | PGAUTHUSER        PGUSER
conninfo | authuser          user
frontend | conn->pgauthuser  conn->pguser backend | port->auth_user   port->user_name

> Why do we actually need ldap_map_dn? Shouldn't this just be what
> happens if you specify map= on an ldap connection?

For simple-bind setups, I think requiring users to match an entire DN
is probably unnecessary (and/or dangerous once you start getting into
regex mapping), so the map uses the bare username by default. My intent
was for that to have the same default behavior as cert maps.

Thanks,
--Jacob

On Tue, 2021-09-28 at 18:02 +0000, Jacob Champion wrote:
> On Tue, 2021-09-28 at 15:38 +0200, Magnus Hagander wrote:
> > I'm a bit hesitant about the ldapuser libpq parameter. Do we really
> > want to limit ourselves to just ldap, if we allow this? I mean, why
> > not allow say radius or pam to also specify a different username for
> > the external system? If we want to do that, now or in the future, we
> > should have a much more generic parameter name, something like
> > authuser?
> 
> I'd be on board with a more general option name.
> 
> So from the perspective of a SASL exchange, PGUSER would be the
> authorization identity, and PGAUTHUSER, say, would be the
> authentication identity. Is "auth" a clear enough prefix that users and
> devs will understand what the difference is between the two?
> 
>          | authn             authz
> ---------+-----------------------------------
>   envvar | PGAUTHUSER        PGUSER
> conninfo | authuser          user
> frontend | conn->pgauthuser  conn->pguser backend | port->auth_user   port->user_name
> 
> > Why do we actually need ldap_map_dn? Shouldn't this just be what
> > happens if you specify map= on an ldap connection?
> 
> For simple-bind setups, I think requiring users to match an entire DN
> is probably unnecessary (and/or dangerous once you start getting into
> regex mapping), so the map uses the bare username by default. My intent
> was for that to have the same default behavior as cert maps.
> 
> Thanks,
> --Jacob

On Tue, 2021-09-28 at 18:08 +0000, Jacob Champion wrote:
> >          | authn             authz
> > ---------+-----------------------------------
> >   envvar | PGAUTHUSER        PGUSER
> > conninfo | authuser          user
> > frontend | conn->pgauthuser  conn->pguser backend | port->auth_user   port->user_name

Ugh, PEBKAC problems today... apologies. This should have been

         | authn             authz
---------+-----------------------------------
  envvar | PGAUTHUSER        PGUSER
conninfo | authuser          user
frontend | conn->pgauthuser  conn->pguser
 backend | port->auth_user   port->user_name

--Jacob

On Tue, 2021-09-28 at 18:15 +0000, Jacob Champion wrote:
>          | authn             authz
> ---------+-----------------------------------
>   envvar | PGAUTHUSER        PGUSER
> conninfo | authuser          user
> frontend | conn->pgauthuser  conn->pguser
>  backend | port->auth_user   port->user_name

v3 attached, which uses the above naming scheme and removes the stale
TODO. Changes in since-v2.

--Jacob

On Fri, 2021-10-29 at 17:38 +0000, Jacob Champion wrote:
> v3 attached, which uses the above naming scheme and removes the stale
> TODO. Changes in since-v2.

v4 rebases over the recent TAP changes.

--Jacob