Thanks for review everyone! A v2 of the patch which I believe addresses all concerns raised is attached.
> On 6 Jan 2020, at 07:01, Michael Paquier <mich...@paquier.xyz> wrote: > > On Thu, Jan 02, 2020 at 09:46:44PM +0000, cary huang wrote: >> I agree with Arthur that it makes sense to check the validity of >> "conn->sslmaxprotocolversion" first before checking if it is larger >> than "conn->sslminprotocolversion" > > Here I don't agree. Why not just let OpenSSL handle things with > SSL_CTX_set_min_proto_version? We don't bother about that in the > backend code for that reason on top of keeping the code more simple > with less error handling. And things are cleaner when it comes to > this libpq patch by giving up with the INT_MIN hack. 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. 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. >> In the patch, if the client does not supply "sslminprotocolversion", >> it will run to "else" statement and sets TLS min version to "INT_MIN", >> which is a huge negative number and of course openssl won't set >> it. I think this else statement can be enhanced a little to set >> "sslminprotocolversion" to TLSv1.2 by default to match the server >> and provide some warning message that TLS minimum has defaulted to >> TLSv1.2. > > In this patch fe-secure-openssl.c has just done a copy-paste of > SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version > present in be-secure-openssl.c. That's not good. Could you refactor > that please as a separate file? Done. I opted for a more generic header to make usage of the code easier, not sure if thats ok. 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. > The patch should have tests in src/test/ssl/, like for invalid values, > incorrect combinations leading to failures, etc. Also done. cheers ./daniel
libpq_minmaxproto_v2.patch
Description: Binary data
