Обсуждение: Proposal: Support custom authentication methods using hooks

Hi Samay,

> I wanted to submit a patch to expose 2 new hooks (one for the authentication check and another one for error
reporting)in auth.c. These will allow users to implement their own authentication methods for Postgres or add custom
logicaround authentication. 

I like the idea - PostgreSQL is all about extendability. Also, well
done with TAP tests and an example extension. This being said, I
didn't look at the code yet, but cfbot seems to be happy with it:
http://cfbot.cputube.org/

> One constraint in the current implementation is that we allow only one authentication provider to be loaded at a
time.In the future, we can add more functionality to maintain an array of hooks and call the appropriate one based on
theprovider name in the pg_hba line. 

This sounds like a pretty severe and unnecessary limitation to me. Do
you think it would be difficult to bypass it in the first
implementation?

--
Best regards,
Aleksander Alekseev

On 2/24/22 04:16, Aleksander Alekseev wrote:
> Hi Samay,
>
>> I wanted to submit a patch to expose 2 new hooks (one for the authentication check and another one for error
reporting)in auth.c. These will allow users to implement their own authentication methods for Postgres or add custom
logicaround authentication.
 
> I like the idea - PostgreSQL is all about extendability. Also, well
> done with TAP tests and an example extension. This being said, I
> didn't look at the code yet, but cfbot seems to be happy with it:
> http://cfbot.cputube.org/
>
>> One constraint in the current implementation is that we allow only one authentication provider to be loaded at a
time.In the future, we can add more functionality to maintain an array of hooks and call the appropriate one based on
theprovider name in the pg_hba line.
 
> This sounds like a pretty severe and unnecessary limitation to me. Do
> you think it would be difficult to bypass it in the first
> implementation?



Yeah, I think we  would want a set of providers that could be looked up
at runtime.


I think this is likely to me material for release 16, so there's plenty
of time to get it right.


cheers


andrew

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

On Thu, 2022-02-17 at 11:25 -0800, samay sharma wrote:
> To enable this, I've proposed adding a new authentication method
> "custom" which can be specified in pg_hba.conf and takes a mandatory
> argument  "provider" specifying which authentication provider to use.
> I've also moved a couple static functions to headers so that
> extensions can call them.
> 
> Sample pg_hba.conf line to use a custom provider:
> 
> host    all             all             ::1/128              
>   custom provider=test

One caveat is that this only works given information available from
existing authentication methods, because that's all the client
supports. In practice, it seems to only be useful with plaintext
password authentication over an SSL connection.

I still like the approach though. There's a lot of useful stuff you can
do at authentication time with only the connection information and a
password. It could be useful to authenticate against different
services, or some kind of attack detection, etc.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> On Thu, 2022-02-17 at 11:25 -0800, samay sharma wrote:
>> To enable this, I've proposed adding a new authentication method
>> "custom" which can be specified in pg_hba.conf and takes a mandatory
>> argument  "provider" specifying which authentication provider to use.

> One caveat is that this only works given information available from
> existing authentication methods, because that's all the client
> supports. In practice, it seems to only be useful with plaintext
> password authentication over an SSL connection.

... and, since we can't readily enforce that the client only sends
those cleartext passwords over suitably-encrypted connections, this
could easily be a net negative for security.  Not sure that I think
it's a good idea.

            regards, tom lane

Hi,

On 2022-02-24 17:02:45 -0800, Jeff Davis wrote:
> On Thu, 2022-02-17 at 11:25 -0800, samay sharma wrote:
> One caveat is that this only works given information available from
> existing authentication methods, because that's all the client
> supports. In practice, it seems to only be useful with plaintext
> password authentication over an SSL connection.

Why is it restricted to that? You could do sasl negotiation as well from what
I can see? And that'd theoretically also allow to negotiate whether the client
supports different ways of doing auth?  Not saying that that's easy, but I
don't think it's a fundamental restriction.

I also can imagine things like using selinux labeling of connections.

We have several useful authentication technologies built ontop of plaintext
exchange. Radius, Ldap, Pam afaics could be implemented as an extension?

Greetings,

Andres Freund

On Thu, 2022-02-24 at 19:47 -0800, Andres Freund wrote:
> Why is it restricted to that? You could do sasl negotiation as well
> from what
> I can see? And that'd theoretically also allow to negotiate whether
> the client
> supports different ways of doing auth?  Not saying that that's easy,
> but I
> don't think it's a fundamental restriction.

Good point! It would only work with enhanced clients though -- maybe in
the future we'd make libpq pluggable with new auth methods?

> We have several useful authentication technologies built ontop of
> plaintext
> exchange. Radius, Ldap, Pam afaics could be implemented as an
> extension?

Yes, and it means that we won't have to extend that list in core in the
future when new methods become popular.

Regards,
    Jeff Davis

On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
> ... and, since we can't readily enforce that the client only sends
> those cleartext passwords over suitably-encrypted connections, this
> could easily be a net negative for security.  Not sure that I think
> it's a good idea.

I don't understand your point. Can't you just use "hostssl" rather than
"host"?

Also there are some useful cases that don't really require SSL, like
when the client and host are on the same machine, or if you have a
network secured some other way.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
>> ... and, since we can't readily enforce that the client only sends
>> those cleartext passwords over suitably-encrypted connections, this
>> could easily be a net negative for security.  Not sure that I think
>> it's a good idea.

> I don't understand your point. Can't you just use "hostssl" rather than
> "host"?

My point is that sending cleartext passwords over the wire is an
insecure-by-definition protocol that we shouldn't be encouraging
more use of.

            regards, tom lane

Hi,

On 2022-02-25 09:33:45 -0800, Jeff Davis wrote:
> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
> > ... and, since we can't readily enforce that the client only sends
> > those cleartext passwords over suitably-encrypted connections, this
> > could easily be a net negative for security.  Not sure that I think
> > it's a good idea.
> 
> I don't understand your point. Can't you just use "hostssl" rather than
> "host"?

And the extension could check Port->ssl_in_use before sendAuthRequest(AUTH_REQ_PASSWORD)
if it wanted to restrict it.

Greetings,

Andres Freund

On 2/25/22 12:39 PM, Tom Lane wrote:
> Jeff Davis <pgsql@j-davis.com> writes:
>> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
>>> ... and, since we can't readily enforce that the client only sends
>>> those cleartext passwords over suitably-encrypted connections, this
>>> could easily be a net negative for security.  Not sure that I think
>>> it's a good idea.
> 
>> I don't understand your point. Can't you just use "hostssl" rather than
>> "host"?
> 
> My point is that sending cleartext passwords over the wire is an
> insecure-by-definition protocol that we shouldn't be encouraging
> more use of.

This is my general feeling as well. We just spent a bunch of effort 
adding, refining, and making SCRAM the default method. I think doing 
anything that would drive more use of sending plaintext passwords, even 
over TLS, is counter to that.

I do understand arguments for (e.g. systems that require checking 
password complexity), but I wonder if it's better for us to delegate 
that to an external auth system. Regardless, I can get behind Andres' 
point to "check Port->ssl_in_use before sendAuthRequest(AUTH_REQ_PASSWORD)".

I'm generally in favor of being able to support additional 
authentication methods, the first one coming to mind is supporting OIDC. 
Having a pluggable auth infrastructure could possibly make such efforts 
easier. I'm definitely intrigued.

Jonathan

On Fri, 2022-02-25 at 12:39 -0500, Tom Lane wrote:
> My point is that sending cleartext passwords over the wire is an
> insecure-by-definition protocol that we shouldn't be encouraging
> more use of.

We can require custom auth entries in pg_hba.conf to also specify
local, hostssl or hostgssenc.

It might annoy people who have a network secured at some other layer,
or who have the client on the same machine as the host. We could allow
plain "host" if someone specifies "customplain" explicitly.

Regards,
    Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:
> On Fri, 2022-02-25 at 12:39 -0500, Tom Lane wrote:
>> My point is that sending cleartext passwords over the wire is an
>> insecure-by-definition protocol that we shouldn't be encouraging
>> more use of.

> We can require custom auth entries in pg_hba.conf to also specify
> local, hostssl or hostgssenc.

That's just a band-aid, though.  It does nothing for the other
reasons not to want cleartext passwords, notably that you might
be giving your password to a compromised server.

I'm happy to add support for custom auth methods if they can use
a protocol that's safer than cleartext-password.  But if that's the
only feasible option, then we're just encouraging people to use
insecure methods.

I also have in mind here that there has been discussion of giving
libpq a feature to refuse, on the client side, to send cleartext
passwords.  (I don't recall right now if that's been mentioned
publicly or only among the security team, but it's definitely
under consideration.)

So I think this proposal needs more thought.  A server-side hook
alone is not a useful improvement IMO; it's closer to being an
attractive nuisance.

            regards, tom lane

On Fri, 2022-02-25 at 13:40 -0500, Jonathan S. Katz wrote:
> I'm generally in favor of being able to support additional 
> authentication methods, the first one coming to mind is supporting OIDC. 
> Having a pluggable auth infrastructure could possibly make such efforts 
> easier. I'm definitely intrigued.

I'm hoping to dust off my OAuth patch and see if it can be ported on
top of this proposal.

--Jacob

Hi,

On 2022-02-25 14:10:39 -0500, Tom Lane wrote:
> I'm happy to add support for custom auth methods if they can use
> a protocol that's safer than cleartext-password.  But if that's the
> only feasible option, then we're just encouraging people to use
> insecure methods.

It looks like scram can be used without much trouble. All the necessary
infrastructure to implement it without duplication appears to be public.  The
plugin would need to get a secret from whereever and call
  CheckSASLAuth(&pg_be_scram_mech, port, shadow_pass, logdetail);

or if it needs to do something more complicated than CheckSASLAuth(), it can
use AUTH_REQ_SASL{,_FIN CONT} itself.

Of course whether it's viable depends on what the custom auth method wants to
do. But it's not a restriction of the authentication plugin model.

Greetings,

Andres Freund

On Fri, 2022-02-25 at 14:10 -0500, Tom Lane wrote:
> I'm happy to add support for custom auth methods if they can use
> a protocol that's safer than cleartext-password.  But if that's the
> only feasible option, then we're just encouraging people to use
> insecure methods.

FWIW, I'd like to be able to use a token in the password field, and
then authenticate that token. So I didn't intend to send an actual
password in plaintext.

Maybe we should have a new "token" connection parameter so that libpq
can allow sending a token plaintext but refuse to send a password in
plaintext?

> I also have in mind here that there has been discussion of giving
> libpq a feature to refuse, on the client side, to send cleartext
> passwords.

I am generally in favor of that idea, but I'm not sure that will
completely resolve the issue. For instance, should it also refuse MD5?

Regards,
    Jeff Davis

On 28.02.22 00:17, Jeff Davis wrote:
>> I also have in mind here that there has been discussion of giving
>> libpq a feature to refuse, on the client side, to send cleartext
>> passwords.
> I am generally in favor of that idea, but I'm not sure that will
> completely resolve the issue. For instance, should it also refuse MD5?

Presumably that feature could be more generally "refuse these 
authentication mechanisms" rather than only one hardcoded one.

On 17.02.22 20:25, samay sharma wrote:
> A use case where this is useful are environments where you want 
> authentication to be centrally managed across different services. This 
> is a common deployment model for cloud providers where customers like to 
> use single sign on and authenticate across different services including 
> Postgres. Implementing this now is tricky as it requires syncing that 
> authentication method's credentials with Postgres (and that gets 
> trickier with TTL/expiry etc.). With these hooks, you can implement an 
> extension to check credentials directly using the 
> authentication provider's APIs.

We already have a variety of authentication mechanisms that support 
central management: LDAP, PAM, Kerberos, Radius.  What other mechanisms 
are people thinking about implementing using these hooks?  Maybe there 
are a bunch of them, in which case a hook system might be sensible, but 
if there are only one or two plausible ones, we could also just make 
them built in.

Greetings,

* Jonathan S. Katz (jkatz@postgresql.org) wrote:
> On 2/25/22 12:39 PM, Tom Lane wrote:
> >Jeff Davis <pgsql@j-davis.com> writes:
> >>On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
> >>>... and, since we can't readily enforce that the client only sends
> >>>those cleartext passwords over suitably-encrypted connections, this
> >>>could easily be a net negative for security.  Not sure that I think
> >>>it's a good idea.
> >
> >>I don't understand your point. Can't you just use "hostssl" rather than
> >>"host"?
> >
> >My point is that sending cleartext passwords over the wire is an
> >insecure-by-definition protocol that we shouldn't be encouraging
> >more use of.
>
> This is my general feeling as well. We just spent a bunch of effort adding,
> refining, and making SCRAM the default method. I think doing anything that
> would drive more use of sending plaintext passwords, even over TLS, is
> counter to that.

Agreed.

> I do understand arguments for (e.g. systems that require checking password
> complexity), but I wonder if it's better for us to delegate that to an
> external auth system. Regardless, I can get behind Andres' point to "check
> Port->ssl_in_use before sendAuthRequest(AUTH_REQ_PASSWORD)".

Password complexity is only needed to be checked at the time of password
change though, which is not on every login, and should be after a
confirmed mutual authentication between the client and the server.
That's a very different situation.

> I'm generally in favor of being able to support additional authentication
> methods, the first one coming to mind is supporting OIDC. Having a pluggable
> auth infrastructure could possibly make such efforts easier. I'm definitely
> intrigued.

I'm not thrilled with the idea, for my part.

Thanks,

Stephen

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Fri, 2022-02-25 at 14:10 -0500, Tom Lane wrote:
> > I also have in mind here that there has been discussion of giving
> > libpq a feature to refuse, on the client side, to send cleartext
> > passwords.
>
> I am generally in favor of that idea, but I'm not sure that will
> completely resolve the issue. For instance, should it also refuse MD5?

md5 should be removed.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> md5 should be removed.

Really? I've always thought that the arguments against it were
overblown for our use-case.  At any rate, it's likely to be
years before we could practically do that, since it's the best
that older client libraries can manage.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > md5 should be removed.
>
> Really? I've always thought that the arguments against it were
> overblown for our use-case.  At any rate, it's likely to be
> years before we could practically do that, since it's the best
> that older client libraries can manage.

Yes, really, it's a known-broken system which suffers from such an old
and well known attack that it's been given a name: pass-the-hash.  As
was discussed on this thread even, just the fact that it's not trivial
to break on the wire doesn't make it not-broken, particularly when we
use the username (which is rather commonly the same one used across
multiple systems..) as the salt.  Worse, md5 isn't exactly the pinnacle
of hashing techniques around these days.

The wiki page goes over it in some detail regarding LM/NTLM which
suffers the same problem (and also uses a challenge-response for the
over-the-network bits): https://en.wikipedia.org/wiki/Pass_the_hash

Further, a whole bunch of effort was put in to get scram support added
to the different libraries and language bindings and such, specifically
to allow us to get to a point where we can drop md5.  Even after it's
removed, folks will have 5 years before the release that removes it is
the oldest supported release.  I don't think we'll somehow get agreement
to remove it for v15, so it'll be 5 major versions of overlap (11->15)
by the time v16 comes out, and a total of 10 years of support for scram
before md5 is gone.

That's plenty, it's time to move on.

Keeping it around will just push out the point at which everyone will
finally be done with it, as there's really only two groups: those who
have already moved to scram, and those who won't move until they want to
upgrade to a release that doesn't have md5.

Thanks,

Stephen

On Mon, Feb 28, 2022 at 04:42:55PM -0500, Stephen Frost wrote:
> Keeping it around will just push out the point at which everyone will
> finally be done with it, as there's really only two groups: those who
> have already moved to scram, and those who won't move until they want to
> upgrade to a release that doesn't have md5.

FWIW, I am not sure if we are at this point yet.  An extra reason to
remove it would be that it is a support burden, but I don't have seen
in recent memory any problems related to it that required any deep
changes in the way to use it, and its code paths are independent.

The last time I played with this area is the recent error handling
improvement with cryptohashes but MD5 has actually helped here in
detecting the problem as a patched OpenSSL would complain if trying to
use MD5 as hash function when FIPS is enabled.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Mon, Feb 28, 2022 at 04:42:55PM -0500, Stephen Frost wrote:
> > Keeping it around will just push out the point at which everyone will
> > finally be done with it, as there's really only two groups: those who
> > have already moved to scram, and those who won't move until they want to
> > upgrade to a release that doesn't have md5.
>
> FWIW, I am not sure if we are at this point yet.  An extra reason to
> remove it would be that it is a support burden, but I don't have seen
> in recent memory any problems related to it that required any deep
> changes in the way to use it, and its code paths are independent.

Ongoing reports that there's a known vulnerability aren't great to have
to deal with.  We can at least point people to scram but that's not
great.

> The last time I played with this area is the recent error handling
> improvement with cryptohashes but MD5 has actually helped here in
> detecting the problem as a patched OpenSSL would complain if trying to
> use MD5 as hash function when FIPS is enabled.

Having to continue to deal with md5 as an algorithm when it's known to
be notably less secure and so much so that organizations essentially ban
its use for exactly what we're using it for, in fact, another reason to
remove it, not a reason to keep it.  Better code coverage testing of
error paths is the answer to making sure that our error handling behaves
properly.

Thanks,

Stephen

On Mon, Feb 28, 2022 at 03:46:34PM -0500, Stephen Frost wrote:
> md5 should be removed.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 3/1/22 8:31 AM, Stephen Frost wrote:
> Greetings,
> 
> * Michael Paquier (michael@paquier.xyz) wrote:
>> On Mon, Feb 28, 2022 at 04:42:55PM -0500, Stephen Frost wrote:
>>> Keeping it around will just push out the point at which everyone will
>>> finally be done with it, as there's really only two groups: those who
>>> have already moved to scram, and those who won't move until they want to
>>> upgrade to a release that doesn't have md5.
>>
>> FWIW, I am not sure if we are at this point yet.  An extra reason to
>> remove it would be that it is a support burden, but I don't have seen
>> in recent memory any problems related to it that required any deep
>> changes in the way to use it, and its code paths are independent.
> 
> Ongoing reports that there's a known vulnerability aren't great to have
> to deal with.  We can at least point people to scram but that's not
> great.
> 
>> The last time I played with this area is the recent error handling
>> improvement with cryptohashes but MD5 has actually helped here in
>> detecting the problem as a patched OpenSSL would complain if trying to
>> use MD5 as hash function when FIPS is enabled.
> 
> Having to continue to deal with md5 as an algorithm when it's known to
> be notably less secure and so much so that organizations essentially ban
> its use for exactly what we're using it for, in fact, another reason to
> remove it, not a reason to keep it.  Better code coverage testing of
> error paths is the answer to making sure that our error handling behaves
> properly.

So, I generally agree with Stephen and his arguments for dropping the 
md5 mechanism.

If you're moving to a newer version of PostgreSQL, you likely have to 
update your connection drivers anyway (rebuilt against new libpq, 
supporting any changes in the protocol, etc). I would prefer more data 
to support that argument, but this is generally what you need to do.

However, we may need to step towards it. We took one step last release 
with defaulting to SCRAM. Perhaps this release we add a warning for 
anything using md5 auth that "this will be removed in a future release." 
(or specifically v16). We should also indicate in the docs that md5 is 
deprecated and will be removed.

We also need to think about upgrading: what happens if I try to upgrade 
and I still have user passwords stored in a md5 hash? What if all my 
password-based users have a md5 hash, does pg_upgrade fail?

As much as I want md5 gone, I think v16 is the earliest we can do it, 
but we can lay the groundwork for it now.

Thanks,

Jonathan

