diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8ba47a67d43f..a1deec356c79 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -18,6 +18,7 @@ Minor Behavior Changes * http: avoiding delay-close for HTTP/1.0 responses framed by connection: close as well as HTTP/1.1 if the request is fully read. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.skip_delay_close`` to false. * http: now the max concurrent streams of http2 connection can not only be adjusted down according to the SETTINGS frame but also can be adjusted up, of course, it can not exceed the configured upper bounds. This fix is guarded by ``envoy.reloadable_features.http2_allow_capacity_increase_by_settings``. * http: when writing custom filters, `injectEncodedDataToFilterChain` and `injectDecodedDataToFilterChain` now trigger sending of headers if they were not yet sent due to `StopIteration`. Previously, calling one of the inject functions in that state would trigger an assertion. See issue #19891 for more details. +* perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update. Bug Fixes --------- 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 d1c1229814e7..179e1a4b4adc 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -359,10 +359,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 @@ -387,10 +389,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(); } @@ -400,10 +404,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_; @@ -433,10 +439,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 d7ce778387f6..4074a121f5e9 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 e59a32842770..6711d97ba179 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -828,7 +828,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/http/http3/conn_pool_test.cc b/test/common/http/http3/conn_pool_test.cc index ba86af349632..cf50de5bce26 100644 --- a/test/common/http/http3/conn_pool_test.cc +++ b/test/common/http/http3/conn_pool_test.cc @@ -57,7 +57,7 @@ class MockPoolConnectResultCallback : public PoolConnectResultCallback { class Http3ConnPoolImplTest : public Event::TestUsingSimulatedTime, public testing::Test { public: Http3ConnPoolImplTest() { - EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _)) + EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)) .WillRepeatedly(Return(ssl_context_)); factory_.emplace(std::unique_ptr( new NiceMock), diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc index 4f055be83a9f..39f880c23e87 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, test_address_ = Network::Utility::resolveUrl( absl::StrCat("tcp://", Network::Test::getLoopbackAddressUrlString(GetParam()), ":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 3c49ad23f35e..802086062d2f 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -102,6 +102,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 06b3405b7d2a..a1133b525d28 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 4160758acafe..2b8b46bc0673 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -45,7 +45,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 f4568860b223..597cd4dc12d8 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); } @@ -1108,8 +1144,6 @@ TEST_F(ClientContextConfigImplTest, RSA1024Pkcs12) { 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("Failed to load certificate chain from .*selfsigned_rsa_1024_certkey.p12, " @@ -1120,7 +1154,7 @@ TEST_F(ClientContextConfigImplTest, RSA1024Pkcs12) { "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); } @@ -1139,7 +1173,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. @@ -1154,10 +1189,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. @@ -1172,10 +1206,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. @@ -1190,10 +1223,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"); @@ -1209,11 +1240,9 @@ TEST_F(ClientContextConfigImplTest, NonP256EcdsaPkcs12) { 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), EnvoyException, + manager_.createSslClientContext(store, client_context_config), EnvoyException, "Failed to load certificate chain from .*selfsigned_ecdsa_p384_certkey.p12, " "only P-256 ECDSA certificates are supported"); } @@ -1447,10 +1476,8 @@ TEST_F(ClientContextConfigImplTest, PasswordWrongPkcs12) { 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 pkcs12 from ", pkcs12_path)); } @@ -1476,10 +1503,8 @@ TEST_F(ClientContextConfigImplTest, PasswordNotSuppliedPkcs12) { 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 pkcs12 from ", pkcs12_path)); } @@ -1508,10 +1533,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)); } @@ -1753,8 +1776,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"); } @@ -1851,8 +1874,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 104242ce0c92..beef65a850c0 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 {})); }