On Fri, Jan 24, 2020 at 12:19:31PM +0100, Daniel Gustafsson wrote: > Attached is a v5 of the patch which hopefully address all the comments raised, > sorry for the delay.
Thanks for the new version.
psql: error: could not connect to server: invalid or unsupported
maximum protocol version specified: TLSv1.3
Running the regression tests with OpenSSL 1.0.1, 1.0.2 or 1.1.0 fails,
because TLSv1.3 (TLS1_3_VERSION) is not supported in those versions.
I think that it is better to just rely on TLSv1.2 for all that,
knowing that the server default for the minimum bound is v1.2.
+test_connect_fails(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1.2",
+ qr/invalid protocol version range/,
+ "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
+
+test_connect_fails(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require
sslminprotocolversion=TLSv1.3 sslmaxprotocolversion=tlsv1",
+ qr/invalid protocol version range/,
+ "connect with an incorrect range of TLS protocol versions
leaving no versions allowed");
This is testing twice pattern the same thing, and I am not sure if is
is worth bothering about the special case with TLSv1 (using just a
comparison with pg_strcasecmp you don't actually need those special
checks..).
Tests should make sure that a failure happens when an incorrect value
is set for sslminprotocolversion and sslmaxprotocolversion.
For readability, I think that it is better to consider NULL or empty
values for the parameters to be valid. They are actually valid
values, because they just get ignored when creating the connection.
Adding an assertion within the routine for the protocol range check to
make sure that the values are valid makes the code more robust.
+ {"sslminprotocolversion", "PGSSLMINPROTOCOLVERSION", NULL,
NULL,
+ "SSL-Minimum-Protocol-Version", "", /*
sizeof("TLSv1.x") */ 7,
+ offsetof(struct pg_conn, sslminprotocolversion)},
+
+ {"sslmaxprotocolversion", "PGSSLMAXPROTOCOLVERSION", NULL,
NULL,
+ "SSL-Maximum-Protocol-Version", "", /*
sizeof("TLSv1.x") */ 7,
Missing a zero-terminator in the count here. And actually
gssencmode is wrong as well.. I'll report that on a different
thread.
+# Test min/mix protocol versions
Typo here.
+bool
+pq_verify_ssl_protocol_option(const char *protocolversion)
[...]
+bool
+pq_verify_ssl_protocol_range(const char *min, const char *max)
Both routines are just used in fe-connect.c to validate the connection
parameters, so it is better to keep them static and in fe-connect.c in
my opinion.
+ if (*(min + strlen("TLSv1.")) > *(max + strlen("TLSv1.")))
+ return false;
It is enough to use pg_strcasecmp() here.
Hm. I am not sure that having a separate section "Client Protocol
Usage" brings much, so I have removed this one, and added an extra
sentence for the maximum protocol regarding its value for testing or
protocol compatibility.
The regression tests of postgres_fdw failed because of the new
parameters. One update later, they run fine.
So, attached is an updated version of the patch that I have spent a
couple of hours polishing. What do you think?
--
Michael
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 0cc59f1be1..987ab660cb 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -1274,6 +1274,9 @@ X509_NAME_to_cstring(X509_NAME *name)
* 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.
+ *
+ * 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)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 80b54bc92b..8498f32f8d 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -320,6 +320,14 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Require-Peer", "", 10,
offsetof(struct pg_conn, requirepeer)},
+ {"sslminprotocolversion", "PGSSLMINPROTOCOLVERSION", NULL, NULL,
+ "SSL-Minimum-Protocol-Version", "", 8, /* sizeof("TLSv1.x") == 8 */
+ offsetof(struct pg_conn, sslminprotocolversion)},
+
+ {"sslmaxprotocolversion", "PGSSLMAXPROTOCOLVERSION", NULL, NULL,
+ "SSL-Maximum-Protocol-Version", "", 8, /* sizeof("TLSv1.x") == 8 */
+ offsetof(struct pg_conn, sslmaxprotocolversion)},
+
/*
* As with SSL, all GSS options are exposed even in builds that don't have
* support.
@@ -426,6 +434,8 @@ static char *passwordFromFile(const char *hostname, const char *port, const char
const char *username, const char *pgpassfile);
static void pgpassfileWarning(PGconn *conn);
static void default_threadlock(int acquire);
+static bool sslVerifyProtocolVersion(const char *version);
+static bool sslVerifyProtocolRange(const char *min, const char *max);
/* global variable because fe-auth.c needs to access it */
@@ -1285,6 +1295,40 @@ connectOptions2(PGconn *conn)
goto oom_error;
}
+ /*
+ * Validate TLS protocol versions for sslminprotocolversion and
+ * sslmaxprotocolversion.
+ */
+ if (!sslVerifyProtocolVersion(conn->sslminprotocolversion))
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid sslminprotocolversion value: \"%s\"\n"),
+ conn->sslminprotocolversion);
+ return false;
+ }
+ if (!sslVerifyProtocolVersion(conn->sslmaxprotocolversion))
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid sslmaxprotocolversion value: \"%s\"\n"),
+ conn->sslmaxprotocolversion);
+ return false;
+ }
+
+ /*
+ * Check if the range of SSL protocols defined is correct. This is done
+ * at this early step because this is independent of the SSL
+ * implementation used, and this avoids unnecessary cycles with an
+ * already-built SSL context when the connection is being established, as
+ * it would be doomed anyway.
+ */
+ if (!sslVerifyProtocolRange(conn->sslminprotocolversion,
+ conn->sslmaxprotocolversion))
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid SSL protocol version range"));
+ return false;
+ }
+
/*
* validate gssencmode option
*/
@@ -4001,6 +4045,10 @@ freePGconn(PGconn *conn)
free(conn->sslcompression);
if (conn->requirepeer)
free(conn->requirepeer);
+ if (conn->sslminprotocolversion)
+ free(conn->sslminprotocolversion);
+ if (conn->sslmaxprotocolversion)
+ free(conn->sslmaxprotocolversion);
if (conn->gssencmode)
free(conn->gssencmode);
if (conn->krbsrvname)
@@ -7031,6 +7079,71 @@ pgpassfileWarning(PGconn *conn)
}
}
+/*
+ * Check if the SSL procotol value given in input is valid or not.
+ * This is used as a sanity check routine for the connection parameters
+ * sslminprotocolversion and sslmaxprotocolversion.
+ */
+static bool
+sslVerifyProtocolVersion(const char *version)
+{
+ /*
+ * An empty string and a NULL value are considered valid as it is
+ * equivalent to ignoring the parameter.
+ */
+ if (!version || strlen(version) == 0)
+ return true;
+
+ if (pg_strcasecmp(version, "TLSv1") == 0 ||
+ pg_strcasecmp(version, "TLSv1.1") == 0 ||
+ pg_strcasecmp(version, "TLSv1.2") == 0 ||
+ pg_strcasecmp(version, "TLSv1.3") == 0)
+ return true;
+
+ /* anything else is wrong */
+ return false;
+}
+
+
+/*
+ * Ensure that the SSL protocol range given in input is correct. The check
+ * is performed on the input string to keep it TLS backend agnostic. Input
+ * to this function is expected verified with sslVerifyProtocolVersion().
+ */
+static bool
+sslVerifyProtocolRange(const char *min, const char *max)
+{
+ Assert(sslVerifyProtocolVersion(min) &&
+ sslVerifyProtocolVersion(max));
+
+ /* If at least one of the bounds is not set, the range is valid */
+ if (min == NULL || max == NULL || strlen(min) == 0 || strlen(max) == 0)
+ return true;
+
+ /*
+ * If the minimum version is the lowest one we accept, then all options
+ * for the maximum are valid.
+ */
+ if (pg_strcasecmp(min, "TLSv1") == 0)
+ return true;
+
+ /*
+ * The minimum bound is valid, and cannot be TLSv1, so using TLSv1 for the
+ * maximum is incorrect.
+ */
+ if (pg_strcasecmp(max, "TLSv1") == 0)
+ return false;
+
+ /*
+ * At this point we know that we have a mix of TLSv1.1 through 1.3
+ * versions.
+ */
+ if (pg_strcasecmp(min, max) > 0)
+ return false;
+
+ return true;
+}
+
/*
* Obtain user's home directory, return in given buffer
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0e84fc8ac6..acf2addcc9 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -30,6 +30,7 @@
#include "fe-auth.h"
#include "fe-secure-common.h"
#include "libpq-int.h"
+#include "common/openssl.h"
#ifdef WIN32
#include "win32.h"
@@ -95,6 +96,7 @@ static long win32_ssl_create_mutex = 0;
#endif /* ENABLE_THREAD_SAFETY */
static PQsslKeyPassHook_type PQsslKeyPassHook = NULL;
+static int ssl_protocol_version_to_openssl(const char *protocol);
/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
@@ -787,6 +789,8 @@ initialize_SSL(PGconn *conn)
bool have_cert;
bool have_rootcert;
EVP_PKEY *pkey = NULL;
+ int ssl_max_ver;
+ int ssl_min_ver;
/*
* We'll need the home directory if any of the relevant parameters are
@@ -843,6 +847,54 @@ initialize_SSL(PGconn *conn)
/* Disable old protocol versions */
SSL_CTX_set_options(SSL_context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
+ if (conn->sslminprotocolversion &&
+ strlen(conn->sslminprotocolversion) != 0)
+ {
+ ssl_min_ver = ssl_protocol_version_to_openssl(conn->sslminprotocolversion);
+
+ if (ssl_min_ver == -1)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid value \"%s\" for minimum version of SSL protocol\n"),
+ conn->sslminprotocolversion);
+ return -1;
+ }
+
+ if (!SSL_CTX_set_min_proto_version(SSL_context, ssl_min_ver))
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set minimum version of SSL protocol: %s\n"),
+ err);
+ return -1;
+ }
+ }
+
+ if (conn->sslmaxprotocolversion &&
+ strlen(conn->sslmaxprotocolversion) != 0)
+ {
+ ssl_max_ver = ssl_protocol_version_to_openssl(conn->sslmaxprotocolversion);
+
+ if (ssl_max_ver == -1)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid value \"%s\" for maximum version of SSL protocol\n"),
+ conn->sslmaxprotocolversion);
+ return -1;
+ }
+
+ if (!SSL_CTX_set_max_proto_version(SSL_context, ssl_max_ver))
+ {
+ char *err = SSLerrmessage(ERR_get_error());
+
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not set maximum version of SSL protocol: %s\n"),
+ err);
+ return -1;
+ }
+ }
+
/*
* Disable OpenSSL's moving-write-buffer sanity check, because it causes
* unnecessary failures in nonblocking send cases.
@@ -1659,3 +1711,37 @@ PQssl_passwd_cb(char *buf, int size, int rwflag, void *userdata)
else
return PQdefaultSSLKeyPassHook(buf, size, conn);
}
+
+/*
+ * Convert TLS protocol version string to OpenSSL values
+ *
+ * If a version is passed that is not supported by the current OpenSSL version,
+ * then we return -1. If a non-negative value is returned, subsequent code can
+ * assume it is working with a supported version.
+ *
+ * Note: this is rather similar to the backend routine in be-secure-openssl.c,
+ * so make sure to update both routines if changing this one.
+ */
+static int
+ssl_protocol_version_to_openssl(const char *protocol)
+{
+ if (pg_strcasecmp("TLSv1", protocol) == 0)
+ return TLS1_VERSION;
+
+#ifdef TLS1_1_VERSION
+ if (pg_strcasecmp("TLSv1.1", protocol) == 0)
+ return TLS1_1_VERSION;
+#endif
+
+#ifdef TLS1_2_VERSION
+ if (pg_strcasecmp("TLSv1.2", protocol) == 0)
+ return TLS1_2_VERSION;
+#endif
+
+#ifdef TLS1_3_VERSION
+ if (pg_strcasecmp("TLSv1.3", protocol) == 0)
+ return TLS1_3_VERSION;
+#endif
+
+ return -1;
+}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 79bc3780ff..72931e6019 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -367,6 +367,8 @@ struct pg_conn
char *krbsrvname; /* Kerberos service name */
char *gsslib; /* What GSS library to use ("gssapi" or
* "sspi") */
+ char *sslminprotocolversion; /* minimum TLS protocol version */
+ char *sslmaxprotocolversion; /* maximum TLS protocol version */
/* Type of connection to make. Possible values: any, read-write. */
char *target_session_attrs;
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 7b18402cf6..6b57b16fab 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 => 86;
+ plan tests => 93;
}
else
{
@@ -356,6 +356,27 @@ command_like(
^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,_null_,_null_,_null_\r?$}mx,
'pg_stat_ssl view without client certificate');
+# Test min/max SSL protocol versions.
+test_connect_ok(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require sslminprotocolversion=TLSv1.2 sslmaxprotocolversion=TLSv1.2",
+ "connection success with correct range of TLS protocol versions");
+test_connect_fails(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require sslminprotocolversion=TLSv1.2 sslmaxprotocolversion=TLSv1.1",
+ qr/invalid SSL protocol version range/,
+ "connection failure with incorrect range of TLS protocol versions");
+test_connect_fails(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require sslminprotocolversion=incorrect_tls",
+ qr/invalid sslminprotocolversion value/,
+ "connection failure with an incorrect SSL protocol minimum bound");
+test_connect_fails(
+ $common_connstr,
+ "sslrootcert=ssl/root+server_ca.crt sslmode=require sslmaxprotocolversion=incorrect_tls",
+ qr/invalid sslmaxprotocolversion value/,
+ "connection failure with an incorrect SSL protocol maximum bound");
+
### Server-side tests.
###
### Test certificate authorization.
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index fcbf7fafbd..9a24c19ccb 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1732,6 +1732,40 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="libpq-connect-sslminprotocolversion" xreflabel="sslminprotocolversion">
+ <term><literal>sslminprotocolversion</literal></term>
+ <listitem>
+ <para>
+ This parameter specifies the minimum SSL/TLS protocol version to allow
+ for the connection. Valid values are <literal>TLSv1</literal>,
+ <literal>TLSv1.1</literal>, <literal>TLSv1.2</literal> and
+ <literal>TLSv1.3</literal>. The supported protocols depend on the
+ version of <productname>OpenSSL</productname> used, older versions
+ not supporting the most modern protocol versions. If not set, this
+ parameter is ignored and the connection will use the minimum bound
+ defined by the backend.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="libpq-connect-sslmaxprotocolversion" xreflabel="sslmaxprotocolversion">
+ <term><literal>sslmaxprotocolversion</literal></term>
+ <listitem>
+ <para>
+ This parameter specifies the maximum SSL/TLS protocol version to allow
+ for the connection. Valid values are <literal>TLSv1</literal>,
+ <literal>TLSv1.1</literal>, <literal>TLSv1.2</literal> and
+ <literal>TLSv1.3</literal>. The supported protocols depend on the
+ version of <productname>OpenSSL</productname> used, older versions
+ not supporting the most modern protocol versions. If not set, this
+ parameter is ignored and the connection will use the maximum bound
+ defined by the backend, if set. Setting the maximum protocol version
+ is mainly useful for testing or if some component has issues working
+ with a newer protocol.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-krbsrvname" xreflabel="krbsrvname">
<term><literal>krbsrvname</literal></term>
<listitem>
@@ -7120,6 +7154,26 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough)
</para>
</listitem>
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLMINPROTOCOLVERSION</envar></primary>
+ </indexterm>
+ <envar>PGSSLMINPROTOCOLVERSION</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslminprotocolversion"/> connection parameter.
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ <indexterm>
+ <primary><envar>PGSSLMAXPROTOCOLVERSION</envar></primary>
+ </indexterm>
+ <envar>PGSSLMAXPROTOCOLVERSION</envar> behaves the same as the <xref
+ linkend="libpq-connect-sslminprotocolversion"/> connection parameter.
+ </para>
+ </listitem>
+
<listitem>
<para>
<indexterm>
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index ebe7bfde23..62c2697920 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8898,7 +8898,7 @@ DO $d$
END;
$d$;
ERROR: invalid option "password"
-HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size
+HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, sslminprotocolversion, sslmaxprotocolversion, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size
CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
PL/pgSQL function inline_code_block line 3 at EXECUTE
-- If we add a password for our user mapping instead, we should get a different
signature.asc
Description: PGP signature
