Re: Setting min/max TLS protocol in clientside libpq
Daniel Gustafsson <[email protected]> Fri, 10 Jan 2020 00:01:36 +0100
| Newsgroups | gmane.comp.db.postgresql.devel.general |
|---|---|
| Message-ID | <[email protected]> |
Thanks for review everyone! A v2 of the patch which I believe addresses all concerns raised is attached. > On 6 Jan 2020, at 07:01, Michael Paquier <[email protected]> wrote: > > On Thu, Jan 02, 2020 at 09:46:44PM +0000, cary huang wrote: >> I agree with Arthur that it makes sense to check the validity of >> "conn->sslmaxprotocolversion" first before checking if it is larger >> than "conn->sslminprotocolversion" > > Here I don't agree. Why not just let OpenSSL handle things with > SSL_CTX_set_min_proto_version? We don't bother about that in the > backend code for that reason on top of keeping the code more simple > with less error handling. And things are cleaner when it comes to > this libpq patch by giving up with the INT_MIN hack. I looked into this and it turns out that OpenSSL does nothing to prevent the caller from setting a nonsensical protocol range like min=tlsv1.3,max=tlsv1.1. Thus, it's quite easy to screw up the backend server config and get it to start properly, but with quite unrelated error messages as a result on connection. Since I think this needs to be dealt with for both backend and frontend (if this is accepted), I removed it from this patch to return to it in a separate thread. >> In the patch, if the client does not supply "sslminprotocolversion", >> it will run to "else" statement and sets TLS min version to "INT_MIN", >> which is a huge negative number and of course openssl won't set >> it. I think this else statement can be enhanced a little to set >> "sslminprotocolversion" to TLSv1.2 by default to match the server >> and provide some warning message that TLS minimum has defaulted to >> TLSv1.2. > > In this patch fe-secure-openssl.c has just done a copy-paste of > SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version > present in be-secure-openssl.c. That's not good. Could you refactor > that please as a separate file? Done. I opted for a more generic header to make usage of the code easier, not sure if thats ok. One thing I noticed when looking at it is that we now have sha2_openssl.c and openssl_protocol.c in src/common. For easier visual grouping of OpenSSL functionality, it makes sense to me to rename sha2_openssl.c to openssl_sha2.c, but that might just be pointless churn. > The patch should have tests in src/test/ssl/, like for invalid values, > incorrect combinations leading to failures, etc. Also done. cheers ./daniel
libpq_minmaxproto_v2.patch
(application/octet-stream, 17.6 KB)
From 2f280935fb300c48a5082191d4e51b5ed1239283 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson <[email protected]> Date: Sat, 30 Nov 2019 01:32:04 +0100 Subject: [PATCH] Allow setting min/max TLS protocol version in libpq In the backend there are GUCs to control the minimum and maximum TLS versions to allow for a connection, but the clientside libpq lacked this ability. Disallowing servers which aren't providing secure TLS protocols is of interest to clients, but we provide a maximum protocol version setting by the same rationale as for the backend; to aid with testing and to cope with misbehaving software. This refactors the OpenSSL replacement functions for setting TLS version from the backend to src/common to avoid code duplication. --- doc/src/sgml/libpq.sgml | 65 +++++++++++++ src/backend/libpq/be-secure-openssl.c | 99 +------------------ src/common/Makefile | 3 +- src/common/openssl_protocol.c | 115 +++++++++++++++++++++++ src/include/common/openssl.h | 27 ++++++ src/interfaces/libpq/fe-connect.c | 8 ++ src/interfaces/libpq/fe-secure-openssl.c | 81 ++++++++++++++++ src/interfaces/libpq/libpq-int.h | 2 + src/test/ssl/t/001_ssltests.pl | 14 ++- 9 files changed, 314 insertions(+), 100 deletions(-) create mode 100644 src/common/openssl_protocol.c create mode 100644 src/include/common/openssl.h diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 64cff49c4d..5c7816ce6f 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1727,6 +1727,33 @@ 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 + doesn't support the modern protocol versions. + </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. The supported values are the same as for <literal> + sslminprotocolversion</literal>. Setting a maximum protocol version is + generally only useful for testing, or in case there are software components + which doesn't support newer protocol versions. + </para> + </listitem> + </varlistentry> + <varlistentry id="libpq-connect-krbsrvname" xreflabel="krbsrvname"> <term><literal>krbsrvname</literal></term> <listitem> @@ -7115,6 +7142,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> @@ -7788,6 +7835,24 @@ ldap://ldap.acme.com/cn=dbserver,cn=hosts?pgconnectinfo?base?(objectclass=*) </sect2> + <sect2> + <title>Client Protocol Usage</title> + + <para> + When connecting using SSL, the client and server negotiate which protocol + to use for the connection. <productname>PostgreSQL</productname> supports + <literal>TLSv1</literal>, <literal>TLSv1.1</literal>, <literal>TLSv1.2</literal> + and <literal>TLSv1.3</literal>, but the protocols available depends on the + version of <productname>OpenSSL</productname> which the client is using. + The minimum requested version can be specified with <literal>sslminprotocolversion</literal>, + which will ensure that the connection use that version, or higher, or fails. + The maximum requested version can be specified with <literal>sslmaxprotocolversion</literal>, + but this is mainly only useful for testing, or in case a component doesn't + work with a newer protocol. + </para> + + </sect2> + <sect2 id="libpq-ssl-fileusage"> <title>SSL Client File Usage</title> diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 62f1fcab2b..0cc59f1be1 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -36,6 +36,7 @@ #include <openssl/ec.h> #endif +#include "common/openssl.h" #include "libpq/libpq.h" #include "miscadmin.h" #include "pgstat.h" @@ -69,11 +70,6 @@ static bool ssl_is_server_start; static int ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel); -#ifndef SSL_CTX_set_min_proto_version -static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version); -static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version); -#endif - /* ------------------------------------------------------------ */ /* Public interface */ @@ -1314,96 +1310,3 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel) GetConfigOption(guc_name, false, false)))); return -1; } - -/* - * Replacements for APIs present in newer versions of OpenSSL - */ -#ifndef SSL_CTX_set_min_proto_version - -/* - * OpenSSL versions that support TLS 1.3 shouldn't get here because they - * already have these functions. So we don't have to keep updating the below - * code for every new TLS version, and eventually it can go away. But let's - * just check this to make sure ... - */ -#ifdef TLS1_3_VERSION -#error OpenSSL version mismatch -#endif - -static int -SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version) -{ - int ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3; - - if (version > TLS1_VERSION) - ssl_options |= SSL_OP_NO_TLSv1; - /* - * Some OpenSSL versions define TLS*_VERSION macros but not the - * corresponding SSL_OP_NO_* macro, so in those cases we have to return - * unsuccessfully here. - */ -#ifdef TLS1_1_VERSION - if (version > TLS1_1_VERSION) - { -#ifdef SSL_OP_NO_TLSv1_1 - ssl_options |= SSL_OP_NO_TLSv1_1; -#else - return 0; -#endif - } -#endif -#ifdef TLS1_2_VERSION - if (version > TLS1_2_VERSION) - { -#ifdef SSL_OP_NO_TLSv1_2 - ssl_options |= SSL_OP_NO_TLSv1_2; -#else - return 0; -#endif - } -#endif - - SSL_CTX_set_options(ctx, ssl_options); - - return 1; /* success */ -} - -static int -SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version) -{ - int ssl_options = 0; - - AssertArg(version != 0); - - /* - * Some OpenSSL versions define TLS*_VERSION macros but not the - * corresponding SSL_OP_NO_* macro, so in those cases we have to return - * unsuccessfully here. - */ -#ifdef TLS1_1_VERSION - if (version < TLS1_1_VERSION) - { -#ifdef SSL_OP_NO_TLSv1_1 - ssl_options |= SSL_OP_NO_TLSv1_1; -#else - return 0; -#endif - } -#endif -#ifdef TLS1_2_VERSION - if (version < TLS1_2_VERSION) - { -#ifdef SSL_OP_NO_TLSv1_2 - ssl_options |= SSL_OP_NO_TLSv1_2; -#else - return 0; -#endif - } -#endif - - SSL_CTX_set_options(ctx, ssl_options); - - return 1; /* success */ -} - -#endif /* !SSL_CTX_set_min_proto_version */ diff --git a/src/common/Makefile b/src/common/Makefile index ffb0f6edff..142e6edfbb 100644 --- a/src/common/Makefile +++ b/src/common/Makefile @@ -73,7 +73,8 @@ OBJS_COMMON = \ wait_error.o ifeq ($(with_openssl),yes) -OBJS_COMMON += sha2_openssl.o +OBJS_COMMON += sha2_openssl.o \ + openssl_protocol.o else OBJS_COMMON += sha2.o endif diff --git a/src/common/openssl_protocol.c b/src/common/openssl_protocol.c new file mode 100644 index 0000000000..eef86b8a68 --- /dev/null +++ b/src/common/openssl_protocol.c @@ -0,0 +1,115 @@ +/*------------------------------------------------------------------------- + * + * openssl_protocol.c + * OpenSSL functionality shared between frontend and backend + * + * This should only be used if code is compiled with OpenSSL support. + * + * Portions Copyright (c) 2018-2020, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/common/openssl_protocol.c + * + *------------------------------------------------------------------------- + */ + +#ifndef FRONTEND +#include "postgres.h" +#else +#include "postgres_fe.h" +#endif + +#include <openssl/ssl.h> + +/* + * Replacements for APIs present in newer versions of OpenSSL + */ +#ifndef SSL_CTX_set_min_proto_version + +/* + * OpenSSL versions that support TLS 1.3 shouldn't get here because they + * already have these functions. So we don't have to keep updating the below + * code for every new TLS version, and eventually it can go away. But let's + * just check this to make sure ... + */ +#ifdef TLS1_3_VERSION +#error OpenSSL version mismatch +#endif + +static int +SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version) +{ + int ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3; + + if (version > TLS1_VERSION) + ssl_options |= SSL_OP_NO_TLSv1; + /* + * Some OpenSSL versions define TLS*_VERSION macros but not the + * corresponding SSL_OP_NO_* macro, so in those cases we have to return + * unsuccessfully here. + */ +#ifdef TLS1_1_VERSION + if (version > TLS1_1_VERSION) + { +#ifdef SSL_OP_NO_TLSv1_1 + ssl_options |= SSL_OP_NO_TLSv1_1; +#else + return 0; +#endif + } +#endif +#ifdef TLS1_2_VERSION + if (version > TLS1_2_VERSION) + { +#ifdef SSL_OP_NO_TLSv1_2 + ssl_options |= SSL_OP_NO_TLSv1_2; +#else + return 0; +#endif + } +#endif + + SSL_CTX_set_options(ctx, ssl_options); + + return 1; /* success */ +} + +static int +SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version) +{ + int ssl_options = 0; + + AssertArg(version != 0); + + /* + * Some OpenSSL versions define TLS*_VERSION macros but not the + * corresponding SSL_OP_NO_* macro, so in those cases we have to return + * unsuccessfully here. + */ +#ifdef TLS1_1_VERSION + if (version < TLS1_1_VERSION) + { +#ifdef SSL_OP_NO_TLSv1_1 + ssl_options |= SSL_OP_NO_TLSv1_1; +#else + return 0; +#endif + } +#endif +#ifdef TLS1_2_VERSION + if (version < TLS1_2_VERSION) + { +#ifdef SSL_OP_NO_TLSv1_2 + ssl_options |= SSL_OP_NO_TLSv1_2; +#else + return 0; +#endif + } +#endif + + SSL_CTX_set_options(ctx, ssl_options); + + return 1; /* success */ +} + +#endif /* !SSL_CTX_set_min_proto_version */ diff --git a/src/include/common/openssl.h b/src/include/common/openssl.h new file mode 100644 index 0000000000..b1aee24ee8 --- /dev/null +++ b/src/include/common/openssl.h @@ -0,0 +1,27 @@ +/*------------------------------------------------------------------------- + * + * openssl.h + * OpenSSL supporting functionality shared between frontend and backend + * + * Portions Copyright (c) 2018-2020, PostgreSQL Global Development Group + * + * IDENTIFICATION + * src/include/common/openssl.h + * + *------------------------------------------------------------------------- + */ +#ifndef _OPENSSL_H_ +#define _OPENSSL_H_ + +#ifdef USE_OPENSSL +#include <openssl/ssl.h> + +/* src/common/openssl_protocol.c */ +#ifndef SSL_CTX_set_min_proto_version +static int SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version); +static int SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version); +#endif + +#endif + +#endif /* _OPENSSL_H_ */ diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 89b134665b..91d7ced2fd 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", "", /* sizeof("tlsv1.x") */ 7, + offsetof(struct pg_conn, sslminprotocolversion)}, + + {"sslmaxprotocolversion", "PGSSLMAXPROTOCOLVERSION", NULL, NULL, + "SSL-Maximum-Protocol-Version", "", /* sizeof("tlvs1.x") */ 7, + offsetof(struct pg_conn, sslmaxprotocolversion)}, + /* * As with SSL, all GSS options are exposed even in builds that don't have * support. diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 0e84fc8ac6..f20d8fa287 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,52 @@ 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) + { + ssl_min_ver = ssl_protocol_version_to_openssl(conn->sslminprotocolversion); + + if (ssl_min_ver == -1) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid minimum protocol version specified: %s\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("unable to set minimum protocol version specified: %s\n"), + err); + return -1; + } + } + + if (conn->sslmaxprotocolversion) + { + ssl_max_ver = ssl_protocol_version_to_openssl(conn->sslmaxprotocolversion); + + if (ssl_max_ver == -1) + { + printfPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid or unsupported maximum protocol version specified: %s\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("unable to set maximum SSL version specified: %s\n"), + err); + return -1; + } + } + /* * Disable OpenSSL's moving-write-buffer sanity check, because it causes * unnecessary failures in nonblocking send cases. @@ -1659,3 +1709,34 @@ PQssl_passwd_cb(char *buf, int size, int rwflag, void *userdata) else return PQdefaultSSLKeyPassHook(buf, size, conn); } + +/* + * Convert TLS protocol versionstring to OpenSSL values + * + * If a version is passed that is not supported by the current OpenSSL version, + * then we return -1. If a nonnegative value is returned, subsequent code can + * assume it's working with a supported version. + */ +static int +ssl_protocol_version_to_openssl(const char *protocol) +{ + if ((pg_strcasecmp("tlsv1", protocol) == 0) || pg_strcasecmp("tlsv1.0", 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 1e36be20bf..97a4d7246d 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 83fcd5e839..e7726bccfe 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 => 87; } else { @@ -338,6 +338,18 @@ command_like( ^\d+,t,TLSv[\d.]+,[\w-]+,\d+,f,_null_,_null_,_null_\r?$}mx, 'pg_stat_ssl view without client certificate'); +# Test min/mix protocol versions +test_connect_ok( + $common_connstr, + "sslrootcert=ssl/root+server_ca.crt sslmode=require sslminprotocolversion=tlsv1.2 sslmaxprotocolversion=tlsv1.3", + "connect with correct range of allowed TLS protocol versions"); + +test_connect_fails( + $common_connstr, + "sslrootcert=ssl/root+server_ca.crt sslmode=require sslminprotocolversion=tlsv1.3 sslmaxprotocolversion=tlsv1.2", + qr/SSL error/, + "connect with an incorrect range of TLS protocol versions leaving no versions allowed"); + ### Server-side tests. ### ### Test certificate authorization. -- 2.21.0 (Apple Git-122.2)