Обсуждение: allowing "map" for password auth methods with clientcert=verify-full

Hi,

Since PostgreSQL 12 (0516c61b756e39) we have allowed for the ability to 
set "clientcert=verify-full" against various HBA authentication methods. 
This provides the ability to provide "multi-factor authentication" e.g. 
a client must provide both a valid certificate with a CN (or DN) that 
matches the user account, as well as a separate authentication challenge 
(e.g. a password).

With certificate-based authentication methods and other methods, we 
allow for users to specify a mapping in pg_ident, e.g. if one needs to 
perform a rewrite on the CN to match the username that is specified 
within PostgreSQL.

It seems logical that we should allow for something like:

    hostssl all all all scram-sha-256 clientcert=verify-full map=map

so we can accept certificates that may have CNs that can be mapped to a 
PostgreSQL user name.

Currently we can't do this, as one will get the error:

 > authentication option "map" is only valid for authentication methods
 > ident, peer, gssapi, sspi, and cert

I propose the below patch to add the currently supported password 
methods, scram-sha-256 + md5 to allow for the "map" parameter to be 
used. I hesitate to add md5 given we're trying to phase it out, so open 
to debate there.

With my testing, this does work when you specify clientcert=verify-full: 
PostgreSQL will correctly map the certificate. If you do not have 
clientcert=verify-full, the mapping appears to do nothing.

If this seems acceptable/valid, I'll add the appropriate documentation 
and whatever else may be required.

Thanks,

Jonathan

"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> With certificate-based authentication methods and other methods, we 
> allow for users to specify a mapping in pg_ident, e.g. if one needs to 
> perform a rewrite on the CN to match the username that is specified 
> within PostgreSQL.

> It seems logical that we should allow for something like:
>     hostssl all all all scram-sha-256 clientcert=verify-full map=map
> so we can accept certificates that may have CNs that can be mapped to a 
> PostgreSQL user name.

I think this is conflating two different things: a mapping from the
username given in the startup packet, and a mapping from the TLS
certificate CN.  Using the same keyword and terminology for both
is going to lead to pain.  I'm on board with the idea if we can
disentangle that, though.

            regards, tom lane

On 10/26/21 3:26 PM, Tom Lane wrote:
> "Jonathan S. Katz" <jkatz@postgresql.org> writes:
>> With certificate-based authentication methods and other methods, we
>> allow for users to specify a mapping in pg_ident, e.g. if one needs to
>> perform a rewrite on the CN to match the username that is specified
>> within PostgreSQL.
> 
>> It seems logical that we should allow for something like:
>>     hostssl all all all scram-sha-256 clientcert=verify-full map=map
>> so we can accept certificates that may have CNs that can be mapped to a
>> PostgreSQL user name.
> 
> I think this is conflating two different things: a mapping from the
> username given in the startup packet, and a mapping from the TLS
> certificate CN.  Using the same keyword and terminology for both
> is going to lead to pain.  I'm on board with the idea if we can
> disentangle that, though.

Hm, don't we already have that already when using "cert" combined with 
the "map" parameter? This is the main reason I "stumbled" upon this 
recommendation.

Based on what you say and if we're continuing with this functionality, 
would solving the conflation be a matter of primarily documentation?

Thanks,

Jonathan

"Jonathan S. Katz" <jkatz@postgresql.org> writes:
> On 10/26/21 3:26 PM, Tom Lane wrote:
>> I think this is conflating two different things: a mapping from the
>> username given in the startup packet, and a mapping from the TLS
>> certificate CN.  Using the same keyword and terminology for both
>> is going to lead to pain.  I'm on board with the idea if we can
>> disentangle that, though.

> Hm, don't we already have that already when using "cert" combined with 
> the "map" parameter? This is the main reason I "stumbled" upon this 
> recommendation.

I'm not exactly convinced that the existing design is any good.
I'm suggesting that we stop and think about it before propagating
it to a bunch of other use-cases.

Per "21.2. User Name Maps", I think that the map parameter is supposed
to translate from the startup packet's user name to the SQL role name.
ISTM that what is in the cert CN might be different from either
(particularly by perhaps having a domain name attached).  So I'd be
happier if there were a separate mapping available for the CN.

            regards, tom lane

On 10/26/21 18:16, Tom Lane wrote:
> "Jonathan S. Katz" <jkatz@postgresql.org> writes:
>> On 10/26/21 3:26 PM, Tom Lane wrote:
>>> I think this is conflating two different things: a mapping from the
>>> username given in the startup packet, and a mapping from the TLS
>>> certificate CN.  Using the same keyword and terminology for both
>>> is going to lead to pain.  I'm on board with the idea if we can
>>> disentangle that, though.
>> Hm, don't we already have that already when using "cert" combined with 
>> the "map" parameter? This is the main reason I "stumbled" upon this 
>> recommendation.
> I'm not exactly convinced that the existing design is any good.
> I'm suggesting that we stop and think about it before propagating
> it to a bunch of other use-cases.
>
> Per "21.2. User Name Maps", I think that the map parameter is supposed
> to translate from the startup packet's user name to the SQL role name.
> ISTM that what is in the cert CN might be different from either
> (particularly by perhaps having a domain name attached).  So I'd be
> happier if there were a separate mapping available for the CN.
>
>             


Possibly slightly off topic, but

The cert+map pattern is very useful in conjunction with pgbouncer. Using
it with an auth query to get the password pgbouncer doesn't even need to
have a list of users, and we in effect delegate authentication to
pgbouncer.

It would be nice to have + and @ expansion for the usernames in the
ident file, like there is for pg_hba.conf.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Tue, 2021-10-26 at 18:16 -0400, Tom Lane wrote:
> Per "21.2. User Name Maps", I think that the map parameter is supposed
> to translate from the startup packet's user name to the SQL role name.

I may have misunderstood what you wrote, but IIUC the startup packet's
user name _is_ the SQL role name, even when using a map. The map is
just determining whether or not the authenticated ID (pulled from a
certificate, or from Kerberos, or etc.) is authorized to use that role
name. It's not a translation, because you can have a one-to-many user
mapping (where me@example.com is allowed to log in as `me` or
`postgres` or `admin` or...).

Please correct me if I've missed something -- I need to have it right
in my head, given my other patches in this area...

--Jacob

On Wed, 2021-10-27 at 10:12 -0400, Andrew Dunstan wrote:
> Possibly slightly off topic, but
> 
> The cert+map pattern is very useful in conjunction with pgbouncer. Using
> it with an auth query to get the password pgbouncer doesn't even need to
> have a list of users, and we in effect delegate authentication to
> pgbouncer.
> 
> It would be nice to have + and @ expansion for the usernames in the
> ident file, like there is for pg_hba.conf.

(Probably is off-topic :D but +1 to the concept. Combined with LDAP
mapping that could make some of the ad-hoc LDAP-to-Postgres sync
scripts a lot simpler.)

--Jacob

On 10/27/21 12:14 PM, Jacob Champion wrote:
> On Tue, 2021-10-26 at 18:16 -0400, Tom Lane wrote:
>> Per "21.2. User Name Maps", I think that the map parameter is supposed
>> to translate from the startup packet's user name to the SQL role name.
> 
> I may have misunderstood what you wrote, but IIUC the startup packet's
> user name _is_ the SQL role name, even when using a map. The map is
> just determining whether or not the authenticated ID (pulled from a
> certificate, or from Kerberos, or etc.) is authorized to use that role
> name. It's not a translation, because you can have a one-to-many user
> mapping (where me@example.com is allowed to log in as `me` or
> `postgres` or `admin` or...).
> 
> Please correct me if I've missed something -- I need to have it right
> in my head, given my other patches in this area...

To Tom's earlier point, I understand why we may want to pause and think 
about this.

I don't know the whole history of the "pg_ident.conf" file, but judging 
by the name, my guess is that the mapping functionality started with the 
"ident" authentication support, and then it was used for other auth 
types that could benefit from mapping (cert/gssapi etc.). The 
documentation referenced also skews towards describing what the original 
functionality for ident does.

That said, the existing functionality does match what Jacob is 
describing and what my own understanding is.

The patch I propose just layers on top of the existing functionality -- 
  you could even argue that it's "fixing a bug" that we did not add the 
current "map" support for the case of "clientcert=verify-full" given we 
do introspect the certificate CN to see if it matches the SQL role name.

In terms of other user mapping functionality, we have ad hoc support for 
FDWs when trying to map to a user in a different server:

https://www.postgresql.org/docs/current/sql-createusermapping.html

I'm unsure if there is anything we'd want to leverage here, as the 
overall goal of this is to provide the ability to establish a connection 
with a remote server.