On 2/28/22 5:26 AM, Peter Eisentraut wrote:
> On 17.02.22 20:25, samay sharma wrote:
>> A use case where this is useful are environments where you want 
>> authentication to be centrally managed across different services. This 
>> is a common deployment model for cloud providers where customers like 
>> to use single sign on and authenticate across different services 
>> including Postgres. Implementing this now is tricky as it requires 
>> syncing that authentication method's credentials with Postgres (and 
>> that gets trickier with TTL/expiry etc.). With these hooks, you can 
>> implement an extension to check credentials directly using the 
>> authentication provider's APIs.
> 
> We already have a variety of authentication mechanisms that support 
> central management: LDAP, PAM, Kerberos, Radius.  What other mechanisms 
> are people thinking about implementing using these hooks?  Maybe there 
> are a bunch of them, in which case a hook system might be sensible, but 
> if there are only one or two plausible ones, we could also just make 
> them built in.

OIDC is the big one that comes to mind.

Jonathan

Hi,

On 2022-02-28 11:26:06 +0100, Peter Eisentraut wrote:
> We already have a variety of authentication mechanisms that support central
> management: LDAP, PAM, Kerberos, Radius.

LDAP, PAM and Radius all require cleartext passwords, so aren't a great
solution based on the concerns voiced in this thread. IME Kerberos is
operationally too complicated to really be used, unless it's already part of
the operating environment.


> What other mechanisms are people thinking about implementing using these
> hooks?

The cases I've heard about are about centralizing auth across multiple cloud
services. Including secret management in some form. E.g. allowing an
application to auth to postgres, redis and having the secret provided by
infrastructure, rather than having to hardcode it in config or such.

I can't see application developers configuring kerberos and I don't think
LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
stuff that's not portable across OSs. Radius is probably the most realistic,
but at least as implemented doesn't seem flexible enough (e.g. no access to
group memberships etc).

Nor does baking stuff like that in seem realistic to me, it'll presumably be
too cloud provider specific.

Greetings,

Andres Freund

Hi,

On 2022-02-25 13:40:54 -0500, Jonathan S. Katz wrote:
> On 2/25/22 12:39 PM, Tom Lane wrote:
> > My point is that sending cleartext passwords over the wire is an
> > insecure-by-definition protocol that we shouldn't be encouraging
> > more use of.
> 
> This is my general feeling as well. We just spent a bunch of effort adding,
> refining, and making SCRAM the default method. I think doing anything that
> would drive more use of sending plaintext passwords, even over TLS, is
> counter to that.

I want to again emphasize that, as proposed, a custom auth method can use
SCRAM if relevant for it, with a small amount of code. So the whole plaintext
discussion seems independent.

Samay, what do you think about updating the test plugin to do SCRAM instead of
plaintext, just to highlight that fact?

Greetings,

Andres Freund

On 01.03.22 22:17, Jonathan S. Katz wrote:
> If you're moving to a newer version of PostgreSQL, you likely have to 
> update your connection drivers anyway (rebuilt against new libpq, 
> supporting any changes in the protocol, etc). I would prefer more data 
> to support that argument, but this is generally what you need to do.
> 
> However, we may need to step towards it. We took one step last release 
> with defaulting to SCRAM. Perhaps this release we add a warning for 
> anything using md5 auth that "this will be removed in a future release." 
> (or specifically v16). We should also indicate in the docs that md5 is 
> deprecated and will be removed.

I find that a lot of people are still purposely using md5.  Removing it 
now or in a year would be quite a disruption.

It's also worth considering that keeping the code equipped to handle 
different kinds of password hashing would help it stay in shape if we 
ever need to add support for the next SHA after 256 or whatever.

On 01.03.22 22:34, Andres Freund wrote:
> The cases I've heard about are about centralizing auth across multiple cloud
> services. Including secret management in some form. E.g. allowing an
> application to auth to postgres, redis and having the secret provided by
> infrastructure, rather than having to hardcode it in config or such.
> 
> I can't see application developers configuring kerberos and I don't think
> LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
> alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
> stuff that's not portable across OSs. Radius is probably the most realistic,
> but at least as implemented doesn't seem flexible enough (e.g. no access to
> group memberships etc).
> 
> Nor does baking stuff like that in seem realistic to me, it'll presumably be
> too cloud provider specific.

Let's gather some more information on this.  PostgreSQL should support 
the authentication that many people want to use out of the box.  I don't 
think it would be good to be at a point where all the built-in methods 
are outdated and if you want to use the good stuff you have to hunt for 
plugins.  The number of different cloud APIs is effectively small.  I 
expect that there are a lot of similarities, like they probably all need 
support for http calls, they might need support for caching lookups, 
etc.  OIDC was mentioned elsewhere.  That's a standard.  Is that 
compatible with any cloud providers?  Would that be enough for many users?

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-02-28 11:26:06 +0100, Peter Eisentraut wrote:
> > We already have a variety of authentication mechanisms that support central
> > management: LDAP, PAM, Kerberos, Radius.
>
> LDAP, PAM and Radius all require cleartext passwords, so aren't a great
> solution based on the concerns voiced in this thread. IME Kerberos is
> operationally too complicated to really be used, unless it's already part of
> the operating environment.

... which is very, very, very often is, wrt Kerberos.

> > What other mechanisms are people thinking about implementing using these
> > hooks?
>
> The cases I've heard about are about centralizing auth across multiple cloud
> services. Including secret management in some form. E.g. allowing an
> application to auth to postgres, redis and having the secret provided by
> infrastructure, rather than having to hardcode it in config or such.

This sounds a lot like OAUTH or similar, which might be useful to
consider adding if it can be done reasonably.

> I can't see application developers configuring kerberos and I don't think
> LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
> alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
> stuff that's not portable across OSs. Radius is probably the most realistic,
> but at least as implemented doesn't seem flexible enough (e.g. no access to
> group memberships etc).

Great thing about Kerberos is that, in general, application developers
don't really need to do much to configure it.

> Nor does baking stuff like that in seem realistic to me, it'll presumably be
> too cloud provider specific.

I don't quite buy an argument that hinges on the idea of centralized
auth across multiple cloud services, as suggested above, while also
being too cloud provider specific to be something that could be baked
in, as said here.  Maybe I'm misunderstanding.  Also a bit concerned
that adding hooks on presumptions about what some cloud provider needs
isn't a good plan.

In general though, I'm certainly more supportive of the idea of adding
support for authentication mechanisms that are standardized and work
across multiple cloud providers and services in general, than about
adding things which are specific to one provider.  I don't particularly
care for the idea of adding hooks and then having cloud providers go off
and develop their own proprietary authentication system that aren't
standardized and which don't have public review, which seems like it's
the use-case for adding them.  I'm not dead-set against it, but it just
doesn't strike me as a good area to add hooks for folks to use.  Better
would be baked in code that follows a well defined and reviewed RFC that
anyone can look at and make sure follows the standard.

Thanks,

Stephen

Greetings,

* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
> On 01.03.22 22:34, Andres Freund wrote:
> >The cases I've heard about are about centralizing auth across multiple cloud
> >services. Including secret management in some form. E.g. allowing an
> >application to auth to postgres, redis and having the secret provided by
> >infrastructure, rather than having to hardcode it in config or such.
> >
> >I can't see application developers configuring kerberos and I don't think
> >LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
> >alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
> >stuff that's not portable across OSs. Radius is probably the most realistic,
> >but at least as implemented doesn't seem flexible enough (e.g. no access to
> >group memberships etc).
> >
> >Nor does baking stuff like that in seem realistic to me, it'll presumably be
> >too cloud provider specific.
>
> Let's gather some more information on this.  PostgreSQL should support the
> authentication that many people want to use out of the box.  I don't think
> it would be good to be at a point where all the built-in methods are
> outdated and if you want to use the good stuff you have to hunt for plugins.
> The number of different cloud APIs is effectively small.  I expect that
> there are a lot of similarities, like they probably all need support for
> http calls, they might need support for caching lookups, etc.  OIDC was
> mentioned elsewhere.  That's a standard.  Is that compatible with any cloud
> providers?  Would that be enough for many users?

I tend to agree with this, and to that end I'd argue that we should
probably be working to drop support for things like PAM, with
appropriate code replacing any useful capabilities that folks were using
it for (which would also mean it'd work across all the OS's we support,
which would be nice..).

Thanks,

Stephen

On 3/2/22 3:24 AM, Peter Eisentraut wrote:
> On 01.03.22 22:17, Jonathan S. Katz wrote:
>> If you're moving to a newer version of PostgreSQL, you likely have to 
>> update your connection drivers anyway (rebuilt against new libpq, 
>> supporting any changes in the protocol, etc). I would prefer more data 
>> to support that argument, but this is generally what you need to do.
>>
>> However, we may need to step towards it. We took one step last release 
>> with defaulting to SCRAM. Perhaps this release we add a warning for 
>> anything using md5 auth that "this will be removed in a future 
>> release." (or specifically v16). We should also indicate in the docs 
>> that md5 is deprecated and will be removed.
> 
> I find that a lot of people are still purposely using md5.  Removing it 
> now or in a year would be quite a disruption.

What are the reasons they are still purposely using it? The ones I have 
seen/heard are:

- Using an older driver
- On a pre-v10 PG
- Unaware of SCRAM

What I'm proposing above is to start the process of deprecating it as an 
auth method, which also allows to continue the education efforts to 
upgrae. Does that make sense?

> It's also worth considering that keeping the code equipped to handle 
> different kinds of password hashing would help it stay in shape if we 
> ever need to add support for the next SHA after 256 or whatever.

I think it's fine to keep the hashing code. The end goal is to remove 
the md5 authentication mechanism.

Thanks,

Jonathan

On Tue, Mar  1, 2022 at 08:31:19AM -0500, Stephen Frost wrote:
> > The last time I played with this area is the recent error handling
> > improvement with cryptohashes but MD5 has actually helped here in
> > detecting the problem as a patched OpenSSL would complain if trying to
> > use MD5 as hash function when FIPS is enabled.
> 
> Having to continue to deal with md5 as an algorithm when it's known to
> be notably less secure and so much so that organizations essentially ban
> its use for exactly what we're using it for, in fact, another reason to

Really?  I thought it was publicly-visible MD5 hashes that were the
biggest problem.  Our 32-bit salt during the connection is a problem, of
course.

> remove it, not a reason to keep it.  Better code coverage testing of
> error paths is the answer to making sure that our error handling behaves
> properly.

What is the logic to removing md5 but keeping 'password'?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Tue, Mar  1, 2022 at 08:31:19AM -0500, Stephen Frost wrote:
> > > The last time I played with this area is the recent error handling
> > > improvement with cryptohashes but MD5 has actually helped here in
> > > detecting the problem as a patched OpenSSL would complain if trying to
> > > use MD5 as hash function when FIPS is enabled.
> >
> > Having to continue to deal with md5 as an algorithm when it's known to
> > be notably less secure and so much so that organizations essentially ban
> > its use for exactly what we're using it for, in fact, another reason to
>
> Really?  I thought it was publicly-visible MD5 hashes that were the
> biggest problem.  Our 32-bit salt during the connection is a problem, of
> course.

Neither are good.  Not sure that we really need to spend a lot of effort
trying to figure out which issue is the biggest problem.

> > remove it, not a reason to keep it.  Better code coverage testing of
> > error paths is the answer to making sure that our error handling behaves
> > properly.
>
> What is the logic to removing md5 but keeping 'password'?

I don't think we should keep 'password'.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Bruce Momjian (bruce@momjian.us) wrote:
>> What is the logic to removing md5 but keeping 'password'?

> I don't think we should keep 'password'.

I don't see much point in that unless we deprecate *all* the
auth methods that transmit a cleartext password.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Bruce Momjian (bruce@momjian.us) wrote:
> >> What is the logic to removing md5 but keeping 'password'?
>
> > I don't think we should keep 'password'.
>
> I don't see much point in that unless we deprecate *all* the
> auth methods that transmit a cleartext password.

I'm not sure that it's quite so simple.  Perhaps we should also drop
LDAP and I don't really think PAM was ever terribly good for us to have,
but at least PAM and RADIUS could possibly be used with OTP solutions
(and maybe LDAP?  Not sure, don't think I've seen that but perhaps..),
rendering sniffing of what's transmitted less valuable.  We don't
support that for 'password' itself or for 'md5' in any serious way
though.

We really should drop ident already though.

Thanks,

Stephen

On Wed, Mar  2, 2022 at 10:09:31AM -0500, Stephen Frost wrote:
> I'm not sure that it's quite so simple.  Perhaps we should also drop
> LDAP and I don't really think PAM was ever terribly good for us to have,
> but at least PAM and RADIUS could possibly be used with OTP solutions
> (and maybe LDAP?  Not sure, don't think I've seen that but perhaps..),
> rendering sniffing of what's transmitted less valuable.  We don't
> support that for 'password' itself or for 'md5' in any serious way
> though.

I thought all the plain-password methods were already using SSL
(hopefully with certificate authentication) and they were therefore
safe.  Why would we remove something like LDAP if that is what the site
is already using?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

On 02.03.22 15:16, Jonathan S. Katz wrote:
>> I find that a lot of people are still purposely using md5.  Removing 
>> it now or in a year would be quite a disruption.
> 
> What are the reasons they are still purposely using it? The ones I have 
> seen/heard are:
> 
> - Using an older driver
> - On a pre-v10 PG
> - Unaware of SCRAM

I'm not really sure, but it seems like they are content with what they 
have and don't want to bother with the new fancy stuff.

> What I'm proposing above is to start the process of deprecating it as an 
> auth method, which also allows to continue the education efforts to 
> upgrae. Does that make sense?

I'm not in favor of starting a process that will result in removal of 
the md5 method at this time.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Wed, Mar  2, 2022 at 10:09:31AM -0500, Stephen Frost wrote:
> > I'm not sure that it's quite so simple.  Perhaps we should also drop
> > LDAP and I don't really think PAM was ever terribly good for us to have,
> > but at least PAM and RADIUS could possibly be used with OTP solutions
> > (and maybe LDAP?  Not sure, don't think I've seen that but perhaps..),
> > rendering sniffing of what's transmitted less valuable.  We don't
> > support that for 'password' itself or for 'md5' in any serious way
> > though.
>
> I thought all the plain-password methods were already using SSL
> (hopefully with certificate authentication) and they were therefore
> safe.  Why would we remove something like LDAP if that is what the site
> is already using?

We don't require SSL to be used with them..?  Further, as already
discussed on this thread, SSL only helps with on-the-wire, doesn't
address the risk of a compromised server.  LDAP, in particular, is
terrible in this regard because it's a centralized password system,
meaning that one compromised server will lead to an attacker gaining
full access to the victim's account throughout the enterprise.

Thanks,

Stephen

Greetings,

* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
> On 02.03.22 15:16, Jonathan S. Katz wrote:
> >>I find that a lot of people are still purposely using md5.  Removing it
> >>now or in a year would be quite a disruption.
> >
> >What are the reasons they are still purposely using it? The ones I have
> >seen/heard are:
> >
> >- Using an older driver
> >- On a pre-v10 PG
> >- Unaware of SCRAM
>
> I'm not really sure, but it seems like they are content with what they have
> and don't want to bother with the new fancy stuff.

There were lots and lots of folks who were comfortable with
recovery.conf, yet we removed that without any qualms from one major
version to the next.  md5 will have had 5 years of overlap with scram.

> >What I'm proposing above is to start the process of deprecating it as an
> >auth method, which also allows to continue the education efforts to
> >upgrae. Does that make sense?
>
> I'm not in favor of starting a process that will result in removal of the
> md5 method at this time.

I am.

Thanks,

Stephen

On Wed, Mar 2, 2022 at 10:29 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Bruce Momjian (bruce@momjian.us) wrote:
> > On Wed, Mar  2, 2022 at 10:09:31AM -0500, Stephen Frost wrote:
> > > I'm not sure that it's quite so simple.  Perhaps we should also drop
> > > LDAP and I don't really think PAM was ever terribly good for us to have,
> > > but at least PAM and RADIUS could possibly be used with OTP solutions
> > > (and maybe LDAP?  Not sure, don't think I've seen that but perhaps..),
> > > rendering sniffing of what's transmitted less valuable.  We don't
> > > support that for 'password' itself or for 'md5' in any serious way
> > > though.
> >
> > I thought all the plain-password methods were already using SSL
> > (hopefully with certificate authentication) and they were therefore
> > safe.  Why would we remove something like LDAP if that is what the site
> > is already using?
>
> We don't require SSL to be used with them..?  Further, as already
> discussed on this thread, SSL only helps with on-the-wire, doesn't
> address the risk of a compromised server.  LDAP, in particular, is
> terrible in this regard because it's a centralized password system,
> meaning that one compromised server will lead to an attacker gaining
> full access to the victim's account throughout the enterprise.

I want to add support for the deprecation move, and I think/hope that
my multi-password patchset can help make the transition less painful.

I also want to throw out another existing issue with MD5, namely that
the salt as the role is fundamentally problematic, even outside of
trivial pass-the-hash attacks one could build a rainbow table today
that uses 'postgres' as the salt, and get admin credentials that can
be used for password stuffing attacks against other enterprise assets.
This means PG is actively enabling lateral movement through enterprise
systems if MD5 passwords are used.

On 3/2/22 10:30 AM, Stephen Frost wrote:
> Greetings,
> 
> * Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
>> On 02.03.22 15:16, Jonathan S. Katz wrote:
>>>> I find that a lot of people are still purposely using md5.  Removing it
>>>> now or in a year would be quite a disruption.
>>>
>>> What are the reasons they are still purposely using it? The ones I have
>>> seen/heard are:
>>>
>>> - Using an older driver
>>> - On a pre-v10 PG
>>> - Unaware of SCRAM
>>
>> I'm not really sure, but it seems like they are content with what they have
>> and don't want to bother with the new fancy stuff.

By that argument, we should have kept "password" (plain) as an 
authentication method.

The specific use-cases I've presented are all solvable issues. The 
biggest challenging with existing users is the upgrade process, which is 
why I'd rather we begin a deprecation process and see if there are any 
ways we can make the md5 => SCRAM transition easier.

> There were lots and lots of folks who were comfortable with
> recovery.conf, yet we removed that without any qualms from one major
> version to the next.  md5 will have had 5 years of overlap with scram.

I do agree with Stephen in principle here. I encountered upgrade 
challenges in this an challenge with updating automation to handle this 
change.

>>> What I'm proposing above is to start the process of deprecating it as an
>>> auth method, which also allows to continue the education efforts to
>>> upgrae. Does that make sense?
>>
>> I'm not in favor of starting a process that will result in removal of the
>> md5 method at this time.
> 
> I am.

+1 for starting this process. It may still take a few more years, but we 
should help our users to move away from an auth method with known issues.

Thanks,

Jonathan

On Wed, Mar  2, 2022 at 10:29:45AM -0500, Stephen Frost wrote:
> We don't require SSL to be used with them..?  Further, as already
> discussed on this thread, SSL only helps with on-the-wire, doesn't
> address the risk of a compromised server.  LDAP, in particular, is
> terrible in this regard because it's a centralized password system,
> meaning that one compromised server will lead to an attacker gaining
> full access to the victim's account throughout the enterprise.

Yes, but the site chose LDAP, and I don't think it is our place to tell
them what to use.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Wed, Mar  2, 2022 at 10:29:45AM -0500, Stephen Frost wrote:
> > We don't require SSL to be used with them..?  Further, as already
> > discussed on this thread, SSL only helps with on-the-wire, doesn't
> > address the risk of a compromised server.  LDAP, in particular, is
> > terrible in this regard because it's a centralized password system,
> > meaning that one compromised server will lead to an attacker gaining
> > full access to the victim's account throughout the enterprise.
>
> Yes, but the site chose LDAP, and I don't think it is our place to tell
> them what to use.

