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
e73e67c.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20200114035420[email protected]
Backpatch-through: 12

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index dc8f910ea46bfaac67a36e9081814e00ff9483ec..c94fe584177bec0a87363dc966b938a73195f4fd 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -201,6 +201,10 @@ static bool check_cluster_name(char **newval, void **extra, GucSource source);
 static const char *show_unix_socket_permissions(void);
 static const char *show_log_file_mode(void);
 static const char *show_data_directory_mode(void);
+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);
@@ -4522,7 +4526,7 @@ static struct config_enum ConfigureNamesEnum[] =
        &ssl_min_protocol_version,
        PG_TLS1_VERSION,
        ssl_protocol_versions_info + 1, /* don't allow PG_TLS_ANY */
-       NULL, NULL, NULL
+       check_ssl_min_protocol_version, NULL, NULL
    },
 
    {
@@ -4534,7 +4538,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 */
@@ -11462,6 +11466,49 @@ show_data_directory_mode(void)
    return buf;
 }
 
+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 67a3a28db6a1d997cf8dbddb17d5b82bd25264af..66278381bd24be8d9ec56dd4ec181ff2de1f8af9 100644 (file)
--- 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 => 75;
+   plan tests => 77;
 }
 else
 {
@@ -87,6 +87,24 @@ 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'
+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

diff --git a/src/test/ssl/t/SSLServer.pm b/src/test/ssl/t/SSLServer.pm
index d25c38dbbc7f1271f119ded38ebfde52e85ecb73..228cddf3a2cfbd02cfce865ce8130874c7ad930f 100644 (file)
--- a/src/test/ssl/t/SSLServer.pm
+++ b/src/test/ssl/t/SSLServer.pm
@@ -128,7 +128,7 @@ sub configure_test_server_for_ssl
    print $conf "log_statement=all\n";
 
    # enable SSL and set up server key
-   print $conf "include 'sslconfig.conf'";
+   print $conf "include 'sslconfig.conf'\n";
 
    close $conf;