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
signature.asc
Description: PGP signature
