Letting the client choose the protocol to use during a SASL exchange

On Tue, Apr 04, 2017 at 03:02:30PM +0900, Michael Paquier wrote:

There is still one open item pending for SCRAM that has not been
treated which is mentioned here:
/messages/by-id/b081887e-1712-3aa4-7dbe-e012333d50e4@iki.fi

When doing an authentication with SASL, the server decides what is the
mechanism that the client has to use. As SCRAM-SHA-256 is only one of
such mechanisms, it would be nice to have something more generic and
have the server return to the client a list of protocols that the
client can choose from. And also the server achnowledge which protocol
is going to be used.

Note that RFC4422 has some content on the matter
https://tools.ietf.org/html/rfc4422#section-3.1:
Mechanism negotiation is protocol specific.

Commonly, a protocol will specify that the server advertises
supported and available mechanisms to the client via some facility
provided by the protocol, and the client will then select the "best"
mechanism from this list that it supports and finds suitable.

So once the server sends back the list of mechanisms that are
supported, the client is free to use what it wants.

On HEAD, a 'R' message with AUTH_REQ_SASL followed by
SCRAM_SHA256_NAME is sent to let the client know what is the mechanism
to use for the SASL exchange. In the future, this should be extended
so as a list of names is sent, for example a comma-separated list, but
we are free to choose the format we want here. With this list at hand,
the client can then choose the protocol it thinks is the best. Still,
there is a gap with our current implementation because the server
expects the first message from the client to have a SCRAM format, but
that's true only if SCRAM-SHA-256 is used as mechanism.

In order to cover this gap, it seems to me that we need to have an
intermediate state before the server is switched to FE_SCRAM_INIT so
as the mechanism used is negotiated between the two parties. Once the
protocol negotiation is done, the server can then move on with the
mechanism to use. This would be important in the future to allow more
SASL mechanisms to work. I am adding an open item for that.

If any SCRAM open item is a beta blocker, it's this one. (But SASLprep is
also in or near that status.) Post-beta wire protocol changes are bad,
considering beta is normally the time for projects like pgjdbc and npgsql to
start adapting to such changes.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Heikki,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/06/2017 08:13 AM, Noah Misch wrote:

If any SCRAM open item is a beta blocker, it's this one. (But SASLprep is
also in or near that status.) Post-beta wire protocol changes are bad,
considering beta is normally the time for projects like pgjdbc and npgsql to
start adapting to such changes.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.

I will work on this next week. I haven't given it much thought yet, but
I think it's going to be pretty straightforward. It won't require much
code yet, as we only support one SASL mechanism. We just need to ensure
that we don't paint ourselves in the corner with the protocol.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/04/17 19:05, Heikki Linnakangas wrote:

On 04/06/2017 08:13 AM, Noah Misch wrote:

If any SCRAM open item is a beta blocker, it's this one. (But
SASLprep is
also in or near that status.) Post-beta wire protocol changes are bad,
considering beta is normally the time for projects like pgjdbc and
npgsql to
start adapting to such changes.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.

I will work on this next week. I haven't given it much thought yet,
but I think it's going to be pretty straightforward. It won't require
much code yet, as we only support one SASL mechanism. We just need to
ensure that we don't paint ourselves in the corner with the protocol.

I think this could easily extended from the current message. SCRAM
does not force any concrete way of negotiating the protocols supported.
The current SASL message sends the only SCRAM method supported. I think
it should be enough to make this a CSV, which for a single value, is the
same as we have today (so no code change required, only documentation).
The other message that needs to be changed is the password one, the
first time the client sends the SCRAM "client-first-message", which
needs to contain the algorithm selected. As of today, that could be a
constant (at least in the code) and validate is value.

So I guess code changes would be minimal. I could be wrong, of course.

I'm working myself on Java's (pgjdbc) implementation, and I will
hopefully have a prototype by next week to try it.

�lvaro

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 4 April 2017 at 02:02, Michael Paquier <michael.paquier@gmail.com> wrote:

Hi all,

There is still one open item pending for SCRAM that has not been
treated which is mentioned here:
/messages/by-id/b081887e-1712-3aa4-7dbe-e012333d50e4@iki.fi

When doing an authentication with SASL, the server decides what is the
mechanism that the client has to use. As SCRAM-SHA-256 is only one of
such mechanisms, it would be nice to have something more generic and
have the server return to the client a list of protocols that the
client can choose from. And also the server achnowledge which protocol
is going to be used.

Note that RFC4422 has some content on the matter
https://tools.ietf.org/html/rfc4422#section-3.1:
Mechanism negotiation is protocol specific.

Commonly, a protocol will specify that the server advertises
supported and available mechanisms to the client via some facility
provided by the protocol, and the client will then select the "best"
mechanism from this list that it supports and finds suitable.

So once the server sends back the list of mechanisms that are
supported, the client is free to use what it wants.

On HEAD, a 'R' message with AUTH_REQ_SASL followed by
SCRAM_SHA256_NAME is sent to let the client know what is the mechanism
to use for the SASL exchange. In the future, this should be extended
so as a list of names is sent, for example a comma-separated list, but
we are free to choose the format we want here. With this list at hand,
the client can then choose the protocol it thinks is the best. Still,
there is a gap with our current implementation because the server
expects the first message from the client to have a SCRAM format, but
that's true only if SCRAM-SHA-256 is used as mechanism.

How would we provide the list of protocols? Surely the protocol is
defined by pg_hba.conf, which makes it dependent upon username,
database and ip range. If the list were accurate, it would allow an
attacker to discover how best to attack. If the list were inaccurate
it would just be an annoyance.

At minimum, providing the list of protocols means an extra round trip
to the server.

So while RFC4422 might say what "commonly" happens, I don't think it
applies sensibly to PostgreSQL, especially when we only have one
method.

ISTM that if you have a valid role to connect to then you'll also know
what authentication mechanism to use so you should be able to specify
the mechanism in your connection message and save the extra trip.

In order to cover this gap, it seems to me that we need to have an
intermediate state before the server is switched to FE_SCRAM_INIT so
as the mechanism used is negotiated between the two parties. Once the
protocol negotiation is done, the server can then move on with the
mechanism to use. This would be important in the future to allow more
SASL mechanisms to work. I am adding an open item for that.

So I don't see a gap or an open item on that point. I see a potential
future feature.

For extensibility, we may also want to revisit the choice of defining
'scram' in pg_hba.conf instead of 'sasl'...

I'd like to see us replace "scram" with "sasl=scram-sha-256".

So when we extend it in future, we already have the syntax in place to
support "sasl=newmethod", rather than introduce syntax that we already
know will become outdated in future.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs <simon@2ndquadrant.com> writes:

How would we provide the list of protocols? Surely the protocol is
defined by pg_hba.conf, which makes it dependent upon username,
database and ip range. If the list were accurate, it would allow an
attacker to discover how best to attack. If the list were inaccurate
it would just be an annoyance.

At minimum, providing the list of protocols means an extra round trip
to the server.

Yeah, that's a problem.

ISTM that if you have a valid role to connect to then you'll also know
what authentication mechanism to use so you should be able to specify
the mechanism in your connection message and save the extra trip.

I do not buy that in the least. It has never been the case before now
that clients know in advance what the auth challenge method will be.
If we put that requirement on them for SCRAM, we're just going to be
exporting a lot of pain and end-user-visible inconsistency.

Perhaps we could turn this around: have the client send (in the connection
request packet) a list of auth protocols it thinks it is able to handle.
(I'm envisioning this as being more or less fixed for any one version of
any one client, since it would basically mean "I have code to do X, Y, or
Z".) Then the server can pick one that is allowed by pg_hba.conf, or it
can just ignore the list and send what it wants anyway, probably leading
to client disconnect.

We could avoid this being a protocol break by having the server's default
assumption being that the client can handle all pre-SCRAM auth protocols.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/04/17 22:05, Tom Lane wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

How would we provide the list of protocols? Surely the protocol is
defined by pg_hba.conf, which makes it dependent upon username,
database and ip range. If the list were accurate, it would allow an
attacker to discover how best to attack. If the list were inaccurate
it would just be an annoyance.
At minimum, providing the list of protocols means an extra round trip
to the server.

Yeah, that's a problem.

I don't see it. The message AuthenticationSASL.String could contain
a CSV of the SCRAM protocols supported. This is specially important to
support channel binding (which is just another protocol name for this
matter), which is the really enhanced security mechanism of SCRAM. Since
this message is sent regardless, and the client replies with
PasswordMessage, no extra round trip is required. However,
PasswordMessage needs to also include a field with the name of the
selected protocol (it is the client who picks). Or a different message
would need to be created, but no extra round-trips more than those
required by SCRAM itself (4 messages for SCRAM + 1 extra for the server
to tell the client it needs to use SCRAM).

ISTM that if you have a valid role to connect to then you'll also know
what authentication mechanism to use so you should be able to specify
the mechanism in your connection message and save the extra trip.

I do not buy that in the least. It has never been the case before now
that clients know in advance what the auth challenge method will be.
If we put that requirement on them for SCRAM, we're just going to be
exporting a lot of pain and end-user-visible inconsistency.

Perhaps we could turn this around: have the client send (in the connection
request packet) a list of auth protocols it thinks it is able to handle.

Per the SCRAM RFC, it is the server who advertises and the client
who picks.

Regards,

�lvaro

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6 April 2017 at 16:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Perhaps we could turn this around: have the client send (in the connection
request packet) a list of auth protocols it thinks it is able to handle.
(I'm envisioning this as being more or less fixed for any one version of
any one client, since it would basically mean "I have code to do X, Y, or
Z".) Then the server can pick one that is allowed by pg_hba.conf,

+1

Much better plan.

or it
can just ignore the list and send what it wants anyway, probably leading
to client disconnect.

It would need to follow one of the requested protocols, but mark the
request as doomed. Otherwise we'd be revealing information. That's
what SCRAM does now.

Since the list is currently length one, we can add more later when we
get a list potentially > 1.

We could avoid this being a protocol break by having the server's default
assumption being that the client can handle all pre-SCRAM auth protocols.

+1

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa <aht@8kdata.com> wrote:

I don't see it. The message AuthenticationSASL.String could contain a
CSV of the SCRAM protocols supported. This is specially important to support
channel binding (which is just another protocol name for this matter), which
is the really enhanced security mechanism of SCRAM. Since this message is
sent regardless, and the client replies with PasswordMessage, no extra round
trip is required. However, PasswordMessage needs to also include a field
with the name of the selected protocol (it is the client who picks). Or a
different message would need to be created, but no extra round-trips more
than those required by SCRAM itself (4 messages for SCRAM + 1 extra for the
server to tell the client it needs to use SCRAM).

Yes, it seems to me that the list of protocols to send should be done
by sendAuthRequest(). Then the client parses the received string, and
sends an extra 'p' message with its choice before sending the first
SCRAM message. So there is no need for any extra round trips.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6 April 2017 at 16:15, Álvaro Hernández Tortosa <aht@8kdata.com> wrote:

Per the SCRAM RFC, it is the server who advertises and the client who picks.

Yes, but what does the RFC say about how to handle servers with an pg_hba.conf?

How and what will we advertise?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Apr 7, 2017 at 7:29 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 6 April 2017 at 16:15, Álvaro Hernández Tortosa <aht@8kdata.com> wrote:

Per the SCRAM RFC, it is the server who advertises and the client who picks.

Yes, but what does the RFC say about how to handle servers with an pg_hba.conf?

How and what will we advertise?

Did you read the first email of this thread? The RFC says that the
protocol implementers are free to do what they want as this is
protocol-specific. At least that's what I understand.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/06/2017 11:16 PM, Simon Riggs wrote:

or it
can just ignore the list and send what it wants anyway, probably leading
to client disconnect.

It would need to follow one of the requested protocols, but mark the
request as doomed. Otherwise we'd be revealing information. That's
what SCRAM does now.

It's not a secret today, what authentication method the server requires.
You can't really hide it, anyway, as the client could probe with
different lists of supported methods, and see which method the server
picks in each case.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/06/2017 11:05 PM, Tom Lane wrote:

Perhaps we could turn this around: have the client send (in the connection
request packet) a list of auth protocols it thinks it is able to handle.
(I'm envisioning this as being more or less fixed for any one version of
any one client, since it would basically mean "I have code to do X, Y, or
Z".) Then the server can pick one that is allowed by pg_hba.conf, or it
can just ignore the list and send what it wants anyway, probably leading
to client disconnect.

That list of supported authentication methods would need to be included
in the startup message. Unfortunately, there is no way to add options to
the startup message, without breaking compatibility with old servers. If
there is an option in the startup message that the server doesn't
understand, it will treat it as a GUC, and you get an "unrecognized
configuration parameter" after authentication.

It would be nice to change that, so that the server would ignore
parameters that it doesn't understand that begin with "optional_"
prefix, for example. But it won't help us right now.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 7 April 2017 at 16:33, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

That list of supported authentication methods would need to be included in
the startup message. Unfortunately, there is no way to add options to the
startup message, without breaking compatibility with old servers. If there
is an option in the startup message that the server doesn't understand, it
will treat it as a GUC, and you get an "unrecognized configuration
parameter" after authentication.

sasl.mechanisms = 'SCRAM_SHA256'

:p

No, I'm not seriously suggesting we abuse that.

Personally I think it's reasonable enough to let the server send a 'B'
message with supported auth modes. I'm not overly concerned about the
small information leak that provides. We're unlikely to be able to
convincingly fake execution of any and all SASL auth methods the
client may request, and since they may require any arbitrary number of
message exchanges we'd basically land up blackholeing clients that try
an unsupported auth-method.

No thanks. It's one area I'd rather honestly say "nope, we don't
support that". In which case the client can easily enough probe for
all known methods, and we might as well just tell it up front.

This is hardly new. Most servers with neotiable auth, like SMTP, IMAP,
etc, have the server supply a list of auth mechs.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/07/2017 11:57 AM, Craig Ringer wrote:

On 7 April 2017 at 16:33, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

That list of supported authentication methods would need to be included in
the startup message. Unfortunately, there is no way to add options to the
startup message, without breaking compatibility with old servers. If there
is an option in the startup message that the server doesn't understand, it
will treat it as a GUC, and you get an "unrecognized configuration
parameter" after authentication.

sasl.mechanisms = 'SCRAM_SHA256'

:p

No, I'm not seriously suggesting we abuse that.

Hmm, that's not such a bad idea, actually. It only goes back to 9.2,
though. Before that, the prefix needed to be listed in
custom_variable_classes, or you got an error. 9.2 is the oldest
supported version, but libpq should still be able to connect to older
versions.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/07/2017 01:13 AM, Michael Paquier wrote:

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa <aht@8kdata.com> wrote:

I don't see it. The message AuthenticationSASL.String could contain a
CSV of the SCRAM protocols supported. This is specially important to support
channel binding (which is just another protocol name for this matter), which
is the really enhanced security mechanism of SCRAM. Since this message is
sent regardless, and the client replies with PasswordMessage, no extra round
trip is required. However, PasswordMessage needs to also include a field
with the name of the selected protocol (it is the client who picks). Or a
different message would need to be created, but no extra round-trips more
than those required by SCRAM itself (4 messages for SCRAM + 1 extra for the
server to tell the client it needs to use SCRAM).

Yes, it seems to me that the list of protocols to send should be done
by sendAuthRequest(). Then the client parses the received string, and
sends an extra 'p' message with its choice before sending the first
SCRAM message. So there is no need for any extra round trips.

I started writing down the protocol docs, based on the above idea. See
attached. The AuthenticationSASL message now contains a list of mechanisms.

Does that seem clear to you? If so, I'll change the code to match the
attached docs.

I added two new message formats to the docs, SASLResponse and
SASLInitialResponse. Those use the same type byte as PasswordMessage,
'p', but I decided to describe them as separate messages for
documentation purposes, since the content is completely different
depending on whether the message is sent as part of SASL, GSS, md5, or
password authentication. IOW, this is not a change in the
implementation, only in the way it's documented.

While working on this, and reading the RFCs more carefully, I noticed
one detail we should change, to be spec-complicant. The SASL spec
specifies that a SASL authentication exchange consists of
challenge-response pairs. There must be a response to each challenge. If
the last message in the authentication mechanism (SCRAM in this case)
goes in the server->client direction, then that message must sent as
"additional data" in the server->client message that tells the client
that the authentication was successful. That's AuthenticationOK in the
PostgreSQL protocol. In the current implementation, the
server-final-message is sent as an AuthenticationSASLContinue message,
and the client doesn't respond to that.

We should change that, so that the server-final-message is sent as
"additional data" in the AuthenticationOK message. The attached docs
patch describes that, rather than what the current implementation does.

(For your convenience, I built the HTML docs with this patch, and put
them up at http://hlinnaka.iki.fi/temp/scram-wip-docs/protocol.html for
viewing)

- Heikki

On 10/04/17 14:57, Heikki Linnakangas wrote:

On 04/07/2017 01:13 AM, Michael Paquier wrote:

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:

I don't see it. The message AuthenticationSASL.String could
contain a
CSV of the SCRAM protocols supported. This is specially important to
support
channel binding (which is just another protocol name for this
matter), which
is the really enhanced security mechanism of SCRAM. Since this
message is
sent regardless, and the client replies with PasswordMessage, no
extra round
trip is required. However, PasswordMessage needs to also include a
field
with the name of the selected protocol (it is the client who picks).
Or a
different message would need to be created, but no extra round-trips
more
than those required by SCRAM itself (4 messages for SCRAM + 1 extra
for the
server to tell the client it needs to use SCRAM).

Yes, it seems to me that the list of protocols to send should be done
by sendAuthRequest(). Then the client parses the received string, and
sends an extra 'p' message with its choice before sending the first
SCRAM message. So there is no need for any extra round trips.

I started writing down the protocol docs, based on the above idea. See
attached. The AuthenticationSASL message now contains a list of
mechanisms.

Does that seem clear to you? If so, I'll change the code to match the
attached docs.

I added two new message formats to the docs, SASLResponse and
SASLInitialResponse. Those use the same type byte as PasswordMessage,
'p', but I decided to describe them as separate messages for
documentation purposes, since the content is completely different
depending on whether the message is sent as part of SASL, GSS, md5, or
password authentication. IOW, this is not a change in the
implementation, only in the way it's documented.

While working on this, and reading the RFCs more carefully, I noticed
one detail we should change, to be spec-complicant. The SASL spec
specifies that a SASL authentication exchange consists of
challenge-response pairs. There must be a response to each challenge.
If the last message in the authentication mechanism (SCRAM in this
case) goes in the server->client direction, then that message must
sent as "additional data" in the server->client message that tells the
client that the authentication was successful. That's AuthenticationOK
in the PostgreSQL protocol. In the current implementation, the
server-final-message is sent as an AuthenticationSASLContinue message,
and the client doesn't respond to that.

We should change that, so that the server-final-message is sent as
"additional data" in the AuthenticationOK message. The attached docs
patch describes that, rather than what the current implementation does.

(For your convenience, I built the HTML docs with this patch, and put
them up at http://hlinnaka.iki.fi/temp/scram-wip-docs/protocol.html
for viewing)

- Heikki

Thanks for posting the patched HTML. In my opinion, all looks good
except that:

- I will add an extra String (a CSV) to AuthenticationSASL message for
channel binding names, so that message format can remain without changes
when channel binding is implemented. It can be empty.

- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message. Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.

Álvaro

--

Álvaro Hernández Tortosa

-----------
<8K>data

On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:

Thanks for posting the patched HTML. In my opinion, all looks good
except that:

- I will add an extra String (a CSV) to AuthenticationSASL message for
channel binding names, so that message format can remain without changes
when channel binding is implemented. It can be empty.

Note that SCRAM-SHA-256 with channel binding has a different SASL
mechanism name, SRAM-SHA-256-PLUS. No need for a separate flag or string
for channel binding. When support for channel binding is added to the
server, it will advertise two SASL mechanisms in the AuthenticationSASL
message, SCRAM-SHA-256 and SCRAM-SHA-256-PLUS. (Or just
SCRAM-SHA-256-PLUS, if channel-binding is required).

- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message.

The problem with that is that the SCRAM spec dictates that the username
must be encoded in UTF-8, but PostgreSQL supports non-UTF-8 usernames.

Or did you mean that, if the username is sent, it must match the one in
the startup packet, but an empty string would always be allowed? That
would be reasonable.

Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.

I'm not sure I follow. The username is sent from client to server, and
currently, the server will ignore it. If you're writing a client
library, it can send whatever it wants. (Although again I would
recommend an empty string, to avoid confusion. Sending the same username
as in the startup packet, as long as it's in UTF-8, seems reasonable too.)

Thanks for reviewing this! I'll start hacking on code changes to go with
these docs.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/04/17 21:41, Heikki Linnakangas wrote:

On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:

Thanks for posting the patched HTML. In my opinion, all looks good
except that:

- I will add an extra String (a CSV) to AuthenticationSASL message for
channel binding names, so that message format can remain without changes
when channel binding is implemented. It can be empty.

Note that SCRAM-SHA-256 with channel binding has a different SASL
mechanism name, SRAM-SHA-256-PLUS. No need for a separate flag or
string for channel binding. When support for channel binding is added
to the server, it will advertise two SASL mechanisms in the
AuthenticationSASL message, SCRAM-SHA-256 and SCRAM-SHA-256-PLUS. (Or
just SCRAM-SHA-256-PLUS, if channel-binding is required).

Channel binding needs to specify actually three things:
- The mechanism, which is indeed suffixed "-PLUS".
- The channel binding name, which is described here:
https://tools.ietf.org/html/rfc5056. Types are also IANA-registered (see
https://www.iana.org/assignments/channel-binding-types/channel-binding-types.xhtml).
SCRAM mandates to implement 'tls-unique', but other channel binding
types could be supported (like 'tls-server-end-point' for example).
- The channel binding data, which is channel binding mechanism
dependent, and is sent as part of the client last message.

What I'm talking about here is the second one, the channel binding
type (name).

- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message.

The problem with that is that the SCRAM spec dictates that the
username must be encoded in UTF-8, but PostgreSQL supports non-UTF-8
usernames.

Or did you mean that, if the username is sent, it must match the one
in the startup packet, but an empty string would always be allowed?
That would be reasonable.

Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.

I'm not sure I follow. The username is sent from client to server, and
currently, the server will ignore it. If you're writing a client
library, it can send whatever it wants. (Although again I would
recommend an empty string, to avoid confusion. Sending the same
username as in the startup packet, as long as it's in UTF-8, seems
reasonable too.)

OK, understood. I will not let then the SCRAM implementation I'm
writing to allow for empty string as the user name, but in the pgjdbc
glue code send "ignore" as the user name or something like that ;P

Thanks for reviewing this! I'll start hacking on code changes to go
with these docs.

Thanks for writing the code :)

Álvaro

--

Álvaro Hernández Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Apr 11, 2017 at 4:41 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 04/10/2017 09:33 PM, Álvaro Hernández Tortosa wrote:

- If the username used is the one sent in the startup message, rather
than leaving it empty in the client-first-message, I would force it to
be the same as the used in the startuo message.

The problem with that is that the SCRAM spec dictates that the username must
be encoded in UTF-8, but PostgreSQL supports non-UTF-8 usernames.

Or did you mean that, if the username is sent, it must match the one in the
startup packet, but an empty string would always be allowed? That would be
reasonable.

That sounds sensible from here.

Otherwise we may confuse
some client implementations which would probably consider that as an
error; for one, my implementation would currently throw an error if
username is empty, and I think that's correct.

I'm not sure I follow. The username is sent from client to server, and
currently, the server will ignore it. If you're writing a client library, it
can send whatever it wants. (Although again I would recommend an empty
string, to avoid confusion. Sending the same username as in the startup
packet, as long as it's in UTF-8, seems reasonable too.)

Thanks for reviewing this! I'll start hacking on code changes to go with
these docs.

Sounds good to me, thanks! I looked at the doc patch for now, and here
are some nits.

+  <para>
+    SCRAM-SHA-256 is the only implemented SASL mechanism, at the moment. It is
+    described in detail in [RFC7677] and [RFC5741]. (called just
SCRAM from now on)
+  </para>
Perhaps those should be links to the RFCs.

+<para>
+  Client sends a SASLResponse messsage, with SCRAM
+  <literal>client-final-message</literal> as the content.
+</para>
Typo. Message needs two 's'.

+  <literal>server-final-message</> in the "additional data" field.
+</para>
+
+</sect2>
+
I think that you are missing a </sect1> markup here.

+<listitem>
+<para>
+                Authentication method specific "additional data". If this
+                message contains no additional data, this field is omitted.
+</para>
+</listitem>
So that's for the first message generated by the client in the
exchange. Better than my previous idea. Shouldn't double quotes be
replaced by proper markups?
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/10/2017 11:03 PM, Álvaro Hernández Tortosa wrote:

Channel binding needs to specify actually three things:
- The mechanism, which is indeed suffixed "-PLUS".
- The channel binding name, which is described here:
https://tools.ietf.org/html/rfc5056. Types are also IANA-registered (see
https://www.iana.org/assignments/channel-binding-types/channel-binding-types.xhtml).
SCRAM mandates to implement 'tls-unique', but other channel binding
types could be supported (like 'tls-server-end-point' for example).
- The channel binding data, which is channel binding mechanism
dependent, and is sent as part of the client last message.

What I'm talking about here is the second one, the channel binding
type (name).

Oh, I see. According to the SCRAM RFC, "tls-unique" is used by default.
I don't see us implementing anything else any time soon. PostgreSQL
doesn't support any other "channel type" than TLS, and tls-unique is the
only one that makes sense for TLS.

If we do need it after all, the server could advertise the additional
channel binding types as additional SASL mechanisms in the
AuthenticationSASL message, maybe something like:

"SCRAM-SHA-256"
"SCRAM-SHA-256-PLUS" (for tls-unique)
"SCRAM-SHA-256-PLUS ssh-unique" (for hypothetical ssh channel binding)

The same trick can be used to advertise any other SASL mechanism
specific options, if needed in the future.

I'm not sure I follow. The username is sent from client to server, and
currently, the server will ignore it. If you're writing a client
library, it can send whatever it wants. (Although again I would
recommend an empty string, to avoid confusion. Sending the same
username as in the startup packet, as long as it's in UTF-8, seems
reasonable too.)

OK, understood. I will not let then the SCRAM implementation I'm
writing to allow for empty string as the user name, but in the pgjdbc
glue code send "ignore" as the user name or something like that ;P

Hmm, so the SASL library you're using doesn't like sending an empty
string as the username? Now that I look at RFC5802 more closely, it says:

If the preparation of the username fails or results in an empty
string, the server SHOULD abort the authentication exchange.

Perhaps we should reserve a magic user name to mean "same as startup
message", in addition or instead of the empty string. We actually
discussed that already at [1]/messages/by-id/551A8C2F.7050409@iki.fi, but we forgot about it since.

[1]: /messages/by-id/551A8C2F.7050409@iki.fi

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/04/17 08:50, Heikki Linnakangas wrote:

On 04/10/2017 11:03 PM, �lvaro Hern�ndez Tortosa wrote:

Channel binding needs to specify actually three things:
- The mechanism, which is indeed suffixed "-PLUS".
- The channel binding name, which is described here:
https://tools.ietf.org/html/rfc5056. Types are also IANA-registered (see
https://www.iana.org/assignments/channel-binding-types/channel-binding-types.xhtml).

SCRAM mandates to implement 'tls-unique', but other channel binding
types could be supported (like 'tls-server-end-point' for example).
- The channel binding data, which is channel binding mechanism
dependent, and is sent as part of the client last message.

What I'm talking about here is the second one, the channel binding
type (name).

Oh, I see. According to the SCRAM RFC, "tls-unique" is used by
default. I don't see us implementing anything else any time soon.
PostgreSQL doesn't support any other "channel type" than TLS, and
tls-unique is the only one that makes sense for TLS.

If we do need it after all, the server could advertise the additional
channel binding types as additional SASL mechanisms in the
AuthenticationSASL message, maybe something like:

"SCRAM-SHA-256"
"SCRAM-SHA-256-PLUS" (for tls-unique)
"SCRAM-SHA-256-PLUS ssh-unique" (for hypothetical ssh channel binding)

The same trick can be used to advertise any other SASL mechanism
specific options, if needed in the future.

Why not add an extra field to the message? This scheme has in my
opinion some disadvantages:

- You assume no extensibility. Maybe Postgres will implement other
mechanisms for channel binding. Maybe not. But why limit ourselves?
- Apart from tls-unique there are others today, like
tls-server-end-point and who knows if in the future TLS 1.x comes with
something like 'tls-unique-1.x'
- Why have to parse the string (separated by spaces) when you could use
different fields and have no parsing at all?
- How do you advertise different SCRAM mechanisms with different channel
binding types? And a mix of SCRAM mechanisms with and without channel
binding? If I got it right, with your proposal it would be something like:

Field 1: SCRAM-SHA-256,SCRAM-SHA-256-PLUS tls-unique,SCRAM-SHA-1-PLUS
tls-unique,SCRAM-SHA-512-PLUS tls-unique

(which is basically a CSV of pairs where the right part of the pair
might be empty; too much IMHO for a single field)

vs

Field 1:
SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
(simple CSV)
Field 2: tls-unique (String)

Is there any reason not to use two fields? AuthenticationSASL is a
new message, I guess we can freely choose its format, right?

I'm not sure I follow. The username is sent from client to server, and
currently, the server will ignore it. If you're writing a client
library, it can send whatever it wants. (Although again I would
recommend an empty string, to avoid confusion. Sending the same
username as in the startup packet, as long as it's in UTF-8, seems
reasonable too.)

OK, understood. I will not let then the SCRAM implementation I'm
writing to allow for empty string as the user name, but in the pgjdbc
glue code send "ignore" as the user name or something like that ;P

Hmm, so the SASL library you're using doesn't like sending an empty
string as the username? Now that I look at RFC5802 more closely, it says:

If the preparation of the username fails or results in an empty
string, the server SHOULD abort the authentication exchange.

Perhaps we should reserve a magic user name to mean "same as startup
message", in addition or instead of the empty string. We actually
discussed that already at [1], but we forgot about it since.

That works. Please let me know what is the "magic constant" chosen ;P

Thanks,

�lvaro

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/11/2017 11:55 AM, �lvaro Hern�ndez Tortosa wrote:

On 11/04/17 08:50, Heikki Linnakangas wrote:

Oh, I see. According to the SCRAM RFC, "tls-unique" is used by
default. I don't see us implementing anything else any time soon.
PostgreSQL doesn't support any other "channel type" than TLS, and
tls-unique is the only one that makes sense for TLS.

If we do need it after all, the server could advertise the additional
channel binding types as additional SASL mechanisms in the
AuthenticationSASL message, maybe something like:

"SCRAM-SHA-256"
"SCRAM-SHA-256-PLUS" (for tls-unique)
"SCRAM-SHA-256-PLUS ssh-unique" (for hypothetical ssh channel binding)

The same trick can be used to advertise any other SASL mechanism
specific options, if needed in the future.

Why not add an extra field to the message? This scheme has in my
opinion some disadvantages:

- You assume no extensibility. Maybe Postgres will implement other
mechanisms for channel binding. Maybe not. But why limit ourselves?
- Apart from tls-unique there are others today, like
tls-server-end-point and who knows if in the future TLS 1.x comes with
something like 'tls-unique-1.x'
- Why have to parse the string (separated by spaces) when you could use
different fields and have no parsing at all?

I don't think an option separated by space is any more difficult to
parse than a separate field. I'm envisioning that the "parsing" would be
simply:

if (strcmp(sasl_mechanism, "SCRAM-SHA-256") == 0) { ... }
else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS") == 0) { ... }
else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS tls-awesome") == 0)
{ ... }

This can be extended for more complicated options, if necessary.
Although if we find that we need a dozen different options, I think
we've done something wrong.

- How do you advertise different SCRAM mechanisms with different channel
binding types? And a mix of SCRAM mechanisms with and without channel
binding? If I got it right, with your proposal it would be something like:

Field 1: SCRAM-SHA-256,SCRAM-SHA-256-PLUS tls-unique,SCRAM-SHA-1-PLUS
tls-unique,SCRAM-SHA-512-PLUS tls-unique

(which is basically a CSV of pairs where the right part of the pair
might be empty; too much IMHO for a single field)

Yes, except that in my proposal, the list is not a comma-separated
string, but a list of null-terminated strings, similar to how some other
lists of options in the FE/BE protocol are transmitted.

vs

Field 1:
SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
(simple CSV)
Field 2: tls-unique (String)

What if tls-unique is only supported with SCRAM-SHA-256-PLUS, while
SCRAM-SHA-512-PLUS requires tls-awesome? What about possible other
options you might need to tack on to mechanisms? This seems less
flexible. I'm not saying that either of those cases is very likely, but
I don't think it's very likely we'll need extra options or different
channel binding types any time soon, anyway.

Is there any reason not to use two fields? AuthenticationSASL is a
new message, I guess we can freely choose its format, right?

Yes, we can choose the format freely.

In summary, I think the simple list of mechanism names is best, because:

* It's simple, and doesn't have any extra fields or options that are not
needed right now.
* It's flexible, for future extension. We can add new mechanism entries
with extra options, not just new channel binding types, if needed, and
existing clients (i.e. v10 clients) will ignore them.

Perhaps we should reserve a magic user name to mean "same as startup
message", in addition or instead of the empty string. We actually
discussed that already at [1], but we forgot about it since.

That works. Please let me know what is the "magic constant" chosen ;P

I was hoping you'd have some good suggestions :-). Unfortunately there
is no reserved username prefix or anything like that, so whatever we
choose might also be a real username. That's OK, but it could be
confusing, if we ever tried to do something different with the SASL
username. How about "pg_same_as_startup_message"?

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/04/17 12:23, Heikki Linnakangas wrote:

On 04/11/2017 11:55 AM, �lvaro Hern�ndez Tortosa wrote:

On 11/04/17 08:50, Heikki Linnakangas wrote:

Oh, I see. According to the SCRAM RFC, "tls-unique" is used by
default. I don't see us implementing anything else any time soon.
PostgreSQL doesn't support any other "channel type" than TLS, and
tls-unique is the only one that makes sense for TLS.

If we do need it after all, the server could advertise the additional
channel binding types as additional SASL mechanisms in the
AuthenticationSASL message, maybe something like:

"SCRAM-SHA-256"
"SCRAM-SHA-256-PLUS" (for tls-unique)
"SCRAM-SHA-256-PLUS ssh-unique" (for hypothetical ssh channel binding)

The same trick can be used to advertise any other SASL mechanism
specific options, if needed in the future.

Why not add an extra field to the message? This scheme has in my
opinion some disadvantages:

- You assume no extensibility. Maybe Postgres will implement other
mechanisms for channel binding. Maybe not. But why limit ourselves?
- Apart from tls-unique there are others today, like
tls-server-end-point and who knows if in the future TLS 1.x comes with
something like 'tls-unique-1.x'
- Why have to parse the string (separated by spaces) when you could use
different fields and have no parsing at all?

I don't think an option separated by space is any more difficult to
parse than a separate field. I'm envisioning that the "parsing" would
be simply:

if (strcmp(sasl_mechanism, "SCRAM-SHA-256") == 0) { ... }
else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS") == 0) { ... }
else if (strcmp(sasl_mechanism, "SCRAM-SHA-256-PLUS tls-awesome") ==
0) { ... }

This can be extended for more complicated options, if necessary.
Although if we find that we need a dozen different options, I think
we've done something wrong.

- How do you advertise different SCRAM mechanisms with different channel
binding types? And a mix of SCRAM mechanisms with and without channel
binding? If I got it right, with your proposal it would be something
like:

Field 1: SCRAM-SHA-256,SCRAM-SHA-256-PLUS tls-unique,SCRAM-SHA-1-PLUS
tls-unique,SCRAM-SHA-512-PLUS tls-unique

(which is basically a CSV of pairs where the right part of the pair
might be empty; too much IMHO for a single field)

Yes, except that in my proposal, the list is not a comma-separated
string, but a list of null-terminated strings, similar to how some
other lists of options in the FE/BE protocol are transmitted.

The fact that you null terminate them (fine with me) doesn't change
my reasoning. How do you separate multiple channel binding methods? And
do you realize that you will be repeating the channel binding methods
without reason? A contrived but legal, possible example:

Field1:
SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-512-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-999-PLUS tls-unique tls-awesome yet-another-tls\0

vs

Field 1:
SCRAM-SHA-256 SCRAM-SHA-512 SCRAM-SHA-999 SCRAM-SHA-256-PLUS
SCRAM-SHA-512-PLUS SCRAM-SHA-999-PLUS\0

Field 2:
tls-unique tls-awesome yet-another-tls\0

Parsing and validation of the first option is significantly more
complex than the second option.

vs

Field 1:
SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
(simple CSV)
Field 2: tls-unique (String)

What if tls-unique is only supported with SCRAM-SHA-256-PLUS, while
SCRAM-SHA-512-PLUS requires tls-awesome?

It can't happen. The RFC clearly states that they are orthogonal.
It is left to the implementations support one or the other, but no
reason to limit applicability of a given binding method to a given SCRAM
mechanisms (or viceversa).

What about possible other options you might need to tack on to
mechanisms? This seems less flexible. I'm not saying that either of
those cases is very likely, but I don't think it's very likely we'll
need extra options or different channel binding types any time soon,
anyway.

As I said, channel binding and SCRAM mechanisms are orthogonal.

On the flip side, a contrived parsing mechanism with optional
fields and many that would be repeated all the time seems far from
ideal. And far less clear.

Is there any reason not to use two fields? AuthenticationSASL is a
new message, I guess we can freely choose its format, right?

Yes, we can choose the format freely.

Cool.

In summary, I think the simple list of mechanism names is best, because:

* It's simple, and doesn't have any extra fields or options that are
not needed right now.

I think it is too complicated and mixing things that are
orthogonal. Since protocol is hard to extend, adding an extra field for
the channel binding mechanisms -even if unused as of PG 10- is a good
thing. If you need to change the protocol later, that's a very bad thing
(realistically, we all know it won't be changed and some hack would need
to be implemented).

* It's flexible, for future extension. We can add new mechanism
entries with extra options, not just new channel binding types, if
needed, and existing clients (i.e. v10 clients) will ignore them.

I think the extra field allows for much more extension possibilities.

Having said all this, choice is yours :)

Perhaps we should reserve a magic user name to mean "same as startup
message", in addition or instead of the empty string. We actually
discussed that already at [1], but we forgot about it since.

That works. Please let me know what is the "magic constant" chosen ;P

I was hoping you'd have some good suggestions :-). Unfortunately there
is no reserved username prefix or anything like that, so whatever we
choose might also be a real username. That's OK, but it could be
confusing, if we ever tried to do something different with the SASL
username. How about "pg_same_as_startup_message"?

It doesn't matter if it conflicts with a real name, since it will
be ignored anyway ^_^ So I'd either pick a short string for saving a few
bytes (something like "*") or something that looks cool like a function
name like "pg.get_from_startup_message()" ^_^ (I prefer the former)

�lvaro

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/11/2017 01:39 PM, �lvaro Hern�ndez Tortosa wrote:

The fact that you null terminate them (fine with me) doesn't change
my reasoning. How do you separate multiple channel binding methods? And
do you realize that you will be repeating the channel binding methods
without reason? A contrived but legal, possible example:

Field1:
SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-512-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-999-PLUS tls-unique tls-awesome yet-another-tls\0

I was actually thinking of:

SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS\0
SCRAM-SHA-256-PLUS tls-awesome\0
SCRAM-SHA-256-PLUS yet-another-tls\0
SCRAM-SHA-512-PLUS\0
SCRAM-SHA-512-PLUS tls-awesome\0
SCRAM-SHA-512-PLUS yet-another-tls\0
SCRAM-SHA-999-PLUS\0
SCRAM-SHA-999-PLUS tls-awesome\0
SCRAM-SHA-999-PLUS yet-another-tls\0

In practice, I don't foresee us having this many options, so the
verbosity won't be an issue. Parsing this is very straightforward.

vs

Field 1:
SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
(simple CSV)
Field 2: tls-unique (String)

What if tls-unique is only supported with SCRAM-SHA-256-PLUS, while
SCRAM-SHA-512-PLUS requires tls-awesome?

It can't happen. The RFC clearly states that they are orthogonal.
It is left to the implementations support one or the other, but no
reason to limit applicability of a given binding method to a given SCRAM
mechanisms (or viceversa).

Well, if tls-unique is found to be insecure, a future SCRAM-SHA-512-PLUS
spec might well define that the default for that mechanism is
tls-unique-new-and-secure rather than tls-unique. Maybe even forbid
using tls-unique altogether. I don't think that's likely, but this is
all about future-proofing, so I'd rather keep it flexible.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/04/17 13:21, Heikki Linnakangas wrote:

On 04/11/2017 01:39 PM, �lvaro Hern�ndez Tortosa wrote:

The fact that you null terminate them (fine with me) doesn't change
my reasoning. How do you separate multiple channel binding methods? And
do you realize that you will be repeating the channel binding methods
without reason? A contrived but legal, possible example:

Field1:
SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-512-PLUS tls-unique tls-awesome yet-another-tls\0
SCRAM-SHA-999-PLUS tls-unique tls-awesome yet-another-tls\0

I was actually thinking of:

SCRAM-SHA-256\0
SCRAM-SHA-512\0
SCRAM-SHA-999\0
SCRAM-SHA-256-PLUS\0
SCRAM-SHA-256-PLUS tls-awesome\0
SCRAM-SHA-256-PLUS yet-another-tls\0
SCRAM-SHA-512-PLUS\0
SCRAM-SHA-512-PLUS tls-awesome\0
SCRAM-SHA-512-PLUS yet-another-tls\0
SCRAM-SHA-999-PLUS\0
SCRAM-SHA-999-PLUS tls-awesome\0
SCRAM-SHA-999-PLUS yet-another-tls\0

In practice, I don't foresee us having this many options, so the
verbosity won't be an issue. Parsing this is very straightforward.

That's maybe slightly better, since -I agree- verbosity is not an
issue. But parsing (parsers, and validators) are still more complex (you
need to check that if suffix is -PLUS you need to split by space and
find another field with another format based on another lookup table of
IANA registry names and so forth). Vs: this field is for SCRAM names,
this field is for channel binding names. Done.

Let me exemplify. In Java-ish syntax, your type would be something
like:

List<Pair<ScramMechanism,ChannelBindingType>> from where you need to
extract individually ScramMechanisms and unique(ChannelBindingType)

My proposal would have two lists:

List<ScramMechanism>
List<ChannelBindingType>

which is exactly what you need.

So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice :)

vs

Field 1:
SCRAM-SHA-256,SCRAM-SHA-256-PLUS,SCRAM-SHA-1-PLUS,SCRAM-SHA-512-PLUS
(simple CSV)
Field 2: tls-unique (String)

What if tls-unique is only supported with SCRAM-SHA-256-PLUS, while
SCRAM-SHA-512-PLUS requires tls-awesome?

It can't happen. The RFC clearly states that they are orthogonal.
It is left to the implementations support one or the other, but no
reason to limit applicability of a given binding method to a given SCRAM
mechanisms (or viceversa).

Well, if tls-unique is found to be insecure, a future
SCRAM-SHA-512-PLUS spec might well define that the default for that
mechanism is tls-unique-new-and-secure rather than tls-unique. Maybe
even forbid using tls-unique altogether. I don't think that's likely,
but this is all about future-proofing, so I'd rather keep it flexible.

If it would be insecure, I'd immediately stop it from being
advertised, and problem solved. Nothing would change (under my proposal).

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/11/2017 02:32 PM, �lvaro Hern�ndez Tortosa wrote:

So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice :)

So, here's my more full-fledged proposal.

The first patch refactors libpq code, by moving the responsibility of
reading the GSS/SSPI/SASL/MD5 specific data from the authentication
request packet, from the enormous switch-case construct in
PQConnectPoll(), into pg_fe_sendauth(). This isn't strictly necessary,
but I think it's useful cleanup anyway, and now that there's a bit more
structure in the AuthenticationSASL message, the old way was getting
awkward.

The second patch contains the protocol changes, and adds the
documentation for it.

- Heikki

On 12/04/17 19:34, Heikki Linnakangas wrote:

On 04/11/2017 02:32 PM, �lvaro Hern�ndez Tortosa wrote:

So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice :)

So, here's my more full-fledged proposal.

The first patch refactors libpq code, by moving the responsibility of
reading the GSS/SSPI/SASL/MD5 specific data from the authentication
request packet, from the enormous switch-case construct in
PQConnectPoll(), into pg_fe_sendauth(). This isn't strictly necessary,
but I think it's useful cleanup anyway, and now that there's a bit
more structure in the AuthenticationSASL message, the old way was
getting awkward.

The second patch contains the protocol changes, and adds the
documentation for it.

- Heikki

Hi Heikki.

Thanks for the patches :)

By looking at the them, and unless I'm missing something, I don't
see how the extra information for the future implementation of channel
binding would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:
<variablelist>
<varlistentry>
<term>
String
</term>
<listitem>
<para>
Name of a SASL authentication mechanism.
</para>
</listitem>
</varlistentry>
</variablelist>

How do you plan to implement it, in future versions, without
modifying the AuthenticationSASL message? Or is it OK to add new fields
to a message in future PostgreSQL versions, without considering that a
protocol change?

On a side note, I'd mention that the list of SASL authentication
mechanisms contains valid IANA Registry SCRAM names
(https://www.iana.org/assignments/sasl-mechanisms/sasl-mechanisms.xhtml#scram)for
SCRAM authentication messages (making it more clear what values would be
expected there from the client).

I hope to start testing this from Java the coming weekend. I will
keep you posted.

�lvaro

--

�lvaro Hern�ndez Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Apr 13, 2017 at 2:34 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 04/11/2017 02:32 PM, Álvaro Hernández Tortosa wrote:

So I still see your proposal more awkward and less clear, mixing
things that are separate. But again, your choice :)

So, here's my more full-fledged proposal.

The first patch refactors libpq code, by moving the responsibility of
reading the GSS/SSPI/SASL/MD5 specific data from the authentication request
packet, from the enormous switch-case construct in PQConnectPoll(), into
pg_fe_sendauth(). This isn't strictly necessary, but I think it's useful
cleanup anyway, and now that there's a bit more structure in the
AuthenticationSASL message, the old way was getting awkward.

The second patch contains the protocol changes, and adds the documentation
for it.

Thanks for the updated patches! I had a close look at them.

Let's begin with 0001...

            /*
             * Negotiation generated data to be sent to the client.
             */
-           elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+           elog(DEBUG4, "sending SASL challenge of length %u", outputlen);

sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
+
+ pfree(output);
}
This will leak one byte if output is of length 0. I think that the
pfree call should be moved out of this if condition and only called if
output is not NULL. That's a nit, and in the code this scenario cannot
happen, but I would recommend to be correct.

+static int
+pg_SASL_init(PGconn *conn, int payloadLen)
 {
+   char        auth_mechanism[21];
So this assumes that any SASL mechanism name will never be longer than
20 characters. Looking at the link of IANA that Alvaro is providing
upthread this is true, could you add a comment that this relies on
such an assumption?

+   if (initialresponselen != 0)
+   {
+       res = pqPacketSend(conn, 'p', initialresponse, initialresponselen);
+       free(initialresponse);
+
+       if (res != STATUS_OK)
+           return STATUS_ERROR;
+   }
Here also initialresponse could be free'd only if it is not NULL.

And now for 0002...

No much changes in the docs I like the split done for GSS and SSPI messages.

+       /*
+        * The StringInfo guarantees that there's a \0 byte after the
+        * response.
+        */
+       Assert(input[inputlen] == '\0');
+       pq_getmsgend(&buf);
Shouldn't this also check input == NULL? This could crash the
assertion and pg_be_scram_exchange is able to handle a NULL input
message.

+    * Parse the list of SASL authentication mechanisms in the
+    * AuthenticationSASL message, and select the best mechanism that we
+    * support.  (Only SCRAM-SHA-256 is supported at the moment.)
     */
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:

By looking at the them, and unless I'm missing something, I don't see
how the extra information for the future implementation of channel binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:
<variablelist>
<varlistentry>
<term>
String
</term>
<listitem>
<para>
Name of a SASL authentication mechanism.
</para>
</listitem>
</varlistentry>
</variablelist>
How do you plan to implement it, in future versions, without modifying
the AuthenticationSASL message? Or is it OK to add new fields to a message
in future PostgreSQL versions, without considering that a protocol change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/13/2017 05:54 AM, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:

By looking at the them, and unless I'm missing something, I don't see
how the extra information for the future implementation of channel binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:
<variablelist>
<varlistentry>
<term>
String
</term>
<listitem>
<para>
Name of a SASL authentication mechanism.
</para>
</listitem>
</varlistentry>
</variablelist>
How do you plan to implement it, in future versions, without modifying
the AuthenticationSASL message? Or is it OK to add new fields to a message
in future PostgreSQL versions, without considering that a protocol change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.

Right, when we get channel binding, the server will list "SCRAM-SHA-256"
and "SCRAM-SHA-256-PLUS" as the list of mechanisms. And if we get
channel binding using something else than tls-unique, then those will be
added as extra mechanisms, too, e.g. "SCRAM-SHA-256-PLUS tls-awesome".

I don't think that needs to be discussed in the docs yet, because a
client will simply ignore any mechanisms it doesn't understand. Although
perhaps it would be good to mention explicitly that even though SASL
defines that mechanism names have a particular format - all ASCII
upper-case, max 20 chars - the client should accept and ignore arbitrary
strings, not limited to that format.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13/04/17 13:24, Heikki Linnakangas wrote:

On 04/13/2017 05:54 AM, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:

By looking at the them, and unless I'm missing something, I
don't see
how the extra information for the future implementation of channel
binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator
after
the last authentication mechanism name. For each mechanism, there is
the
following:
<variablelist>
<varlistentry>
<term>
String
</term>
<listitem>
<para>
Name of a SASL authentication mechanism.
</para>
</listitem>
</varlistentry>
</variablelist>
How do you plan to implement it, in future versions, without
modifying
the AuthenticationSASL message? Or is it OK to add new fields to a
message
in future PostgreSQL versions, without considering that a protocol
change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.

Right, when we get channel binding, the server will list
"SCRAM-SHA-256" and "SCRAM-SHA-256-PLUS" as the list of mechanisms.
And if we get channel binding using something else than tls-unique,
then those will be added as extra mechanisms, too, e.g.
"SCRAM-SHA-256-PLUS tls-awesome".

And how about supporting different SCRAM mechanisms with different
possible channel bindings? Separate by space too? So given a field, is
the first item the SCRAM mechanism, and all the remaning the channel
binding methods? I.e.:

SCRAM-SHA-256-PLUS tls-awesome tls-awesome2 tls-awesome3\0...

Please note that if this is the solution chosen:

- A lot of parsing and convention is required (first arg is the SCRAM
mechanism, succesive are channel binding; tls-unique is always
"implied", etc)
- Channel binding names will be repeated for every SCRAM mechanism with
"-PLUS". This is not needed, SCRAM mechanisms and channel binding are
separate things.
- Channel binding names will not be present on others, making the parser
even more complex.

All this vs, again, stating SCRAM mechanisms on one list and
channel binding on another list, which is to my much more KISS. But...
anyway, if this is the decision made, at least I think this should be
documented now, because client parsers need to be designed one way or
another.

Thanks,

Álvaro

--

Álvaro Hernández Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13/04/17 04:54, Michael Paquier wrote:

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
<aht@8kdata.com> wrote:

By looking at the them, and unless I'm missing something, I don't see
how the extra information for the future implementation of channel binding
would be added (without changing the protocol). Relevant part is:

The message body is a list of SASL authentication mechanisms, in the
server's order of preference. A zero byte is required as terminator after
the last authentication mechanism name. For each mechanism, there is the
following:
<variablelist>
<varlistentry>
<term>
String
</term>
<listitem>
<para>
Name of a SASL authentication mechanism.
</para>
</listitem>
</varlistentry>
</variablelist>
How do you plan to implement it, in future versions, without modifying
the AuthenticationSASL message? Or is it OK to add new fields to a message
in future PostgreSQL versions, without considering that a protocol change?

I don't quite understand the complain here, it is perfectly fine to
add as many null-terminated names as you want with this model. The
patches would make the server just send one mechanism name now, but
nothing prevents the addition of more.

I think I explained in my previous reply, but just in case: there
are two lists here: SCRAM mechanism and channel binding mechanisms. They
are orthogonal, you could pick them separately (only with the -PLUS
variants, of course). All two (both SCRAM and channel binding
mechanisms) have to be advertised by the server.

Álvaro

--

Álvaro Hernández Tortosa

-----------
<8K>data

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 04/13/2017 02:35 PM, Álvaro Hernández Tortosa wrote:

On 13/04/17 13:24, Heikki Linnakangas wrote:

Right, when we get channel binding, the server will list
"SCRAM-SHA-256" and "SCRAM-SHA-256-PLUS" as the list of mechanisms.
And if we get channel binding using something else than tls-unique,
then those will be added as extra mechanisms, too, e.g.
"SCRAM-SHA-256-PLUS tls-awesome".

And how about supporting different SCRAM mechanisms with different
possible channel bindings? Separate by space too? So given a field, is
the first item the SCRAM mechanism, and all the remaning the channel
binding methods? I.e.:

SCRAM-SHA-256-PLUS tls-awesome tls-awesome2 tls-awesome3\0...

I think we're going in circles.. Yeah, we could do that. Or they could
be listed as separate mechanisms:

SCRAM-SHA-256-PLUS\0
SCRAM-SHA-256-PLUS tls-awesome\0
SCRAM-SHA-256-PLUS tls-awesome2\0
SCRAM-SHA-256-PLUS tls-awesome3\0
\0

One alternative is that you could list extra channel bindings that are
supported by all the mechanisms, as separate entries. So the list could be:

binding tls-unique\0
binding tls-awesome\0
binding tls-awesome2\0
binding tls-awesome3\0
SCRAM-SHA-256-PLUS\0
SCRAM-SHA-512-PLUS\0
\0

which would mean that those bindings are supported by all the mechanisms
that follow. I think this would achieve the same thing as your proposed
separate field, with the current proposed protocol.

But again, I'm 99% sure we won't need it, and we don't need to decide
the exact syntax for channel bindings yet. We have the flexibility now,
so we can cross the bridge when we get there.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

My only desire would be to have a final spec and implement the full parser
now, not have to change it in the future. We already know today all the
requirements, so please pick one and I will follow it :)

On Apr 13, 2017 13:47, "Heikki Linnakangas" <hlinnaka@iki.fi> wrote:

On 04/13/2017 05:53 AM, Michael Paquier wrote:

Thanks for the updated patches! I had a close look at them.

Let's begin with 0001...

/*
* Negotiation generated data to be sent to the client.
*/
-           elog(DEBUG4, "sending SASL response token of length %u", outputlen);
+           elog(DEBUG4, "sending SASL challenge of length %u", outputlen);

sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
+
+ pfree(output);
}
This will leak one byte if output is of length 0. I think that the
pfree call should be moved out of this if condition and only called if
output is not NULL. That's a nit, and in the code this scenario cannot
happen, but I would recommend to be correct.

Fixed.

+static int
+pg_SASL_init(PGconn *conn, int payloadLen)
{
+   char        auth_mechanism[21];
So this assumes that any SASL mechanism name will never be longer than
20 characters. Looking at the link of IANA that Alvaro is providing
upthread this is true, could you add a comment that this relies on
such an assumption?

I picked 20 characters for the buffer, because that's what the SASL spec
says is the maximum, but note that the code doesn't actually rely on
that. It checks the length of the incoming string, and throws a "SASL
mechanism not supported" error if it doesn't fit in the buffer. 20 is
enough to hold "SCRAM-SHA-256", which is the only supported mechanism.

Also note that the second patch replaces this code, anyway..

+   if (initialresponselen != 0)
+   {
+       res = pqPacketSend(conn, 'p', initialresponse, initialresponselen);
+       free(initialresponse);
+
+       if (res != STATUS_OK)
+           return STATUS_ERROR;
+   }
Here also initialresponse could be free'd only if it is not NULL.

Fixed.

And now for 0002...

No much changes in the docs I like the split done for GSS and SSPI messages.

+       /*
+        * The StringInfo guarantees that there's a \0 byte after the
+        * response.
+        */
+       Assert(input[inputlen] == '\0');
+       pq_getmsgend(&buf);
Shouldn't this also check input == NULL? This could crash the
assertion and pg_be_scram_exchange is able to handle a NULL input
message.

Yep, fixed!

+    * Parse the list of SASL authentication mechanisms in the
+    * AuthenticationSASL message, and select the best mechanism that we
+    * support.  (Only SCRAM-SHA-256 is supported at the moment.)
*/
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).

Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported
mechanism. In general, there is no way to tell libpq to e.g. not do
plain password authentication, which is more pressing than choosing a
particular SASL mechanism. So I think we should have libpq options to
control that, but it's a bigger feature than just adding a debug
environment variable here.

Thanks for the review! I've pushed these patches, after a bunch of
little cleanups here and there, and fixing a few garden-variety bugs in
the GSS/SSPI changes.

Álvaro, you're good to go and implement the JDBC support based on this.
Thanks! Please keep me informed on how it goes, and let me know if you
run into any trouble, or if there's any discrepancies or ambiguity in
the docs that we should fix.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Apr 14, 2017 at 1:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 04/13/2017 05:53 AM, Michael Paquier wrote:

+    * Parse the list of SASL authentication mechanisms in the
+    * AuthenticationSASL message, and select the best mechanism that we
+    * support.  (Only SCRAM-SHA-256 is supported at the moment.)
*/
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).

Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported
mechanism. In general, there is no way to tell libpq to e.g. not do plain
password authentication, which is more pressing than choosing a particular
SASL mechanism. So I think we should have libpq options to control that, but
it's a bigger feature than just adding a debug environment variable here.

Of course, my last sentence implied that this may be useful once more
than 1 mechanism is added. This definitely cannot be a connection
parameter. Your last sentence makes me guess that we agree on that.
But those are thoughts for later..

Thanks for the review! I've pushed these patches, after a bunch of little
cleanups here and there, and fixing a few garden-variety bugs in the
GSS/SSPI changes.

Committed patches look good to me after a second lookup. Thanks!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 14 Apr. 2017 10:44, "Michael Paquier" <michael.paquier@gmail.com> wrote:

On Fri, Apr 14, 2017 at 1:37 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 04/13/2017 05:53 AM, Michael Paquier wrote:

+    * Parse the list of SASL authentication mechanisms in the
+    * AuthenticationSASL message, and select the best mechanism that we
+    * support.  (Only SCRAM-SHA-256 is supported at the moment.)
*/
-   if (strcmp(auth_mechanism, SCRAM_SHA256_NAME) == 0)
+   for (;;)
Just an idea here: being able to enforce the selection with an
environment variable (useful for testing as well in the future).

Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported
mechanism. In general, there is no way to tell libpq to e.g. not do plain
password authentication, which is more pressing than choosing a particular
SASL mechanism. So I think we should have libpq options to control that,

but

it's a bigger feature than just adding a debug environment variable here.

Of course, my last sentence implied that this may be useful once more
than 1 mechanism is added. This definitely cannot be a connection
parameter. Your last sentence makes me guess that we agree on that.
But those are thoughts for later..

Are we going to have issues with with mech negotiation re the ability to
store auth data for >1 mech and access it early enough?

Presumably we'll need multiple digests for a user. For example if we want
to allow the choice of mechs scram-256 and scram-512 we need different
stored hashes for the same user in pg_authid. And we'll possibly need to be
able to tell at the time we advertise mechs which users have creds for
which mechs otherwise we'll advertise mechs they can never succeed. The
client has no way to make a sensible choice of mech if some arbitrary
subset (maybe just 1) will work for a given user.

There's no point advertising scram-512 if only -256 can work for 'bob'
because that's what we have in pg_authid.

Yes, filtering the advertised mechs exposes info. But not being able to log
in if you're the legitimate user without configuring the client with your
password hash format would suck too.

Thanks for the review! I've pushed these patches, after a bunch of little
cleanups here and there, and fixing a few garden-variety bugs in the
GSS/SSPI changes.

Committed patches look good to me after a second lookup. Thanks!
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Apr 14, 2017 at 8:28 PM, Craig Ringer
<craig.ringer@2ndquadrant.com> wrote:

There's no point advertising scram-512 if only -256 can work for 'bob'
because that's what we have in pg_authid.

The possibility to have multiple verifiers has other benefits than
that, password rolling being one. We may want to revisit that once
there is a need to have a pg_auth_verifiers, my intuition on the
matter is that we are years away from it, but we'll very likely need
it for more reasons than the one you are raising here.

Yes, filtering the advertised mechs exposes info. But not being able to log
in if you're the legitimate user without configuring the client with your
password hash format would suck too.

Yup.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers