Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Fri, Apr 14, 2017 at 8:28 PM, Craig Ringer
 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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Craig Ringer]

On 14 Apr. 2017 10:44, "Michael Paquier"  wrote:

On Fri, Apr 14, 2017 at 1:37 AM, Heikki Linnakangas  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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Fri, Apr 14, 2017 at 1:37 AM, Heikki Linnakangas  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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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)
 {
+   charauth_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();
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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 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:



 String



 Name of a SASL authentication mechanism.




 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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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



String



Name of a SASL authentication mechanism.




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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 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:



String



Name of a SASL authentication mechanism.




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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Thu, Apr 13, 2017 at 6:37 AM, Álvaro Hernández Tortosa
 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:
> 
> 
> 
> String
> 
> 
> 
> Name of a SASL authentication mechanism.
> 
> 
> 
> 
> 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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Thu, Apr 13, 2017 at 2:34 AM, 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.

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)
 {
+   charauth_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();
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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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:



String



Name of a SASL authentication mechanism.






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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

>From 29c958dab9f8ece5d855b335c09cc9125606774a Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Wed, 12 Apr 2017 16:01:18 +0300
Subject: [PATCH 1/2] Refactor libpq authentication request processing.

Move the responsibility of reading the data from the authentication request
message from PQconnectPoll() to pg_fe_sendauth(). This way, PQconnectPoll()
doesn't need to know about the different authentication request types,
and we don't need the extra fields in the pg_conn struct to pass the data
from PQconnectPoll() to pg_fe_sendauth() anymore.

In the backend, free each SASL message after sending it. It's not a lot
of wasted memory, but a leak nevertheless. Adding the pfree() revealed
a little bug in build_server_first_message(): It attempts to keeps a copy
of the sent message, but it was missing a pstrdup(), so the pointer
started to dangle, after adding the pfree() into CheckSCRAMAuth().
---
 src/backend/libpq/auth-scram.c|  10 +-
 src/backend/libpq/auth.c  |  10 +-
 src/interfaces/libpq/fe-auth.c| 214 +-
 src/interfaces/libpq/fe-auth.h|   2 +-
 src/interfaces/libpq/fe-connect.c | 113 +++-
 src/interfaces/libpq/fe-misc.c|  13 +++
 src/interfaces/libpq/libpq-int.h  |  11 +-
 7 files changed, 204 insertions(+), 169 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 5077ff33b1..a47c48d980 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -161,10 +161,10 @@ static char *scram_MockSalt(const char *username);
  * needs to be called before doing any exchange.  It will be filled later
  * after the beginning of the exchange with verifier data.
  *
- * 'username' is the provided by the client.  'shadow_pass' is the role's
- * password verifier, from pg_authid.rolpassword.  If 'shadow_pass' is NULL, we
- * still perform an authentication exchange, but it will fail, as if an
- * incorrect password was given.
+ * 'username' is the username provided by the client in the startup message.
+ * 'shadow_pass' is the role's password verifier, from pg_authid.rolpassword.
+ * If 'shadow_pass' is NULL, we still perform an authentication exchange, but
+ * it will fail, as if an incorrect password was given.
  */
 void *
 pg_be_scram_init(const char *username, const char *shadow_pass)
@@ -984,7 +984,7 @@ build_server_first_message(scram_state *state)
  state->client_nonce, state->server_nonce,
  state->salt, state->iterations);
 
-	return state->server_first_message;
+	return pstrdup(state->server_first_message);
 }
 
 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 66ead9381d..681b93855f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -872,6 +872,8 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 	strlen(SCRAM_SHA256_NAME) + 1);
 
 	/*
+	 * Initialize the status tracker for message exchanges.
+	 *
 	 * If the user doesn't exist, or doesn't have a valid password, or it's
 	 * expired, we still go through the motions of SASL authentication, but
 	 * tell the authentication method that the authentication is "doomed".
@@ -880,8 +882,6 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 	 * This is because we don't want to reveal to an attacker what usernames
 	 * are valid, nor which users have a valid password.
 	 */
-
-	/* Initialize the status tracker for message exchanges */
 	scram_opaq = pg_be_scram_init(port->user_name, shadow_pass);
 
 	/*
@@ -918,7 +918,7 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			return STATUS_ERROR;
 		}
 
-		elog(DEBUG4, "Processing received SASL token of length %d", buf.len);
+		elog(DEBUG4, "Processing received SASL response of length %d", buf.len);
 
 		/*
 		 * we pass 'logdetail' as NULL when doing a mock authentication,
@@ -936,9 +936,11 @@ CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail)
 			/*
 			 * 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, 

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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> from where you need to 
extract individually ScramMechanisms and unique(ChannelBindingType)


My proposal would have two lists:

List
List

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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 

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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



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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Tue, Apr 11, 2017 at 4:41 AM, Heikki Linnakangas  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.

+  
+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)
+  
Perhaps those should be links to the RFCs.

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

+  server-final-message in the "additional data" field.
+
+
+
+
I think that you are missing a  markup here.

+
+
+Authentication method specific "additional data". If this
+message contains no additional data, this field is omitted.
+
+
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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa  
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



0001-Add-docs-for-SASL-authentication-in-protocol.patch
Description: invalid/octet-stream

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

On 7 April 2017 at 16:33, Heikki Linnakangas  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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Craig Ringer]

On 7 April 2017 at 16:33, Heikki Linnakangas  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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Heikki Linnakangas]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Fri, Apr 7, 2017 at 7:29 AM, Simon Riggs  wrote:
> On 6 April 2017 at 16:15, Álvaro Hernández Tortosa  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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Simon Riggs]

On 6 April 2017 at 16:15, Álvaro Hernández Tortosa  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 Riggshttp://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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

On Fri, Apr 7, 2017 at 5:15 AM, Álvaro Hernández Tortosa  
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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Simon Riggs]

On 6 April 2017 at 16:05, 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,

+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 Riggshttp://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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Álvaro Hernández Tortosa]

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

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Tom Lane]

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

Re: [HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Simon Riggs]

On 4 April 2017 at 02:02, Michael Paquier  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-e012333d5...@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 Riggshttp://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

[HACKERS] Letting the client choose the protocol to use during a SASL exchange

[Michael Paquier]

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-e012333d5...@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


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