[PATCH] Support pg_ident mapping for LDAP
Hello,
There was a brief discussion [1]/messages/by-id/94f6b945f9ca8cabe2b9d2a38ec19dca6f90a083.camel@vmware.com 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]: /messages/by-id/94f6b945f9ca8cabe2b9d2a38ec19dca6f90a083.camel@vmware.com
Attachments:
Allow-user-name-mapping-with-LDAP.patchtext/x-patch; name=Allow-user-name-mapping-with-LDAP.patchDownload+291-16
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, Sep 1, 2021 at 11:43 AM 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.
--Jacob
Hi,
+ 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;
Cheers
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, Sep 1, 2021 at 1:56 PM Jacob Champion <pchampion@vmware.com> wrote:
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
Hi,
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.
I still think my recommendation is concise.
Cheers
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
Show quoted text
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_nameWhy 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