Set libcrypto callbacks for all connection threads in libpq

Based on an analysis of the OpenSSL code with Jacob, moving to EVP for
the cryptohash computations makes necessary the setup of the libcrypto
callbacks that were getting set only for SSL connections, but not for
connections without SSL.  Not setting the callbacks makes the use of
threads potentially unsafe for connections calling cryptohashes during
authentication, like MD5 or SCRAM, if a failure happens during a
cryptohash computation.  The logic setting the libssl and libcrypto
states is then split into two parts, both using the same locking, with
libcrypto being set up for SSL and non-SSL connections, while SSL
connections set any libssl state afterwards as needed.

Prior to this commit, only SSL connections would have set libcrypto
callbacks that are necessary to ensure a proper thread locking when
using multiple concurrent threads in libpq (ENABLE_THREAD_SAFETY).  Note
that this is only required for OpenSSL 1.0.2 and 1.0.1 (oldest version
supported on HEAD), as 1.1.0 has its own internal locking and it has
dropped support for CRYPTO_set_locking_callback().

Tests with up to 300 threads with OpenSSL 1.0.1 and 1.0.2, mixing SSL
and non-SSL connection threads did not show any performance impact after
some micro-benchmarking.  pgbench can be used here with -C and a
mostly-empty script (with one \set meta-command for example) to stress
authentication requests, and we have mixed that with some custom
programs for testing.

Reported-by: Jacob Champion
Author: Michael Paquier
Reviewed-by: Jacob Champion
Discussion: https://postgr.es/m/fd3ba610085f1ff54623478cf2f7adf5af193cbb.camel@vmware.com

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 29054bad7b4bb5ffd9327500d05b8e5cd56d8661..4e21057d0f9b3d5308985de36d5baf0329d7938d 100644 (file)
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2887,6 +2887,16 @@ keep_going:                      /* We will come back to here until there is
 
 #ifdef USE_SSL
 
+               /*
+                * Enable the libcrypto callbacks before checking if SSL needs
+                * to be done.  This is done before sending the startup packet
+                * as depending on the type of authentication done, like MD5
+                * or SCRAM that use cryptohashes, the callbacks would be
+                * required even without a SSL connection
+                */
+               if (pqsecure_initialize(conn, false, true) < 0)
+                   goto error_return;
+
                /*
                 * If SSL is enabled and we haven't already got encryption of
                 * some sort running, request SSL instead of sending the
@@ -2998,8 +3008,14 @@ keep_going:                      /* We will come back to here until there is
                    {
                        /* mark byte consumed */
                        conn->inStart = conn->inCursor;
-                       /* Set up global SSL state if required */
-                       if (pqsecure_initialize(conn) != 0)
+
+                       /*
+                        * Set up global SSL state if required.  The crypto
+                        * state has already been set if libpq took care of
+                        * doing that, so there is no need to make that happen
+                        * again.
+                        */
+                       if (pqsecure_initialize(conn, true, false) != 0)
                            goto error_return;
                    }
                    else if (SSLok == 'N')

diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0fa10a23b4a5d652913c51ec3fed32c34a988e1e..9c2222c1d15916b39086557732bc08337698f22a 100644 (file)
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -85,7 +85,7 @@ static bool pq_init_crypto_lib = true;
 static bool ssl_lib_initialized = false;
 
 #ifdef ENABLE_THREAD_SAFETY
-static long ssl_open_connections = 0;
+static long crypto_open_connections = 0;
 
 #ifndef WIN32
 static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -111,7 +111,7 @@ pgtls_init_library(bool do_ssl, int do_crypto)
     * Disallow changing the flags while we have open connections, else we'd
     * get completely confused.
     */
-   if (ssl_open_connections != 0)
+   if (crypto_open_connections != 0)
        return;
 #endif
 
@@ -635,7 +635,7 @@ pq_lockingcallback(int mode, int n, const char *file, int line)
  * override it.
  */
 int
-pgtls_init(PGconn *conn)
+pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
 {
 #ifdef ENABLE_THREAD_SAFETY
 #ifdef WIN32
@@ -684,22 +684,28 @@ pgtls_init(PGconn *conn)
            }
        }
 
-       if (ssl_open_connections++ == 0)
+       if (do_crypto && !conn->crypto_loaded)
        {
-           /*
-            * These are only required for threaded libcrypto applications,
-            * but make sure we don't stomp on them if they're already set.
-            */
-           if (CRYPTO_get_id_callback() == NULL)
-               CRYPTO_set_id_callback(pq_threadidcallback);
-           if (CRYPTO_get_locking_callback() == NULL)
-               CRYPTO_set_locking_callback(pq_lockingcallback);
+           if (crypto_open_connections++ == 0)
+           {
+               /*
+                * These are only required for threaded libcrypto
+                * applications, but make sure we don't stomp on them if
+                * they're already set.
+                */
+               if (CRYPTO_get_id_callback() == NULL)
+                   CRYPTO_set_id_callback(pq_threadidcallback);
+               if (CRYPTO_get_locking_callback() == NULL)
+                   CRYPTO_set_locking_callback(pq_lockingcallback);
+           }
+
+           conn->crypto_loaded = true;
        }
    }
 #endif                         /* HAVE_CRYPTO_LOCK */
 #endif                         /* ENABLE_THREAD_SAFETY */
 
-   if (!ssl_lib_initialized)
+   if (!ssl_lib_initialized && do_ssl)
    {
        if (pq_init_ssl_lib)
        {
@@ -740,10 +746,10 @@ destroy_ssl_system(void)
    if (pthread_mutex_lock(&ssl_config_mutex))
        return;
 
-   if (pq_init_crypto_lib && ssl_open_connections > 0)
-       --ssl_open_connections;
+   if (pq_init_crypto_lib && crypto_open_connections > 0)
+       --crypto_open_connections;
 
-   if (pq_init_crypto_lib && ssl_open_connections == 0)
+   if (pq_init_crypto_lib && crypto_open_connections == 0)
    {
        /*
         * No connections left, unregister libcrypto callbacks, if no one
@@ -1402,46 +1408,63 @@ pgtls_close(PGconn *conn)
 {
    bool        destroy_needed = false;
 
-   if (conn->ssl)
+   if (conn->ssl_in_use)
    {
-       /*
-        * We can't destroy everything SSL-related here due to the possible
-        * later calls to OpenSSL routines which may need our thread
-        * callbacks, so set a flag here and check at the end.
-        */
-       destroy_needed = true;
+       if (conn->ssl)
+       {
+           /*
+            * We can't destroy everything SSL-related here due to the
+            * possible later calls to OpenSSL routines which may need our
+            * thread callbacks, so set a flag here and check at the end.
+            */
 
-       SSL_shutdown(conn->ssl);
-       SSL_free(conn->ssl);
-       conn->ssl = NULL;
-       conn->ssl_in_use = false;
-   }
+           SSL_shutdown(conn->ssl);
+           SSL_free(conn->ssl);
+           conn->ssl = NULL;
+           conn->ssl_in_use = false;
 
-   if (conn->peer)
-   {
-       X509_free(conn->peer);
-       conn->peer = NULL;
-   }
+           destroy_needed = true;
+       }
+
+       if (conn->peer)
+       {
+           X509_free(conn->peer);
+           conn->peer = NULL;
+       }
 
 #ifdef USE_SSL_ENGINE
-   if (conn->engine)
+       if (conn->engine)
+       {
+           ENGINE_finish(conn->engine);
+           ENGINE_free(conn->engine);
+           conn->engine = NULL;
+       }
+#endif
+   }
+   else
    {
-       ENGINE_finish(conn->engine);
-       ENGINE_free(conn->engine);
-       conn->engine = NULL;
+       /*
+        * In the non-SSL case, just remove the crypto callbacks if the
+        * connection has then loaded.  This code path has no dependency
+        * on any pending SSL calls.
+        */
+       if (conn->crypto_loaded)
+           destroy_needed = true;
    }
-#endif
 
    /*
-    * This will remove our SSL locking hooks, if this is the last SSL
-    * connection, which means we must wait to call it until after all SSL
-    * calls have been made, otherwise we can end up with a race condition and
-    * possible deadlocks.
+    * This will remove our crypto locking hooks if this is the last
+    * connection using libcrypto which means we must wait to call it until
+    * after all the potential SSL calls have been made, otherwise we can end
+    * up with a race condition and possible deadlocks.
     *
     * See comments above destroy_ssl_system().
     */
    if (destroy_needed)
+   {
        destroy_ssl_system();
+       conn->crypto_loaded = false;
+   }
 }
 
 

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index c601071838141112965fdbb1e5b6f94a2baff967..b15d8d137cecdc792d3c213da33a4e870ede9117 100644 (file)
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -159,12 +159,12 @@ PQinitOpenSSL(int do_ssl, int do_crypto)
  * Initialize global SSL context
  */
 int
-pqsecure_initialize(PGconn *conn)
+pqsecure_initialize(PGconn *conn, bool do_ssl, bool do_crypto)
 {
    int         r = 0;
 
 #ifdef USE_SSL
-   r = pgtls_init(conn);
+   r = pgtls_init(conn, do_ssl, do_crypto);
 #endif
 
    return r;
@@ -191,8 +191,7 @@ void
 pqsecure_close(PGconn *conn)
 {
 #ifdef USE_SSL
-   if (conn->ssl_in_use)
-       pgtls_close(conn);
+   pgtls_close(conn);
 #endif
 }
 

diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index adf149a76f9cae10e89829f8bd4d2b71d2d8aaa0..2f052f61f82cf5fe15197040413f88b0cd845603 100644 (file)
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -486,6 +486,11 @@ struct pg_conn
    void       *engine;         /* dummy field to keep struct the same if
                                 * OpenSSL version changes */
 #endif
+   bool        crypto_loaded;  /* Track if libcrypto locking callbacks have
+                                * been done for this connection. This can be
+                                * removed once support for OpenSSL 1.0.2 is
+                                * removed as this locking is handled
+                                * internally in OpenSSL >= 1.1.0. */
 #endif                         /* USE_OPENSSL */
 #endif                         /* USE_SSL */
 
@@ -667,7 +672,7 @@ extern int  pqWriteReady(PGconn *conn);
 
 /* === in fe-secure.c === */
 
-extern int pqsecure_initialize(PGconn *);
+extern int pqsecure_initialize(PGconn *, bool, bool);
 extern PostgresPollingStatusType pqsecure_open_client(PGconn *);
 extern void pqsecure_close(PGconn *);
 extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len);
@@ -696,11 +701,13 @@ extern void pgtls_init_library(bool do_ssl, int do_crypto);
  * Initialize SSL library.
  *
  * The conn parameter is only used to be able to pass back an error
- * message - no connection-local setup is made here.
+ * message - no connection-local setup is made here.  do_ssl controls
+ * if SSL is initialized, and do_crypto does the same for the crypto
+ * part.
  *
  * Returns 0 if OK, -1 on failure (adding a message to conn->errorMessage).
  */
-extern int pgtls_init(PGconn *conn);
+extern int pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto);
 
 /*
  * Begin or continue negotiating a secure session.