Thread: should libpq also require TLSv1.2 by default?

should libpq also require TLSv1.2 by default?

From
Peter Eisentraut
Date:
In PG13, we raised the server-side default of ssl_min_protocol_version 
to TLSv1.2.  We also added a connection setting named 
ssl_min_protocol_version to libpq.  But AFAICT, the default value of the 
libpq setting is empty, so any protocol version will be accepted.  Is 
this what we wanted?  Should we raise the default in libpq as well?

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



Re: should libpq also require TLSv1.2 by default?

From
Daniel Gustafsson
Date:
> On 24 Jun 2020, at 08:39, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> In PG13, we raised the server-side default of ssl_min_protocol_version to TLSv1.2.  We also added a connection
settingnamed ssl_min_protocol_version to libpq.  But AFAICT, the default value of the libpq setting is empty, so any
protocolversion will be accepted.  Is this what we wanted?  Should we raise the default in libpq as well? 

This was discussed [0] when the connection settings were introduced, and the
concensus was to leave them alone [1] to allow for example a new pg_dump to
work against an old server.  Re-reading the thread I think the argument still
holds, but I was about to respond "yes, let's do this" before refreshing my
memory.  Perhaps we should add a comment explaining this along the lines of the
attached?

cheers ./daniel

[0] https://www.postgresql.org/message-id/157800160408.1198.1714906047977693148.pgcf%40coridan.postgresql.org
[1] https://www.postgresql.org/message-id/31993.1578321474%40sss.pgh.pa.us


Attachment

Re: should libpq also require TLSv1.2 by default?

From
Magnus Hagander
Date:


On Wed, Jun 24, 2020 at 10:33 AM Daniel Gustafsson <daniel@yesql.se> wrote:
> On 24 Jun 2020, at 08:39, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> In PG13, we raised the server-side default of ssl_min_protocol_version to TLSv1.2.  We also added a connection setting named ssl_min_protocol_version to libpq.  But AFAICT, the default value of the libpq setting is empty, so any protocol version will be accepted.  Is this what we wanted?  Should we raise the default in libpq as well?

This was discussed [0] when the connection settings were introduced, and the
concensus was to leave them alone [1] to allow for example a new pg_dump to
work against an old server.  Re-reading the thread I think the argument still
holds, but I was about to respond "yes, let's do this" before refreshing my
memory.  Perhaps we should add a comment explaining this along the lines of the
attached?


Another argument for not changing the default is that if you want to use SSL in any meaningful way you have to *already* change the connection string (with sslmode=require or verify-*), so it's not unreasonable to make that consideration at the same time.

It might also be worth noting that it's not really "any protocol version", it means it will be "whatever the openssl configuration says", I think? For example, debian buster sets:

[system_default_sect]
MinProtocol = TLSv1.2

Which I believe means that if your libpq app is running on debian buster, it will be min v1.2 already (and it would likely be more useful to use ssl_min_protocol_version to *lower* that when connecting to older servers).

//Magnus

Re: should libpq also require TLSv1.2 by default?

From
Daniel Gustafsson
Date:
> On 24 Jun 2020, at 10:46, Magnus Hagander <magnus@hagander.net> wrote:

> It might also be worth noting that it's not really "any protocol version", it means it will be "whatever the openssl
configurationsays", I think? For example, debian buster sets: 
>
> [system_default_sect]
> MinProtocol = TLSv1.2
>
> Which I believe means that if your libpq app is running on debian buster, it will be min v1.2 already

Correct, that being said I'm not sure how common it is for distributions to set
a default protocol version.  The macOS versions I have handy doesn't enforce a
default version, nor does Ubuntu 20.04, FreeBSD 12 or OpenBSD 6.5 AFAICT.

> (and it would likely be more useful to use ssl_min_protocol_version to *lower* that when connecting to older
servers).

That is indeed one use-case for the connection parameter.

cheers ./daniel


Re: should libpq also require TLSv1.2 by default?

From
Peter Eisentraut
Date:
On 2020-06-24 10:33, Daniel Gustafsson wrote:
>> In PG13, we raised the server-side default of ssl_min_protocol_version to TLSv1.2.  We also added a connection
settingnamed ssl_min_protocol_version to libpq.  But AFAICT, the default value of the libpq setting is empty, so any
protocolversion will be accepted.  Is this what we wanted?  Should we raise the default in libpq as well?
 
> 
> This was discussed [0] when the connection settings were introduced, and the
> concensus was to leave them alone [1] to allow for example a new pg_dump to
> work against an old server.  Re-reading the thread I think the argument still
> holds, but I was about to respond "yes, let's do this" before refreshing my
> memory.  Perhaps we should add a comment explaining this along the lines of the
> attached?
> 
> [0] https://www.postgresql.org/message-id/157800160408.1198.1714906047977693148.pgcf%40coridan.postgresql.org
> [1] https://www.postgresql.org/message-id/31993.1578321474%40sss.pgh.pa.us

ISTM that these discussions went through the same questions and 
arguments that were made regarding the server-side change but arrived at 
a different conclusion.  So I suggest to reconsider this so that we 
don't ship with contradictory results.

That doesn't necessarily mean that we have to make a change, but we 
should make sure our rationale is sound.

Note that all OpenSSL versions that do not support TLSv1.2 also do not 
support TLSv1.1.  So by saying, in effect, that TLSv1.2 is too new to 
require, we are saying that we need to keep supporting TLSv1.0 -- which 
is heavily deprecated.  Also note that the first OpenSSL version with 
support for TLSv1.2 shipped on March 14, 2012.

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



Re: should libpq also require TLSv1.2 by default?

From
Bruce Momjian
Date:
On Wed, Jun 24, 2020 at 07:57:31PM +0200, Peter Eisentraut wrote:
> On 2020-06-24 10:33, Daniel Gustafsson wrote:
> > > In PG13, we raised the server-side default of ssl_min_protocol_version to TLSv1.2.  We also added a connection
settingnamed ssl_min_protocol_version to libpq.  But AFAICT, the default value of the libpq setting is empty, so any
protocolversion will be accepted.  Is this what we wanted?  Should we raise the default in libpq as well?
 
> > 
> > This was discussed [0] when the connection settings were introduced, and the
> > concensus was to leave them alone [1] to allow for example a new pg_dump to
> > work against an old server.  Re-reading the thread I think the argument still
> > holds, but I was about to respond "yes, let's do this" before refreshing my
> > memory.  Perhaps we should add a comment explaining this along the lines of the
> > attached?
> > 
> > [0] https://www.postgresql.org/message-id/157800160408.1198.1714906047977693148.pgcf%40coridan.postgresql.org
> > [1] https://www.postgresql.org/message-id/31993.1578321474%40sss.pgh.pa.us
> 
> ISTM that these discussions went through the same questions and arguments
> that were made regarding the server-side change but arrived at a different
> conclusion.  So I suggest to reconsider this so that we don't ship with
> contradictory results.
> 
> That doesn't necessarily mean that we have to make a change, but we should
> make sure our rationale is sound.
> 
> Note that all OpenSSL versions that do not support TLSv1.2 also do not
> support TLSv1.1.  So by saying, in effect, that TLSv1.2 is too new to
> require, we are saying that we need to keep supporting TLSv1.0 -- which is
> heavily deprecated.  Also note that the first OpenSSL version with support
> for TLSv1.2 shipped on March 14, 2012.

I do think mismatched SSL requirements between client and server is
confusing, though I can see the back-version pg_dump being an issue. 
Maybe a clear error message would help here.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee




Re: should libpq also require TLSv1.2 by default?

From
Daniel Gustafsson
Date:
> On 24 Jun 2020, at 19:57, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-06-24 10:33, Daniel Gustafsson wrote:
>>> In PG13, we raised the server-side default of ssl_min_protocol_version to TLSv1.2.  We also added a connection
settingnamed ssl_min_protocol_version to libpq.  But AFAICT, the default value of the libpq setting is empty, so any
protocolversion will be accepted.  Is this what we wanted?  Should we raise the default in libpq as well? 
>> This was discussed [0] when the connection settings were introduced, and the
>> concensus was to leave them alone [1] to allow for example a new pg_dump to
>> work against an old server.  Re-reading the thread I think the argument still
>> holds, but I was about to respond "yes, let's do this" before refreshing my
>> memory.  Perhaps we should add a comment explaining this along the lines of the
>> attached?
>> [0] https://www.postgresql.org/message-id/157800160408.1198.1714906047977693148.pgcf%40coridan.postgresql.org
>> [1] https://www.postgresql.org/message-id/31993.1578321474%40sss.pgh.pa.us
>
> ISTM that these discussions went through the same questions and arguments that were made regarding the server-side
changebut arrived at a different conclusion.  So I suggest to reconsider this so that we don't ship with contradictory
results.

I don't think anyone argues against safe defaults for communication between
upgraded clients and upgraded servers.  That being said; out of the box, an
upgraded client *will* use TLSv1.2 when connecting to a upgraded server due to
the server defaults requirements (assuming the server hasn't been reconfigured
with a lower TLS version, but since we're talking defaults we have to assume
that).

The problem comes when an updated client needs to talk to an old server which
hasn't been upgraded and which use an older OpenSSL version.  If we set TLSv1.2
as the minimum client version, the user will have to amend a connection string
which used to work when talking to a server which hasn't changed.  If we don't
raise the default, a user to wants nothing lower than TLSv1.2 will have to
amend the connection string.

> That doesn't necessarily mean that we have to make a change, but we should make sure our rationale is sound.

Totally agree.  I think I, FWIW, still vote for keeping it at 1.0 to not break
connections to old servers, since upgraded/new servers will enforce 1.2
anyways.

As mentioned elsewhere in the thread, maybe this is also something which can be
done more easily if we improve the error reporting?  Right now it's fairly
cryptic IMO.

> Note that all OpenSSL versions that do not support TLSv1.2 also do not support TLSv1.1.  So by saying, in effect,
thatTLSv1.2 is too new to require, we are saying that we need to keep supporting TLSv1.0 -- which is heavily
deprecated. Also note that the first OpenSSL version with support for TLSv1.2 shipped on March 14, 2012. 

Correct, this being the 1.0.1 release which is referred to.

cheers ./daniel


Re: should libpq also require TLSv1.2 by default?

From
Michael Paquier
Date:
On Thu, Jun 25, 2020 at 12:30:03AM +0200, Daniel Gustafsson wrote:
> I don't think anyone argues against safe defaults for communication between
> upgraded clients and upgraded servers.  That being said; out of the box, an
> upgraded client *will* use TLSv1.2 when connecting to a upgraded server due to
> the server defaults requirements (assuming the server hasn't been reconfigured
> with a lower TLS version, but since we're talking defaults we have to assume
> that).

My take here is to let things as they are for libpq.  pg_dump is a very
good argument, because we allow backward compatibility with a newer
version of the tool, not upward compatibility.

> The problem comes when an updated client needs to talk to an old server which
> hasn't been upgraded and which use an older OpenSSL version.  If we set TLSv1.2
> as the minimum client version, the user will have to amend a connection string
> which used to work when talking to a server which hasn't changed.  If we don't
> raise the default, a user to wants nothing lower than TLSv1.2 will have to
> amend the connection string.

Yeah, and I would not be surprised to see cases where people complain
to us about that when attempting to reach one of their old boxes,
breaking some stuff they have been relying on for years by forcing the
addition of a tls_min_server_protocol in the connection string.  It is
a more important step that we enforce TLSv1.2 on the server side
actually, and libpq just follows up automatically with that.

> As mentioned elsewhere in the thread, maybe this is also something which can be
> done more easily if we improve the error reporting?  Right now it's fairly
> cryptic IMO.

This part may be tricky to get right I think, because the error comes
directly from OpenSSL when negotiating the protocol used between the
client and the server, like "no protocols available" or such.
--
Michael

Attachment

Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jun 25, 2020 at 12:30:03AM +0200, Daniel Gustafsson wrote:
>> As mentioned elsewhere in the thread, maybe this is also something which can be
>> done more easily if we improve the error reporting?  Right now it's fairly
>> cryptic IMO.

> This part may be tricky to get right I think, because the error comes
> directly from OpenSSL when negotiating the protocol used between the
> client and the server, like "no protocols available" or such.

Can we do something comparable to the backend's HINT protocol, where
we add on a comment that's only mostly-likely to be right?

            regards, tom lane



Re: should libpq also require TLSv1.2 by default?

From
Michael Paquier
Date:
On Wed, Jun 24, 2020 at 10:50:39PM -0400, Tom Lane wrote:
> Can we do something comparable to the backend's HINT protocol, where
> we add on a comment that's only mostly-likely to be right?

OpenSSL publishes its error codes as of openssl/sslerr.h, and it looks
like the two error codes we would need to worry about are
SSL_R_UNSUPPORTED_PROTOCOL and SSL_R_NO_PROTOCOLS_AVAILABLE.  So we
could for example amend open_client_SSL() when negotiating the SSL
connection in libpq with error messages or hints that help better than
the current state of things, but that also means an extra maintenance
on our side to make sure that we keep in sync with new error codes
coming from the OpenSSL world.
--
Michael

Attachment

Re: should libpq also require TLSv1.2 by default?

From
Robert Haas
Date:
On Wed, Jun 24, 2020 at 9:50 PM Michael Paquier <michael@paquier.xyz> wrote:
> Yeah, and I would not be surprised to see cases where people complain
> to us about that when attempting to reach one of their old boxes,
> breaking some stuff they have been relying on for years by forcing the
> addition of a tls_min_server_protocol in the connection string.  It is
> a more important step that we enforce TLSv1.2 on the server side
> actually, and libpq just follows up automatically with that.

I wonder how much of a problem this really is. A few quick Google
searches suggests that support for TLSv1.2 was added to OpenSSL in
v1.0.1, which was released in March 2012. If packagers adopted that
version for the following PostgreSQL release, they would have had
TLSv1.2 support from PostgreSQL 9.2 onward. Some people may have taken
longer to adopt it, but even if they waited a year or two, all
versions that they built with older OpenSSL versions would now be out
of support. It doesn't seem that likely that there are going to be
that many people using pg_dump to upgrade directly from PostgreSQL 9.2
-- or even 9.4 -- to PostgreSQL 13. Skipping six or eight major
versions in a single upgrade is a little unusual, in my experience.
And even if someone does want to do that, we haven't broken it; it'll
still work fine if they are connecting through a UNIX socket, and if
not, they can disable SSL or just specify that they're OK with an
older protocol version. That doesn't seem like a big deal, especially
if we can adopt Tom's suggestion of giving them a warning about what
went wrong.

Let's also consider the other side of this argument, which is that a
decent number of PostgreSQL users are operating in environments where
they are required for regulatory compliance to prohibit the use of
TLSv1.0 and TLSv1.1. Those people will be happy if that is the default
on both the client and the server side by default. They will probably
be somewhat happy anyway, because now we have an option for it, which
we didn't before. But they'll be more happy if it's the default. Now,
we can't please everybody here. Is it more important to please people
who would like insecure TLS versions disabled by default, or to please
people who want to use insecure TLS versions to back up old servers?
Seems debatable. Based on my own experience, I'd guess there are more
users who want to avoid insecure TLS versions than there are users who
want to use them to back up very old servers, so I'd tentatively favor
changing the default. However, I don't know whether my experiences are
representative.

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



Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> I wonder how much of a problem this really is.

Yeah.  I find Robert's points about that pretty persuasive: by now
needing to connect to a server without TLSv1.2 support, *and* needing to
do so with SSL on, ought to be a tiny niche use case (much smaller than
the number of people who would like a more secure default).  If we can
make the error message about this be reasonably clear then I don't have
an objection to changing libpq's default.

            regards, tom lane



Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 24 Jun 2020, at 10:46, Magnus Hagander <magnus@hagander.net> wrote:
>> It might also be worth noting that it's not really "any protocol version", it means it will be "whatever the openssl
configurationsays", I think? For example, debian buster sets: 
>>
>> [system_default_sect]
>> MinProtocol = TLSv1.2
>>
>> Which I believe means that if your libpq app is running on debian buster, it will be min v1.2 already

> Correct, that being said I'm not sure how common it is for distributions to set
> a default protocol version.  The macOS versions I have handy doesn't enforce a
> default version, nor does Ubuntu 20.04, FreeBSD 12 or OpenBSD 6.5 AFAICT.

Yeah, this.  I experimented with connecting current libpq to a 9.2-vintage
server I'd built with openssl 0.9.8x, and was surprised to find I couldn't
do so unless I explicitly set "ssl_min_protocol_version=tlsv1".  After
some digging I verified that that's because RHEL8's openssl.cnf sets

MinProtocol = TLSv1.2
MaxProtocol = TLSv1.3

Interestingly, Fedora 32 is laxer:

MinProtocol = TLSv1
MaxProtocol = TLSv1.3

I concur with Daniel's finding that current macOS and FreeBSD don't
enforce anything in this area.  Nonetheless, for a pretty significant
fraction of the world, our claim that the default is to not enforce
any minimum protocol version is a lie.

My feeling now is that we'd be better off defaulting
ssl_min_protocol_version to something nonempty, just to make this
behavior platform-independent.  We certainly can't leave the docs
as they are.

Also, I confirm that the failure looks like

$ psql -h ... -d "dbname=postgres sslmode=require"
psql: error: could not connect to server: SSL error: unsupported protocol

While that's not *that* awful, if you realize that "protocol" means
TLS version, many people probably won't without a hint.  It does not
help any that the message doesn't mention either the offered TLS version
or the version limits being enforced.  I'm not sure we can do anything
about the former, but reducing the number of variables affecting the
latter seems like a smart idea.

BTW, the server-side report of the problem looks like

LOG:  could not accept SSL connection: wrong version number

            regards, tom lane



Re: should libpq also require TLSv1.2 by default?

From
Bruce Momjian
Date:
On Thu, Jun 25, 2020 at 06:44:05PM -0400, Tom Lane wrote:
> BTW, the server-side report of the problem looks like
> 
> LOG:  could not accept SSL connection: wrong version number

I like that one.  ;-)

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee




Re: should libpq also require TLSv1.2 by default?

From
Daniel Gustafsson
Date:
> On 26 Jun 2020, at 00:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> My feeling now is that we'd be better off defaulting
> ssl_min_protocol_version to something nonempty, just to make this
> behavior platform-independent.  We certainly can't leave the docs
> as they are.

Yeah, given the concensus in this thread and your findings I think we should
default to TLSv1.2 as originally proposed.

I still think there will be instances of existing connections to old servers
that will all of a sudden break, but it's probably true that it's not a common
setup.  Optimizing for the majority and helping the minority with documentation
is IMO the winning move.

> Also, I confirm that the failure looks like
>
> $ psql -h ... -d "dbname=postgres sslmode=require"
> psql: error: could not connect to server: SSL error: unsupported protocol
>
> While that's not *that* awful, if you realize that "protocol" means
> TLS version, many people probably won't without a hint.  It does not
> help any that the message doesn't mention either the offered TLS version
> or the version limits being enforced.  I'm not sure we can do anything
> about the former, but reducing the number of variables affecting the
> latter seems like a smart idea.

+1

> BTW, the server-side report of the problem looks like
>
> LOG:  could not accept SSL connection: wrong version number

I can totally see some thinking that it's the psql version at client side which
is referred to and not the TLS protocol version.  Perhaps we should add a hint
there as well?

cheers ./daniel


Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 26 Jun 2020, at 00:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, the server-side report of the problem looks like
>> LOG:  could not accept SSL connection: wrong version number

> I can totally see some thinking that it's the psql version at client side which
> is referred to and not the TLS protocol version.  Perhaps we should add a hint
> there as well?

Not sure.  We can't fix it in the case we're mainly concerned about,
namely an out-of-support server version.  At the same time, it's certainly
true that "version number" is way too under-specified in this context.
Maybe improving this against the day that TLSv2 exists would be smart.

            regards, tom lane



Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Here is a quick attempt at getting libpq and the server to report
suitable hint messages about SSL version problems.

The main thing I don't like about this as formulated is that I hard-wired
knowledge of the minimum and maximum SSL versions into the hint messages.
That's clearly not very maintainable, but it seems really hard to keep the
messages readable/useful without giving concrete version numbers.
Anybdy have a better idea?  Is there a reasonably direct way to ask
OpenSSL what its min and max versions are?

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8adf64c78e..380268636a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -72,6 +72,7 @@ static bool dummy_ssl_passwd_cb_called = false;
 static bool ssl_is_server_start;

 static int    ssl_protocol_version_to_openssl(int v);
+static const char *ssl_protocol_version_to_string(int v);

 /* ------------------------------------------------------------ */
 /*                         Public interface                        */
@@ -365,6 +366,7 @@ be_tls_open_server(Port *port)
     int            err;
     int            waitfor;
     unsigned long ecode;
+    bool        give_proto_hint;

     Assert(!port->ssl);
     Assert(!port->peer);
@@ -451,10 +453,44 @@ aloop:
                              errmsg("could not accept SSL connection: EOF detected")));
                 break;
             case SSL_ERROR_SSL:
+                switch (ERR_GET_REASON(ecode))
+                {
+                        /*
+                         * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                         * server min/max protocol version limits don't
+                         * overlap.  UNSUPPORTED_PROTOCOL and
+                         * WRONG_VERSION_NUMBER have been observed when trying
+                         * to communicate with an old OpenSSL library.  It's
+                         * not very clear what would make OpenSSL return the
+                         * other codes listed here, but a hint about protocol
+                         * versions seems like it's appropriate for all.
+                         */
+                    case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                    case SSL_R_UNSUPPORTED_PROTOCOL:
+                    case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                    case SSL_R_UNKNOWN_SSL_VERSION:
+                    case SSL_R_UNSUPPORTED_SSL_VERSION:
+                    case SSL_R_VERSION_TOO_HIGH:
+                    case SSL_R_VERSION_TOO_LOW:
+                    case SSL_R_WRONG_SSL_VERSION:
+                    case SSL_R_WRONG_VERSION_NUMBER:
+                        give_proto_hint = true;
+                        break;
+                    default:
+                        give_proto_hint = false;
+                        break;
+                }
                 ereport(COMMERROR,
                         (errcode(ERRCODE_PROTOCOL_VIOLATION),
                          errmsg("could not accept SSL connection: %s",
-                                SSLerrmessage(ecode))));
+                                SSLerrmessage(ecode)),
+                         give_proto_hint ? errhint("This may indicate that the client does not support any SSL
protocolversion between %s and %s.", 
+                                                   ssl_min_protocol_version ?
+                                                   ssl_protocol_version_to_string(ssl_min_protocol_version) :
+                                                   "TLSv1",
+                                                   ssl_max_protocol_version ?
+                                                   ssl_protocol_version_to_string(ssl_max_protocol_version) :
+                                                   "TLSv1.3") : 0));
                 break;
             case SSL_ERROR_ZERO_RETURN:
                 ereport(COMMERROR,
@@ -1328,6 +1364,29 @@ ssl_protocol_version_to_openssl(int v)
     return -1;
 }

+/*
+ * Likewise provide a mapping to strings.
+ */
+static const char *
+ssl_protocol_version_to_string(int v)
+{
+    switch (v)
+    {
+        case PG_TLS_ANY:
+            return "any";
+        case PG_TLS1_VERSION:
+            return "TLSv1";
+        case PG_TLS1_1_VERSION:
+            return "TLSv1.1";
+        case PG_TLS1_2_VERSION:
+            return "TLSv1.2";
+        case PG_TLS1_3_VERSION:
+            return "TLSv1.3";
+    }
+
+    return "(unrecognized)";
+}
+

 static void
 default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 2d813ef5f9..71407bffd4 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1304,6 +1304,40 @@ open_client_SSL(PGconn *conn)
                                       libpq_gettext("SSL error: %s\n"),
                                       err);
                     SSLerrfree(err);
+                    switch (ERR_GET_REASON(ecode))
+                    {
+                            /*
+                             * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                             * server min/max protocol version limits don't
+                             * overlap.  UNSUPPORTED_PROTOCOL and
+                             * WRONG_VERSION_NUMBER have been observed when
+                             * trying to communicate with an old OpenSSL
+                             * library.  It's not very clear what would make
+                             * OpenSSL return the other codes listed here, but
+                             * a hint about protocol versions seems like it's
+                             * appropriate for all.
+                             */
+                        case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                        case SSL_R_UNSUPPORTED_PROTOCOL:
+                        case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                        case SSL_R_UNKNOWN_SSL_VERSION:
+                        case SSL_R_UNSUPPORTED_SSL_VERSION:
+                        case SSL_R_VERSION_TOO_HIGH:
+                        case SSL_R_VERSION_TOO_LOW:
+                        case SSL_R_WRONG_SSL_VERSION:
+                        case SSL_R_WRONG_VERSION_NUMBER:
+                            appendPQExpBuffer(&conn->errorMessage,
+                                              libpq_gettext("This may indicate that the server does not support any
SSLprotocol version between %s and %s.\n"), 
+                                              conn->ssl_min_protocol_version ?
+                                              conn->ssl_min_protocol_version :
+                                              "TLSv1",
+                                              conn->ssl_max_protocol_version ?
+                                              conn->ssl_max_protocol_version :
+                                              "TLSv1.3");
+                            break;
+                        default:
+                            break;
+                    }
                     pgtls_close(conn);
                     return PGRES_POLLING_FAILED;
                 }

Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
I wrote:
> Anybdy have a better idea?  Is there a reasonably direct way to ask
> OpenSSL what its min and max versions are?

After some digging, there apparently is not.  At first glance it would
seem that SSL_get_min_proto_version/SSL_get_max_proto_version should
help, but in reality they're just blindingly useless, because they
return zero in most cases of interest.  And when they don't return zero
they might give us a code that we don't recognize, so there's no future
proofing to be had from using them.  Plus they don't exist before
openssl 1.1.1.

It looks like, when they exist, we could use them to discover any
restrictions openssl.cnf has set on the allowed protocol versions ...
but I'm not really convinced that's worth the trouble.  If we up the
libpq default to TLSv1.2 then there probably won't be any real-world
cases where openssl.cnf affects our results.

So I propose the attached.  The hack in openssl.h to guess the
min/max supported versions is certainly nothing but a hack;
but I see no way to do better.

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8adf64c78e..778b166753 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -72,6 +72,7 @@ static bool dummy_ssl_passwd_cb_called = false;
 static bool ssl_is_server_start;

 static int    ssl_protocol_version_to_openssl(int v);
+static const char *ssl_protocol_version_to_string(int v);

 /* ------------------------------------------------------------ */
 /*                         Public interface                        */
@@ -365,6 +366,7 @@ be_tls_open_server(Port *port)
     int            err;
     int            waitfor;
     unsigned long ecode;
+    bool        give_proto_hint;

     Assert(!port->ssl);
     Assert(!port->peer);
@@ -451,10 +453,48 @@ aloop:
                              errmsg("could not accept SSL connection: EOF detected")));
                 break;
             case SSL_ERROR_SSL:
+                switch (ERR_GET_REASON(ecode))
+                {
+                        /*
+                         * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                         * server min/max protocol version limits don't
+                         * overlap.  UNSUPPORTED_PROTOCOL,
+                         * WRONG_VERSION_NUMBER, and
+                         * TLSV1_ALERT_PROTOCOL_VERSION have been observed
+                         * when trying to communicate with an old OpenSSL
+                         * library.  It's not very clear what would make
+                         * OpenSSL return the other codes listed here, but a
+                         * hint about protocol versions seems like it's
+                         * appropriate for all.
+                         */
+                    case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                    case SSL_R_UNSUPPORTED_PROTOCOL:
+                    case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                    case SSL_R_UNKNOWN_SSL_VERSION:
+                    case SSL_R_UNSUPPORTED_SSL_VERSION:
+                    case SSL_R_VERSION_TOO_HIGH:
+                    case SSL_R_VERSION_TOO_LOW:
+                    case SSL_R_WRONG_SSL_VERSION:
+                    case SSL_R_WRONG_VERSION_NUMBER:
+                    case SSL_R_TLSV1_ALERT_PROTOCOL_VERSION:
+                        give_proto_hint = true;
+                        break;
+                    default:
+                        give_proto_hint = false;
+                        break;
+                }
                 ereport(COMMERROR,
                         (errcode(ERRCODE_PROTOCOL_VIOLATION),
                          errmsg("could not accept SSL connection: %s",
-                                SSLerrmessage(ecode))));
+                                SSLerrmessage(ecode)),
+                         give_proto_hint ?
+                         errhint("This may indicate that the client does not support any SSL protocol version between
%sand %s.", 
+                                 ssl_min_protocol_version ?
+                                 ssl_protocol_version_to_string(ssl_min_protocol_version) :
+                                 MIN_OPENSSL_TLS_VERSION,
+                                 ssl_max_protocol_version ?
+                                 ssl_protocol_version_to_string(ssl_max_protocol_version) :
+                                 MAX_OPENSSL_TLS_VERSION) : 0));
                 break;
             case SSL_ERROR_ZERO_RETURN:
                 ereport(COMMERROR,
@@ -1328,6 +1368,29 @@ ssl_protocol_version_to_openssl(int v)
     return -1;
 }

+/*
+ * Likewise provide a mapping to strings.
+ */
+static const char *
+ssl_protocol_version_to_string(int v)
+{
+    switch (v)
+    {
+        case PG_TLS_ANY:
+            return "any";
+        case PG_TLS1_VERSION:
+            return "TLSv1";
+        case PG_TLS1_1_VERSION:
+            return "TLSv1.1";
+        case PG_TLS1_2_VERSION:
+            return "TLSv1.2";
+        case PG_TLS1_3_VERSION:
+            return "TLSv1.3";
+    }
+
+    return "(unrecognized)";
+}
+

 static void
 default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
diff --git a/src/include/common/openssl.h b/src/include/common/openssl.h
index 47fa129994..e4ef53ef70 100644
--- a/src/include/common/openssl.h
+++ b/src/include/common/openssl.h
@@ -17,12 +17,30 @@
 #ifdef USE_OPENSSL
 #include <openssl/ssl.h>

+/*
+ * OpenSSL doesn't provide any very nice way to identify the min/max
+ * protocol versions the library supports, so we fake it as best we can.
+ * Note in particular that this doesn't account for restrictions that
+ * might be specified in the installation's openssl.cnf.
+ */
+#define MIN_OPENSSL_TLS_VERSION  "TLSv1"
+
+#if defined(TLS1_3_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.3"
+#elif defined(TLS1_2_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.2"
+#elif defined(TLS1_1_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.1"
+#else
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1"
+#endif
+
 /* src/common/protocol_openssl.c */
 #ifndef SSL_CTX_set_min_proto_version
 extern int    SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
 extern int    SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
 #endif

-#endif
+#endif                            /* USE_OPENSSL */

 #endif                            /* COMMON_OPENSSL_H */
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 2d813ef5f9..10fa09020d 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1304,6 +1304,42 @@ open_client_SSL(PGconn *conn)
                                       libpq_gettext("SSL error: %s\n"),
                                       err);
                     SSLerrfree(err);
+                    switch (ERR_GET_REASON(ecode))
+                    {
+                            /*
+                             * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                             * server min/max protocol version limits don't
+                             * overlap.  UNSUPPORTED_PROTOCOL,
+                             * WRONG_VERSION_NUMBER, and
+                             * TLSV1_ALERT_PROTOCOL_VERSION have been observed
+                             * when trying to communicate with an old OpenSSL
+                             * library.  It's not very clear what would make
+                             * OpenSSL return the other codes listed here, but
+                             * a hint about protocol versions seems like it's
+                             * appropriate for all.
+                             */
+                        case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                        case SSL_R_UNSUPPORTED_PROTOCOL:
+                        case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                        case SSL_R_UNKNOWN_SSL_VERSION:
+                        case SSL_R_UNSUPPORTED_SSL_VERSION:
+                        case SSL_R_VERSION_TOO_HIGH:
+                        case SSL_R_VERSION_TOO_LOW:
+                        case SSL_R_WRONG_SSL_VERSION:
+                        case SSL_R_WRONG_VERSION_NUMBER:
+                        case SSL_R_TLSV1_ALERT_PROTOCOL_VERSION:
+                            appendPQExpBuffer(&conn->errorMessage,
+                                              libpq_gettext("This may indicate that the server does not support any
SSLprotocol version between %s and %s.\n"), 
+                                              conn->ssl_min_protocol_version ?
+                                              conn->ssl_min_protocol_version :
+                                              MIN_OPENSSL_TLS_VERSION,
+                                              conn->ssl_max_protocol_version ?
+                                              conn->ssl_max_protocol_version :
+                                              MAX_OPENSSL_TLS_VERSION);
+                            break;
+                        default:
+                            break;
+                    }
                     pgtls_close(conn);
                     return PGRES_POLLING_FAILED;
                 }
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index dfc292872a..ea1909c08d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1745,9 +1745,9 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
         <literal>TLSv1.1</literal>, <literal>TLSv1.2</literal> and
         <literal>TLSv1.3</literal>. The supported protocols depend on the
         version of <productname>OpenSSL</productname> used, older versions
