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 <michael@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