It's our decision what we want to support and maintain in the code base
and what we don't.  Folks often ask for things that we don't or won't
support and this isn't any different from that.  We also remove things
on a rather regular basis even when they're being used- generally
because we have something better, as we do here.  I disagree that an
argument of 'some people use it so we can't remove it' holds any weight
here.

Thanks,

Stephen

On Wed, Mar  2, 2022 at 10:54:27AM -0500, Stephen Frost wrote:
> It's our decision what we want to support and maintain in the code base
> and what we don't.  Folks often ask for things that we don't or won't
> support and this isn't any different from that.  We also remove things
> on a rather regular basis even when they're being used- generally
> because we have something better, as we do here.  I disagree that an
> argument of 'some people use it so we can't remove it' holds any weight
> here.

I disagree.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Greetings,

* Bruce Momjian (bruce@momjian.us) wrote:
> On Wed, Mar  2, 2022 at 10:54:27AM -0500, Stephen Frost wrote:
> > It's our decision what we want to support and maintain in the code base
> > and what we don't.  Folks often ask for things that we don't or won't
> > support and this isn't any different from that.  We also remove things
> > on a rather regular basis even when they're being used- generally
> > because we have something better, as we do here.  I disagree that an
> > argument of 'some people use it so we can't remove it' holds any weight
> > here.
>
> I disagree.

With... which?  We removed recovery.conf without any warning between
major releases, yet it was used by every single PG file-based backup and
restore solution out there and by every single organization that had
ever done a restore of PG since it was introduced in 8.0.  Passing
around cleartext passwords with the LDAP authentication method is
clearly bad from a security perspective and it's a bunch of code to
support that, along with it being quite complicated to configure and get
set up (arguably harder than Kerberos, if you want my 2c).  If you want
to say that it's valuable for us to continue to maintain that code
because it's good and useful and might even be the only option for some
people, fine, though I disagree, but I don't think my argument that we
shouldn't keep it just because *someone* is using it is any different
from our general project policy about features.

Thanks,

Stephen

Hi,

On 2022-03-02 09:32:26 +0100, Peter Eisentraut wrote:
> On 01.03.22 22:34, Andres Freund wrote:
> > The cases I've heard about are about centralizing auth across multiple cloud
> > services. Including secret management in some form. E.g. allowing an
> > application to auth to postgres, redis and having the secret provided by
> > infrastructure, rather than having to hardcode it in config or such.
> > 
> > I can't see application developers configuring kerberos and I don't think
> > LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
> > alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
> > stuff that's not portable across OSs. Radius is probably the most realistic,
> > but at least as implemented doesn't seem flexible enough (e.g. no access to
> > group memberships etc).
> > 
> > Nor does baking stuff like that in seem realistic to me, it'll presumably be
> > too cloud provider specific.
> 
> Let's gather some more information on this.  PostgreSQL should support the
> authentication that many people want to use out of the box.  I don't think
> it would be good to be at a point where all the built-in methods are
> outdated and if you want to use the good stuff you have to hunt for plugins.
> The number of different cloud APIs is effectively small.  I expect that
> there are a lot of similarities, like they probably all need support for
> http calls, they might need support for caching lookups, etc.  OIDC was
> mentioned elsewhere.  That's a standard.  Is that compatible with any cloud
> providers?  Would that be enough for many users?

I'm not opposed to putting something into the source tree eventually, if we
can figure out an overlapping set of capabilities that's useful enough. But I
don't see that as a reason to not make auth extensible? If anything, the
contrary - it's going to be much easier to figure out what this should look
like if it can be iteratively worked on with unmodified postgres.

Even if we were to put something in core, it seems that contrib/ would be a
better place than auth.c for something like it.

I think we should consider moving pam, ldap, radius to contrib
eventually. That way we'd not need to entirely remove them, but a "default"
postgres wouldn't have support for auth methods requiring plaintext secret
transmission.

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-02 09:32:26 +0100, Peter Eisentraut wrote:
> > On 01.03.22 22:34, Andres Freund wrote:
> > > The cases I've heard about are about centralizing auth across multiple cloud
> > > services. Including secret management in some form. E.g. allowing an
> > > application to auth to postgres, redis and having the secret provided by
> > > infrastructure, rather than having to hardcode it in config or such.
> > >
> > > I can't see application developers configuring kerberos and I don't think
> > > LDAP, PAM, Radius are a great answer either, due to the plaintext requirement
> > > alone? LDAP is pretty clearly dying technology, PAM is fragile complicated C
> > > stuff that's not portable across OSs. Radius is probably the most realistic,
> > > but at least as implemented doesn't seem flexible enough (e.g. no access to
> > > group memberships etc).
> > >
> > > Nor does baking stuff like that in seem realistic to me, it'll presumably be
> > > too cloud provider specific.
> >
> > Let's gather some more information on this.  PostgreSQL should support the
> > authentication that many people want to use out of the box.  I don't think
> > it would be good to be at a point where all the built-in methods are
> > outdated and if you want to use the good stuff you have to hunt for plugins.
> > The number of different cloud APIs is effectively small.  I expect that
> > there are a lot of similarities, like they probably all need support for
> > http calls, they might need support for caching lookups, etc.  OIDC was
> > mentioned elsewhere.  That's a standard.  Is that compatible with any cloud
> > providers?  Would that be enough for many users?
>
> I'm not opposed to putting something into the source tree eventually, if we
> can figure out an overlapping set of capabilities that's useful enough. But I
> don't see that as a reason to not make auth extensible? If anything, the
> contrary - it's going to be much easier to figure out what this should look
> like if it can be iteratively worked on with unmodified postgres.
>
> Even if we were to put something in core, it seems that contrib/ would be a
> better place than auth.c for something like it.
>
> I think we should consider moving pam, ldap, radius to contrib
> eventually. That way we'd not need to entirely remove them, but a "default"
> postgres wouldn't have support for auth methods requiring plaintext secret
> transmission.

Part of the point, for my part anyway, of dropping support for plaintext
transmission would be to remove support for that from libpq, otherwise a
compromised server could still potentially convince a client to provide
a plaintext password be sent to it.

I also just generally disagree with the idea that it makes sense for
these things to be in contrib.  We should be dropping them because
they're insecure- moving them to contrib doesn't change the issue that
we're distributing authentication solutions that send (either through an
encrypted tunnel, or not, neither is good) that pass plaintext passwords
around.

Thanks,

Stephen

Hi,

On 2022-03-02 15:26:32 -0500, Stephen Frost wrote:
> Part of the point, for my part anyway, of dropping support for plaintext
> transmission would be to remove support for that from libpq, otherwise a
> compromised server could still potentially convince a client to provide
> a plaintext password be sent to it.

IMO that's an argument for an opt-in option to permit plaintext, not an
argument for removal of the code alltogether. Even that will need a long
transition time, because it's effectively a form of an ABI break. Upgrading
libpq will suddenly cause applications to stop working. So adding an opt-out
option to disable plaintext is the next step...

I don't think it makes sense to discuss this topic as part of this thread
really. It seems wholly independent of making authentication pluggable.


> I also just generally disagree with the idea that it makes sense for
> these things to be in contrib.  We should be dropping them because
> they're insecure- moving them to contrib doesn't change the issue that
> we're distributing authentication solutions that send (either through an
> encrypted tunnel, or not, neither is good) that pass plaintext passwords
> around.

Shrug. I don't think it's a good idea to just leave people hanging without a
replacement. It's OK to make it a bit harder and require explicit
configuration, but dropping support for reasonable configurations IMO is
something we should be very hesitant doing.

Greetings,

Andres Freund

> On 2/25/22 12:39 PM, Tom Lane wrote:
>> Jeff Davis <pgsql@j-davis.com> writes:
>>> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
>>>> ... and, since we can't readily enforce that the client only sends
>>>> those cleartext passwords over suitably-encrypted connections, this
>>>> could easily be a net negative for security.  Not sure that I think
>>>> it's a good idea.
>> 
>>> I don't understand your point. Can't you just use "hostssl" rather
>>> than
>>> "host"?
>> My point is that sending cleartext passwords over the wire is an
>> insecure-by-definition protocol that we shouldn't be encouraging
>> more use of.
> 
> This is my general feeling as well. We just spent a bunch of effort
> adding, refining, and making SCRAM the default method. I think doing
> anything that would drive more use of sending plaintext passwords,
> even over TLS, is counter to that.

There's at least one use case to use plaintext passwords. Pgpool-II
accepts plaintext passwords from frontend (from frontend's point of
view, it looks as if the frontend speaks with PostgreSQL server which
requests the plaintext password authentication), then negotiates with
backends regarding authentication method they demand. Suppose we have
2 PostgreSQL clusters and they require md5 auth. They send different
password encryption salt and Pgpool-II deal with each server using the
salt and password. So Pgpool-II needs plaintext password.  Same thing
can be said to SCRAM-SHA-256 authentication because it's kind of
challenge/response based authentication.

Actually it is possible for Pgpool-II to not use plaintext passwords
reading from frontend. In this case passwords are stored in a file and
Pgpool-II reads passwords from the file. But this is annoying for
users because they have to sync the passwords stored in PostgreSQL
with the passwords stored in the file.

So, dropping plaintext password authentication support from libpq will
make it impossible for users to use the former method.

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

> Yes, really, it's a known-broken system which suffers from such an old
> and well known attack that it's been given a name: pass-the-hash.  As
> was discussed on this thread even, just the fact that it's not trivial
> to break on the wire doesn't make it not-broken, particularly when we
> use the username (which is rather commonly the same one used across
> multiple systems..) as the salt.  Worse, md5 isn't exactly the pinnacle

I am not a big fan of md5 auth but saying that md5 auth uses username
as the salt is oversimplified. The md5 hashed password shored in
pg_shadow is created as md5(password + username).  But the md5 hashed
password flying over wire is using a random salt like md5(md5(password
+ username) + random_salt).

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Wed, Mar 02, 2022 at 11:15:28AM -0500, Stephen Frost wrote:
> With... which?  We removed recovery.conf without any warning between
> major releases, yet it was used by every single PG file-based backup and
> restore solution out there and by every single organization that had
> ever done a restore of PG since it was introduced in 8.0.

There was much more to the changes around recovery.conf, with the
integration of its parameters as GUCs so it had a lot of benefits,
and that's why it has baked for 3~4 years as far as I recall.  There
is SCRAM as a replacement of MD5 already in core, but as Jonathan said
upthread the upgrade from an instance that still has MD5 hashes may
finish by being tricky for some users because this does not concern
only pg_upgrade (this could fail if it detects MD5 hashes in
pg_authid), and we don't really know how to solve the pg_dump bit, do
we?  And it seems to me that this is not a matter of just blocking the
load of MD5 hashes in the backend in the case of logical dumps.

> Passing
> around cleartext passwords with the LDAP authentication method is
> clearly bad from a security perspective and it's a bunch of code to
> support that, along with it being quite complicated to configure and get
> set up (arguably harder than Kerberos, if you want my 2c).  If you want
> to say that it's valuable for us to continue to maintain that code
> because it's good and useful and might even be the only option for some
> people, fine, though I disagree, but I don't think my argument that we
> shouldn't keep it just because *someone* is using it is any different
> from our general project policy about features.

MD5 is still used at auth method by many people, as is LDAP.  They
have their design flaws (backend LDAP, MD5 hashes in old backups), but
those code areas are pretty mature as well, so a simple removal could
hurt the user base of PG quite a lot IMO.
--
Michael

On 02.03.22 21:26, Stephen Frost wrote:
> Part of the point, for my part anyway, of dropping support for plaintext
> transmission would be to remove support for that from libpq, otherwise a
> compromised server could still potentially convince a client to provide
> a plaintext password be sent to it.

I think there should be a generalized feature for client-side selecting 
or filtering of authentication methods.  As long as there exists more 
than one method, there will be tradeoffs and users might want to avoid 
one or the other.  I don't think removing a method outright is the right 
solution for that.

On 02.03.22 16:45, Jonathan S. Katz wrote:
> By that argument, we should have kept "password" (plain) as an 
> authentication method.

For comparison, the time between adding md5 and removing password was 16 
years.  It has been 5 years since scram was added.

On 02.03.22 15:16, Jonathan S. Katz wrote:
> What are the reasons they are still purposely using it? The ones I have 
> seen/heard are:
> 
> - Using an older driver
> - On a pre-v10 PG
> - Unaware of SCRAM

Another reason is that SCRAM presents subtle operational issues in 
distributed systems.  As someone who is involved with products such as 
pgbouncer and bdr, I am aware that there are still unresolved problems 
and ongoing research in that area.  Maybe they can all be solved 
eventually, even if it is concluding "you can't do that anymore" in 
certain cases, but it's not all solved yet, and falling back to the 
best-method-before-this-one is a useful workaround.

I'm thinking there might be room for an authentication method between 
plain and scram that is less complicated and allows distributed systems 
to be set up more easily.  I don't know what that would be, but I don't 
think we should prohibit the consideration of "anything less than SCRAM".

I notice that a lot of internet services are promoting "application 
passwords" nowadays.  I don't know the implementation details of that, 
but it appears that the overall idea is to have instead of one 
high-value password have many frequently generated medium-value 
passwords.  We also have a recent proposal to store multiple passwords 
per user.  (Obviously that could apply to SCRAM and not-SCRAM equally.) 
That's the kind of direction I would like to explore.

On 02.03.22 21:49, samay sharma wrote:
> I think we are discussing two topics in this thread which in my opinion 
> are orthogonal.
> 
> (a) Should we make authentication methods pluggable by exposing these 
> hooks? - These will allow users to add plugins of their own to support 
> whatever auth method they like. One immediate use case (and what 
> prompted me to start looking at this) is Azure Active Directory 
> integration which is a common request from Azure customers. We could 
> also, over time, move some of our existing auth methods into extensions 
> if we don’t want to maintain them in core.

I don't think people are necessarily opposed to that.

At the moment, it is not possible to judge whether the hook interface 
you have chosen is appropriate.

I suggest you actually implement the Azure provider, then make the hook 
interface, and then show us both and we can see what to do with it.

One thing that has been requested, and I would support that, is that a 
plugged-in authentication method should look like a built-in one.  So 
for example it should be able to register a real name, instead of 
"custom".  I think a fair bit of refactoring work might be appropriate 
in order to make the authentication code more modular.

On Thu, Mar 3, 2022 at 4:45 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 02.03.22 16:45, Jonathan S. Katz wrote:
> > By that argument, we should have kept "password" (plain) as an
> > authentication method.
>
> For comparison, the time between adding md5 and removing password was 16
> years.  It has been 5 years since scram was added.

It's been 7 years since this thread:
https://www.postgresql.org/message-id/54DBCBCF.9000600@vmware.com

As Jonathan and Stephen and others have said, anyone who wishes to
continue using MD5 or other plaintext methods can keep doing that for
5 more years with a supported version of PG. There is no excuse to
leave well known, flawed mechanisms in PG16.

Greetings,

* Tatsuo Ishii (ishii@sraoss.co.jp) wrote:
> > On 2/25/22 12:39 PM, Tom Lane wrote:
> >> Jeff Davis <pgsql@j-davis.com> writes:
> >>> On Thu, 2022-02-24 at 20:47 -0500, Tom Lane wrote:
> >>>> ... and, since we can't readily enforce that the client only sends
> >>>> those cleartext passwords over suitably-encrypted connections, this
> >>>> could easily be a net negative for security.  Not sure that I think
> >>>> it's a good idea.
> >>
> >>> I don't understand your point. Can't you just use "hostssl" rather
> >>> than
> >>> "host"?
> >> My point is that sending cleartext passwords over the wire is an
> >> insecure-by-definition protocol that we shouldn't be encouraging
> >> more use of.
> >
> > This is my general feeling as well. We just spent a bunch of effort
> > adding, refining, and making SCRAM the default method. I think doing
> > anything that would drive more use of sending plaintext passwords,
> > even over TLS, is counter to that.
>
> There's at least one use case to use plaintext passwords. Pgpool-II
> accepts plaintext passwords from frontend (from frontend's point of
> view, it looks as if the frontend speaks with PostgreSQL server which
> requests the plaintext password authentication), then negotiates with
> backends regarding authentication method they demand. Suppose we have
> 2 PostgreSQL clusters and they require md5 auth. They send different
> password encryption salt and Pgpool-II deal with each server using the
> salt and password. So Pgpool-II needs plaintext password.  Same thing
> can be said to SCRAM-SHA-256 authentication because it's kind of
> challenge/response based authentication.

Passing around plaintext passwords isn't good, regardless of what's
doing it.  That some things do that today is a problem, not something
that should be held up as an example use-case that we should be
retaining support for.

> Actually it is possible for Pgpool-II to not use plaintext passwords
> reading from frontend. In this case passwords are stored in a file and
> Pgpool-II reads passwords from the file. But this is annoying for
> users because they have to sync the passwords stored in PostgreSQL
> with the passwords stored in the file.

Users authenticating to an application and then independently having
applications authenticate to the database server is actually rather
common.  I agree that proxying credentials is generally better and I'm
hoping to commit support for exactly that during this CF for GSSAPI (aka
Kerberos), where it's cleanly supported.  Proxying using plaintext
passwords isn't good though.

> So, dropping plaintext password authentication support from libpq will
> make it impossible for users to use the former method.

Yes, just like dropping support for md5 would make it impossible for
users to have their passwords be hashed with md5, which is an altogether
good thing considering how easy it is to brute-force md5 these days.

Thanks,

Stephen

On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:
> On 02.03.22 16:45, Jonathan S. Katz wrote:
> > By that argument, we should have kept "password" (plain) as an
> > authentication method.
> 
> For comparison, the time between adding md5 and removing password was 16
> years.  It has been 5 years since scram was added.

Uh, when did we remove "password".  I still see it mentioned in
pg_hba.conf.  Am I missing something?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

Greetings,

* Tatsuo Ishii (ishii@sraoss.co.jp) wrote:
> > Yes, really, it's a known-broken system which suffers from such an old
> > and well known attack that it's been given a name: pass-the-hash.  As
> > was discussed on this thread even, just the fact that it's not trivial
> > to break on the wire doesn't make it not-broken, particularly when we
> > use the username (which is rather commonly the same one used across
> > multiple systems..) as the salt.  Worse, md5 isn't exactly the pinnacle
>
> I am not a big fan of md5 auth but saying that md5 auth uses username
> as the salt is oversimplified. The md5 hashed password shored in
> pg_shadow is created as md5(password + username).  But the md5 hashed
> password flying over wire is using a random salt like md5(md5(password
> + username) + random_salt).

Err, no, it's not oversimplified at all- we do, in fact, as you say
above, use the username as the salt for what gets stored in pg_authid
(pg_shadow is just a view).  That's absolutely a problem because servers
can be compromised, backups can be compromised, and when it comes to PG
servers you don't even need to actually bother cracking the password
once you've gained access to an md5 value in pg_authid anyway.

Yes, we do use a challenge/response over the wire but that doesn't
absolve us of the fact that the hashes we store in pg_authid with the
md5 method is subject to pass-the-hash and brute-force attacks against
it.  If anything, the challenge/response over the wire is less useful
considering the common usage of TLS these days.

Thanks,

Stephen

On 3/3/22 12:23 PM, Bruce Momjian wrote:
> On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:
>> On 02.03.22 16:45, Jonathan S. Katz wrote:
>>> By that argument, we should have kept "password" (plain) as an
>>> authentication method.
>>
>> For comparison, the time between adding md5 and removing password was 16
>> years.  It has been 5 years since scram was added.
> 
> Uh, when did we remove "password".  I still see it mentioned in
> pg_hba.conf.  Am I missing something?

