On Tue, Aug 20, 2019 at 07:09:25PM -0700, Jeff Davis wrote: > OK, new patch attached. Seems like everyone is in agreement that we > need a channel_binding param.
+ <para> + A setting of <literal>require</literal> means that the connection must + employ channel binding; and that the client will not respond to + requests from the server for cleartext password or MD5 + authentication. The default setting is <literal>prefer</literal>, + which employs channel binding if available. + </para> 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... +#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. + with <productname>PostgreSQL</productname> 11.0 or later servers using Here I would use PostgreSQL 11, only mentioning the major version as it was also available at beta time. case AUTH_REQ_OK: + if (conn->channel_binding[0] == 'r' && !conn->channel_bound) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("Channel binding required but not offered by server\n")); + return STATUS_ERROR; + } 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. 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". + 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. -- Michael
signature.asc
Description: PGP signature