Re: Setting min/max TLS protocol in clientside libpq

--VLAOICcq5m4DWEYr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jan 14, 2020 at 11:01:00PM +0100, Daniel Gustafsson wrote:
> Files renamed to match existing naming convention, the rest of the patch left
> unchanged.

+   if ((pg_strcasecmp("tlsv1", protocol) == 0) || pg_strcasecmp("tlsv1.0", protocol) == 0)
+       return TLS1_VERSION;
"TLSv1.0" is not a supported grammar in the backend.  So I would just
drop it in libpq.  It is also not documented.

+ * Portions Copyright (c) 2018-2020, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *       src/common/protocol_openssl.c
It is a nobrainer to just use those lines for copyright notices
instead:
 * Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California

+     <varlistentry id="libpq-connect-sslminprotocolversion"
xreflabel="sslminprotocolversion">
+      <term><literal>sslminprotocolversion</literal></term>
Got to wonder if we had better not use underscores for those new
parameter names as they are much longer than their cousins.
Underscores would make things more inconsistent.

+       if (ssl_min_ver == -1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("invalid minimum protocol version specified: %s\n"),
+                             conn->sslminprotocolversion);
+           return -1;
+       }
[...]
+       if (ssl_max_ver == -1)
+       {
+           printfPQExpBuffer(&conn->errorMessage,
+                             libpq_gettext("invalid or unsupported maximum protocol version specified: %s\n"),
+                             conn->sslmaxprotocolversion);
+           return -1;
+       }
Error messages for the min/max are inconsistent.  I would just use
"unsupported", because...

Following with your complain on the other thread about the GUC
handling for the min/max protocol parameter. Shouldn't you validate
the supported values in connectOptions2() like what's done for the
other parameters?  This way, you can make the difference between an
invalid value and an unsupported value with two different error
strings.  By validating the values at an earlier stage, you save a
couple of cycles for the application.

+        <literal>TLSv1.3</literal>.  The supported protocols depend on the
+        version of <productname>OpenSSL</productname> used, older versions
+        doesn't support the modern protocol versions.
Incorrect grammar => s/doesn't/don't/.

It would be good to mention that the default is no value, meaning that
the minimum and/or the maximum are not enforced in the SSL context.

+   if (conn->sslminprotocolversion)
+   {
[...]
+   if (conn->sslmaxprotocolversion)
+   {
You are missing two checks for empty strings here (aka strlen == 0).
These should be treated the same as no value to enforce the protocol
to.  (Let's not add an alias for "any".)

+ * Convert TLS protocol versionstring to OpenSSL values
Space needed here => "version string".

A nit, perhaps unnecessary, but I would use "TLSv1.1", etc. in the
values harcoded for libpq.  That's easier to grep after, and
consistent with the existing conventions even if you apply a
case-insensitive comparison.
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4eqfEACgkQnvQgOdby
QH2I6Q/8DHPp4jFteffK0l/1jh1FWtvBQKviZbK6e2fvDe4G08hfvpHCLoEZj6yS
7boFgV1XhHratHKtJw/wRzy/5CEjJMdlURHo4ufeYCPOlJQlfK1hqg4oGBaNGhSi
LDps4vx4IL/SoMaewiFai0VtOeHj9upSpFNXeN9L020g+e+FDDyUuNS7VpEqoZgA
x0P3XmFYVPPRyfyI+ylpvxCedIqvkhmOWLmCY4zlXSbel8QicxBIHvrc6DhAU239
t4cGZDnBv7l6LpCXoEahmc6Qz9dXNoQsIkFF6zebG8yI0/EpAp9Mk+HkYC4mLN5s
c0RPA0rCIgKIPBNqFLhfLsHlbx0FttfFsEcW5w71KuZysD1re2UHtkdDAlRjJfya
ywbO0VKU0Qw6iTWC9d0OQas4/M6LdtoYMvH3CSCis9f+yc/BKLDtvfMD/fgzdb5L
eOyv8twBFezoer4Q/KJdD5nXap7FS0xJyjuj/Q5UVUJpApwHr8XbEEA7/hY+FB6l
EoTDNGtKjkJRu/kc4WLGZsjc3S/6SHWmsSibzl8ZgGdrGjdTbw0gQmopGKArs6Vu
cxynG4smd0+j+6HVIMxdQEMcmRtC3kCHAJgvw/O1r9D2T8igFysj+1eBg5S7VTjX
RGXqXoVjMp4kUWLu3uL/5Vu97NnVUc2Jo9qZv80gnlqbrpo02ak=
=IJpJ
-----END PGP SIGNATURE-----

--VLAOICcq5m4DWEYr--