Re: Setting min/max TLS protocol in clientside libpq

On Fri, Jan 10, 2020 at 12:01:36AM +0100, Daniel Gustafsson wrote:
> I looked into this and it turns out that OpenSSL does nothing to prevent the
> caller from setting a nonsensical protocol range like min=tlsv1.3,max=tlsv1.1.
> Thus, it's quite easy to screw up the backend server config and get it to start
> properly, but with quite unrelated error messages as a result on connection.

FWIW, here is the error produced, and that's confusing:
$ psql -d "host=localhost sslmode=require"
psql: error: could not connect to server: SSL error: tlsv1 alert
internal error

> Since I think this needs to be dealt with for both backend and frontend (if
> this is accepted), I removed it from this patch to return to it in a separate
> thread.

HEAD and back branches only care about the backend, so I think that we
should address this part first as your patch would I guess reuse the
interface we finish by using for the backend. Looking at OpenSSL, I
agree that there is no internal logic to perform sanity checks on the
min/max bounds. Still I can see that OpenSSL 1.1.0 has added some
"get" routines for SSL_CTX_set_min/max_proto_version:
https://www.openssl.org/docs/man1.1.0/man3/SSL_CTX_set_min_proto_version.html

Hmmmmeuh. It would be perfect to rely only on OpenSSL for that part
to bring some sanity, and compare the results fetched from the SSL
context so as we don't have to worry about special cases in with the
GUC reload if the parameter is not set, or the parameter value is not
supported. Now, OpenSSL <= 1.0.2 cannot do that, and you can get the
values set only after doing the set, so adding the compatibility
argument it is much more tempting to use our
ssl_protocol_version_to_openssl() wrapper and complain iff:
- both the min and max are supported values.
- min/max are incompatible.
And the check needs to be done before attempting to set the min/max
protos so as you don't finish with an incorrect intermediate state.
Daniel, are you planning to start a new thread?

> One thing I noticed when looking at it is that we now have sha2_openssl.c and
> openssl_protocol.c in src/common. For easier visual grouping of OpenSSL
> functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c,
> but that might just be pointless churn.

Databases like consistency, and so do I, so no issues from me to do a
rename of the sha2.c file. That makes sense with the addition of the
new file.
--
Michael