Improve errors when setting incorrect bounds for SSL protocols
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:
/messages/by-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.patchtext/x-diff; charset=us-asciiDownload
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
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:
/messages/by-id/9CFA34EE-F670-419D-B92C-CB7943A27573@yesql.seIt 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 errorIt 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
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.patchtext/x-diff; charset=us-asciiDownload
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
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
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
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
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
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
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
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.patchtext/x-diff; charset=us-asciiDownload
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
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
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
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.patchapplication/octet-stream; name=minmaxproto_guc.patch; x-unix-mode=0644Download
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)
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
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