On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote: > On 14 Jan 2020, at 04:54, Michael Paquier <mich...@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
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
signature.asc
Description: PGP signature
