oauth2: refactor SDS secret provider to remove a data race (envoyprox…

…y#32625) Commit Message: Refactor SDS secret provider to use TLV for values and make it a common utility to prevent code duplication. Additional Description: Risk Level: low Testing: done Docs Changes: none Release Notes: none Fixes: envoyproxy#21273 Signed-off-by: Kuat Yessenov <[email protected]>
kyessenov · Feb 29, 2024 · 88cc302 · 88cc302
1 parent 44d4d82
commit 88cc302
Show file tree

Hide file tree

Showing 12 changed files with 91 additions and 70 deletions.
diff --git 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
@@ -57,7 +57,8 @@ Http::FilterFactoryCb FilterFactory::createFilterFactoryFromProtoTyped(
   }
 
   auto secret_reader = std::make_shared<SDSSecretReader>(
-      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<FilterConfig>(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
@@ -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<Logger::Id::filter> {

diff --git 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<ThreadLocal::MockInstance> 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
@@ -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
@@ -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<envoy::extensions::transport_sockets::tls::v3::GenericSecret>(
               generic_secret)) {}
 
+ThreadLocalGenericSecretProvider::ThreadLocalGenericSecretProvider(
+    GenericSecretConfigProviderSharedPtr&& provider, ThreadLocal::SlotAllocator& tls, Api::Api& api)
+    : provider_(provider), api_(api),
+      tls_(std::make_unique<ThreadLocal::TypedSlot<ThreadLocalSecret>>(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<ThreadLocalSecret>(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<ThreadLocalSecret> 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
@@ -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<ThreadLocalSecret> 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
@@ -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
@@ -64,9 +64,9 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
     throw EnvoyException("invalid HMAC secret configuration");
   }
 
-  auto secret_reader =
-      std::make_shared<SDSSecretReader>(secret_provider_token_secret, secret_provider_hmac_secret,
-                                        context.serverFactoryContext().api());
+  auto secret_reader = std::make_shared<SDSSecretReader>(
+      std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret),
+      context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api());
   auto config = std::make_shared<FilterConfig>(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
@@ -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
@@ -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<ThreadLocal::MockInstance> 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
@@ -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