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