Обсуждение: Add "password_protocol" connection parameter to libpq

Libpq doesn't have a way to control which password protocols are used.
For example, the client might expect the server to be using SCRAM, but
it actually ends up using plain password authentication instead.

This patch adds:

  password_protocol = {plaintext|md5|scram-sha-256|scram-sha-256-plus}

as a connection parameter. Libpq will then reject any authentication
request from the server that is less secure than this setting. Setting
it to "plaintext" (default) will answer to any kind of authentication
request.

I'm not 100% happy with the name "password_protocol", but other names I
could think of seemed likely to cause confusion.

Regards,
    Jeff Davis

On Thu, Aug 08, 2019 at 03:38:20PM -0700, Jeff Davis wrote:
> Libpq doesn't have a way to control which password protocols are used.
> For example, the client might expect the server to be using SCRAM, but
> it actually ends up using plain password authentication instead.

Thanks for working on this!

> I'm not 100% happy with the name "password_protocol", but other names I
> could think of seemed likely to cause confusion.

What about auth_protocol then?  It seems to me that it could be useful
to have the restriction on AUTH_REQ_MD5 as well.

> Sets the least-secure password protocol allowable when using password
> authentication. Options are: "plaintext", "md5", "scram-sha-256", or
> "scram-sha-256-plus".

This makes it sound like there is a linear hierarchy among all those
protocols, which is true in this case, but if the list of supported
protocols is extended in the future it may be not.

I think that this should have TAP tests in src/test/authentication/ so
as we make sure of the semantics.  For the channel-binding part, the
logic path for the test would be src/test/ssl.

+#define DefaultPasswordProtocol "plaintext"
I think that we are going to need another default value for that, like
"all" to reduce the confusion that SCRAM, MD5 and co are still
included in the authorized set in this case.

Another thing that was discussed on the topic would be to allow a list
of authorized protocols instead.  I personally don't think that we
need to go necessarily this way, but it could make the integration of
things line scram-sha-256,scram-sha-256-plus easier to integrate in
application flows.
--
Michael

On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote:
> What about auth_protocol then?  It seems to me that it could be
> useful
> to have the restriction on AUTH_REQ_MD5 as well.

auth_protocol does sound like a good name. I'm not sure what you mean
regarding MD5 though.

> This makes it sound like there is a linear hierarchy among all those
> protocols, which is true in this case, but if the list of supported
> protocols is extended in the future it may be not.

We already have that concept to a lesser extent, with the md5
authentication method also permitting scram-sha-256.

Also note that the server chooses what kind of authentication request
to send, which imposes a hierarchy of password < md5 < sasl. Within the
sasl authentication request the server can advertise multiple supported
mechanisms, though, so there doesn't need to be a hierarchy among sasl
mechanisms.

> I think that this should have TAP tests in src/test/authentication/
> so
> as we make sure of the semantics.  For the channel-binding part, the
> logic path for the test would be src/test/ssl.

Will do.

> Another thing that was discussed on the topic would be to allow a
> list
> of authorized protocols instead.  I personally don't think that we
> need to go necessarily this way, but it could make the integration of
> things line scram-sha-256,scram-sha-256-plus easier to integrate in
> application flows.

That sounds good, but there are a lot of possibilities and I can't
quite decide which way to go.

We could expose it as an SASL option like:

   saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha-
256-plus}

But that doesn't allow for multiple acceptable mechanisms, which could
make migration a pain. We try to use a comma-list like:

   saslmode = {disable|prefer|require}
   saslmech = all | {scram-hash-256|scram-hash-256-plus}[,...]

Or we could over-engineer it to do something like:

   saslmode = {disable|prefer|require}
   saslmech = all | {scram|future_mech}[,...]
   scramhash = all | {sha-256|future_hash}[,...]
   scram_channel_binding = {disable|prefer|require}

(Aside: is the channel binding only a SCRAM concept, or also a SASL
concept?)

Also, working with libpq I found myself wondering why everything is
based on strings instead of enums or some other structure. Do you know
why it's done that way?

Regards,
    Jeff Davis

On Thu, Aug 08, 2019 at 11:16:24PM -0700, Jeff Davis wrote:
> On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote:
> > What about auth_protocol then?  It seems to me that it could be
> > useful
> > to have the restriction on AUTH_REQ_MD5 as well.
>
> auth_protocol does sound like a good name. I'm not sure what you mean
> regarding MD5 though.

Sorry, I meant krb5 here.

> We already have that concept to a lesser extent, with the md5
> authentication method also permitting scram-sha-256.

That's present to ease upgrades, and once the AUTH_REQ part is
received the client knows what it needs to go through.

> That sounds good, but there are a lot of possibilities and I can't
> quite decide which way to go.
>
> We could expose it as an SASL option like:
>
>    saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha-
> 256-plus}

Or we could shape password_protocol so as it takes a list of
protocols, as a white list of authorized things in short.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Thu, Aug 08, 2019 at 11:16:24PM -0700, Jeff Davis wrote:
> > On Fri, 2019-08-09 at 12:00 +0900, Michael Paquier wrote:
> > > What about auth_protocol then?  It seems to me that it could be
> > > useful
> > > to have the restriction on AUTH_REQ_MD5 as well.
> >
> > auth_protocol does sound like a good name. I'm not sure what you mean
> > regarding MD5 though.

I don't really care for auth_protocol as that's pretty close to
"auth_method" and that isn't what we're talking about here- this isn't
the user picking the auth method, per se, but rather saying which of the
password-based mechanisms for communicating that the user knows the
password is acceptable.  Letting users choose which auth methods are
allowed might also be interesting (as in- we are in a Kerberized
environment and therefore no client should ever be using any auth method
except GSS, could be a reasonable ask) but it's not the same thing.

> Sorry, I meant krb5 here.

What restriction are you suggesting here wrt krb5..?

> > We already have that concept to a lesser extent, with the md5
> > authentication method also permitting scram-sha-256.
>
> That's present to ease upgrades, and once the AUTH_REQ part is
> received the client knows what it needs to go through.

I don't think there's any question that, of the methods available to
prove that you know what the password is, simply sending the password to
the server as cleartext is the least secure.  If I, as a user, decide
that I don't really care what method is used, it's certainly simpler to
just say 'plaintext' than to have to list out every possible option.

Having an 'any' option, as mentioned before, could be an alternative
though.

I agree with the point that there isn't any guarantee that it'll always
be clear-cut as to which of two methods is "better".

From a user perspective, it seems like the main things are "don't send
my password in the clear to the server", and "require channel binding to
prove there isn't a MITM".  I have to admit that I like the idea of
requiring scram to be used and not allowing md5 though.

> > That sounds good, but there are a lot of possibilities and I can't
> > quite decide which way to go.
> >
> > We could expose it as an SASL option like:
> >
> >    saslmode = {disable|prefer|require-scram-sha-256|require-scram-sha-
> > 256-plus}
>
> Or we could shape password_protocol so as it takes a list of
> protocols, as a white list of authorized things in short.

I'm not really a fan of "saslmode" or anything else involving SASL
for this because we don't *really* do SASL- if we did, we'd have that as
an auth method and we'd have our Kerberos support be through SASL along
with potentially everything else...  I'm not against going there but I
don't think that's what you were suggesting here.

Thanks,

Stephen

On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote:
> Having an 'any' option, as mentioned before, could be an alternative
> though.

...

> I agree with the point that there isn't any guarantee that it'll
> always
> be clear-cut as to which of two methods is "better".
> 
> From a user perspective, it seems like the main things are "don't
> send
> my password in the clear to the server", and "require channel binding
> to
> prove there isn't a MITM".  I have to admit that I like the idea of
> requiring scram to be used and not allowing md5 though.

So it seems like we are leaning toward:

   password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha-
256-plus}[,...]