I may have explained this wrong. The protocol still supports "plain" but 
we removed the ability to store passwords in plaintext:

"Remove the ability to store unencrypted passwords on the server

"The password_encryption server parameter no longer supports off or 
plain. The UNENCRYPTED option is no longer supported in CREATE/ALTER 
USER ... PASSWORD. Similarly, the --unencrypted option has been removed 
from createuser. Unencrypted passwords migrated from older versions will 
be stored encrypted in this release. The default setting for 
password_encryption is still md5."

Jonathan

[1] https://www.postgresql.org/docs/release/10.0/

Greetings,

* Peter Eisentraut (peter.eisentraut@enterprisedb.com) wrote:
> On 02.03.22 21:49, samay sharma wrote:
> >I think we are discussing two topics in this thread which in my opinion
> >are orthogonal.
> >
> >(a) Should we make authentication methods pluggable by exposing these
> >hooks? - These will allow users to add plugins of their own to support
> >whatever auth method they like. One immediate use case (and what prompted
> >me to start looking at this) is Azure Active Directory integration which
> >is a common request from Azure customers. We could also, over time, move
> >some of our existing auth methods into extensions if we don’t want to
> >maintain them in core.
>
> I don't think people are necessarily opposed to that.

I'm not thrilled with it, at least.  It's not clear that just backend
hooks would be enough either- certainly a number of our existing
mechanisms require support in libpq and those are generally the ones
that are more secure too (GSSAPI, Certs), so how would that work with
something that's 'pluggable'?  Are we going to have libpq loading in
libraries too?

> At the moment, it is not possible to judge whether the hook interface you
> have chosen is appropriate.

Agreed.

> I suggest you actually implement the Azure provider, then make the hook
> interface, and then show us both and we can see what to do with it.

Better- implement a standard that's also supported by Azure and not
something proprietary that can't be evaluated or which hasn't been
reviewed by security experts.

Thanks,

Stephen

On Thu, Mar  3, 2022 at 12:38:32PM -0500, Jonathan Katz wrote:
> On 3/3/22 12:23 PM, Bruce Momjian wrote:
> > On Thu, Mar  3, 2022 at 10:45:42AM +0100, Peter Eisentraut wrote:
> > > On 02.03.22 16:45, Jonathan S. Katz wrote:
> > > > By that argument, we should have kept "password" (plain) as an
> > > > authentication method.
> > > 
> > > For comparison, the time between adding md5 and removing password was 16
> > > years.  It has been 5 years since scram was added.
> > 
> > Uh, when did we remove "password".  I still see it mentioned in
> > pg_hba.conf.  Am I missing something?
> 
> I may have explained this wrong. The protocol still supports "plain" but we
> removed the ability to store passwords in plaintext:
> 
> "Remove the ability to store unencrypted passwords on the server
> 
> "The password_encryption server parameter no longer supports off or plain.
> The UNENCRYPTED option is no longer supported in CREATE/ALTER USER ...
> PASSWORD. Similarly, the --unencrypted option has been removed from
> createuser. Unencrypted passwords migrated from older versions will be
> stored encrypted in this release. The default setting for
> password_encryption is still md5."

OK, that does make sense.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

>> So, dropping plaintext password authentication support from libpq will
>> make it impossible for users to use the former method.
> 
> Yes, just like dropping support for md5 would make it impossible for
> users to have their passwords be hashed with md5, which is an altogether
> good thing considering how easy it is to brute-force md5 these days.

I still don't understand why using plaintex password authentication
over SSL connection is considered insecure. Actually we have been
stating opposite in the manual:
https://www.postgresql.org/docs/14/auth-password.html

"If the connection is protected by SSL encryption then password can be
used safely, though."

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Thu, Mar 3, 2022 at 11:50 PM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>
> >> So, dropping plaintext password authentication support from libpq will
> >> make it impossible for users to use the former method.
> >
> > Yes, just like dropping support for md5 would make it impossible for
> > users to have their passwords be hashed with md5, which is an altogether
> > good thing considering how easy it is to brute-force md5 these days.
>
> I still don't understand why using plaintex password authentication
> over SSL connection is considered insecure. Actually we have been
> stating opposite in the manual:
> https://www.postgresql.org/docs/14/auth-password.html
>
> "If the connection is protected by SSL encryption then password can be
> used safely, though."

If you aren't doing client verification (i.e., cert in pg_hba) and are
not doing verify-full on the client side then a man-in-the-middle
attack on TLS is trivial, and the plaintext password will be
sniffable.

The documentation should be updated to say under no circumstances is this safe.

On Thu, 2022-03-03 at 11:12 +0100, Peter Eisentraut wrote:
> At the moment, it is not possible to judge whether the hook interface 
> you have chosen is appropriate.
> 
> I suggest you actually implement the Azure provider, then make the hook 
> interface, and then show us both and we can see what to do with it.

To add a data point here, I've rebased my OAUTHBEARER experiment [1] on
top of this patchset. (That should work with Azure's OIDC provider, and
if it doesn't, I'd like to know why.)

After the port, here are the changes I still needed to carry in the
backend to get the tests passing:

- I needed to add custom HBA options to configure the provider.
- I needed to declare usermap support so that my provider could
actually use check_usermap().
- I had to modify the SASL mechanism registration to allow a custom
maximum message length, but I think that's not the job of Samay's
proposal to fix; it's just a needed improvement to CheckSASLAuth().

Obviously, the libpq frontend still needs to understand how to speak
the new SASL mechanism. There are third-party SASL implementations that
are plugin-based, which could potentially ease the pain here, at the
expense of a major dependency and a very new distribution model.

--Jacob

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

>> I still don't understand why using plaintex password authentication
>> over SSL connection is considered insecure. Actually we have been
>> stating opposite in the manual:
>> https://www.postgresql.org/docs/14/auth-password.html
>>
>> "If the connection is protected by SSL encryption then password can be
>> used safely, though."
> 
> If you aren't doing client verification (i.e., cert in pg_hba) and are
> not doing verify-full on the client side then a man-in-the-middle
> attack on TLS is trivial, and the plaintext password will be
> sniffable.

So the plaintext password is safe if used with hostssl + verify-full
(server side) and sslmode = verify-full (client side), right?

Best reagards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Wed, 2022-03-02 at 10:54 -0500, Stephen Frost wrote:
> It's our decision what we want to support and maintain in the code
> base
> and what we don't.

That might be an argument in favor of custom auth methods, because we
could move built-in methods that we don't like into a contrib module
that implements it as a custom auth method.

Regards,
    Jeff Davis

On Fri, Mar 4, 2022 at 6:03 PM Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>
> >> I still don't understand why using plaintex password authentication
> >> over SSL connection is considered insecure. Actually we have been
> >> stating opposite in the manual:
> >> https://www.postgresql.org/docs/14/auth-password.html
> >>
> >> "If the connection is protected by SSL encryption then password can be
> >> used safely, though."
> >
> > If you aren't doing client verification (i.e., cert in pg_hba) and are
> > not doing verify-full on the client side then a man-in-the-middle
> > attack on TLS is trivial, and the plaintext password will be
> > sniffable.
>
> So the plaintext password is safe if used with hostssl + verify-full
> (server side) and sslmode = verify-full (client side), right?
>

That would be safe-in-transit so long as everything was configured
properly and all certificates were protected. Unfortunately PG doesn't
make this incredibly easy to implement, it allows only 1 client root
cert, the client side doesn't understand system certificate stores or
PKI, etc.

Further, if someone gains access to the password hashes there is still
a pass-the-hash vulnerability, though.

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Wed, 2022-03-02 at 10:54 -0500, Stephen Frost wrote:
> > It's our decision what we want to support and maintain in the code
> > base
> > and what we don't.
>
> That might be an argument in favor of custom auth methods, because we
> could move built-in methods that we don't like into a contrib module
> that implements it as a custom auth method.

Feel like I already answered this but just to be clear- I don't view
that as actually addressing the issue since we'd still be maintaining
and distributing insecure auth methods.

Thanks,

Stephen

On Tue, Mar 8, 2022 at 9:28 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Jeff Davis (pgsql@j-davis.com) wrote:
> > On Wed, 2022-03-02 at 10:54 -0500, Stephen Frost wrote:
> > > It's our decision what we want to support and maintain in the code
> > > base
> > > and what we don't.
> >
> > That might be an argument in favor of custom auth methods, because we
> > could move built-in methods that we don't like into a contrib module
> > that implements it as a custom auth method.
>
> Feel like I already answered this but just to be clear- I don't view
> that as actually addressing the issue since we'd still be maintaining
> and distributing insecure auth methods.

+1.

And contrib, in particular, is already a mix of very important, stable
ad useful things, and things that are just pure testing or examples
that nobody in their right mind should use. Putting something security
related there seems like a terrible idea on it's own, independent from
shipping things that are known insecure. (yes, I know sepgsql it
there. Which certainly doesn't help tell people if it's something that
could be relied on or not)

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

Greetings,

* samay sharma (smilingsamay@gmail.com) wrote:
> On Fri, Mar 4, 2022 at 11:15 AM Jacob Champion <pchampion@vmware.com> wrote:
> > On Thu, 2022-03-03 at 11:12 +0100, Peter Eisentraut wrote:
> > > At the moment, it is not possible to judge whether the hook interface
> > > you have chosen is appropriate.
> > >
> > > I suggest you actually implement the Azure provider, then make the hook
> > > interface, and then show us both and we can see what to do with it.
> >
> > To add a data point here, I've rebased my OAUTHBEARER experiment [1] on
> > top of this patchset. (That should work with Azure's OIDC provider, and
> > if it doesn't, I'd like to know why.)
>
> Firstly, thanks for doing this. It helps to have another data point and the
> feedback you provided is very valuable. I've looked to address it with the
> patchset attached to this email.
>
> This patch-set adds the following:
>
> * Allow multiple custom auth providers to be registered (Addressing
> feedback from Aleksander and Andrew)
> * Modify the test extension to use SCRAM to exchange secrets (Based on
> Andres's suggestion)
> * Add support for custom auth options to configure provider's behavior (by
> exposing a new hook) (Required by OAUTHBEARER)
> * Allow custom auth methods to use usermaps. (Required by OAUTHBEARER)

That we're having to extend this quite a bit to work for the proposed
OAUTH patches and that it still doesn't do anything for the client side
(note that the OAUTHBEARER patches are still patching libpq to add
support directly and surely will still be even with this latest patch
set ...) makes me seriously doubt that this is the right direction to be
going in.

> > After the port, here are the changes I still needed to carry in the
> > backend to get the tests passing:
> >
> > - I needed to add custom HBA options to configure the provider.
>
> Could you try to rebase your patch to use the options hook and let me know
> if it satisfies your requirements?
>
> Please let me know if there's any other feedback.

How about- if we just added OAUTH support directly into libpq and the
backend, would that work with Azure's OIDC provider?  If not, why not?
If it does, then what's the justification for trying to allow custom
backend-only authentication methods?

Thanks,

Stephen

On Wed, 2022-03-16 at 11:02 -0400, Stephen Frost wrote:
> That we're having to extend this quite a bit to work for the proposed
> OAUTH patches and that it still doesn't do anything for the client
> side
> (note that the OAUTHBEARER patches are still patching libpq to add
> support directly and surely will still be even with this latest patch
> set ...) makes me seriously doubt that this is the right direction to
> be
> going in.

I don't follow this concern. I assume you're referring to the last two
bullet points, which I repeat below:

* Add support for custom auth options to configure provider's
behavior: 

Even without OAUTH I think this would have been requested.
Configuration of some kind is going to be necessary. Without custom
options, I guess the provider would need to define its own config file?
Not the end of the world, but not ideal.

* Allow custom auth methods to use usermaps:

This is really just appending the "custom" method to a list of other
methods that are  allowed to specify a usermap in pg_hba.conf. Again, I
don't see anything specific to OAUTH, and this would likely have been
requested regardless.

> How about- if we just added OAUTH support directly into libpq and the
> backend, would that work with Azure's OIDC provider?  If not, why
> not?
> If it does, then what's the justification for trying to allow custom
> backend-only authentication methods?

I understand the point, but it also has a flip side: if custom auth
works, why do we need to block waiting for a complete core OAUTH
feature?

Authentication seems like a good candidate for allowing custom methods.
New ones are always coming along, being used in new ways, updating to
newer crypto, or falling out of favor. We've accumulated quite a few.

Regards,
    Jeff Davis

On Tue, 2022-03-15 at 12:27 -0700, samay sharma wrote:
> This patch-set adds the following:
> 
> * Allow multiple custom auth providers to be registered (Addressing
> feedback from Aleksander and Andrew)
> * Modify the test extension to use SCRAM to exchange secrets (Based
> on Andres's suggestion)
> * Add support for custom auth options to configure provider's
> behavior (by exposing a new hook) (Required by OAUTHBEARER)
> * Allow custom auth methods to use usermaps. (Required by
> OAUTHBEARER)

Review:

* I retract my previous comment that "...it seems to only be useful
with plaintext password authentication over an SSL connection"[0].
  - it can be used with SCRAM, as you've shown, which could be useful
for storing the credentials elsewhere
  - it can be used with one-time auth tokens, or short-lived auth
tokens, obtained from some other service
  - it can be used with SASL to negotiate with special clients that
support some other auth protocol (this is not shown, but having custom
auth would make it interesting to experiment in this area)

* There's a typo in the top comment for the test module, where you say
that it lives in "contrib", but it actually lives in
"src/test/modules".

* Don't specify ".so" in shared_preload_libraries (in the test module).

* Needs docs.

* I'm wondering whether provider initialization/lookup might have a
performance impact. Does it make sense to initialize the
CustomAuthProvider once when parsing the hba.conf, and then cache it in
the HbaLine or somewhere?

* When specifying the provider in pg_hba.conf, I first made a mistake
and specified it as "test_auth_provider" (which is the module name)
rather than "test" (which is the provider name). Maybe the message
could be slightly reworded in this case, like "no authentication
provider registered with name %s"? As is, the emphasis seems to be on
failing to load the library, which confused me at first.

* Should be a check like "if
(!process_shared_preload_libraries_in_progress) ereport(...)" in
RegisterAuthProvider.

* Could use some fuzz testing on the hba line parsing. For instance, I
was able to specify "provider" twice. And if I specify "allow" first,
then "provider" second, it fails (this is probably fine to require
provider to be first, but needs to be documented/commented somewhere).

* If you are intending for your custom provider to be open source, it
would be helpful to show the code (list, github link, whatever), even
if rough. That would ensure that it's really solving at least one real
use case and we aren't forgetting something.

* In general I like this patch. Trying to catch up on the rest of the
discussion.

Regards,
    Jeff Davis

[0] 
https://www.postgresql.org/message-id/bfc55e8045453659df26cd60035bfbb4b9530052.camel@j-davis.com

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Wed, 2022-03-16 at 11:02 -0400, Stephen Frost wrote:
> > That we're having to extend this quite a bit to work for the proposed
> > OAUTH patches and that it still doesn't do anything for the client
> > side
> > (note that the OAUTHBEARER patches are still patching libpq to add
> > support directly and surely will still be even with this latest patch
> > set ...) makes me seriously doubt that this is the right direction to
> > be
> > going in.
>
> I don't follow this concern. I assume you're referring to the last two
> bullet points, which I repeat below:
>
> * Add support for custom auth options to configure provider's
> behavior:
>
> Even without OAUTH I think this would have been requested.
> Configuration of some kind is going to be necessary. Without custom
> options, I guess the provider would need to define its own config file?
> Not the end of the world, but not ideal.

Yes, configuration of some kind will be needed and getting that
configuration correct is going to be essential to being able to have a
secure authentication system.

> > How about- if we just added OAUTH support directly into libpq and the
> > backend, would that work with Azure's OIDC provider?  If not, why
> > not?
> > If it does, then what's the justification for trying to allow custom
> > backend-only authentication methods?
>
> I understand the point, but it also has a flip side: if custom auth
> works, why do we need to block waiting for a complete core OAUTH
> feature?

First, let's be clear- we aren't actually talking about custom or
pluggable authentication here, at least when it comes to PG as a
project.  For it to actually be pluggable, it needs to be supported on
both the client side and the server side, not just the server side.

That this keeps getting swept under the carpet makes me feel like this
isn't actually an argument about the best way to move the PG project
forward but rather has another aim.  I don't think we should even
consider accepting a patch to make this something that works only on the
server side as, in such a case, we wouldn't even be able to have an
extension of our own that leverages it or tests it without bespoke code
for that purpose.  I certainly don't think we should add code to libpq
for some auth method that isn't even available in a default install of
PG, as would happen if we accepted both this patch and the patch to add
OAUTH support to libpq.  What exactly is the thinking on how this would
move forward?  We'd commit this custom support that requires an external
extension to actually work and then add hacked-in code to libpq in order
for folks to be able to use OAUTH?  What happens when, and not if,
something changes in that extension that requires a change on the client
side..?  Are we going to be dealing with CVEs for the libpq side of
this?

> Authentication seems like a good candidate for allowing custom methods.

It's not and this is clear from looking at history.  We have seen this
time and time again- PAM, SASL, RADIUS could be included, EAP, etc.
You'll note that we already support some of these, yet you'd like to now
add some set of hooks in addition to these pluggable options that
already exist because, presumably, they don't actually solve what you're
looking for.  That's actually rather typical when it comes to
authentication methods- everyone has this idea that it can be pluggable
or a few hooks to allow a custom method will work for the next great
thing, but that's not what actually happens.

> New ones are always coming along, being used in new ways, updating to
> newer crypto, or falling out of favor. We've accumulated quite a few.

I agree that we need to get rid of some of the ones that we've got, but
even so, at least the ones we have are maintained and updated to the
extent possible for us to fix issues with them.  The idea that
externally maintained custom auth methods would be taken care of in such
a way is quite far fetched in my view.

I also disagree that they're coming along so quickly and so fast that
it's actually difficult for us to include or support, we've covered
quite a few, after all, as you seem to agree with.

Thanks,

Stephen

Greetings,

* samay sharma (smilingsamay@gmail.com) wrote:
> On Wed, Mar 16, 2022 at 8:02 AM Stephen Frost <sfrost@snowman.net> wrote:
> > How about- if we just added OAUTH support directly into libpq and the
> > backend, would that work with Azure's OIDC provider?  If not, why not?
>
> Overall, Azure AD implements OIDC, so this could be doable. However, we'll
> have to explore further to see if the current implementation would satisfy
> all the requirements.

Great, would be useful to know what's discovered here and if OIDC
doesn't do what you're looking for, how you're intending to work on
improving it to make it so that it does, in an open and transparent
manner through the RFC process.

> > If it does, then what's the justification for trying to allow custom
> > backend-only authentication methods?
>
> The only goal of this patch wasn't to enable support for Azure AD. That's
> just one client. Users might have a need to add or change auth methods in
> the future and providing that extensibility so we don't need to have core
> changes for each one of them would be useful. I know there isn't alignment
> on this yet, but if we'd like to move certain auth methods out of core into
> extensions, then this might provide a good framework for that.

Hand-waving at this and saying that other people want it is not
justification for it- who else, exactly, has come along and asked for
it?  I also disagree that we actually want to move authentication
methods out of core- that was an argument brought up in this thread as
justification for why these hooks should exist but I don't see anyone
other than the folks that want the hooks actually saying that's
something they want.  There are some folks who agree that we should drop
support for certain authentication methods but that's not at all the
same thing (and no, moving them to contrib isn't somehow deprecating or
removing them from what we have to support or making it so that we don't
have to deal with them).

Thanks,

Stephen

On 16.03.22 21:27, samay sharma wrote:
> The only goal of this patch wasn't to enable support for Azure AD. 
> That's just one client. Users might have a need to add or change auth 
> methods in the future and providing that extensibility so we don't need 
> to have core changes for each one of them would be useful. I know there 
> isn't alignment on this yet, but if we'd like to move certain auth 
> methods out of core into extensions, then this might provide a good 
> framework for that.

Looking at the existing authentication methods

# METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
# "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".

how many of these could have been implemented using a plugin mechanism 
that was designed before the new method was considered?  Probably not 
many.  So I am fundamentally confused how this patch set can make such 
an ambitious claim.  Maybe the scope needs to be clarified first.  What 
kinds of authentication methods do you want to plug in?  What kinds of 
methods are out of scope?  What are examples of each one?

Hi,

On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> Looking at the existing authentication methods
> 
> # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> 
> how many of these could have been implemented using a plugin mechanism that
> was designed before the new method was considered?  Probably not many.

trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
possible. I think sspi is doable as well, but I don't know it well enough to
be confident. gss without transport encryption could have as well, I
think. Even scram-sha-256 looks possible, although that'd have been a good bit
harder.  Where do you see the problems?

Only stuff tying into transport encryption is clearly not doable via the
proposed API, but that's hardly the fault of an authentication API?

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > Looking at the existing authentication methods
> >
> > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> >
> > how many of these could have been implemented using a plugin mechanism that
> > was designed before the new method was considered?  Probably not many.
>
> trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> possible. I think sspi is doable as well, but I don't know it well enough to
> be confident. gss without transport encryption could have as well, I
> think. Even scram-sha-256 looks possible, although that'd have been a good bit
> harder.  Where do you see the problems?

I agree with Peter and don't feel like the above actually answered the
question in any fashion.  The question wasn't "which auth methods could
be implemented using these new set of hooks?" but rather- which could
have been implemented using a plugin mechanism that was designed
*before* the new method was considered?  The answer to that is
'basically none'.  There even existed a plugin mechanism (PAM) before
many of them, and they weren't able implemented using it.  That's
entirely the point- while this might be an interesting way to redesign
what we have now, should we feel that's useful to do (I don't and think
it would be an actively bad thing for the project to try and do...)
there's no reason to believe that it'll actually be useful for the
*next* auth method that comes along.

> Only stuff tying into transport encryption is clearly not doable via the
> proposed API, but that's hardly the fault of an authentication API?

This is absolutely the wrong way to look at it.  The authentication
methods that are coming along today are designed to tie into the
transport encryption because that's *needed* to avoid MITM attacks.  I'd
much rather we generally avoid including ones that don't support that
and we certainly shouldn't be trying to build a generic system which
explicitly doesn't support it.

Thanks,

Stephen

Hi,

On 2022-03-17 14:03:31 -0400, Stephen Frost wrote:
> * Andres Freund (andres@anarazel.de) wrote:
> > On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > > Looking at the existing authentication methods
> > > 
> > > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> > > 
> > > how many of these could have been implemented using a plugin mechanism that
> > > was designed before the new method was considered?  Probably not many.
> > 
> > trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> > possible. I think sspi is doable as well, but I don't know it well enough to
> > be confident. gss without transport encryption could have as well, I
> > think. Even scram-sha-256 looks possible, although that'd have been a good bit
> > harder.  Where do you see the problems?
> 
> I agree with Peter and don't feel like the above actually answered the
> question in any fashion.  The question wasn't "which auth methods could
> be implemented using these new set of hooks?" but rather- which could
> have been implemented using a plugin mechanism that was designed
> *before* the new method was considered?  The answer to that is
> 'basically none'.

I fail to see how you come to that conclusion.


> There even existed a plugin mechanism (PAM) before many of them, and they
> weren't able implemented using it.

That's nonsensical. PAM is a platform specific API to start with - so it
doesn't make sense to implement additional postgres authentication mechanism
with it. It also wasn't added to postgres as a base mechanism for extensible
authentication. And it's fundamentally restricted in the kind of secret
exchanges that can be implemented with it.


> That's entirely the point- while this might be an interesting way to
> redesign what we have now, should we feel that's useful to do (I don't and
> think it would be an actively bad thing for the project to try and do...)
> there's no reason to believe that it'll actually be useful for the *next*
> auth method that comes along.

What concrete limitation do you see in the API? It basically gives you the
same powers as writing something in auth.c directly. And due to AUTH_REQ_SASL*
we have fairly extensible ways of exchanging authentication data.


> > Only stuff tying into transport encryption is clearly not doable via the
> > proposed API, but that's hardly the fault of an authentication API?
> 
> This is absolutely the wrong way to look at it.  The authentication
> methods that are coming along today are designed to tie into the
> transport encryption because that's *needed* to avoid MITM attacks.  I'd
> much rather we generally avoid including ones that don't support that
> and we certainly shouldn't be trying to build a generic system which
> explicitly doesn't support it.

So you're saying that any authentication API needs to come together with a
runtime extendable secure transport mechanism? That feels like raising the bar
ridiculously into the sky. If we want to add more transport mechanisms - and
I'm not sure we do - that's a very sizable project on its own. And
independent.

Note that you *can* combine existing secure transport mechanisms with the
proposed API. You can use Port->{ssl,gss,peer_cn,...} to query information and
do further openssl etc calls.

Greetings,

Andres Freund

On Thu, 2022-03-17 at 14:03 -0400, Stephen Frost wrote:
> * Andres Freund (andres@anarazel.de) wrote:
> > 
> > trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> > possible. I think sspi is doable as well, but I don't know it well enough to
> > be confident. gss without transport encryption could have as well, I
> > think. Even scram-sha-256 looks possible, although that'd have been a good bit
> > harder.  Where do you see the problems?
> 
> I agree with Peter and don't feel like the above actually answered the
> question in any fashion.  The question wasn't "which auth methods could
> be implemented using these new set of hooks?" but rather- which could
> have been implemented using a plugin mechanism that was designed
> *before* the new method was considered?  The answer to that is
> 'basically none'.  There even existed a plugin mechanism (PAM) before
> many of them, and they weren't able implemented using it.  That's
> entirely the point- while this might be an interesting way to redesign
> what we have now, should we feel that's useful to do (I don't and think
> it would be an actively bad thing for the project to try and do...)
> there's no reason to believe that it'll actually be useful for the
> *next* auth method that comes along.

We already have the beginnings of a SASL framework; part of the work we
did with the OAUTHBEARER experiment was to generalize more pieces of
it. More generalization can still be done. Samay has shown that one
SASL mechanism can be ported, I'm in the process of porting another,
and we already know that the system can handle a non-SASL (password-
based) mechanism.

So that's a pretty good start.

> > Only stuff tying into transport encryption is clearly not doable via the
> > proposed API, but that's hardly the fault of an authentication API?
> 
> This is absolutely the wrong way to look at it.  The authentication
> methods that are coming along today are designed to tie into the
> transport encryption because that's *needed* to avoid MITM attacks.  I'd
> much rather we generally avoid including ones that don't support that
> and we certainly shouldn't be trying to build a generic system which
> explicitly doesn't support it.

Many SASL mechanisms support channel binding, which the server already
has helpers for, and which SCRAM is already using. At least one SASL
mechanism (GSSAPI) supports the negotiation of transport
confidentiality.

These are answerable questions. I don't think it's fair to ask Samay to
have perfect answers while simultaneously implementing a bunch of
different requests in code and going through the brainstorming process
alongside us all. (It would be fair if Samay were claiming the design
were finished, but that's not what I've seen so far.)

--Jacob

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-17 14:03:31 -0400, Stephen Frost wrote:
> > * Andres Freund (andres@anarazel.de) wrote:
> > > On 2022-03-17 12:10:51 +0100, Peter Eisentraut wrote:
> > > > Looking at the existing authentication methods
> > > >
> > > > # METHOD can be "trust", "reject", "md5", "password", "scram-sha-256",
> > > > # "gss", "sspi", "ident", "peer", "pam", "ldap", "radius" or "cert".
> > > >
> > > > how many of these could have been implemented using a plugin mechanism that
> > > > was designed before the new method was considered?  Probably not many.
> > >
> > > trust, reject, md5, password, ident, peer, pam, ldap, radius look trivially
> > > possible. I think sspi is doable as well, but I don't know it well enough to
> > > be confident. gss without transport encryption could have as well, I
> > > think. Even scram-sha-256 looks possible, although that'd have been a good bit
> > > harder.  Where do you see the problems?
> >
> > I agree with Peter and don't feel like the above actually answered the
> > question in any fashion.  The question wasn't "which auth methods could
> > be implemented using these new set of hooks?" but rather- which could
> > have been implemented using a plugin mechanism that was designed
> > *before* the new method was considered?  The answer to that is
> > 'basically none'.
>
> I fail to see how you come to that conclusion.

...

> > There even existed a plugin mechanism (PAM) before many of them, and they
> > weren't able implemented using it.
>
> That's nonsensical. PAM is a platform specific API to start with - so it
> doesn't make sense to implement additional postgres authentication mechanism
> with it. It also wasn't added to postgres as a base mechanism for extensible
> authentication. And it's fundamentally restricted in the kind of secret
> exchanges that can be implemented with it.

Exactly because of this.  The folks who came up with PAM didn't forsee
the direction that authentication methods were going to go in and I
don't see any particular reason why we're smarter than they were.

> > That's entirely the point- while this might be an interesting way to
> > redesign what we have now, should we feel that's useful to do (I don't and
> > think it would be an actively bad thing for the project to try and do...)
> > there's no reason to believe that it'll actually be useful for the *next*
> > auth method that comes along.
>
> What concrete limitation do you see in the API? It basically gives you the
> same powers as writing something in auth.c directly. And due to AUTH_REQ_SASL*
> we have fairly extensible ways of exchanging authentication data.

I'm not one to predict what the next authentication method to come down
the road is but I doubt that an API we come up with today will actually
work to perfectly implement it.  GSSAPI was supposed to be an extensible
authentication method too.

Also- how is this going to work client-side in libpq?

> > > Only stuff tying into transport encryption is clearly not doable via the
> > > proposed API, but that's hardly the fault of an authentication API?
> >
> > This is absolutely the wrong way to look at it.  The authentication
> > methods that are coming along today are designed to tie into the
> > transport encryption because that's *needed* to avoid MITM attacks.  I'd
> > much rather we generally avoid including ones that don't support that
> > and we certainly shouldn't be trying to build a generic system which
> > explicitly doesn't support it.
>
> So you're saying that any authentication API needs to come together with a
> runtime extendable secure transport mechanism? That feels like raising the bar
> ridiculously into the sky. If we want to add more transport mechanisms - and
> I'm not sure we do - that's a very sizable project on its own. And
> independent.

No, that's not what I'm saying.  Authentication mechanisms should have a
way to tie themselves to the secure transport layer is what I was
getting at, which you seemed to be saying wasn't possible with this API.

However, it's certainly a relevant point that something like encrypted
GSSAPI still wouldn't be able to be implemented with this.  I don't know
that it's required for a new auth method to have that, but not being
able to do it with the proposed API is another point against this
approach.  That is, even the existing methods that we've got today
wouldn't all be able to work with this.

> Note that you *can* combine existing secure transport mechanisms with the
> proposed API. You can use Port->{ssl,gss,peer_cn,...} to query information and
> do further openssl etc calls.

If it's possible then that's good, though I do have concerns about how
this plays into support for different transport layers or even libraries
once we get support for an alternative to openssl.

Thanks,

Stephen

On 2022-03-16 18:50:23 -0400, Stephen Frost wrote:
> First, let's be clear- we aren't actually talking about custom or
> pluggable authentication here, at least when it comes to PG as a
> project.  For it to actually be pluggable, it needs to be supported on
> both the client side and the server side, not just the server side.
> 
> That this keeps getting swept under the carpet makes me feel like this
> isn't actually an argument about the best way to move the PG project
> forward but rather has another aim.

This is insulting and unjustified. IMO completely inappropriate for the list /
community. I've also brought this up privately, but I thought it important to
state so publically.

Hi,

On 2022-03-17 22:13:27 -0400, Stephen Frost wrote:
> This isn’t the first time I asked about this on this thread, yet the
> question about why this is only being discussed as backend changes, and
> with the only goal seeming to be to have a backend loadable module, without
> anything on the client side for something that’s clearly both a server and
> client side concern, seems to just be ignored, which make me feel like my
> comments and the concerns that I raise aren’t being appreciated.

It's imo a separate project to make the client side extensible. There's plenty
of authentication methods that don't need any further client side support than
the existing SASL (or password if OK for some use case) messages, which most
clients (including libpq) already know.

Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit (unless
you have server side access to clear text passwords, but brrr). But there's
plenty that can be done with that. Proxying auth via a central postgres server
with the secrets, sharing secrets with other data stores also understanding
SCRAM-SHA-256, ...

There definitely *also* are important authentication methods that can't be
implemented without further client side support. Some of those could plausibly
be tackled on the protocol / libpq level in a way that they could be used by
multiple authentication methods. Other authentication methods definitely need
dedicated code in the client (although the protocol likely can be fairly
generic).

Given that there's benefit from the server side extensibility alone, I don't
see much benefit in tying the server side extensibility to the client side
extensibility.

Greetings,

Andres Freund

Hi,

On 2022-03-18 00:03:37 -0400, Stephen Frost wrote:
> On Thu, Mar 17, 2022 at 23:25 Andres Freund <andres@anarazel.de> wrote:
> > It's imo a separate project to make the client side extensible. There's
> > plenty
> > of authentication methods that don't need any further client side support
> > than
> > the existing SASL (or password if OK for some use case) messages, which
> > most
> > clients (including libpq) already know.
> >
> > Of course the fact that libpq "only" speaks SCRAM-SHA-256 is a limit
> > (unless
> > you have server side access to clear text passwords, but brrr). But there's
> > plenty that can be done with that. Proxying auth via a central postgres
> > server
> > with the secrets, sharing secrets with other data stores also understanding
> > SCRAM-SHA-256, ...
> >
> > There definitely *also* are important authentication methods that can't be
> > implemented without further client side support. Some of those could
> > plausibly
> > be tackled on the protocol / libpq level in a way that they could be used
> > by
> > multiple authentication methods. Other authentication methods definitely
> > need
> > dedicated code in the client (although the protocol likely can be fairly
> > generic).
> >
> > Given that there's benefit from the server side extensibility alone, I
> > don't
> > see much benefit in tying the server side extensibility to the client side
> > extensibility.
> 
> 
> How are we going to reasonably test this then?

The test module in the patch is a good starting point.


> I also don’t think that I agree that it’s acceptable to only have the
> ability to extend the authentication on the server side as that implies a
> whole bunch of client-side work that goes above and beyond just
> implementing an authentication system if it’s not possible to leverage
> libpq (which nearly all languages out there use..).  Not addressing that
> side of it also makes me concerned that whatever we do here won’t be
> right/enough/etc.

You're skipping over my point of everything that can be made to use
SASL-SCRAM-256 just working with the existing libpq? Again?

If you want to argue that somehow that communicating via SASL-SCRAM-256 from a
plugin is not a sufficient use-case, or that the API should be shaped more
specifically to just use SASL-SCRAM-256, fine. Argue it. Otherwise I don't
know what to do except ignore you.

Greetings,

Andres Freund

Hi,

On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > I also don’t think that I agree that it’s acceptable to only have the
> > > ability to extend the authentication on the server side as that implies a
> > > whole bunch of client-side work that goes above and beyond just
> > > implementing an authentication system if it’s not possible to leverage
> > > libpq (which nearly all languages out there use..).  Not addressing that
> > > side of it also makes me concerned that whatever we do here won’t be
> > > right/enough/etc.
> >
> > You're skipping over my point of everything that can be made to use
> > SASL-SCRAM-256 just working with the existing libpq? Again?

> The plan is to make scram pluggable on the client side?  That isn’t what’s
> been proposed here that I’ve seen.

Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
implementing an authentication method that wants to store secrets outside of
postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
don't need to make scram pluggable client-side. From the perspective of the
client it'd look *exactly* like the normal auth flow with the server.

What's proposed here is not really about adding new authentication protocols
between FE/BE (although some *limited* forms of that might be possible). It's
primarily about using the existing FE/BE authentication exchanges
(AUTH_REQ_{PASSWORD,SASL*,...}) to validate against something other than the
builtin pg_authid.rolpassword.  Because drivers already know those
authentication exchanges, it doesn't need to learn new tricks.

As I said in
https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
when describing the above, that's not enough to implement every desireable
authentication method - but there's real use-cases where using the existing
SASL-SCRAM-256 is sufficient.


> I also wasn’t aware that we felt that it’s acceptable to just ignore other
> committers.

I'm not talking about going ahead and committing. Just not continuing
discussing this topci and spending my time more fruitfully on something else.

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > > I also don’t think that I agree that it’s acceptable to only have the
> > > > ability to extend the authentication on the server side as that implies a
> > > > whole bunch of client-side work that goes above and beyond just
> > > > implementing an authentication system if it’s not possible to leverage
> > > > libpq (which nearly all languages out there use..).  Not addressing that
> > > > side of it also makes me concerned that whatever we do here won’t be
> > > > right/enough/etc.
> > >
> > > You're skipping over my point of everything that can be made to use
> > > SASL-SCRAM-256 just working with the existing libpq? Again?
>
> > The plan is to make scram pluggable on the client side?  That isn’t what’s
> > been proposed here that I’ve seen.
>
> Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
> implementing an authentication method that wants to store secrets outside of
> postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
> don't need to make scram pluggable client-side. From the perspective of the
> client it'd look *exactly* like the normal auth flow with the server.

Then the only way to get support for something like, say, OAUTH, is to
modify the core code.

That strikes me as entirely defeating the point of having this be
extensible, since you still have to modify the core code to get support
for it.

> What's proposed here is not really about adding new authentication protocols
> between FE/BE (although some *limited* forms of that might be possible). It's
> primarily about using the existing FE/BE authentication exchanges
> (AUTH_REQ_{PASSWORD,SASL*,...}) to validate against something other than the
> builtin pg_authid.rolpassword.  Because drivers already know those
> authentication exchanges, it doesn't need to learn new tricks.

The OAUTH patch was specifically changed to try and use this and yet it
still had to modify the core code on the libpq side (at least, maybe
other things or maybe not depending on later changes to this patch).
I don't get why such an exercise would have been done if the goal is to
just allow what's in rolpassword to be pulled from somewhere else or why
we would be talking about different auth methods in the first place if
that's the goal here.  That the patch was also brought up in the context
of wanting to add support for another auth method also doesn't seem to
support the argument that this is just about changing where the value in
rolpassword comes from.

> As I said in
> https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
> when describing the above, that's not enough to implement every desireable
> authentication method - but there's real use-cases where using the existing
> SASL-SCRAM-256 is sufficient.

That this apparently isn't actually about adding new authentication
protocols or methods sure strikes me as odd when folks were adjusting
proposed patches specificially to use this for new authentication
methods and the subject is "Proposal: Support custom authentication
methods using hooks".  If what's happening is still actually SCRAM then
I also don't get why we would change pg_hba.conf to say 'custom' instead
of scram-sha-256 w/ some option to say how to go get the actual token,
or why there isn't anything done to deal with passwords being changed.
Basically, I don't agree with more-or-less anything that the patch is
doing if that's the goal.  If the goal is to actually change where the
token in rolpassword comes from for existing authentication methods and
specifically isn't to actually try to use this to support new
authenitcation methods, then I'd suggest laying it out that way and
having it be an option for scram-sha-256 or whatever other methods
(though that seems like the only one that should really be changed in
this way, if you want my 2c, as the rest that would work this way are
basically broken, that being md5) and then make sure to have a way to
handle password changes too.

> > I also wasn’t aware that we felt that it’s acceptable to just ignore other
> > committers.
>
> I'm not talking about going ahead and committing. Just not continuing
> discussing this topci and spending my time more fruitfully on something else.

I'm still a -1 on this patch.  Maybe there's a way to get a shared
storage for rolpassword and maybe that could even be extensible if we
want it to be (though I'd be more inclined to just build it in...) and
if that's actually the goal but this doesn't seem like the right
approach to be taking.  I also don't think that it's a good idea to have
a synchronous call-out during authentication as that can get really
painful and if you're cacheing it to deal with the risks of that, well,
seems like you'd be better off just using rolpassword (and perhaps with
Joshua's patches to allow more than one of those to exist at a time).

Thanks,

Stephen

Hi,

On 2022-03-18 15:23:21 -0400, Stephen Frost wrote:
> * Andres Freund (andres@anarazel.de) wrote:
> > On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > > > I also don’t think that I agree that it’s acceptable to only have the
> > > > > ability to extend the authentication on the server side as that implies a
> > > > > whole bunch of client-side work that goes above and beyond just
> > > > > implementing an authentication system if it’s not possible to leverage
> > > > > libpq (which nearly all languages out there use..).  Not addressing that
> > > > > side of it also makes me concerned that whatever we do here won’t be
> > > > > right/enough/etc.
> > > >
> > > > You're skipping over my point of everything that can be made to use
> > > > SASL-SCRAM-256 just working with the existing libpq? Again?
> > 
> > > The plan is to make scram pluggable on the client side?  That isn’t what’s
> > > been proposed here that I’ve seen.
> > 
> > Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
> > implementing an authentication method that wants to store secrets outside of
> > postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
> > don't need to make scram pluggable client-side. From the perspective of the
> > client it'd look *exactly* like the normal auth flow with the server.
> 
> Then the only way to get support for something like, say, OAUTH, is to
> modify the core code.

It wasn't the goal of the patch to add oauth support. I think it's a good sign
that the patch allowed Jacob to move the server side code in an extension, but
it's a benefit not a requirement.

I think we might be able to build ontop of this to make things even more
extensible and it's worth having the discussion whether the proposal can be
made more extensible with a reasonable amount of complexity. But that's
different from faulting the patch for not achieving something it didn't set
out to do.


> That strikes me as entirely defeating the point of having this be
> extensible, since you still have to modify the core code to get support
> for it.

Since it wasn't the goal to add oauth...


> I don't get why such an exercise would have been done if the goal is to
> just allow what's in rolpassword to be pulled from somewhere else or why
> we would be talking about different auth methods in the first place if
> that's the goal here.

It's called that way because auth.c calling it that. See

void
ClientAuthentication(Port *port)
...
    switch (port->hba->auth_method)

even though the different auth_methods use a smaller number of ways of
exchanges of secrets than we have "auth methods". So it doesn't at all seem
insane to name something allowing different authentication methods in the
sense of auth.c "custom authentication methods".



> That the patch was also brought up in the context of wanting to add support
> for another auth method also doesn't seem to support the argument that this
> is just about changing where the value in rolpassword comes from.

My point isn't that it's *just* the changing the value of rollpassword, but
that the authentication exchange with the client just uses the existing secret
exchanges, because they're sufficient for plenty use cases.


> > As I said in
> > https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
> > when describing the above, that's not enough to implement every desireable
> > authentication method - but there's real use-cases where using the existing
> > SASL-SCRAM-256 is sufficient.
> 
> That this apparently isn't actually about adding new authentication
> protocols or methods sure strikes me as odd when folks were adjusting
> proposed patches specificially to use this for new authentication
> methods and the subject is "Proposal: Support custom authentication
> methods using hooks"

See above. Note also it was called "Support custom authentication methods" not
"Support custom authentication protocols".


> If what's happening is still actually SCRAM then
> I also don't get why we would change pg_hba.conf to say 'custom' instead
> of scram-sha-256 w/ some option to say how to go get the actual token,
> or why there isn't anything done to deal with passwords being changed.

Normally make extension APIs reasonably generic. If you want to argue against
that here, I think that's a debate worth having (as I explicitly said
upthread!), but we should be having it about that then.

It does seem good to me that the proposed API could implement stuff like peer,
bsd, PAM, etc without a problem.  And there's plenty other things that don't
require different authentication protocols on the pg protocol level - using os
specific extensions to safely get the remote user of tcp connections would be
cool for example.


> Basically, I don't agree with more-or-less anything that the patch is
> doing if that's the goal.  If the goal is to actually change where the
> token in rolpassword comes from for existing authentication methods and
> specifically isn't to actually try to use this to support new
> authenitcation methods

s/methods/protocols/


>, then I'd suggest laying it out that way and
> having it be an option for scram-sha-256 or whatever other methods
> (though that seems like the only one that should really be changed in
> this way, if you want my 2c, as the rest that would work this way are
> basically broken, that being md5) and then make sure to have a way to
> handle password changes too.

I think that'd make it less useful, but still useful. E.g. doing different
external <-> internal username translation than what's built in.


> I'm still a -1 on this patch.

Fair enough, as long as you're doing it on what the patch actually tries to do
rather than something different.


> Maybe there's a way to get a shared
> storage for rolpassword and maybe that could even be extensible if we
> want it to be (though I'd be more inclined to just build it in...)

How would you add external storage for secrets in a builtin way?


> I also don't think that it's a good idea to have
> a synchronous call-out during authentication as that can get really
> painful

There's tradeoffs, for sure. But normally we don't prevent people from doing
stupid stuff with our extensibility (if anything we try to keep some debatable
stuff out of core by allowing it to be done in extensions).


> and if you're cacheing it to deal with the risks of that, well,
> seems like you'd be better off just using rolpassword (and perhaps with
> Joshua's patches to allow more than one of those to exist at a time).

That imo solves a separate problem.

Greetings,

Andres Freund

On 15.03.22 20:27, samay sharma wrote:
> This patch-set adds the following:
> 
> * Allow multiple custom auth providers to be registered (Addressing 
> feedback from Aleksander and Andrew)
> * Modify the test extension to use SCRAM to exchange secrets (Based on 
> Andres's suggestion)
> * Add support for custom auth options to configure provider's behavior 
> (by exposing a new hook) (Required by OAUTHBEARER)
> * Allow custom auth methods to use usermaps. (Required by OAUTHBEARER)

Some feedback on this specific patch set:

Custom authentication methods should be able to register their own name 
other than "custom".  You ought to refactor things so that existing 
methods such as ldap and pam go through your extension interface.  So 
the whole thing should be more like a lookup table or list with some 
built-in entries that modules can dynamically add on to.

Then you also don't need a test module, since the existing 
authentication methods would already test the interfaces.

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-03-18 15:23:21 -0400, Stephen Frost wrote:
> > * Andres Freund (andres@anarazel.de) wrote:
> > > On 2022-03-18 00:45:49 -0400, Stephen Frost wrote:
> > > > > I also don’t think that I agree that it’s acceptable to only have the
> > > > > > ability to extend the authentication on the server side as that implies a
> > > > > > whole bunch of client-side work that goes above and beyond just
> > > > > > implementing an authentication system if it’s not possible to leverage
> > > > > > libpq (which nearly all languages out there use..).  Not addressing that
> > > > > > side of it also makes me concerned that whatever we do here won’t be
> > > > > > right/enough/etc.
> > > > >
> > > > > You're skipping over my point of everything that can be made to use
> > > > > SASL-SCRAM-256 just working with the existing libpq? Again?
> > >
> > > > The plan is to make scram pluggable on the client side?  That isn’t what’s
> > > > been proposed here that I’ve seen.
> > >
> > > Libpq and many other drivers already speaks SASL-SCRAM-256. If you're
> > > implementing an authentication method that wants to store secrets outside of
> > > postgres (centrally in another postgres instance, ldap, mysql or whatnot) you
> > > don't need to make scram pluggable client-side. From the perspective of the
> > > client it'd look *exactly* like the normal auth flow with the server.
> >
> > Then the only way to get support for something like, say, OAUTH, is to
> > modify the core code.
>
> It wasn't the goal of the patch to add oauth support. I think it's a good sign
> that the patch allowed Jacob to move the server side code in an extension, but
> it's a benefit not a requirement.

I disagree that it's actually at all useful when considering this
change, specifically because it doesn't move the actual goalposts at
all when it comes to adding support for new authentication methods for
PG overall as the core code still has to be modified through the regular
process.  I get that the idea of this patch isn't to add oauth, but I
don't think it's sensible to claim that the changes to the oauth patch
to use these hooks on just the server side while still having a bunch of
code in libpq for oauth makes this set of hooks make sense.  I
definitely don't think we should ever even consider adding support for
something on the libpq side which require a 'custom' auth module on the
server side that isn't included in core.  Putting it into contrib would
just ultimately make it really painful for users to deal with without
any benefit that I can see either as we'd still have to maintain it and
update it through the regular PG process.  Trying hard to give the
benefit of the doubt here, maybe you could argue that by having the
module in contrib and therefore not loaded unless requested that the
system might be overall 'more secure', but given that we already require
admins to specify what the auth method is with pg_hba and that hasn't
been a source of a lot of CVEs around somehow getting the wrong code in
there, I just don't see it as a sensible justfication.

> I think we might be able to build ontop of this to make things even more
> extensible and it's worth having the discussion whether the proposal can be
> made more extensible with a reasonable amount of complexity. But that's
> different from faulting the patch for not achieving something it didn't set
> out to do.

That it wasn't clear just what the idea of this patch was strikes me as
another point against it, really.  The thread talked about custom
authentication methods and the modification to pg_hba clearly made it
out to be a top-level alternative to SCRAM and the other methods even
though it sounds like that wasn't the intent, or maybe was but was only
part of it (but then, what's the other part..?  That's still unclear to
me even after reading these latest emails..).

> > That strikes me as entirely defeating the point of having this be
> > extensible, since you still have to modify the core code to get support
> > for it.
>
> Since it wasn't the goal to add oauth...

But.. a patch was specifically proposed on this thread to use this to
add oauth, with encouragment from the folks working on this patch, and
that was then used, even just above, as a reason why this was a useful
change to make, but I don't see what about it makes it such.

> > I don't get why such an exercise would have been done if the goal is to
> > just allow what's in rolpassword to be pulled from somewhere else or why
> > we would be talking about different auth methods in the first place if
> > that's the goal here.
>
> It's called that way because auth.c calling it that. See
>
> void
> ClientAuthentication(Port *port)
> ...
>     switch (port->hba->auth_method)
>
> even though the different auth_methods use a smaller number of ways of
> exchanges of secrets than we have "auth methods". So it doesn't at all seem
> insane to name something allowing different authentication methods in the
> sense of auth.c "custom authentication methods".

The thing is though that the ones that do are the older ones that
generally aren't all that secure..  SCRAM is alright, RADIUS isn't
great unless it's a OTP kind of thing (and even then it's not great),
but md5, password, pam (tho it doesn't even actually use sendAuthRequest
and recv_password_packet), bsd, ldap, etc aren't.  The more secure
methods are the ones like gssapi, sspi, certificate and the layered ones
on top of those, which aren't that simple.  This is also the direction
that things are going in and why I dislike the idea of trying to extend
it in this way with the 'simple' exchange.  I'd also say that if you
really want a 'simple' exchange with a centralized service, you could
already just use RADIUS.

Also ... even md5 and SCRAM aren't actually just a simple exchange
as there's computation and back-and-forth between the client and the
server that's specific to those.  That there's a need for client-side
code that's specific to the auth method which goes to my point above
that this isn't just a server-side thing.

> > That the patch was also brought up in the context of wanting to add support
> > for another auth method also doesn't seem to support the argument that this
> > is just about changing where the value in rolpassword comes from.
>
> My point isn't that it's *just* the changing the value of rollpassword, but
> that the authentication exchange with the client just uses the existing secret
> exchanges, because they're sufficient for plenty use cases.

I'm really not following what this is saying.  If the client-side is
doing SCRAM but the server-side is doing 'custom', how is that going to
actually work unless what's actually happening is... just SCRAM?  If
only the server-side is 'custom', while the client thinks it's doing
SCRAM, then the client is going to:

Send to server (at least): username + random nonce

Get ... something back from the server, but needs to be a nonce, a salt,
and an iteration count

Calculate the SaltedPW from the password + iteration from server

Calculate Auth as client-first msg +,+ server-first msg +,+ client-final

Calculate the ClientProof using ClientKey XOR HMAC(H(ClientKey), Auth)

Send to server client-final and ClientProof

Get back ... something? from the server, but it had better match up with
what the client calculates as the right answer

Calculate the ServerSignature based on what the server sent and compare

I don't really follow how that would actually work if the whole thing
isn't just SCRAM, and if that's what it is, then why do the whole
'custom' thing at all instead of just providing a way for someone to
centralize rolpassword?

> > > As I said in
> > > https://www.postgresql.org/message-id/20220318032520.t2bcwrnterg6lq6g%40alap3.anarazel.de
> > > when describing the above, that's not enough to implement every desireable
> > > authentication method - but there's real use-cases where using the existing
> > > SASL-SCRAM-256 is sufficient.
> >
> > That this apparently isn't actually about adding new authentication
> > protocols or methods sure strikes me as odd when folks were adjusting
> > proposed patches specificially to use this for new authentication
> > methods and the subject is "Proposal: Support custom authentication
> > methods using hooks"
>
> See above. Note also it was called "Support custom authentication methods" not
> "Support custom authentication protocols".

I don't get what is being implied here as the distinction between the
two.  I see how maybe you could say that RADIUS and password are
"methods" and not "protocols" because they're simple and just "send what
the user provided to the server" and "let the server tell you if you're
authorized or not" but that's the kind of thing we should really be
moving away from because they're just not secure.

> > If what's happening is still actually SCRAM then
> > I also don't get why we would change pg_hba.conf to say 'custom' instead
> > of scram-sha-256 w/ some option to say how to go get the actual token,
> > or why there isn't anything done to deal with passwords being changed.
>
> Normally make extension APIs reasonably generic. If you want to argue against
> that here, I think that's a debate worth having (as I explicitly said
> upthread!), but we should be having it about that then.
>
> It does seem good to me that the proposed API could implement stuff like peer,
> bsd, PAM, etc without a problem.  And there's plenty other things that don't
> require different authentication protocols on the pg protocol level - using os
> specific extensions to safely get the remote user of tcp connections would be
> cool for example.

But, even with one of your examples, peer, there has to be client-side
code that knows that's what is going on and checking things to make sure
it's a secure connection.  Both sides should be be calling getpeereid(),
as we support with requirepeer on the client-side and pg_ident map on
the server-side, so that the client and the server are verifying each
other.  That PAM doesn't support a way to do that is really a strike
against it, not something that we should be modeling or building on top
of.

> > Basically, I don't agree with more-or-less anything that the patch is
> > doing if that's the goal.  If the goal is to actually change where the
> > token in rolpassword comes from for existing authentication methods and
> > specifically isn't to actually try to use this to support new
> > authenitcation methods
>
> s/methods/protocols/

Responded to this above ..

> >, then I'd suggest laying it out that way and
> > having it be an option for scram-sha-256 or whatever other methods
> > (though that seems like the only one that should really be changed in
> > this way, if you want my 2c, as the rest that would work this way are
> > basically broken, that being md5) and then make sure to have a way to
> > handle password changes too.
>
> I think that'd make it less useful, but still useful. E.g. doing different
> external <-> internal username translation than what's built in.

I don't generally have an issue with the idea of having an external
username translation but I don't think this is the way to go about
having that.

> > I'm still a -1 on this patch.
>
> Fair enough, as long as you're doing it on what the patch actually tries to do
> rather than something different.

I still have doubts about what exactly the patch is trying to do.  When
I thought that the idea was to have a centralized rolpassword, suddenly
things made sense, but then it was argued that it was trying to do more
than that and I'm left unclear as to what that 'more' was if it's only
to be something that's done on the server side.

> > Maybe there's a way to get a shared
> > storage for rolpassword and maybe that could even be extensible if we
> > want it to be (though I'd be more inclined to just build it in...)
>
> How would you add external storage for secrets in a builtin way?

We have code to reach out to an external LDAP server already, we could
certainly add an option to the scram auth method that says "go get the
value for this from this LDAP server", or we could add support for
reaching out to another PG server and running a query to get the value,
or add support for some other protocol that makes sense to query such
information out of, like a KMS.  None of those seem hard, and we have
support for LDAP and libpq already too.

> > I also don't think that it's a good idea to have
> > a synchronous call-out during authentication as that can get really
> > painful
>
> There's tradeoffs, for sure. But normally we don't prevent people from doing
> stupid stuff with our extensibility (if anything we try to keep some debatable
> stuff out of core by allowing it to be done in extensions).

We continue to have too much stuff in extensions imv, but that's a whole
other discussion.

> > and if you're cacheing it to deal with the risks of that, well,
> > seems like you'd be better off just using rolpassword (and perhaps with
> > Joshua's patches to allow more than one of those to exist at a time).
>
> That imo solves a separate problem.

Not sure which is being referred to here but also not sure that it's
terribly germane to this particular thread or patch.

Thanks,

Stephen

[CFM hat] Okay, either way you look at it, this patchset needs author
work before any further review can be done. Peter has given some
additional feedback on next steps, Stephen has asked for clarification
on the goals of the patchset, etc. I feel pretty confident in marking
this Returned with Feedback.

[dev hat] That said, I plan to do some additional dev work on top of
this over the next couple of months. The ideal case would be to provide
a server-only extension that provides additional features to existing
clients in the wild (i.e. no client-side changes).

I'm also interested in plugging an existing 3rd-party SASL library into
the client, which would help extensibility on that side.

--Jacob

Greetings,

* samay sharma (smilingsamay@gmail.com) wrote:
> On Tue, Aug 2, 2022 at 2:48 PM Jacob Champion <jchampion@timescale.com>
> wrote:
> > [dev hat] That said, I plan to do some additional dev work on top of
> > this over the next couple of months. The ideal case would be to provide
> > a server-only extension that provides additional features to existing
> > clients in the wild (i.e. no client-side changes).
> >
> > I'm also interested in plugging an existing 3rd-party SASL library into
> > the client, which would help extensibility on that side.
>
> That would be interesting.

Again, server-side only is not interesting and not a direction that
makes sense to go in because it doesn't provide any way to have
trust established in both directions, which is what all modern
authentication methods do (certificates, kerberos, scram) and is what we
should expect from anything new in this space.  If anything, the other
auth methods should be ripped out entirely (password, md5, ldap, etc),
but certainly not used as a basis for new work or a place to try and add
new features, as they're all well known to have serious vulnerabilities.

As it specifically relates to SASL- if there's work to properly support
SASL for psql/libpq while linking to an external library which supports,
say, Kerberos through SASL, then sure, having that could work out well
and I wouldn't object to it, but I don't agree that we should have
dedicated SASL code which links to an external library on the server
side without any way to exercise it through libpq/psql, or vice-versa.

I also don't agree that this makes sense as an extension as we don't
have any way for extensions to make changes in libpq or psql, again
leading to the issue that it either can't be exercised or we create some
dependency on an external SASL library for libpq but object to having
that same dependency on the server side, which doesn't seem sensible to
me.  Requiring admins to jump through hoops to install an extension
where we have such a dependency on a library anyway doesn't make sense
either.

Thanks,

Stephen

Hi,

On 2022-08-03 16:28:08 -0400, Stephen Frost wrote:
> Again, server-side only is not interesting and not a direction that
> makes sense to go in because it doesn't provide any way to have
> trust established in both directions, which is what all modern
> authentication methods do (certificates, kerberos, scram) and is what we
> should expect from anything new in this space.

As explained repeatedly before, that's plainly not true. The patch allows
e.g. to use the exact scram flow we already have, with the code we have for
that (e.g. using a different source of secret).  In fact the example extension
does so (starting in v3, from 2022-03-15):
Check 0002 from
https://www.postgresql.org/message-id/CAJxrbyxgFzfqby%2BVRCkeAhJnwVZE50%2BZLPx0JT2TDg9LbZtkCg%40mail.gmail.com

If you're ideologically opposed to allowing extensibility in this specific
area: Say so, instead of repeating this bogus argument over and over.


> If anything, the other auth methods should be ripped out entirely (password,
> md5, ldap, etc), but certainly not used as a basis for new work or a place
> to try and add new features, as they're all well known to have serious
> vulnerabilities.

I don't think we'd help users if we ripped out all those methods without a
replacement, but relegating them to contrib/ and thus requiring that they
explicitly get configured in the server seems like a decent step. But imo
that's a separate discussion.


> I also don't agree that this makes sense as an extension as we don't
> have any way for extensions to make changes in libpq or psql, again
> leading to the issue that it either can't be exercised or we create some
> dependency on an external SASL library for libpq but object to having
> that same dependency on the server side, which doesn't seem sensible to
> me.  Requiring admins to jump through hoops to install an extension
> where we have such a dependency on a library anyway doesn't make sense
> either.

This argument doesn't make a whole lot of sense to me - as explained above you
can use the existing scram flow for plenty usecases. I'd be a bit more
convinced if you'd argue that the extension API should just allow providing a
different source of secrets for the existing scram flow (I'd argue that that's
not the best point for extensibility, but that'd be more a question of taste).

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-08-03 16:28:08 -0400, Stephen Frost wrote:
> > Again, server-side only is not interesting and not a direction that
> > makes sense to go in because it doesn't provide any way to have
> > trust established in both directions, which is what all modern
> > authentication methods do (certificates, kerberos, scram) and is what we
> > should expect from anything new in this space.
>
> As explained repeatedly before, that's plainly not true. The patch allows
> e.g. to use the exact scram flow we already have, with the code we have for
> that (e.g. using a different source of secret).  In fact the example extension
> does so (starting in v3, from 2022-03-15):

Sure, thanks to the bespoke code in libpq for supporting SCRAM.  I don't
think it makes sense to move the server-side code for that into an
extension as it just makes work for a bunch of people.  Having a way to
have the authenticator token for scram exist elsewhere doesn't strike me
as unreasonable but that's not the same thing and is certainly more
constrained in terms of what it's doing.

> If you're ideologically opposed to allowing extensibility in this specific
> area: Say so, instead of repeating this bogus argument over and over.

Considering how much I pushed for and supported the effort to include
SCRAM, I hardly would say that I'm against making improvements in this
area.

Further, I outlined exactly how extensability in this area could be
achieved: use some good third party library that provides multiple SASL
methods then just add support for that library to *PG*, on both the
libpq and the server sides.

What I don't think makes sense is adding extensibility to the server
side, especially if it's through a 3rd party library, but then not
adding support for that to libpq.  I don't follow what the rationale is
for explicitly excluding libpq from this discussion.

To be clear- I'm not explicitly saying that we can only add
extensibility with SCRAM, I'm just saying that whatever we're doing here
to add other actual authentication methods we should be ensuring is done
on both sides with a way for each side to authenticate the other side,
as all of the modern authentication methods we have already do.  If we
got SASL added and it included support for SCRAM and Kerberos through
that and was a common enough library that we felt comfortable ripping
out our own implementations in favor of what that library provides,
sure, that'd be something to consider too (though I tend to doubt we'd
get so lucky as to have one that worked with the existing SASL/SCRAM
stuff we have today since we had to do some odd things there with the
username, as I recall).

> > If anything, the other auth methods should be ripped out entirely (password,
> > md5, ldap, etc), but certainly not used as a basis for new work or a place
> > to try and add new features, as they're all well known to have serious
> > vulnerabilities.
>
> I don't think we'd help users if we ripped out all those methods without a
> replacement, but relegating them to contrib/ and thus requiring that they
> explicitly get configured in the server seems like a decent step. But imo
> that's a separate discussion.

We have a replacement for password and md5 and it's SCRAM.  For my 2c, I
don't see the value in continuing to have those in any form at this
point.  I concede that I may not get consensus on that but I don't
really see how moving them to contrib would actually be helpful.

> > I also don't agree that this makes sense as an extension as we don't
> > have any way for extensions to make changes in libpq or psql, again
> > leading to the issue that it either can't be exercised or we create some
> > dependency on an external SASL library for libpq but object to having
> > that same dependency on the server side, which doesn't seem sensible to
> > me.  Requiring admins to jump through hoops to install an extension
> > where we have such a dependency on a library anyway doesn't make sense
> > either.
>
> This argument doesn't make a whole lot of sense to me - as explained above you
> can use the existing scram flow for plenty usecases. I'd be a bit more
> convinced if you'd argue that the extension API should just allow providing a
> different source of secrets for the existing scram flow (I'd argue that that's
> not the best point for extensibility, but that'd be more a question of taste).

As I think I said before, I don't particularly object to the idea of
having an alternative backing store for pg_authid (though note that I'm
actively working on extending that further to allow multiple
authenticators to be able to be configured for a role, so whatever that
backing store is would need to be adjusted if that ends up getting
committed into core).  That's quite a different thing from my
perspective.

Thanks,

Stephen

Hi,

On 2022-08-03 17:21:58 -0400, Stephen Frost wrote:
> * Andres Freund (andres@anarazel.de) wrote:
> > On 2022-08-03 16:28:08 -0400, Stephen Frost wrote:
> > > Again, server-side only is not interesting and not a direction that
> > > makes sense to go in because it doesn't provide any way to have
> > > trust established in both directions, which is what all modern
> > > authentication methods do (certificates, kerberos, scram) and is what we
> > > should expect from anything new in this space.
> > 
> > As explained repeatedly before, that's plainly not true. The patch allows
> > e.g. to use the exact scram flow we already have, with the code we have for
> > that (e.g. using a different source of secret).  In fact the example extension
> > does so (starting in v3, from 2022-03-15):
> 
> Sure, thanks to the bespoke code in libpq for supporting SCRAM.  I don't
> think it makes sense to move the server-side code for that into an
> extension as it just makes work for a bunch of people.  Having a way to
> have the authenticator token for scram exist elsewhere doesn't strike me
> as unreasonable but that's not the same thing and is certainly more
> constrained in terms of what it's doing.

The question is: Why is providing that ability not a good step, even if
somewhat constrainted? One argument would be that a later "protocol level
extensibility" effort would require significant changes to the API - but I
don't think that'd be the case. And even if, this isn't a large extensibility
surface, so API evolution wouldn't be a problem.


> Further, I outlined exactly how extensability in this area could be
> achieved: use some good third party library that provides multiple SASL
> methods then just add support for that library to *PG*, on both the
> libpq and the server sides.

> What I don't think makes sense is adding extensibility to the server
> side, especially if it's through a 3rd party library, but then not
> adding support for that to libpq.  I don't follow what the rationale is
> for explicitly excluding libpq from this discussion.

The rationale is trivial: Breaking down projects into manageable, separately
useful, steps is a very sensible way of doing development. Even if we get
protocol extensibility, we're still going to need an extension API like it's
provided by the patchset. After protocol extensibility the authentication
extensions would then have more functions it could call to control the auth
flow with the client.


> To be clear- I'm not explicitly saying that we can only add
> extensibility with SCRAM, I'm just saying that whatever we're doing here
> to add other actual authentication methods we should be ensuring is done
> on both sides with a way for each side to authenticate the other side,
> as all of the modern authentication methods we have already do.

But *why* do these need to be tied together?


> > > If anything, the other auth methods should be ripped out entirely (password,
> > > md5, ldap, etc), but certainly not used as a basis for new work or a place
> > > to try and add new features, as they're all well known to have serious
> > > vulnerabilities.
> > 
> > I don't think we'd help users if we ripped out all those methods without a
> > replacement, but relegating them to contrib/ and thus requiring that they
> > explicitly get configured in the server seems like a decent step. But imo
> > that's a separate discussion.
> 
> We have a replacement for password and md5 and it's SCRAM.  For my 2c, I
> don't see the value in continuing to have those in any form at this
> point.  I concede that I may not get consensus on that but I don't
> really see how moving them to contrib would actually be helpful.

I'm much more on board with ripping out password and md5 than ldap, radius,
pam et al.

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2022-08-03 17:21:58 -0400, Stephen Frost wrote:
> > * Andres Freund (andres@anarazel.de) wrote:
> > > On 2022-08-03 16:28:08 -0400, Stephen Frost wrote:
> > > > Again, server-side only is not interesting and not a direction that
> > > > makes sense to go in because it doesn't provide any way to have
> > > > trust established in both directions, which is what all modern
> > > > authentication methods do (certificates, kerberos, scram) and is what we
> > > > should expect from anything new in this space.
> > >
> > > As explained repeatedly before, that's plainly not true. The patch allows
> > > e.g. to use the exact scram flow we already have, with the code we have for
> > > that (e.g. using a different source of secret).  In fact the example extension
> > > does so (starting in v3, from 2022-03-15):
> >
> > Sure, thanks to the bespoke code in libpq for supporting SCRAM.  I don't
> > think it makes sense to move the server-side code for that into an
> > extension as it just makes work for a bunch of people.  Having a way to
> > have the authenticator token for scram exist elsewhere doesn't strike me
> > as unreasonable but that's not the same thing and is certainly more
> > constrained in terms of what it's doing.
>
> The question is: Why is providing that ability not a good step, even if
> somewhat constrainted? One argument would be that a later "protocol level
> extensibility" effort would require significant changes to the API - but I
> don't think that'd be the case. And even if, this isn't a large extensibility
> surface, so API evolution wouldn't be a problem.

I'm quite confused by this as I feel like I've said multiple times that
having a way to have the SCRAM authenticator token come from somewhere
else seems reasonable.  If that's not what you're referring to above by
'that ability' then it'd really help if you could clarify what you mean
there.

> > Further, I outlined exactly how extensability in this area could be
> > achieved: use some good third party library that provides multiple SASL
> > methods then just add support for that library to *PG*, on both the
> > libpq and the server sides.
>
> > What I don't think makes sense is adding extensibility to the server
> > side, especially if it's through a 3rd party library, but then not
> > adding support for that to libpq.  I don't follow what the rationale is
> > for explicitly excluding libpq from this discussion.
>
> The rationale is trivial: Breaking down projects into manageable, separately
> useful, steps is a very sensible way of doing development. Even if we get
> protocol extensibility, we're still going to need an extension API like it's
> provided by the patchset. After protocol extensibility the authentication
> extensions would then have more functions it could call to control the auth
> flow with the client.
>
> > To be clear- I'm not explicitly saying that we can only add
> > extensibility with SCRAM, I'm just saying that whatever we're doing here
> > to add other actual authentication methods we should be ensuring is done
> > on both sides with a way for each side to authenticate the other side,
> > as all of the modern authentication methods we have already do.
>
> But *why* do these need to be tied together?

I'm also confused by this.  Surely you'd need a way to *test* such a new
authentication library being added to the core code and that naturally
requires having it be implemented on both sides, so I don't see why you
wouldn't just do that from the get go.  I don't see how it makes sense
to talk about such a project as if it could possibly be server-side
only.  The example being discussed- adding SASL support using a third
party library such as libsasl2, would certainly be something that you'd
add on both sides and then test if you got it right and make sure that
it worked.  Recent examples also bear this out- I can't imagine anyone
would have gotten behind the idea of adding GSSAPI encryption to just
the server side, even if you could somehow argue that it would have been
a separately useful step because maybe someone out there happened to
have implemented it in their own client.  The same is true of Kerberos
credential delegation- yeah, we could implement that just on the server,
but if libpq isn't doing it then we couldn't test it as part of the core
project and it's certainly not some huge amount of additional work to
implement it in libpq too, where it then immediately becomes available
to a large number of downstream interfaces like psycopg2 and various
others.  The few non-libpq-based interfaces typically follow what we do
in core on the client side anyway, such as the JDBC project.

If the idea is that we should just add hooks to allow someone to
reimplement SCRAM but external to the core server and only on the server
side then what's going to be on the client side to interface with that?
How would one add support for whatever that new authentication method is
to libpq, where we don't have the ability to plug in other
authentication methods?  Further, how would we have any idea how to test
that we don't break that?  Lastly, if we find some serious flaw in our
existing implementation of SCRAM (no matter how unlikely ...) that
requires us to change these hooks, we're in a world of trouble.  This is
quite different from other parts of the system as it's inherent to how
users are externally authenticated and that's not an area to take
lightly.

> > > > If anything, the other auth methods should be ripped out entirely (password,
> > > > md5, ldap, etc), but certainly not used as a basis for new work or a place
> > > > to try and add new features, as they're all well known to have serious
> > > > vulnerabilities.
> > >
> > > I don't think we'd help users if we ripped out all those methods without a
> > > replacement, but relegating them to contrib/ and thus requiring that they
> > > explicitly get configured in the server seems like a decent step. But imo
> > > that's a separate discussion.
> >
> > We have a replacement for password and md5 and it's SCRAM.  For my 2c, I
> > don't see the value in continuing to have those in any form at this
> > point.  I concede that I may not get consensus on that but I don't
> > really see how moving them to contrib would actually be helpful.
>
> I'm much more on board with ripping out password and md5 than ldap, radius,
> pam et al.

That would at least be some progress in the right direction, though not
as far as I'd like to go.  Combined with adding options to libpq which
prevents it from ever sending a password in the clear across the wire,
as required by all of those other methods, would definitely help since
then we could strongly encourage users to enable that option, or better,
set it by default and force users to have to set some option that
explicitly allows such insecure password handling to be allowed.

Thanks,

Stephen

Greetings,

Want to resurface the OAUTH support topic in the context of the
concerns raised here.

> How about- if we just added OAUTH support directly into libpq and the
> backend, would that work with Azure's OIDC provider? If not, why not?
> If it does, then what's the justification for trying to allow custom
> backend-only authentication methods?

We've explored this option, and prepared a patch which has bare-bone
OAUTHBEARER mechanism implementation on both server- and libpq- sides.
Based on existing SASL exchange support used for SCRAM.

Most of the work has been done by @Jacob Champion and was referenced
in this thread before.
We validated the approach would work with Azure AD and other OIDC
providers with token validation logic delegated to provider-specific
extension.

The thread link is here:

https://www.postgresql.org/message-id/flat/CABkiuWo4fJQ7dhqgYLtJh41kpCkT6iXOO8Eym3Rdh5tx2RJCJw%40mail.gmail.com#f94c36969a68a07c087fa9af0f5401e1

With the client- and server- side support, the proposed patch would allow:
- Identity providers to publish PG extensions to validate their
specific token format.
- Client ecosystem to build generic OAUTH flows using OIDC provider
metadata defined in RFCs
- Cloud providers to support a combination of Identity providers they work with.

Looking forward to bring the discussion to that thread, and decide if
we can proceed with the OAUTH support.

Thanks,
Andrey.

On Wed, Jan 25, 2023 at 10:55 AM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greetings,
>
> * Andres Freund (andres@anarazel.de) wrote:
> > On 2022-08-03 17:21:58 -0400, Stephen Frost wrote:
> > > * Andres Freund (andres@anarazel.de) wrote:
> > > > On 2022-08-03 16:28:08 -0400, Stephen Frost wrote:
> > > > > Again, server-side only is not interesting and not a direction that
> > > > > makes sense to go in because it doesn't provide any way to have
> > > > > trust established in both directions, which is what all modern
> > > > > authentication methods do (certificates, kerberos, scram) and is what we
> > > > > should expect from anything new in this space.
> > > >
> > > > As explained repeatedly before, that's plainly not true. The patch allows
> > > > e.g. to use the exact scram flow we already have, with the code we have for
> > > > that (e.g. using a different source of secret).  In fact the example extension
> > > > does so (starting in v3, from 2022-03-15):
> > >
> > > Sure, thanks to the bespoke code in libpq for supporting SCRAM.  I don't
> > > think it makes sense to move the server-side code for that into an
> > > extension as it just makes work for a bunch of people.  Having a way to
> > > have the authenticator token for scram exist elsewhere doesn't strike me
> > > as unreasonable but that's not the same thing and is certainly more
> > > constrained in terms of what it's doing.
> >
> > The question is: Why is providing that ability not a good step, even if
> > somewhat constrainted? One argument would be that a later "protocol level
> > extensibility" effort would require significant changes to the API - but I
> > don't think that'd be the case. And even if, this isn't a large extensibility
> > surface, so API evolution wouldn't be a problem.
>
> I'm quite confused by this as I feel like I've said multiple times that
> having a way to have the SCRAM authenticator token come from somewhere
> else seems reasonable.  If that's not what you're referring to above by
> 'that ability' then it'd really help if you could clarify what you mean
> there.
>
> > > Further, I outlined exactly how extensability in this area could be
> > > achieved: use some good third party library that provides multiple SASL
> > > methods then just add support for that library to *PG*, on both the
> > > libpq and the server sides.
> >
> > > What I don't think makes sense is adding extensibility to the server
> > > side, especially if it's through a 3rd party library, but then not
> > > adding support for that to libpq.  I don't follow what the rationale is
> > > for explicitly excluding libpq from this discussion.
> >
> > The rationale is trivial: Breaking down projects into manageable, separately
> > useful, steps is a very sensible way of doing development. Even if we get
> > protocol extensibility, we're still going to need an extension API like it's
> > provided by the patchset. After protocol extensibility the authentication
> > extensions would then have more functions it could call to control the auth
> > flow with the client.
> >
> > > To be clear- I'm not explicitly saying that we can only add
> > > extensibility with SCRAM, I'm just saying that whatever we're doing here
> > > to add other actual authentication methods we should be ensuring is done
> > > on both sides with a way for each side to authenticate the other side,
> > > as all of the modern authentication methods we have already do.
> >
> > But *why* do these need to be tied together?
>
> I'm also confused by this.  Surely you'd need a way to *test* such a new
> authentication library being added to the core code and that naturally
> requires having it be implemented on both sides, so I don't see why you
> wouldn't just do that from the get go.  I don't see how it makes sense
> to talk about such a project as if it could possibly be server-side
> only.  The example being discussed- adding SASL support using a third
> party library such as libsasl2, would certainly be something that you'd
> add on both sides and then test if you got it right and make sure that
> it worked.  Recent examples also bear this out- I can't imagine anyone
> would have gotten behind the idea of adding GSSAPI encryption to just
> the server side, even if you could somehow argue that it would have been
> a separately useful step because maybe someone out there happened to
> have implemented it in their own client.  The same is true of Kerberos
> credential delegation- yeah, we could implement that just on the server,
> but if libpq isn't doing it then we couldn't test it as part of the core
> project and it's certainly not some huge amount of additional work to
> implement it in libpq too, where it then immediately becomes available
> to a large number of downstream interfaces like psycopg2 and various
> others.  The few non-libpq-based interfaces typically follow what we do
> in core on the client side anyway, such as the JDBC project.
>
> If the idea is that we should just add hooks to allow someone to
> reimplement SCRAM but external to the core server and only on the server
> side then what's going to be on the client side to interface with that?
> How would one add support for whatever that new authentication method is
> to libpq, where we don't have the ability to plug in other
> authentication methods?  Further, how would we have any idea how to test
> that we don't break that?  Lastly, if we find some serious flaw in our
> existing implementation of SCRAM (no matter how unlikely ...) that
> requires us to change these hooks, we're in a world of trouble.  This is
> quite different from other parts of the system as it's inherent to how
> users are externally authenticated and that's not an area to take
> lightly.
>
> > > > > If anything, the other auth methods should be ripped out entirely (password,
> > > > > md5, ldap, etc), but certainly not used as a basis for new work or a place
> > > > > to try and add new features, as they're all well known to have serious
> > > > > vulnerabilities.
> > > >
> > > > I don't think we'd help users if we ripped out all those methods without a
> > > > replacement, but relegating them to contrib/ and thus requiring that they
> > > > explicitly get configured in the server seems like a decent step. But imo
> > > > that's a separate discussion.
> > >
> > > We have a replacement for password and md5 and it's SCRAM.  For my 2c, I
> > > don't see the value in continuing to have those in any form at this
> > > point.  I concede that I may not get consensus on that but I don't
> > > really see how moving them to contrib would actually be helpful.
> >
> > I'm much more on board with ripping out password and md5 than ldap, radius,
> > pam et al.
>
> That would at least be some progress in the right direction, though not
> as far as I'd like to go.  Combined with adding options to libpq which
> prevents it from ever sending a password in the clear across the wire,
> as required by all of those other methods, would definitely help since
> then we could strongly encourage users to enable that option, or better,
> set it by default and force users to have to set some option that
> explicitly allows such insecure password handling to be allowed.
>
> Thanks,
>
> Stephen

Greetings,

* Andrey Chudnovsky (achudnovskij@gmail.com) wrote:
> The thread link is here:
>
https://www.postgresql.org/message-id/flat/CABkiuWo4fJQ7dhqgYLtJh41kpCkT6iXOO8Eym3Rdh5tx2RJCJw%40mail.gmail.com#f94c36969a68a07c087fa9af0f5401e1

Thanks for pointing out the updates on that thread, I've responded over
there with my thoughts.

Thanks again,

Stephen

(Splitting off a new thread, and mostly clearing the CC list so people
can escape if they want, because this is a heckuva tangent.)

TL;DR: the attached patchset lets you authenticate with Postgres by
forwarding SCRAM authentication to a backend LDAP server, and I use it
to make arguments for long-term plans.

= Motivation =

Upthread (and in other threads) there have been a bunch of related open
questions around authentication:

- Should our authn/z framework be extensible, or should core provide the
only supported implementations?
- If it's extensible, are server-side extensions useful by themselves,
or do we need to have client-side extension points as well?
- What would a good extension API look like? Should extensions be able
to switch out core's implementation of the exchange (e.g. entire SASL
mechanisms) or should they just provide individual authentication
details to core (e.g. SCRAM secrets)?
- Plaintext auth is terrible. Can we get rid of it?

Additionally, there have been recent conversations about the next steps
for our SCRAM implementation:

- Do we need to maintain RFC compatibility, or is it sufficient for
libpq and Postgres to speak the same non-standard flavor?
- Do we want to maintain multiple channel binding types? Can we make use
of endpoint bindings effectively or should we switch to unique?

I like talking about code I can see, so I've written an experimental
feature that touches on several of these points at once.

= An LDAP/SCRAM Bridge =

So this thing, based on Samay's server-side auth hooks, authenticates
the user by forwarding the client's SCRAM header to an LDAP server. If
the LDAP exchange succeeds, the user is allowed in. (The security of
that hinges on ensuring that the Postgres user and the SCRAM/LDAP user
are in fact the same.)

I see this architecture -- if not necessarily the implementation -- as a
straight upgrade from plaintext LDAP auth. The secret remains embedded
in LDAP, never exposed. So the Postgres server can't log in as the user
again after it drops its backend connection, and it can't even pretend
to be the LDAP server in the future, though it can still use that
in-progress connection attempt to act on behalf of the user.

(I hear you asking, "doesn't this break with channel bindings, since
they prevent MITM attacks?" and the answer is "no, or at least it's not
supposed to." We implement an endpoint binding -- which permits MITMs
*if* they also hold the server's private key -- instead of a unique
binding. In other words, we explicitly allow proxied authentication
between servers holding the same certificate.

Unfortunately, OpenLDAP 2.6 implements something it *calls*
tls-server-end-point but is *not*, so if you want to see that in action
you have to patch OpenLDAP. Which is not very helpful for my argument.
Ah well.)

This patchset should ideally have required zero client side changes, but
because our SCRAM implementation is slightly nonstandard too -- it
doesn't embed the username into the SCRAM data -- libpq can't talk to
the OpenLDAP/Cyrus SASL implementation. I included a quick-and-bad patch
to fix it in libpq; that needs its own conversation.

= My Arguments =

Okay, so going back to the open questions:

I think this is exactly the sort of thing that's too niche to be in core
but might be extremely useful for the few people it applies to, so I'm
proposing it as an argument in favor of general authn/z extensibility.

Furthermore, this is an example of a server-side-only extension that is
useful by itself and is still compatible with a (SCRAM-compliant) client.

This approach works best by switching out the entire implementation of
the SCRAM SASL mechanism. You wouldn't be able to implement this
architecture with a more targeted, secrets-focused API. (And personally
I think the idea of exchanging SCRAM secrets between machines is
malpractice. Those are the building blocks of user trust; we're supposed
to protect them accordingly.)

Keeping the OAuth thread in mind, I still think it would be most helpful
to develop and switch out SASL mechanisms on *both* the server and
client side, independently of the Postgres major version. (Which means
letting third parties do it too. I know there are valid concerns about
that; I still want Postgres to have the best general implementations.)

My preference would be to use an existing pluggable SASL implementation
rather than growing our own. But when we chose not to implement SCRAM to
the letter, we tied our own hands a bit. I briefly tried porting the
server side to two separate third-party SASL libraries, and I
immediately ran into compatibility issues with the SCRAM username (not
included in the libpq flavor of SCRAM, as noted above) and the default
channel binding (endpoint instead of unique). We'd probably need to
backport compatibility fixes if we wanted to roll out third-party SASL.

Finally, I think allowing a server to proxy your authority via an
endpoint binding is something you should be able to opt into -- or at
least opt out of? And I don't want to remove endpoint bindings entirely,
because I think this functionality is potentially very useful. I'd like
to see us support both, and eventually default to unique.

= Patch Roadmap =

- 0001-4 are Samay's server-side auth hook set, rebased over HEAD.
- 0005 is a quick client-side hack to include the username in the SCRAM
header, making it compliant enough to interoperate in the simplest
cases. (It's not compliant enough to be safe, yet.)
- 0006 is the actual implementation, including tests so you can try it
out too.
- Finally, the OpenLDAP-* patch is for OpenLDAP 2.6.4; it's a hack to
implement standard channel bindings so you can prove to yourself that
they work. If you apply this, set $ldap_supports_channel_binding in the
Perl test so you can see it working.

Thanks,
--Jacob

Greetings,

* Jacob Champion (jchampion@timescale.com) wrote:
> This patchset should ideally have required zero client side changes, but
> because our SCRAM implementation is slightly nonstandard too -- it
> doesn't embed the username into the SCRAM data -- libpq can't talk to
> the OpenLDAP/Cyrus SASL implementation. I included a quick-and-bad patch
> to fix it in libpq; that needs its own conversation.

Indeed it does... as there were specific reasons that what we
implemented for PG's SCRAM doesn't embed the username into the SCRAM
data and my recollection is that we don't because SCRAM (or maybe SASL?
either way though...) requires it to be utf-8 encoded and we support a
lot of other encoding that aren't that, so it wouldn't work for a lot
of our users.

Not really seeing that as being something we get to be picky about or
decide to change our mind on.  It's unfortunate that the standard seems
to be short-sighted in this way but that's the reality of it.

> I think this is exactly the sort of thing that's too niche to be in core
> but might be extremely useful for the few people it applies to, so I'm
> proposing it as an argument in favor of general authn/z extensibility.

If it was able to actually work for our users (and maybe it is if we
make it optional somehow) and we have users who want it (which certainly
seems plausible) then I'd say that it's something we would benefit from
having in core.  While it wouldn't solve all the issues with 'ldap'
auth, if it works with AD's LDAP servers and doesn't require the
password to be passed from the client to the server (in any of this, be
the client psql, pgadmin, or the PG server when it goes to talk to the
LDAP server..) then that would be a fantastic thing and we could
replace the existing 'ldap' auth with that and be *much* better off for
it, and so would our users.

Thanks,

Stephen

On Mon, Feb 27, 2023 at 12:43 PM Stephen Frost <sfrost@snowman.net> wrote:
> * Jacob Champion (jchampion@timescale.com) wrote:
> > This patchset should ideally have required zero client side changes, but
> > because our SCRAM implementation is slightly nonstandard too -- it
> > doesn't embed the username into the SCRAM data -- libpq can't talk to
> > the OpenLDAP/Cyrus SASL implementation. I included a quick-and-bad patch
> > to fix it in libpq; that needs its own conversation.
>
> Indeed it does... as there were specific reasons that what we
> implemented for PG's SCRAM doesn't embed the username into the SCRAM
> data and my recollection is that we don't because SCRAM (or maybe SASL?
> either way though...) requires it to be utf-8 encoded and we support a
> lot of other encoding that aren't that, so it wouldn't work for a lot
> of our users.

Yes. Interoperable authentication is going to have to solve those
sorts of problems somehow, though. And there are a bunch of levers to
pull: clients aren't required to run their sent usernames through
SASLprep; our existing servers don't mind having it sent, so we could
potentially backport a client change; and then after that it's a game
of balancing compatibility and compliance. A deeper conversation for
sure.

> > I think this is exactly the sort of thing that's too niche to be in core
> > but might be extremely useful for the few people it applies to, so I'm
> > proposing it as an argument in favor of general authn/z extensibility.
>
> If it was able to actually work for our users (and maybe it is if we
> make it optional somehow) and we have users who want it (which certainly
> seems plausible) then I'd say that it's something we would benefit from
> having in core.  While it wouldn't solve all the issues with 'ldap'
> auth, if it works with AD's LDAP servers and doesn't require the
> password to be passed from the client to the server (in any of this, be
> the client psql, pgadmin, or the PG server when it goes to talk to the
> LDAP server..) then that would be a fantastic thing and we could
> replace the existing 'ldap' auth with that and be *much* better off for
> it, and so would our users.

I think the argument you're making here boils down to "if it's not
niche, it should be in core", and I agree with that general sentiment.
But not all of the prerequisites you stated are met. I see no evidence
that Active Directory supports SCRAM [1], for instance, unless the MS
documentation is just omitting it.

Even if it were applicable to every single LDAP deployment, I'd still
like users to be able to choose the best authentication available in
their setups without first having to convince the community. They can
maintain the things that make sense for them, like they do with
extensions. And the authors can iterate on better authentication out
of cycle, like extension authors do.

--Jacob

[1] https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/e7d814a5-4cb5-4b0d-b408-09d79988b550

Greetings,

* Jacob Champion (jchampion@timescale.com) wrote:
> On Mon, Feb 27, 2023 at 12:43 PM Stephen Frost <sfrost@snowman.net> wrote:
> > * Jacob Champion (jchampion@timescale.com) wrote:
> > > This patchset should ideally have required zero client side changes, but
> > > because our SCRAM implementation is slightly nonstandard too -- it
> > > doesn't embed the username into the SCRAM data -- libpq can't talk to
> > > the OpenLDAP/Cyrus SASL implementation. I included a quick-and-bad patch
> > > to fix it in libpq; that needs its own conversation.
> >
> > Indeed it does... as there were specific reasons that what we
> > implemented for PG's SCRAM doesn't embed the username into the SCRAM
> > data and my recollection is that we don't because SCRAM (or maybe SASL?
> > either way though...) requires it to be utf-8 encoded and we support a
> > lot of other encoding that aren't that, so it wouldn't work for a lot
> > of our users.
>
> Yes. Interoperable authentication is going to have to solve those
> sorts of problems somehow, though. And there are a bunch of levers to
> pull: clients aren't required to run their sent usernames through
> SASLprep; our existing servers don't mind having it sent, so we could
> potentially backport a client change; and then after that it's a game
> of balancing compatibility and compliance. A deeper conversation for
> sure.

We did solve those problems with SCRAM, by not exactly following the
unfortunately short-sighted standard.  What we have gets us the benefits
of SCRAM and generally follows the standard without giving up non-utf8
encodings of roles.  If there's a compelling reason to support another
authentication method then I'd hope we would look for similar ways to
support it without giving up too much.

I certainly don't see it making sense to backport some change in this
area.

> > > I think this is exactly the sort of thing that's too niche to be in core
> > > but might be extremely useful for the few people it applies to, so I'm
> > > proposing it as an argument in favor of general authn/z extensibility.
> >
> > If it was able to actually work for our users (and maybe it is if we
> > make it optional somehow) and we have users who want it (which certainly
> > seems plausible) then I'd say that it's something we would benefit from
> > having in core.  While it wouldn't solve all the issues with 'ldap'
> > auth, if it works with AD's LDAP servers and doesn't require the
> > password to be passed from the client to the server (in any of this, be
> > the client psql, pgadmin, or the PG server when it goes to talk to the
> > LDAP server..) then that would be a fantastic thing and we could
> > replace the existing 'ldap' auth with that and be *much* better off for
> > it, and so would our users.
>
> I think the argument you're making here boils down to "if it's not
> niche, it should be in core", and I agree with that general sentiment.

Good.  That certainly seems like a start.

> But not all of the prerequisites you stated are met. I see no evidence
> that Active Directory supports SCRAM [1], for instance, unless the MS
> documentation is just omitting it.

That's certainly unfortunate if that's the case.  The question then
becomes- are a lot of people using SCRAM to connect to OpenLDAP, to the
extent that it's a sensible thing for us to support and gives us a more
secure option for 'ldap' auth than what exists today where we're passing
around cleartext passwords?  That seems like it's at least plausible.

> Even if it were applicable to every single LDAP deployment, I'd still
> like users to be able to choose the best authentication available in
> their setups without first having to convince the community. They can
> maintain the things that make sense for them, like they do with
> extensions. And the authors can iterate on better authentication out
> of cycle, like extension authors do.

Considering how few outside-of-core well maintained extensions for PG
exist, I have an extremely hard time buying off on the argument that the
authentication system is a smart area for us to encourage approach in.

Broadly speaking, I feel confident saying that authentication systems,
rather like encryption libraries, should be standardized, well
documented, heavily reviewed, and carefully maintained.  If the argument
is that there are these teams out there who are itching to write amazing
new authentication methods for PG who are going to spend time
documenting them, reviewing them, and maintaining them, then we should
try to get those folks to work with the community to add support for
these new methods so that everyone can benefit from them- not
encouraging them to be independent projects which have to work through
hooks that limit how those authentication methods are able to be
integrated.  Consider things like pg_stat_ssl and pg_stat_gssapi.  What
about pg_ident maps?  Will each of these end up with their own version
of that?  And those are questions beyond just the issue around making
sure that these external extensions are well maintained and that we
don't end up breaking them through changes to core.  I just don't see,
at least today, authentication methods in the same light as, say, data
types.

Thanks,

Stephen

On Tue, Feb 28, 2023 at 6:53 PM Stephen Frost <sfrost@snowman.net> wrote:
> * Jacob Champion (jchampion@timescale.com) wrote:
> > Yes. Interoperable authentication is going to have to solve those
> > sorts of problems somehow, though. And there are a bunch of levers to
> > pull: clients aren't required to run their sent usernames through
> > SASLprep; our existing servers don't mind having it sent, so we could
> > potentially backport a client change; and then after that it's a game
> > of balancing compatibility and compliance. A deeper conversation for
> > sure.
>
> We did solve those problems with SCRAM, by not exactly following the
> unfortunately short-sighted standard.  What we have gets us the benefits
> of SCRAM and generally follows the standard

"Almost standard" in this case is just a different standard, minus
interoperability, plus new edge cases. I understand why it was done,
and I'm trying to provide some evidence in favor of changing it. But
again, that's a separate conversation; I don't have a patchset
proposal ready to go yet, and I'd rather talk about specifics.

> > But not all of the prerequisites you stated are met. I see no evidence
> > that Active Directory supports SCRAM [1], for instance, unless the MS
> > documentation is just omitting it.
>
> That's certainly unfortunate if that's the case.  The question then
> becomes- are a lot of people using SCRAM to connect to OpenLDAP, to the
> extent that it's a sensible thing for us to support and gives us a more
> secure option for 'ldap' auth than what exists today where we're passing
> around cleartext passwords?  That seems like it's at least plausible.

I don't know for sure -- I don't have an LDAP userbase to poll for
that, anymore -- but I somewhat doubt it. (And it's moot unless the
compatibility problem is fixed.)

> Broadly speaking, I feel confident saying that authentication systems,
> rather like encryption libraries, should be standardized, well
> documented, heavily reviewed, and carefully maintained.

I mean, I agree with that statement in isolation; those are all
generally good for user safety, which is the end goal. I disagree with
your premise that those goals are achieved by doing everything
in-house, or that letting other people do it is in opposition to those
goals.

Keep in mind, you just explained why it's good that you chose to
depart from the standard in ways that prevent interoperability with
other (reviewed, standardized, maintained) SCRAM implementations, and
that you're generally opposed to backporting compatibility fixes that
would allow migration to those implementations. That doesn't seem
internally consistent to me.

> If the argument
> is that there are these teams out there who are itching to write amazing
> new authentication methods for PG who are going to spend time
> documenting them, reviewing them, and maintaining them, then we should
> try to get those folks to work with the community to add support for
> these new methods so that everyone can benefit from them-

That's not my argument, and even if it were, I think this is
depressingly Postgres-centric. Why would a team of subject matter
experts want to constantly duplicate their work over every network
protocol, as opposed to banding together in a common place (e.g. IETF)
to design an extensible authentication system (e.g. SASL) and then
contributing their expertise to implementations of that system (e.g.
Cyrus, gsasl)?

If you had made those arguments in favor of your own in-house crypto
over OpenSSL -- "Why don't those cryptographers just come here to work
on PG-RSA, which is better than (and incompatible with) regular RSA
because of the changes we made to address their shortsighted designs?"
-- then wouldn't the response be obvious?

> not
> encouraging them to be independent projects which have to work through
> hooks that limit how those authentication methods are able to be
> integrated.

That's a false choice. I don't think we'd have to encourage them to be
fully independent projects, and I don't really want us to design our
own authentication hooks in the end, as I pointed out in my initial
mail.

> Consider things like pg_stat_ssl and pg_stat_gssapi. What
> about pg_ident maps?  Will each of these end up with their own version
> of that?

I should hope not. For one, I'd imagine a framework which required
that would be vetoed pretty quickly, and I'm not really here to argue
in favor of bad frameworks. If a particular approach causes terrible
maintenance characteristics, we would probably just not take that
approach.

For two, usermaps are an authorization concern that can usually be
separated from the question of "who am I talking to". There's no
reason for, say, a SCRAM implementation to care about them, because
it's not authorizing anything. (And the OAuth work will hopefully be a
decent bellwether of handling authn and authz concerns with the same
primitives.)

> And those are questions beyond just the issue around making
> sure that these external extensions are well maintained

This sounds a lot like Not Invented Here. Why would it be on you to ensure that?

> and that we
> don't end up breaking them through changes to core.

If we implement core methods using the same framework, that risk goes
way down. Even more so if we use someone else's framework with its own
API guarantees, like we do with e.g. GSS.

--Jacob