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
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
signature.asc
Description: PGP signature
