Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2c0cefc

Browse files
committed
Set libcrypto callbacks for all connection threads in libpq
Based on an analysis of the OpenSSL code with Jacob, moving to EVP forthe cryptohash computations makes necessary the setup of the libcryptocallbacks that were getting set only for SSL connections, but not forconnections without SSL. Not setting the callbacks makes the use ofthreads potentially unsafe for connections calling cryptohashes duringauthentication, like MD5 or SCRAM, if a failure happens during acryptohash computation. The logic setting the libssl and libcryptostates is then split into two parts, both using the same locking, withlibcrypto being set up for SSL and non-SSL connections, while SSLconnections set any libssl state afterwards as needed.Prior to this commit, only SSL connections would have set libcryptocallbacks that are necessary to ensure a proper thread locking whenusing multiple concurrent threads in libpq (ENABLE_THREAD_SAFETY). Notethat this is only required for OpenSSL 1.0.2 and 1.0.1 (oldest versionsupported on HEAD), as 1.1.0 has its own internal locking and it hasdropped support for CRYPTO_set_locking_callback().Tests with up to 300 threads with OpenSSL 1.0.1 and 1.0.2, mixing SSLand non-SSL connection threads did not show any performance impact aftersome micro-benchmarking. pgbench can be used here with -C and amostly-empty script (with one \set meta-command for example) to stressauthentication requests, and we have mixed that with some customprograms for testing.Reported-by: Jacob ChampionAuthor: Michael PaquierReviewed-by: Jacob ChampionDiscussion:https://postgr.es/m/fd3ba610085f1ff54623478cf2f7adf5af193cbb.camel@vmware.com
1 parent3f0daeb commit2c0cefc

File tree

4 files changed

+96
-51
lines changed

4 files changed

+96
-51
lines changed

‎src/interfaces/libpq/fe-connect.c

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2887,6 +2887,16 @@ PQconnectPoll(PGconn *conn)
28872887

28882888
#ifdefUSE_SSL
28892889

2890+
/*
2891+
* Enable the libcrypto callbacks before checking if SSL needs
2892+
* to be done. This is done before sending the startup packet
2893+
* as depending on the type of authentication done, like MD5
2894+
* or SCRAM that use cryptohashes, the callbacks would be
2895+
* required even without a SSL connection
2896+
*/
2897+
if (pqsecure_initialize(conn, false, true)<0)
2898+
gotoerror_return;
2899+
28902900
/*
28912901
* If SSL is enabled and we haven't already got encryption of
28922902
* some sort running, request SSL instead of sending the
@@ -2998,8 +3008,14 @@ PQconnectPoll(PGconn *conn)
29983008
{
29993009
/* mark byte consumed */
30003010
conn->inStart=conn->inCursor;
3001-
/* Set up global SSL state if required */
3002-
if (pqsecure_initialize(conn)!=0)
3011+
3012+
/*
3013+
* Set up global SSL state if required. The crypto
3014+
* state has already been set if libpq took care of
3015+
* doing that, so there is no need to make that happen
3016+
* again.
3017+
*/
3018+
if (pqsecure_initialize(conn, true, false)!=0)
30033019
gotoerror_return;
30043020
}
30053021
elseif (SSLok=='N')

‎src/interfaces/libpq/fe-secure-openssl.c

Lines changed: 65 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ static bool pq_init_crypto_lib = true;
8585
staticboolssl_lib_initialized= false;
8686

8787
#ifdefENABLE_THREAD_SAFETY
88-
staticlongssl_open_connections=0;
88+
staticlongcrypto_open_connections=0;
8989

9090
#ifndefWIN32
9191
staticpthread_mutex_tssl_config_mutex=PTHREAD_MUTEX_INITIALIZER;
@@ -111,7 +111,7 @@ pgtls_init_library(bool do_ssl, int do_crypto)
111111
* Disallow changing the flags while we have open connections, else we'd
112112
* get completely confused.
113113
*/
114-
if (ssl_open_connections!=0)
114+
if (crypto_open_connections!=0)
115115
return;
116116
#endif
117117