-        not supporting the most modern protocol versions. If not set, this
-        parameter is ignored and the connection will use the minimum bound
-        defined by the backend.
+        not supporting the most modern protocol versions. If not specified,
+        the default is <literal>TLSv1.2</literal>, which satisfies industry
+        best practices as of this writing.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 2c87b34028..27c9bb46ee 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -320,7 +320,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
         "Require-Peer", "", 10,
     offsetof(struct pg_conn, requirepeer)},

-    {"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", NULL, NULL,
+    {"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
         "SSL-Minimum-Protocol-Version", "", 8,    /* sizeof("TLSv1.x") == 8 */
     offsetof(struct pg_conn, ssl_min_protocol_version)},


Re: should libpq also require TLSv1.2 by default?

From
Daniel Gustafsson
Date:
> On 26 Jun 2020, at 22:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
>> Anybdy have a better idea?  Is there a reasonably direct way to ask
>> OpenSSL what its min and max versions are?
>
> After some digging, there apparently is not.

AFAIK everyone either #ifdef around the TLS1_x_VERSION macros or the OpenSSL
versioning and use hardcoded knowledge based on that.  The latter is fairly
shaky since configure options can disable protocols.  At least in past
versions, the validation for protocol range in OpenSSL ssl_lib was doing pretty
much that too.

> So I propose the attached.

SSL_R_UNKNOWN_PROTOCOL seem to covers cases when someone manages to perform
something which OpenSSL believes is a broken SSLv2 connection, but their own
client-level code use it to refer to SSL as well as TLS.  Maybe it's worth
adding as a belts and suspenders type thing?

I've only had a chance to read the patches, but they read pretty much just like
I had in mind that we could do this. +1 on both patches from an eye-ball POV.

Is this targeting v13 or v14?  In case of the former, the release notes entry
for raising the default minimum version should perhaps be tweaked as it now
just refers to the GUC which is a tad misleading.

> The hack in openssl.h to guess the
> min/max supported versions is certainly nothing but a hack;
> but I see no way to do better.

If anything it might useful to document in the comment that we're only
concerned with TLS versions, SSL2/3 are disabled in the library initialization.

cheers ./daniel


Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
> SSL_R_UNKNOWN_PROTOCOL seem to covers cases when someone manages to perform
> something which OpenSSL believes is a broken SSLv2 connection, but their own
> client-level code use it to refer to SSL as well as TLS.  Maybe it's worth
> adding as a belts and suspenders type thing?

No objection on my part.

> Is this targeting v13 or v14?  In case of the former, the release notes entry
> for raising the default minimum version should perhaps be tweaked as it now
> just refers to the GUC which is a tad misleading.

I think Peter is proposing that we change this in v13.  I didn't look
at the release notes; usually we cover this sort of thing in-bulk
when we update the release notes later in beta.

> If anything it might useful to document in the comment that we're only
> concerned with TLS versions, SSL2/3 are disabled in the library initialization.

Good point.

            regards, tom lane



Re: should libpq also require TLSv1.2 by default?

From
Tom Lane
Date:
I wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> SSL_R_UNKNOWN_PROTOCOL seem to covers cases when someone manages to perform
>> something which OpenSSL believes is a broken SSLv2 connection, but their own
>> client-level code use it to refer to SSL as well as TLS.  Maybe it's worth
>> adding as a belts and suspenders type thing?

> No objection on my part.

>> If anything it might useful to document in the comment that we're only
>> concerned with TLS versions, SSL2/3 are disabled in the library initialization.

> Good point.

Pushed with those corrections.  I also rewrote the comment about which
error codes we'd seen in practice, after realizing that one of my tests
had been affected by the presence of "MinProtocol = TLSv1.2" in
RHEL8's openssl.cnf (causing a max setting less than that to be a local
configuration error, not something the server had rejected).

            regards, tom lane