On 05/08/18 15:08, Michael Paquier wrote:
> On Sun, Aug 05, 2018 at 02:00:04PM +0300, Heikki Linnakangas wrote:
>> I did some further testing with this, compiling with and without
>> HAVE_BE_TLS_GET_CERTIFICATE_HASH and HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH,
>> and fixed a few combinations that did not work. And I fixed the other
>> comment typos etc. that you pointed out.
>
> Two things that I am really unhappy about is first that you completely
> wiped out the test suite for channel binding. We know that channel
> binding will be used once HAVE_X509_GET_SIGNATURE_NID is set, hence why
> didn't you keep the check on supports_tls_server_end_point to determine
> if the connection should be a failure or a success?
That test just tested that the scram_channel_binding libpq option works,
but I removed the option. I know you wanted to keep it as a feature
flag, but as discussed earlier, I don't think that'd be useful.
> Then, I also find the meddling around HAVE_X509_GET_SIGNATURE_NID and
> the other flags over-complicated, but I won't fight hard on that point
> if you want to go your way.
I don't feel too strongly about this either, so if you want to write a
patch to refactor that, I'm all ears. Note that I had to do something,
so that the server code knows whether to advertise SCRAM-SHA-256-PLUS or
not, and likewise that the client knows whether to choose channel
binding or not.
- Heikki
