From 70dd1aed07dadea062db32d960460ea1cd7cae5f Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Wed, 13 Oct 2021 14:03:45 +0200 Subject: [PATCH] dnsdist: Use per-thread credentials for GnuTLS client connections It looks like there is a race in some versions when the credentials are shared between several threads opening TLS client connections. --- pdns/dnsdistdist/dnsdist-tsan.supp | 1 + pdns/iputils.hh | 1 - pdns/libssl.cc | 12 ++--- pdns/libssl.hh | 6 +-- pdns/tcpiohandler.cc | 85 ++++++++++++++++++++++-------- 5 files changed, 72 insertions(+), 33 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-tsan.supp b/pdns/dnsdistdist/dnsdist-tsan.supp index 39ffe6cd48..c8c2d50e34 100644 --- a/pdns/dnsdistdist/dnsdist-tsan.supp +++ b/pdns/dnsdistdist/dnsdist-tsan.supp @@ -11,3 +11,4 @@ race:DownstreamState::setDown race:DownstreamState::setUp race:DownstreamState::setAuto race:updateHealthCheckResult +race:carbonDumpThread diff --git a/pdns/iputils.hh b/pdns/iputils.hh index 42e7a2bffe..45690731a7 100644 --- a/pdns/iputils.hh +++ b/pdns/iputils.hh @@ -30,7 +30,6 @@ #include #include "pdnsexception.hh" #include "misc.hh" -#include #include #include #include diff --git a/pdns/libssl.cc b/pdns/libssl.cc index 19c6d78a73..2c0e8be682 100644 --- a/pdns/libssl.cc +++ b/pdns/libssl.cc @@ -804,21 +804,21 @@ std::unique_ptr libssl_set_key_log_file(std::unique_ptr& ctx, int (*cb)(SSL* s, unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg) +void libssl_set_npn_select_callback(SSL_CTX* ctx, int (*cb)(SSL* s, unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg) { #ifdef HAVE_SSL_CTX_SET_NEXT_PROTO_SELECT_CB - SSL_CTX_set_next_proto_select_cb(ctx.get(), cb, arg); + SSL_CTX_set_next_proto_select_cb(ctx, cb, arg); #endif } -void libssl_set_alpn_select_callback(std::unique_ptr& ctx, int (*cb)(SSL* s, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg) +void libssl_set_alpn_select_callback(SSL_CTX* ctx, int (*cb)(SSL* s, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg) { #ifdef HAVE_SSL_CTX_SET_ALPN_SELECT_CB - SSL_CTX_set_alpn_select_cb(ctx.get(), cb, arg); + SSL_CTX_set_alpn_select_cb(ctx, cb, arg); #endif } -bool libssl_set_alpn_protos(std::unique_ptr& ctx, const std::vector>& protos) +bool libssl_set_alpn_protos(SSL_CTX* ctx, const std::vector>& protos) { #ifdef HAVE_SSL_CTX_SET_ALPN_PROTOS std::vector wire; @@ -830,7 +830,7 @@ bool libssl_set_alpn_protos(std::unique_ptr& ctx, co wire.push_back(length); wire.insert(wire.end(), proto.begin(), proto.end()); } - return SSL_CTX_set_alpn_protos(ctx.get(), wire.data(), wire.size()) == 0; + return SSL_CTX_set_alpn_protos(ctx, wire.data(), wire.size()) == 0; #else return false; #endif diff --git a/pdns/libssl.hh b/pdns/libssl.hh index 2af0f4ef89..4561b4a496 100644 --- a/pdns/libssl.hh +++ b/pdns/libssl.hh @@ -127,11 +127,11 @@ std::unique_ptr libssl_init_server_context(const TLS std::unique_ptr libssl_set_key_log_file(std::unique_ptr& ctx, const std::string& logFile); /* called in a client context, if the client advertised more than one ALPN values and the server returned more than one as well, to select the one to use. */ -void libssl_set_npn_select_callback(std::unique_ptr& ctx, int (*cb)(SSL* s, unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg); +void libssl_set_npn_select_callback(SSL_CTX* ctx, int (*cb)(SSL* s, unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg); /* called in a server context, to select an ALPN value advertised by the client if any */ -void libssl_set_alpn_select_callback(std::unique_ptr& ctx, int (*cb)(SSL* s, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg); +void libssl_set_alpn_select_callback(SSL_CTX* ctx, int (*cb)(SSL* s, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg), void* arg); /* set the supported ALPN protos in client context */ -bool libssl_set_alpn_protos(std::unique_ptr& ctx, const std::vector>& protos); +bool libssl_set_alpn_protos(SSL_CTX* ctx, const std::vector>& protos); std::string libssl_get_error_string(); diff --git a/pdns/tcpiohandler.cc b/pdns/tcpiohandler.cc index 0cac651ce2..01332bbb71 100644 --- a/pdns/tcpiohandler.cc +++ b/pdns/tcpiohandler.cc @@ -99,7 +99,7 @@ public: } /* client-side connection */ - OpenSSLTLSConnection(const std::string& hostname, int socket, const struct timeval& timeout, SSL_CTX* tlsCtx): d_conn(std::unique_ptr(SSL_new(tlsCtx), SSL_free)), d_hostname(hostname), d_timeout(timeout) + OpenSSLTLSConnection(const std::string& hostname, int socket, const struct timeval& timeout, std::shared_ptr& tlsCtx): d_tlsCtx(tlsCtx), d_conn(std::unique_ptr(SSL_new(tlsCtx.get()), SSL_free)), d_hostname(hostname), d_timeout(timeout) { d_socket = socket; @@ -487,7 +487,10 @@ private: static std::atomic_flag s_initTLSConnIndex; std::vector> d_tlsSessions; + /* server context */ std::shared_ptr d_feContext; + /* client context */ + std::shared_ptr d_tlsCtx; std::unique_ptr d_conn; std::string d_hostname; struct timeval d_timeout; @@ -500,7 +503,7 @@ class OpenSSLTLSIOCtx: public TLSCtx { public: /* server side context */ - OpenSSLTLSIOCtx(TLSFrontend& fe): d_feContext(std::make_shared(fe.d_addr, fe.d_tlsConfig)), d_tlsCtx(std::unique_ptr(nullptr, SSL_CTX_free)) + OpenSSLTLSIOCtx(TLSFrontend& fe): d_feContext(std::make_shared(fe.d_addr, fe.d_tlsConfig)) { d_ticketsKeyRotationDelay = fe.d_tlsConfig.d_ticketsKeyRotationDelay; @@ -535,7 +538,7 @@ public: } /* client side context */ - OpenSSLTLSIOCtx(const TLSContextParameters& params): d_tlsCtx(std::unique_ptr(nullptr, SSL_CTX_free)) + OpenSSLTLSIOCtx(const TLSContextParameters& params) { int sslOptions = SSL_OP_NO_SSLv2 | @@ -549,9 +552,9 @@ public: registerOpenSSLUser(); #ifdef HAVE_TLS_CLIENT_METHOD - d_tlsCtx = std::unique_ptr(SSL_CTX_new(TLS_client_method()), SSL_CTX_free); + d_tlsCtx = std::shared_ptr(SSL_CTX_new(TLS_client_method()), SSL_CTX_free); #else - d_tlsCtx = std::unique_ptr(SSL_CTX_new(SSLv23_client_method()), SSL_CTX_free); + d_tlsCtx = std::shared_ptr(SSL_CTX_new(SSLv23_client_method()), SSL_CTX_free); #endif if (!d_tlsCtx) { ERR_print_errors_fp(stderr); @@ -661,7 +664,7 @@ public: std::unique_ptr getClientConnection(const std::string& host, int socket, const struct timeval& timeout) override { - return std::make_unique(host, socket, timeout, d_tlsCtx.get()); + return std::make_unique(host, socket, timeout, d_tlsCtx); } void rotateTicketsKey(time_t now) override @@ -696,11 +699,11 @@ public: { if (d_feContext && d_feContext->d_tlsCtx) { d_alpnProtos = protos; - libssl_set_alpn_select_callback(d_feContext->d_tlsCtx, alpnServerSelectCallback, this); + libssl_set_alpn_select_callback(d_feContext->d_tlsCtx.get(), alpnServerSelectCallback, this); return true; } if (d_tlsCtx) { - return libssl_set_alpn_protos(d_tlsCtx, protos); + return libssl_set_alpn_protos(d_tlsCtx.get(), protos); } return false; } @@ -708,7 +711,7 @@ public: bool setNextProtocolSelectCallback(bool(*cb)(unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen)) override { d_nextProtocolSelectCallback = cb; - libssl_set_npn_select_callback(d_tlsCtx, npnSelectCallback, this); + libssl_set_npn_select_callback(d_tlsCtx.get(), npnSelectCallback, this); return true; } @@ -757,8 +760,8 @@ private: } std::vector> d_alpnProtos; // store the supported ALPN protocols, so that the server can select based on what the client sent - std::shared_ptr d_feContext; - std::unique_ptr d_tlsCtx; // client context, on a server-side the context is stored in d_feContext->d_tlsCtx + std::shared_ptr d_feContext{nullptr}; + std::shared_ptr d_tlsCtx{nullptr}; // client context, on a server-side the context is stored in d_feContext->d_tlsCtx bool (*d_nextProtocolSelectCallback)(unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen){nullptr}; }; @@ -891,7 +894,7 @@ class GnuTLSConnection: public TLSConnection { public: /* server side connection */ - GnuTLSConnection(int socket, const struct timeval& timeout, const gnutls_certificate_credentials_t creds, const gnutls_priority_t priorityCache, std::shared_ptr& ticketsKey, bool enableTickets): d_ticketsKey(ticketsKey), d_conn(std::unique_ptr(nullptr, gnutls_deinit)) + GnuTLSConnection(int socket, const struct timeval& timeout, std::shared_ptr& creds, const gnutls_priority_t priorityCache, std::shared_ptr& ticketsKey, bool enableTickets): d_creds(creds), d_ticketsKey(ticketsKey), d_conn(std::unique_ptr(nullptr, gnutls_deinit)) { unsigned int sslOptions = GNUTLS_SERVER | GNUTLS_NONBLOCK; #ifdef GNUTLS_NO_SIGNAL @@ -908,7 +911,7 @@ public: d_conn = std::unique_ptr(conn, gnutls_deinit); conn = nullptr; - if (gnutls_credentials_set(d_conn.get(), GNUTLS_CRD_CERTIFICATE, creds) != GNUTLS_E_SUCCESS) { + if (gnutls_credentials_set(d_conn.get(), GNUTLS_CRD_CERTIFICATE, d_creds.get()) != GNUTLS_E_SUCCESS) { throw std::runtime_error("Error setting certificate and key to TLS connection"); } @@ -931,7 +934,7 @@ public: } /* client-side connection */ - GnuTLSConnection(const std::string& host, int socket, const struct timeval& timeout, const gnutls_certificate_credentials_t creds, const gnutls_priority_t priorityCache, bool validateCerts): d_conn(std::unique_ptr(nullptr, gnutls_deinit)), d_host(host), d_client(true) + GnuTLSConnection(const std::string& host, int socket, const struct timeval& timeout, std::shared_ptr& creds, const gnutls_priority_t priorityCache, bool validateCerts): d_creds(creds), d_conn(std::unique_ptr(nullptr, gnutls_deinit)), d_host(host), d_client(true) { unsigned int sslOptions = GNUTLS_CLIENT | GNUTLS_NONBLOCK; #ifdef GNUTLS_NO_SIGNAL @@ -948,7 +951,7 @@ public: d_conn = std::unique_ptr(conn, gnutls_deinit); conn = nullptr; - int rc = gnutls_credentials_set(d_conn.get(), GNUTLS_CRD_CERTIFICATE, creds); + int rc = gnutls_credentials_set(d_conn.get(), GNUTLS_CRD_CERTIFICATE, d_creds.get()); if (rc != GNUTLS_E_SUCCESS) { throw std::runtime_error("Error setting certificate and key to TLS connection: " + std::string(gnutls_strerror(rc))); } @@ -1404,9 +1407,10 @@ public: } private: - std::vector> d_tlsSessions; + std::shared_ptr d_creds; std::shared_ptr d_ticketsKey; std::unique_ptr d_conn; + std::vector> d_tlsSessions; std::string d_host; bool d_client{false}; bool d_handshakeDone{false}; @@ -1416,7 +1420,7 @@ class GnuTLSIOCtx: public TLSCtx { public: /* server side context */ - GnuTLSIOCtx(TLSFrontend& fe): d_creds(std::unique_ptr(nullptr, gnutls_certificate_free_credentials)), d_enableTickets(fe.d_tlsConfig.d_enableTickets) + GnuTLSIOCtx(TLSFrontend& fe): d_enableTickets(fe.d_tlsConfig.d_enableTickets) { int rc = 0; d_ticketsKeyRotationDelay = fe.d_tlsConfig.d_ticketsKeyRotationDelay; @@ -1427,7 +1431,7 @@ public: throw std::runtime_error("Error allocating credentials for TLS context on " + fe.d_addr.toStringWithPort() + ": " + gnutls_strerror(rc)); } - d_creds = std::unique_ptr(creds, gnutls_certificate_free_credentials); + d_creds = std::shared_ptr(creds, gnutls_certificate_free_credentials); creds = nullptr; for (const auto& pair : fe.d_tlsConfig.d_certKeyPairs) { @@ -1472,7 +1476,7 @@ public: } /* client side context */ - GnuTLSIOCtx(const TLSContextParameters& params): d_creds(std::unique_ptr(nullptr, gnutls_certificate_free_credentials)), d_enableTickets(true), d_validateCerts(params.d_validateCertificates) + GnuTLSIOCtx(const TLSContextParameters& params): d_contextParameters(std::make_unique(params)), d_enableTickets(true), d_validateCerts(params.d_validateCertificates) { int rc = 0; @@ -1482,7 +1486,7 @@ public: throw std::runtime_error("Error allocating credentials for TLS context: " + std::string(gnutls_strerror(rc))); } - d_creds = std::unique_ptr(creds, gnutls_certificate_free_credentials); + d_creds = std::shared_ptr(creds, gnutls_certificate_free_credentials); creds = nullptr; if (params.d_validateCertificates) { @@ -1524,16 +1528,49 @@ public: ticketsKey = *(d_ticketsKey.read_lock()); } - auto connection = std::make_unique(socket, timeout, d_creds.get(), d_priorityCache, ticketsKey, d_enableTickets); + auto connection = std::make_unique(socket, timeout, d_creds, d_priorityCache, ticketsKey, d_enableTickets); if (!d_protos.empty()) { connection->setALPNProtos(d_protos); } return connection; } + static std::shared_ptr getPerThreadCredentials(bool validate, const std::string& caStore) + { + static thread_local std::map, std::shared_ptr> t_credentials; + auto& entry = t_credentials[{validate, caStore}]; + if (!entry) { + gnutls_certificate_credentials_t creds; + int rc = gnutls_certificate_allocate_credentials(&creds); + if (rc != GNUTLS_E_SUCCESS) { + throw std::runtime_error("Error allocating credentials for TLS context: " + std::string(gnutls_strerror(rc))); + } + + entry = std::shared_ptr(creds, gnutls_certificate_free_credentials); + creds = nullptr; + + if (validate) { + if (caStore.empty()) { + rc = gnutls_certificate_set_x509_system_trust(entry.get()); + if (rc < 0) { + throw std::runtime_error("Error adding the system's default trusted CAs: " + std::string(gnutls_strerror(rc))); + } + } + else { + rc = gnutls_certificate_set_x509_trust_file(entry.get(), caStore.c_str(), GNUTLS_X509_FMT_PEM); + if (rc < 0) { + throw std::runtime_error("Error adding '" + caStore + "' to the trusted CAs: " + std::string(gnutls_strerror(rc))); + } + } + } + } + return entry; + } + std::unique_ptr getClientConnection(const std::string& host, int socket, const struct timeval& timeout) override { - auto connection = std::make_unique(host, socket, timeout, d_creds.get(), d_priorityCache, d_validateCerts); + auto creds = getPerThreadCredentials(d_contextParameters->d_validateCertificates, d_contextParameters->d_caStore); + auto connection = std::make_unique(host, socket, timeout, creds, d_priorityCache, d_validateCerts); if (!d_protos.empty()) { connection->setALPNProtos(d_protos); } @@ -1594,7 +1631,9 @@ public: } private: - std::unique_ptr d_creds; + /* client context parameters */ + std::unique_ptr d_contextParameters{nullptr}; + std::shared_ptr d_creds; std::vector> d_protos; gnutls_priority_t d_priorityCache{nullptr}; SharedLockGuarded> d_ticketsKey{nullptr}; -- 2.47.2