Re: [JDBC] Channel binding support for SCRAM-SHA-256

On 9/10/17 22:37, Michael Paquier wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
>
> And again a new set to fix the rotten bits caused by 85f4d63.

Here is a review of the meat of the code, leaving aside the discussion
of the libpq connection parameters.

Overall, the structure of the code makes sense and it fits in well with
the existing SCRAM code.

I think the channel-binding negotiation on the client side is wrong.
The logic in the patch is

+#ifdef USE_SSL
+ if (state->ssl_in_use)
+ appendPQExpBuffer(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
+ else
+ appendPQExpBuffer(&buf, "y");
+#else
+ appendPQExpBuffer(&buf, "n");
+#endif

But if SSL is compiled in but not used, the client does not in fact
support channel binding (for that connection), so it should send "n".

The "y" flag should be sent if ssl_in_use but the client did not see the
server advertise SCRAM-SHA256-PLUS. That logic is missing entirely in
this patch.

You have the server reject a client that does not support channel
binding ("n") on all SSL connections. I don't think this is correct.
It is up to the client to use channel binding or not, even on SSL
connections.

We should update pg_hba.conf to allow a method specification of
"scram-sha256-plus", i.e., only advertise the channel binding variant to
the client. Then you could make policy decisions like rejecting clients
that do not use channel binding on SSL connections. This could be a
separate patch later.

The error message in the "p" case if SSL is not in use is a bit
confusing: "but the server does not need it". I think this could be
left at the old message "but it is not supported". This ties into my
interpretation from above that whether channel binding is "supported"
depends on whether SSL is in use for a particular connection.

Some small code things:

- prefer to use size_t over int for length (tls_finish_len etc.)

- tls_finish should be tls_finished

- typos: certificate_bash -> certificate_hash

In the patch for tls-server-end-point, I think the selection of the hash
function deviates slightly from the RFC. The RFC only says to
substitute MD5 and SHA-1. It doesn't say to turn SHA-224 into SHA-256,
for example. There is also the problem that the code as written will
turn any unrecognized hash method into SHA-256. If think the code
should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
for the rest. (I don't know anything about the details of OpenSSL APIs,
but that function sounded right to me.)

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services