On 11/16/17 16:50, Michael Paquier wrote:
> On Thu, Nov 16, 2017 at 10:47 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> Pushed 0001, will continue with 0002.
>
> Thanks!
I have combed through the 0002 patch in detail now. I have attached my
result.
I cleaned up a bunch of variable names to be more precise. I also
removed some unnecessary code. For example the whole deal about
channel_binding_advertised was not necessary, because it's implied by
the chosen SASL mechanism. We also don't need to have separate USE_SSL
sections in most cases, because state->ssl_in_use already takes care of
that.
I made some significant changes to the logic.
The selection of the channel binding flag (n/y/p) in the client seemed
wrong. Your code treated 'y' as an error, but I think that is a
legitimate case, for example a PG11 client connecting to a PG10 server
over SSL. The client supports channel binding in that case and
(correctly) thinks the server does not, so we use the 'y' flag and
proceed normally without channel binding.
The selection of the mechanism in the client was similarly incorrect, I
think. The code would not tolerate a situation were an SSL connection
is in use but the server does not advertise the -PLUS mechanism, which
again could be from a PG10 server.
The creation of the channel binding data didn't match the spec, because
the gs2-header (p=type,,) was not included in the data put through
base64. This was done incorrectly on both server and client, so the
protocol still worked. (However, in the non-channel-binding case we
hardcode "biws", which is exactly the base64-encoding of the gs2-header.
So that was inconsistent.)
I think we also need to backpatch a bug fix into PG10 so that the server
can accept base64("y,,") as channel binding data. Otherwise, the above
scenario of a PG11 client connecting to a PG10 server over SSL will
currently fail because the server will not accept the channel binding data.
Please check my patch and think through these changes. I'm happy to
commit the patch as is if there are no additional insights.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
