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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers