Re: Negotiating the SCRAM channel binding type - Mailing list pgsql-hackers

On Fri, Jul 20, 2018 at 08:05:04PM +0300, Heikki Linnakangas wrote:
> In any case, I'm quite convinced now that we should just remove tls-unique,
> and stick to tls-server-end-point. Patch attached, please take a look.

You are making good points here.

> This removes the scram_channel_binding libpq option altogether, since there
> is now only one supported channel binding type.

This can also be used to switch off channel binding on the client-side,
which is the behavior you could get only by using a version of libpq
compiled with v10 in the context of an SSL connection.  Do we really
want to lose this switch as well?  I like feature switches.

-   "SCRAM authentication with invalid channel binding");
+   "Basic SCRAM authentication");
Or "SCRAM authentication with SSL and channel binding?"

+PostgreSQL is <literal>tls-server-end-point</literal>.  If other channel
+binding types are supported in the future, the server will advertise
+them as separate SASL mechanisms.
I don't think that this is actually true, per my remark of upthread we
could also use an option-based approach with each SASL mechanism sent by
the server.  I would recommend to remove this last sentence.

+   man-in-the-middle attacks by mixing the signature of the server's
+   certificate into the transmitted password hash. While a fake server can
+   retransmit the real server's certificate, it doesn't have access to the
+   private key matching that certificate, and therefore cannot prove it is
+   the owner, causing SSL connection failure
Nit: insisting on the fact that tls-server-end-point is used in this
context.

+void
+pg_be_scram_get_mechanisms(Port *port, StringInfo buf)
+{
+   /*
+    * Advertise the mechanisms in decreasing order of importance.  So the
+    * channel-binding variants go first, if they are supported. Channel
+    * binding is only supported with SSL, and only if the SSL implementation
+    * has a function to get the certificate's hash
[...]
+#ifdef HAVE_BE_TLS_GET_CERTIFICATE_HASH
+   if (strcmp(selected_mech, SCRAM_SHA_256_PLUS_NAME) == 0 && port->ssl_in_use)
+       state->channel_binding_in_use = true;
+   else
+#endif
Hm.  I think that this should be part of the set of APIs that each SSL
implementation has to provide.  It is not clear to me yet if using the
flavor of SSL in Windows or macos universe will allow end-point to work,
and this could make this proposal more complicated.  And
HAVE_BE_TLS_GET_CERTIFICATE_HASH is something OpenSSL-ish, so I would
recommend to remove all SSL-implementation-specific code from auth*.c
and fe-auth*.c, keeping those in their own file.  One simple way to
address this problem would be to make each SSL implementation return a
boolean to tell if it supports SCRAM channel binding or not, with Port*
of PGconn* in input to be able to filter connections using SSL or not.

+    if (strcmp(channel_binding_type, "tls-server-end-point") != 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_PROTOCOL_VIOLATION),
+                (errmsg("unsupported SCRAM channel-binding type
\"%s\"",
+                        sanitize_str(channel_binding_type)))));
Let's give up on sanitize_str.  I am not sure that it is a good idea to
print in error messages server-side something that the client has sent.

-    * is advertised in decreasing order of importance.  So the
-    * channel-binding variants go first, if they are supported. Channel
-    * binding is only supported in SSL builds.
+    * is advertised in decreasing order of importance.
This comment is a duplicate of what is with pg_be_scram_get_mechanisms.

+#ifdef HAVE_X509_GET_SIGNATURE_NID
char *
pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
{
-#ifdef HAVE_X509_GET_SIGNATURE_NID
I would have kept the ifdef at its original place, to keep the
OpenSSL-specific CFLAGS [be|fe]-secure-openssl.c.

And a couple of lines down the call to be_tls_get_certificate_hash in
auth-scram.c is only protected by USE_SSL...  So compilation would
likely break once a new SSL implementation is added, and libpq-be.h gets
uglier.

+ * The username to  was provided by the client in the startup message, and is
Nit: two spaces between "to" and "was".

+   errdetail("The client selected SCRAM-SHA-256 without channel
binding, but the SCRAM message includes channel binding data,")));
Why a comma at the end?

+       /*
+        * Chose channel binding, but the SSL library doesn't support it.
+        * Shouldn't happen.
+        */
+       termPQExpBuffer(&buf);
+       return NULL;
An error message is misisng here?
--
Michael