pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedImprove errors when setting incorrect bounds for SSL protocols
15+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2020-04-29 e30b0b5cfa Fix check for conflicting SSL min/max protocol settings
2020-03-23 79dfa8afb2 Add bound checks for ssl_min_protocol_version and ssl_max_protocol_version
2020-02-06 414c2fd1e1 2f4733993a Revert "Add GUC checks for ssl_min_protocol_version and ssl_max_protocol_version"
2020-01-18 41aadeeb12 ac2dcca5df Add GUC checks for ssl_min_protocol_version and ssl_max_protocol_version
* Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-14 03:54 Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-01-14 03:54 UTC (permalink / raw)
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Daniel Gustafsson <daniel@yesql.se>
Hi all,
(Daniel G. in CC.)
As discussed on the thread to be able to set the min/max SSL protocols
with libpq, when mixing incorrect bounds the user experience is not
that good:
https://www.postgresql.org/message-id/9CFA34EE-F670-419D-B92C-CB7943A27573@yesql.se
It happens that the error generated with incorrect combinations
depends solely on what OpenSSL thinks is fine, and that's the
following:
psql: error: could not connect to server: SSL error: tlsv1 alert
internal error
It is hard for users to understand what such an error means and how to
act on it.
Please note that OpenSSL 1.1.0 has added two routines to be able to
get the min/max protocols set in a context, called
SSL_CTX_get_min/max_proto_version. Thinking about older versions of
OpenSSL I think that it is better to use
ssl_protocol_version_to_openssl to do the parsing work. I also found
that it is easier to check for compatible versions after setting both
bounds in the SSL context, so as there is no need to worry about
invalid values depending on the build of OpenSSL used.
So attached is a patch to improve the detection of incorrect
combinations. Once applied, we get a complain about an incorrect
version at server startup (FATAL) or reload (LOG). The patch includes
new regression tests.
Thoughts?
--
Michael
Attachments:
ssl-proto-context-v1.patch (text/x-diff, 5.2K) download
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 62f1fcab2b..75fdb2e91b 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -67,8 +67,7 @@ static bool SSL_initialized = false;
static bool dummy_ssl_passwd_cb_called = false;
static bool ssl_is_server_start;
-static int ssl_protocol_version_to_openssl(int v, const char *guc_name,
- int loglevel);
+static int ssl_protocol_version_to_openssl(int v);
#ifndef SSL_CTX_set_min_proto_version
static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
@@ -84,6 +83,8 @@ be_tls_init(bool isServerStart)
{
STACK_OF(X509_NAME) *root_cert_list = NULL;
SSL_CTX *context;
+ int ssl_ver_min = -1;
+ int ssl_ver_max = -1;
/* This stuff need be done only once. */
if (!SSL_initialized)
@@ -192,13 +193,19 @@ be_tls_init(bool isServerStart)
if (ssl_min_protocol_version)
{
- int ssl_ver = ssl_protocol_version_to_openssl(ssl_min_protocol_version,
- "ssl_min_protocol_version",
- isServerStart ? FATAL : LOG);
+ ssl_ver_min = ssl_protocol_version_to_openssl(ssl_min_protocol_version);
- if (ssl_ver == -1)
+ if (ssl_ver_min == -1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("%s setting %s not supported by this build",
+ "ssl_min_protocol_version",
+ GetConfigOption("ssl_min_protocol_version",
+ false, false))));
goto error;
- if (!SSL_CTX_set_min_proto_version(context, ssl_ver))
+ }
+
+ if (!SSL_CTX_set_min_proto_version(context, ssl_ver_min))
{
ereport(isServerStart ? FATAL : LOG,
(errmsg("could not set minimum SSL protocol version")));
@@ -208,13 +215,19 @@ be_tls_init(bool isServerStart)
if (ssl_max_protocol_version)
{
- int ssl_ver = ssl_protocol_version_to_openssl(ssl_max_protocol_version,
- "ssl_max_protocol_version",
- isServerStart ? FATAL : LOG);
+ ssl_ver_max = ssl_protocol_version_to_openssl(ssl_max_protocol_version);
- if (ssl_ver == -1)
+ if (ssl_ver_max == -1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("%s setting %s not supported by this build",
+ "ssl_max_protocol_version",
+ GetConfigOption("ssl_max_protocol_version",
+ false, false))));
goto error;
- if (!SSL_CTX_set_max_proto_version(context, ssl_ver))
+ }
+
+ if (!SSL_CTX_set_max_proto_version(context, ssl_ver_max))
{
ereport(isServerStart ? FATAL : LOG,
(errmsg("could not set maximum SSL protocol version")));
@@ -222,6 +235,20 @@ be_tls_init(bool isServerStart)
}
}
+ /* Check compatibility of min/max protocols */
+ if (ssl_min_protocol_version &&
+ ssl_max_protocol_version)
+ {
+ /*
+ * No need to check for invalid values (-1) for each protocol number
+ * as the code above would have already generated an error.
+ */
+ if (ssl_ver_min > ssl_ver_max)
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("incompatible min/max SSL protocol versions set")));
+ goto error;
+ }
+
/* disallow SSL session tickets */
SSL_CTX_set_options(context, SSL_OP_NO_TICKET);
@@ -1275,12 +1302,11 @@ X509_NAME_to_cstring(X509_NAME *name)
* guc.c independent of OpenSSL availability and version.
*
* If a version is passed that is not supported by the current OpenSSL
- * version, then we log with the given loglevel and return (if we return) -1.
- * If a nonnegative value is returned, subsequent code can assume it's working
- * with a supported version.
+ * version, then we return -1. If a nonnegative value is returned,
+ * subsequent code can assume it's working with a supported version.
*/
static int
-ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
+ssl_protocol_version_to_openssl(int v)
{
switch (v)
{
@@ -1308,10 +1334,6 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
#endif
}
- ereport(loglevel,
- (errmsg("%s setting %s not supported by this build",
- guc_name,
- GetConfigOption(guc_name, false, false))));
return -1;
}
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 83fcd5e839..7931ebc067 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
if ($ENV{with_openssl} eq 'yes')
{
- plan tests => 84;
+ plan tests => 86;
}
else
{
@@ -97,6 +97,22 @@ command_ok(
'restart succeeds with password-protected key file');
$node->_update_pid(1);
+# Test compatibility of SSL protocols.
+# TLSv1.1 is lower than TLSv1.2, so it won't work.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version='TLSv1.1'});
+command_fails(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart fails with incorrect SSL protocol bounds');
+# Go back to the defaults, this works.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version=''});
+command_ok(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart succeeds with correct SSL protocol bounds');
+
### Run client-side tests.
###
### Test that libpq accepts/rejects the connection correctly, depending
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-14 10:21 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2020-01-14 10:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
> On 14 Jan 2020, at 04:54, Michael Paquier <michael@paquier.xyz> wrote:
>
> Hi all,
> (Daniel G. in CC.)
>
> As discussed on the thread to be able to set the min/max SSL protocols
> with libpq, when mixing incorrect bounds the user experience is not
> that good:
> https://www.postgresql.org/message-id/9CFA34EE-F670-419D-B92C-CB7943A27573@yesql.se
>
> It happens that the error generated with incorrect combinations
> depends solely on what OpenSSL thinks is fine, and that's the
> following:
> psql: error: could not connect to server: SSL error: tlsv1 alert
> internal error
>
> It is hard for users to understand what such an error means and how to
> act on it.
Correct, it's an easy mistake to make but based on the error it might take some
time to figure it out.
> Please note that OpenSSL 1.1.0 has added two routines to be able to
> get the min/max protocols set in a context, called
> SSL_CTX_get_min/max_proto_version. Thinking about older versions of
> OpenSSL I think that it is better to use
> ssl_protocol_version_to_openssl to do the parsing work. I also found
> that it is easier to check for compatible versions after setting both
> bounds in the SSL context, so as there is no need to worry about
> invalid values depending on the build of OpenSSL used.
I'm not convinced that it's a good idea to check for incompatible protocol
range in the OpenSSL backend. We've spent a lot of energy to make the TLS code
library agnostic and pluggable, and since identifying a basic configuration
error isn't OpenSSL specific I think it should be in the guc code. That would
keep the layering as well as ensure that we don't mistakenly treat this
differently should we get a second TLS backend.
cheers ./daniel
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-15 02:28 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 2 replies; 15+ messages in thread
From: Michael Paquier @ 2020-01-15 02:28 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote:
> On 14 Jan 2020, at 04:54, Michael Paquier <michael@paquier.xyz> wrote:
>> Please note that OpenSSL 1.1.0 has added two routines to be able to
>> get the min/max protocols set in a context, called
>> SSL_CTX_get_min/max_proto_version. Thinking about older versions of
>> OpenSSL I think that it is better to use
>> ssl_protocol_version_to_openssl to do the parsing work. I also found
>> that it is easier to check for compatible versions after setting both
>> bounds in the SSL context, so as there is no need to worry about
>> invalid values depending on the build of OpenSSL used.
>
> I'm not convinced that it's a good idea to check for incompatible protocol
> range in the OpenSSL backend. We've spent a lot of energy to make the TLS code
> library agnostic and pluggable, and since identifying a basic configuration
> error isn't OpenSSL specific I think it should be in the guc code. That would
> keep the layering as well as ensure that we don't mistakenly treat this
> differently should we get a second TLS backend.
Good points. And the get routines are not that portable in OpenSSL
either even if HEAD supports 1.0.1 and newer versions... Attached is
an updated patch which uses a GUC check for both parameters, and
provides a hint on top of the original error message. The SSL context
does not get reloaded if there is an error, so the errors from OpenSSL
cannot be triggered as far as I checked (after mixing a couple of
corrent and incorrect combinations manually).
--
Michael
Attachments:
ssl-proto-context-v2.patch (text/x-diff, 3.9K) download
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e5f8a1301f..d97fe3beb2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -204,6 +204,10 @@ static const char *show_log_file_mode(void);
static const char *show_data_directory_mode(void);
static bool check_backtrace_functions(char **newval, void **extra, GucSource source);
static void assign_backtrace_functions(const char *newval, void *extra);
+static bool check_ssl_min_protocol_version(int *newval, void **extra,
+ GucSource source);
+static bool check_ssl_max_protocol_version(int *newval, void **extra,
+ GucSource source);
static bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
static void assign_recovery_target_timeline(const char *newval, void *extra);
static bool check_recovery_target(char **newval, void **extra, GucSource source);
@@ -4594,7 +4598,7 @@ static struct config_enum ConfigureNamesEnum[] =
&ssl_min_protocol_version,
PG_TLS1_2_VERSION,
ssl_protocol_versions_info + 1, /* don't allow PG_TLS_ANY */
- NULL, NULL, NULL
+ check_ssl_min_protocol_version, NULL, NULL
},
{
@@ -4606,7 +4610,7 @@ static struct config_enum ConfigureNamesEnum[] =
&ssl_max_protocol_version,
PG_TLS_ANY,
ssl_protocol_versions_info,
- NULL, NULL, NULL
+ check_ssl_max_protocol_version, NULL, NULL
},
/* End-of-list marker */
@@ -11603,6 +11607,49 @@ assign_backtrace_functions(const char *newval, void *extra)
backtrace_symbol_list = (char *) extra;
}
+static bool
+check_ssl_min_protocol_version(int *newval, void **extra, GucSource source)
+{
+ int new_ssl_min_protocol_version = *newval;
+
+ /* PG_TLS_ANY is not supported for the minimum bound */
+ Assert(new_ssl_min_protocol_version > PG_TLS_ANY);
+
+ if (ssl_max_protocol_version &&
+ new_ssl_min_protocol_version > ssl_max_protocol_version)
+ {
+ GUC_check_errhint("\"%s\" cannot be higher than \"%s\".",
+ "ssl_min_protocol_version",
+ "ssl_max_protocol_version");
+ GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
+ return false;
+ }
+
+ return true;
+}
+
+static bool
+check_ssl_max_protocol_version(int *newval, void **extra, GucSource source)
+{
+ int new_ssl_max_protocol_version = *newval;
+
+ /* if PG_TLS_ANY, there is no need to check the bounds */
+ if (new_ssl_max_protocol_version == PG_TLS_ANY)
+ return true;
+
+ if (ssl_min_protocol_version &&
+ ssl_min_protocol_version > new_ssl_max_protocol_version)
+ {
+ GUC_check_errhint("\"%s\" cannot be lower than \"%s\".",
+ "ssl_max_protocol_version",
+ "ssl_min_protocol_version");
+ GUC_check_errcode(ERRCODE_INVALID_PARAMETER_VALUE);
+ return false;
+ }
+
+ return true;
+}
+
static bool
check_recovery_target_timeline(char **newval, void **extra, GucSource source)
{
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 83fcd5e839..7931ebc067 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
if ($ENV{with_openssl} eq 'yes')
{
- plan tests => 84;
+ plan tests => 86;
}
else
{
@@ -97,6 +97,22 @@ command_ok(
'restart succeeds with password-protected key file');
$node->_update_pid(1);
+# Test compatibility of SSL protocols.
+# TLSv1.1 is lower than TLSv1.2, so it won't work.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version='TLSv1.1'});
+command_fails(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart fails with incorrect SSL protocol bounds');
+# Go back to the defaults, this works.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version=''});
+command_ok(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart succeeds with correct SSL protocol bounds');
+
### Run client-side tests.
###
### Test that libpq accepts/rejects the connection correctly, depending
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-15 17:34 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2020-01-15 17:34 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
> On 15 Jan 2020, at 03:28, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote:
>> On 14 Jan 2020, at 04:54, Michael Paquier <michael@paquier.xyz> wrote:
>>> Please note that OpenSSL 1.1.0 has added two routines to be able to
>>> get the min/max protocols set in a context, called
>>> SSL_CTX_get_min/max_proto_version. Thinking about older versions of
>>> OpenSSL I think that it is better to use
>>> ssl_protocol_version_to_openssl to do the parsing work. I also found
>>> that it is easier to check for compatible versions after setting both
>>> bounds in the SSL context, so as there is no need to worry about
>>> invalid values depending on the build of OpenSSL used.
>>
>> I'm not convinced that it's a good idea to check for incompatible protocol
>> range in the OpenSSL backend. We've spent a lot of energy to make the TLS code
>> library agnostic and pluggable, and since identifying a basic configuration
>> error isn't OpenSSL specific I think it should be in the guc code. That would
>> keep the layering as well as ensure that we don't mistakenly treat this
>> differently should we get a second TLS backend.
>
> Good points. And the get routines are not that portable in OpenSSL
> either even if HEAD supports 1.0.1 and newer versions... Attached is
> an updated patch which uses a GUC check for both parameters, and
> provides a hint on top of the original error message. The SSL context
> does not get reloaded if there is an error, so the errors from OpenSSL
> cannot be triggered as far as I checked (after mixing a couple of
> corrent and incorrect combinations manually).
This is pretty much exactly the patch I was intending to write for this, so +1
from me.
cheers ./daniel
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-16 01:00 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-01-16 01:00 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Jan 15, 2020 at 06:34:39PM +0100, Daniel Gustafsson wrote:
> This is pretty much exactly the patch I was intending to write for this, so +1
> from me.
Thanks for the review. Let's wait a couple of days to see if others
have objections or more comments about this patch, but I'd like to
fix the issue and backpatch down to 12 where the parameters have been
introduced.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-18 03:36 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-01-18 03:36 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Jan 16, 2020 at 10:00:52AM +0900, Michael Paquier wrote:
> Thanks for the review. Let's wait a couple of days to see if others
> have objections or more comments about this patch, but I'd like to
> fix the issue and backpatch down to 12 where the parameters have been
> introduced.
And committed.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-01-20 08:11 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 15+ messages in thread
From: Peter Eisentraut @ 2020-01-20 08:11 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; Daniel Gustafsson <daniel@yesql.se>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On 2020-01-15 03:28, Michael Paquier wrote:
> Good points. And the get routines are not that portable in OpenSSL
> either even if HEAD supports 1.0.1 and newer versions... Attached is
> an updated patch which uses a GUC check for both parameters, and
> provides a hint on top of the original error message. The SSL context
> does not get reloaded if there is an error, so the errors from OpenSSL
> cannot be triggered as far as I checked (after mixing a couple of
> corrent and incorrect combinations manually).
The reason this wasn't done originally is that it is not correct to have
GUC check hooks that refer to other GUC variables, because otherwise you
get inconsistent behavior depending on the order of processing of the
assignments. In this case, I think it would work because you have
symmetric checks for both variables, but in general it is a problematic
strategy.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-02-06 19:04 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Tom Lane @ 2020-02-06 19:04 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Daniel Gustafsson <daniel@yesql.se>; Postgres hackers <pgsql-hackers@lists.postgresql.org>
Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jan 16, 2020 at 10:00:52AM +0900, Michael Paquier wrote:
>> Thanks for the review. Let's wait a couple of days to see if others
>> have objections or more comments about this patch, but I'd like to
>> fix the issue and backpatch down to 12 where the parameters have been
>> introduced.
> And committed.
I just happened to look at this patch while working on the release notes.
I think this is a bad idea and very probably creates worse problems than
it fixes. As we have learned painfully in the past, you can't have GUC
check or assign hooks that look at other GUC variables, because that
creates order-of-operations problems. If a postgresql.conf update is
trying to change both values (hardly an unlikely scenario, for this
pair of variables) then the checks are going to be comparing against the
old values of the other variables, leading to either incorrect rejections
of valid states or incorrect acceptances of invalid states. It's pure
accident that the particular cases tested in the regression tests behave
sanely.
I think this should be reverted. Perhaps there's a way to do it without
these problems, but we failed to find one in the past.
regards, tom lane
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-02-06 22:30 Daniel Gustafsson <daniel@yesql.se>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2020-02-06 22:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; Postgres hackers <pgsql-hackers@lists.postgresql.org>
> On 6 Feb 2020, at 20:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think this should be reverted. Perhaps there's a way to do it without
> these problems, but we failed to find one in the past.
Or change to the v1 patch in this thread, which avoids the problem by doing it
in the OpenSSL code. It's a shame to have generic TLS functionality be OpenSSL
specific when everything else TLS has been abstracted, but not working is
clearly a worse option.
cheers ./daniel
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-02-07 00:33 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-02-07 00:33 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Feb 06, 2020 at 11:30:40PM +0100, Daniel Gustafsson wrote:
> Or change to the v1 patch in this thread, which avoids the problem by doing it
> in the OpenSSL code. It's a shame to have generic TLS functionality be OpenSSL
> specific when everything else TLS has been abstracted, but not working is
> clearly a worse option.
The v1 would work just fine considering that, as the code would be
invoked in a context where all GUCs are already loaded. That's too
late before the release though, so I have reverted 41aadee, and
attached is a new patch to consider with improvements compared to v1
mainly in the error messages.
--
Michael
Attachments:
ssl-proto-context-v3.patch (text/x-diff, 5.4K) download
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 18d3fff068..5b772fd58c 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -68,8 +68,7 @@ static bool SSL_initialized = false;
static bool dummy_ssl_passwd_cb_called = false;
static bool ssl_is_server_start;
-static int ssl_protocol_version_to_openssl(int v, const char *guc_name,
- int loglevel);
+static int ssl_protocol_version_to_openssl(int v);
/* ------------------------------------------------------------ */
/* Public interface */
@@ -80,6 +79,8 @@ be_tls_init(bool isServerStart)
{
STACK_OF(X509_NAME) *root_cert_list = NULL;
SSL_CTX *context;
+ int ssl_ver_min = -1;
+ int ssl_ver_max = -1;
/* This stuff need be done only once. */
if (!SSL_initialized)
@@ -188,13 +189,19 @@ be_tls_init(bool isServerStart)
if (ssl_min_protocol_version)
{
- int ssl_ver = ssl_protocol_version_to_openssl(ssl_min_protocol_version,
- "ssl_min_protocol_version",
- isServerStart ? FATAL : LOG);
+ ssl_ver_min = ssl_protocol_version_to_openssl(ssl_min_protocol_version);
- if (ssl_ver == -1)
+ if (ssl_ver_min == -1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("%s setting %s not supported by this build",
+ "ssl_min_protocol_version",
+ GetConfigOption("ssl_min_protocol_version",
+ false, false))));
goto error;
- if (!SSL_CTX_set_min_proto_version(context, ssl_ver))
+ }
+
+ if (!SSL_CTX_set_min_proto_version(context, ssl_ver_min))
{
ereport(isServerStart ? FATAL : LOG,
(errmsg("could not set minimum SSL protocol version")));
@@ -204,13 +211,19 @@ be_tls_init(bool isServerStart)
if (ssl_max_protocol_version)
{
- int ssl_ver = ssl_protocol_version_to_openssl(ssl_max_protocol_version,
- "ssl_max_protocol_version",
- isServerStart ? FATAL : LOG);
+ ssl_ver_max = ssl_protocol_version_to_openssl(ssl_max_protocol_version);
- if (ssl_ver == -1)
+ if (ssl_ver_max == -1)
+ {
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("%s setting %s not supported by this build",
+ "ssl_max_protocol_version",
+ GetConfigOption("ssl_max_protocol_version",
+ false, false))));
goto error;
- if (!SSL_CTX_set_max_proto_version(context, ssl_ver))
+ }
+
+ if (!SSL_CTX_set_max_proto_version(context, ssl_ver_max))
{
ereport(isServerStart ? FATAL : LOG,
(errmsg("could not set maximum SSL protocol version")));
@@ -218,6 +231,23 @@ be_tls_init(bool isServerStart)
}
}
+ /* Check compatibility of min/max protocols */
+ if (ssl_min_protocol_version &&
+ ssl_max_protocol_version)
+ {
+ /*
+ * No need to check for invalid values (-1) for each protocol number
+ * as the code above would have already generated an error.
+ */
+ if (ssl_ver_min > ssl_ver_max)
+ ereport(isServerStart ? FATAL : LOG,
+ (errmsg("could not set SSL protocol version range"),
+ errdetail("\"%s\" cannot be higher than \"%s\"",
+ "ssl_min_protocol_version",
+ "ssl_max_protocol_version")));
+ goto error;
+ }
+
/* disallow SSL session tickets */
SSL_CTX_set_options(context, SSL_OP_NO_TICKET);
@@ -1271,15 +1301,14 @@ X509_NAME_to_cstring(X509_NAME *name)
* guc.c independent of OpenSSL availability and version.
*
* If a version is passed that is not supported by the current OpenSSL
- * version, then we log with the given loglevel and return (if we return) -1.
- * If a nonnegative value is returned, subsequent code can assume it's working
- * with a supported version.
+ * version, then we return -1. If a nonnegative value is returned,
+ * subsequent code can assume it's working with a supported version.
*
* Note: this is rather similar to libpq's routine in fe-secure-openssl.c,
* so make sure to update both routines if changing this one.
*/
static int
-ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
+ssl_protocol_version_to_openssl(int v)
{
switch (v)
{
@@ -1307,9 +1336,5 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
#endif
}
- ereport(loglevel,
- (errmsg("%s setting %s not supported by this build",
- guc_name,
- GetConfigOption(guc_name, false, false))));
return -1;
}
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index e740099aca..d035ac7fc9 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
if ($ENV{with_openssl} eq 'yes')
{
- plan tests => 91;
+ plan tests => 93;
}
else
{
@@ -97,6 +97,22 @@ command_ok(
'restart succeeds with password-protected key file');
$node->_update_pid(1);
+# Test compatibility of SSL protocols.
+# TLSv1.1 is lower than TLSv1.2, so it won't work.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version='TLSv1.1'});
+command_fails(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart fails with incorrect SSL protocol bounds');
+# Go back to the defaults, this works.
+$node->append_conf('postgresql.conf',
+ qq{ssl_min_protocol_version='TLSv1.2'
+ssl_max_protocol_version=''});
+command_ok(
+ [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ],
+ 'restart succeeds with correct SSL protocol bounds');
+
### Run client-side tests.
###
### Test that libpq accepts/rejects the connection correctly, depending
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-03-19 21:54 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2020-03-19 21:54 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Postgres hackers <pgsql-hackers@lists.postgresql.org>
> On 7 Feb 2020, at 01:33, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 06, 2020 at 11:30:40PM +0100, Daniel Gustafsson wrote:
>> Or change to the v1 patch in this thread, which avoids the problem by doing it
>> in the OpenSSL code. It's a shame to have generic TLS functionality be OpenSSL
>> specific when everything else TLS has been abstracted, but not working is
>> clearly a worse option.
>
> The v1 would work just fine considering that, as the code would be
> invoked in a context where all GUCs are already loaded. That's too
> late before the release though, so I have reverted 41aadee, and
> attached is a new patch to consider with improvements compared to v1
> mainly in the error messages.
Having gone back to look at this, I can't think of a better way to implement
this and I think we should go ahead with the proposed patch.
In this message we aren't quoting the TLS protocol setting:
+ (errmsg("%s setting %s not supported by this build",
..but in this detail we are:
+ errdetail("\"%s\" cannot be higher than \"%s\"",
Perhaps we should be consistent across all ereports?
Marking as ready for committer.
cheers ./daniel
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-03-23 02:06 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-03-23 02:06 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Thu, Mar 19, 2020 at 10:54:35PM +0100, Daniel Gustafsson wrote:
> In this message we aren't quoting the TLS protocol setting:
> + (errmsg("%s setting %s not supported by this build",
> ..but in this detail we are:
> + errdetail("\"%s\" cannot be higher than \"%s\"",
> Perhaps we should be consistent across all ereports?
Right. Using quotes is a more popular style when it comes to GUC
parameters and their values, so switched to use that, and committed
the patch. Thanks for the review.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-04-29 11:57 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gustafsson @ 2020-04-29 11:57 UTC (permalink / raw)
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>; +Cc: Michael Paquier <michael@paquier.xyz>
Working in the TLS corners of the backend, I found while re-reviewing and
re-testing for the release that this patch actually was a small, but vital,
brick shy of a load. The error handling is always invoked due to a set of
missing braces. Going into the check will cause the context to be freed and
be_tls_open_server error out. The tests added narrowly escapes it by not
setting the max version in the final test, but I'm not sure it's worth changing
that now as not setting a value is an interesting testcase too. Sorry for
missing that at the time of reviewing.
cheers ./daniel
Attachments:
minmaxproto_guc.patch (application/octet-stream, 1.4K) download
From 00f2753e696709ee81529c7728a77ddf33a43784 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 29 Apr 2020 01:00:00 +0200
Subject: [PATCH] Fix check for conflicting min/max protocol settings
Commit 79dfa8afb296e1b0dcffbe674e04c5f25cc13dfd introduced a check
to catch when the minimum protocol version was set higher than the
maximum version. The conditional block lacked braces however so the
error handling always kicks in, which frees the SSL context making
the backend no longer working for SSL connections. Fix by enclosing
the block with braces.
---
src/backend/libpq/be-secure-openssl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index a65f920343..42c5c07e58 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -226,12 +226,14 @@ be_tls_init(bool isServerStart)
* as the code above would have already generated an error.
*/
if (ssl_ver_min > ssl_ver_max)
+ {
ereport(isServerStart ? FATAL : LOG,
(errmsg("could not set SSL protocol version range"),
errdetail("\"%s\" cannot be higher than \"%s\"",
"ssl_min_protocol_version",
"ssl_max_protocol_version")));
- goto error;
+ goto error;
+ }
}
/* disallow SSL session tickets */
--
2.21.1 (Apple Git-122.3)
=
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-04-29 23:14 Michael Paquier <michael@paquier.xyz>
parent: Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 15+ messages in thread
From: Michael Paquier @ 2020-04-29 23:14 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
On Wed, Apr 29, 2020 at 01:57:49PM +0200, Daniel Gustafsson wrote:
> Working in the TLS corners of the backend, I found while re-reviewing and
> re-testing for the release that this patch actually was a small, but vital,
> brick shy of a load. The error handling is always invoked due to a set of
> missing braces. Going into the check will cause the context to be freed and
> be_tls_open_server error out. The tests added narrowly escapes it by not
> setting the max version in the final test, but I'm not sure it's worth changing
> that now as not setting a value is an interesting testcase too. Sorry for
> missing that at the time of reviewing.
Good catch, fixed. We would still have keep around the SSL old
context if both bounds were set. Testing this case would mean one
extra full restart of the server, and I am not sure either if that's
worth the extra cost here.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Improve errors when setting incorrect bounds for SSL protocols
@ 2020-04-30 07:51 Daniel Gustafsson <daniel@yesql.se>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gustafsson @ 2020-04-30 07:51 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
> On 30 Apr 2020, at 01:14, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 29, 2020 at 01:57:49PM +0200, Daniel Gustafsson wrote:
>> Working in the TLS corners of the backend, I found while re-reviewing and
>> re-testing for the release that this patch actually was a small, but vital,
>> brick shy of a load. The error handling is always invoked due to a set of
>> missing braces. Going into the check will cause the context to be freed and
>> be_tls_open_server error out. The tests added narrowly escapes it by not
>> setting the max version in the final test, but I'm not sure it's worth changing
>> that now as not setting a value is an interesting testcase too. Sorry for
>> missing that at the time of reviewing.
>
> Good catch, fixed. We would still have keep around the SSL old
> context if both bounds were set. Testing this case would mean one
> extra full restart of the server, and I am not sure either if that's
> worth the extra cost here.
Agreed. I don't think the cost is warranted given the low probability of new
errors around here, so I think the commit as it stands is sufficient. Thanks.
cheers ./daniel
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2020-04-30 07:51 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 03:54 Improve errors when setting incorrect bounds for SSL protocols Michael Paquier <michael@paquier.xyz>
2020-01-14 10:21 ` Daniel Gustafsson <daniel@yesql.se>
2020-01-15 02:28 ` Michael Paquier <michael@paquier.xyz>
2020-01-15 17:34 ` Daniel Gustafsson <daniel@yesql.se>
2020-01-16 01:00 ` Michael Paquier <michael@paquier.xyz>
2020-01-18 03:36 ` Michael Paquier <michael@paquier.xyz>
2020-02-06 19:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-06 22:30 ` Daniel Gustafsson <daniel@yesql.se>
2020-02-07 00:33 ` Michael Paquier <michael@paquier.xyz>
2020-03-19 21:54 ` Daniel Gustafsson <daniel@yesql.se>
2020-03-23 02:06 ` Michael Paquier <michael@paquier.xyz>
2020-04-29 11:57 ` Daniel Gustafsson <daniel@yesql.se>
2020-04-29 23:14 ` Michael Paquier <michael@paquier.xyz>
2020-04-30 07:51 ` Daniel Gustafsson <daniel@yesql.se>
2020-01-20 08:11 ` Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox