There have been some requests to be able to select the TLS versions
PostgreSQL is using. We currently only hardcode that SSLv2 and SSLv3
are disabled, but there is also some interest now in disabling TLSv1.0
and TLSv1.1. Also, I've had some issues in some combinations with the
new TLSv1.3, so there is perhaps also some use for disabling at the top end.

Attached is a patch that implements this. For example:

ssl_min_protocol_version = 'TLSv1'
ssl_max_protocol_version = 'any'

For reference, here is similar functionality implemented elsewhere:

https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslprotocol

Unlike those two, which offer a list of protocols to use, I have gone
with min and max settings. I think that is easier to use, and it also
maps better to the newer OpenSSL API (SSL_CTX_set_min_proto_version()
etc.). The older SSL_CTX_set_options()-based approach is deprecated and
has some very weird behaviors that would make it complicated to use for
anything more than a min/max.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 1 Oct 2018, at 22:21, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> There have been some requests to be able to select the TLS versions
> PostgreSQL is using. We currently only hardcode that SSLv2 and SSLv3
> are disabled, but there is also some interest now in disabling TLSv1.0
> and TLSv1.1. Also, I've had some issues in some combinations with the
> new TLSv1.3, so there is perhaps also some use for disabling at the top end.
>
> Attached is a patch that implements this. For example:
>
> ssl_min_protocol_version = 'TLSv1'
> ssl_max_protocol_version = ‘any'

I don’t think ‘any’ is a clear name for a setting which means “the highest
supported version”. How about ‘max_supported’ or something similar?

> For reference, here is similar functionality implemented elsewhere:
>
> https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
> https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslprotocol
>
> Unlike those two, which offer a list of protocols to use, I have gone
> with min and max settings.

FWIW, libcurl also supports a min/max approach with CURLOPT_SSLVERSION:

https://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html

+1 for using a min/max approach for setting the version, and it should be
trivial to add support for in the pending GnuTLS and Secure Transport patches.

cheers ./daniel

On 01/10/2018 23:30, Daniel Gustafsson wrote:
>> ssl_min_protocol_version = 'TLSv1'
>> ssl_max_protocol_version = ‘any'
>
> I don’t think ‘any’ is a clear name for a setting which means “the highest
> supported version”. How about ‘max_supported’ or something similar?

I can see the argument for an alternative, but your suggestion is a
mouthful.

> +1 for using a min/max approach for setting the version, and it should be
> trivial to add support for in the pending GnuTLS and Secure Transport patches.

AFAICT, in GnuTLS this is done via the "priorities" setting that also
sets the ciphers. There is no separate API for just the TLS version.
It would be interesting to see how Secure Transport can do it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 2 Oct 2018, at 14:23, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 01/10/2018 23:30, Daniel Gustafsson wrote:
>>> ssl_min_protocol_version = 'TLSv1'
>>> ssl_max_protocol_version = ‘any'
>>
>> I don’t think ‘any’ is a clear name for a setting which means “the highest
>> supported version”. How about ‘max_supported’ or something similar?
>
> I can see the argument for an alternative, but your suggestion is a
> mouthful.

Agreed, but I can’t think of a better wording. Perhaps just ‘tls_max’?

>> +1 for using a min/max approach for setting the version, and it should be
>> trivial to add support for in the pending GnuTLS and Secure Transport patches.
>
> AFAICT, in GnuTLS this is done via the "priorities" setting that also
> sets the ciphers. There is no separate API for just the TLS version.
> It would be interesting to see how Secure Transport can do it.

Secure Transport has a fairly neat API for this, SSLSetProtocolVersionMax() and
SSLSetProtocolVersionMin() (available since Lion).

cheers ./daniel

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

I've reviewed the patch and here are my comments.

The feature seems useful a lot of application servers are implementing minimal TLS protocol versions.
I don't see a way to restrict libpq to only connect with certain protocol versions. Maybe that is a separate patch but it would make this feature harder to test in the future.

I tested with a server configured to via the options to only TLS1.3 and clients without TLSv1.3 support and confirmed that I couldn't connect with SSL. This is fine
I tested with options to restrict the max version to TLSv1 and verified that the clients connected with TLSv1. This is fine
I tested with a min protocol version greater than the max. The server started up (Do we want this to be an warning on startup?) but I wasn't able to connect with SSL. The following was in the server log

could not accept SSL connection: unknown protocol

I tested with a max protocol version set to any. This is fine.
I tested putting TLSv1.3 in the config file when my openssl library did not support 1.3. This is fine.

I am updating the patch status to ready for committer.

The new status of this patch is: Ready for Committer

On Mon, Oct 1, 2018 at 4:21 PM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> There have been some requests to be able to select the TLS versions
> PostgreSQL is using. We currently only hardcode that SSLv2 and SSLv3
> are disabled, but there is also some interest now in disabling TLSv1.0
> and TLSv1.1. Also, I've had some issues in some combinations with the
> new TLSv1.3, so there is perhaps also some use for disabling at the top end.
>
> Attached is a patch that implements this. For example:
>
> ssl_min_protocol_version = 'TLSv1'
> ssl_max_protocol_version = 'any'

+1. Maybe it would make sense to spell 'any' as the empty string.
Intuitively, it makes more sense to me to think about there being no
maximum than to think about the maximum being anything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 05, 2018 at 03:01:58PM -0500, Robert Haas wrote:
> On Mon, Oct 1, 2018 at 4:21 PM Peter Eisentraut
> <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> > There have been some requests to be able to select the TLS versions
> > PostgreSQL is using. We currently only hardcode that SSLv2 and SSLv3
> > are disabled, but there is also some interest now in disabling TLSv1.0
> > and TLSv1.1. Also, I've had some issues in some combinations with the
> > new TLSv1.3, so there is perhaps also some use for disabling at the top end.
> >
> > Attached is a patch that implements this. For example:
> >
> > ssl_min_protocol_version = 'TLSv1'
> > ssl_max_protocol_version = 'any'
>
> +1. Maybe it would make sense to spell 'any' as the empty string.
> Intuitively, it makes more sense to me to think about there being no
> maximum than to think about the maximum being anything.

..and now, I'm finally beginning to see the reasoning that led Oracle
to conflate NULL and empty string.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Monday, November 5, 2018, David Fetter <david(at)fetter(dot)org> wrote:

> On Mon, Nov 05, 2018 at 03:01:58PM -0500, Robert Haas wrote:
> > On Mon, Oct 1, 2018 at 4:21 PM Peter Eisentraut
> > <peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> > >
> > > Attached is a patch that implements this. For example:
> > >
> > > ssl_min_protocol_version = 'TLSv1'
> > > ssl_max_protocol_version = 'any'
> >
> > +1. Maybe it would make sense to spell 'any' as the empty string.
> > Intuitively, it makes more sense to me to think about there being no
> > maximum than to think about the maximum being anything.
>
> ..and now, I'm finally beginning to see the reasoning that led Oracle
> to conflate NULL and empty string.
>

Seems like a situation for ‘n/a’ though maybe that’s too English-centric...

I’m a bit uncertain about the mix of name and number in something that
purports to be a version and thus should be numeric only. SSLv3 and TLSv2
would not be comparable in terms of min/max...but I haven’t delved deeply
into the feature either.

David J.

On Mon, Nov 05, 2018 at 03:01:58PM -0500, Robert Haas wrote:
> +1. Maybe it would make sense to spell 'any' as the empty string.
> Intuitively, it makes more sense to me to think about there being no
> maximum than to think about the maximum being anything.

I have looked at the code a bit yesterday and the implementation as well
as how things are handled with OpenSSL looked sane to me. The
suggestion of using an empty string as the default instead of 'any' also
makes sense per your argument
--
Michael

On 04/11/2018 04:24, Steve Singer wrote:
> The feature seems useful a lot of application servers are implementing minimal TLS protocol versions.
> I don't see a way to restrict libpq to only connect with certain protocol versions. Maybe that is a separate patch but it would make this feature harder to test in the future.

Client-side support could be separate patch, yes. It seems less important.

> I am updating the patch status to ready for committer.
>
> The new status of this patch is: Ready for Committer

Committed with the change 'any' -> '' that was discussed elsewhere in
the thread. Thanks.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services