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

On Thu, Apr 13, 2017 at 2:34 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 04/11/2017 02:32 PM, Álvaro Hernández Tortosa wrote:
>>
>> So I still see your proposal more awkward and less clear, mixing
>> things that are separate. But again, your choice :)
>
>
> So, here's my more full-fledged proposal.
>
> The first patch refactors libpq code, by moving the responsibility of
> reading the GSS/SSPI/SASL/MD5 specific data from the authentication request
> packet, from the enormous switch-case construct in PQConnectPoll(), into
> pg_fe_sendauth(). This isn't strictly necessary, but I think it's useful
> cleanup anyway, and now that there's a bit more structure in the
> AuthenticationSASL message, the old way was getting awkward.
>
> The second patch contains the protocol changes, and adds the documentation
> for it.

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

Let's begin with 0001...

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

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

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

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

And now for 0002...

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

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

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