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.
Overallthe functionality works and the chosen TLS protocol aligns with the min/max TLS protocol settings on the PG
serverside.
I agree with Arthur that it makes sense to check the validity of "conn->sslmaxprotocolversion" first before checking if
itis 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
defaultthe server accepts TLSv1.2 and above. I think on the client side, it also makes sense to have the same defaults
asthe server. In the patch, if the client does not supply "sslminprotocolversion", it will run to "else" statement and
setsTLS min version to "INT_MIN", which is a huge negative number and of course openssl won't set it. I think this else
statementcan be enhanced a little to set "sslminprotocolversion" to TLSv1.2 by default to match the server and provide
somewarning message that TLS minimum has defaulted to TLSv1.2.
Cary
HighGo Software Canada
