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