From 2ada52edb555289335b124b06f7a176b293efae2 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 15 Aug 2018 16:53:05 -0700 Subject: [PATCH 01/27] Rebase to master. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 1 + include/envoy/secret/secret_manager.h | 25 ++- include/envoy/secret/secret_provider.h | 1 + include/envoy/server/BUILD | 1 + .../envoy/server/transport_socket_config.h | 13 ++ include/envoy/ssl/BUILD | 2 +- include/envoy/ssl/context_config.h | 16 +- include/envoy/upstream/cluster_manager.h | 1 + source/common/common/logger.h | 1 + source/common/config/BUILD | 1 + source/common/config/protobuf_link_hacks.h | 2 + source/common/config/resources.h | 1 + source/common/secret/BUILD | 3 + source/common/secret/secret_manager_impl.cc | 34 ++++ source/common/secret/secret_manager_impl.h | 14 +- source/common/ssl/BUILD | 4 +- source/common/ssl/context_config_impl.cc | 64 ++++---- source/common/ssl/context_config_impl.h | 53 +++++-- source/common/ssl/context_manager_impl.cc | 8 + source/common/ssl/ssl_socket.cc | 38 ++++- source/common/ssl/ssl_socket.h | 32 +++- source/common/upstream/BUILD | 2 + .../upstream/health_discovery_service.cc | 1 + source/common/upstream/upstream_impl.cc | 5 + source/common/upstream/upstream_impl.h | 15 +- .../transport_sockets/ssl/config.cc | 4 +- source/server/listener_manager_impl.cc | 1 + source/server/server.cc | 4 +- source/server/server.h | 2 +- source/server/transport_socket_config_impl.h | 9 +- test/common/grpc/BUILD | 1 + .../grpc_client_integration_test_harness.h | 8 +- test/common/secret/BUILD | 2 + .../common/secret/secret_manager_impl_test.cc | 50 ++++++ test/common/ssl/BUILD | 3 +- test/common/ssl/context_impl_test.cc | 148 +++++++++++++----- test/common/ssl/ssl_certs_test.h | 4 +- test/common/ssl/ssl_socket_test.cc | 46 +++--- test/integration/BUILD | 33 +++- test/integration/ads_integration_test.cc | 6 +- test/integration/http_integration.cc | 10 +- test/integration/http_integration.h | 6 +- .../sds_static_integration_test.cc | 37 +---- test/integration/ssl_integration_test.cc | 12 +- test/integration/ssl_integration_test.h | 1 - test/integration/ssl_utility.cc | 35 ++++- test/integration/ssl_utility.h | 4 +- .../integration/tcp_proxy_integration_test.cc | 3 +- test/integration/tcp_proxy_integration_test.h | 1 - test/integration/xfcc_integration_test.cc | 4 +- test/integration/xfcc_integration_test.h | 4 +- test/mocks/secret/BUILD | 1 + test/mocks/secret/mocks.h | 12 ++ test/mocks/server/mocks.cc | 4 +- test/mocks/server/mocks.h | 7 +- 55 files changed, 606 insertions(+), 194 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 6304003c5546a..bf632fa49c9ed 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -29,5 +29,6 @@ envoy_cc_library( deps = [ ":secret_provider_interface", "@envoy_api//envoy/api/v2/auth:cert_cc", + "@envoy_api//envoy/api/v2/core:config_source_cc", ], ) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 85f5e7228f723..c3775f1b0de0c 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -6,12 +6,17 @@ #include "envoy/secret/secret_provider.h" namespace Envoy { + +namespace Server { +namespace Configuration { +class TransportSocketFactoryContext; +} // namespace Configuration +} // namespace Server + namespace Secret { /** - * A manager for static secrets. - * - * TODO(jaebong) Support dynamic secrets. + * A manager for static and dynamic secrets. */ class SecretManager { public: @@ -37,6 +42,20 @@ class SecretManager { */ virtual TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider( const envoy::api::v2::auth::TlsCertificate& tls_certificate) PURE; + + /** + * Finds and returns a dynamic secret provider associated to SDS config. Create + * a new one if such provider does not exist. + * + * @param config_source a protobuf message object contains SDS config source. + * @param config_name a name that uniquely refers to the SDS config source + * @param secret_provider_context context that provides components for creating and initializing + * secret provider. + * @return the dynamic TLS secret provider. + */ + virtual TlsCertificateConfigProviderSharedPtr findOrCreateDynamicSecretProvider( + const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE; }; } // namespace Secret diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index b43b06ed727a0..a624a7a420458 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -4,6 +4,7 @@ #include "envoy/common/callback.h" #include "envoy/common/pure.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { diff --git a/include/envoy/server/BUILD b/include/envoy/server/BUILD index 35f6829a7fad5..3a367b75343fd 100644 --- a/include/envoy/server/BUILD +++ b/include/envoy/server/BUILD @@ -177,6 +177,7 @@ envoy_cc_library( hdrs = ["transport_socket_config.h"], deps = [ "//include/envoy/event:dispatcher_interface", + "//include/envoy/init:init_interface", "//include/envoy/local_info:local_info_interface", "//include/envoy/network:transport_socket_interface", "//include/envoy/runtime:runtime_interface", diff --git a/include/envoy/server/transport_socket_config.h b/include/envoy/server/transport_socket_config.h index 143be130d0796..5f082fda925de 100644 --- a/include/envoy/server/transport_socket_config.h +++ b/include/envoy/server/transport_socket_config.h @@ -3,6 +3,7 @@ #include #include "envoy/event/dispatcher.h" +#include "envoy/init/init.h" #include "envoy/local_info/local_info.h" #include "envoy/network/transport_socket.h" #include "envoy/runtime/runtime.h" @@ -63,6 +64,18 @@ class TransportSocketFactoryContext { * @return the server-wide stats store. */ virtual Stats::Store& stats() PURE; + + /** + * Pass an init manager to register dynamic secret provider. + * @param init_manager instance of init manager. + */ + virtual void setInitManager(Init::Manager& init_manager) PURE; + + /** + * @return a pointer pointing to the instance of an init manager, or nullptr + * if not set. + */ + virtual Init::Manager* initManager() PURE; }; class TransportSocketConfigFactory { diff --git a/include/envoy/ssl/BUILD b/include/envoy/ssl/BUILD index 315b9a5071d5a..e4e8582d124fa 100644 --- a/include/envoy/ssl/BUILD +++ b/include/envoy/ssl/BUILD @@ -22,7 +22,7 @@ envoy_cc_library( name = "context_config_interface", hdrs = ["context_config.h"], deps = [ - ":tls_certificate_config_interface", + "//include/envoy/secret:secret_provider_interface", ], ) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 749cc8451656d..4138c2b1931f1 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -5,9 +5,12 @@ #include #include "envoy/common/pure.h" -#include "envoy/ssl/tls_certificate_config.h" +#include "envoy/secret/secret_provider.h" namespace Envoy { +namespace Secret { +class SecretCallbacks; +} namespace Ssl { /** @@ -95,6 +98,17 @@ class ContextConfig { * @return The maximum TLS protocol version to negotiate. */ virtual unsigned maxProtocolVersion() const PURE; + + /** + * @return true if the ssl config is ready. + */ + virtual bool isReady() const PURE; + + /** + * Add secret callback into context config. + * @param callback callback that is executed by context config. + */ + virtual void setUpdateCallback(Secret::SecretCallbacks& callback) PURE; }; class ClientContextConfig : public virtual ContextConfig { diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index ae9d5b767d23c..ed4f2c76a70f1 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -26,6 +26,7 @@ #include "envoy/upstream/upstream.h" namespace Envoy { + namespace Upstream { /** diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 3746377a8b1b3..6e86eaa5acb9e 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -44,6 +44,7 @@ namespace Logger { FUNCTION(router) \ FUNCTION(runtime) \ FUNCTION(stats) \ + FUNCTION(secret) \ FUNCTION(testing) \ FUNCTION(thrift) \ FUNCTION(tracing) \ diff --git a/source/common/config/BUILD b/source/common/config/BUILD index cd52a50719dcc..179a9d32c95b5 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -242,6 +242,7 @@ envoy_cc_library( hdrs = ["protobuf_link_hacks.h"], deps = [ "@envoy_api//envoy/service/discovery/v2:ads_cc", + "@envoy_api//envoy/service/discovery/v2:sds_cc", "@envoy_api//envoy/service/ratelimit/v2:rls_cc", ], ) diff --git a/source/common/config/protobuf_link_hacks.h b/source/common/config/protobuf_link_hacks.h index 6792f3e797c1e..6a92846253550 100644 --- a/source/common/config/protobuf_link_hacks.h +++ b/source/common/config/protobuf_link_hacks.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/service/discovery/v2/ads.pb.h" +#include "envoy/service/discovery/v2/sds.pb.h" #include "envoy/service/ratelimit/v2/rls.pb.h" namespace Envoy { @@ -9,4 +10,5 @@ namespace Envoy { // This file should be included ONLY if this hack is required. const envoy::service::discovery::v2::AdsDummy _ads_dummy; const envoy::service::ratelimit::v2::RateLimitRequest _rls_dummy; +const envoy::service::discovery::v2::SdsDummy _sds_dummy; } // namespace Envoy diff --git a/source/common/config/resources.h b/source/common/config/resources.h index 03f0c9a2efd8b..69ed2d91a46dc 100644 --- a/source/common/config/resources.h +++ b/source/common/config/resources.h @@ -15,6 +15,7 @@ class TypeUrlValues { const std::string Listener{"type.googleapis.com/envoy.api.v2.Listener"}; const std::string Cluster{"type.googleapis.com/envoy.api.v2.Cluster"}; const std::string ClusterLoadAssignment{"type.googleapis.com/envoy.api.v2.ClusterLoadAssignment"}; + const std::string Secret{"type.googleapis.com/envoy.api.v2.auth.Secret"}; const std::string RouteConfiguration{"type.googleapis.com/envoy.api.v2.RouteConfiguration"}; }; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index a92edf5117bce..045b4ef70865b 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -13,8 +13,11 @@ envoy_cc_library( srcs = ["secret_manager_impl.cc"], hdrs = ["secret_manager_impl.h"], deps = [ + ":sds_api_lib", ":secret_provider_impl_lib", "//include/envoy/secret:secret_manager_interface", + "//include/envoy/server:transport_socket_config_interface", + "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "@envoy_api//envoy/api/v2/auth:cert_cc", ], diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index f3f2c9549ea8f..bae2e944424b2 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -3,6 +3,7 @@ #include "envoy/common/exception.h" #include "common/common/assert.h" +#include "common/secret/sds_api.h" #include "common/secret/secret_provider_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -37,5 +38,38 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific return std::make_shared(tls_certificate); } +TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider( + const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { + std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name; + + auto secret_provider = dynamic_secret_providers_[map_key].lock(); + if (!secret_provider) { + ASSERT(secret_provider_context.initManager() != nullptr); + + std::function unregister_secret_provider = [map_key, config_name, sds_config_source, + this]() { + ENVOY_LOG(debug, "Unregister secret provider. name: {}, sds config: {}", config_name, + sds_config_source.DebugString()); + auto secret_provider = dynamic_secret_providers_.find(map_key); + if (secret_provider != dynamic_secret_providers_.end()) { + dynamic_secret_providers_.erase(map_key); + } else { + ENVOY_LOG(error, "secret provider does not exist. name: {}, sds config: {}", config_name, + sds_config_source.DebugString()); + } + }; + + secret_provider = std::make_shared( + secret_provider_context.localInfo(), secret_provider_context.dispatcher(), + secret_provider_context.random(), secret_provider_context.stats(), + secret_provider_context.clusterManager(), *secret_provider_context.initManager(), + sds_config_source, config_name, unregister_secret_provider); + dynamic_secret_providers_[map_key] = secret_provider; + } + + return secret_provider; +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 6af790f50c422..d1e3e06a7a5ff 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -4,6 +4,7 @@ #include "envoy/secret/secret_manager.h" #include "envoy/secret/secret_provider.h" +#include "envoy/server/transport_socket_config.h" #include "envoy/ssl/tls_certificate_config.h" #include "common/common/logger.h" @@ -11,17 +12,28 @@ namespace Envoy { namespace Secret { -class SecretManagerImpl : public SecretManager, Logger::Loggable { +class SecretManagerImpl : public SecretManager, Logger::Loggable { public: void addStaticSecret(const envoy::api::v2::auth::Secret& secret) override; + TlsCertificateConfigProviderSharedPtr findStaticTlsCertificateProvider(const std::string& name) const override; + TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider( const envoy::api::v2::auth::TlsCertificate& tls_certificate) override; + TlsCertificateConfigProviderSharedPtr findOrCreateDynamicSecretProvider( + const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override; + private: + // Manages pairs of secret name and TlsCertificateConfigProviderSharedPtr. std::unordered_map static_tls_certificate_providers_; + + // map hash code of SDS config source and SdsApi object. + std::unordered_map> + dynamic_secret_providers_; }; } // namespace Secret diff --git a/source/common/ssl/BUILD b/source/common/ssl/BUILD index abc98f802ff0b..8a4a4b015432f 100644 --- a/source/common/ssl/BUILD +++ b/source/common/ssl/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( ":utility_lib", "//include/envoy/network:connection_interface", "//include/envoy/network:transport_socket_interface", + "//include/envoy/stats:stats_macros", "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/common:minimal_logger_lib", @@ -34,8 +35,9 @@ envoy_cc_library( "ssl", ], deps = [ - "//include/envoy/secret:secret_manager_interface", + "//include/envoy/secret:secret_callbacks_interface", "//include/envoy/secret:secret_provider_interface", + "//include/envoy/server:transport_socket_config_interface", "//include/envoy/ssl:context_config_interface", "//source/common/common:assert_lib", "//source/common/common:empty_string", diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index fd9f49ed210ce..2ab7e5a747bff 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -14,34 +14,35 @@ namespace Envoy { namespace Ssl { -namespace { - -Secret::TlsCertificateConfigProviderSharedPtr -getTlsCertificateConfigProvider(const envoy::api::v2::auth::CommonTlsContext& config, - Secret::SecretManager& secret_manager) { +Secret::TlsCertificateConfigProviderSharedPtr ContextConfigImpl::getTlsCertificateConfigProvider( + const envoy::api::v2::auth::CommonTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) { if (!config.tls_certificates().empty()) { const auto& tls_certificate = config.tls_certificates(0); if (!tls_certificate.has_certificate_chain() && !tls_certificate.has_private_key()) { return nullptr; } - return secret_manager.createInlineTlsCertificateProvider(config.tls_certificates(0)); + return factory_context.secretManager().createInlineTlsCertificateProvider( + config.tls_certificates(0)); } if (!config.tls_certificate_sds_secret_configs().empty()) { const auto& sds_secret_config = config.tls_certificate_sds_secret_configs(0); - - auto secret_provider = - secret_manager.findStaticTlsCertificateProvider(sds_secret_config.name()); - if (!secret_provider) { - throw EnvoyException( - fmt::format("Static secret is not defined: {}", sds_secret_config.name())); + if (!sds_secret_config.has_sds_config()) { + // static secret + auto secret_provider = factory_context.secretManager().findStaticTlsCertificateProvider( + sds_secret_config.name()); + if (!secret_provider) { + throw EnvoyException(fmt::format("Unknown static secret: {}", sds_secret_config.name())); + } + return secret_provider; + } else { + return factory_context.secretManager().findOrCreateDynamicSecretProvider( + sds_secret_config.sds_config(), sds_secret_config.name(), factory_context); } - return secret_provider; } return nullptr; } -} // namespace - const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = "[ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-CHACHA20-POLY1305]:" "[ECDHE-RSA-AES128-GCM-SHA256|ECDHE-RSA-CHACHA20-POLY1305]:" @@ -58,8 +59,9 @@ const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256"; -ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContext& config, - Secret::SecretManager& secret_manager) +ContextConfigImpl::ContextConfigImpl( + const envoy::api::v2::auth::CommonTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) : alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), alt_alpn_protocols_(config.deprecated_v1().alt_alpn_protocols()), cipher_suites_(StringUtil::nonEmptyStringOrDefault( @@ -73,7 +75,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex Config::DataSource::read(config.validation_context().crl(), true)), certificate_revocation_list_path_( Config::DataSource::getPath(config.validation_context().crl()).value_or(EMPTY_STRING)), - tls_certficate_provider_(getTlsCertificateConfigProvider(config, secret_manager)), + secret_callback_(nullptr), + tls_certficate_provider_(getTlsCertificateConfigProvider(config, factory_context)), verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), config.validation_context().verify_subject_alt_name().end()), verify_certificate_hash_list_(config.validation_context().verify_certificate_hash().begin(), @@ -85,6 +88,7 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(), TLS1_VERSION)), max_protocol_version_( tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), TLS1_2_VERSION)) { + if (ca_cert_.empty()) { if (!certificate_revocation_list_.empty()) { throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA", @@ -122,8 +126,9 @@ unsigned ContextConfigImpl::tlsVersionFromProto( } ClientContextConfigImpl::ClientContextConfigImpl( - const envoy::api::v2::auth::UpstreamTlsContext& config, Secret::SecretManager& secret_manager) - : ContextConfigImpl(config.common_tls_context(), secret_manager), + const envoy::api::v2::auth::UpstreamTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) + : ContextConfigImpl(config.common_tls_context(), factory_context), server_name_indication_(config.sni()), allow_renegotiation_(config.allow_renegotiation()) { // BoringSSL treats this as a C string, so embedded NULL characters will not // be handled correctly. @@ -137,19 +142,21 @@ ClientContextConfigImpl::ClientContextConfigImpl( } } -ClientContextConfigImpl::ClientContextConfigImpl(const Json::Object& config, - Secret::SecretManager& secret_manager) +ClientContextConfigImpl::ClientContextConfigImpl( + const Json::Object& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) : ClientContextConfigImpl( [&config] { envoy::api::v2::auth::UpstreamTlsContext upstream_tls_context; Config::TlsContextJson::translateUpstreamTlsContext(config, upstream_tls_context); return upstream_tls_context; }(), - secret_manager) {} + factory_context) {} ServerContextConfigImpl::ServerContextConfigImpl( - const envoy::api::v2::auth::DownstreamTlsContext& config, Secret::SecretManager& secret_manager) - : ContextConfigImpl(config.common_tls_context(), secret_manager), + const envoy::api::v2::auth::DownstreamTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) + : ContextConfigImpl(config.common_tls_context(), factory_context), require_client_certificate_( PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, require_client_certificate, false)), session_ticket_keys_([&config] { @@ -180,15 +187,16 @@ ServerContextConfigImpl::ServerContextConfigImpl( } } -ServerContextConfigImpl::ServerContextConfigImpl(const Json::Object& config, - Secret::SecretManager& secret_manager) +ServerContextConfigImpl::ServerContextConfigImpl( + const Json::Object& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) : ServerContextConfigImpl( [&config] { envoy::api::v2::auth::DownstreamTlsContext downstream_tls_context; Config::TlsContextJson::translateDownstreamTlsContext(config, downstream_tls_context); return downstream_tls_context; }(), - secret_manager) {} + factory_context) {} // Append a SessionTicketKey to keys, initializing it with key_data. // Throws if key_data is invalid. diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index cb6b1c92c9698..c59b23a64b057 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -4,8 +4,9 @@ #include #include "envoy/api/v2/auth/cert.pb.h" -#include "envoy/secret/secret_manager.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/secret/secret_provider.h" +#include "envoy/server/transport_socket_config.h" #include "envoy/ssl/context_config.h" #include "common/common/empty_string.h" @@ -18,6 +19,13 @@ static const std::string INLINE_STRING = ""; class ContextConfigImpl : public virtual Ssl::ContextConfig { public: + ~ContextConfigImpl() override { + if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { + tls_certficate_provider_->removeUpdateCallback(*secret_callback_); + secret_callback_ = nullptr; + } + } + // Ssl::ContextConfig const std::string& alpnProtocols() const override { return alpn_protocols_; } const std::string& altAlpnProtocols() const override { return alt_alpn_protocols_; } @@ -51,15 +59,35 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { unsigned minProtocolVersion() const override { return min_protocol_version_; }; unsigned maxProtocolVersion() const override { return max_protocol_version_; }; + bool isReady() const override { + // either tls_certficate_provider_ is nullptr or + // tls_certficate_provider_->secret() is NOT nullptr. + return !tls_certficate_provider_ || tls_certficate_provider_->secret(); + } + + void setUpdateCallback(Secret::SecretCallbacks& callback) override { + if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { + tls_certficate_provider_->removeUpdateCallback(*secret_callback_); + } + secret_callback_ = &callback; + if (tls_certficate_provider_.get() != nullptr) { + tls_certficate_provider_->addUpdateCallback(callback); + } + } + protected: ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContext& config, - Secret::SecretManager& secret_manager); + Server::Configuration::TransportSocketFactoryContext& factory_context); private: static unsigned tlsVersionFromProto(const envoy::api::v2::auth::TlsParameters_TlsProtocol& version, unsigned default_version); + Secret::TlsCertificateConfigProviderSharedPtr getTlsCertificateConfigProvider( + const envoy::api::v2::auth::CommonTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context); + static const std::string DEFAULT_CIPHER_SUITES; static const std::string DEFAULT_ECDH_CURVES; @@ -71,6 +99,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string ca_cert_path_; const std::string certificate_revocation_list_; const std::string certificate_revocation_list_path_; + Secret::SecretCallbacks* secret_callback_; Secret::TlsCertificateConfigProviderSharedPtr tls_certficate_provider_; const std::vector verify_subject_alt_name_list_; const std::vector verify_certificate_hash_list_; @@ -82,10 +111,12 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { class ClientContextConfigImpl : public ContextConfigImpl, public ClientContextConfig { public: - explicit ClientContextConfigImpl(const envoy::api::v2::auth::UpstreamTlsContext& config, - Secret::SecretManager& secret_manager); - explicit ClientContextConfigImpl(const Json::Object& config, - Secret::SecretManager& secret_manager); + explicit ClientContextConfigImpl( + const envoy::api::v2::auth::UpstreamTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context); + explicit ClientContextConfigImpl( + const Json::Object& config, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context); // Ssl::ClientContextConfig const std::string& serverNameIndication() const override { return server_name_indication_; } @@ -98,10 +129,12 @@ class ClientContextConfigImpl : public ContextConfigImpl, public ClientContextCo class ServerContextConfigImpl : public ContextConfigImpl, public ServerContextConfig { public: - explicit ServerContextConfigImpl(const envoy::api::v2::auth::DownstreamTlsContext& config, - Secret::SecretManager& secret_manager); - explicit ServerContextConfigImpl(const Json::Object& config, - Secret::SecretManager& secret_manager); + explicit ServerContextConfigImpl( + const envoy::api::v2::auth::DownstreamTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context); + explicit ServerContextConfigImpl( + const Json::Object& config, + Server::Configuration::TransportSocketFactoryContext& secret_provider_context); // Ssl::ServerContextConfig bool requireClientCertificate() const override { return require_client_certificate_; } diff --git a/source/common/ssl/context_manager_impl.cc b/source/common/ssl/context_manager_impl.cc index 0409a608332ba..2b5958a8152a6 100644 --- a/source/common/ssl/context_manager_impl.cc +++ b/source/common/ssl/context_manager_impl.cc @@ -22,6 +22,10 @@ void ContextManagerImpl::removeEmptyContexts() { ClientContextSharedPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config) { + if (!config.isReady()) { + return nullptr; + } + ClientContextSharedPtr context = std::make_shared(scope, config); removeEmptyContexts(); contexts_.emplace_back(context); @@ -31,6 +35,10 @@ ContextManagerImpl::createSslClientContext(Stats::Scope& scope, const ClientCont ServerContextSharedPtr ContextManagerImpl::createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, const std::vector& server_names) { + if (!config.isReady()) { + return nullptr; + } + ServerContextSharedPtr context = std::make_shared(scope, config, server_names, runtime_); removeEmptyContexts(); diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 27e149dd0c638..1989e6c8ab083 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -402,39 +402,67 @@ std::string SslSocket::subjectLocalCertificate() const { return getSubjectFromCertificate(cert); } +namespace { +SslSocketFactoryStats generateStats(const std::string& prefix, Stats::Scope& store) { + return { + ALL_SSL_SOCKET_FACTORY_STATS(POOL_COUNTER_PREFIX(store, prefix + "_ssl_socket_factory."))}; +} +} // namespace + ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope) - : manager_(manager), stats_scope_(stats_scope), config_(std::move(config)), - ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {} + : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), + config_(std::move(config)), + ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { + config_->setUpdateCallback(*this); +} Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); } else { + ENVOY_LOG(debug, "Create NotReadySslSocket"); + stats_.upstream_connection_reset_by_sds_.inc(); return std::make_unique(); } } bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } +void ClientSslSocketFactory::onAddOrUpdateSecret() { + ENVOY_LOG(debug, "Secret is updated."); + ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); + stats_.ssl_context_update_by_sds_.inc(); +} + ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names) - : manager_(manager), stats_scope_(stats_scope), config_(std::move(config)), - server_names_(server_names), - ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, 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_)) { + config_->setUpdateCallback(*this); +} Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); } else { + ENVOY_LOG(debug, "Create NotReadySslSocket"); + stats_.downstream_connection_reset_by_sds_.inc(); return std::make_unique(); } } bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } +void ServerSslSocketFactory::onAddOrUpdateSecret() { + ENVOY_LOG(debug, "Secret is updated."); + ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); + stats_.ssl_context_update_by_sds_.inc(); +} + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index c6b8f92b4d63f..b195e3d4ed7d0 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -5,7 +5,9 @@ #include "envoy/network/connection.h" #include "envoy/network/transport_socket.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" #include "common/ssl/context_impl.h" @@ -15,6 +17,20 @@ namespace Envoy { namespace Ssl { +// clang-format off +#define ALL_SSL_SOCKET_FACTORY_STATS(COUNTER) \ + COUNTER(ssl_context_update_by_sds) \ + COUNTER(upstream_connection_reset_by_sds) \ + COUNTER(downstream_connection_reset_by_sds) +// clang-format on + +/** + * Wrapper struct for SSL socket factory stats. @see stats_macros.h + */ +struct SslSocketFactoryStats { + ALL_SSL_SOCKET_FACTORY_STATS(GENERATE_COUNTER_STRUCT) +}; + enum class InitialState { Client, Server }; class SslSocket : public Network::TransportSocket, @@ -67,7 +83,9 @@ class SslSocket : public Network::TransportSocket, mutable std::string cached_url_encoded_pem_encoded_peer_certificate_; }; -class ClientSslSocketFactory : public Network::TransportSocketFactory { +class ClientSslSocketFactory : public Network::TransportSocketFactory, + public Secret::SecretCallbacks, + Logger::Loggable { public: ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope); @@ -75,14 +93,20 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory { Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; + // Secret::SecretCallbacks + void onAddOrUpdateSecret() override; + private: Ssl::ContextManager& manager_; Stats::Scope& stats_scope_; + SslSocketFactoryStats stats_; ClientContextConfigPtr config_; ClientContextSharedPtr ssl_ctx_; }; -class ServerSslSocketFactory : public Network::TransportSocketFactory { +class ServerSslSocketFactory : public Network::TransportSocketFactory, + public Secret::SecretCallbacks, + Logger::Loggable { public: ServerSslSocketFactory(ServerContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); @@ -90,9 +114,13 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory { Network::TransportSocketPtr createTransportSocket() const override; bool implementsSecureTransport() const override; + // Secret::SecretCallbacks + void onAddOrUpdateSecret() override; + private: Ssl::ContextManager& manager_; Stats::Scope& stats_scope_; + SslSocketFactoryStats stats_; ServerContextConfigPtr config_; const std::vector server_names_; ServerContextSharedPtr ssl_ctx_; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 2d43d7509e810..5dc2bdd54ca63 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -406,6 +406,8 @@ envoy_cc_library( "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", "//source/common/upstream:locality_lib", + "//source/server:init_manager_lib", + "//source/server:transport_socket_config_lib", "@envoy_api//envoy/api/v2/core:base_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", ], diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 6f6f998a40c31..815992a0e306a 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -229,6 +229,7 @@ ClusterInfoConstSharedPtr ProdClusterInfoFactory::createClusterInfo( Envoy::Server::Configuration::TransportSocketFactoryContextImpl factory_context( ssl_context_manager, *scope, cm, local_info, dispatcher, random, stats); + // TODO(JimmyCYJ): Support SDS for HDS cluster. Network::TransportSocketFactoryPtr socket_factory = Upstream::createTransportSocketFactory(cluster, factory_context); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5e3e4fa91c228..529c9bb1257a8 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -508,6 +508,7 @@ ClusterImplBase::ClusterImplBase( Server::Configuration::TransportSocketFactoryContext& factory_context, Stats::ScopePtr&& stats_scope, bool added_via_api) : runtime_(runtime) { + factory_context.setInitManager(init_manager_); auto socket_factory = createTransportSocketFactory(cluster, factory_context); info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), runtime, std::move(socket_factory), @@ -571,6 +572,10 @@ void ClusterImplBase::onPreInitComplete() { } initialization_started_ = true; + init_manager_.initialize([this]() { onInitDone(); }); +} + +void ClusterImplBase::onInitDone() { if (health_checker_ && pending_initialize_health_checks_ == 0) { for (auto& host_set : prioritySet().hostSetsPerPriority()) { pending_initialize_health_checks_ += host_set->hosts().size(); diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index ec864b296ff86..b2028b968edff 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -40,6 +40,8 @@ #include "common/upstream/outlier_detection_impl.h" #include "common/upstream/resource_manager_impl.h" +#include "server/init_manager_impl.h" + namespace Envoy { namespace Upstream { @@ -518,12 +520,21 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable( MessageUtil::downcastAndValidate(message), - context.secretManager()); + context); return std::make_unique( std::move(client_config), context.sslContextManager(), context.statsScope()); } @@ -36,7 +36,7 @@ Network::TransportSocketFactoryPtr DownstreamSslSocketFactory::createTransportSo const std::vector& server_names) { auto server_config = std::make_unique( MessageUtil::downcastAndValidate(message), - context.secretManager()); + context); return std::make_unique( std::move(server_config), context.sslContextManager(), context.statsScope(), server_names); } diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 4e7249b205d5f..c4ece6422abd2 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -231,6 +231,7 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st parent_.server_.sslContextManager(), *listener_scope_, parent_.server_.clusterManager(), parent_.server_.localInfo(), parent_.server_.dispatcher(), parent_.server_.random(), parent_.server_.stats()); + factory_context.setInitManager(initManager()); addFilterChain( PROTOBUF_GET_WRAPPED_OR_DEFAULT(filter_chain_match, destination_port, 0), destination_ips, server_names, filter_chain_match.transport_protocol(), application_protocols, diff --git a/source/server/server.cc b/source/server/server.cc index fae27e9416948..7cfa6656d2a8a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -55,9 +55,9 @@ InstanceImpl::InstanceImpl(Options& options, TimeSource& time_source, dispatcher_(api_->allocateDispatcher(time_source)), singleton_manager_(new Singleton::ManagerImpl()), handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)), - random_generator_(std::move(random_generator)), listener_component_factory_(*this), + random_generator_(std::move(random_generator)), + secret_manager_(new Secret::SecretManagerImpl()), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks, time_source), - secret_manager_(new Secret::SecretManagerImpl()), dns_resolver_(dispatcher_->createDnsResolver({})), access_log_manager_(*api_, *dispatcher_, access_log_lock, store), terminated_(false) { diff --git a/source/server/server.h b/source/server/server.h index 1b9850ee8eb0b..71b4af2a7e2b6 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -210,11 +210,11 @@ class InstanceImpl : Logger::Loggable, public Instance { Network::ConnectionHandlerPtr handler_; Runtime::RandomGeneratorPtr random_generator_; Runtime::LoaderPtr runtime_loader_; + std::unique_ptr secret_manager_; std::unique_ptr ssl_context_manager_; ProdListenerComponentFactory listener_component_factory_; ProdWorkerFactory worker_factory_; std::unique_ptr listener_manager_; - std::unique_ptr secret_manager_; std::unique_ptr config_; Network::DnsResolverSharedPtr dns_resolver_; Event::TimerPtr stat_flush_timer_; diff --git a/source/server/transport_socket_config_impl.h b/source/server/transport_socket_config_impl.h index d5adcd09a575a..c9747cc6b5411 100644 --- a/source/server/transport_socket_config_impl.h +++ b/source/server/transport_socket_config_impl.h @@ -24,12 +24,12 @@ class TransportSocketFactoryContextImpl : public TransportSocketFactoryContext { Stats::Scope& statsScope() const override { return stats_scope_; } - Upstream::ClusterManager& clusterManager() override { return cluster_manager_; } - Secret::SecretManager& secretManager() override { return cluster_manager_.clusterManagerFactory().secretManager(); } + Upstream::ClusterManager& clusterManager() override { return cluster_manager_; } + const LocalInfo::LocalInfo& localInfo() override { return local_info_; } Event::Dispatcher& dispatcher() override { return dispatcher_; } @@ -38,6 +38,10 @@ class TransportSocketFactoryContextImpl : public TransportSocketFactoryContext { Stats::Store& stats() override { return stats_; } + void setInitManager(Init::Manager& init_manager) override { init_manager_ = &init_manager; } + + Init::Manager* initManager() override { return init_manager_; } + private: Ssl::ContextManager& context_manager_; Stats::Scope& stats_scope_; @@ -46,6 +50,7 @@ class TransportSocketFactoryContextImpl : public TransportSocketFactoryContext { Event::Dispatcher& dispatcher_; Envoy::Runtime::RandomGenerator& random_; Stats::Store& stats_; + Init::Manager* init_manager_{}; }; } // namespace Configuration diff --git a/test/common/grpc/BUILD b/test/common/grpc/BUILD index a448f215ef975..bc1084240ad3b 100644 --- a/test/common/grpc/BUILD +++ b/test/common/grpc/BUILD @@ -99,6 +99,7 @@ envoy_cc_test_library( "//source/common/http/http2:conn_pool_lib", "//test/integration:integration_lib", "//test/mocks/local_info:local_info_mocks", + "//test/mocks/server:server_mocks", "//test/proto:helloworld_proto_cc", "//test/test_common:test_time_lib", ], diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 0f0861e5f7cd8..51162fa4f5d51 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -13,7 +13,7 @@ #include "test/integration/fake_upstream.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/local_info/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/proto/helloworld.pb.h" @@ -464,7 +464,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/clientkey.pem")); } - auto cfg = std::make_unique(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, factory_context_); mock_cluster_info_->transport_socket_factory_ = std::make_unique( std::move(cfg), context_manager_, *stats_store_); @@ -494,7 +494,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); } - auto cfg = std::make_unique(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, factory_context_); static Stats::Scope* upstream_stats_store = new Stats::IsolatedStoreImpl(); return std::make_unique( @@ -502,7 +502,7 @@ class GrpcSslClientIntegrationTest : public GrpcClientIntegrationTest { } bool use_client_cert_{}; - NiceMock secret_manager_; + testing::NiceMock factory_context_; }; } // namespace diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index 7f6013a3db8d1..cbb7aff3e74d4 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -15,7 +15,9 @@ envoy_cc_test( "//test/common/ssl/test_data:certs", ], deps = [ + "//source/common/secret:sds_api_lib", "//source/common/secret:secret_manager_impl_lib", + "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", "//test/test_common:registry_lib", "//test/test_common:utility_lib", diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index bf27f559fd4ff..f957d3c301ca5 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -3,14 +3,19 @@ #include "envoy/api/v2/auth/cert.pb.h" #include "envoy/common/exception.h" +#include "common/secret/sds_api.h" #include "common/secret/secret_manager_impl.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::Return; +using testing::ReturnRef; + namespace Envoy { namespace Secret { namespace { @@ -69,6 +74,51 @@ name: "abc.com" "Secret type not implemented"); } +TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { + Server::MockInstance server; + std::unique_ptr secret_manager(new SecretManagerImpl()); + + NiceMock secret_context; + + envoy::api::v2::core::ConfigSource config_source; + { + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + Stats::IsolatedStoreImpl stats; + NiceMock cluster_manager; + NiceMock init_manager; + EXPECT_CALL(secret_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(secret_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(secret_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(secret_context, stats()).WillOnce(ReturnRef(stats)); + EXPECT_CALL(secret_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager)); + + auto secret_provider = + secret_manager->findOrCreateDynamicSecretProvider(config_source, "abc.com", secret_context); + std::string yaml = + R"EOF( +name: "abc.com" +tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + Protobuf::RepeatedPtrField secret_resources; + auto secret_config = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); + std::dynamic_pointer_cast(secret_provider)->onConfigUpdate(secret_resources, ""); + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + secret_provider->secret()->certificateChain()); + const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), + secret_provider->secret()->privateKey()); + } +} + } // namespace } // namespace Secret } // namespace Envoy diff --git a/test/common/ssl/BUILD b/test/common/ssl/BUILD index 91c5364d7fd82..a0a47930f0956 100644 --- a/test/common/ssl/BUILD +++ b/test/common/ssl/BUILD @@ -36,7 +36,6 @@ envoy_cc_test( "//test/mocks/buffer:buffer_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", - "//test/mocks/secret:secret_mocks", "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", "//test/test_common:environment_lib", @@ -58,13 +57,13 @@ envoy_cc_test( ], deps = [ "//source/common/json:json_loader_lib", - "//source/common/secret:secret_manager_impl_lib", "//source/common/ssl:context_config_lib", "//source/common/ssl:context_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", "//test/mocks/runtime:runtime_mocks", "//test/mocks/secret:secret_mocks", + "//test/mocks/server:server_mocks", "//test/test_common:environment_lib", ], ) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 4c533ea17c196..70dcc0e9badcf 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -2,7 +2,7 @@ #include #include "common/json/json_loader.h" -#include "common/secret/secret_manager_impl.h" +#include "common/secret/sds_api.h" #include "common/ssl/context_config_impl.h" #include "common/ssl/context_impl.h" #include "common/stats/isolated_store_impl.h" @@ -10,12 +10,14 @@ #include "test/common/ssl/ssl_certs_test.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" using testing::NiceMock; +using testing::ReturnRef; namespace Envoy { namespace Ssl { @@ -81,7 +83,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, factory_context_); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -100,7 +102,7 @@ TEST_F(SslContextImplTest, TestExpiringCert) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, factory_context_); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -123,7 +125,7 @@ TEST_F(SslContextImplTest, TestExpiredCert) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, factory_context_); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -141,7 +143,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, factory_context_); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -167,7 +169,7 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { TEST_F(SslContextImplTest, TestNoCert) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString("{}"); - ClientContextConfigImpl cfg(*loader, secret_manager_); + ClientContextConfigImpl cfg(*loader, factory_context_); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -180,7 +182,6 @@ class SslServerContextImplTicketTest : public SslContextImplTest { public: static void loadConfig(ServerContextConfigImpl& cfg) { Runtime::MockLoader runtime; - NiceMock secret_manager; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; ServerContextSharedPtr server_ctx( @@ -196,15 +197,15 @@ class SslServerContextImplTicketTest : public SslContextImplTest { server_cert->mutable_private_key()->set_filename( TestEnvironment::substitute("{{ test_tmpdir }}/unittestkey.pem")); - NiceMock secret_manager; - ServerContextConfigImpl server_context_config(cfg, secret_manager); + NiceMock factory_context; + ServerContextConfigImpl server_context_config(cfg, factory_context); loadConfig(server_context_config); } static void loadConfigJson(const std::string& json) { Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - NiceMock secret_manager; - ServerContextConfigImpl cfg(*loader, secret_manager); + NiceMock factory_context; + ServerContextConfigImpl cfg(*loader, factory_context); loadConfig(cfg); } }; @@ -361,28 +362,28 @@ class ClientContextConfigImplTest : public SslCertsTest {}; // Validate that empty SNI (according to C string rules) fails config validation. TEST(ClientContextConfigImplTest, EmptyServerNameIndication) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; tls_context.set_sni(std::string("\000", 1)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, "SNI names containing NULL-byte are not allowed"); tls_context.set_sni(std::string("a\000b", 3)); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, "SNI names containing NULL-byte are not allowed"); } // Validate that values other than a hex-encoded SHA-256 fail config validation. TEST(ClientContextConfigImplTest, InvalidCertificateHash) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; tls_context.mutable_common_tls_context() ->mutable_validation_context() // This is valid hex-encoded string, but it doesn't represent SHA-256 (80 vs 64 chars). ->add_verify_certificate_hash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); - ClientContextConfigImpl client_context_config(tls_context, secret_manager); + ClientContextConfigImpl client_context_config(tls_context, factory_context); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -393,12 +394,12 @@ TEST(ClientContextConfigImplTest, InvalidCertificateHash) { // Validate that values other than a base64-encoded SHA-256 fail config validation. TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; tls_context.mutable_common_tls_context() ->mutable_validation_context() // Not a base64-encoded string. ->add_verify_certificate_spki("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); - ClientContextConfigImpl client_context_config(tls_context, secret_manager); + ClientContextConfigImpl client_context_config(tls_context, factory_context); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -410,14 +411,48 @@ TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { // TODO(PiotrSikora): Support multiple TLS certificates. TEST(ClientContextConfigImplTest, MultipleTlsCertificates) { envoy::api::v2::auth::UpstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, "Multiple TLS certificates are not supported for client contexts"); } +TEST(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { + envoy::api::v2::auth::UpstreamTlsContext tls_context; + NiceMock factory_context; + tls_context.mutable_common_tls_context()->add_tls_certificates(); + tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); + EXPECT_THROW_WITH_MESSAGE( + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, + "Multiple TLS certificates are not supported for client contexts"); +} + +TEST(ClientContextConfigImplTest, SecretNotReady) { + envoy::api::v2::auth::UpstreamTlsContext tls_context; + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + Stats::IsolatedStoreImpl stats; + NiceMock cluster_manager; + NiceMock init_manager; + NiceMock factory_context; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats)); + EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + ClientContextConfigImpl client_context_config(tls_context, factory_context); + // When sds secret is not downloaded, config is not ready. + EXPECT_FALSE(client_context_config.isReady()); +} + TEST(ClientContextConfigImplTest, StaticTlsCertificates) { envoy::api::v2::auth::Secret secret_config; @@ -432,16 +467,15 @@ name: "abc.com" MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - secret_manager->addStaticSecret(secret_config); - envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() ->mutable_tls_certificate_sds_secret_configs() ->Add() ->set_name("abc.com"); - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()); + NiceMock factory_context; + factory_context.secretManager().addStaticSecret(secret_config); + ClientContextConfigImpl client_context_config(tls_context, factory_context); const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), @@ -465,9 +499,8 @@ name: "abc.com" MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - - secret_manager->addStaticSecret(secret_config); + NiceMock factory_context; + factory_context.secretManager().addStaticSecret(secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() @@ -476,8 +509,8 @@ name: "abc.com" ->set_name("missing"); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()), - EnvoyException, "Static secret is not defined: missing"); + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, + "Unknown static secret: missing"); } // Multiple TLS certificates are not yet supported, but one is expected for @@ -485,23 +518,60 @@ name: "abc.com" // TODO(PiotrSikora): Support multiple TLS certificates. TEST(ServerContextConfigImplTest, MultipleTlsCertificates) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, "A single TLS certificate is required for server contexts"); tls_context.mutable_common_tls_context()->add_tls_certificates(); tls_context.mutable_common_tls_context()->add_tls_certificates(); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl client_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, "A single TLS certificate is required for server contexts"); } +TEST(ServerContextConfigImplTest, TlsCertificatesAndSdsConfig) { + envoy::api::v2::auth::DownstreamTlsContext tls_context; + NiceMock factory_context; + EXPECT_THROW_WITH_MESSAGE( + ServerContextConfigImpl server_context_config(tls_context, factory_context), EnvoyException, + "A single TLS certificate is required for server contexts"); + tls_context.mutable_common_tls_context()->add_tls_certificates(); + tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); + EXPECT_THROW_WITH_MESSAGE( + ServerContextConfigImpl server_context_config(tls_context, factory_context), EnvoyException, + "A single TLS certificate is required for server contexts"); +} + +TEST(ServerContextConfigImplTest, SecretNotReady) { + envoy::api::v2::auth::DownstreamTlsContext tls_context; + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + Stats::IsolatedStoreImpl stats; + NiceMock cluster_manager; + NiceMock init_manager; + NiceMock factory_context; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats)); + EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + ServerContextConfigImpl server_context_config(tls_context, factory_context); + // When sds secret is not downloaded, config is not ready. + EXPECT_FALSE(server_context_config.isReady()); +} + // TlsCertificate messages must have a cert for servers. TEST(ServerContextImplTest, TlsCertificateNonEmpty) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; tls_context.mutable_common_tls_context()->add_tls_certificates(); - ServerContextConfigImpl client_context_config(tls_context, secret_manager); + ServerContextConfigImpl client_context_config(tls_context, factory_context); Runtime::MockLoader runtime; ContextManagerImpl manager(runtime); Stats::IsolatedStoreImpl store; @@ -514,7 +584,7 @@ TEST(ServerContextImplTest, TlsCertificateNonEmpty) { // Cannot ignore certificate expiration without a trusted CA. TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { envoy::api::v2::auth::DownstreamTlsContext tls_context; - NiceMock secret_manager; + NiceMock factory_context; envoy::api::v2::auth::CertificateValidationContext* server_validation_ctx = tls_context.mutable_common_tls_context()->mutable_validation_context(); @@ -522,7 +592,7 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl server_context_config(tls_context, factory_context), EnvoyException, "Certificate validity period is always ignored without trusted CA"); envoy::api::v2::auth::TlsCertificate* server_cert = @@ -534,19 +604,19 @@ TEST(ServerContextConfigImplTest, InvalidIgnoreCertsNoCA) { server_validation_ctx->set_allow_expired_certificate(false); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, secret_manager)); + EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, factory_context)); server_validation_ctx->set_allow_expired_certificate(true); EXPECT_THROW_WITH_MESSAGE( - ServerContextConfigImpl server_context_config(tls_context, secret_manager), EnvoyException, + ServerContextConfigImpl server_context_config(tls_context, factory_context), EnvoyException, "Certificate validity period is always ignored without trusted CA"); // But once you add a trusted CA, you should be able to create the context. server_validation_ctx->mutable_trusted_ca()->set_filename( TestEnvironment::substitute("{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem")); - EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, secret_manager)); + EXPECT_NO_THROW(ServerContextConfigImpl server_context_config(tls_context, factory_context)); } } // namespace Ssl diff --git a/test/common/ssl/ssl_certs_test.h b/test/common/ssl/ssl_certs_test.h index 37b85cad3f458..ad345ef1b2c2a 100644 --- a/test/common/ssl/ssl_certs_test.h +++ b/test/common/ssl/ssl_certs_test.h @@ -1,6 +1,6 @@ #pragma once -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "gtest/gtest.h" @@ -12,6 +12,6 @@ class SslCertsTest : public testing::Test { TestEnvironment::exec({TestEnvironment::runfilesPath("test/common/ssl/gen_unittest_certs.sh")}); } - testing::NiceMock secret_manager_; + testing::NiceMock factory_context_; }; } // namespace Envoy diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 5d00bf3356584..4e79e5be24d11 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -51,10 +51,10 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ bool expect_success, const Network::Address::IpVersion version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - NiceMock secret_manager; + testing::NiceMock factory_context; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -68,7 +68,7 @@ void testUtil(const std::string& client_ctx_json, const std::string& server_ctx_ Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + auto client_cfg = std::make_unique(*client_ctx_loader, factory_context); Ssl::ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( @@ -149,7 +149,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, const Network::Address::IpVersion version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - NiceMock secret_manager; + testing::NiceMock factory_context; ContextManagerImpl manager(runtime); std::string new_session = EMPTY_STRING; @@ -159,7 +159,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, std::vector server_names(filter_chain.filter_chain_match().server_names().begin(), filter_chain.filter_chain_match().server_names().end()); auto server_cfg = - std::make_unique(filter_chain.tls_context(), secret_manager); + std::make_unique(filter_chain.tls_context(), factory_context); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, server_names); @@ -171,7 +171,7 @@ const std::string testUtilV2(const envoy::api::v2::Listener& server_proto, Network::MockConnectionHandler connection_handler; Network::ListenerPtr listener = dispatcher.createListener(socket, callbacks, true, false); - auto client_cfg = std::make_unique(client_ctx_proto, secret_manager); + auto client_cfg = std::make_unique(client_ctx_proto, factory_context); ClientContextSharedPtr client_ctx(manager.createSslClientContext(stats_store, *client_cfg)); ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( @@ -1546,7 +1546,7 @@ TEST_P(SslSocketTest, FlushCloseDuringHandshake) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context_); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -1603,7 +1603,7 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context_); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -1623,7 +1623,7 @@ TEST_P(SslSocketTest, HalfClose) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager_); + auto client_cfg = std::make_unique(*client_ctx_loader, factory_context_); ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -1675,7 +1675,7 @@ TEST_P(SslSocketTest, HalfClose) { TEST_P(SslSocketTest, ClientAuthMultipleCAs) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - NiceMock secret_manager; + testing::NiceMock factory_context; std::string server_ctx_json = R"EOF( { @@ -1686,7 +1686,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -1705,7 +1705,7 @@ TEST_P(SslSocketTest, ClientAuthMultipleCAs) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + auto client_cfg = std::make_unique(*client_ctx_loader, factory_context); ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -1762,13 +1762,15 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, const Network::Address::IpVersion ip_version) { Stats::IsolatedStoreImpl stats_store; Runtime::MockLoader runtime; - NiceMock secret_manager; + testing::NiceMock factory_context; ContextManagerImpl manager(runtime); Json::ObjectSharedPtr server_ctx_loader1 = TestEnvironment::jsonLoadFromString(server_ctx_json1); Json::ObjectSharedPtr server_ctx_loader2 = TestEnvironment::jsonLoadFromString(server_ctx_json2); - auto server_cfg1 = std::make_unique(*server_ctx_loader1, secret_manager); - auto server_cfg2 = std::make_unique(*server_ctx_loader2, secret_manager); + auto server_cfg1 = + std::make_unique(*server_ctx_loader1, factory_context); + auto server_cfg2 = + std::make_unique(*server_ctx_loader2, factory_context); Ssl::ServerSslSocketFactory server_ssl_socket_factory1(std::move(server_cfg1), manager, stats_store, server_names1); Ssl::ServerSslSocketFactory server_ssl_socket_factory2(std::move(server_cfg2), manager, @@ -1786,7 +1788,7 @@ void testTicketSessionResumption(const std::string& server_ctx_json1, Network::ListenerPtr listener2 = dispatcher.createListener(socket2, callbacks, true, false); Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager); + auto client_cfg = std::make_unique(*client_ctx_loader, factory_context); ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher.createClientConnection( socket1.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2127,10 +2129,10 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context_); Json::ObjectSharedPtr server2_ctx_loader = TestEnvironment::jsonLoadFromString(server2_ctx_json); auto server2_cfg = - std::make_unique(*server2_ctx_loader, secret_manager_); + std::make_unique(*server2_ctx_loader, factory_context_); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -2154,7 +2156,7 @@ TEST_P(SslSocketTest, ClientAuthCrossListenerSessionResumption) { )EOF"; Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - auto client_cfg = std::make_unique(*client_ctx_loader, secret_manager_); + auto client_cfg = std::make_unique(*client_ctx_loader, factory_context_); ClientSslSocketFactory ssl_socket_factory(std::move(client_cfg), manager, stats_store); Network::ClientConnectionPtr client_connection = dispatcher_->createClientConnection( socket.localAddress(), Network::Address::InstanceConstSharedPtr(), @@ -2241,7 +2243,7 @@ TEST_P(SslSocketTest, SslError) { )EOF"; Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - auto server_cfg = std::make_unique(*server_ctx_loader, secret_manager_); + auto server_cfg = std::make_unique(*server_ctx_loader, factory_context_); ContextManagerImpl manager(runtime); Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, std::vector{}); @@ -2583,7 +2585,7 @@ class SslReadBufferLimitTest : public SslSocketTest { void initialize() { server_ctx_loader_ = TestEnvironment::jsonLoadFromString(server_ctx_json_); auto server_cfg = - std::make_unique(*server_ctx_loader_, secret_manager_); + std::make_unique(*server_ctx_loader_, factory_context_); manager_.reset(new ContextManagerImpl(runtime_)); server_ssl_socket_factory_.reset(new ServerSslSocketFactory( std::move(server_cfg), *manager_, stats_store_, std::vector{})); @@ -2592,7 +2594,7 @@ class SslReadBufferLimitTest : public SslSocketTest { client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); auto client_cfg = - std::make_unique(*client_ctx_loader_, secret_manager_); + std::make_unique(*client_ctx_loader_, factory_context_); client_ssl_socket_factory_.reset( new ClientSslSocketFactory(std::move(client_cfg), *manager_, stats_store_)); diff --git a/test/integration/BUILD b/test/integration/BUILD index 6a7aaaa48e0f6..8848ce4b98fe5 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -34,7 +34,7 @@ envoy_cc_test( "//source/extensions/transport_sockets/ssl:config", "//test/common/grpc:grpc_client_integration_lib", "//test/mocks/runtime:runtime_mocks", - "//test/mocks/secret:secret_mocks", + "//test/mocks/server:server_mocks", "//test/test_common:network_utility_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:cds_cc", @@ -463,6 +463,35 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "sds_dynamic_integration_test", + srcs = [ + "sds_dynamic_integration_test.cc", + ], + data = [ + "//test/config/integration/certs", + ], + deps = [ + ":http_integration_lib", + "//source/common/config:protobuf_link_hacks", + "//source/common/config:resources_lib", + "//source/common/event:dispatcher_includes", + "//source/common/event:dispatcher_lib", + "//source/common/network:connection_lib", + "//source/common/network:utility_lib", + "//source/common/ssl:context_config_lib", + "//source/common/ssl:context_lib", + "//source/extensions/filters/listener/tls_inspector:config", + "//source/extensions/transport_sockets/ssl:config", + "//test/common/grpc:grpc_client_integration_lib", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/secret:secret_mocks", + "//test/test_common:utility_lib", + "@envoy_api//envoy/config/transport_socket/capture/v2alpha:capture_cc", + "@envoy_api//envoy/data/tap/v2alpha:capture_cc", + ], +) + envoy_cc_test( name = "ssl_integration_test", srcs = [ @@ -568,7 +597,7 @@ envoy_cc_test( "//source/common/http:header_map_lib", "//source/extensions/filters/listener/tls_inspector:config", "//source/extensions/transport_sockets/ssl:config", - "//test/mocks/secret:secret_mocks", + "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", ], ) diff --git a/test/integration/ads_integration_test.cc b/test/integration/ads_integration_test.cc index 37239700ede4b..0e81ec82b0037 100644 --- a/test/integration/ads_integration_test.cc +++ b/test/integration/ads_integration_test.cc @@ -21,7 +21,7 @@ #include "test/integration/http_integration.h" #include "test/integration/utility.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" #include "test/test_common/utility.h" @@ -117,7 +117,7 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest, TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcert.pem")); tls_cert->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/upstreamkey.pem")); - auto cfg = std::make_unique(tls_context, secret_manager_); + auto cfg = std::make_unique(tls_context, factory_context_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( @@ -299,7 +299,7 @@ class AdsIntegrationTest : public AdsIntegrationBaseTest, return dynamic_cast(*message_ptr); } - testing::NiceMock secret_manager_; + testing::NiceMock factory_context_; Runtime::MockLoader runtime_; Ssl::ContextManagerImpl context_manager_{runtime_}; FakeStreamPtr ads_stream_; diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 1761bfd006385..a3451fb4cdcac 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -80,7 +80,6 @@ IntegrationCodecClient::IntegrationCodecClient( connection_->addConnectionCallbacks(callbacks_); setCodecConnectionCallbacks(codec_callbacks_); dispatcher.run(Event::Dispatcher::RunType::Block); - EXPECT_TRUE(connected_); } void IntegrationCodecClient::flushWrite() { @@ -170,7 +169,7 @@ IntegrationCodecClientPtr HttpIntegrationTest::makeHttpConnection(uint32_t port) } IntegrationCodecClientPtr -HttpIntegrationTest::makeHttpConnection(Network::ClientConnectionPtr&& conn) { +HttpIntegrationTest::makeRawHttpConnection(Network::ClientConnectionPtr&& conn) { std::shared_ptr cluster{new NiceMock()}; Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription( cluster, fmt::format("tcp://{}:80", Network::Test::getLoopbackAddressUrlString(version_)))}; @@ -178,6 +177,13 @@ HttpIntegrationTest::makeHttpConnection(Network::ClientConnectionPtr&& conn) { *dispatcher_, std::move(conn), host_description, downstream_protocol_)}; } +IntegrationCodecClientPtr +HttpIntegrationTest::makeHttpConnection(Network::ClientConnectionPtr&& conn) { + auto codec = makeRawHttpConnection(std::move(conn)); + EXPECT_TRUE(codec->connected()); + return codec; +} + HttpIntegrationTest::HttpIntegrationTest(Http::CodecClient::Type downstream_protocol, Network::Address::IpVersion version, const std::string& config) diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 6d86ef8df5d7d..b3f342ba50719 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -25,7 +25,8 @@ class IntegrationCodecClient : public Http::CodecClientProd { IntegrationStreamDecoderPtr makeHeaderOnlyRequest(const Http::HeaderMap& headers); IntegrationStreamDecoderPtr makeRequestWithBody(const Http::HeaderMap& headers, uint64_t body_size); - bool sawGoAway() { return saw_goaway_; } + bool sawGoAway() const { return saw_goaway_; } + bool connected() const { return connected_; } void sendData(Http::StreamEncoder& encoder, absl::string_view data, bool end_stream); void sendData(Http::StreamEncoder& encoder, Buffer::Instance& data, bool end_stream); void sendData(Http::StreamEncoder& encoder, uint64_t size, bool end_stream); @@ -81,6 +82,9 @@ class HttpIntegrationTest : public BaseIntegrationTest { protected: IntegrationCodecClientPtr makeHttpConnection(uint32_t port); + // Makes a http connection object without checking its connected state. + IntegrationCodecClientPtr makeRawHttpConnection(Network::ClientConnectionPtr&& conn); + // Makes a http connection object with asserting a connected state. IntegrationCodecClientPtr makeHttpConnection(Network::ClientConnectionPtr&& conn); // Sets downstream_protocol_ and alters the HTTP connection manager codec type in the diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index e3a24c3c42989..cc259d31ffe7f 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -15,6 +15,7 @@ #include "test/mocks/init/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/network_utility.h" #include "test/test_common/utility.h" @@ -68,8 +69,7 @@ class SdsStaticDownstreamIntegrationTest registerTestServerPorts({"http"}); - client_ssl_ctx_ = - createClientSslTransportSocketFactory(false, false, context_manager_, secret_manager_); + client_ssl_ctx_ = createClientSslTransportSocketFactory(false, false, context_manager_); } void TearDown() override { @@ -88,7 +88,6 @@ class SdsStaticDownstreamIntegrationTest private: Runtime::MockLoader runtime_; Ssl::ContextManagerImpl context_manager_{runtime_}; - NiceMock secret_manager_; Network::TransportSocketFactoryPtr client_ssl_ctx_; }; @@ -144,41 +143,13 @@ class SdsStaticUpstreamIntegrationTest } void createUpstreams() override { - fake_upstreams_.emplace_back( - new FakeUpstream(createUpstreamSslContext(), 0, FakeHttpConnection::Type::HTTP1, version_)); - } - - Network::TransportSocketFactoryPtr createUpstreamSslContext() { - envoy::api::v2::auth::DownstreamTlsContext tls_context; - auto* common_tls_context = tls_context.mutable_common_tls_context(); - common_tls_context->add_alpn_protocols("h2"); - common_tls_context->add_alpn_protocols("http/1.1"); - common_tls_context->mutable_deprecated_v1()->set_alt_alpn_protocols("http/1.1"); - - auto* validation_context = common_tls_context->mutable_validation_context(); - validation_context->mutable_trusted_ca()->set_filename( - TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); - validation_context->add_verify_certificate_hash( - "E0:F3:C8:CE:5E:2E:A3:05:F0:70:1F:F5:12:E3:6E:2E:" - "97:92:82:84:A2:28:BC:F7:73:32:D3:39:30:A1:B6:FD"); - - auto* tls_certificate = common_tls_context->add_tls_certificates(); - tls_certificate->mutable_certificate_chain()->set_filename( - TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem")); - tls_certificate->mutable_private_key()->set_filename( - TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); - - auto cfg = std::make_unique(tls_context, secret_manager_); - - static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); - return std::make_unique( - std::move(cfg), context_manager_, *upstream_stats_store, std::vector{}); + fake_upstreams_.emplace_back(new FakeUpstream(createUpstreamSslContext(context_manager_), 0, + FakeHttpConnection::Type::HTTP1, version_)); } private: Runtime::MockLoader runtime_; Ssl::ContextManagerImpl context_manager_{runtime_}; - NiceMock secret_manager_; }; INSTANTIATE_TEST_CASE_P(IpVersions, SdsStaticUpstreamIntegrationTest, diff --git a/test/integration/ssl_integration_test.cc b/test/integration/ssl_integration_test.cc index 3ed603e9b37c6..f72f0e7c518c0 100644 --- a/test/integration/ssl_integration_test.cc +++ b/test/integration/ssl_integration_test.cc @@ -35,14 +35,10 @@ void SslIntegrationTest::initialize() { context_manager_.reset(new ContextManagerImpl(*runtime_)); registerTestServerPorts({"http"}); - client_ssl_ctx_plain_ = - createClientSslTransportSocketFactory(false, false, *context_manager_, secret_manager_); - client_ssl_ctx_alpn_ = - createClientSslTransportSocketFactory(true, false, *context_manager_, secret_manager_); - client_ssl_ctx_san_ = - createClientSslTransportSocketFactory(false, true, *context_manager_, secret_manager_); - client_ssl_ctx_alpn_san_ = - createClientSslTransportSocketFactory(true, true, *context_manager_, secret_manager_); + client_ssl_ctx_plain_ = createClientSslTransportSocketFactory(false, false, *context_manager_); + client_ssl_ctx_alpn_ = createClientSslTransportSocketFactory(true, false, *context_manager_); + client_ssl_ctx_san_ = createClientSslTransportSocketFactory(false, true, *context_manager_); + client_ssl_ctx_alpn_san_ = createClientSslTransportSocketFactory(true, true, *context_manager_); } void SslIntegrationTest::TearDown() { diff --git a/test/integration/ssl_integration_test.h b/test/integration/ssl_integration_test.h index 73f96c514a201..a883d3b9c3cb8 100644 --- a/test/integration/ssl_integration_test.h +++ b/test/integration/ssl_integration_test.h @@ -32,7 +32,6 @@ class SslIntegrationTest : public HttpIntegrationTest, private: std::unique_ptr runtime_; std::unique_ptr context_manager_; - NiceMock secret_manager_; Network::TransportSocketFactoryPtr client_ssl_ctx_plain_; Network::TransportSocketFactoryPtr client_ssl_ctx_alpn_; diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index bb6ccb9f20b10..a5f7e71daa798 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -7,6 +7,7 @@ #include "common/ssl/ssl_socket.h" #include "test/integration/server.h" +#include "test/mocks/server/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" @@ -14,8 +15,7 @@ namespace Envoy { namespace Ssl { Network::TransportSocketFactoryPtr -createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager, - Secret::SecretManager& secret_manager) { +createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager) { const std::string json_plain = R"EOF( { "ca_cert_file": "{{ test_rundir }}/test/config/integration/certs/cacert.pem", @@ -59,12 +59,41 @@ createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& conte target = san ? json_san : json_plain; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - auto cfg = std::make_unique(*loader, secret_manager); + NiceMock mock_factory_ctx; + auto cfg = std::make_unique(*loader, mock_factory_ctx); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ new Ssl::ClientSslSocketFactory(std::move(cfg), context_manager, *client_stats_store)}; } +Network::TransportSocketFactoryPtr createUpstreamSslContext(ContextManager& context_manager) { + envoy::api::v2::auth::DownstreamTlsContext tls_context; + auto* common_tls_context = tls_context.mutable_common_tls_context(); + common_tls_context->add_alpn_protocols("h2"); + common_tls_context->add_alpn_protocols("http/1.1"); + common_tls_context->mutable_deprecated_v1()->set_alt_alpn_protocols("http/1.1"); + + auto* validation_context = common_tls_context->mutable_validation_context(); + validation_context->mutable_trusted_ca()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); + validation_context->add_verify_certificate_hash( + "E0:F3:C8:CE:5E:2E:A3:05:F0:70:1F:F5:12:E3:6E:2E:" + "97:92:82:84:A2:28:BC:F7:73:32:D3:39:30:A1:B6:FD"); + + auto* tls_certificate = common_tls_context->add_tls_certificates(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); + + NiceMock mock_factory_ctx; + auto cfg = std::make_unique(tls_context, mock_factory_ctx); + + static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); + return std::make_unique( + std::move(cfg), context_manager, *upstream_stats_store, std::vector{}); +} + Network::Address::InstanceConstSharedPtr getSslAddress(const Network::Address::IpVersion& version, int port) { std::string url = diff --git a/test/integration/ssl_utility.h b/test/integration/ssl_utility.h index d2ff42561bd44..268004dc756d0 100644 --- a/test/integration/ssl_utility.h +++ b/test/integration/ssl_utility.h @@ -9,8 +9,8 @@ namespace Envoy { namespace Ssl { Network::TransportSocketFactoryPtr -createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager, - Secret::SecretManager& secret_manager); +createClientSslTransportSocketFactory(bool alpn, bool san, ContextManager& context_manager); +Network::TransportSocketFactoryPtr createUpstreamSslContext(ContextManager& context_manager); Network::Address::InstanceConstSharedPtr getSslAddress(const Network::Address::IpVersion& version, int port); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 31fc601e0cc65..c8ebe3c43921c 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -383,8 +383,7 @@ void TcpProxySslIntegrationTest::setupConnections() { // Set up the SSl client. Network::Address::InstanceConstSharedPtr address = Ssl::getSslAddress(version_, lookupPort("tcp_proxy")); - context_ = - Ssl::createClientSslTransportSocketFactory(false, false, *context_manager_, secret_manager_); + context_ = Ssl::createClientSslTransportSocketFactory(false, false, *context_manager_); ssl_client_ = dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(), context_->createTransportSocket(), nullptr); diff --git a/test/integration/tcp_proxy_integration_test.h b/test/integration/tcp_proxy_integration_test.h index 9c75ee45fd924..b136c51c8bc5f 100644 --- a/test/integration/tcp_proxy_integration_test.h +++ b/test/integration/tcp_proxy_integration_test.h @@ -41,7 +41,6 @@ class TcpProxySslIntegrationTest : public TcpProxyIntegrationTest { ConnectionStatusCallbacks connect_callbacks_; MockWatermarkBuffer* client_write_buffer_; std::shared_ptr payload_reader_; - testing::NiceMock secret_manager_; }; } // namespace diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index e4d7c0903979d..0d81e51bfa8d5 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -61,7 +61,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b target = json_tls; } Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(target); - auto cfg = std::make_unique(*loader, secret_manager_); + auto cfg = std::make_unique(*loader, factory_context_); static auto* client_stats_store = new Stats::TestIsolatedStoreImpl(); return Network::TransportSocketFactoryPtr{ new Ssl::ClientSslSocketFactory(std::move(cfg), *context_manager_, *client_stats_store)}; @@ -76,7 +76,7 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createUpstreamSslContext )EOF"; Json::ObjectSharedPtr loader = TestEnvironment::jsonLoadFromString(json); - auto cfg = std::make_unique(*loader, secret_manager_); + auto cfg = std::make_unique(*loader, factory_context_); static Stats::Scope* upstream_stats_store = new Stats::TestIsolatedStoreImpl(); return std::make_unique( std::move(cfg), *context_manager_, *upstream_stats_store, std::vector{}); diff --git a/test/integration/xfcc_integration_test.h b/test/integration/xfcc_integration_test.h index e762b1720f416..d8302ea05816e 100644 --- a/test/integration/xfcc_integration_test.h +++ b/test/integration/xfcc_integration_test.h @@ -6,7 +6,7 @@ #include "test/integration/http_integration.h" #include "test/integration/server.h" #include "test/mocks/runtime/mocks.h" -#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -56,7 +56,7 @@ class XfccIntegrationTest : public HttpIntegrationTest, Network::TransportSocketFactoryPtr client_tls_ssl_ctx_; Network::TransportSocketFactoryPtr client_mtls_ssl_ctx_; Network::TransportSocketFactoryPtr upstream_ssl_ctx_; - testing::NiceMock secret_manager_; + testing::NiceMock factory_context_; }; } // namespace Xfcc } // namespace Envoy diff --git a/test/mocks/secret/BUILD b/test/mocks/secret/BUILD index d3283e265d0d6..43fc682bb7da0 100644 --- a/test/mocks/secret/BUILD +++ b/test/mocks/secret/BUILD @@ -15,6 +15,7 @@ envoy_cc_mock( deps = [ "//include/envoy/secret:secret_callbacks_interface", "//include/envoy/secret:secret_manager_interface", + "//include/envoy/server:transport_socket_config_interface", "//include/envoy/ssl:tls_certificate_config_interface", "//source/common/secret:secret_provider_impl_lib", ], diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 907d7b66e8e57..e2ff48f23ce08 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -2,6 +2,7 @@ #include "envoy/secret/secret_callbacks.h" #include "envoy/secret/secret_manager.h" +#include "envoy/server/transport_socket_config.h" #include "envoy/ssl/tls_certificate_config.h" #include "gmock/gmock.h" @@ -21,6 +22,17 @@ class MockSecretManager : public SecretManager { MOCK_METHOD1(createInlineTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::auth::TlsCertificate& tls_certificate)); + MOCK_METHOD3(findOrCreateDynamicSecretProvider, + TlsCertificateConfigProviderSharedPtr( + const envoy::api::v2::core::ConfigSource&, const std::string&, + Server::Configuration::TransportSocketFactoryContext&)); +}; + +class MockSecretCallbacks : public SecretCallbacks { +public: + MockSecretCallbacks(); + ~MockSecretCallbacks(); + MOCK_METHOD0(onAddOrUpdateSecret, void()); }; class MockSecretCallbacks : public SecretCallbacks { diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 61e3dc6c5cae0..7ea2f3cd4ac1b 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -171,7 +171,9 @@ MockFactoryContext::MockFactoryContext() MockFactoryContext::~MockFactoryContext() {} -MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() {} +MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() + : secret_manager_(new Secret::SecretManagerImpl()) {} + MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() {} MockListenerFactoryContext::MockListenerFactoryContext() {} diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index e0deddecfc33a..bd62535f854f1 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -433,14 +433,19 @@ class MockTransportSocketFactoryContext : public TransportSocketFactoryContext { MockTransportSocketFactoryContext(); ~MockTransportSocketFactoryContext(); + Secret::SecretManager& secretManager() override { return *(secret_manager_.get()); } + MOCK_METHOD0(sslContextManager, Ssl::ContextManager&()); MOCK_CONST_METHOD0(statsScope, Stats::Scope&()); MOCK_METHOD0(clusterManager, Upstream::ClusterManager&()); - MOCK_METHOD0(secretManager, Secret::SecretManager&()); MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_METHOD0(dispatcher, Event::Dispatcher&()); MOCK_METHOD0(random, Envoy::Runtime::RandomGenerator&()); MOCK_METHOD0(stats, Stats::Store&()); + MOCK_METHOD1(setInitManager, void(Init::Manager&)); + MOCK_METHOD0(initManager, Init::Manager*()); + + std::unique_ptr secret_manager_; }; class MockListenerFactoryContext : public virtual MockFactoryContext, From 98d9f26b008d8a4ca7249850ce77833ccdd25723 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 15 Aug 2018 17:11:02 -0700 Subject: [PATCH 02/27] add files. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_callbacks.h | 1 + .../sds_dynamic_integration_test.cc | 345 ++++++++++++++++++ 2 files changed, 346 insertions(+) create mode 100644 test/integration/sds_dynamic_integration_test.cc diff --git a/include/envoy/secret/secret_callbacks.h b/include/envoy/secret/secret_callbacks.h index f27cd7583b377..b4ee4b8148373 100644 --- a/include/envoy/secret/secret_callbacks.h +++ b/include/envoy/secret/secret_callbacks.h @@ -11,6 +11,7 @@ namespace Secret { class SecretCallbacks { public: virtual ~SecretCallbacks() {} + virtual void onAddOrUpdateSecret() PURE; }; diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc new file mode 100644 index 0000000000000..191a615240a25 --- /dev/null +++ b/test/integration/sds_dynamic_integration_test.cc @@ -0,0 +1,345 @@ +#include +#include + +#include "envoy/service/discovery/v2/sds.pb.h" + +#include "common/config/resources.h" +#include "common/event/dispatcher_impl.h" +#include "common/network/connection_impl.h" +#include "common/network/utility.h" +#include "common/ssl/context_config_impl.h" +#include "common/ssl/context_manager_impl.h" + +#include "test/common/grpc/grpc_client_integration.h" +#include "test/integration/http_integration.h" +#include "test/integration/server.h" +#include "test/integration/ssl_utility.h" +#include "test/mocks/init/mocks.h" +#include "test/mocks/runtime/mocks.h" +#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/network_utility.h" +#include "test/test_common/utility.h" + +#include "absl/strings/match.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "integration.h" +#include "utility.h" + +using testing::NiceMock; +using testing::Return; + +namespace Envoy { +namespace Ssl { + +// Hack to force linking of the service: https://github.com/google/protobuf/issues/4221. +const envoy::service::discovery::v2::SdsDummy _sds_dummy; + +// Sds integration base class with following support: +// * functions to create sds upstream, and send sds response +// * functions to create secret protobuf. +class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, + public Grpc::GrpcClientIntegrationParamTest { +public: + SdsDynamicIntegrationBaseTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, ipVersion()) {} + +protected: + void createSdsStream(FakeUpstream& upstream) { + sds_upstream_ = &upstream; + AssertionResult result1 = sds_upstream_->waitForHttpConnection(*dispatcher_, sds_connection_); + RELEASE_ASSERT(result1, result1.message()); + + AssertionResult result2 = sds_connection_->waitForNewStream(*dispatcher_, sds_stream_); + RELEASE_ASSERT(result2, result2.message()); + sds_stream_->startGrpcStream(); + } + + envoy::api::v2::auth::Secret getServerSecret() { + envoy::api::v2::auth::Secret secret; + secret.set_name("server_cert"); + auto* tls_certificate = secret.mutable_tls_certificate(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); + return secret; + } + + envoy::api::v2::auth::Secret getClientSecret() { + envoy::api::v2::auth::Secret secret; + secret.set_name("client_cert"); + auto* tls_certificate = secret.mutable_tls_certificate(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/clientcert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::runfilesPath("/test/config/integration/certs/clientkey.pem")); + return secret; + } + + envoy::api::v2::auth::Secret getWrongSecret() { + envoy::api::v2::auth::Secret secret; + secret.set_name("wrong_cert"); + return secret; + } + + void sendSdsResponse(const envoy::api::v2::auth::Secret& secret) { + envoy::api::v2::DiscoveryResponse discovery_response; + discovery_response.set_version_info("1"); + discovery_response.set_type_url(Config::TypeUrl::get().Secret); + discovery_response.add_resources()->PackFrom(secret); + + sds_stream_->sendGrpcMessage(discovery_response); + } + + void cleanUpSdsConnection() { + ASSERT(sds_upstream_ != nullptr); + + // Don't ASSERT fail if an ADS reconnect ends up unparented. + sds_upstream_->set_allow_unexpected_disconnects(true); + AssertionResult result = sds_connection_->close(); + RELEASE_ASSERT(result, result.message()); + result = sds_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + sds_connection_.reset(); + } + + void PrintServerCounters() { + std::cerr << "all counters" << std::endl; + for (const auto& c : test_server_->counters()) { + std::cerr << "counter: " << c->name() << ", value: " << c->value() << std::endl; + } + } + + Runtime::MockLoader runtime_; + Ssl::ContextManagerImpl context_manager_{runtime_}; + FakeHttpConnectionPtr sds_connection_; + FakeUpstream* sds_upstream_{}; + FakeStreamPtr sds_stream_; +}; + +// Downstream SDS integration test: static Listener with ssl cert from SDS +class SdsDynamicDownstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { +public: + void initialize() override { + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + auto* common_tls_context = bootstrap.mutable_static_resources() + ->mutable_listeners(0) + ->mutable_filter_chains(0) + ->mutable_tls_context() + ->mutable_common_tls_context(); + common_tls_context->add_alpn_protocols("http/1.1"); + + auto* validation_context = common_tls_context->mutable_validation_context(); + validation_context->mutable_trusted_ca()->set_filename( + TestEnvironment::runfilesPath("test/config/integration/certs/cacert.pem")); + validation_context->add_verify_certificate_hash( + "E0:F3:C8:CE:5E:2E:A3:05:F0:70:1F:F5:12:E3:6E:2E:" + "97:92:82:84:A2:28:BC:F7:73:32:D3:39:30:A1:B6:FD"); + + // Modify the listener ssl cert to use SDS from sds_cluster + auto* secret_config = common_tls_context->add_tls_certificate_sds_secret_configs(); + secret_config->set_name("server_cert"); + auto* config_source = secret_config->mutable_sds_config(); + auto* api_config_source = config_source->mutable_api_config_source(); + api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + auto* grpc_service = api_config_source->add_grpc_services(); + setGrpcService(*grpc_service, "sds_cluster", fake_upstreams_.back()->localAddress()); + + // Add a static sds cluster + auto* sds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + sds_cluster->set_name("sds_cluster"); + sds_cluster->mutable_http2_protocol_options(); + }); + + HttpIntegrationTest::initialize(); + client_ssl_ctx_ = createClientSslTransportSocketFactory(false, false, context_manager_); + } + + void createUpstreams() override { + HttpIntegrationTest::createUpstreams(); + // SDS upstream + fake_upstreams_.emplace_back( + new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, enable_half_close_)); + } + + void TearDown() override { + cleanUpSdsConnection(); + + client_ssl_ctx_.reset(); + cleanupUpstreamAndDownstream(); + fake_upstream_connection_.reset(); + codec_client_.reset(); + } + + Network::ClientConnectionPtr makeSslClientConnection() { + Network::Address::InstanceConstSharedPtr address = getSslAddress(version_, lookupPort("http")); + return dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(), + client_ssl_ctx_->createTransportSocket(), nullptr); + } + +private: + Network::TransportSocketFactoryPtr client_ssl_ctx_; +}; + +INSTANTIATE_TEST_CASE_P(IpVersionsClientType, SdsDynamicDownstreamIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +// A test that SDS server send a good server secret for a static listener. +// The first ssl request should be OK. +TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { + pre_worker_start_test_steps_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getServerSecret()); + }; + initialize(); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(true, &creator); +} + +// A test that SDS server send a bad secret for a static listener, +// The first ssl request should fail at connecting. +// then SDS send a good server secret, the second request should be OK. +TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { + pre_worker_start_test_steps_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getWrongSecret()); + }; + initialize(); + + codec_client_ = makeRawHttpConnection(makeSslClientConnection()); + // the connection state is not connected. + EXPECT_FALSE(codec_client_->connected()); + codec_client_->connection()->close(Network::ConnectionCloseType::NoFlush); + + sendSdsResponse(getServerSecret()); + + // Wait for ssl_context_updated_by_sds counter. + if (version_ == Network::Address::IpVersion::v4) { + test_server_->waitForCounterGe( + "listener.127.0.0.1_0.server_ssl_socket_factory.ssl_context_update_by_sds", 1); + } else { + test_server_->waitForCounterGe( + "listener.[__1]_0.server_ssl_socket_factory.ssl_context_update_by_sds", 1); + } + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + testRouterHeaderOnlyRequestAndResponse(true, &creator); +} + +// Upstream SDS integration test: a static cluster has ssl cert from SDS. +class SdsDynamicUpstreamIntegrationTest : public SdsDynamicIntegrationBaseTest { +public: + void initialize() override { + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + // add sds cluster first. + auto* sds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + sds_cluster->set_name("sds_cluster"); + sds_cluster->mutable_http2_protocol_options(); + + // change the first cluster with ssl and sds. + auto* secret_config = bootstrap.mutable_static_resources() + ->mutable_clusters(0) + ->mutable_tls_context() + ->mutable_common_tls_context() + ->add_tls_certificate_sds_secret_configs(); + + secret_config->set_name("client_cert"); + auto* config_source = secret_config->mutable_sds_config(); + auto* api_config_source = config_source->mutable_api_config_source(); + api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC); + auto* grpc_service = api_config_source->add_grpc_services(); + setGrpcService(*grpc_service, "sds_cluster", fake_upstreams_.back()->localAddress()); + }); + + HttpIntegrationTest::initialize(); + registerTestServerPorts({"http"}); + } + + void TearDown() override { + cleanUpSdsConnection(); + + cleanupUpstreamAndDownstream(); + fake_upstream_connection_.reset(); + codec_client_.reset(); + + test_server_.reset(); + fake_upstreams_.clear(); + } + + void createUpstreams() override { + // This is for backend with ssl + fake_upstreams_.emplace_back(new FakeUpstream(createUpstreamSslContext(context_manager_), 0, + FakeHttpConnection::Type::HTTP1, version_)); + // This is sds. + fake_upstreams_.emplace_back( + new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_, enable_half_close_)); + } +}; + +INSTANTIATE_TEST_CASE_P(IpVersions, SdsDynamicUpstreamIntegrationTest, + GRPC_CLIENT_INTEGRATION_PARAMS); + +// To test a static cluster with sds. SDS send a good client secret first. +// The first request should work. +TEST_P(SdsDynamicUpstreamIntegrationTest, BasicSuccess) { + pre_worker_start_test_steps_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getClientSecret()); + }; + + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + // There is a race condition here; there are two static clusters: + // backend cluster_0 with sds and sds_cluser. cluster_0 is created first, its init_manager + // is called so it issues a sds call, but fail since sds_cluster is not added yet. + // so cluster_0 is initialized with an empty secret. initialize() will not wait and will return. + // the testing request will be called, even though in the pre_workder_function, a good sds is + // send, the cluster will be updated with good secret, the testing request may fail if it is + // before context is updated. Hence, need to wait for context_update counter. + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + + testRouterHeaderOnlyRequestAndResponse(true); +} + +// To test a static cluster with sds. SDS send a bad client secret first. +// The first request should fail with 503, then SDS sends a good client secret, +// the second request should work. +TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { + pre_worker_start_test_steps_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getWrongSecret()); + }; + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + // Make a simple request, should get 503 + BufferingStreamDecoderPtr response = IntegrationUtil::makeSingleRequest( + lookupPort("http"), "GET", "/test/long/url", "", downstream_protocol_, version_); + ASSERT_TRUE(response->complete()); + EXPECT_STREQ("503", response->headers().Status()->value().c_str()); + + // To flush out the reset connection from the first request in upstream. + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + + sendSdsResponse(getClientSecret()); + test_server_->waitForCounterGe( + "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + + testRouterHeaderOnlyRequestAndResponse(true); +} + +} // namespace Ssl +} // namespace Envoy From 98b7ffc8244b29709a7a3a6f85875a81c2320d01 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 15 Aug 2018 17:14:57 -0700 Subject: [PATCH 03/27] Fix format. Signed-off-by: JimmyCYJ --- include/envoy/ssl/context_config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 4138c2b1931f1..683823d8940e0 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -10,7 +10,7 @@ namespace Envoy { namespace Secret { class SecretCallbacks; -} +} // namespace Secret namespace Ssl { /** From f33fe64d8c011ced9b588039f3b92f30c82d14ac Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 15 Aug 2018 17:58:58 -0700 Subject: [PATCH 04/27] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/upstream/cluster_manager.h | 1 - source/common/ssl/context_config_impl.cc | 6 +++++- source/common/ssl/context_config_impl.h | 4 ---- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/envoy/upstream/cluster_manager.h b/include/envoy/upstream/cluster_manager.h index ed4f2c76a70f1..ae9d5b767d23c 100644 --- a/include/envoy/upstream/cluster_manager.h +++ b/include/envoy/upstream/cluster_manager.h @@ -26,7 +26,6 @@ #include "envoy/upstream/upstream.h" namespace Envoy { - namespace Upstream { /** diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 2ab7e5a747bff..302eddab54bfe 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -14,7 +14,9 @@ namespace Envoy { namespace Ssl { -Secret::TlsCertificateConfigProviderSharedPtr ContextConfigImpl::getTlsCertificateConfigProvider( +namespace { + +Secret::TlsCertificateConfigProviderSharedPtr getTlsCertificateConfigProvider( const envoy::api::v2::auth::CommonTlsContext& config, Server::Configuration::TransportSocketFactoryContext& factory_context) { if (!config.tls_certificates().empty()) { @@ -43,6 +45,8 @@ Secret::TlsCertificateConfigProviderSharedPtr ContextConfigImpl::getTlsCertifica return nullptr; } +} // namespace + const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = "[ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-CHACHA20-POLY1305]:" "[ECDHE-RSA-AES128-GCM-SHA256|ECDHE-RSA-CHACHA20-POLY1305]:" diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index c59b23a64b057..f72d284cc08e7 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -84,10 +84,6 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { tlsVersionFromProto(const envoy::api::v2::auth::TlsParameters_TlsProtocol& version, unsigned default_version); - Secret::TlsCertificateConfigProviderSharedPtr getTlsCertificateConfigProvider( - const envoy::api::v2::auth::CommonTlsContext& config, - Server::Configuration::TransportSocketFactoryContext& factory_context); - static const std::string DEFAULT_CIPHER_SUITES; static const std::string DEFAULT_ECDH_CURVES; From 448dff395321d1df8de391e924b38313b7457b0f Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 16 Aug 2018 11:19:29 -0700 Subject: [PATCH 05/27] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/ssl/context_config.h | 2 +- source/common/secret/secret_manager_impl.cc | 23 +++++++++++---------- source/common/secret/secret_manager_impl.h | 3 +++ source/common/ssl/context_config_impl.h | 2 +- source/common/ssl/ssl_socket.cc | 4 ++-- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 683823d8940e0..5b74602348e3a 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -108,7 +108,7 @@ class ContextConfig { * Add secret callback into context config. * @param callback callback that is executed by context config. */ - virtual void setUpdateCallback(Secret::SecretCallbacks& callback) PURE; + virtual void setSecretUpdateCallback(Secret::SecretCallbacks& callback) PURE; }; class ClientContextConfig : public virtual ContextConfig { diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index bae2e944424b2..8fa543ca844b8 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -38,6 +38,16 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific return std::make_shared(tls_certificate); } +void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { + ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); + auto secret_provider = dynamic_secret_providers_.find(map_key); + if (secret_provider != dynamic_secret_providers_.end()) { + dynamic_secret_providers_.erase(map_key); + } else { + ENVOY_LOG(error, "secret provider does not exist. hash key: {}", map_key); + } +} + TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { @@ -47,17 +57,8 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecr if (!secret_provider) { ASSERT(secret_provider_context.initManager() != nullptr); - std::function unregister_secret_provider = [map_key, config_name, sds_config_source, - this]() { - ENVOY_LOG(debug, "Unregister secret provider. name: {}, sds config: {}", config_name, - sds_config_source.DebugString()); - auto secret_provider = dynamic_secret_providers_.find(map_key); - if (secret_provider != dynamic_secret_providers_.end()) { - dynamic_secret_providers_.erase(map_key); - } else { - ENVOY_LOG(error, "secret provider does not exist. name: {}, sds config: {}", config_name, - sds_config_source.DebugString()); - } + std::function unregister_secret_provider = [map_key, this]() { + this->removeDynamicSecretProvider(map_key); }; secret_provider = std::make_shared( diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index d1e3e06a7a5ff..b68047281d36f 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -27,6 +27,9 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable static_tls_certificate_providers_; diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index f72d284cc08e7..aae08e282d679 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -65,7 +65,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { return !tls_certficate_provider_ || tls_certficate_provider_->secret(); } - void setUpdateCallback(Secret::SecretCallbacks& callback) override { + void setSecretUpdateCallback(Secret::SecretCallbacks& callback) override { if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { tls_certficate_provider_->removeUpdateCallback(*secret_callback_); } diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 1989e6c8ab083..6d85920425de5 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -415,7 +415,7 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), config_(std::move(config)), ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { - config_->setUpdateCallback(*this); + config_->setSecretUpdateCallback(*this); } Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { @@ -443,7 +443,7 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, : 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_)) { - config_->setUpdateCallback(*this); + config_->setSecretUpdateCallback(*this); } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { From 41432b3b4e61f96ab765259e8e71c8a5b14e986f Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 16 Aug 2018 11:30:04 -0700 Subject: [PATCH 06/27] Revise per comments. Signed-off-by: JimmyCYJ --- source/common/ssl/context_config_impl.cc | 7 +++++++ source/common/ssl/context_config_impl.h | 17 ++++++----------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 302eddab54bfe..8d8591be21f7c 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -109,6 +109,13 @@ ContextConfigImpl::ContextConfigImpl( } } +ContextConfigImpl::~ContextConfigImpl() { + if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { + tls_certficate_provider_->removeUpdateCallback(*secret_callback_); + secret_callback_ = nullptr; + } +} + unsigned ContextConfigImpl::tlsVersionFromProto( const envoy::api::v2::auth::TlsParameters_TlsProtocol& version, unsigned default_version) { switch (version) { diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index aae08e282d679..ee378e5208a13 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -19,12 +19,7 @@ static const std::string INLINE_STRING = ""; class ContextConfigImpl : public virtual Ssl::ContextConfig { public: - ~ContextConfigImpl() override { - if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { - tls_certficate_provider_->removeUpdateCallback(*secret_callback_); - secret_callback_ = nullptr; - } - } + ~ContextConfigImpl() override; // Ssl::ContextConfig const std::string& alpnProtocols() const override { return alpn_protocols_; } @@ -66,11 +61,11 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { } void setSecretUpdateCallback(Secret::SecretCallbacks& callback) override { - if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { - tls_certficate_provider_->removeUpdateCallback(*secret_callback_); - } - secret_callback_ = &callback; - if (tls_certficate_provider_.get() != nullptr) { + if (tls_certficate_provider_) { + if (secret_callback_) { + tls_certficate_provider_->removeUpdateCallback(*secret_callback_); + } + secret_callback_ = &callback; tls_certficate_provider_->addUpdateCallback(callback); } } From 006c41cd35b1cee6a3f9be1770b575f0142d4107 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 16 Aug 2018 12:22:03 -0700 Subject: [PATCH 07/27] Revise per comments. Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 8fa543ca844b8..21224bb54bcbe 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -40,10 +40,8 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); - auto secret_provider = dynamic_secret_providers_.find(map_key); - if (secret_provider != dynamic_secret_providers_.end()) { - dynamic_secret_providers_.erase(map_key); - } else { + + if (dynamic_secret_providers_.erase(map_key) == 0) { ENVOY_LOG(error, "secret provider does not exist. hash key: {}", map_key); } } From 95acda82140e1355610df901c018d869400443bb Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 16 Aug 2018 12:28:14 -0700 Subject: [PATCH 08/27] Revise per comments. Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 21224bb54bcbe..745e01ace9ef0 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -56,7 +56,7 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecr ASSERT(secret_provider_context.initManager() != nullptr); std::function unregister_secret_provider = [map_key, this]() { - this->removeDynamicSecretProvider(map_key); + removeDynamicSecretProvider(map_key); }; secret_provider = std::make_shared( From 045dcdfdca9632e3bf76c3b55a4529bc81767efe Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Fri, 17 Aug 2018 14:56:30 -0700 Subject: [PATCH 09/27] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/ssl/context_config.h | 10 +++++----- source/common/secret/secret_manager_impl.cc | 4 +--- source/common/ssl/context_config_impl.cc | 1 - source/common/upstream/upstream_impl.h | 2 +- source/server/init_manager_impl.h | 1 + 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 5b74602348e3a..dd3ee7f92a78f 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -5,12 +5,10 @@ #include #include "envoy/common/pure.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/secret/secret_provider.h" namespace Envoy { -namespace Secret { -class SecretCallbacks; -} // namespace Secret namespace Ssl { /** @@ -100,12 +98,14 @@ class ContextConfig { virtual unsigned maxProtocolVersion() const PURE; /** - * @return true if the ssl config is ready. + * @return true if the ContextConfig is able to provide secrets to create SSL context, + * and false if dynamic secrets are expected but are not downloaded from SDS server yet. */ virtual bool isReady() const PURE; /** - * Add secret callback into context config. + * Add secret callback into context config. When dynamic secrets are in use and new secrets + * are downloaded from SDS server, this callback is invoked to update SSL context. * @param callback callback that is executed by context config. */ virtual void setSecretUpdateCallback(Secret::SecretCallbacks& callback) PURE; diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 745e01ace9ef0..f017bc27a7ec2 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -41,9 +41,7 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); - if (dynamic_secret_providers_.erase(map_key) == 0) { - ENVOY_LOG(error, "secret provider does not exist. hash key: {}", map_key); - } + ASSERT(dynamic_secret_providers_.erase(map_key) == 1); } TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider( diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 8d8591be21f7c..242b041a527aa 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -112,7 +112,6 @@ ContextConfigImpl::ContextConfigImpl( ContextConfigImpl::~ContextConfigImpl() { if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { tls_certficate_provider_->removeUpdateCallback(*secret_callback_); - secret_callback_ = nullptr; } } diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index b2028b968edff..d2a7260697244 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -527,7 +527,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable Date: Mon, 20 Aug 2018 11:21:06 -0700 Subject: [PATCH 10/27] Revise per comments. Signed-off-by: JimmyCYJ --- source/common/ssl/ssl_socket.cc | 28 ++++++++++++++++++++++++---- source/common/ssl/ssl_socket.h | 7 +++++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 6d85920425de5..6d75d2fa7c72a 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -413,12 +413,17 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, 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_)) { + config_(std::move(config)) { config_->setSecretUpdateCallback(*this); + + std::unique_lock lock(ssl_ctx_mutex_); + ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); } Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { + // SDS would update ssl_ctx_ when Envoy is running. + // Need a read lock to let multiple threads gain read access to ssl_ctx_. + std::shared_lock lock(ssl_ctx_mutex_); if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); } else { @@ -431,6 +436,11 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } void ClientSslSocketFactory::onAddOrUpdateSecret() { + // SSL context update happens when Envoy is running and SDS is updating SSL + // context, need a write lock to make sure only main thread could have write access + // to ssl_ctx_. + std::unique_lock lock(ssl_ctx_mutex_); + ENVOY_LOG(debug, "Secret is updated."); ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); stats_.ssl_context_update_by_sds_.inc(); @@ -441,12 +451,17 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, Stats::Scope& stats_scope, 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_)) { + config_(std::move(config)), server_names_(server_names) { config_->setSecretUpdateCallback(*this); + + std::unique_lock lock(ssl_ctx_mutex_); + ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { + // SDS would update ssl_ctx_ when Envoy is running. + // Need a read lock to let multiple threads gain read access to ssl_ctx_. + std::shared_lock lock(ssl_ctx_mutex_); if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); } else { @@ -459,6 +474,11 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } void ServerSslSocketFactory::onAddOrUpdateSecret() { + // SSL context update happens when Envoy is running and SDS is updating SSL + // context, need a write lock to make sure only main thread could have write access + // to ssl_ctx_. + std::unique_lock lock(ssl_ctx_mutex_); + ENVOY_LOG(debug, "Secret is updated."); ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); stats_.ssl_context_update_by_sds_.inc(); diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index b195e3d4ed7d0..eaa2e17c3346a 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include "envoy/network/connection.h" @@ -102,6 +103,9 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, SslSocketFactoryStats stats_; ClientContextConfigPtr config_; ClientContextSharedPtr ssl_ctx_; + // Protects ssl_ctx_ from read access by multiple threads, and guarantees only + // one thread can write to ssl_ctx_. + mutable std::shared_timed_mutex ssl_ctx_mutex_; }; class ServerSslSocketFactory : public Network::TransportSocketFactory, @@ -124,6 +128,9 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, ServerContextConfigPtr config_; const std::vector server_names_; ServerContextSharedPtr ssl_ctx_; + // Protects ssl_ctx_ from read access by multiple threads, and guarantees only + // one thread can write to ssl_ctx_. + mutable std::shared_timed_mutex ssl_ctx_mutex_; }; } // namespace Ssl From 6f75d4bfa99d5d9eddaded36f6ac6924334764f1 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Mon, 20 Aug 2018 11:50:19 -0700 Subject: [PATCH 11/27] Remove unnecessary lock and fix compile issue. Signed-off-by: JimmyCYJ --- source/common/ssl/ssl_socket.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 6d75d2fa7c72a..99fa051ca16de 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -413,11 +413,9 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, Ssl::ContextManager& manager, Stats::Scope& stats_scope) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), - config_(std::move(config)) { + config_(std::move(config)), + ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { config_->setSecretUpdateCallback(*this); - - std::unique_lock lock(ssl_ctx_mutex_); - ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); } Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { @@ -451,11 +449,9 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, Stats::Scope& stats_scope, const std::vector& server_names) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)), - config_(std::move(config)), server_names_(server_names) { + config_(std::move(config)), server_names_(server_names), + ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) { config_->setSecretUpdateCallback(*this); - - std::unique_lock lock(ssl_ctx_mutex_); - ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { From 74f0d1e75a1fd6eeaf889c9af20680ba5bcb00f7 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 21 Aug 2018 18:39:25 -0700 Subject: [PATCH 12/27] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 6 +++--- include/envoy/secret/secret_provider.h | 2 +- source/common/secret/secret_manager_impl.cc | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index c3775f1b0de0c..47d27ac6cbdb6 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -47,11 +47,11 @@ class SecretManager { * Finds and returns a dynamic secret provider associated to SDS config. Create * a new one if such provider does not exist. * - * @param config_source a protobuf message object contains SDS config source. - * @param config_name a name that uniquely refers to the SDS config source + * @param config_source a protobuf message object containing a SDS config source. + * @param config_name a name that uniquely refers to the SDS config source. * @param secret_provider_context context that provides components for creating and initializing * secret provider. - * @return the dynamic TLS secret provider. + * @return TlsCertificateConfigProviderSharedPtr the dynamic TLS secret provider. */ virtual TlsCertificateConfigProviderSharedPtr findOrCreateDynamicSecretProvider( const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index a624a7a420458..e910c607f1e3a 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -24,7 +24,7 @@ template class SecretProvider { /** * Add secret update callback into secret provider. - * It is safe to call this method by main thread and is safe to be invoked + * It is safe to call this method by main thread and callback is safe to be invoked * on main thread. * @param callback callback that is executed by secret provider. * @return CallbackHandle the handle which can remove that update callback. diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index f017bc27a7ec2..862c9ae099013 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -41,18 +41,20 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); - ASSERT(dynamic_secret_providers_.erase(map_key) == 1); + RELEASE_ASSERT(dynamic_secret_providers_.erase(map_key) == 1, ""); } TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { - std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name; + const std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name; auto secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { ASSERT(secret_provider_context.initManager() != nullptr); + // SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before + // SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi. std::function unregister_secret_provider = [map_key, this]() { removeDynamicSecretProvider(map_key); }; From 166a30fdcd680481c6a8d3155717fbba45bbe908 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 22 Aug 2018 11:10:36 -0700 Subject: [PATCH 13/27] Remove lock. Signed-off-by: JimmyCYJ --- source/common/ssl/ssl_socket.cc | 16 ---------------- source/common/ssl/ssl_socket.h | 6 ------ 2 files changed, 22 deletions(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 99fa051ca16de..6d85920425de5 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -419,9 +419,6 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, } Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { - // SDS would update ssl_ctx_ when Envoy is running. - // Need a read lock to let multiple threads gain read access to ssl_ctx_. - std::shared_lock lock(ssl_ctx_mutex_); if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); } else { @@ -434,11 +431,6 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } void ClientSslSocketFactory::onAddOrUpdateSecret() { - // SSL context update happens when Envoy is running and SDS is updating SSL - // context, need a write lock to make sure only main thread could have write access - // to ssl_ctx_. - std::unique_lock lock(ssl_ctx_mutex_); - ENVOY_LOG(debug, "Secret is updated."); ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_); stats_.ssl_context_update_by_sds_.inc(); @@ -455,9 +447,6 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { - // SDS would update ssl_ctx_ when Envoy is running. - // Need a read lock to let multiple threads gain read access to ssl_ctx_. - std::shared_lock lock(ssl_ctx_mutex_); if (ssl_ctx_) { return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); } else { @@ -470,11 +459,6 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } void ServerSslSocketFactory::onAddOrUpdateSecret() { - // SSL context update happens when Envoy is running and SDS is updating SSL - // context, need a write lock to make sure only main thread could have write access - // to ssl_ctx_. - std::unique_lock lock(ssl_ctx_mutex_); - ENVOY_LOG(debug, "Secret is updated."); ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_); stats_.ssl_context_update_by_sds_.inc(); diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index eaa2e17c3346a..f72fed8250fb6 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -103,9 +103,6 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, SslSocketFactoryStats stats_; ClientContextConfigPtr config_; ClientContextSharedPtr ssl_ctx_; - // Protects ssl_ctx_ from read access by multiple threads, and guarantees only - // one thread can write to ssl_ctx_. - mutable std::shared_timed_mutex ssl_ctx_mutex_; }; class ServerSslSocketFactory : public Network::TransportSocketFactory, @@ -128,9 +125,6 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, ServerContextConfigPtr config_; const std::vector server_names_; ServerContextSharedPtr ssl_ctx_; - // Protects ssl_ctx_ from read access by multiple threads, and guarantees only - // one thread can write to ssl_ctx_. - mutable std::shared_timed_mutex ssl_ctx_mutex_; }; } // namespace Ssl From 98b29db8e0e1cb220eae9b3120d60be6914247a0 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 22 Aug 2018 11:11:53 -0700 Subject: [PATCH 14/27] Remove unused include. Signed-off-by: JimmyCYJ --- source/common/ssl/ssl_socket.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index f72fed8250fb6..b195e3d4ed7d0 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -1,7 +1,6 @@ #pragma once #include -#include #include #include "envoy/network/connection.h" From b182b23695f92a38cbae9214a6e435286a670d91 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Fri, 24 Aug 2018 16:36:14 -0700 Subject: [PATCH 15/27] Update caller to use SdsApi. Signed-off-by: JimmyCYJ --- include/envoy/ssl/context_config.h | 2 +- source/common/ssl/context_config_impl.cc | 6 +++--- source/common/ssl/context_config_impl.h | 13 +++++-------- source/common/ssl/ssl_socket.cc | 4 ++-- test/mocks/secret/mocks.h | 7 ------- 5 files changed, 11 insertions(+), 21 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index dd3ee7f92a78f..3e06021e4591b 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -108,7 +108,7 @@ class ContextConfig { * are downloaded from SDS server, this callback is invoked to update SSL context. * @param callback callback that is executed by context config. */ - virtual void setSecretUpdateCallback(Secret::SecretCallbacks& callback) PURE; + virtual void setSecretUpdateCallback(std::function callback) PURE; }; class ClientContextConfig : public virtual ContextConfig { diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 242b041a527aa..0c2c4ca8d120d 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -79,8 +79,8 @@ ContextConfigImpl::ContextConfigImpl( Config::DataSource::read(config.validation_context().crl(), true)), certificate_revocation_list_path_( Config::DataSource::getPath(config.validation_context().crl()).value_or(EMPTY_STRING)), - secret_callback_(nullptr), tls_certficate_provider_(getTlsCertificateConfigProvider(config, factory_context)), + secret_update_callback_handle_(nullptr), verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), config.validation_context().verify_subject_alt_name().end()), verify_certificate_hash_list_(config.validation_context().verify_certificate_hash().begin(), @@ -110,8 +110,8 @@ ContextConfigImpl::ContextConfigImpl( } ContextConfigImpl::~ContextConfigImpl() { - if (tls_certficate_provider_.get() != nullptr && secret_callback_ != nullptr) { - tls_certficate_provider_->removeUpdateCallback(*secret_callback_); + if (secret_update_callback_handle_) { + secret_update_callback_handle_->remove(); } } diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index ee378e5208a13..bf80317741d97 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -60,14 +60,11 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { return !tls_certficate_provider_ || tls_certficate_provider_->secret(); } - void setSecretUpdateCallback(Secret::SecretCallbacks& callback) override { - if (tls_certficate_provider_) { - if (secret_callback_) { - tls_certficate_provider_->removeUpdateCallback(*secret_callback_); - } - secret_callback_ = &callback; - tls_certficate_provider_->addUpdateCallback(callback); + void setSecretUpdateCallback(std::function callback) override { + if (secret_update_callback_handle_) { + secret_update_callback_handle_->remove(); } + secret_update_callback_handle_ = tls_certficate_provider_->addUpdateCallback(callback); } protected: @@ -90,8 +87,8 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string ca_cert_path_; const std::string certificate_revocation_list_; const std::string certificate_revocation_list_path_; - Secret::SecretCallbacks* secret_callback_; Secret::TlsCertificateConfigProviderSharedPtr tls_certficate_provider_; + Common::CallbackHandle* secret_update_callback_handle_; const std::vector verify_subject_alt_name_list_; const std::vector verify_certificate_hash_list_; const std::vector verify_certificate_spki_list_; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 6d85920425de5..4e74ebd40ac4c 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -415,7 +415,7 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), config_(std::move(config)), ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { - config_->setSecretUpdateCallback(*this); + config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { @@ -443,7 +443,7 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, : 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_)) { - config_->setSecretUpdateCallback(*this); + config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index e2ff48f23ce08..9885c80f9b2ed 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -35,12 +35,5 @@ class MockSecretCallbacks : public SecretCallbacks { MOCK_METHOD0(onAddOrUpdateSecret, void()); }; -class MockSecretCallbacks : public SecretCallbacks { -public: - MockSecretCallbacks(); - ~MockSecretCallbacks(); - MOCK_METHOD0(onAddOrUpdateSecret, void()); -}; - } // namespace Secret } // namespace Envoy From 508f4aba6d1ffd15fa06ffeb3a0ed484bee8ab63 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Sat, 25 Aug 2018 21:47:32 -0700 Subject: [PATCH 16/27] Check provider is nullptr. Signed-off-by: JimmyCYJ --- source/common/ssl/context_config_impl.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index bf80317741d97..184e1f8e90cf2 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -61,10 +61,12 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { } void setSecretUpdateCallback(std::function callback) override { - if (secret_update_callback_handle_) { - secret_update_callback_handle_->remove(); + if (tls_certficate_provider_) { + if (secret_update_callback_handle_) { + secret_update_callback_handle_->remove(); + } + secret_update_callback_handle_ = tls_certficate_provider_->addUpdateCallback(callback); } - secret_update_callback_handle_ = tls_certficate_provider_->addUpdateCallback(callback); } protected: From 1b8e91eed1f9012283a01671ce7f06d4ac35cda8 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Tue, 28 Aug 2018 16:47:22 -0700 Subject: [PATCH 17/27] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 2 +- source/common/secret/secret_manager_impl.cc | 2 +- source/common/secret/secret_manager_impl.h | 2 +- source/common/ssl/context_config_impl.cc | 2 +- source/common/ssl/ssl_socket.cc | 16 ++++++++++++---- test/common/secret/secret_manager_impl_test.cc | 4 ++-- test/mocks/secret/mocks.h | 2 +- 7 files changed, 19 insertions(+), 11 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 47d27ac6cbdb6..9eb9670d8b39b 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -53,7 +53,7 @@ class SecretManager { * secret provider. * @return TlsCertificateConfigProviderSharedPtr the dynamic TLS secret provider. */ - virtual TlsCertificateConfigProviderSharedPtr findOrCreateDynamicSecretProvider( + virtual TlsCertificateConfigProviderSharedPtr findOrCreateTlsCertificateProvider( const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE; }; diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 862c9ae099013..123e6154340f4 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -44,7 +44,7 @@ void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) RELEASE_ASSERT(dynamic_secret_providers_.erase(map_key) == 1, ""); } -TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateDynamicSecretProvider( +TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { const std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name; diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index b68047281d36f..feb3ccb858311 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -22,7 +22,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable(ssl_ctx_, Ssl::InitialState::Client); + // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and + // creating SslSocket using ssl_ctx. Capture ssl_ctx_ into a local variable so that we check and + // use the same ssl_ctx to create SslSocket. + auto ssl_ctx = ssl_ctx_; + if (ssl_ctx) { + return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Client); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); stats_.upstream_connection_reset_by_sds_.inc(); @@ -447,8 +451,12 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, } Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { - if (ssl_ctx_) { - return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); + // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and + // creating SslSocket using ssl_ctx. Capture ssl_ctx_ into a local variable so that we check and + // use the same ssl_ctx to create SslSocket. + auto ssl_ctx = ssl_ctx_; + if (ssl_ctx) { + return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Server); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); stats_.downstream_connection_reset_by_sds_.inc(); diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index f957d3c301ca5..e49cd36dd7f82 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -95,8 +95,8 @@ TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { EXPECT_CALL(secret_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager)); - auto secret_provider = - secret_manager->findOrCreateDynamicSecretProvider(config_source, "abc.com", secret_context); + auto secret_provider = secret_manager->findOrCreateTlsCertificateProvider( + config_source, "abc.com", secret_context); std::string yaml = R"EOF( name: "abc.com" diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 9885c80f9b2ed..f41ee31933c01 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -22,7 +22,7 @@ class MockSecretManager : public SecretManager { MOCK_METHOD1(createInlineTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::auth::TlsCertificate& tls_certificate)); - MOCK_METHOD3(findOrCreateDynamicSecretProvider, + MOCK_METHOD3(findOrCreateTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::core::ConfigSource&, const std::string&, Server::Configuration::TransportSocketFactoryContext&)); From 336a950130949a0162066fbacafb5bf8ae23494b Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 12:16:02 -0700 Subject: [PATCH 18/27] Revise per comments. Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 7 ++++--- source/common/ssl/context_config_impl.h | 4 ++-- source/common/ssl/ssl_socket.cc | 4 ++-- source/common/ssl/ssl_socket.h | 4 ++-- source/server/server.cc | 4 ++-- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 123e6154340f4..96b70204e3dfa 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -41,15 +41,16 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); - RELEASE_ASSERT(dynamic_secret_providers_.erase(map_key) == 1, ""); + auto num_deleted = dynamic_secret_providers_.erase(map_key); + RELEASE_ASSERT(num_deleted == 1, ""); } TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { - const std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name; + const std::string map_key = sds_config_source.SerializeAsString + config_name; - auto secret_provider = dynamic_secret_providers_[map_key].lock(); + TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { ASSERT(secret_provider_context.initManager() != nullptr); diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 184e1f8e90cf2..dc3b292aecbf6 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -55,9 +55,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { unsigned maxProtocolVersion() const override { return max_protocol_version_; }; bool isReady() const override { - // either tls_certficate_provider_ is nullptr or + // Either tls_certficate_provider_ is nullptr or // tls_certficate_provider_->secret() is NOT nullptr. - return !tls_certficate_provider_ || tls_certficate_provider_->secret(); + return !tls_certficate_provider_ || tls_certficate_provider_->secret() != nullptr; } void setSecretUpdateCallback(std::function callback) override { diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 58cd8b6400ee1..923691785d64b 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -427,7 +427,7 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Client); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); - stats_.upstream_connection_reset_by_sds_.inc(); + stats_.upstream_context_secrets_not_ready.inc(); return std::make_unique(); } } @@ -459,7 +459,7 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Server); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); - stats_.downstream_connection_reset_by_sds_.inc(); + stats_.downstream_context_secrets_not_ready.inc(); return std::make_unique(); } } diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h index b195e3d4ed7d0..951af874a655f 100644 --- a/source/common/ssl/ssl_socket.h +++ b/source/common/ssl/ssl_socket.h @@ -20,8 +20,8 @@ namespace Ssl { // clang-format off #define ALL_SSL_SOCKET_FACTORY_STATS(COUNTER) \ COUNTER(ssl_context_update_by_sds) \ - COUNTER(upstream_connection_reset_by_sds) \ - COUNTER(downstream_connection_reset_by_sds) + COUNTER(upstream_context_secrets_not_ready) \ + COUNTER(downstream_context_secrets_not_ready) // clang-format on /** diff --git a/source/server/server.cc b/source/server/server.cc index 7cfa6656d2a8a..5e168a2dfe7ec 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -56,8 +56,8 @@ InstanceImpl::InstanceImpl(Options& options, TimeSource& time_source, singleton_manager_(new Singleton::ManagerImpl()), handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)), random_generator_(std::move(random_generator)), - secret_manager_(new Secret::SecretManagerImpl()), listener_component_factory_(*this), - worker_factory_(thread_local_, *api_, hooks, time_source), + secret_manager_(std::make_unique()), + listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks, time_source), dns_resolver_(dispatcher_->createDnsResolver({})), access_log_manager_(*api_, *dispatcher_, access_log_lock, store), terminated_(false) { From f36a870699d78646c9f7a2be56f06f58029eb81a Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 13:34:26 -0700 Subject: [PATCH 19/27] Fix stats name. Signed-off-by: JimmyCYJ --- source/common/ssl/ssl_socket.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 923691785d64b..5ba3ac9eb7dde 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -427,7 +427,7 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Client); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); - stats_.upstream_context_secrets_not_ready.inc(); + stats_.upstream_context_secrets_not_ready_.inc(); return std::make_unique(); } } @@ -459,7 +459,7 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons return std::make_unique(std::move(ssl_ctx), Ssl::InitialState::Server); } else { ENVOY_LOG(debug, "Create NotReadySslSocket"); - stats_.downstream_context_secrets_not_ready.inc(); + stats_.downstream_context_secrets_not_ready_.inc(); return std::make_unique(); } } From f02ae4b5c6589ca659899f54da5e5cd1b3adaad6 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 13:58:32 -0700 Subject: [PATCH 20/27] Fix compile error. Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 96b70204e3dfa..8bcf0084eaea1 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -48,7 +48,7 @@ void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { - const std::string map_key = sds_config_source.SerializeAsString + config_name; + const std::string map_key = sds_config_source.SerializeAsString() + config_name; TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { From 54a8dd1265fa3cc10d057964e378156471f5300f Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 16:29:54 -0700 Subject: [PATCH 21/27] Fix format. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_manager.h | 5 +++-- source/common/secret/secret_manager_impl.cc | 20 ++++++-------------- source/common/secret/secret_manager_impl.h | 2 +- test/common/ssl/context_impl_test.cc | 6 ++++-- test/mocks/secret/mocks.h | 6 +++--- 5 files changed, 17 insertions(+), 22 deletions(-) diff --git a/include/envoy/secret/secret_manager.h b/include/envoy/secret/secret_manager.h index 6cb38686ea9f9..ca02d639643ab 100644 --- a/include/envoy/secret/secret_manager.h +++ b/include/envoy/secret/secret_manager.h @@ -52,14 +52,15 @@ class SecretManager { const envoy::api::v2::auth::TlsCertificate& tls_certificate) PURE; /** - * @param certificate_validation_context the protobuf config of the certificate validation context. + * @param certificate_validation_context the protobuf config of the certificate validation + * context. * @return a CertificateValidationContextConfigProviderSharedPtr created from * certificate_validation_context. */ virtual CertificateValidationContextConfigProviderSharedPtr createInlineCertificateValidationContextProvider( const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) - PURE; + PURE; /** * Finds and returns a dynamic secret provider associated to SDS config. Create diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 250f5c5515cd1..c23330cf9e82c 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -57,7 +57,6 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertific return std::make_shared(tls_certificate); } - CertificateValidationContextConfigProviderSharedPtr SecretManagerImpl::createInlineCertificateValidationContextProvider( const envoy::api::v2::auth::CertificateValidationContext& certificate_validation_context) { @@ -75,11 +74,9 @@ void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { - const std::string - map_key = sds_config_source.SerializeAsString() + config_name; + const std::string map_key = sds_config_source.SerializeAsString() + config_name; - TlsCertificateConfigProviderSharedPtr - secret_provider = dynamic_secret_providers_[map_key].lock(); + TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); if (!secret_provider) { ASSERT(secret_provider_context.initManager() != nullptr); @@ -90,15 +87,10 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertific }; secret_provider = std::make_shared( - secret_provider_context.localInfo(), - secret_provider_context.dispatcher(), - secret_provider_context.random(), - secret_provider_context.stats(), - secret_provider_context.clusterManager(), - *secret_provider_context.initManager(), - sds_config_source, - config_name, - unregister_secret_provider); + secret_provider_context.localInfo(), secret_provider_context.dispatcher(), + secret_provider_context.random(), secret_provider_context.stats(), + secret_provider_context.clusterManager(), *secret_provider_context.initManager(), + sds_config_source, config_name, unregister_secret_provider); dynamic_secret_providers_[map_key] = secret_provider; } diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h index 37e69d875a849..a6017ff8719c3 100644 --- a/source/common/secret/secret_manager_impl.h +++ b/source/common/secret/secret_manager_impl.h @@ -29,7 +29,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable factory_context; @@ -430,7 +431,8 @@ TEST(ClientContextConfigImplTest, TlsCertificatesAndSdsConfig) { "Multiple TLS certificates are not supported for client contexts"); } -// Validate context config supports SDS, and is marked as not ready if secrets are not yet downloaded. +// Validate context config supports SDS, and is marked as not ready if secrets are not yet +// downloaded. TEST(ClientContextConfigImplTest, SecretNotReady) { envoy::api::v2::auth::UpstreamTlsContext tls_context; NiceMock local_info; diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index cc98619b7c3ae..2637d6d850325 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -25,9 +25,9 @@ class MockSecretManager : public SecretManager { TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::auth::TlsCertificate& tls_certificate)); MOCK_METHOD1(createInlineCertificateValidationContextProvider, - CertificateValidationContextConfigProviderSharedPtr( - const envoy::api::v2::auth::CertificateValidationContext& - certificate_validation_context)); + CertificateValidationContextConfigProviderSharedPtr( + const envoy::api::v2::auth::CertificateValidationContext& + certificate_validation_context)); MOCK_METHOD3(findOrCreateTlsCertificateProvider, TlsCertificateConfigProviderSharedPtr( const envoy::api::v2::core::ConfigSource&, const std::string&, From 599a459167796f24a9eff7cb8332a11a54cb1c94 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 16:38:59 -0700 Subject: [PATCH 22/27] Fix compile error. Signed-off-by: JimmyCYJ --- source/common/ssl/context_config_impl.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index ee8b6290b0ead..aef7c1bdeec19 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -46,10 +46,11 @@ Secret::TlsCertificateConfigProviderSharedPtr getTlsCertificateConfigProvider( } Secret::CertificateValidationContextConfigProviderSharedPtr -getCertificateValidationContextConfigProvider(const envoy::api::v2::auth::CommonTlsContext& config, - Secret::SecretManager& secret_manager) { +getCertificateValidationContextConfigProvider( + const envoy::api::v2::auth::CommonTlsContext& config, + Server::Configuration::TransportSocketFactoryContext& factory_context) { if (config.has_validation_context()) { - return secret_manager.createInlineCertificateValidationContextProvider( + return factory_context.secretManager().createInlineCertificateValidationContextProvider( config.validation_context()); } if (config.has_validation_context_sds_secret_config()) { @@ -57,7 +58,8 @@ getCertificateValidationContextConfigProvider(const envoy::api::v2::auth::Common if (!sds_secret_config.has_sds_config()) { // static secret auto secret_provider = - secret_manager.findStaticCertificateValidationContextProvider(sds_secret_config.name()); + factory_context.secretManager().findStaticCertificateValidationContextProvider( + sds_secret_config.name()); if (!secret_provider) { throw EnvoyException(fmt::format("Unknown static certificate validation context: {}", sds_secret_config.name())); @@ -95,10 +97,10 @@ ContextConfigImpl::ContextConfigImpl( RepeatedPtrUtil::join(config.tls_params().cipher_suites(), ":"), DEFAULT_CIPHER_SUITES)), ecdh_curves_(StringUtil::nonEmptyStringOrDefault( RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)), - tls_certficate_provider_(getTlsCertificateConfigProvider(config, secret_manager)), + tls_certficate_provider_(getTlsCertificateConfigProvider(config, factory_context)), secret_update_callback_handle_(nullptr), certficate_validation_context_provider_( - getCertificateValidationContextConfigProvider(config, secret_manager)), + getCertificateValidationContextConfigProvider(config, factory_context)), min_protocol_version_( tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(), TLS1_VERSION)), max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), From 11a358d939506969fd479d2afde1166c125e6dc5 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 17:08:31 -0700 Subject: [PATCH 23/27] Fix test. Signed-off-by: JimmyCYJ --- test/common/ssl/context_impl_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index b509b2559880e..c83954c04eb92 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -504,8 +504,8 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { )EOF"; MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), tls_certificate_secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - secret_manager->addStaticSecret(tls_certificate_secret_config); + NiceMock factory_context; + factory_context.secretManager().addStaticSecret(tls_certificate_secret_config); envoy::api::v2::auth::Secret certificate_validation_context_secret_config; const std::string certificate_validation_context_yaml = R"EOF( name: "def.com" @@ -515,7 +515,7 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { )EOF"; MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), certificate_validation_context_secret_config); - secret_manager->addStaticSecret(certificate_validation_context_secret_config); + factory_context.secretManager().addStaticSecret(certificate_validation_context_secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() @@ -525,7 +525,7 @@ TEST(ClientContextConfigImplTest, StaticCertificateValidationContext) { tls_context.mutable_common_tls_context() ->mutable_validation_context_sds_secret_config() ->set_name("def.com"); - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()); + ClientContextConfigImpl client_context_config(tls_context, factory_context); const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem"; EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), @@ -576,8 +576,8 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { )EOF"; MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), tls_certificate_secret_config); - std::unique_ptr secret_manager(new Secret::SecretManagerImpl()); - secret_manager->addStaticSecret(tls_certificate_secret_config); + NiceMock factory_context; + factory_context.secretManager().addStaticSecret(tls_certificate_secret_config); envoy::api::v2::auth::Secret certificate_validation_context_secret_config; const std::string certificate_validation_context_yaml = R"EOF( name: "def.com" @@ -587,7 +587,7 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { )EOF"; MessageUtil::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml), certificate_validation_context_secret_config); - secret_manager->addStaticSecret(certificate_validation_context_secret_config); + factory_context.secretManager().addStaticSecret(certificate_validation_context_secret_config); envoy::api::v2::auth::UpstreamTlsContext tls_context; tls_context.mutable_common_tls_context() @@ -598,8 +598,8 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { ->mutable_validation_context_sds_secret_config() ->set_name("missing"); EXPECT_THROW_WITH_MESSAGE( - ClientContextConfigImpl client_context_config(tls_context, *secret_manager.get()), - EnvoyException, "Unknown static certificate validation context: missing"); + ClientContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, + "Unknown static certificate validation context: missing"); } // Multiple TLS certificates are not yet supported, but one is expected for From 92a4fcad6a48324c3c6a75b9359c6c53dc04077f Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 29 Aug 2018 18:40:35 -0700 Subject: [PATCH 24/27] Replace RELEASE_ASSERT with ASSERT. Signed-off-by: JimmyCYJ --- source/common/secret/secret_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index c23330cf9e82c..a310d59777ac9 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -68,7 +68,7 @@ void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); auto num_deleted = dynamic_secret_providers_.erase(map_key); - RELEASE_ASSERT(num_deleted == 1, ""); + ASSERT(num_deleted == 1, ""); } TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider( From cf3400aee940137fce515c9c0a9dc07cb741c5f0 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 30 Aug 2018 16:15:25 -0700 Subject: [PATCH 25/27] Fix integration test caused by merging master. Signed-off-by: JimmyCYJ --- .../sds_dynamic_integration_test.cc | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 191a615240a25..3ec6d34a12a90 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -43,7 +43,8 @@ class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, public Grpc::GrpcClientIntegrationParamTest { public: SdsDynamicIntegrationBaseTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, ipVersion()) {} + : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, ipVersion()), + server_cert_("server_cert"), client_cert_("client_cert") {} protected: void createSdsStream(FakeUpstream& upstream) { @@ -58,7 +59,7 @@ class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, envoy::api::v2::auth::Secret getServerSecret() { envoy::api::v2::auth::Secret secret; - secret.set_name("server_cert"); + secret.set_name(server_cert_); auto* tls_certificate = secret.mutable_tls_certificate(); tls_certificate->mutable_certificate_chain()->set_filename( TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem")); @@ -69,7 +70,7 @@ class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, envoy::api::v2::auth::Secret getClientSecret() { envoy::api::v2::auth::Secret secret; - secret.set_name("client_cert"); + secret.set_name(client_cert_); auto* tls_certificate = secret.mutable_tls_certificate(); tls_certificate->mutable_certificate_chain()->set_filename( TestEnvironment::runfilesPath("/test/config/integration/certs/clientcert.pem")); @@ -78,9 +79,10 @@ class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, return secret; } - envoy::api::v2::auth::Secret getWrongSecret() { + envoy::api::v2::auth::Secret getWrongSecret(const std::string& secret_name) { envoy::api::v2::auth::Secret secret; - secret.set_name("wrong_cert"); + secret.set_name(secret_name); + secret.mutable_tls_certificate(); return secret; } @@ -112,6 +114,8 @@ class SdsDynamicIntegrationBaseTest : public HttpIntegrationTest, } } + const std::string server_cert_; + const std::string client_cert_; Runtime::MockLoader runtime_; Ssl::ContextManagerImpl context_manager_{runtime_}; FakeHttpConnectionPtr sds_connection_; @@ -208,7 +212,7 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { pre_worker_start_test_steps_ = [this]() { createSdsStream(*(fake_upstreams_[1])); - sendSdsResponse(getWrongSecret()); + sendSdsResponse(getWrongSecret(server_cert_)); }; initialize(); @@ -318,7 +322,7 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, BasicSuccess) { TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { pre_worker_start_test_steps_ = [this]() { createSdsStream(*(fake_upstreams_[1])); - sendSdsResponse(getWrongSecret()); + sendSdsResponse(getWrongSecret(client_cert_)); }; initialize(); fake_upstreams_[0]->set_allow_unexpected_disconnects(true); From a7b9d9fa9e23a27773edf59ee9ded7c39c2571c9 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 30 Aug 2018 17:39:18 -0700 Subject: [PATCH 26/27] Add unit tests to cover NotReadySslSocket. Signed-off-by: JimmyCYJ --- test/common/ssl/BUILD | 1 + test/common/ssl/ssl_socket_test.cc | 82 ++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/test/common/ssl/BUILD b/test/common/ssl/BUILD index a0a47930f0956..36c928b705e9e 100644 --- a/test/common/ssl/BUILD +++ b/test/common/ssl/BUILD @@ -20,6 +20,7 @@ envoy_cc_test( ], external_deps = ["ssl"], deps = [ + "//include/envoy/network:transport_socket_interface", "//source/common/buffer:buffer_lib", "//source/common/common:empty_string", "//source/common/event:dispatcher_includes", diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 4e79e5be24d11..8cbda11fb8143 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -2,6 +2,8 @@ #include #include +#include "envoy/network/transport_socket.h" + #include "common/buffer/buffer_impl.h" #include "common/common/empty_string.h" #include "common/event/dispatcher_impl.h" @@ -2580,6 +2582,86 @@ TEST_P(SslSocketTest, GetRequestedServerName) { GetParam()); } +// Validate that if downstream secrets are not yet downloaded from SDS server, Envoy creates +// NotReadySslSocket object to handle downstream connection. +TEST_P(SslSocketTest, DownstreamNotReadySslSocket) { + Stats::IsolatedStoreImpl stats_store; + Runtime::MockLoader runtime; + testing::NiceMock factory_context; + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + NiceMock cluster_manager; + NiceMock init_manager; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); + + envoy::api::v2::auth::DownstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto server_cfg = std::make_unique(tls_context, factory_context); + EXPECT_EQ(nullptr, server_cfg->tlsCertificate()); + EXPECT_FALSE(server_cfg->isReady()); + + ContextManagerImpl manager(runtime); + Ssl::ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, stats_store, + std::vector{}); + auto transport_socket = server_ssl_socket_factory.createTransportSocket(); + EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); + EXPECT_EQ(nullptr, transport_socket->ssl()); + Buffer::OwnedImpl buffer; + Network::IoResult result = transport_socket->doRead(buffer); + EXPECT_EQ(Network::PostIoAction::Close, result.action_); + result = transport_socket->doWrite(buffer, true); + EXPECT_EQ(Network::PostIoAction::Close, result.action_); +} + +// Validate that if upstream secrets are not yet downloaded from SDS server, Envoy creates +// NotReadySslSocket object to handle upstream connection. +TEST_P(SslSocketTest, UpstreamNotReadySslSocket) { + Stats::IsolatedStoreImpl stats_store; + Runtime::MockLoader runtime; + testing::NiceMock factory_context; + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + NiceMock cluster_manager; + NiceMock init_manager; + EXPECT_CALL(factory_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(factory_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(factory_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(factory_context, stats()).WillOnce(ReturnRef(stats_store)); + EXPECT_CALL(factory_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(factory_context, initManager()).WillRepeatedly(Return(&init_manager)); + + envoy::api::v2::auth::UpstreamTlsContext tls_context; + auto sds_secret_configs = + tls_context.mutable_common_tls_context()->mutable_tls_certificate_sds_secret_configs()->Add(); + sds_secret_configs->set_name("abc.com"); + sds_secret_configs->mutable_sds_config(); + auto client_cfg = std::make_unique(tls_context, factory_context); + EXPECT_EQ(nullptr, client_cfg->tlsCertificate()); + EXPECT_FALSE(client_cfg->isReady()); + + ContextManagerImpl manager(runtime); + Ssl::ClientSslSocketFactory client_ssl_socket_factory(std::move(client_cfg), manager, + stats_store); + auto transport_socket = client_ssl_socket_factory.createTransportSocket(); + EXPECT_EQ(EMPTY_STRING, transport_socket->protocol()); + EXPECT_EQ(nullptr, transport_socket->ssl()); + Buffer::OwnedImpl buffer; + Network::IoResult result = transport_socket->doRead(buffer); + EXPECT_EQ(Network::PostIoAction::Close, result.action_); + result = transport_socket->doWrite(buffer, true); + EXPECT_EQ(Network::PostIoAction::Close, result.action_); +} + class SslReadBufferLimitTest : public SslSocketTest { public: void initialize() { From 0dcd867a206f9cdf7e565d34abcea64c0b8d13a8 Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 30 Aug 2018 18:59:36 -0700 Subject: [PATCH 27/27] Revise per comments. Signed-off-by: JimmyCYJ --- .../common/secret/secret_manager_impl_test.cc | 60 +++++++++---------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index d45e67150d145..c340e63342f47 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -139,47 +139,45 @@ name: "abc.com" TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) { Server::MockInstance server; - std::unique_ptr secret_manager(new SecretManagerImpl()); + std::unique_ptr secret_manager(std::make_unique()); NiceMock secret_context; envoy::api::v2::core::ConfigSource config_source; - { - NiceMock local_info; - NiceMock dispatcher; - NiceMock random; - Stats::IsolatedStoreImpl stats; - NiceMock cluster_manager; - NiceMock init_manager; - EXPECT_CALL(secret_context, localInfo()).WillOnce(ReturnRef(local_info)); - EXPECT_CALL(secret_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); - EXPECT_CALL(secret_context, random()).WillOnce(ReturnRef(random)); - EXPECT_CALL(secret_context, stats()).WillOnce(ReturnRef(stats)); - EXPECT_CALL(secret_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); - EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager)); - - auto secret_provider = secret_manager->findOrCreateTlsCertificateProvider( - config_source, "abc.com", secret_context); - std::string yaml = - R"EOF( + NiceMock local_info; + NiceMock dispatcher; + NiceMock random; + Stats::IsolatedStoreImpl stats; + NiceMock cluster_manager; + NiceMock init_manager; + EXPECT_CALL(secret_context, localInfo()).WillOnce(ReturnRef(local_info)); + EXPECT_CALL(secret_context, dispatcher()).WillOnce(ReturnRef(dispatcher)); + EXPECT_CALL(secret_context, random()).WillOnce(ReturnRef(random)); + EXPECT_CALL(secret_context, stats()).WillOnce(ReturnRef(stats)); + EXPECT_CALL(secret_context, clusterManager()).WillOnce(ReturnRef(cluster_manager)); + EXPECT_CALL(secret_context, initManager()).WillRepeatedly(Return(&init_manager)); + + auto secret_provider = + secret_manager->findOrCreateTlsCertificateProvider(config_source, "abc.com", secret_context); + const std::string yaml = + R"EOF( name: "abc.com" tls_certificate: certificate_chain: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" private_key: filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" - )EOF"; - Protobuf::RepeatedPtrField secret_resources; - auto secret_config = secret_resources.Add(); - MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); - std::dynamic_pointer_cast(secret_provider)->onConfigUpdate(secret_resources, ""); - const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; - EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), - secret_provider->secret()->certificateChain()); - const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; - EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), - secret_provider->secret()->privateKey()); - } +)EOF"; + Protobuf::RepeatedPtrField secret_resources; + auto secret_config = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); + dynamic_cast(*secret_provider).onConfigUpdate(secret_resources, ""); + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + secret_provider->secret()->certificateChain()); + const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), + secret_provider->secret()->privateKey()); } } // namespace