Re: Setting min/max TLS protocol in clientside libpq

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: cary huang <hcary328(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Setting min/max TLS protocol in clientside libpq
Date: 2020-01-09 23:01:36
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)paquier(dot)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

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

Attachment Content-Type Size
libpq_minmaxproto_v2.patch application/octet-stream 17.6 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message cary huang 2020-01-09 23:17:47 Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Previous Message Andrew Dunstan 2020-01-09 22:16:11 Re: Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings