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.
> 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.
Yeah, that makes sense. Even more now that I have just removed
support for OpenSSL 0.9.8 and 1.0.0 ;)
There could be an argument to lower down the default if we count for
backends built with OpenSSL versions older than libpq, but I am not
ready to buy that there would be many of those.
> 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? For example openssl-protocol.c in
src/common/? src/common/ stuff is built with -fPIC since 7143b3e so
there is no need to include directly the source files in the
Makefile. A shame you cannot do that for
ssl_protocol_version_to_openssl(), so for that a note would be welcome
on top of the former backend routine and the one you are adding.
The patch has conflicts with libpq-int.h as far as I can see. That
should be easy enough to solve.
The patch should have tests in src/test/ssl/, like for invalid values,
incorrect combinations leading to failures, etc.
--
Michael
