Date: Mon, 30 Sep 2019 16:08:26 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jeff Davis <jdavis@postgresql.org>,
	pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Add libpq parameter 'channel_binding'.
Message-ID: <20190930070826.GM2888@paquier.xyz>
References: <E1iCVWg-00057D-8U@gemulon.postgresql.org>
 <24857.1569775891@sss.pgh.pa.us>
 <20190929234733.GA2888@paquier.xyz>
 <20190930064539.GK2888@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="oJAv8lSwuaQsYd0G"
Content-Disposition: inline
In-Reply-To: <20190930064539.GK2888@paquier.xyz>
Precedence: bulk


--oJAv8lSwuaQsYd0G
Content-Type: multipart/mixed; boundary="gWRamliw218YCZO9"
Content-Disposition: inline


--gWRamliw218YCZO9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Sep 30, 2019 at 03:45:39PM +0900, Michael Paquier wrote:
> If the server publishes SCRAM-SHA-256-PLUS and the server does not
> support channel binding, then we get this error message:
> "channel binding is required, but server did not offer an
> authentication method that supports channel binding."
> So that's the part which is wrong.
>=20
> Now, I am not completely sure that the suggested change is completely
> right either as we would get an error in this scenario when
> channel_binding is "prefer" or "require".  For "require", this error
> message is fine.  However, for "prefer", shouldn't we do what we do on
> HEAD, aka *not* select SCRAM-SHA-256-PLUS and switch to SCRAM-SHA-256?
> This would have the advantage to make the connection work with default
> parameters.

So, something like the attached looks better to me.  Using a server
which publishes SCRAM-SHA-256-PLUS, I get the following over SSL:
1) client supports channel binding:
1-1) channel_binding =3D disable =3D> OK, with SCRAM-SHA-256=20
1-2) channel_binding =3D prefer =3D> OK, with SCRAM-SHA-256-PLUS
1-3) channel_binding =3D require =3D> OK, with SCRAM-SHA-256-PLUS
2) client does not support channel binding
2-1) channel_binding =3D disable =3D> OK, with SCRAM-SHA-256
2-2) channel_binding =3D prefer =3D> OK, with SCRAM-SHA-256
2-3) channel_binding =3D require =3D> failure with new error message,
instead of the confusing one.

The bug is with 2-3, and Tom's suggestion would have switched 2-2 to a
failure (2-2 works on HEAD).
--
Michael

--gWRamliw218YCZO9
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="channel-binding-fix-v1.patch"

diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 04118d54e2..d6e4036641 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -471,14 +471,28 @@ pg_SASL_init(PGconn *conn, int payloadlen)
 		{
 			if (conn->ssl_in_use)
 			{
-				/*
-				 * The server has offered SCRAM-SHA-256-PLUS, which is only
-				 * supported by the client if a hash of the peer certificate
-				 * can be created, and if channel_binding is not disabled.
-				 */
+				/* The server has offered SCRAM-SHA-256-PLUS */
+
 #ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
+				/*
+				 * The client supports channel binding, which is chosen if
+				 * channel_binding is not disabled.
+				 */
 				if (conn->channel_binding[0] != 'd')	/* disable */
 					selected_mechanism = SCRAM_SHA_256_PLUS_NAME;
+#else
+				/*
+				 * The client does not support channel binding.  If it is
+				 * required, complain immediately instead of the error below
+				 * which would be confusing as the server is publishing
+				 * SCRAM-SHA-256-PLUS.
+				 */
+				if (conn->channel_binding[0] == 'r')	/* require */
+				{
+					printfPQExpBuffer(&conn->errorMessage,
+									  libpq_gettext("channel binding is required, but client does not support it\n"));
+					goto error;
+				}
 #endif
 			}
 			else

--gWRamliw218YCZO9--

--oJAv8lSwuaQsYd0G
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl2RqeoACgkQnvQgOdby
QH25gA//ZpUvDbz8je0kUVSg0x3RpHikzP/KE0tTynOpPFybIMDCfsXnEHYoMDWF
noxSLKeLZW85xVF1dzNmYeEaPI4khPZnB85YK8sPleaLxp/uuDNbM8Z8ALL9TpYD
izLxI2psUEfmTnVvnZ1bqrIDYssR5xTV41s6dzrGpbgFbaAxxPhbU7jPen31ENhs
OF8N5q9wzpesiQzquqciTN9eZdhJEcLFoTtTMkA2yzOqcTGF6Jx3M4Lh7FV3atzA
TZzqAKywHQ+bAyRyYsNEoAtLkpKdQ3NEGoxJGLLFd6Jb6UVVjHCJFEQjftlW1Esd
5c3Y9vdXubOvJLETg8n60JENtB2NUZB0hr1/TsDXJEizBe29iBhxanbCsfNoOe4/
YsJmyBaQ6P9mTYXoIKgcijTYp9g7Q3jvb1i0fFNLpRb2SEzYFeU+7HD6aS4j92x4
mXvDIvnO1tdfDiYRXoAQ90QhCG82B2O9Mv7cAYSbat24/bsZYHbajdY1aKEaJBWq
jZt/X6jtmFfbla3jjlKGpM/ZxJvLS5D36+3Yzq+gro/ufndG+ObDe7WvN/vp9pjW
LLzkZ7P+jZ++kb5i1dhy0QW7hwjhFaKBm/r/DWIB+k0t6Z47DIEK57E/NRBI+X1O
FLwMLMMYllUAPHJE0kNXi5JLKmAHxUTNKN/AVZwSVWpcAsxv0Kw=
=eA7M
-----END PGP SIGNATURE-----

--oJAv8lSwuaQsYd0G--