On Wed, 2019-08-21 at 16:12 +0900, Michael Paquier wrote:
> This counts for other auth requests as well like krb5, no? I think
> that we should also add the "disable" flavor for symmetry, and
> also...
Added "disable" option, and I refactored so that it would look
explicitly for an expected auth req based on the connection parameters.
I had to also make the client tell the server that it does not support
channel binding if channel_binding=disable, otherwise the server
complains.
> +#define DefaultChannelBinding "prefer"
> If the build of Postgres does not support SSL (USE_SSL is not
> defined), I think that DefaultChannelBinding should be "disable".
> That would make things consistent with sslmode.
Done.
> Doing the filtering at the time of AUTH_REQ_OK is necessary for
> "trust", but shouldn't this be done as well earlier for other request
> types? This could save round-trips to the server if we know that an
> exchange begins with a protocol which will never satisfy this
> request, saving efforts for a client doomed to fail after the first
> AUTH_REQ received. That would be the case for all AUTH_REQ, except
> the SASL ones and of course AUTH_REQ_OK.
Done.
>
> Could you please add negative tests in
> src/test/authentication/? What
> could be covered there is that the case where "prefer" (and
> "disable"?) is defined then the authentication is able to go through,
> and that with "require" we get a proper failure as SSL is not used.
> Tests in src/test/ssl/ could include:
> - Make sure that "require" works properly.
> - Test after "disable".
Done.
> + if (conn->channel_binding[0] == 'r')
> Maybe this should comment that this means "require", in a fashion
> similar to what is done when checking conn->sslmode.
Done.
New patch attached.
Regards,
Jeff Davis