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.


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


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

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:

Reply via email to