Proposal: Support custom authentication methods using hooks

Hi all,

On Thu, Feb 17, 2022 at 11:25 AM samay sharma <smilingsamay@gmail.com>
wrote:

Hi all,

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 logic around authentication.

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.

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

As an example and a test case, I've added an extension named
test_auth_provider in src/test/modules which fetches credentials from
a pre-defined array. I've also added tap tests for the extension to test
this functionality.

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 the provider name in the pg_hba line.

A couple of my tests are flaky and sometimes fail in CI. I think the
reason for that is I don't wait for pg_hba reload to be processed before
checking logs for error messages. I didn't find an immediate way to address
that and I'm looking into it but wanted to get an initial version out for
feedback on the approach taken and interfaces. Once those get finalized, I
can submit a patch to add docs as well.

I wanted to submit a v2 of my patches.

To fix the flaky tests, I decided to avoid checking the log files for
pg_hba reload errors and rely on the output of pg_hba_file_rules. While
doing that, I found two bugs (in my code) which were causing the custom
provider line to not be outputted correctly in pg_hba_file_rules.

This updated patch-set fixes those bugs and also uses pg_hba_file_rules to
check for errors arising due to improper configuration. After these
changes, I've stopped seeing CI failures.

Looking forward to feedback on the overall change and the approach taken.

Regards,
Samay

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 logic around 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 the provider 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 logic around 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 the provider 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

Hi Aleksander,

On Thu, Feb 24, 2022 at 1:17 AM Aleksander Alekseev <
aleksander@timescale.com> 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 logic around 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/

Thank you!

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 the provider 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?

Just to clarify, the limitation is around usage of multiple custom
providers but does allow users to use the existing methods with one custom
authentication method. The reasons I thought this is ok to start with are:
* It allows users to plugin a custom authentication method which they can't
do today.
* Users *generally* have an authentication method for their database (eg.
we use ldap for authentication, or we use md5 passwords for
authentication). While it is possible, I'm not sure how many users use
completely different authentication methods (other than standard ones like
password and trust) for different IPs/databases for their same instance.

So, I thought this should be good enough for a number of use cases.

I thought about adding support for multiple custom providers and the
approach I came up with is: Maintaining a list of all providers (their
names, check functions and error functions), making sure they are all valid
while parsing pg_hba.conf and calling the right one when we see the custom
keyword in pg_hba.conf based on name. I'm not sure (I haven't yet checked)
if we have other such hooks in Postgres where multiple extensions use them
and Postgres calls the right hook based on input (instead of just calling
whoever has the hook).

Therefore, I wanted to start with something simple so users can begin using
auth methods of their choice, and add multiple providers as an enhancement
after doing more research and coming up with the right way to implement it.

That being said, any thoughts on the approach I mentioned above? I'll
look into it and see if it's not too difficult to implement this.

Regards,
Samay

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 Wed, Mar 2, 2022 at 12:32 AM 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 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.

Please note that these hooks do not restrict auth providers to use only
plaintext auth methods. We could do SCRAM with secrets which are stored
outside of Postgres using this auth plugin too. I can modify the
test_auth_provider sample extension to do something like that as Andres
suggested.

I don't see much point in that unless we deprecate *all* the

auth methods that transmit a cleartext password.

(b) Should we allow plaintext auth methods (and should we deprecate a few
currently supported auth methods which use plaintext exchange)? - I think
this is also a very important discussion but has many factors to consider
independent of the hooks. Whatever decision is made here, we can impose
that in auth.c later for plugins. For eg. allow plugins to only do
plaintext stuff with SSL enabled (by checking if Port->ssl_in_use), or if
we remove AUTH_REQ_PASSWORD, then plugins any way can’t use plaintext
exchange with the client.

So, overall, if we are open to allowing plugins for auth methods, I can
proceed to modify the test_auth_provider extension to use SCRAM and allow
registering multiple providers for this specific change.

Regards,

Samay

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:
/messages/by-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]/messages/by-id/d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com 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]: /messages/by-id/d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.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/

Hi,

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)

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.

Regards,
Samay

- 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

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

Hi,

On Wed, Mar 16, 2022 at 8:02 AM Stephen Frost <sfrost@snowman.net> wrote:

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.

I think I should have clarified this in my previous email. There is nothing
specific to OAUTHBEARER in these changes. Having auth options and using
usermaps is something which any auth method could require. I had not added
them yet, but I'm pretty sure this would have been requested.

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?

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.

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.

Regards,
Samay

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]/messages/by-id/bfc55e8045453659df26cd60035bfbb4b9530052.camel@j-davis.com.
- 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]: /messages/by-id/bfc55e8045453659df26cd60035bfbb4b9530052.camel@j-davis.com
/messages/by-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.

Greetings,

On Thu, Mar 17, 2022 at 17:02 Andres Freund <andres@anarazel.de> wrote:

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.

I am concerned.

I don’t intend to insult you or anyone else on this thread. I’m sorry.

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.

I had drafted a response to your private email to me but hadn’t wanted to
send it without going over it again after taking time to be sure I had
cooled down and was being level-headed in my response.

I am sorry. I am still concerned.

Thanks,

Stephen

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

Greetings,

On Thu, Mar 17, 2022 at 23:25 Andres Freund <andres@anarazel.de> wrote:

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.

How are we going to reasonably test this then?

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.

This is not an area, in my view, where flexibility for the sake of it is
good. Misunderstandings between the client and the server or between how
the core code and the hooks interact seem very likely and could easily lead
to insecure setups and a good chance for CVEs.

Much like encryption, authentication isn’t easy to get right. We should be
working to implement standard that experts, through RFCs and similar
well-defined protocols, have defined in the core code with lots of eyes
looking at it.

So, very much a -1 from me for trying to extend the backend in this way. I
see a lot of risk and not much, if any, benefit. I’d much rather see us
add support directly in the core code, on the client and sever side, for
OAUTH and other well defined authentication methods, and we even have an
active patch for that could make progress on that with a bit of review.

Thanks,

Stephen

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

Greetings,

On Fri, Mar 18, 2022 at 00:24 Andres Freund <andres@anarazel.de> wrote:

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.

Without a complete, independent, “this is how it will really be used” on
both the server and client side set of tests I’m not sure that it is.

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. Or is the idea that we are going to
have built-in support for some subset of “custom” things, but only on the
client side because it’s hard to do custom things there, but they’re custom
and have to be loaded through shared preload libraries on the server side?

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.

One thrust of my argument is that if we are going to support custom
authentication methods then we need to consider both sides of that, not
just the server side. Another is- it’s highly unlikely that the next
authentication method will actually be able to be implemented using these
custom hooks, based on the history we have seen of pluggable authentication
systems, and therefore I don’t agree that they make sense or will be what
we need in the future, which seems to be a large thrust of the argument
here. I’m also concerned about the risks that this presents to the project
in that there will be arguments about where the fault lies between the
hooks and the core code, as this is just inherently security-related bits,
yet that doesn’t seem to be addressed. Either way though, it strikes me as
likely to be leaving our users in a bad position either way.

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

Thanks,

Stephen

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
/messages/by-id/20220318032520.t2bcwrnterg6lq6g@alap3.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
/messages/by-id/20220318032520.t2bcwrnterg6lq6g@alap3.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
/messages/by-id/20220318032520.t2bcwrnterg6lq6g@alap3.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
/messages/by-id/20220318032520.t2bcwrnterg6lq6g@alap3.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

Hi Jacob,

On Tue, Aug 2, 2022 at 2:48 PM Jacob Champion <jchampion@timescale.com>
wrote:

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

That makes sense. I just got back to working on this patch and am aiming to
get this ready for the next commitfest. I plan to address the feedback by
Peter and Jeff and come up with a use case to help clarify the goals to
better answer Stephen's questions.

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

Regards,
Samay

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
/messages/by-id/CAJxrbyxgFzfqby+VRCkeAhJnwVZE50+ZLPx0JT2TDg9LbZtkCg@mail.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:
/messages/by-id/CABkiuWo4fJQ7dhqgYLtJh41kpCkT6iXOO8Eym3Rdh5tx2RJCJw@mail.gmail.com

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.

Greetings,

* Andrey Chudnovsky (achudnovskij@gmail.com) wrote:

The thread link is here:
/messages/by-id/CABkiuWo4fJQ7dhqgYLtJh41kpCkT6iXOO8Eym3Rdh5tx2RJCJw@mail.gmail.com

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]https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-adts/e7d814a5-4cb5-4b0d-b408-09d79988b550, 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