Обсуждение: should libpq also require TLSv1.2 by default?
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
> 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
Вложения
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
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
> 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
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
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
> 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
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
Вложения
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
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
Вложения
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
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
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
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
> 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
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
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;
}
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)},
> 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
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
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