Re: Setting min/max TLS protocol in clientside libpq

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