From 1bbd453bd124dda6533efbe3c26fc16a391126b3 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Mon, 24 May 2021 20:57:45 -0600 Subject: [PATCH 1/2] Adds a new config to parallelize loading of ssl_multicert.config The new setting is (default: 0): CONFIG proxy.config.ssl.server.multicert.concurrency INT 24 A setting of -1 will create one loader thread per system CPU. The CPU used during load will be pretty heavy, loading certs is costly. If enabbled, we will also allow the first load to consume all CPU. --- doc/admin-guide/files/records.config.en.rst | 7 ++ iocore/net/P_SSLConfig.h | 1 + iocore/net/P_SSLUtils.h | 12 ++- iocore/net/SSLConfig.cc | 5 + iocore/net/SSLStats.cc | 1 - iocore/net/SSLUtils.cc | 111 +++++++++++++++----- mgmt/RecordsConfig.cc | 4 +- 7 files changed, 107 insertions(+), 34 deletions(-) diff --git a/doc/admin-guide/files/records.config.en.rst b/doc/admin-guide/files/records.config.en.rst index 6fbbde8f3c0..dc6000a667a 100644 --- a/doc/admin-guide/files/records.config.en.rst +++ b/doc/admin-guide/files/records.config.en.rst @@ -3463,6 +3463,13 @@ SSL Termination :file:`ssl_multicert.config` file successfully load. If false (``0``), SSL certificate load failures will not prevent |TS| from starting. +.. ts:cv:: CONFIG proxy.config.ssl.server.multicert.concurrency INT 0 + + If set to a non-zero value, allow the loading and reloading of + :file:`ssl_multicert.config` to be parallelized. The value specifies the + concurrency, i.e. how many threads will process the file in parallel. A + value of ``-1`` means to create one thread for each core. + .. ts:cv:: CONFIG proxy.config.ssl.server.cert.path STRING /config The location of the SSL certificates and chains used for accepting diff --git a/iocore/net/P_SSLConfig.h b/iocore/net/P_SSLConfig.h index 0ecab2de7dc..bf3a192a969 100644 --- a/iocore/net/P_SSLConfig.h +++ b/iocore/net/P_SSLConfig.h @@ -76,6 +76,7 @@ struct SSLConfigParams : public ConfigInfo { char *cipherSuite; char *client_cipherSuite; int configExitOnLoadError; + int configLoadConcurrency; int clientCertLevel; int verify_depth; int ssl_origin_session_cache; diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h index 1d876946441..c6649aefbde 100644 --- a/iocore/net/P_SSLUtils.h +++ b/iocore/net/P_SSLUtils.h @@ -60,8 +60,8 @@ class SSLMultiCertConfigLoader struct CertLoadData { std::vector cert_names_list, key_list, ca_list, ocsp_list; }; - SSLMultiCertConfigLoader(const SSLConfigParams *p) : _params(p) {} - virtual ~SSLMultiCertConfigLoader(){}; + SSLMultiCertConfigLoader(const SSLConfigParams *p) : _params(p) { ink_mutex_init(&m_mutex); } + virtual ~SSLMultiCertConfigLoader() { ink_mutex_destroy(&m_mutex); }; bool load(SSLCertLookup *lookup); @@ -89,8 +89,12 @@ class SSLMultiCertConfigLoader std::set &names); private: - virtual const char *_debug_tag() const; + using SSLConfigLines = std::vector>; + bool _store_ssl_ctx(SSLCertLookup *lookup, const shared_SSLMultiCertConfigParams &ssl_multi_cert_params); + void _load_lines(SSLCertLookup *lookup, SSLConfigLines::const_iterator begin, SSLConfigLines::const_iterator end); + + virtual const char *_debug_tag() const; virtual void _set_handshake_callbacks(SSL_CTX *ctx); virtual bool _setup_session_cache(SSL_CTX *ctx); virtual bool _setup_dialog(SSL_CTX *ctx, const SSLMultiCertConfigParams *sslMultCertSettings); @@ -103,6 +107,8 @@ class SSLMultiCertConfigLoader virtual bool _set_info_callback(SSL_CTX *ctx); virtual bool _set_npn_callback(SSL_CTX *ctx); virtual bool _set_alpn_callback(SSL_CTX *ctx); + + ink_mutex m_mutex; // Mutex around concurrent loading / reloading of multicert configurations }; // Create a new SSL server context fully configured (cert and keys are optional). diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc index 2af43bcdfb4..c5ef0304d4c 100644 --- a/iocore/net/SSLConfig.cc +++ b/iocore/net/SSLConfig.cc @@ -33,6 +33,7 @@ #include #include +#include #include "tscore/ink_config.h" #include "tscore/ink_platform.h" @@ -307,6 +308,10 @@ SSLConfigParams::initialize() configFilePath = ats_stringdup(RecConfigReadConfigPath("proxy.config.ssl.server.multicert.filename")); REC_ReadConfigInteger(configExitOnLoadError, "proxy.config.ssl.server.multicert.exit_on_load_fail"); + REC_ReadConfigInteger(configLoadConcurrency, "proxy.config.ssl.server.multicert.concurrency"); + if (configLoadConcurrency < 0) { + configLoadConcurrency = std::thread::hardware_concurrency(); + } REC_ReadConfigStringAlloc(ssl_server_private_key_path, "proxy.config.ssl.server.private_key.path"); set_paths_helper(ssl_server_private_key_path, nullptr, &serverKeyPathOnly, nullptr); diff --git a/iocore/net/SSLStats.cc b/iocore/net/SSLStats.cc index aaf3a8f8cc0..79c0f313615 100644 --- a/iocore/net/SSLStats.cc +++ b/iocore/net/SSLStats.cc @@ -257,7 +257,6 @@ SSLInitializeStatistics() RecRegisterRawStat(ssl_rsb, RECT_PROCESS, statName.c_str(), RECD_INT, RECP_NON_PERSISTENT, (int)ssl_cipher_stats_start + index, RecRawStatSyncSum); SSL_CLEAR_DYN_STAT((int)ssl_cipher_stats_start + index); - Debug("ssl", "registering SSL cipher metric '%s'", statName.c_str()); } } diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 4a190d00b9e..c0081b087c9 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -49,6 +49,9 @@ #include #include #include +#include +#include +#include #include #include @@ -1531,7 +1534,7 @@ SSLCreateServerContext(const SSLConfigParams *params, const SSLMultiCertConfigPa } /** - Insert SSLCertContext (SSL_CTX ans options) into SSLCertLookup with key. + Insert SSLCertContext (SSL_CTX and options) into SSLCertLookup with key. Do NOT call SSL_CTX_set_* functions from here. SSL_CTX should be set up by SSLMultiCertConfigLoader::init_server_ssl_ctx(). */ bool @@ -1601,6 +1604,12 @@ SSLMultiCertConfigLoader::_store_single_ssl_ctx(SSLCertLookup *lookup, const sha { bool inserted = false; shared_ssl_ticket_key_block keyblock = nullptr; + + // This may be obsessively large critical section to lock, but there is code written in many places + // which were not intended to be reentrant. The importan / expensive part to avoid locking around is + // the actual loading of the cert, which happens just before this. + ink_mutex_acquire(&m_mutex); + // Load the session ticket key if session tickets are not disabled if (sslMultCertSettings->session_ticket_enabled != 0) { keyblock = shared_ssl_ticket_key_block(ssl_context_enable_tickets(ctx.get(), nullptr), ticket_block_free); @@ -1637,6 +1646,9 @@ SSLMultiCertConfigLoader::_store_single_ssl_ctx(SSLCertLookup *lookup, const sha } } + // Release the mutex now, we should be in the clear. + ink_mutex_release(&m_mutex); + if (inserted) { if (SSLConfigParams::init_ssl_ctx_cb) { SSLConfigParams::init_ssl_ctx_cb(ctx.get(), true); @@ -1721,22 +1733,49 @@ ssl_extract_certificate(const matcher_line *line_info, SSLMultiCertConfigParams return true; } -bool -SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) +// This is a helper function to make it easier to parallelize the configuration parsing. This +// can be called directly over the entire range, or from stl::thread CTOR, with sub-ranges. +void +SSLMultiCertConfigLoader::_load_lines(SSLCertLookup *lookup, SSLConfigLines::const_iterator begin, + SSLConfigLines::const_iterator end) { - const SSLConfigParams *params = this->_params; - - char *tok_state = nullptr; - char *line = nullptr; - unsigned line_num = 0; - matcher_line line_info; - const matcher_tags sslCertTags = {nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, false}; + const SSLConfigParams *params = this->_params; + + for (auto it = begin; it != end; ++it) { + auto &&[line, line_num] = *it; + shared_SSLMultiCertConfigParams sslMultiCertSettings = std::make_shared(); + const char *errPtr; + matcher_line line_info; + + errPtr = parseConfigLine(line, &line_info, &sslCertTags); + Debug("ssl", "currently parsing %s", line); + if (errPtr != nullptr) { + RecSignalWarning(REC_SIGNAL_CONFIG_ERROR, "%s: discarding %s entry at line %d: %s", __func__, params->configFilePath, + line_num, errPtr); + } else { + if (ssl_extract_certificate(&line_info, sslMultiCertSettings.get())) { + // There must be a certificate specified unless the tunnel action is set + if (sslMultiCertSettings->cert || sslMultiCertSettings->opt != SSLCertContextOption::OPT_TUNNEL) { + this->_store_ssl_ctx(lookup, sslMultiCertSettings); + } else { + Warning("No ssl_cert_name specified and no tunnel action set"); + } + } + } + } +} +bool +SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) +{ Note("%s loading ...", ts::filename::SSL_MULTICERT); + Debug("ssl", "%s loading ...", ts::filename::SSL_MULTICERT); + const SSLConfigParams *params = this->_params; std::error_code ec; std::string content{ts::file::load(ts::file::path{params->configFilePath}, ec)}; + if (ec) { switch (ec.value()) { case ENOENT: @@ -1754,6 +1793,13 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) REC_ReadConfigInteger(elevate_setting, "proxy.config.ssl.cert.load_elevated"); ElevateAccess elevate_access(elevate_setting ? ElevateAccess::FILE_PRIVILEGE : 0); + // Store all the lines in the list, slightly clunky but lets us parallelize this if allowed + // ToDo: Maybe change the parser here to use a more C++'ish line parser rather than tokLine()... + char *line = nullptr; + unsigned line_num = 0; + char *tok_state = nullptr; + SSLConfigLines single_lines; + line = tokLine(content.data(), &tok_state); while (line != nullptr) { line_num++; @@ -1764,29 +1810,36 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) } if (*line != '\0' && *line != '#') { - shared_SSLMultiCertConfigParams sslMultiCertSettings = std::make_shared(); - const char *errPtr; - - errPtr = parseConfigLine(line, &line_info, &sslCertTags); - Debug("ssl", "currently parsing %s", line); - if (errPtr != nullptr) { - RecSignalWarning(REC_SIGNAL_CONFIG_ERROR, "%s: discarding %s entry at line %d: %s", __func__, params->configFilePath, - line_num, errPtr); - } else { - if (ssl_extract_certificate(&line_info, sslMultiCertSettings.get())) { - // There must be a certificate specified unless the tunnel action is set - if (sslMultiCertSettings->cert || sslMultiCertSettings->opt != SSLCertContextOption::OPT_TUNNEL) { - this->_store_ssl_ctx(lookup, sslMultiCertSettings); - } else { - Warning("No ssl_cert_name specified and no tunnel action set"); - } - } - } + single_lines.push_back(std::make_tuple(line, line_num)); } - line = tokLine(nullptr, &tok_state); } + // Process all the lines if we're not running parallelization on multiple threads + if (params->configLoadConcurrency > 0) { + std::size_t bucket_size = std::max(1u, static_cast(single_lines.size() / params->configLoadConcurrency)); + SSLConfigLines::const_iterator current = std::as_const(single_lines).begin(); + std::list threads; + std::size_t num_lines = single_lines.size(); + + while (num_lines > 0) { + SSLConfigLines::const_iterator last = current + std::min(num_lines, bucket_size); + + num_lines -= std::min(bucket_size, num_lines); + threads.push_back(std::thread(&SSLMultiCertConfigLoader::_load_lines, this, lookup, current, last)); + current = last; + } + + // Wait for all the threads to finish their tasks. + for (std::thread &th : threads) { + if (th.joinable()) + th.join(); + } + ink_assert(num_lines == 0); // Make sure no lines where not processed... + } else { + this->_load_lines(lookup, single_lines.begin(), single_lines.end()); + } + // We *must* have a default context even if it can't possibly work. The default context is used to // bootstrap the SSL handshake so that we can subsequently do the SNI lookup to switch to the real // context. diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc index 2d5ed23393e..05dc9e80009 100644 --- a/mgmt/RecordsConfig.cc +++ b/mgmt/RecordsConfig.cc @@ -1109,7 +1109,9 @@ static const RecordElement RecordsConfig[] = {RECT_CONFIG, "proxy.config.ssl.server.multicert.filename", RECD_STRING, ts::filename::SSL_MULTICERT, RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , {RECT_CONFIG, "proxy.config.ssl.server.multicert.exit_on_load_fail", RECD_INT, "1", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL} -, + , + {RECT_CONFIG, "proxy.config.ssl.server.multicert.concurrency", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "^[0-9]+$", RECA_NULL} + , {RECT_CONFIG, "proxy.config.ssl.servername.filename", RECD_STRING, ts::filename::SNI, RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL} , {RECT_CONFIG, "proxy.config.ssl.server.ticket_key.filename", RECD_STRING, nullptr, RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL} From 7bdb85421e9b1069004c71d66878460e0cbaf667 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Wed, 26 May 2021 12:42:44 -0600 Subject: [PATCH 2/2] If enabled, allow the initial load to use all CPU --- iocore/net/P_SSLUtils.h | 2 +- iocore/net/QUICMultiCertConfigLoader.cc | 2 +- iocore/net/SSLConfig.cc | 2 +- iocore/net/SSLUtils.cc | 6 ++++-- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/iocore/net/P_SSLUtils.h b/iocore/net/P_SSLUtils.h index c6649aefbde..194d1cf9ffd 100644 --- a/iocore/net/P_SSLUtils.h +++ b/iocore/net/P_SSLUtils.h @@ -63,7 +63,7 @@ class SSLMultiCertConfigLoader SSLMultiCertConfigLoader(const SSLConfigParams *p) : _params(p) { ink_mutex_init(&m_mutex); } virtual ~SSLMultiCertConfigLoader() { ink_mutex_destroy(&m_mutex); }; - bool load(SSLCertLookup *lookup); + bool load(SSLCertLookup *lookup, bool firstLoad); virtual SSL_CTX *default_server_ssl_ctx(); virtual SSL_CTX *init_server_ssl_ctx(CertLoadData const &data, const SSLMultiCertConfigParams *sslMultCertSettings, diff --git a/iocore/net/QUICMultiCertConfigLoader.cc b/iocore/net/QUICMultiCertConfigLoader.cc index 17a80ca0acb..87821b94d95 100644 --- a/iocore/net/QUICMultiCertConfigLoader.cc +++ b/iocore/net/QUICMultiCertConfigLoader.cc @@ -53,7 +53,7 @@ QUICCertConfig::reconfigure() SSLCertLookup *lookup = new SSLCertLookup(); QUICMultiCertConfigLoader loader(params); - loader.load(lookup); + loader.load(lookup, 0 == _config_id); _config_id = configProcessor.set(_config_id, lookup); } diff --git a/iocore/net/SSLConfig.cc b/iocore/net/SSLConfig.cc index c5ef0304d4c..48ddde6d618 100644 --- a/iocore/net/SSLConfig.cc +++ b/iocore/net/SSLConfig.cc @@ -505,7 +505,7 @@ SSLCertificateConfig::reconfigure() } SSLMultiCertConfigLoader loader(params); - loader.load(lookup); + loader.load(lookup, 0 == configid); // The flag indicates if this is a first time load or not if (!lookup->is_valid) { retStatus = false; diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index c0081b087c9..dd2d7055d9f 100644 --- a/iocore/net/SSLUtils.cc +++ b/iocore/net/SSLUtils.cc @@ -1767,7 +1767,7 @@ SSLMultiCertConfigLoader::_load_lines(SSLCertLookup *lookup, SSLConfigLines::con } bool -SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) +SSLMultiCertConfigLoader::load(SSLCertLookup *lookup, bool firstLoad) { Note("%s loading ...", ts::filename::SSL_MULTICERT); Debug("ssl", "%s loading ...", ts::filename::SSL_MULTICERT); @@ -1817,7 +1817,9 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup) // Process all the lines if we're not running parallelization on multiple threads if (params->configLoadConcurrency > 0) { - std::size_t bucket_size = std::max(1u, static_cast(single_lines.size() / params->configLoadConcurrency)); + int num_threads = firstLoad ? std::max(static_cast(std::thread::hardware_concurrency()), params->configLoadConcurrency) : + params->configLoadConcurrency; + std::size_t bucket_size = std::max(1u, static_cast(single_lines.size() / num_threads)); SSLConfigLines::const_iterator current = std::as_const(single_lines).begin(); std::list threads; std::size_t num_lines = single_lines.size();