diff --git a/contrib/sxg/filters/http/source/BUILD b/contrib/sxg/filters/http/source/BUILD index a6bb7cec6708..1d3b5b9231a6 100644 --- a/contrib/sxg/filters/http/source/BUILD +++ b/contrib/sxg/filters/http/source/BUILD @@ -28,6 +28,7 @@ envoy_cc_library( "//source/common/http:codes_lib", "//source/common/stats:symbol_table_lib", "//source/common/stats:utility_lib", + "//source/common/secret:secret_provider_impl_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//contrib/envoy/extensions/filters/http/sxg/v3alpha:pkg_cc_proto", # use boringssl alias to select fips vs non-fips version. diff --git a/contrib/sxg/filters/http/source/config.cc b/contrib/sxg/filters/http/source/config.cc index 68b7d2292a72..541216f8454f 100644 --- a/contrib/sxg/filters/http/source/config.cc +++ b/contrib/sxg/filters/http/source/config.cc @@ -57,7 +57,8 @@ Http::FilterFactoryCb FilterFactory::createFilterFactoryFromProtoTyped( } auto secret_reader = std::make_shared( - secret_provider_certificate, secret_provider_private_key, server_context.api()); + std::move(secret_provider_certificate), std::move(secret_provider_private_key), + server_context.threadLocal(), server_context.api()); auto config = std::make_shared(proto_config, server_context.timeSource(), secret_reader, stat_prefix, context.scope()); return [config](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/contrib/sxg/filters/http/source/filter_config.h b/contrib/sxg/filters/http/source/filter_config.h index ca3cbfdb5300..a54f51794259 100644 --- a/contrib/sxg/filters/http/source/filter_config.h +++ b/contrib/sxg/filters/http/source/filter_config.h @@ -4,7 +4,7 @@ #include "envoy/stats/scope.h" #include "envoy/stats/stats_macros.h" -#include "source/common/config/datasource.h" +#include "source/common/secret/secret_provider_impl.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "contrib/envoy/extensions/filters/http/sxg/v3alpha/sxg.pb.h" @@ -37,39 +37,18 @@ class SecretReader { class SDSSecretReader : public SecretReader { public: - SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr certificate_provider, - Secret::GenericSecretConfigProviderSharedPtr private_key_provider, Api::Api& api) - : update_callback_client_(readAndWatchSecret(certificate_, certificate_provider, api)), - update_callback_token_(readAndWatchSecret(private_key_, private_key_provider, api)) {} - + SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr&& certificate_provider, + Secret::GenericSecretConfigProviderSharedPtr&& private_key_provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api) + : certificate_(std::move(certificate_provider), tls, api), + private_key_(std::move(private_key_provider), tls, api) {} // SecretReader - const std::string& certificate() const override { return certificate_; } - const std::string& privateKey() const override { return private_key_; } + const std::string& certificate() const override { return certificate_.secret(); } + const std::string& privateKey() const override { return private_key_.secret(); } private: - Envoy::Common::CallbackHandlePtr - readAndWatchSecret(std::string& value, - Secret::GenericSecretConfigProviderSharedPtr& secret_provider, Api::Api& api) { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = - THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), std::string); - } - - return secret_provider->addUpdateCallback([secret_provider, &api, &value]() { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), - std::string); - } - }); - } - - std::string certificate_; - std::string private_key_; - - Envoy::Common::CallbackHandlePtr update_callback_client_; - Envoy::Common::CallbackHandlePtr update_callback_token_; + Secret::ThreadLocalGenericSecretProvider certificate_; + Secret::ThreadLocalGenericSecretProvider private_key_; }; class FilterConfig : public Logger::Loggable { diff --git a/contrib/sxg/filters/http/test/filter_test.cc b/contrib/sxg/filters/http/test/filter_test.cc index 933198e6f6c7..a8ba5c027e18 100644 --- a/contrib/sxg/filters/http/test/filter_test.cc +++ b/contrib/sxg/filters/http/test/filter_test.cc @@ -380,7 +380,9 @@ TEST_F(FilterTest, SdsDynamicGenericSecret) { config_source, "private_key", secret_context, init_manager); auto private_key_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_; - SDSSecretReader secret_reader(certificate_secret_provider, private_key_secret_provider, *api); + NiceMock tls; + SDSSecretReader secret_reader(std::move(certificate_secret_provider), + std::move(private_key_secret_provider), tls, *api); EXPECT_TRUE(secret_reader.certificate().empty()); EXPECT_TRUE(secret_reader.privateKey().empty()); diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index b7b25aedbe5b..d6e61c39531f 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -32,6 +32,9 @@ envoy_cc_library( hdrs = ["secret_provider_impl.h"], deps = [ "//envoy/secret:secret_provider_interface", + "//envoy/thread_local:thread_local_interface", + "//envoy/thread_local:thread_local_object", + "//source/common/config:datasource_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", diff --git a/source/common/secret/secret_provider_impl.cc b/source/common/secret/secret_provider_impl.cc index 6aed80d0107c..ea2ff427ec19 100644 --- a/source/common/secret/secret_provider_impl.cc +++ b/source/common/secret/secret_provider_impl.cc @@ -3,6 +3,7 @@ #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" #include "source/common/common/assert.h" +#include "source/common/config/datasource.h" #include "source/common/ssl/certificate_validation_context_config_impl.h" #include "source/common/ssl/tls_certificate_config_impl.h" @@ -36,5 +37,33 @@ GenericSecretConfigProviderImpl::GenericSecretConfigProviderImpl( std::make_unique( generic_secret)) {} +ThreadLocalGenericSecretProvider::ThreadLocalGenericSecretProvider( + GenericSecretConfigProviderSharedPtr&& provider, ThreadLocal::SlotAllocator& tls, Api::Api& api) + : provider_(provider), api_(api), + tls_(std::make_unique>(tls)), + cb_(provider_->addUpdateCallback([this] { update(); })) { + std::string value; + if (const auto* secret = provider_->secret(); secret != nullptr) { + value = + THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api_), std::string); + } + tls_->set([value = std::move(value)](Event::Dispatcher&) { + return std::make_shared(value); + }); +} + +const std::string& ThreadLocalGenericSecretProvider::secret() const { return (*tls_)->value_; } + +// This function is executed on the main during xDS update and can throw. +void ThreadLocalGenericSecretProvider::update() { + std::string value; + if (const auto* secret = provider_->secret(); secret != nullptr) { + value = + THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api_), std::string); + } + tls_->runOnAllThreads( + [value = std::move(value)](OptRef tls) { tls->value_ = value; }); +} + } // namespace Secret } // namespace Envoy diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index 566f53bf2a86..981f1c84cf41 100644 --- a/source/common/secret/secret_provider_impl.h +++ b/source/common/secret/secret_provider_impl.h @@ -6,6 +6,8 @@ #include "envoy/secret/secret_provider.h" #include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/ssl/tls_certificate_config.h" +#include "envoy/thread_local/thread_local.h" +#include "envoy/thread_local/thread_local_object.h" namespace Envoy { namespace Secret { @@ -108,5 +110,28 @@ class GenericSecretConfigProviderImpl : public GenericSecretConfigProvider { Secret::GenericSecretPtr generic_secret_; }; +/** + * A utility secret provider that uses thread local values to share the updates to the secrets from + * the main to the workers. + **/ +class ThreadLocalGenericSecretProvider { +public: + ThreadLocalGenericSecretProvider(GenericSecretConfigProviderSharedPtr&& provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api); + const std::string& secret() const; + +private: + struct ThreadLocalSecret : public ThreadLocal::ThreadLocalObject { + explicit ThreadLocalSecret(const std::string& value) : value_(value) {} + std::string value_; + }; + void update(); + GenericSecretConfigProviderSharedPtr provider_; + Api::Api& api_; + ThreadLocal::TypedSlotPtr tls_; + // Must be last since it has a non-trivial de-registering destructor. + Common::CallbackHandlePtr cb_; +}; + } // namespace Secret } // namespace Envoy diff --git a/source/extensions/filters/http/oauth2/BUILD b/source/extensions/filters/http/oauth2/BUILD index 936579f59266..a236dfa7583f 100644 --- a/source/extensions/filters/http/oauth2/BUILD +++ b/source/extensions/filters/http/oauth2/BUILD @@ -49,10 +49,10 @@ envoy_cc_library( "//envoy/server:filter_config_interface", "//source/common/common:assert_lib", "//source/common/common:empty_string", - "//source/common/config:datasource_lib", "//source/common/crypto:utility_lib", "//source/common/formatter:substitution_formatter_lib", "//source/common/protobuf:utility_lib", + "//source/common/secret:secret_provider_impl_lib", "//source/extensions/filters/http/common:pass_through_filter_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/extensions/filters/http/oauth2/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/oauth2/config.cc b/source/extensions/filters/http/oauth2/config.cc index 98fd1f7dd6d8..1a7878b92f26 100644 --- a/source/extensions/filters/http/oauth2/config.cc +++ b/source/extensions/filters/http/oauth2/config.cc @@ -64,9 +64,9 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped( throw EnvoyException("invalid HMAC secret configuration"); } - auto secret_reader = - std::make_shared(secret_provider_token_secret, secret_provider_hmac_secret, - context.serverFactoryContext().api()); + auto secret_reader = std::make_shared( + std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret), + context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api()); auto config = std::make_shared(proto_config, cluster_manager, secret_reader, context.scope(), stats_prefix); diff --git a/source/extensions/filters/http/oauth2/filter.h b/source/extensions/filters/http/oauth2/filter.h index 715cd3230cdd..d2a594241a35 100644 --- a/source/extensions/filters/http/oauth2/filter.h +++ b/source/extensions/filters/http/oauth2/filter.h @@ -16,11 +16,11 @@ #include "source/common/common/assert.h" #include "source/common/common/matchers.h" -#include "source/common/config/datasource.h" #include "source/common/formatter/substitution_formatter.h" #include "source/common/http/header_map_impl.h" #include "source/common/http/header_utility.h" #include "source/common/http/utility.h" +#include "source/common/secret/secret_provider_impl.h" #include "source/extensions/filters/http/common/pass_through_filter.h" #include "source/extensions/filters/http/oauth2/oauth.h" #include "source/extensions/filters/http/oauth2/oauth_client.h" @@ -45,39 +45,17 @@ class SecretReader { class SDSSecretReader : public SecretReader { public: - SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr client_secret_provider, - Secret::GenericSecretConfigProviderSharedPtr token_secret_provider, Api::Api& api) - : update_callback_client_(readAndWatchSecret(client_secret_, client_secret_provider, api)), - update_callback_token_(readAndWatchSecret(token_secret_, token_secret_provider, api)) {} - - const std::string& clientSecret() const override { return client_secret_; } - - const std::string& tokenSecret() const override { return token_secret_; } + SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr&& client_secret_provider, + Secret::GenericSecretConfigProviderSharedPtr&& token_secret_provider, + ThreadLocal::SlotAllocator& tls, Api::Api& api) + : client_secret_(std::move(client_secret_provider), tls, api), + token_secret_(std::move(token_secret_provider), tls, api) {} + const std::string& clientSecret() const override { return client_secret_.secret(); } + const std::string& tokenSecret() const override { return token_secret_.secret(); } private: - Envoy::Common::CallbackHandlePtr - readAndWatchSecret(std::string& value, - Secret::GenericSecretConfigProviderSharedPtr& secret_provider, Api::Api& api) { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = - THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), std::string); - } - - return secret_provider->addUpdateCallback([secret_provider, &api, &value]() { - const auto* secret = secret_provider->secret(); - if (secret != nullptr) { - value = THROW_OR_RETURN_VALUE(Config::DataSource::read(secret->secret(), true, api), - std::string); - } - }); - } - - std::string client_secret_; - std::string token_secret_; - - Envoy::Common::CallbackHandlePtr update_callback_client_; - Envoy::Common::CallbackHandlePtr update_callback_token_; + Secret::ThreadLocalGenericSecretProvider client_secret_; + Secret::ThreadLocalGenericSecretProvider token_secret_; }; /** diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 3ac4819bc635..231f899fd611 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -247,7 +247,9 @@ TEST_F(OAuth2Test, SdsDynamicGenericSecret) { config_source, "token", secret_context, init_manager); auto token_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_; - SDSSecretReader secret_reader(client_secret_provider, token_secret_provider, *api); + NiceMock tls; + SDSSecretReader secret_reader(std::move(client_secret_provider), std::move(token_secret_provider), + tls, *api); EXPECT_TRUE(secret_reader.clientSecret().empty()); EXPECT_TRUE(secret_reader.tokenSecret().empty()); diff --git a/tools/code_format/config.yaml b/tools/code_format/config.yaml index 0e96847c58dd..7da3e054be32 100644 --- a/tools/code_format/config.yaml +++ b/tools/code_format/config.yaml @@ -162,6 +162,7 @@ paths: - source/common/upstream/health_discovery_service.cc - source/common/secret/sds_api.h - source/common/secret/sds_api.cc + - source/common/secret/secret_provider_impl.cc - source/common/router/router.cc - source/common/config/config_provider_impl.h - source/common/common/logger_delegates.cc