Commit 41aadeeb authored by Michael Paquier's avatar Michael Paquier

Add GUC checks for ssl_min_protocol_version and ssl_max_protocol_version

Mixing incorrect bounds set in the SSL context leads to confusing error
messages generated by OpenSSL which are hard to act on.  New checks are
added within the GUC machinery to improve the user experience as they
apply to any SSL implementation, not only OpenSSL, and doing the checks
beforehand avoids the creation of a SSL during a reload (or startup)
which we know will never be used anyway.

Backpatch down to 12, as those parameters have been introduced by
e73e67c7.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20200114035420.GE1515@paquier.xyz
Backpatch-through: 12
parent 4b754d6c
...@@ -204,6 +204,10 @@ static const char *show_log_file_mode(void); ...@@ -204,6 +204,10 @@ static const char *show_log_file_mode(void);
static const char *show_data_directory_mode(void); static const char *show_data_directory_mode(void);
static bool check_backtrace_functions(char **newval, void **extra, GucSource source); static bool check_backtrace_functions(char **newval, void **extra, GucSource source);
static void assign_backtrace_functions(const char *newval, void *extra); 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 bool check_recovery_target_timeline(char **newval, void **extra, GucSource source);
static void assign_recovery_target_timeline(const char *newval, void *extra); static void assign_recovery_target_timeline(const char *newval, void *extra);
static bool check_recovery_target(char **newval, void **extra, GucSource source); static bool check_recovery_target(char **newval, void **extra, GucSource source);
...@@ -4594,7 +4598,7 @@ static struct config_enum ConfigureNamesEnum[] = ...@@ -4594,7 +4598,7 @@ static struct config_enum ConfigureNamesEnum[] =
&ssl_min_protocol_version, &ssl_min_protocol_version,
PG_TLS1_2_VERSION, PG_TLS1_2_VERSION,
ssl_protocol_versions_info + 1, /* don't allow PG_TLS_ANY */ 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[] = ...@@ -4606,7 +4610,7 @@ static struct config_enum ConfigureNamesEnum[] =
&ssl_max_protocol_version, &ssl_max_protocol_version,
PG_TLS_ANY, PG_TLS_ANY,
ssl_protocol_versions_info, ssl_protocol_versions_info,
NULL, NULL, NULL check_ssl_max_protocol_version, NULL, NULL
}, },
/* End-of-list marker */ /* End-of-list marker */
...@@ -11603,6 +11607,49 @@ assign_backtrace_functions(const char *newval, void *extra) ...@@ -11603,6 +11607,49 @@ assign_backtrace_functions(const char *newval, void *extra)
backtrace_symbol_list = (char *) 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 static bool
check_recovery_target_timeline(char **newval, void **extra, GucSource source) check_recovery_target_timeline(char **newval, void **extra, GucSource source)
{ {
......
...@@ -13,7 +13,7 @@ use SSLServer; ...@@ -13,7 +13,7 @@ use SSLServer;
if ($ENV{with_openssl} eq 'yes') if ($ENV{with_openssl} eq 'yes')
{ {
plan tests => 84; plan tests => 86;
} }
else else
{ {
...@@ -97,6 +97,24 @@ command_ok( ...@@ -97,6 +97,24 @@ command_ok(
'restart succeeds with password-protected key file'); 'restart succeeds with password-protected key file');
$node->_update_pid(1); $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. ### Run client-side tests.
### ###
### Test that libpq accepts/rejects the connection correctly, depending ### Test that libpq accepts/rejects the connection correctly, depending
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment