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

On 04/13/2017 05:53 AM, Michael Paquier wrote:
> Thanks for the updated patches! I had a close look at them.
>
> Let's begin with 0001...
>
>             /*
>              * Negotiation generated data to be sent to the client.
>              */
> -           elog(DEBUG4, "sending SASL response token of length %u", outputlen);
> +           elog(DEBUG4, "sending SASL challenge of length %u", outputlen);
>
>             sendAuthRequest(port, AUTH_REQ_SASL_CONT, output, outputlen);
> +
> +           pfree(output);
>         }
> This will leak one byte if output is of length 0. I think that the
> pfree call should be moved out of this if condition and only called if
> output is not NULL. That's a nit, and in the code this scenario cannot
> happen, but I would recommend to be correct.

Fixed.

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

I picked 20 characters for the buffer, because that's what the SASL spec 
says is the maximum, but note that the code doesn't actually rely on 
that. It checks the length of the incoming string, and throws a "SASL 
mechanism not supported" error if it doesn't fit in the buffer. 20 is 
enough to hold "SCRAM-SHA-256", which is the only supported mechanism.

Also note that the second patch replaces this code, anyway..

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

Fixed.

> And now for 0002...
>
> No much changes in the docs I like the split done for GSS and SSPI messages.
>
> +       /*
> +        * The StringInfo guarantees that there's a \0 byte after the
> +        * response.
> +        */
> +       Assert(input[inputlen] == '\0');
> +       pq_getmsgend(&buf);
> Shouldn't this also check input == NULL? This could crash the
> assertion and pg_be_scram_exchange is able to handle a NULL input
> message.

Yep, fixed!

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

Hmm. It wouldn't do much, as long as SCRAM-SHA-256 is the only supported 
mechanism. In general, there is no way to tell libpq to e.g. not do 
plain password authentication, which is more pressing than choosing a 
particular SASL mechanism. So I think we should have libpq options to 
control that, but it's a bigger feature than just adding a debug 
environment variable here.

Thanks for the review! I've pushed these patches, after a bunch of 
little cleanups here and there, and fixing a few garden-variety bugs in 
the GSS/SSPI changes.

Álvaro, you're good to go and implement the JDBC support based on this. 
Thanks! Please keep me informed on how it goes, and let me know if you 
run into any trouble, or if there's any discrepancies or ambiguity in 
the docs that we should fix.

- Heikki