From 1c2d16066b61f0ed946b0d0a34822e19f06acb4b Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 17 Feb 2022 10:29:27 -0800 Subject: [PATCH] SDS: reduce cost of secret update (#19971) Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 2 + envoy/ssl/context_manager.h | 13 +- .../tls/context_manager_impl.cc | 55 +++---- .../tls/context_manager_impl.h | 14 +- .../transport_sockets/tls/ssl_socket.cc | 17 ++- .../transport_sockets/tls/ssl_socket.h | 4 + source/server/ssl_context_manager.cc | 12 +- test/common/http/conn_pool_grid_test.cc | 2 +- .../client_connection_factory_impl_test.cc | 3 +- .../quic/envoy_quic_proof_source_test.cc | 1 + .../quic/envoy_quic_proof_verifier_test.cc | 1 + .../dynamic_forward_proxy/cluster_test.cc | 2 +- .../tls/context_impl_test.cc | 143 +++++++++++------- test/mocks/ssl/mocks.h | 7 +- test/server/admin/server_info_handler_test.cc | 3 +- test/server/ssl_context_manager_test.cc | 7 +- 16 files changed, 162 insertions(+), 124 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8abda6992b9f..e9b22ce1a0ca 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,6 +9,8 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* +* perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update. + Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/envoy/ssl/context_manager.h b/envoy/ssl/context_manager.h index 0302642db8cf..bf3d16014bf7 100644 --- a/envoy/ssl/context_manager.h +++ b/envoy/ssl/context_manager.h @@ -22,17 +22,15 @@ class ContextManager { /** * Builds a ClientContext from a ClientContextConfig. */ - virtual ClientContextSharedPtr - createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) PURE; + virtual ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, + const ClientContextConfig& config) PURE; /** * Builds a ServerContext from a ServerContextConfig. */ virtual ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context) PURE; + const std::vector& server_names) PURE; /** * @return the number of days until the next certificate being managed will expire. @@ -55,6 +53,11 @@ class ContextManager { * expire, or `absl::nullopt` if no OCSP responses exist. */ virtual absl::optional secondsUntilFirstOcspResponseExpires() const PURE; + + /** + * Remove an existing ssl context. + */ + virtual void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) PURE; }; using ContextManagerPtr = std::unique_ptr; diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/source/extensions/transport_sockets/tls/context_manager_impl.cc index 7c83121892f2..09cfa3f71aa8 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.cc +++ b/source/extensions/transport_sockets/tls/context_manager_impl.cc @@ -14,62 +14,42 @@ namespace Extensions { namespace TransportSockets { namespace Tls { +ContextManagerImpl::ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {} + ContextManagerImpl::~ContextManagerImpl() { - removeEmptyContexts(); KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030"); } -void ContextManagerImpl::removeEmptyContexts() { - contexts_.remove_if([](const std::weak_ptr& n) { return n.expired(); }); -} - -void ContextManagerImpl::removeOldContext(std::shared_ptr old_context) { - if (old_context) { - contexts_.remove_if([old_context](const std::weak_ptr& n) { - std::shared_ptr sp = n.lock(); - if (sp) { - return old_context == sp; - } - return false; - }); - } -} - Envoy::Ssl::ClientContextSharedPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, - const Envoy::Ssl::ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) { + const Envoy::Ssl::ClientContextConfig& config) { if (!config.isReady()) { return nullptr; } Envoy::Ssl::ClientContextSharedPtr context = std::make_shared(scope, config, time_source_); - removeOldContext(old_context); - removeEmptyContexts(); - contexts_.emplace_back(context); + contexts_.insert(context); return context; } -Envoy::Ssl::ServerContextSharedPtr ContextManagerImpl::createSslServerContext( - Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config, - const std::vector& server_names, Envoy::Ssl::ServerContextSharedPtr old_context) { +Envoy::Ssl::ServerContextSharedPtr +ContextManagerImpl::createSslServerContext(Stats::Scope& scope, + const Envoy::Ssl::ServerContextConfig& config, + const std::vector& server_names) { if (!config.isReady()) { return nullptr; } Envoy::Ssl::ServerContextSharedPtr context = std::make_shared(scope, config, server_names, time_source_); - removeOldContext(old_context); - removeEmptyContexts(); - contexts_.emplace_back(context); + contexts_.insert(context); return context; } size_t ContextManagerImpl::daysUntilFirstCertExpires() const { size_t ret = std::numeric_limits::max(); - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { ret = std::min(context->daysUntilFirstCertExpires(), ret); } @@ -79,8 +59,7 @@ size_t ContextManagerImpl::daysUntilFirstCertExpires() const { absl::optional ContextManagerImpl::secondsUntilFirstOcspResponseExpires() const { absl::optional ret; - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { auto next_expiration = context->secondsUntilFirstOcspResponseExpires(); if (next_expiration) { @@ -93,14 +72,22 @@ absl::optional ContextManagerImpl::secondsUntilFirstOcspResponseExpire } void ContextManagerImpl::iterateContexts(std::function callback) { - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { callback(*context); } } } +void ContextManagerImpl::removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) { + if (old_context != nullptr) { + auto erased = contexts_.erase(old_context); + // The contexts is expected to be added before is removed. + // And the prod ssl factory implementation guarantees any context is removed exactly once. + ASSERT(erased == 1); + } +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.h b/source/extensions/transport_sockets/tls/context_manager_impl.h index eb8914fc0009..7a05015e682f 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.h +++ b/source/extensions/transport_sockets/tls/context_manager_impl.h @@ -24,29 +24,27 @@ namespace Tls { */ class ContextManagerImpl final : public Envoy::Ssl::ContextManager { public: - ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {} + explicit ContextManagerImpl(TimeSource& time_source); ~ContextManagerImpl() override; // Ssl::ContextManager Ssl::ClientContextSharedPtr - createSslClientContext(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) override; + createSslClientContext(Stats::Scope& scope, + const Envoy::Ssl::ClientContextConfig& config) override; Ssl::ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context) override; + const std::vector& server_names) override; size_t daysUntilFirstCertExpires() const override; absl::optional secondsUntilFirstOcspResponseExpires() const override; void iterateContexts(std::function callback) override; Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override { return private_key_method_manager_; }; + void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override; private: - void removeEmptyContexts(); - void removeOldContext(std::shared_ptr old_context); TimeSource& time_source_; - std::list> contexts_; + absl::flat_hash_set contexts_; PrivateKeyMethodManagerImpl private_key_method_manager_{}; }; diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 31ed7fd52f01..b403afec7d39 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -356,10 +356,12 @@ ClientSslSocketFactory::ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPt Stats::Scope& stats_scope) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), config_(std::move(config)), - ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_, nullptr)) { + ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } +ClientSslSocketFactory::~ClientSslSocketFactory() { manager_.removeContext(ssl_ctx_); } + Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket( Network::TransportSocketOptionsConstSharedPtr transport_socket_options) const { // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and @@ -384,10 +386,12 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } void ClientSslSocketFactory::onAddOrUpdateSecret() { ENVOY_LOG(debug, "Secret is updated."); + auto ctx = manager_.createSslClientContext(stats_scope_, *config_); { absl::WriterMutexLock l(&ssl_ctx_mu_); - ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_, ssl_ctx_); + std::swap(ctx, ssl_ctx_); } + manager_.removeContext(ctx); stats_.ssl_context_update_by_sds_.inc(); } @@ -397,10 +401,12 @@ ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPt const std::vector& server_names) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)), config_(std::move(config)), server_names_(server_names), - ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_, nullptr)) { + ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) { config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } +ServerSslSocketFactory::~ServerSslSocketFactory() { manager_.removeContext(ssl_ctx_); } + Envoy::Ssl::ClientContextSharedPtr ClientSslSocketFactory::sslCtx() { absl::ReaderMutexLock l(&ssl_ctx_mu_); return ssl_ctx_; @@ -430,10 +436,13 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } void ServerSslSocketFactory::onAddOrUpdateSecret() { ENVOY_LOG(debug, "Secret is updated."); + auto ctx = manager_.createSslServerContext(stats_scope_, *config_, server_names_); { absl::WriterMutexLock l(&ssl_ctx_mu_); - ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_, ssl_ctx_); + std::swap(ctx, ssl_ctx_); } + manager_.removeContext(ctx); + stats_.ssl_context_update_by_sds_.inc(); } diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index a6502cc9548c..4874b78b5585 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -103,6 +103,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope); + ~ClientSslSocketFactory() override; + Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; bool implementsSecureTransport() const override; @@ -133,6 +135,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); + ~ServerSslSocketFactory() override; + Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; bool implementsSecureTransport() const override; diff --git a/source/server/ssl_context_manager.cc b/source/server/ssl_context_manager.cc index 0189da0c6d04..a1a9fc12c9d0 100644 --- a/source/server/ssl_context_manager.cc +++ b/source/server/ssl_context_manager.cc @@ -14,16 +14,14 @@ namespace Server { class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager { Ssl::ClientContextSharedPtr createSslClientContext(Stats::Scope& /* scope */, - const Envoy::Ssl::ClientContextConfig& /* config */, - Envoy::Ssl::ClientContextSharedPtr /* old_context */) override { + const Envoy::Ssl::ClientContextConfig& /* config */) override { throwException(); } Ssl::ServerContextSharedPtr createSslServerContext(Stats::Scope& /* scope */, const Envoy::Ssl::ServerContextConfig& /* config */, - const std::vector& /* server_names */, - Envoy::Ssl::ServerContextSharedPtr /* old_context */) override { + const std::vector& /* server_names */) override { throwException(); } @@ -36,6 +34,12 @@ class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager { Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override { throwException(); } + void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override { + if (old_context) { + throw EnvoyException("SSL is not supported in this configuration"); + } + } + private: [[noreturn]] void throwException() { throw EnvoyException("SSL is not supported in this configuration"); diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 69528416002d..dd32fe08a911 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -691,7 +691,7 @@ TEST_F(ConnectivityGridTest, ConnectionCloseDuringCreation) { Envoy::Ssl::ClientContextConfigPtr config(new NiceMock()); NiceMock factory_context; Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext()); - EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _, _)) + EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _)) .WillOnce(Return(ssl_context)); auto factory = std::make_unique(std::move(config), factory_context); diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc index 97220f85244a..6dae4ee709d9 100644 --- a/test/common/quic/client_connection_factory_impl_test.cc +++ b/test/common/quic/client_connection_factory_impl_test.cc @@ -25,8 +25,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t Network::Test::getLoopbackAddressString(TestEnvironment::getIpVersionsForTest()[0]), ":30")); Ssl::ClientContextSharedPtr context{new Ssl::MockClientContext()}; - EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _)) - .WillOnce(Return(context)); + EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)).WillOnce(Return(context)); factory_ = std::make_unique( std::unique_ptr( new NiceMock), diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index 4e27141773ee..40021edd52bf 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -101,6 +101,7 @@ class SignatureVerifier { NiceMock client_context_config_; NiceMock cert_validation_ctx_config_; std::unique_ptr verifier_; + NiceMock tls_context_manager_; }; class TestSignatureCallback : public quic::ProofSource::SignatureCallback { diff --git a/test/common/quic/envoy_quic_proof_verifier_test.cc b/test/common/quic/envoy_quic_proof_verifier_test.cc index c6b075fe03ea..6aa79b6ee4f8 100644 --- a/test/common/quic/envoy_quic_proof_verifier_test.cc +++ b/test/common/quic/envoy_quic_proof_verifier_test.cc @@ -89,6 +89,7 @@ class EnvoyQuicProofVerifierTest : public testing::Test { NiceMock client_context_config_; Ssl::MockCertificateValidationContextConfig cert_validation_ctx_config_; std::unique_ptr verifier_; + NiceMock tls_context_manager_; }; TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainSuccess) { diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index 097dcf993cd4..eccd93e18699 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -43,7 +43,7 @@ class ClusterTest : public testing::Test, admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_store_, singleton_manager_, tls_, validation_visitor_, *api_, options_); if (uses_tls) { - EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _, _)); + EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _)); } EXPECT_CALL(*dns_cache_manager_, getCache(_)); // Below we return a nullptr handle which has no effect on the code under test but isn't diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 8e4bfc66ef77..023694982000 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -66,6 +66,7 @@ const std::vector& knownCipherSuites() { "PSK-AES256-CBC-SHA", "DES-CBC3-SHA"}); } + } // namespace class SslLibraryCipherSuiteSupport : public ::testing::TestWithParam {}; @@ -95,6 +96,22 @@ TEST_P(SslLibraryCipherSuiteSupport, CipherSuitesNotRemoved) { } class SslContextImplTest : public SslCertsTest { +public: + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ClientContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ServerContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + protected: Event::SimulatedTimeSystem time_system_; ContextManagerImpl manager_{time_system_}; @@ -111,7 +128,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); EXPECT_THROW_WITH_MESSAGE( - manager_.createSslClientContext(store_, cfg, nullptr), EnvoyException, + manager_.createSslClientContext(store_, cfg), EnvoyException, "Failed to initialize cipher suites " "-ALL:+[AES128-SHA|BOGUS1-SHA256]:BOGUS2-SHA:AES256-SHA. The following " "ciphers were rejected when tried individually: BOGUS1-SHA256, BOGUS2-SHA"); @@ -131,8 +148,8 @@ TEST_F(SslContextImplTest, TestExpiringCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); - + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); // Calculate the days until test cert expires auto cert_expiry = TestUtility::parseTime(TEST_UNITTEST_CERT_NOT_AFTER, "%b %d %H:%M:%S %Y GMT"); int64_t days_until_expiry = absl::ToInt64Hours(cert_expiry - absl::Now()) / 24; @@ -152,7 +169,8 @@ TEST_F(SslContextImplTest, TestExpiredCert) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); EXPECT_EQ(0U, context->daysUntilFirstCertExpires()); } @@ -172,7 +190,7 @@ TEST_F(SslContextImplTest, TestContextUpdate) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(expired_yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U); const std::string expiring_yaml = R"EOF( @@ -190,7 +208,8 @@ TEST_F(SslContextImplTest, TestContextUpdate) { ClientContextConfigImpl expiring_cfg(expiring_context, factory_context_); Envoy::Ssl::ClientContextSharedPtr new_context( - manager_.createSslClientContext(store_, expiring_cfg, context)); + manager_.createSslClientContext(store_, expiring_cfg)); + manager_.removeContext(context); // Validate that when the context is updated, daysUntilFirstCertExpires reflects the current // context expiry. @@ -201,8 +220,10 @@ TEST_F(SslContextImplTest, TestContextUpdate) { // Update the context again and validate daysUntilFirstCertExpires still reflects the current // expiry. - Envoy::Ssl::ClientContextSharedPtr updated_context( - manager_.createSslClientContext(store_, cfg, new_context)); + Envoy::Ssl::ClientContextSharedPtr updated_context(manager_.createSslClientContext(store_, cfg)); + manager_.removeContext(new_context); + auto cleanup = cleanUpHelper(updated_context); + EXPECT_EQ(updated_context->daysUntilFirstCertExpires(), 0U); EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U); } @@ -224,7 +245,9 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); + // This is similar to the hack above, but right now we generate the ca_cert and it expires in 15 // days only in the first second that it's valid. We will partially match for up until Days until // Expiration: 1. @@ -274,7 +297,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithSAN) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_cert.pem", "serial_number": ")EOF", @@ -328,7 +352,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithIPSAN) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_ip_cert.pem", "serial_number": ")EOF", @@ -386,7 +411,9 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithExpiration) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); + std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_cert.pem", @@ -416,7 +443,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithExpiration) { TEST_F(SslContextImplTest, TestNoCert) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext config; ClientContextConfigImpl cfg(config, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); EXPECT_EQ(nullptr, context->getCaCertInformation()); EXPECT_TRUE(context->getCertChainInformation().empty()); } @@ -438,9 +466,9 @@ TEST_F(SslContextImplTest, AtMostOneRsaCert) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "at most one certificate of a given type may be specified"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, + "at most one certificate of a given type may be specified"); } // Multiple ECDSA certificates are rejected. @@ -460,9 +488,9 @@ TEST_F(SslContextImplTest, AtMostOneEcdsaCert) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "at most one certificate of a given type may be specified"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, + "at most one certificate of a given type may be specified"); } // Certificates with no subject CN and no SANs are rejected. @@ -478,15 +506,14 @@ TEST_F(SslContextImplTest, MustHaveSubjectOrSAN) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "has neither subject CN nor SAN names"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, "has neither subject CN nor SAN names"); } class SslServerContextImplOcspTest : public SslContextImplTest { public: Envoy::Ssl::ServerContextSharedPtr loadConfig(ServerContextConfigImpl& cfg) { - return manager_.createSslServerContext(store_, cfg, std::vector{}, nullptr); + return manager_.createSslServerContext(store_, cfg, std::vector{}); } Envoy::Ssl::ServerContextSharedPtr loadConfigYaml(const std::string& yaml) { @@ -509,7 +536,8 @@ TEST_F(SslServerContextImplOcspTest, TestFilenameOcspStapleConfigLoads) { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/ocsp/test_data/good_ocsp_resp.der" ocsp_staple_policy: must_staple )EOF"; - loadConfigYaml(tls_context_yaml); + auto context = loadConfigYaml(tls_context_yaml); + auto cleanup = cleanUpHelper(context); } TEST_F(SslServerContextImplOcspTest, TestInlineBytesOcspStapleConfigLoads) { @@ -529,7 +557,8 @@ TEST_F(SslServerContextImplOcspTest, TestInlineBytesOcspStapleConfigLoads) { )EOF", base64_response); - loadConfigYaml(tls_context_yaml); + auto context = loadConfigYaml(tls_context_yaml); + auto cleanup = cleanUpHelper(context); } TEST_F(SslServerContextImplOcspTest, TestInlineStringOcspStapleConfigFails) { @@ -638,6 +667,7 @@ TEST_F(SslServerContextImplOcspTest, TestGetCertInformationWithOCSP) { envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); auto context = loadConfigYaml(yaml); + auto cleanup = cleanUpHelper(context); constexpr absl::string_view this_update = "This Update: "; constexpr absl::string_view next_update = "Next Update: "; @@ -683,7 +713,8 @@ class SslServerContextImplTicketTest : public SslContextImplTest { public: void loadConfig(ServerContextConfigImpl& cfg) { Envoy::Ssl::ServerContextSharedPtr server_ctx( - manager_.createSslServerContext(store_, cfg, std::vector{}, nullptr)); + manager_.createSslServerContext(store_, cfg, std::vector{})); + auto cleanup = cleanUpHelper(server_ctx); } void loadConfigV2(envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext& cfg) { @@ -1002,7 +1033,19 @@ TEST_F(SslServerContextImplTicketTest, StatelessSessionResumptionEnabledWhenKeyI EXPECT_FALSE(server_context_config.disableStatelessSessionResumption()); } -class ClientContextConfigImplTest : public SslCertsTest {}; +class ClientContextConfigImplTest : public SslCertsTest { +public: + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ClientContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + + Event::SimulatedTimeSystem time_system_; + ContextManagerImpl manager_{time_system_}; +}; // Validate that empty SNI (according to C string rules) fails config validation. TEST_F(ClientContextConfigImplTest, EmptyServerNameIndication) { @@ -1029,10 +1072,8 @@ TEST_F(ClientContextConfigImplTest, InvalidCertificateHash) { ->add_verify_certificate_hash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, factory_context); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Invalid hex-encoded SHA-256 .*"); } @@ -1045,10 +1086,8 @@ TEST_F(ClientContextConfigImplTest, InvalidCertificateSpki) { // Not a base64-encoded string. ->add_verify_certificate_spki("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, factory_context); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Invalid base64-encoded SHA-256 .*"); } @@ -1064,10 +1103,9 @@ TEST_F(ClientContextConfigImplTest, RSA2048Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that 1024-bit RSA certificates are rejected. @@ -1082,8 +1120,6 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; std::string error_msg( @@ -1094,7 +1130,7 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) { "with 2048-bit or larger keys are supported" #endif ); - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, error_msg); } @@ -1113,7 +1149,8 @@ TEST_F(ClientContextConfigImplTest, RSA3072Cert) { Event::SimulatedTimeSystem time_system; ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that 4096-bit RSA certificates load successfully. @@ -1128,10 +1165,9 @@ TEST_F(ClientContextConfigImplTest, RSA4096Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that P256 ECDSA certs load. @@ -1146,10 +1182,9 @@ TEST_F(ClientContextConfigImplTest, P256EcdsaCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that non-P256 ECDSA certs are rejected. @@ -1164,10 +1199,8 @@ TEST_F(ClientContextConfigImplTest, NonP256EcdsaCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Failed to load certificate chain from .*selfsigned_ecdsa_p384_cert.pem, " "only P-256 ECDSA certificates are supported"); @@ -1370,10 +1403,8 @@ TEST_F(ClientContextConfigImplTest, PasswordNotSuppliedTlsCertificates) { factory_context_.secretManager().addStaticSecret(secret_config); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, absl::StrCat("Failed to load private key from ", private_key_path)); } @@ -1615,8 +1646,8 @@ TEST_F(ServerContextConfigImplTest, TlsCertificateNonEmpty) { ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; EXPECT_THROW_WITH_MESSAGE( - Envoy::Ssl::ServerContextSharedPtr server_ctx(manager.createSslServerContext( - store, client_context_config, std::vector{}, nullptr)), + Envoy::Ssl::ServerContextSharedPtr server_ctx( + manager.createSslServerContext(store, client_context_config, std::vector{})), EnvoyException, "Server TlsCertificates must have a certificate specified"); } @@ -1713,8 +1744,8 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureNoMethod) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); EXPECT_THROW_WITH_MESSAGE( - Envoy::Ssl::ServerContextSharedPtr server_ctx(manager.createSslServerContext( - store, server_context_config, std::vector{}, nullptr)), + Envoy::Ssl::ServerContextSharedPtr server_ctx( + manager.createSslServerContext(store, server_context_config, std::vector{})), EnvoyException, "Failed to get BoringSSL private key method from provider"); } diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index e36b06fbd99d..06d4d88b06fd 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -25,16 +25,15 @@ class MockContextManager : public ContextManager { ~MockContextManager() override; MOCK_METHOD(ClientContextSharedPtr, createSslClientContext, - (Stats::Scope & scope, const ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context)); + (Stats::Scope & scope, const ClientContextConfig& config)); MOCK_METHOD(ServerContextSharedPtr, createSslServerContext, (Stats::Scope & stats, const ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context)); + const std::vector& server_names)); MOCK_METHOD(size_t, daysUntilFirstCertExpires, (), (const)); MOCK_METHOD(absl::optional, secondsUntilFirstOcspResponseExpires, (), (const)); MOCK_METHOD(void, iterateContexts, (std::function callback)); MOCK_METHOD(Ssl::PrivateKeyMethodManager&, privateKeyMethodManager, ()); + MOCK_METHOD(void, removeContext, (const Envoy::Ssl::ContextSharedPtr& old_context)); }; class MockConnectionInfo : public ConnectionInfo { diff --git a/test/server/admin/server_info_handler_test.cc b/test/server/admin/server_info_handler_test.cc index 9b7e0fa3643f..9d310832e309 100644 --- a/test/server/admin/server_info_handler_test.cc +++ b/test/server/admin/server_info_handler_test.cc @@ -28,7 +28,7 @@ TEST_P(AdminInstanceTest, ContextThatReturnsNullCertDetails) { Extensions::TransportSockets::Tls::ClientContextConfigImpl cfg(config, factory_context); Stats::IsolatedStoreImpl store; Envoy::Ssl::ClientContextSharedPtr client_ctx( - server_.sslContextManager().createSslClientContext(store, cfg, nullptr)); + server_.sslContextManager().createSslClientContext(store, cfg)); const std::string expected_empty_json = R"EOF({ "certificates": [ @@ -45,6 +45,7 @@ TEST_P(AdminInstanceTest, ContextThatReturnsNullCertDetails) { EXPECT_TRUE(client_ctx->getCertChainInformation().empty()); EXPECT_EQ(Http::Code::OK, getCallback("/certs", header_map, response)); EXPECT_EQ(expected_empty_json, response.toString()); + server_.sslContextManager().removeContext(client_ctx); } TEST_P(AdminInstanceTest, Memory) { diff --git a/test/server/ssl_context_manager_test.cc b/test/server/ssl_context_manager_test.cc index 70bc7d6c1a5c..f25dba343ded 100644 --- a/test/server/ssl_context_manager_test.cc +++ b/test/server/ssl_context_manager_test.cc @@ -23,11 +23,10 @@ TEST(SslContextManager, createStub) { // Check we've created a stub, not real manager. EXPECT_EQ(manager->daysUntilFirstCertExpires(), std::numeric_limits::max()); EXPECT_EQ(manager->secondsUntilFirstOcspResponseExpires(), absl::nullopt); - EXPECT_THROW_WITH_MESSAGE(manager->createSslClientContext(scope, client_config, nullptr), + EXPECT_THROW_WITH_MESSAGE(manager->createSslClientContext(scope, client_config), EnvoyException, + "SSL is not supported in this configuration"); + EXPECT_THROW_WITH_MESSAGE(manager->createSslServerContext(scope, server_config, server_names), EnvoyException, "SSL is not supported in this configuration"); - EXPECT_THROW_WITH_MESSAGE( - manager->createSslServerContext(scope, server_config, server_names, nullptr), EnvoyException, - "SSL is not supported in this configuration"); EXPECT_NO_THROW(manager->iterateContexts([](const Envoy::Ssl::Context&) -> void {})); }