The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: tested, passed
Hello I have applied the patch and did some basic testing with various combination of min and max TLS protocol versions. Overall the functionality works and the chosen TLS protocol aligns with the min/max TLS protocol settings on the PG server side. 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" A small suggestion here. I see that PG server defaults TLS min version to be TLSv1.2 and max version to none. So by default the server accepts TLSv1.2 and above. I think on the client side, it also makes sense to have the same defaults as the server. 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. Cary HighGo Software Canada