Or maybe:

   channel_binding = {disable|prefer|require}
   password_plaintext = {disable|enable}
   password_md5 = {disable|enable}

That seems reasonable. It's three options, but no normal use case would
need to set more than two, because channel binding forces scram-sha-
256-plus.

Regards,
    Jeff Davis

On 8/9/19 11:51 AM, Jeff Davis wrote:
> On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote:
>> Having an 'any' option, as mentioned before, could be an alternative
>> though.
>
> ...
>
>> I agree with the point that there isn't any guarantee that it'll
>> always
>> be clear-cut as to which of two methods is "better".
>>
>> From a user perspective, it seems like the main things are "don't
>> send
>> my password in the clear to the server", and "require channel binding
>> to
>> prove there isn't a MITM".  I have to admit that I like the idea of
>> requiring scram to be used and not allowing md5 though.
>
> So it seems like we are leaning toward:
>
>    password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha-
> 256-plus}[,...]

First, thanks for proposing / working on this, I like the idea! :) Happy
to test/review.

As long as this one can handle the current upgrade path that's in place
for going from md5 to SCRAM (which AIUI it should) this makes sense to
me. As stated above, there is a clear hierarchy.

I would almost argue that "plaintext" shouldn't even be an option...if
you have "any" set (arguably default?) then plaintext is available. With
our currently supported versions + driver ecosystem, I hope no one needs
to support a forced plaintext setup.

>
> Or maybe:
>
>    channel_binding = {disable|prefer|require}
>    password_plaintext = {disable|enable}
>    password_md5 = {disable|enable}
>
> That seems reasonable. It's three options, but no normal use case would
> need to set more than two, because channel binding forces scram-sha-
> 256-plus.

Seems to be a lot to configure. I'm more of a fan of the previous
method; it'd work nicely with how we've presently defined things and
should be easy to put into a DSN/URI/env variable.

Thanks,

Jonathan

On 09/08/2019 23:27, Jonathan S. Katz wrote:
> On 8/9/19 11:51 AM, Jeff Davis wrote:
>> On Fri, 2019-08-09 at 09:28 -0400, Stephen Frost wrote:
>>> Having an 'any' option, as mentioned before, could be an alternative
>>> though.
>>
>> ...
>>
>>> I agree with the point that there isn't any guarantee that it'll
>>> always
>>> be clear-cut as to which of two methods is "better".
>>>
>>>  From a user perspective, it seems like the main things are "don't
>>> send
>>> my password in the clear to the server", and "require channel binding
>>> to
>>> prove there isn't a MITM".  I have to admit that I like the idea of
>>> requiring scram to be used and not allowing md5 though.
>>
>> So it seems like we are leaning toward:
>>
>>     password_protocol = any | {plaintext,md5,scram-sha-256,scram-sha-
>> 256-plus}[,...]
> 
> First, thanks for proposing / working on this, I like the idea! :) Happy
> to test/review.
> 
> As long as this one can handle the current upgrade path that's in place
> for going from md5 to SCRAM (which AIUI it should) this makes sense to
> me. As stated above, there is a clear hierarchy.
> 
> I would almost argue that "plaintext" shouldn't even be an option...if
> you have "any" set (arguably default?) then plaintext is available. With
> our currently supported versions + driver ecosystem, I hope no one needs
> to support a forced plaintext setup.

Keep in mind that RADIUS, LDAP and PAM authentication methods are 
'plaintext' over the wire. It's not that bad, when used with 
sslmode=verify-ca/full.

>> Or maybe:
>>
>>     channel_binding = {disable|prefer|require}
>>     password_plaintext = {disable|enable}
>>     password_md5 = {disable|enable}
>>
>> That seems reasonable. It's three options, but no normal use case would
>> need to set more than two, because channel binding forces scram-sha-
>> 256-plus.
> 
> Seems to be a lot to configure. I'm more of a fan of the previous
> method; it'd work nicely with how we've presently defined things and
> should be easy to put into a DSN/URI/env variable.

This is a multi-dimensional problem. "channel_binding=require" is one 
way to prevent MITM attacks, but sslmode=verify-ca is another. (Does 
Kerberos also prevent MITM?) Or you might want to enable plaintext 
passwords over SSL, but not without SSL.

I think we'll need something like the 'ssl_ciphers' GUC, where you can 
choose from a few reasonable default rules, but also enable/disable 
specific methods:

# anything goes (the default)
auth_methods = 'ANY'

# Disable plaintext password authentication. Anything else is accepted.
auth_methods = '-password'

# Only authentication methods that protect from
# Man-in-the-Middle attacks. This allows anything if the server's SSL
# certificate can be verified, and otherwise only SCRAM with
# channel binding
auth_methods = 'MITM'

# The same, but disable plaintext passwords and md5 altogether
auth_methods = 'MITM, -password, -md5'


I'm tempted to also allow 'SSL' and 'SSL-verify-full' as part of the 
same string, so that you could configure everything related to 
connection security in the same option. Not sure if that would make 
things simpler for users, or create more confusion.

- Heikki

On Fri, 2019-08-09 at 16:27 -0400, Jonathan S. Katz wrote:
> Seems to be a lot to configure. I'm more of a fan of the previous
> method; it'd work nicely with how we've presently defined things and
> should be easy to put into a DSN/URI/env variable.

Proposals on the table:

1. Hierarchical semantics, where you specify the least-secure
acceptable method:

  password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}

2. Comma-list approach, where you specify exactly which protocols are
acceptable, or "any" to mean that we don't care.

3. three-setting approach:

  channel_binding = {disable|prefer|require}
  password_plaintext = {disable|enable}
  password_md5 = {disable|enable}

It looks like Jonathan prefers #1.

#1 seems direct and clearly applies today, and corresponds to auth
methods on the server side.

I'm not a fan of #2, it seems likely to result in a bunch of clients
with overly-specific lists of things with long names that can never
really go away.

#3 is a little more abstract, but also seems more future-proof, and may
tie in to what Stephen is talking about with respect to controlling
auth methods from the client, or moving more protocols within SASL.

Regards,
    Jeff Davis

On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote:
> This is a multi-dimensional problem. "channel_binding=require" is
> one 
> way to prevent MITM attacks, but sslmode=verify-ca is another. (Does 
> Kerberos also prevent MITM?) Or you might want to enable plaintext 
> passwords over SSL, but not without SSL.
> 
> I think we'll need something like the 'ssl_ciphers' GUC, where you
> can 
> choose from a few reasonable default rules, but also enable/disable 
> specific methods:

..

> auth_methods = 'MITM, -password, -md5'

Keep in mind this is client configuration, so something reasonable in
postgresql.conf might not be so reasonable in the form:

postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20-
password%2C%20-md5

Another thing to consider is that there's less control configuring on
the client than on the server. The server will send at most one
authentication request based on its own rules, and all the client can
do is either answer it, or disconnect. And the SSL stuff all happens
before that, and won't use an authentication request message at all.

Some protocols allow negotiation within them, like SASL, which gives
the client a bit more freedom. But FE/BE doesn't allow for arbitrary
subsets of authentication methods to be negoitated between client and
server, so I'm worried trying to express it that way will just lead to
clients that break when you upgrade your server.

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote:
> > auth_methods = 'MITM, -password, -md5'
>
> Keep in mind this is client configuration, so something reasonable in
> postgresql.conf might not be so reasonable in the form:

Yeah, that's a really good point.

> postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20-
> password%2C%20-md5
>
> Another thing to consider is that there's less control configuring on
> the client than on the server. The server will send at most one
> authentication request based on its own rules, and all the client can
> do is either answer it, or disconnect. And the SSL stuff all happens
> before that, and won't use an authentication request message at all.

Note that GSSAPI Encryption works the same as SSL in this regard.

Thanks,

Stephen

On Sat, 2019-08-10 at 10:24 +0800, Craig Ringer wrote:
> Before we go too far along with this, lets look at how other
> established protocols do things and the flaws that've been discovered
> in their approaches. If this isn't done with extreme care then
> there's a large risk of negating the benefits offered by adopting
> recent things like SCRAM.

Agreed. I'm happy to hear any proposals better informed by history.

> Frankly I kind of wish we could just use SASL, but there are many
> (many) reasons no to.

I'm curious what the reasons are not to use SASL; do you have a
reference?

> Off the top of my head I can think of these risks:
> 
> * Protocols that allow naïve pre-auth client/server auth negotiation
> (e.g. by finding the overlap in exchanged supported auth-mode lists)
> are subject to MiTM downgrade attacks where the attacker filters out
> protocols it cannot intercept and break from the proposed
> alternatives.

We already have the downgrade problem. That's what I'm trying to solve.

> * Protocols that specify a hierarchy tend to be inflexible and result
> in hard to predict auth mode selections as the options grow. If my
> app wants GSSAPI or SuperStrongAuth but doesn't accept SSPI, and the
> hierarchy is  GSSAPI > SSPI > SuperStrongAuth, it has to fall back to
> a disconnect and retry model like now.

What do you mean "disconnect and retry model"?

I agree that hierarchies are unweildly as the options grow. Then again,
as options grow, we need new versions of the client to support them,
and those new versions might offer more flexible ways to choose between
them.

Of course, we should try to think ahead to avoid needing to constantly
change client connection syntax, but I'm just pointing out that it's
not a one-way door.

> * Protocols that announce supported auth methods before any kind of
> trust is established make life easier for vulnerability scanners and
> worms

This discussion is about the client so I don't see how vulnerability
scanners are relevant.

Regards,
    Jeff Davis

On 8/9/19 7:54 PM, Jeff Davis wrote:
> On Sat, 2019-08-10 at 00:17 +0300, Heikki Linnakangas wrote:
>> This is a multi-dimensional problem. "channel_binding=require" is
>> one
>> way to prevent MITM attacks, but sslmode=verify-ca is another. (Does
>> Kerberos also prevent MITM?) Or you might want to enable plaintext
>> passwords over SSL, but not without SSL.
>>
>> I think we'll need something like the 'ssl_ciphers' GUC, where you
>> can
>> choose from a few reasonable default rules, but also enable/disable
>> specific methods:
>
> ..
>
>> auth_methods = 'MITM, -password, -md5'
>
> Keep in mind this is client configuration, so something reasonable in
> postgresql.conf might not be so reasonable in the form:
>
> postgresql://foo:secret@myhost/mydb?auth_methods=MITM%2C%20-
> password%2C%20-md5

Yeah, and I do agree it is a multi-dimensional problem, but the context
in which I gave my opinion was for the password authentication methods
that PostgreSQL supports natively, i.e. not requiring a 3rd party to
arbitrate via GSSAPI, LDAP etc.

That said, I dove into the code a bit more to look at the behavior
specifically with LDAP, which as described does send back a request for
"AuthenticationCleartextPassword"

If we go with the client sending up a "password_protocol" that is not
plaintext, and the server only provides LDAP authentication, does the
client close the connection? I would say yes.

(And as such, I would also consider adding "plaintext" back to the list,
just to have the explicit option).

The other question I have is that do we have it occur in the
hierarchical manner, i.e. "md5 or better?" I would also say yes to that,
we would just need to clearly document that. Perhaps we adopt a similar
name to "sslmode" e.g. "password_protocol_mode" but that can be debated :)

> Another thing to consider is that there's less control configuring on
> the client than on the server. The server will send at most one
> authentication request based on its own rules, and all the client can
> do is either answer it, or disconnect. And the SSL stuff all happens
> before that, and won't use an authentication request message at all.

Yes. Using the LDAP example above, the client also needs some general
awareness of how it can connect to the server, e.g. "You may want
scram-sha-256 but authentication occurs over LDAP, so don't stop
requesting scram-sha-256!" That said, part of that is a human problem:
it's up to the server administrator to inform clients how they can
connect to PostgreSQL.

> Some protocols allow negotiation within them, like SASL, which gives
> the client a bit more freedom. But FE/BE doesn't allow for arbitrary
> subsets of authentication methods to be negoitated between client and
> server, so I'm worried trying to express it that way will just lead to
> clients that break when you upgrade your server.

Agreed. I see this as a way of a client saying "Hey, I really want to
authenticate with scram-sha-256 or better, so if you don't let me do
that, I'm out." In addition to ensuring it uses the client's desired
password protocol, this could be helpful for testing that the
appropriate authentication rules are set in a server, e.g. one that is
rolling out SCRAM authentication.

And as Heikki mentions, there are other protections a client can use,
e.g. verify-ca/full, to guard against eavesdropping, MITM etc.

Jonathan

On 2019-08-09 23:56, Jeff Davis wrote:
> 1. Hierarchical semantics, where you specify the least-secure
> acceptable method:
> 
>   password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}

What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are
added?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 8/11/19 1:00 PM, Peter Eisentraut wrote:
> On 2019-08-09 23:56, Jeff Davis wrote:
>> 1. Hierarchical semantics, where you specify the least-secure
>> acceptable method:
>>
>>   password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}
>
> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are
> added?

password_protocol =
{any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}?

I'd put one length of digest over another, but I'd still rank a method
that uses channel binding has more protections than one that does not.

Jonathan

On 2019-08-11 21:46, Jonathan S. Katz wrote:
> On 8/11/19 1:00 PM, Peter Eisentraut wrote:
>> On 2019-08-09 23:56, Jeff Davis wrote:
>>> 1. Hierarchical semantics, where you specify the least-secure
>>> acceptable method:
>>>
>>>   password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}
>>
>> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are
>> added?
> 
> password_protocol =
> {any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}?
> 
> I'd put one length of digest over another, but I'd still rank a method
> that uses channel binding has more protections than one that does not.

Sure, but the opposite opinion is also possible.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 8/11/19 3:56 PM, Peter Eisentraut wrote:
> On 2019-08-11 21:46, Jonathan S. Katz wrote:
>> On 8/11/19 1:00 PM, Peter Eisentraut wrote:
>>> On 2019-08-09 23:56, Jeff Davis wrote:
>>>> 1. Hierarchical semantics, where you specify the least-secure
>>>> acceptable method:
>>>>
>>>>   password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}
>>>
>>> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus are
>>> added?
>>
>> password_protocol =
>> {any,md5,scram-sha-256,scram-sha-512,scram-sha-256-plus,scram-sha-512-plus}?
>>
>> I'd put one length of digest over another, but I'd still rank a method
>> that uses channel binding has more protections than one that does not.
>
> Sure, but the opposite opinion is also possible.

That's true, and when originally started composing my note I had it as
(256,512,256-plus,512-plus).

But upon further reflection, the reason I ranked the digest-plus methods
above the digest methods is that there is any additional requirement
imposed by them. The digest methods could be invoked either with/without
TLS, whereas the digest-plus methods *must* use TLS. As such, 256-plus
is explicitly asking for an additional security parameter over 512, i.e.
transmission over TLS, so even if it's a smaller digest, it has the
additional channel binding requirement.

Jonathan

On Sun, 2019-08-11 at 19:00 +0200, Peter Eisentraut wrote:
> On 2019-08-09 23:56, Jeff Davis wrote:
> > 1. Hierarchical semantics, where you specify the least-secure
> > acceptable method:
> > 
> >   password_protocol = {any,md5,scram-sha-256,scram-sha-256-plus}
> 
> What would the hierarchy be if scram-sha-512 and scram-sha-512-plus
> are
> added?


https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com

The weakness of proposal #1 is that it's not very "future-proof" and we
would likely need to change something about it later when we support
new methods. That wouldn't break clients, but it would be annoying to
need to support some old syntax and some new syntax for the connection
parameters.

Proposal #3 does not have this weakness. When we add sha-512, we could
also add a parameter to specify that the client requires a certain hash
algorithm for SCRAM.

Do you favor that existing proposal #3, or are you proposing a fourth
option?

Regards,
    Jeff Davis

On 2019-08-12 18:02, Jeff Davis wrote:
> https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com
> 
> The weakness of proposal #1 is that it's not very "future-proof" and we
> would likely need to change something about it later when we support
> new methods. That wouldn't break clients, but it would be annoying to
> need to support some old syntax and some new syntax for the connection
> parameters.
> 
> Proposal #3 does not have this weakness. When we add sha-512, we could
> also add a parameter to specify that the client requires a certain hash
> algorithm for SCRAM.
> 
> Do you favor that existing proposal #3, or are you proposing a fourth
> option?

In this context, I would prefer #2, but I would expand that to cover all
authentication methods, not only password methods.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Greetings,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 2019-08-12 18:02, Jeff Davis wrote:
> > https://postgr.es/m/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com
> >
> > The weakness of proposal #1 is that it's not very "future-proof" and we
> > would likely need to change something about it later when we support
> > new methods. That wouldn't break clients, but it would be annoying to
> > need to support some old syntax and some new syntax for the connection
> > parameters.
> >
> > Proposal #3 does not have this weakness. When we add sha-512, we could
> > also add a parameter to specify that the client requires a certain hash
> > algorithm for SCRAM.
> >
> > Do you favor that existing proposal #3, or are you proposing a fourth
> > option?
>
> In this context, I would prefer #2, but I would expand that to cover all
> authentication methods, not only password methods.

I'm not really thrilled with approach #2 because it means the user
will have to know which of the PG authentication methods involve, eg,
sending the password in the clear to the server, and which don't, if
what they're really looking for is "don't send my password in the clear
to the server" which seems like a really useful and sensible thing to
ask for.

It also ends up not being very future-proof either, since a user who is
fine with scram-sha-256-plus will probably also be ok with
scram-sha-512-plus, should we ever implement it.

Not to mention that, at least at the moment, we don't let users pick
authentication methods with that kind of specificity on the server side
(how do you require channel binding..?), so the set of "authentication
methods" on the client side and those on the server side end up being
different sets, which strikes me as awfully confusing...

Or did I misunderstand what you were suggesting here wrt "all
authentication methods"?

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> I'm not really thrilled with approach #2 because it means the user
> will have to know which of the PG authentication methods involve, eg,
> sending the password in the clear to the server, and which don't, if
> what they're really looking for is "don't send my password in the clear
> to the server" which seems like a really useful and sensible thing to
> ask for.

What problem do we actually need to solve here?

If the known use-case is just "don't send my password in the clear",
maybe we should just change libpq to refuse to do that, ie reject
plain-password auth methods unless SSL is on (except maybe over
unix sockets?).  Or invent a bool connection option that enables
exactly that.

I'm not really convinced that there is a use-case for client side
specification of allowed auth methods beyond that.  In the end,
if you don't trust the server you're connecting to to handle your
password with reasonable safety, you have got bigger problems than
this one.  And we already have coverage for MITM problems (or if
we don't, this sideshow isn't fixing it).

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > I'm not really thrilled with approach #2 because it means the user
> > will have to know which of the PG authentication methods involve, eg,
> > sending the password in the clear to the server, and which don't, if
> > what they're really looking for is "don't send my password in the clear
> > to the server" which seems like a really useful and sensible thing to
> > ask for.
>
> What problem do we actually need to solve here?
>
> If the known use-case is just "don't send my password in the clear",
> maybe we should just change libpq to refuse to do that, ie reject
> plain-password auth methods unless SSL is on (except maybe over
> unix sockets?).  Or invent a bool connection option that enables
> exactly that.

Right, inventing a bool connection for it was discussed up-thread and
seems like a reasonable idea to me (note: we should absolutely allow the
user to refuse to send the password to the server even over SSL, if they
would prefer to not do so).

> I'm not really convinced that there is a use-case for client side
> specification of allowed auth methods beyond that.  In the end,
> if you don't trust the server you're connecting to to handle your
> password with reasonable safety, you have got bigger problems than
> this one.  And we already have coverage for MITM problems (or if
> we don't, this sideshow isn't fixing it).

Uh, no, we really don't have MITM protection in certain cases, which is
exactly what channel-binding is intended to address, but we can't have
the "server" be able to say "oh, well, I don't support channel binding"
and have the client go "oh, ok, that's just fine, we won't use it then"-
that's a downgrade attack.

What was suggest up-thread to deal with that downgrade risk was a clear
connection option along the lines of "require channel binding" to
prevent that kind of a MITM downgrade attack from working.

I could possibly see some value in a client-side option along the lines
of "only authenticate using GSSAPI", which could prevent some users from
being fooled into sending their PW to a malicious server.  GSSAPI does
prevent MITM attacks (as much as it's able to anyway- each key is
specific to a particular server, so you'd have to have the specific
server's key in order to become a MITM), but if the server says "we
don't do GSSAPI, we do password, please give us your password" then psql
will happily go along with that even in an otherwise properly set up
GSSAPI environment.

Requiring GSSAPI Encryption on the client side should prevent that
though, as of v12, since psql will just refuse if the server claims to
not support GSSAPI Encryption.

Thanks,

Stephen

On 2019-08-12 19:26, Tom Lane wrote:
> What problem do we actually need to solve here?
> 
> If the known use-case is just "don't send my password in the clear",
> maybe we should just change libpq to refuse to do that, ie reject
> plain-password auth methods unless SSL is on (except maybe over
> unix sockets?).  Or invent a bool connection option that enables
> exactly that.

There are several overlapping problems:

1) A downgrade attack by a malicious server.  The server can collect
passwords from unsuspecting clients by just requesting some weak
authentication like plain-text or md5.  This can currently be worked
around by using SSL with server verification, except when considering
the kind of attack that channel binding is supposed to address.

2) A downgrade attack to evade channel binding.  This cannot currently
be worked around.

3) A user not wanting to expose a weakly hashed password to the
(otherwise trusted) server.  This cannot currently be done.

4) A user not wanting to send a password in plain text over the wire.
This can currently be done by requiring SSL.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Aug 09, 2019 at 09:28:50AM -0400, Stephen Frost wrote:
> I don't really care for auth_protocol as that's pretty close to
> "auth_method" and that isn't what we're talking about here- this isn't
> the user picking the auth method, per se, but rather saying which of the
> password-based mechanisms for communicating that the user knows the
> password is acceptable.  Letting users choose which auth methods are
> allowed might also be interesting (as in- we are in a Kerberized
> environment and therefore no client should ever be using any auth method
> except GSS, could be a reasonable ask) but it's not the same thing.
>
> What restriction are you suggesting here wrt krb5..?

What I suggested in this previous set of emails is if it would make
sense to extend what libpq can restrict at authentication time to not
only be password-based authentication methods, but also if we could
have a connection parameter allowing us to say "please I want krb5/gss
and nothing else".  My point is that password-based authentication is
only one portion of the problem as what we are looking at is applying
a filtering on AUTH_REQ messages that libpq receives from the server
(SCRAM with and without channel binding is an exception as that's
handled as part of the SASL set of messages), but at a high level we
are going to need a filtering of the first authentication message
received anyway.

But that's also basically what you outline in this previous paragraph
of yours.
--
Michael

On Mon, Aug 12, 2019 at 07:05:08PM +0200, Peter Eisentraut wrote:
> In this context, I would prefer #2, but I would expand that to cover all
> authentication methods, not only password methods.

I tend to prefer #2 as well and that's the kind of approach we were
tending to agree on when we discussed this issue during the v11 beta
for the downgrade issues with libpq.  And as you say extend it so as
we can apply filtering of more AUTH_REQ requests, inclusing GSS and
krb5.
--
Michael

On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote:
> I tend to prefer #2 as well and that's the kind of approach we were
> tending to agree on when we discussed this issue during the v11 beta
> for the downgrade issues with libpq.  And as you say extend it so as
> we can apply filtering of more AUTH_REQ requests, inclusing GSS and
> krb5.

Can you please offer a concrete proposal? I know the proposals I've put
out aren't perfect (otherwise there wouldn't be three of them), so if
you have something better, please share.

Regards,
    Jeff Davis

On 8/13/19 12:25 PM, Jeff Davis wrote:
> On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote:
>> I tend to prefer #2 as well and that's the kind of approach we were
>> tending to agree on when we discussed this issue during the v11 beta
>> for the downgrade issues with libpq.  And as you say extend it so as
>> we can apply filtering of more AUTH_REQ requests, inclusing GSS and
>> krb5.
>
> Can you please offer a concrete proposal? I know the proposals I've put
> out aren't perfect (otherwise there wouldn't be three of them), so if
> you have something better, please share.

I think all of them get at the same thing, i.e. specifying which
password protocol you want to use, and a lot of it is a matter of how
much onus we want to put on the user.

Back to the thee proposals[1], I've warmed up to #3 a bit. I do think it
puts more onus on the client to set the correct knobs to get the desired
outcome, but what I like is the specific `channel_binding=require`
attribute.

However, I don't think it's completely future proof to adding a new hash
digest. If we wanted to prevent someone from using scram-sha-256 in a
scram-sha-512 world, we'd likely need an option for that.

Alternatively, we could combine 2 & 3, e.g.:

  channel_binding = {disable|prefer|require}

  # comma-separated list of protocols that are ok to the user, remove
  # ones you don't want. empty means all is ok
  password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256-plus"

If the client selects "channel_binding=require" but does not include a
protocol that supports it, we should error. Likewise, if the client does
something like "channel_binding=require" and
"password_protocol=scram-sha-256,scram-sha-256-plus" but the server
refuses to do channel binding, we should error.

I think this gives us both future-proofing against newer password digest
methods + the fix for the downgrade issue.

I would not be opposed to extending "password_protocol" to read
"auth_protocol" or the like and work for everything covered in AUTH_REQ,
but I would need to think about it some more.

Thanks,

Jonathan

[1]
https://www.postgresql.org/message-id/daf0017a1a5c2caabf88a4e00f66b4fcbdfeccad.camel%40j-davis.com

On Tue, 2019-08-13 at 16:51 -0400, Jonathan S. Katz wrote:
> Alternatively, we could combine 2 & 3, e.g.:
> 
>   channel_binding = {disable|prefer|require}
> 
>   # comma-separated list of protocols that are ok to the user, remove
>   # ones you don't want. empty means all is ok
>   password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256-
> plus"

I still feel like lists are over-specifying things. Let me step back
and offer an MVP of a single new parameter:

  channel_binding={prefer|require}

And has a lot of benefits:
    * solves the immediate need to make channel binding useful, which
is a really nice feature
    * compatible with most of the other proposals we're considering, so
we can always extend it when we have a better understanding and
consensus
    * clear purpose for the user
    * doesn't introduce new concepts that might be confusing to the
user, like SASL or the use of "-plus" to mean "with channel binding"
    * guides users toward the good practice of using SSL and SCRAM
    * simple to implement

The other use cases are less clear to me, and seem less urgent.

Regards,
    Jeff Davis

On 8/13/19 6:25 PM, Jeff Davis wrote:
> On Tue, 2019-08-13 at 16:51 -0400, Jonathan S. Katz wrote:
>> Alternatively, we could combine 2 & 3, e.g.:
>>
>>   channel_binding = {disable|prefer|require}
>>
>>   # comma-separated list of protocols that are ok to the user, remove
>>   # ones you don't want. empty means all is ok
>>   password_protocol = "plaintext,md5,scram-sha-256,scram-sha-256-
>> plus"
>
> I still feel like lists are over-specifying things. Let me step back
> and offer an MVP of a single new parameter:
>
>   channel_binding={prefer|require}
>
> And has a lot of benefits:
>     * solves the immediate need to make channel binding useful, which
> is a really nice feature
>     * compatible with most of the other proposals we're considering, so
> we can always extend it when we have a better understanding and
> consensus
>     * clear purpose for the user
>     * doesn't introduce new concepts that might be confusing to the
> user, like SASL or the use of "-plus" to mean "with channel binding"
>     * guides users toward the good practice of using SSL and SCRAM
>     * simple to implement

+1; I agree with your overall argument. The only thing I debate is if we
want to have an explicit "disable" option. Looking at the negotiation
standard[1] specified for channel binding with SCRAM, I don't think we
need an explicit disable option. I can't think of any good use cases for
"disable" off the top of my head either. The only thing is it would be
consistent with some of our other parameters in terms of having an
explicit "opt-out."

Jonathan

[1] https://tools.ietf.org/html/rfc5802#section-6

On Tue, Aug 13, 2019 at 04:51:57PM -0400, Jonathan S. Katz wrote:
> On 8/13/19 12:25 PM, Jeff Davis wrote:
>> On Tue, 2019-08-13 at 11:56 +0900, Michael Paquier wrote:
>>> I tend to prefer #2 as well and that's the kind of approach we were
>>> tending to agree on when we discussed this issue during the v11 beta
>>> for the downgrade issues with libpq.  And as you say extend it so as
>>> we can apply filtering of more AUTH_REQ requests, inclusing GSS and
>>> krb5.
>>
>> Can you please offer a concrete proposal? I know the proposals I've put
>> out aren't perfect (otherwise there wouldn't be three of them), so if
>> you have something better, please share.
>
> I think all of them get at the same thing, i.e. specifying which
> password protocol you want to use, and a lot of it is a matter of how
> much onus we want to put on the user.

What I got in mind was a comma-separated list of authorized protocols
which can be specified as a connection parameter, which extends to all
the types of AUTH_REQ requests that libpq can understand, plus an
extra for channel binding.  I also liked the idea mentioned upthread
of "any" to be an alias to authorize everything, which should be the
default.  So you basically get at that:
auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256-plus,krb5,gss,sspi}

So from an implementation point of view, just using bit flags would
make things rather clean.

> Back to the thee proposals[1], I've warmed up to #3 a bit. I do think it
> puts more onus on the client to set the correct knobs to get the desired
> outcome, but what I like is the specific `channel_binding=require`
> attribute.

I could see a point in separating the channel binding part into a
second parameter though.  We don't have (at least yet) an hba option
to allow only channel binding with scram, so a one-one mapping with
the elements of the connection parameter brings some consistency.

> If the client selects "channel_binding=require" but does not include a
> protocol that supports it, we should error.

Yep.

> Likewise, if the client does
> something like "channel_binding=require" and
> "password_protocol=scram-sha-256,scram-sha-256-plus" but the server
> refuses to do channel binding, we should error.

If using a second parameter to control channel binding requirement, I
don't think that there is any point in keeping scram-sha-256-plus in
password_protocol.

> I would not be opposed to extending "password_protocol" to read
> "auth_protocol" or the like and work for everything covered in AUTH_REQ,
> but I would need to think about it some more.

And for this one I would like to push for not only having
password-based methods considered.
--
Michael

On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote:
> What I got in mind was a comma-separated list of authorized protocols
> which can be specified as a connection parameter, which extends to
> all
> the types of AUTH_REQ requests that libpq can understand, plus an
> extra for channel binding.  I also liked the idea mentioned upthread
> of "any" to be an alias to authorize everything, which should be the
> default.  So you basically get at that:
> auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256-
> plus,krb5,gss,sspi}

What about something corresponding to the auth methods "trust" and
"cert", where no authentication request is sent? That's a funny case,
because the server trusts the client; but that doesn't imply that the
client trusts the server.

This is another reason I don't really like the list. It's impossible to
make it cleanly map to the auth methods, and there are a few ways it
could be confusing to the users.

Given that we all pretty much agree on the need for the separate
channel_binding param, the question is whether we want to (a) address
additional use cases with specific parameters that also justify
themselves; or (b) have a generic list that is supposed to solve many
future use cases.

I vote (a). With (b), the generic list is likely to cause more
confusion, ugliness, and clients that break needlessly in the future.

Regards,
    Jeff Davis

Greetings,

* Jeff Davis (pgsql@j-davis.com) wrote:
> On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote:
> > What I got in mind was a comma-separated list of authorized protocols
> > which can be specified as a connection parameter, which extends to
> > all
> > the types of AUTH_REQ requests that libpq can understand, plus an
> > extra for channel binding.  I also liked the idea mentioned upthread
> > of "any" to be an alias to authorize everything, which should be the
> > default.  So you basically get at that:
> > auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256-
> > plus,krb5,gss,sspi}
>
> What about something corresponding to the auth methods "trust" and
> "cert", where no authentication request is sent? That's a funny case,
> because the server trusts the client; but that doesn't imply that the
> client trusts the server.

I agree that "trust" is odd.  If you want my 2c, we should have to
explicitly *enable* that for psql, otherwise there's the potential that
a MITM could perform a downgrade attack to "trust" and while that might
not expose a user's password, it could expose other information that the
client ends up sending (INSERTs, UPDATEs, etc).

When it comes to "cert"- there is certainly an authentication that
happens and we already have options in psql/libpq to require validation
of the server.  If users want that, they should enable it (I wish we
could make it the default too but that's a different discussion...).

> This is another reason I don't really like the list. It's impossible to
> make it cleanly map to the auth methods, and there are a few ways it
> could be confusing to the users.

I agree with these concerns, just to be clear.

> Given that we all pretty much agree on the need for the separate
> channel_binding param, the question is whether we want to (a) address
> additional use cases with specific parameters that also justify
> themselves; or (b) have a generic list that is supposed to solve many
> future use cases.
>
> I vote (a). With (b), the generic list is likely to cause more
> confusion, ugliness, and clients that break needlessly in the future.

Admittedly, one doesn't preclude the other, and so we could move forward
with the channel binding param, and that's fine- but I seriously hope
that someone finds time to work on further improving the ability for
clients to control what happens during authentication as this, imv
anyway, is an area that we are weak in and it'd be great to improve on
it.

Thanks,

Stephen

On 8/15/19 9:28 PM, Stephen Frost wrote:
> Greetings,
>
> * Jeff Davis (pgsql@j-davis.com) wrote:
>> On Wed, 2019-08-14 at 11:38 +0900, Michael Paquier wrote:
>>> What I got in mind was a comma-separated list of authorized protocols
>>> which can be specified as a connection parameter, which extends to
>>> all
>>> the types of AUTH_REQ requests that libpq can understand, plus an
>>> extra for channel binding.  I also liked the idea mentioned upthread
>>> of "any" to be an alias to authorize everything, which should be the
>>> default.  So you basically get at that:
>>> auth_protocol = {any,password,md5,scram-sha-256,scram-sha-256-
>>> plus,krb5,gss,sspi}
>>
>> What about something corresponding to the auth methods "trust" and
>> "cert", where no authentication request is sent? That's a funny case,
>> because the server trusts the client; but that doesn't imply that the
>> client trusts the server.
>
> I agree that "trust" is odd.  If you want my 2c, we should have to
> explicitly *enable* that for psql, otherwise there's the potential that
> a MITM could perform a downgrade attack to "trust" and while that might
> not expose a user's password, it could expose other information that the
> client ends up sending (INSERTs, UPDATEs, etc).
>
> When it comes to "cert"- there is certainly an authentication that
> happens and we already have options in psql/libpq to require validation
> of the server.  If users want that, they should enable it (I wish we
> could make it the default too but that's a different discussion...).
>
>> This is another reason I don't really like the list. It's impossible to
>> make it cleanly map to the auth methods, and there are a few ways it
>> could be confusing to the users.
>
> I agree with these concerns, just to be clear.

+1.

>
>> Given that we all pretty much agree on the need for the separate
>> channel_binding param, the question is whether we want to (a) address
>> additional use cases with specific parameters that also justify
>> themselves; or (b) have a generic list that is supposed to solve many
>> future use cases.
>>
>> I vote (a). With (b), the generic list is likely to cause more
>> confusion, ugliness, and clients that break needlessly in the future.
>
> Admittedly, one doesn't preclude the other, and so we could move forward
> with the channel binding param, and that's fine- but I seriously hope
> that someone finds time to work on further improving the ability for
> clients to control what happens during authentication as this, imv
> anyway, is an area that we are weak in and it'd be great to improve on
> it.

To be pedantic, +1 on the channel_binding param.

I do agree with option (a), but we should narrow down what that means
for this iteration.

I do see "password_protocol" making sense as a comma-separated list of
options e.g. {plaintext, md5, scram-sha-256}. I would ask if
scram-sha-256-plus makes the list if we have the channel_binding param?

If channel_binding = require, it would essentially ignore any non-plus
options in password_protocol and require scram-sha-256-plus. In a future
scram-sha-512 world, then having scram-sha-256-plus or
scram-sha-512-plus options in "password_protocol" then could be
necessary based on what the user would prefer or require in their
application.

So if we do add a "password_protocol" parameter, then we likely need to
include the -plus's.

I think this is also fairly easy for a user to configure. Some scenarios
scenarios I can see for this are:

1. The user requiring channel binding, so only "channel_binding=require"
 is set.

2. A PostgreSQL cluster transitioning between SCRAM + MD5, and the user
setting password_protocol="scram-sha-256" to guarantee md5 auth does not
take place.

3. A user wanting to ensure a stronger method is used, with some
combination of the scram methods or md5, i.e. ensuring plaintext is not
used.

We would need to provide documentation around the types of password
validation methods are used for the external validators (e.g. LDAP) so
the user's known what to expect if their server is using those methods.

Jonathan

On Fri, Aug 16, 2019 at 02:11:57PM -0400, Jonathan S. Katz wrote:
> To be pedantic, +1 on the channel_binding param.

Seems like we are moving in this direction then.  I don't object to
the introduction of this parameter.  We would likely want to do
something for downgrade attacks in other cases where channel binding
is not used, still there is verify-ca/full even in this case which
offer similar protections for MITM and eavesdropping.

> I would ask if scram-sha-256-plus makes the list if we have the
> channel_binding param?

No in my opinion.

> If channel_binding = require, it would essentially ignore any non-plus
> options in password_protocol and require scram-sha-256-plus. In a future
> scram-sha-512 world, then having scram-sha-256-plus or
> scram-sha-512-plus options in "password_protocol" then could be
> necessary based on what the user would prefer or require in their
> application.

Not including scram-sha-512-plus or scram-sha-256-plus in the
comma-separated list would be a problem for users willing to give for
example scram-sha-256,scram-sha-512-plus as an authorized list of
protocols but I don't think that it makes much sense as they basically
require an SSL connection for tls-server-end-point per the second
element.
--
Michael

On Mon, 2019-08-19 at 14:51 +0900, Michael Paquier wrote:
> On Fri, Aug 16, 2019 at 02:11:57PM -0400, Jonathan S. Katz wrote:
> > To be pedantic, +1 on the channel_binding param.
> 
> Seems like we are moving in this direction then.  I don't object to
> the introduction of this parameter.

OK, new patch attached. Seems like everyone is in agreement that we
need a channel_binding param.

Regards,
    Jeff Davis

On Tue, Aug 20, 2019 at 07:09:25PM -0700, Jeff Davis wrote:
> OK, new patch attached. Seems like everyone is in agreement that we
> need a channel_binding param.

+      <para>
+        A setting of <literal>require</literal> means that the connection must
+        employ channel binding; and that the client will not respond to
+        requests from the server for cleartext password or MD5
+        authentication. The default setting is <literal>prefer</literal>,
+        which employs channel binding if available.
+      </para>
This counts for other auth requests as well like krb5, no?  I think
that we should also add the "disable" flavor for symmetry, and
also...

+#define DefaultChannelBinding  "prefer"
If the build of Postgres does not support SSL (USE_SSL is not
defined), I think that DefaultChannelBinding should be "disable".
That would make things consistent with sslmode.

+        with <productname>PostgreSQL</productname> 11.0 or later servers using
Here I would use PostgreSQL 11, only mentioning the major version as
it was also available at beta time.

        case AUTH_REQ_OK:
+           if (conn->channel_binding[0] == 'r' && !conn->channel_bound)
+           {
+               printfPQExpBuffer(&conn->errorMessage,
+                                 libpq_gettext("Channel binding required but not offered by server\n"));
+               return STATUS_ERROR;
+           }
Doing the filtering at the time of AUTH_REQ_OK is necessary for
"trust", but shouldn't this be done as well earlier for other request
types?  This could save round-trips to the server if we know that an
exchange begins with a protocol which will never satisfy this
request, saving efforts for a client doomed to fail after the first
AUTH_REQ received.  That would be the case for all AUTH_REQ, except
the SASL ones and of course AUTH_REQ_OK.

Could you please add negative tests in src/test/authentication/?  What
could be covered there is that the case where "prefer" (and
"disable"?) is defined then the authentication is able to go through,
and that with "require" we get a proper failure as SSL is not used.
Tests in src/test/ssl/ could include:
- Make sure that "require" works properly.
- Test after "disable".

+               if (conn->channel_binding[0] == 'r')
Maybe this should comment that this means "require", in a fashion
similar to what is done when checking conn->sslmode.
--
Michael

On Wed, 2019-08-21 at 16:12 +0900, Michael Paquier wrote:
> This counts for other auth requests as well like krb5, no?  I think
> that we should also add the "disable" flavor for symmetry, and
> also...

Added "disable" option, and I refactored so that it would look
explicitly for an expected auth req based on the connection parameters.

I had to also make the client tell the server that it does not support
channel binding if channel_binding=disable, otherwise the server
complains.

> +#define DefaultChannelBinding  "prefer"
> If the build of Postgres does not support SSL (USE_SSL is not
> defined), I think that DefaultChannelBinding should be "disable".
> That would make things consistent with sslmode.

Done.

> Doing the filtering at the time of AUTH_REQ_OK is necessary for
> "trust", but shouldn't this be done as well earlier for other request
> types?  This could save round-trips to the server if we know that an
> exchange begins with a protocol which will never satisfy this
> request, saving efforts for a client doomed to fail after the first
> AUTH_REQ received.  That would be the case for all AUTH_REQ, except
> the SASL ones and of course AUTH_REQ_OK.

Done.

> 
> Could you please add negative tests in
> src/test/authentication/?  What
> could be covered there is that the case where "prefer" (and
> "disable"?) is defined then the authentication is able to go through,
> and that with "require" we get a proper failure as SSL is not used.
> Tests in src/test/ssl/ could include:
> - Make sure that "require" works properly.
> - Test after "disable".

Done.

> +               if (conn->channel_binding[0] == 'r')
> Maybe this should comment that this means "require", in a fashion
> similar to what is done when checking conn->sslmode.

Done.

New patch attached.

Regards,
    Jeff Davis

On Wed, Sep 04, 2019 at 09:22:33PM -0700, Jeff Davis wrote:
> New patch attached.

Thanks for the new version, Jeff.

+        with <productname>PostgreSQL</productname> 11 or later servers using
+        the <literal>scram-sha-256</literal> authentication method.

Nit here: "scram-sha-256" refers to the HBA entry.  I would
just use "SCRAM" instead.

In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL
mechanism over a non-SSL connection, should we complain even if
the "disable" mode is used?  It seems to me that there is a point to
complain in this case as a sanity check as the server should really
publicize "SCRAM-SHA-256-PLUS" only over SSL.

When the server only sends SCRAM-SHA-256 in the mechanism list and
"require" mode is used, we complain about "none of the server's SASL
authentication mechanisms are supported" which can be confusing.  Why
not generating a custom error if libpq selects SCRAM-SHA-256 when
"require" is used, say:
"SASL authentication mechanism SCRAM-SHA-256 selected but channel
binding is required"
That could be done by adding an error message when selecting
SCRAM-SHA-256 and then goto the error step.

Actually, it looks that the handling of channel_bound is incorrect.
If the server sends AUTH_REQ_SASL and libpq processes it, then the
flag gets already set.  Now let's imagine that the server is a rogue
one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check
would pass.  It seems to me that we should switch the flag once we are
sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when
the final message is done within pg_SASL_continue().

+# SSL not in use; channel binding still can't work
+reset_pg_hba($node, 'scram-sha-256');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
Worth testing md5 here?

PGCHANNELBINDING needs documentation.
--
Michael

On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote:
> Nit here: "scram-sha-256" refers to the HBA entry.  I would
> just use "SCRAM" instead.

Done.

> In pg_SASL_init(), if the server sends SCRAM-SHA-256-PLUS as SASL
> mechanism over a non-SSL connection, should we complain even if
> the "disable" mode is used?  It seems to me that there is a point to
> complain in this case as a sanity check as the server should really
> publicize "SCRAM-SHA-256-PLUS" only over SSL.

Done.

> When the server only sends SCRAM-SHA-256 in the mechanism list and
> "require" mode is used, we complain about "none of the server's SASL
> authentication mechanisms are supported" which can be confusing.  Why
> not generating a custom error if libpq selects SCRAM-SHA-256 when
> "require" is used, say:
> "SASL authentication mechanism SCRAM-SHA-256 selected but channel
> binding is required"
> That could be done by adding an error message when selecting
> SCRAM-SHA-256 and then goto the error step.

Done.

> Actually, it looks that the handling of channel_bound is incorrect.
> If the server sends AUTH_REQ_SASL and libpq processes it, then the
> flag gets already set.  Now let's imagine that the server is a rogue
> one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check
> would pass.  It seems to me that we should switch the flag once we
> are
> sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when
> the final message is done within pg_SASL_continue().

Thank you! Fixed. I now track whether channel binding is selected, and
also whether SASL actually finished successfully.

> +# SSL not in use; channel binding still can't work
> +reset_pg_hba($node, 'scram-sha-256');
> +$ENV{"PGCHANNELBINDING"} = 'require';
> +test_login($node, 'saslpreptest4a_role', "a", 2);
> Worth testing md5 here?

I added a new md5 test in the ssl test suite. Testing it in the non-SSL 
path doesn't seem like it adds much.

> PGCHANNELBINDING needs documentation.

Done.

Regards,
    Jeff Davis

On Sat, Sep 14, 2019 at 08:42:53AM -0700, Jeff Davis wrote:
> On Fri, 2019-09-06 at 16:05 +0900, Michael Paquier wrote:
>> Actually, it looks that the handling of channel_bound is incorrect.
>> If the server sends AUTH_REQ_SASL and libpq processes it, then the
>> flag gets already set.  Now let's imagine that the server is a rogue
>> one and sends AUTH_REQ_OK just after AUTH_REQ_SASL, then your check
>> would pass.  It seems to me that we should switch the flag once we
>> are
>> sure that the exchange is completed, aka with AUTH_REQ_SASL_FIN when
>> the final message is done within pg_SASL_continue().
>
> Thank you! Fixed. I now track whether channel binding is selected, and
> also whether SASL actually finished successfully.

Ah, I see.  So you have added an extra flag "sasl_finished" to make
sure of that, and still kept around "channel_bound".  Hence the two
flags have to be set to make sure that the SASL exchanged has been
finished and that channel binding has been enabled.  "channel_bound"
is linked to the selected mechanism when the exchange begins, meaning
that it could be possible to do the same check with the selected
mechanism directly from fe_scram_state instead.  "sasl_finished" is
linked to the state where the SASL exchange is finished, so this
basically maps into checking after FE_SCRAM_FINISHED.  Instead of
those two flags, wouldn't it be cleaner to add an extra routine to
fe-auth-scram.c which does the same sanity checks, say
pg_fe_scram_check_state()?  This needs to be careful about three
things, taking in input an opaque pointer to fe_scram_state if channel
binding is required:
- The data is not NULL.
- The sasl mechanism selected is SCRAM-SHA-256-PLUS.
- The state is FE_SCRAM_FINISHED.

What do you think?  There is no need to save down the connection
parameter value into fe_scram_state.

>> +# SSL not in use; channel binding still can't work
>> +reset_pg_hba($node, 'scram-sha-256');
>> +$ENV{"PGCHANNELBINDING"} = 'require';
>> +test_login($node, 'saslpreptest4a_role', "a", 2);
>> Worth testing md5 here?
>
> I added a new md5 test in the ssl test suite. Testing it in the non-SSL
> path doesn't seem like it adds much.

Good idea.

+   if (conn->channel_binding[0] == 'r' && /* require */
+       strcmp(selected_mechanism, SCRAM_SHA_256_NAME) == 0)
+   {
+       printfPQExpBuffer(&conn->errorMessage,
+                         libpq_gettext("SASL authentication mechanism
SCRAM-SHA-256 selected but channel binding is required\n"));
+       goto error;
+   }
Nit here as there are only two mechanisms handled: I would rather
cause the error if the selected mechanism does not match
SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism
matches SCRAM-SHA-256.  Either way is actually fine :)

+    printfPQExpBuffer(&conn->errorMessage,
+                      libpq_gettext("Channel binding required but not
offered by server\n"));
+                   result = false;
Should that be "completed by server" instead?

+           if (areq == AUTH_REQ_SASL_FIN)
+               conn->sasl_finished = true;
This should have a comment about the why it is done if you go this way
with the two flags added to PGconn.

+   if (conn->channel_binding[0] == 'r' && /* require */
+       !conn->ssl_in_use)
+   {
+       printfPQExpBuffer(&conn->errorMessage,
+                         libpq_gettext("Channel binding required, but
SSL not in use\n"));
+       goto error;
+   }
This is not necessary?  If SSL is not in use but the server publishes
SCRAM-SHA-256-PLUS, libpq complains.  If the server sends only
SCRAM-SHA-256 but channel binding is required, we complain down on
"SASL authentication mechanism SCRAM-SHA selected but channel binding
is required".  Or you have in mind that this error message is better?

I think that pgindent would complain with the comment block in
check_expected_areq().
--
Michael

On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote:
> basically maps into checking after FE_SCRAM_FINISHED.  Instead of
> those two flags, wouldn't it be cleaner to add an extra routine to
> fe-auth-scram.c which does the same sanity checks, say
> pg_fe_scram_check_state()?  This needs to be careful about three
> things, taking in input an opaque pointer to fe_scram_state if
> channel
> binding is required:
> - The data is not NULL.
> - The sasl mechanism selected is SCRAM-SHA-256-PLUS.
> - The state is FE_SCRAM_FINISHED.

Yes, I think this does come out a bit cleaner, thank you.

> What do you think?  There is no need to save down the connection
> parameter value into fe_scram_state.

I'm not sure I understand this comment, but I removed the extra boolean
flags.

> Nit here as there are only two mechanisms handled: I would rather
> cause the error if the selected mechanism does not match
> SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism
> matches SCRAM-SHA-256.  Either way is actually fine :)

Done.

> +    printfPQExpBuffer(&conn->errorMessage,
> +                      libpq_gettext("Channel binding required but
> not
> offered by server\n"));
> +                   result = false;
> Should that be "completed by server" instead?

Done.

> is required".  Or you have in mind that this error message is better?

I felt it would be a more useful error message.

> I think that pgindent would complain with the comment block in
> check_expected_areq(). 

Changed.

Regards,
    Jeff Davis

On Thu, Sep 19, 2019 at 05:40:15PM -0700, Jeff Davis wrote:
> On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote:
>> What do you think?  There is no need to save down the connection
>> parameter value into fe_scram_state.
>
> I'm not sure I understand this comment, but I removed the extra boolean
> flags.

Thanks for considering it.  I was just asking about removing those
flags and your thoughts about my thoughts from upthread.

>> is required".  Or you have in mind that this error message is better?
>
> I felt it would be a more useful error message.

Okay, fine by me.

>> I think that pgindent would complain with the comment block in
>> check_expected_areq().
>
> Changed.

A trick to make pgindent to ignore some comment blocks is to use
/*--------- at its top and bottom, FWIW.

+$ENV{PGUSER} = "ssltestuser";
 $ENV{PGPASSWORD} = "pass";
test_connect_ok() can use a complementary string, so I would use that
in the SSL test part instead of relying too much on the environment
for readability, particularly for the last test added with md5testuser.
Using the environment variable in src/test/authentication/ makes
sense.  Maybe that's just a matter of taste :)

+   return (state != NULL && state->state == FE_SCRAM_FINISHED &&
+           strcmp(state->sasl_mechanism, SCRAM_SHA_256_PLUS_NAME) == 0);
I think that we should document in the code why those reasons are
chosen.

I would also add a test for an invalid value of channel_binding.

A comment update is forgotten in libpq-int.h.

+# using the password authentication method; channel binding can't
work
+reset_pg_hba($node, 'password');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
+
+# SSL not in use; channel binding still can't work
+reset_pg_hba($node, 'scram-sha-256');
+$ENV{"PGCHANNELBINDING"} = 'require';
+test_login($node, 'saslpreptest4a_role', "a", 2);
Those two tests are in the test suite dedicated to SASLprep.  I think
that it would be more consistent to just move them to
001_password.pl.  And this does not impact the error coverage.

Missing some indentation in the perl scripts (per pgperltidy).

Those are mainly nits, and attached are the changes I would do to your
patch.  Please feel free to consider those changes as you see fit.
Anyway, the base logic looks good to me, so I am switching the patch
as ready for committer.
--
Michael

On Fri, 2019-09-20 at 13:07 +0900, Michael Paquier wrote:
> Those are mainly nits, and attached are the changes I would do to
> your
> patch.  Please feel free to consider those changes as you see fit.
> Anyway, the base logic looks good to me, so I am switching the patch
> as ready for committer.

Thank you, applied.

* I also changed the comment above pg_fe_scram_channel_bound() to
clarify that the caller must also check that the exchange was
successful.

* I changed the error message when AUTH_REQ_OK is received without
channel binding. It seemed confusing before. I also added a test.

Regards,
    Jeff Davis

On Fri, Sep 20, 2019 at 10:57:04AM -0700, Jeff Davis wrote:
> Thank you, applied.

Okay, I can see which parts you have changed.

> * I also changed the comment above pg_fe_scram_channel_bound() to
> clarify that the caller must also check that the exchange was
> successful.
>
> * I changed the error message when AUTH_REQ_OK is received without
> channel binding. It seemed confusing before. I also added a test.

And both make sense.

+ * Return true if channel binding was employed and the scram exchange
upper('scram')?

Except for this nit, it looks good to me.
--
Michael

On Sat, Sep 21, 2019 at 11:24:30AM +0900, Michael Paquier wrote:
> And both make sense.
>
> + * Return true if channel binding was employed and the scram exchange
> upper('scram')?
>
> Except for this nit, it looks good to me.

For the archive's sake: this has been committed as of d6e612f.

-   * We pretend that the connection is OK for the duration of these
-   * queries.
+   * We pretend that the connection is OK for the duration of
+   * these queries.
The result had some noise diffs.  Perhaps some leftover from the
indentation run?  That's no big deal anyway.
--
Michael