Re: should libpq also require TLSv1.2 by default?

I wrote:
> Anybdy have a better idea?  Is there a reasonably direct way to ask
> OpenSSL what its min and max versions are?

After some digging, there apparently is not.  At first glance it would
seem that SSL_get_min_proto_version/SSL_get_max_proto_version should
help, but in reality they're just blindingly useless, because they
return zero in most cases of interest.  And when they don't return zero
they might give us a code that we don't recognize, so there's no future
proofing to be had from using them.  Plus they don't exist before
openssl 1.1.1.

It looks like, when they exist, we could use them to discover any
restrictions openssl.cnf has set on the allowed protocol versions ...
but I'm not really convinced that's worth the trouble.  If we up the
libpq default to TLSv1.2 then there probably won't be any real-world
cases where openssl.cnf affects our results.

So I propose the attached.  The hack in openssl.h to guess the
min/max supported versions is certainly nothing but a hack;
but I see no way to do better.

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8adf64c78e..778b166753 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -72,6 +72,7 @@ static bool dummy_ssl_passwd_cb_called = false;
 static bool ssl_is_server_start;

 static int    ssl_protocol_version_to_openssl(int v);
+static const char *ssl_protocol_version_to_string(int v);

 /* ------------------------------------------------------------ */
 /*                         Public interface                        */
@@ -365,6 +366,7 @@ be_tls_open_server(Port *port)
     int            err;
     int            waitfor;
     unsigned long ecode;
+    bool        give_proto_hint;

     Assert(!port->ssl);
     Assert(!port->peer);
@@ -451,10 +453,48 @@ aloop:
                              errmsg("could not accept SSL connection: EOF detected")));
                 break;
             case SSL_ERROR_SSL:
+                switch (ERR_GET_REASON(ecode))
+                {
+                        /*
+                         * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                         * server min/max protocol version limits don't
+                         * overlap.  UNSUPPORTED_PROTOCOL,
+                         * WRONG_VERSION_NUMBER, and
+                         * TLSV1_ALERT_PROTOCOL_VERSION have been observed
+                         * when trying to communicate with an old OpenSSL
+                         * library.  It's not very clear what would make
+                         * OpenSSL return the other codes listed here, but a
+                         * hint about protocol versions seems like it's
+                         * appropriate for all.
+                         */
+                    case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                    case SSL_R_UNSUPPORTED_PROTOCOL:
+                    case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                    case SSL_R_UNKNOWN_SSL_VERSION:
+                    case SSL_R_UNSUPPORTED_SSL_VERSION:
+                    case SSL_R_VERSION_TOO_HIGH:
+                    case SSL_R_VERSION_TOO_LOW:
+                    case SSL_R_WRONG_SSL_VERSION:
+                    case SSL_R_WRONG_VERSION_NUMBER:
+                    case SSL_R_TLSV1_ALERT_PROTOCOL_VERSION:
+                        give_proto_hint = true;
+                        break;
+                    default:
+                        give_proto_hint = false;
+                        break;
+                }
                 ereport(COMMERROR,
                         (errcode(ERRCODE_PROTOCOL_VIOLATION),
                          errmsg("could not accept SSL connection: %s",
-                                SSLerrmessage(ecode))));
+                                SSLerrmessage(ecode)),
+                         give_proto_hint ?
+                         errhint("This may indicate that the client does not support any SSL protocol version between
%sand %s.", 
+                                 ssl_min_protocol_version ?
+                                 ssl_protocol_version_to_string(ssl_min_protocol_version) :
+                                 MIN_OPENSSL_TLS_VERSION,
+                                 ssl_max_protocol_version ?
+                                 ssl_protocol_version_to_string(ssl_max_protocol_version) :
+                                 MAX_OPENSSL_TLS_VERSION) : 0));
                 break;
             case SSL_ERROR_ZERO_RETURN:
                 ereport(COMMERROR,
@@ -1328,6 +1368,29 @@ ssl_protocol_version_to_openssl(int v)
     return -1;
 }

+/*
+ * Likewise provide a mapping to strings.
+ */
+static const char *
+ssl_protocol_version_to_string(int v)
+{
+    switch (v)
+    {
+        case PG_TLS_ANY:
+            return "any";
+        case PG_TLS1_VERSION:
+            return "TLSv1";
+        case PG_TLS1_1_VERSION:
+            return "TLSv1.1";
+        case PG_TLS1_2_VERSION:
+            return "TLSv1.2";
+        case PG_TLS1_3_VERSION:
+            return "TLSv1.3";
+    }
+
+    return "(unrecognized)";
+}
+

 static void
 default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
diff --git a/src/include/common/openssl.h b/src/include/common/openssl.h
index 47fa129994..e4ef53ef70 100644
--- a/src/include/common/openssl.h
+++ b/src/include/common/openssl.h
@@ -17,12 +17,30 @@
 #ifdef USE_OPENSSL
 #include <openssl/ssl.h>

+/*
+ * OpenSSL doesn't provide any very nice way to identify the min/max
+ * protocol versions the library supports, so we fake it as best we can.
+ * Note in particular that this doesn't account for restrictions that
+ * might be specified in the installation's openssl.cnf.
+ */
+#define MIN_OPENSSL_TLS_VERSION  "TLSv1"
+
+#if defined(TLS1_3_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.3"
+#elif defined(TLS1_2_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.2"
+#elif defined(TLS1_1_VERSION)
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1.1"
+#else
+#define MAX_OPENSSL_TLS_VERSION  "TLSv1"
+#endif
+
 /* src/common/protocol_openssl.c */
 #ifndef SSL_CTX_set_min_proto_version
 extern int    SSL_CTX_set_min_proto_version(SSL_CTX *ctx, int version);
 extern int    SSL_CTX_set_max_proto_version(SSL_CTX *ctx, int version);
 #endif

-#endif
+#endif                            /* USE_OPENSSL */

 #endif                            /* COMMON_OPENSSL_H */
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 2d813ef5f9..10fa09020d 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1304,6 +1304,42 @@ open_client_SSL(PGconn *conn)
                                       libpq_gettext("SSL error: %s\n"),
                                       err);
                     SSLerrfree(err);
+                    switch (ERR_GET_REASON(ecode))
+                    {
+                            /*
+                             * NO_PROTOCOLS_AVAILABLE occurs if the client and
+                             * server min/max protocol version limits don't
+                             * overlap.  UNSUPPORTED_PROTOCOL,
+                             * WRONG_VERSION_NUMBER, and
+                             * TLSV1_ALERT_PROTOCOL_VERSION have been observed
+                             * when trying to communicate with an old OpenSSL
+                             * library.  It's not very clear what would make
+                             * OpenSSL return the other codes listed here, but
+                             * a hint about protocol versions seems like it's
+                             * appropriate for all.
+                             */
+                        case SSL_R_NO_PROTOCOLS_AVAILABLE:
+                        case SSL_R_UNSUPPORTED_PROTOCOL:
+                        case SSL_R_BAD_PROTOCOL_VERSION_NUMBER:
+                        case SSL_R_UNKNOWN_SSL_VERSION:
+                        case SSL_R_UNSUPPORTED_SSL_VERSION:
+                        case SSL_R_VERSION_TOO_HIGH:
+                        case SSL_R_VERSION_TOO_LOW:
+                        case SSL_R_WRONG_SSL_VERSION:
+                        case SSL_R_WRONG_VERSION_NUMBER:
+                        case SSL_R_TLSV1_ALERT_PROTOCOL_VERSION:
+                            appendPQExpBuffer(&conn->errorMessage,
+                                              libpq_gettext("This may indicate that the server does not support any
SSLprotocol version between %s and %s.\n"), 
+                                              conn->ssl_min_protocol_version ?
+                                              conn->ssl_min_protocol_version :
+                                              MIN_OPENSSL_TLS_VERSION,
+                                              conn->ssl_max_protocol_version ?
+                                              conn->ssl_max_protocol_version :
+                                              MAX_OPENSSL_TLS_VERSION);
+                            break;
+                        default:
+                            break;
+                    }
                     pgtls_close(conn);
                     return PGRES_POLLING_FAILED;
                 }
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index dfc292872a..ea1909c08d 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1745,9 +1745,9 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
         <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.
+        not supporting the most modern protocol versions. If not specified,
+        the default is <literal>TLSv1.2</literal>, which satisfies industry
+        best practices as of this writing.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 2c87b34028..27c9bb46ee 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -320,7 +320,7 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
         "Require-Peer", "", 10,
     offsetof(struct pg_conn, requirepeer)},

-    {"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", NULL, NULL,
+    {"ssl_min_protocol_version", "PGSSLMINPROTOCOLVERSION", "TLSv1.2", NULL,
         "SSL-Minimum-Protocol-Version", "", 8,    /* sizeof("TLSv1.x") == 8 */
     offsetof(struct pg_conn, ssl_min_protocol_version)},