Horton

Thread

Commits

  1. Improve the SASL authentication protocol.

  2. Refactor libpq authentication request processing.

  3. Minor cleanup of backend SCRAM code.

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

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-04T06:02:30Z

    Hi all,

There is still one open item pending for SCRAM that has not been
treated which is mentioned here:
https://www.postgresql.org/message-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.

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

Thoughts?
-- 
Michael

  2. Re: Letting the client choose the protocol to use during a SASL exchange

    Noah Misch <noah@leadboat.com> — 2017-04-06T05:13:01Z

    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:
> https://www.postgresql.org/message-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] 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] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

  3. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-06T17:05:11Z

    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

  4. Re: Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-06T17:23:09Z

    
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

  5. Re: Letting the client choose the protocol to use during a SASL exchange

    Simon Riggs <simon@2ndquadrant.com> — 2017-04-06T19:49:08Z

    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:
> https://www.postgresql.org/message-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

  6. Re: Letting the client choose the protocol to use during a SASL exchange

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-06T20:05:32Z

    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

  7. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-06T20:15:43Z

    
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

  8. Re: Letting the client choose the protocol to use during a SASL exchange

    Simon Riggs <simon@2ndquadrant.com> — 2017-04-06T20:16:10Z

    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

  9. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-06T22:13:16Z

    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

  10. Re: Letting the client choose the protocol to use during a SASL exchange

    Simon Riggs <simon@2ndquadrant.com> — 2017-04-06T22:29:42Z

    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

  11. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-06T22:33:36Z

    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

  12. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-07T08:17:43Z

    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

  13. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-07T08:33:52Z

    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

  14. Re: Letting the client choose the protocol to use during a SASL exchange

    Craig Ringer <craig@2ndquadrant.com> — 2017-04-07T08:57:52Z

    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

  15. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-07T09:08:44Z

    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

  16. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-10T12:57:36Z

    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

  17. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-10T18:33:54Z

    
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

  18. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-10T19:41:10Z

    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

  19. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-10T20:03:37Z

    
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

  20. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-11T04:23:57Z

    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

  21. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-11T06:50:59Z

    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], but we forgot about it since.

[1] https://www.postgresql.org/message-id/551A8C2F.7050409%40iki.fi

- Heikki

  22. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-11T08:55:36Z

    
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

  23. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-11T10:23:29Z

    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

  24. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-11T10:39:18Z

    
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

  25. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-11T11:21:47Z

    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

  26. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-11T11:32:58Z

    
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

  27. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-12T17:34:38Z

    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

  28. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-12T21:37:37Z

    
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

  29. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-13T02:53:19Z

    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

  30. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-13T02:54:59Z

    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

  31. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-13T11:24:28Z

    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

  32. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-13T11:35:03Z

    
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

  33. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-13T11:36:49Z

    
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

  34. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-13T11:47:06Z

    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

  35. Re: Letting the client choose the protocol to use during a SASL exchange

    Álvaro Hernández Tortosa <aht@8kdata.com> — 2017-04-13T11:57:03Z

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

  36. Re: Letting the client choose the protocol to use during a SASL exchange

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-04-13T16:37:33Z

    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

  37. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-14T02:42:52Z

    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

  38. Re: Letting the client choose the protocol to use during a SASL exchange

    Craig Ringer <craig.ringer@2ndquadrant.com> — 2017-04-14T11:28:05Z

    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

  39. Re: Letting the client choose the protocol to use during a SASL exchange

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-14T12:26:44Z

    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