diff --git a/src/conn.c b/src/conn.c index 826f395c1..0fbaef944 100644 --- a/src/conn.c +++ b/src/conn.c @@ -673,11 +673,21 @@ _makeTLSConn(natsConnection *nc) #if defined(NATS_HAS_TLS) natsStatus s = NATS_OK; SSL *ssl = NULL; + bool useLock = false; // Reset nc->errStr before initiating the handshake... nc->errStr[0] = '\0'; + // We will lock by default, unless the option `tlsConcurrentHandshakes` allows + // us to perform the handshake concurrently. Regardless, we will perform the + // handshake under lock for the very first one for this context. This is because + // otherwise we get thread sanitizer reports (looks like there is some lazy + // initialization happening in the OpenSSL library for the very first handshake). + natsMutex_Lock(nc->opts->sslCtx->lock); + useLock = !nc->opts->tlsConcurrentHandshakes || nc->opts->sslCtx->firstHandshake; + if (!useLock) + natsMutex_Unlock(nc->opts->sslCtx->lock); ssl = SSL_new(nc->opts->sslCtx->ctx); if (ssl == NULL) @@ -783,19 +793,16 @@ _makeTLSConn(natsConnection *nc) if (s == NATS_OK) { nc->errStr[0] = '\0'; + nc->sockCtx.ssl = ssl; + if (useLock) + nc->opts->sslCtx->firstHandshake = false; } - - natsMutex_Unlock(nc->opts->sslCtx->lock); - - if (s != NATS_OK) - { - if (ssl != NULL) - SSL_free(ssl); - } - else + else if (ssl != NULL) { - nc->sockCtx.ssl = ssl; + SSL_free(ssl); } + if (useLock) + natsMutex_Unlock(nc->opts->sslCtx->lock); return NATS_UPDATE_ERR_STACK(s); #else diff --git a/src/nats.h b/src/nats.h index 805ba79bc..fedfe0115 100644 --- a/src/nats.h +++ b/src/nats.h @@ -3037,6 +3037,47 @@ natsOptions_SetSecure(natsOptions *opts, bool secure); NATS_EXTERN natsStatus natsOptions_TLSHandshakeFirst(natsOptions *opts); +/** \brief Allows concurrent TLS handshakes. + * + * When creating a #natsOptions object and configuring it with SSL + * related options, an internal SSL context (`SSL_CTX`) is stored in + * the #natsOptions object. If this #natsOptions object is passed to + * multiple #natsConnection_Connect calls, the connections share this + * same `SSL_CTX` object. + * + * Since those connections can connect (and reconnect) from different + * threads, the SSL handshake process is normally protected by a mutex + * that belongs to the shared #natsOptions object. It was historically + * done this way because a data race is reported (in some cases) if + * not the case, and since there was no warning to users that any + * SSL verification callback that would be set would need to be + * thread safe. + * + * If there is no SSL verification callback (or they are known to be + * thread safe), this option can be used to let the library invoke + * the SSL handshake without the aforementioned mutex. This can improve + * performance since now multiple connections can happen concurrently + * instead of being serialized. + * + * \warning The very first SSL handshake will be done under the protection + * of the mutex, which means other connections (that share the same + * #natsOptions object) will not be able to perform their SSL handshake + * until the very first is complete. This is because of OpenSSL internal + * initialization that is performed during the first handshake. Once that + * is done, other SSL handshakes will be done concurrently. + * + * \note If each connection has its own configured #natsOptions, then there is + * no concurrency issue and the SSL handshakes are already performed concurrently, + * and therefore this option has no effect. Of course, if SSL verification + * callback is passed to different #natsOptions objects, then the callback + * has to be thread safe. Even without this option, the risk of data race + * would have already existed. + * + * @param opts the pointer to the #natsOptions obbject. + */ +NATS_EXTERN natsStatus +natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts); + /** \brief Loads the trusted CA certificates from a file. * * Loads the trusted CA certificates from a file. diff --git a/src/natsp.h b/src/natsp.h index 901f11fc6..5e0f7fdf9 100644 --- a/src/natsp.h +++ b/src/natsp.h @@ -207,6 +207,7 @@ typedef struct __natsSSLCtx char *certFileName; char *keyFileName; natsSSLVerifyCb callback; + bool firstHandshake; } natsSSLCtx; @@ -246,6 +247,7 @@ struct __natsOptions bool allowReconnect; bool secure; bool tlsHandshakeFirst; + bool tlsConcurrentHandshakes; int ioBufSize; int maxReconnect; int64_t reconnectWait; diff --git a/src/opts.c b/src/opts.c index 928c98e0f..da29ae904 100644 --- a/src/opts.c +++ b/src/opts.c @@ -285,6 +285,7 @@ _createSSLCtx(natsSSLCtx **newCtx) SSL_CTX_set_min_proto_version(ctx->ctx, TLS1_2_VERSION); SSL_CTX_set_default_verify_paths(ctx->ctx); + ctx->firstHandshake = true; *newCtx = ctx; } else if (ctx != NULL) @@ -381,6 +382,22 @@ natsOptions_TLSHandshakeFirst(natsOptions *opts) return NATS_UPDATE_ERR_STACK(s); } +natsStatus +natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts) +{ + natsStatus s = NATS_OK; + + LOCK_AND_CHECK_OPTIONS(opts, 0); + + s = _getSSLCtx(opts); + if (s == NATS_OK) + opts->tlsConcurrentHandshakes = true; + + UNLOCK_OPTS(opts); + + return NATS_UPDATE_ERR_STACK(s); +} + natsStatus natsOptions_LoadCATrustedCertificates(natsOptions *opts, const char *fileName) { @@ -818,6 +835,12 @@ natsOptions_TLSHandshakeFirst(natsOptions *opts) return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR); } +natsStatus +natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts) +{ + return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR); +} + natsStatus natsOptions_LoadCATrustedCertificates(natsOptions *opts, const char *fileName) { diff --git a/test/list_test.txt b/test/list_test.txt index 7fddc33a5..a487f3c5f 100644 --- a/test/list_test.txt +++ b/test/list_test.txt @@ -273,6 +273,7 @@ _test(SSLHandshakeFirst) _test(SSLHandshakeTimeout) _test(SSLLoadCAFromMemory) _test(SSLMultithreads) +_test(SSLPerfConcurrentConnect) _test(SSLReconnectWithAuthError) _test(SSLServerNameIndication) _test(SSLSkipServerVerification) diff --git a/test/test.c b/test/test.c index 02ed4acdf..9da639b71 100644 --- a/test/test.c +++ b/test/test.c @@ -21470,7 +21470,7 @@ void test_SSLVerifyDynamic(void) IFOK(s, natsOptions_LoadCertificatesChainDynamic(opts, "certs/cert-dynamic.pem", "certs/key-dynamic.pem")); - s = natsConnection_Connect(&nc, opts); + IFOK(s, natsConnection_Connect(&nc, opts)); IFOK(s, natsConnection_SubscribeSync(&sub, nc, "*")); IFOK(s, natsConnection_PublishString(nc, "foo", "test")); IFOK(s, natsConnection_Flush(nc)); @@ -22554,6 +22554,154 @@ void test_SSLAvailable(void) #endif } +#if defined(NATS_HAS_TLS) +static void +_sslConcurrentConn(void *closure) +{ + struct threadArg *args = (struct threadArg*) closure; + natsStatus s = NATS_OK; + natsConnection *nc = NULL; + + natsMutex_Lock(args->m); + while (!args->connected) + natsCondition_Wait(args->c, args->m); + natsMutex_Unlock(args->m); + + s = natsConnection_Connect(&nc, args->opts); + if (s != NATS_OK) + { + natsMutex_Lock(args->m); + if (args->status == NATS_OK) + { + nats_PrintLastErrorStack(stderr); + args->status = s; + } + natsMutex_Unlock(args->m); + } + natsConnection_Destroy(nc); +} + +#define SSL_CONCURRENT_CONNS 10 + +static int +_sslConcurrentVerifyCb(int preverify_ok, void *pctx) +{ + // Access some fields to make sure that we are not triggering data races. + X509_STORE_CTX *ctx = (X509_STORE_CTX*) pctx; + X509 *cert = X509_STORE_CTX_get_current_cert(ctx); + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + STACK_OF(X509) *chain = SSL_get_peer_cert_chain(ssl); + char *issuerName = NULL; + int result = 0; + + issuerName = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0); + if (strstr(issuerName, "Synadia")) + result = 1; + else + result = preverify_ok; + + if (chain == NULL) + { + // To silence compiler warning... + } + + // Sleep on purpose for 500ms. The test creates SSL_CONCURRENT_CONNS + // connections in parallel. If this callback invoked during the SSL + // handshake had a lock shared by connections (like we used to), the + // test would take more than 0.5*SSL_CONCURRENT_CONNS seconds to complete. + nats_Sleep(500); + + OPENSSL_free(issuerName); + + return result; +} +#endif + +void test_SSLPerfConcurrentConnect(void) +{ +#if defined(NATS_HAS_TLS) + natsStatus s = NATS_OK; + natsOptions *opts = NULL; + natsPid serverPid = NATS_INVALID_PID; + int64_t start = 0; + int64_t dur = 0; + natsThread *t[SSL_CONCURRENT_CONNS]; + int i; + struct threadArg args; + int64_t limit; + + s = _createDefaultThreadArgsForCbTests(&args); + if (s == NATS_OK) + s = natsOptions_Create(&opts); + if (opts == NULL) + FAIL("Unable to setup test!"); + + serverPid = _startServer("nats://127.0.0.1:4443", "-config tls.conf", true); + CHECK_SERVER_STARTED(serverPid); + + test("Create options: "); + s = natsOptions_SetURL(opts, "nats://127.0.0.1:4443"); + IFOK(s, natsOptions_SetSecure(opts, true)); + // For test purposes, we provide the CA trusted certs + IFOK(s, natsOptions_LoadCATrustedCertificates(opts, "certs/ca.pem")); + IFOK(s, natsOptions_SetExpectedHostname(opts, "localhost")); + IFOK(s, natsOptions_AllowConcurrentTLSHandshakes(opts)); + IFOK(s, natsOptions_SetSSLVerificationCallback(opts, _sslConcurrentVerifyCb)); + IFOK(s, natsOptions_SetTimeout(opts, 10000)); + testCond(s == NATS_OK); + + args.opts = opts; + args.status = NATS_OK; + + test("Start threads creating connections: "); + for (i=0; (s == NATS_OK) && (i