Re: Setting min/max TLS protocol in clientside libpq - Mailing list pgsql-hackers

On Fri, Jan 24, 2020 at 12:19:31PM +0100, Daniel Gustafsson wrote:
> Attached is a v5 of the patch which hopefully address all the comments raised,
> sorry for the delay.

Thanks for the new version.

psql: error: could not connect to server: invalid or unsupported
maximum protocol version specified: TLSv1.3
Running the regression tests with OpenSSL 1.0.1, 1.0.2 or 1.1.0 fails,
because TLSv1.3 (TLS1_3_VERSION) is not supported in those versions.
I think that it is better to just rely on TLSv1.2 for all that,
knowing that the server default for the minimum bound is v1.2.

+test_connect_fails(
+       $common_connstr,
+       "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1.2",
+       qr/invalid protocol version range/,
+       "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
+
+test_connect_fails(
+       $common_connstr,
+       "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1",
+       qr/invalid protocol version range/,
+       "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
This is testing twice pattern the same thing, and I am not sure if is
is worth bothering about the special case with TLSv1 (using just a
comparison with pg_strcasecmp you don't actually need those special
checks..).

Tests should make sure that a failure happens when an incorrect value
is set for sslminprotocolversion and sslmaxprotocolversion.

For readability, I think that it is better to consider NULL or empty
values for the parameters to be valid.  They are actually valid
values, because they just get ignored when creating the connection.

Adding an assertion within the routine for the protocol range check to
make sure that the values are valid makes the code more robust.

+       {"sslminprotocolversion", "PGSSLMINPROTOCOLVERSION", NULL,
NULL,
+               "SSL-Minimum-Protocol-Version", "",  /*
sizeof("TLSv1.x") */ 7,
+       offsetof(struct pg_conn, sslminprotocolversion)},
+
+       {"sslmaxprotocolversion", "PGSSLMAXPROTOCOLVERSION", NULL,
NULL,
+               "SSL-Maximum-Protocol-Version", "", /*
sizeof("TLSv1.x") */ 7,
Missing a zero-terminator in the count here.  And actually
gssencmode is wrong as well..  I'll report that on a different
thread.

+# Test min/mix protocol versions
Typo here.

+bool
+pq_verify_ssl_protocol_option(const char *protocolversion)
[...]
+bool
+pq_verify_ssl_protocol_range(const char *min, const char *max)
Both routines are just used in fe-connect.c to validate the connection
parameters, so it is better to keep them static and in fe-connect.c in
my opinion.

+       if (*(min + strlen("TLSv1.")) > *(max + strlen("TLSv1.")))
+               return false;
It is enough to use pg_strcasecmp() here.

Hm.  I am not sure that having a separate section "Client Protocol
Usage" brings much, so I have removed this one, and added an extra
sentence for the maximum protocol regarding its value for testing or
protocol compatibility.

The regression tests of postgres_fdw failed because of the new
parameters.  One update later, they run fine.

So, attached is an updated version of the patch that I have spent a
couple of hours polishing.  What do you think?
--
Michael