I think in the context of doing any new work, I'd step back and ask what 
problem is this solving? The main one I think of is an integration with 
a SSO system has a credential with an identifier that does not match 
it's credential in PostgreSQL? (That would be the case I was working on, 
though said case was borrowed from our docs). Are there other cases?

That said, what would make it easier to manage it then? Maybe a lot of 
this is documenting and some expansion on what the pg_ident.conf file 
can do (per Andrew's suggestion). And maybe a new name for said file.

I don't know if we would want to bring any of this into the catalog or 
not -- but perhaps there may be some advantages to that from an 
administration standpoint.

Anyway, those are my initial thoughts on the challenge to think a bit 
more deeply about this. I'd still suggest considering the patch I 
propose as an "immediate fix" for existing versions as, at least to 
myself, I can argue it's a bug. We can then do some more work to make 
the overall system a bit easier/clearer to use and maintain.

Thanks,

Jonathan

On Wed, 2021-10-27 at 12:53 -0400, Jonathan S. Katz wrote:
> The patch I propose just layers on top of the existing functionality -- 
>   you could even argue that it's "fixing a bug" that we did not add the 
> current "map" support for the case of "clientcert=verify-full" given we 
> do introspect the certificate CN to see if it matches the SQL role name.

Well, also to Tom's earlier point, though, this is a different sort of
mapping. Which "map" should we use if someone combines
clientcert=verify-full with an auth method which already uses a map
itself? Does the DBA want to map the auth name, the cert name, or both?

The current usermap support is piecemeal and I'd like to see it
completed, but I think you may be painting yourself into a corner if
you fix it in this way. (From a quick look at the patch, I'm also
worried that this happens to work by accident, but that may just be
FUD.)

> I think in the context of doing any new work, I'd step back and ask what 
> problem is this solving? The main one I think of is an integration with 
> a SSO system has a credential with an identifier that does not match 
> it's credential in PostgreSQL? (That would be the case I was working on, 
> though said case was borrowed from our docs). Are there other cases?
> 
> That said, what would make it easier to manage it then? Maybe a lot of 
> this is documenting and some expansion on what the pg_ident.conf file 
> can do (per Andrew's suggestion). And maybe a new name for said file.

I agree that the authorization system is due for a tuneup, for what
it's worth. Some of the comments from Magnus on my LDAP patch [1] kind
of hinted in that direction as well, I think, even if my approach is
rejected in the end.

--Jacob

[1] https://www.postgresql.org/message-id/flat/1a61806047c536e7528b943d0cfe12608118ca31.camel@vmware.com

Greetings,

* Jacob Champion (pchampion@vmware.com) wrote:
> On Wed, 2021-10-27 at 12:53 -0400, Jonathan S. Katz wrote:
> > The patch I propose just layers on top of the existing functionality --
> >   you could even argue that it's "fixing a bug" that we did not add the
> > current "map" support for the case of "clientcert=verify-full" given we
> > do introspect the certificate CN to see if it matches the SQL role name.
>
> Well, also to Tom's earlier point, though, this is a different sort of
> mapping. Which "map" should we use if someone combines
> clientcert=verify-full with an auth method which already uses a map
> itself? Does the DBA want to map the auth name, the cert name, or both?

My understanding of the mapping system with pg_ident has always been
that it's a mapping from the 'authenticated user' to the 'user in PG'
and the point is to check if that mapping is allowed but not to actually
change anything about who the ultimately logged in user is.

This is pretty clear and simple when the two are entirely disjoint- that
is, with 'peer' or 'gssapi' or 'cert', the authenticated user will be
whatever the authentication system thinks it is- the Unix username for
peer, the Kerberos principle for gssapi, the DN or CN for cert.

In all of the above cases, the username provided to us by the end user
is the role they're trying to log in as and the mapping is just there to
check if that's allowed based on the *authenticated username*.  Perhaps
not surprisingly, 21.2 could use some improvement on this, but it's
certainly the case that the mapping is only there as a permission check
and does not actually change who the user is logging into the system as.
Whatever username is in the startup packet is the role that the user
will be logged in as if they're allowed to log in as that role.

Now, when PG is involved in the authentication of the user, then I agree
that it gets more interesting.  That is- the user wants to log in as
user u1, they have a certificate that has a DN of u1/user/domain, and we
want to verify that they know the password and present the right
certificate- but which password do they need to know?

Today, there's really only one possible answer- the username in the
startup packet, because we don't have any concept of "authenticate to PG
as user u1 and then be logged in as user u2".  If we were to support a
mapping for scram or md5, we'd really need the user to tell us both who
to authenticate as and the user to log into PG as, and then we'd use
pg_ident.conf to ensure that such a mapping is allowed.  If we wanted to
implement something along the lines of "user authenticates as X but is
logged in as Y" automatically, that would need to be something other
than pg_ident.conf, imv.  I see that's been discussed a bit on the other
thread I was explicitly trying to ignore and glad that it's more-or-less
coming to the same conclusion.

Where does that leave us with what Jonathan is suggesting though?  For
my 2c, we shouldn't allow 'map=' to be used for scram or md5 because
it'll just confuse users, until and unless we actually do the PGAUTHUSER
thing and then we can allow 'map=' to check if that mapping is allowed
and the actual SCRAM PW check is done against PGAUTHUSER and then the
logged in user is the user as specified in the startup packet, assuming
that mapping is allowed.  For Jonathan's actual case though, we should
add a 'certmap' option instead and have that be explicitly for the case
where it's scram w/ clientcert=verify-full and then we check the mapping
of the DN/CN to the startup-packet username.  There's no reason this
couldn't also work with a 'map' specified and PGAUTHUSER set and SCRAM
used to verify against that at the same time.

> The current usermap support is piecemeal and I'd like to see it
> completed, but I think you may be painting yourself into a corner if
> you fix it in this way. (From a quick look at the patch, I'm also
> worried that this happens to work by accident, but that may just be
> FUD.)

I don't think it's an accident that it works, but a few comments and
a more explicit option for the user interface would be good.  Admins
would be confused if 'map=xyz' was accepted for SCRAM but then didn't
actually do anything.

> > I think in the context of doing any new work, I'd step back and ask what
> > problem is this solving? The main one I think of is an integration with
> > a SSO system has a credential with an identifier that does not match
> > it's credential in PostgreSQL? (That would be the case I was working on,
> > though said case was borrowed from our docs). Are there other cases?
> >
> > That said, what would make it easier to manage it then? Maybe a lot of
> > this is documenting and some expansion on what the pg_ident.conf file
> > can do (per Andrew's suggestion). And maybe a new name for said file.
>
> I agree that the authorization system is due for a tuneup, for what
> it's worth. Some of the comments from Magnus on my LDAP patch [1] kind
> of hinted in that direction as well, I think, even if my approach is
> rejected in the end.

Perhaps not a surprise, but I continue to be against the idea of adding
anything more to the insecure hack that is our LDAP auth method.  We
should be moving away from that, not adding to it.

That this would also require a new connection option / envvar to tell us
who the user wants to authenticate to LDAP as doesn't exactly make me
any more thrilled with it.

Just my 2c though and I know others don't necessarily agree with me on
this point.

Thanks,

Stephen

On Mon, 2021-11-08 at 15:32 -0500, Stephen Frost wrote:
> Where does that leave us with what Jonathan is suggesting though?  For
> my 2c, we shouldn't allow 'map=' to be used for scram or md5 because
> it'll just confuse users, until and unless we actually do the PGAUTHUSER
> thing and then we can allow 'map=' to check if that mapping is allowed
> and the actual SCRAM PW check is done against PGAUTHUSER and then the
> logged in user is the user as specified in the startup packet, assuming
> that mapping is allowed.  For Jonathan's actual case though, we should
> add a 'certmap' option instead and have that be explicitly for the case
> where it's scram w/ clientcert=verify-full and then we check the mapping
> of the DN/CN to the startup-packet username.  There's no reason this
> couldn't also work with a 'map' specified and PGAUTHUSER set and SCRAM
> used to verify against that at the same time.

Agreed.

> Perhaps not a surprise, but I continue to be against the idea of adding
> anything more to the insecure hack that is our LDAP auth method.  We
> should be moving away from that, not adding to it.

Paraphrasing you from earlier, the "authenticate as one user and then
log in as another" use case is the one I'm trying to expand. LDAP is
just the auth method I happen to have present-day customer cases for.

> That this would also require a new connection option / envvar to tell us
> who the user wants to authenticate to LDAP as doesn't exactly make me
> any more thrilled with it.

Forgetting the LDAP part for a moment, do you have another suggestion
for how we can separate the role name from the user name? The
connection string seemed to be the most straightforward.

--Jacob