@@ -635,7 +635,7 @@ pq_lockingcallback(int mode, int n, const char *file, int line)
635635
* override it.
636636
*/
637637
int
638-
pgtls_init(PGconn*conn)
638+
pgtls_init(PGconn*conn,booldo_ssl,booldo_crypto)
639639
{
640640
#ifdefENABLE_THREAD_SAFETY
641641
#ifdefWIN32
@@ -684,22 +684,28 @@ pgtls_init(PGconn *conn)
684684
}
685685
}
686686

687-
if (ssl_open_connections++==0)
687+
if (do_crypto&& !conn->crypto_loaded)
688688
{
689-
/*
690-
* These are only required for threaded libcrypto applications,
691-
* but make sure we don't stomp on them if they're already set.
692-
*/
693-
if (CRYPTO_get_id_callback()==NULL)
694-
CRYPTO_set_id_callback(pq_threadidcallback);
695-
if (CRYPTO_get_locking_callback()==NULL)
696-
CRYPTO_set_locking_callback(pq_lockingcallback);
689+
if (crypto_open_connections++==0)
690+
{
691+
/*
692+
* These are only required for threaded libcrypto
693+
* applications, but make sure we don't stomp on them if
694+
* they're already set.
695+
*/
696+
if (CRYPTO_get_id_callback()==NULL)
697+
CRYPTO_set_id_callback(pq_threadidcallback);
698+
if (CRYPTO_get_locking_callback()==NULL)
699+
CRYPTO_set_locking_callback(pq_lockingcallback);
700+
}
701+
702+
conn->crypto_loaded= true;
697703
}
698704
}
699705
#endif/* HAVE_CRYPTO_LOCK */
700706
#endif/* ENABLE_THREAD_SAFETY */
701707

702-
if (!ssl_lib_initialized)
708+
if (!ssl_lib_initialized&&do_ssl)
703709
{
704710
if (pq_init_ssl_lib)
705711
{
@@ -740,10 +746,10 @@ destroy_ssl_system(void)
740746
if (pthread_mutex_lock(&ssl_config_mutex))
741747
return;
742748

743-
if (pq_init_crypto_lib&&ssl_open_connections>0)
744-
--ssl_open_connections;
749+
if (pq_init_crypto_lib&&crypto_open_connections>0)
750+
--crypto_open_connections;
745751

746-
if (pq_init_crypto_lib&&ssl_open_connections==0)
752+
if (pq_init_crypto_lib&&crypto_open_connections==0)
747753
{
748754
/*
749755
* No connections left, unregister libcrypto callbacks, if no one
@@ -1402,46 +1408,63 @@ pgtls_close(PGconn *conn)
14021408
{
14031409
booldestroy_needed= false;
14041410

1405-
if (conn->ssl)
1411+
if (conn->ssl_in_use)
14061412
{
1407-
/*
1408-
* We can't destroy everything SSL-related here due to the possible
1409-
* later calls to OpenSSL routines which may need our thread
1410-
* callbacks, so set a flag here and check at the end.
1411-
*/
1412-
destroy_needed= true;
1413+
if (conn->ssl)
1414+
{
1415+
/*
1416+
* We can't destroy everything SSL-related here due to the
1417+
* possible later calls to OpenSSL routines which may need our
1418+
* thread callbacks, so set a flag here and check at the end.
1419+
*/
14131420

1414-
SSL_shutdown(conn->ssl);
1415-
SSL_free(conn->ssl);
1416-
conn->ssl=NULL;
1417-
conn->ssl_in_use= false;
1418-
}
1421+
SSL_shutdown(conn->ssl);
1422+
SSL_free(conn->ssl);
1423+
conn->ssl=NULL;
1424+
conn->ssl_in_use= false;
14191425

1420-
if (conn->peer)
1421-
{
1422-
X509_free(conn->peer);
1423-
conn->peer=NULL;
1424-
}
1426+
destroy_needed= true;
1427+
}
1428+
1429+
if (conn->peer)
1430+
{
1431+
X509_free(conn->peer);
1432+
conn->peer=NULL;
1433+
}
14251434

14261435
#ifdefUSE_SSL_ENGINE
1427-
if (conn->engine)
1436+
if (conn->engine)
1437+
{
1438+
ENGINE_finish(conn->engine);
1439+
ENGINE_free(conn->engine);
1440+
conn->engine=NULL;
1441+
}
1442+
#endif
1443+
}
1444+
else
14281445
{
1429-
ENGINE_finish(conn->engine);
1430-
ENGINE_free(conn->engine);
1431-
conn->engine=NULL;
1446+
/*
1447+
* In the non-SSL case, just remove the crypto callbacks if the
1448+
* connection has then loaded. This code path has no dependency
1449+
* on any pending SSL calls.
1450+
*/
1451+
if (conn->crypto_loaded)
1452+
destroy_needed= true;
14321453
}
1433-
#endif
14341454

14351455
/*
1436-
* This will remove ourSSL locking hooks, if this is the last SSL
1437-
* connection,which means we must wait to call it until after all SSL
1438-
* calls have been made, otherwise we can end up with a race condition and
1439-
* possible deadlocks.
1456+
* This will remove ourcrypto locking hooks if this is the last
1457+
* connection using libcryptowhich means we must wait to call it until
1458+
*after all the potential SSLcalls have been made, otherwise we can end
1459+
*up with a race condition andpossible deadlocks.
14401460
*
14411461
* See comments above destroy_ssl_system().
14421462
*/
14431463
if (destroy_needed)
1464+
{
14441465
destroy_ssl_system();
1466+
conn->crypto_loaded= false;
1467+
}
14451468
}
14461469

14471470

‎src/interfaces/libpq/fe-secure.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,12 +159,12 @@ PQinitOpenSSL(int do_ssl, int do_crypto)
159159
*Initialize global SSL context
160160
*/
161161
int
162-
pqsecure_initialize(PGconn*conn)
162+
pqsecure_initialize(PGconn*conn,booldo_ssl,booldo_crypto)
163163
{
164164
intr=0;
165165

166166
#ifdefUSE_SSL
167-
r=pgtls_init(conn);
167+
r=pgtls_init(conn,do_ssl,do_crypto);
168168
#endif
169169

170170
returnr;
@@ -191,8 +191,7 @@ void
191191
pqsecure_close(PGconn*conn)
192192
{
193193
#ifdefUSE_SSL
194-
if (conn->ssl_in_use)
195-
pgtls_close(conn);
194+
pgtls_close(conn);
196195
#endif
197196
}
198197

‎src/interfaces/libpq/libpq-int.h

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -486,6 +486,11 @@ struct pg_conn
486486
void*engine;/* dummy field to keep struct the same if
487487
* OpenSSL version changes */
488488
#endif
489+
boolcrypto_loaded;/* Track if libcrypto locking callbacks have
490+
* been done for this connection. This can be
491+
* removed once support for OpenSSL 1.0.2 is
492+
* removed as this locking is handled
493+
* internally in OpenSSL >= 1.1.0. */
489494
#endif/* USE_OPENSSL */
490495
#endif/* USE_SSL */
491496

@@ -667,7 +672,7 @@ extern intpqWriteReady(PGconn *conn);
667672

668673
/* === in fe-secure.c === */
669674

670-
externintpqsecure_initialize(PGconn*);
675+
externintpqsecure_initialize(PGconn*,bool,bool);
671676
externPostgresPollingStatusTypepqsecure_open_client(PGconn*);
672677
externvoidpqsecure_close(PGconn*);
673678
externssize_tpqsecure_read(PGconn*,void*ptr,size_tlen);
@@ -696,11 +701,13 @@ extern void pgtls_init_library(bool do_ssl, int do_crypto);
696701
* Initialize SSL library.
697702
*
698703
* The conn parameter is only used to be able to pass back an error
699-
* message - no connection-local setup is made here.
704+
* message - no connection-local setup is made here. do_ssl controls
705+
* if SSL is initialized, and do_crypto does the same for the crypto
706+
* part.
700707
*
701708
* Returns 0 if OK, -1 on failure (adding a message to conn->errorMessage).
702709
*/
703-
externintpgtls_init(PGconn*conn);
710+
externintpgtls_init(PGconn*conn,booldo_ssl,booldo_crypto);
704711

705712
/*
706713
*Begin or continue negotiating a secure session.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp