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

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(at)8kdata(dot)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