fix oauth filter sds update

source/common/http/utility.cc

@@ -249,6 +249,46 @@ bool maybeAdjustForIpv6(absl::string_view absolute_url, uint64_t& offset, uint64
   return true;
 }
 
+std::string parseCookie(const HeaderMap& headers, const std::string& key,
+                        const std::string& cookie) {
+
+  std::string ret;
+
+  headers.iterateReverse([&key, &ret, &cookie](const HeaderEntry& header) -> HeaderMap::Iterate {
+    // Find the cookie headers in the request (typically, there's only one).
+    if (header.key() == cookie) {
+
+      // Split the cookie header into individual cookies.
+      for (const auto& s : StringUtil::splitToken(header.value().getStringView(), ";")) {
+        // Find the key part of the cookie (i.e. the name of the cookie).
+        size_t first_non_space = s.find_first_not_of(' ');
+        size_t equals_index = s.find('=');
+        if (equals_index == absl::string_view::npos) {
+          // The cookie is malformed if it does not have an `=`. Continue
+          // checking other cookies in this header.
+          continue;
+        }
+        const absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
+        // If the key matches, parse the value from the rest of the cookie string.
+        if (k == key) {
+          absl::string_view v = s.substr(equals_index + 1, s.size() - 1);
+
+          // Cookie values may be wrapped in double quotes.
+          // https://tools.ietf.org/html/rfc6265#section-4.1.1
+          if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
+            v = v.substr(1, v.size() - 2);
+          }
+          ret = std::string{v};
+          return HeaderMap::Iterate::Break;
+        }
+      }
+    }
+    return HeaderMap::Iterate::Continue;
+  });
+
+  return ret;
+}
+
 bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) {
   struct http_parser_url u;
   http_parser_url_init(&u);
@@ -377,42 +417,11 @@ absl::string_view Utility::findQueryStringStart(const HeaderString& path) {
 }
 
 std::string Utility::parseCookieValue(const HeaderMap& headers, const std::string& key) {
+  return parseCookie(headers, key, Http::Headers::get().Cookie.get());
+}
 
-  std::string ret;
-
-  headers.iterateReverse([&key, &ret](const HeaderEntry& header) -> HeaderMap::Iterate {
-    // Find the cookie headers in the request (typically, there's only one).
-    if (header.key() == Http::Headers::get().Cookie.get()) {
-
-      // Split the cookie header into individual cookies.
-      for (const auto& s : StringUtil::splitToken(header.value().getStringView(), ";")) {
-        // Find the key part of the cookie (i.e. the name of the cookie).
-        size_t first_non_space = s.find_first_not_of(" ");
-        size_t equals_index = s.find('=');
-        if (equals_index == absl::string_view::npos) {
-          // The cookie is malformed if it does not have an `=`. Continue
-          // checking other cookies in this header.
-          continue;
-        }
-        const absl::string_view k = s.substr(first_non_space, equals_index - first_non_space);
-        // If the key matches, parse the value from the rest of the cookie string.
-        if (k == key) {
-          absl::string_view v = s.substr(equals_index + 1, s.size() - 1);
-
-          // Cookie values may be wrapped in double quotes.
-          // https://tools.ietf.org/html/rfc6265#section-4.1.1
-          if (v.size() >= 2 && v.back() == '"' && v[0] == '"') {
-            v = v.substr(1, v.size() - 2);
-          }
-          ret = std::string{v};
-          return HeaderMap::Iterate::Break;
-        }
-      }
-    }
-    return HeaderMap::Iterate::Continue;
-  });
-
-  return ret;
+std::string Utility::parseSetCookieValue(const Http::HeaderMap& headers, const std::string& key) {
+  return parseCookie(headers, key, Http::Headers::get().SetCookie.get());
 }
 
 std::string Utility::makeSetCookieValue(const std::string& key, const std::string& value,

source/common/http/utility.h

@@ -240,6 +240,14 @@ absl::string_view findQueryStringStart(const HeaderString& path);
  **/
 std::string parseCookieValue(const HeaderMap& headers, const std::string& key);
 
+/**
+ * Parse a particular value out of a set-cookie
+ * @param headers supplies the headers to get the set-cookie from.
+ * @param key the key for the particular set-cookie value to return
+ * @return std::string the parsed set-cookie value, or "" if none exists
+ **/
+std::string parseSetCookieValue(const HeaderMap& headers, const std::string& key);
+
 /**
  * Produce the value for a Set-Cookie header with the given parameters.
  * @param key is the name of the cookie that is being set.

source/extensions/filters/http/oauth2/filter.h

@@ -49,38 +49,35 @@ class SDSSecretReader : public SecretReader {
 public:
   SDSSecretReader(Secret::GenericSecretConfigProviderSharedPtr client_secret_provider,
                   Secret::GenericSecretConfigProviderSharedPtr token_secret_provider, Api::Api& api)
-      : api_(api), client_secret_provider_(std::move(client_secret_provider)),
-        token_secret_provider_(std::move(token_secret_provider)) {
-    readAndWatchSecret(client_secret_, *client_secret_provider_);
-    readAndWatchSecret(token_secret_, *token_secret_provider_);
-  }
+      : 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_; }
 
 private:
-  void readAndWatchSecret(std::string& value,
-                          Secret::GenericSecretConfigProvider& secret_provider) {
-    const auto* secret = secret_provider.secret();
+  Envoy::Common::CallbackHandlePtr
+  readAndWatchSecret(std::string& value,
+                     Secret::GenericSecretConfigProviderSharedPtr& secret_provider, Api::Api& api) {
+    const auto* secret = secret_provider->secret();
     if (secret != nullptr) {
-      value = Config::DataSource::read(secret->secret(), true, api_);
+      value = Config::DataSource::read(secret->secret(), true, api);
     }
 
-    update_callback_ = secret_provider.addUpdateCallback([&secret_provider, this, &value]() {
-      const auto* secret = secret_provider.secret();
+    return secret_provider->addUpdateCallback([secret_provider, &api, &value]() {
+      const auto* secret = secret_provider->secret();
       if (secret != nullptr) {
-        value = Config::DataSource::read(secret->secret(), true, api_);
+        value = Config::DataSource::read(secret->secret(), true, api);
       }
     });
   }
+
   std::string client_secret_;
   std::string token_secret_;
-  Api::Api& api_;
-  Envoy::Common::CallbackHandlePtr update_callback_;
 
-  Secret::GenericSecretConfigProviderSharedPtr client_secret_provider_;
-  Secret::GenericSecretConfigProviderSharedPtr token_secret_provider_;
+  Envoy::Common::CallbackHandlePtr update_callback_client_;
+  Envoy::Common::CallbackHandlePtr update_callback_token_;
 };
 
 /**

test/common/http/utility_test.cc

@@ -549,6 +549,18 @@ TEST(HttpUtility, TestParseCookie) {
   EXPECT_EQ(value, "abc123");
 }
 
+TEST(HttpUtility, TestParseSetCookie) {
+  TestRequestHeaderMapImpl headers{
+      {"someheader", "10.0.0.1"},
+      {"set-cookie", "somekey=somevalue; someotherkey=someothervalue"},
+      {"set-cookie", "abc=def; token=abc123; Expires=Wed, 09 Jun 2021 10:18:14 GMT"},
+      {"set-cookie", "key2=value2; key3=value3"}};
+
+  std::string key{"token"};
+  std::string value = Utility::parseSetCookieValue(headers, key);
+  EXPECT_EQ(value, "abc123");
+}
+
 TEST(HttpUtility, TestParseCookieBadValues) {
   TestRequestHeaderMapImpl headers{{"cookie", "token1=abc123; = "},
                                    {"cookie", "token2=abc123;   "},
@@ -561,6 +573,18 @@ TEST(HttpUtility, TestParseCookieBadValues) {
   EXPECT_EQ(Utility::parseCookieValue(headers, "token4"), "abc123");
 }
 
+TEST(HttpUtility, TestParseSetCookieBadValues) {
+  TestRequestHeaderMapImpl headers{{"set-cookie", "token1=abc123; = "},
+                                   {"set-cookie", "token2=abc123;   "},
+                                   {"set-cookie", "; token3=abc123;"},
+                                   {"set-cookie", "=; token4=\"abc123\""}};
+
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "token1"), "abc123");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "token2"), "abc123");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "token3"), "abc123");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "token4"), "abc123");
+}
+
 TEST(HttpUtility, TestParseCookieWithQuotes) {
   TestRequestHeaderMapImpl headers{
       {"someheader", "10.0.0.1"},
@@ -574,6 +598,19 @@ TEST(HttpUtility, TestParseCookieWithQuotes) {
   EXPECT_EQ(Utility::parseCookieValue(headers, "leadingdquote"), "\"foobar");
 }
 
+TEST(HttpUtility, TestParseSetCookieWithQuotes) {
+  TestRequestHeaderMapImpl headers{
+      {"someheader", "10.0.0.1"},
+      {"set-cookie", "dquote=\"; quoteddquote=\"\"\""},
+      {"set-cookie", "leadingdquote=\"foobar;"},
+      {"set-cookie", "abc=def; token=\"abc123\"; Expires=Wed, 09 Jun 2021 10:18:14 GMT"}};
+
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "token"), "abc123");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "dquote"), "\"");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "quoteddquote"), "\"");
+  EXPECT_EQ(Utility::parseSetCookieValue(headers, "leadingdquote"), "\"foobar");
+}
+
 TEST(HttpUtility, TestMakeSetCookieValue) {
   EXPECT_EQ("name=\"value\"; Max-Age=10",
             Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), false));

test/extensions/filters/http/oauth2/BUILD

@@ -27,7 +27,9 @@ envoy_extension_cc_test(
     srcs = ["oauth_integration_test.cc"],
     extension_name = "envoy.filters.http.oauth2",
     deps = [
+        "//source/common/http:utility_lib",
         "//source/extensions/filters/http/oauth2:config",
+        "//source/extensions/filters/http/oauth2:oauth_lib",
         "//test/integration:http_integration_lib",
         "//test/integration:integration_lib",
         "//test/mocks/server:factory_context_mocks",
@@ -39,6 +41,7 @@ envoy_extension_cc_test(
     srcs = ["filter_test.cc"],
     extension_name = "envoy.filters.http.oauth2",
     deps = [
+        "//source/common/secret:secret_manager_impl_lib",
         "//source/extensions/filters/http/oauth2:config",
         "//source/extensions/filters/http/oauth2:oauth_callback_interface",
         "//source/extensions/filters/http/oauth2:oauth_client",

test/extensions/filters/http/oauth2/filter_test.cc

@@ -10,6 +10,7 @@
 #include "common/http/message_impl.h"
 #include "common/protobuf/message_validator_impl.h"
 #include "common/protobuf/utility.h"
+#include "common/secret/secret_manager_impl.h"
 
 #include "extensions/filters/http/oauth2/filter.h"
 
@@ -155,6 +156,81 @@ class OAuth2Test : public testing::Test {
   Event::SimulatedTimeSystem test_time_;
 };
 
+// Verifies that the OAuth SDSSecretReader correctly updates dynamic generic secret.
+TEST_F(OAuth2Test, SdsDynamicGenericSecret) {
+  NiceMock<Server::MockConfigTracker> config_tracker;
+  Secret::SecretManagerImpl secret_manager{config_tracker};
+  envoy::config::core::v3::ConfigSource config_source;
+
+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> secret_context;
+  NiceMock<LocalInfo::MockLocalInfo> local_info;
+  Api::ApiPtr api = Api::createApiForTest();
+  Stats::IsolatedStoreImpl stats;
+  NiceMock<Init::MockManager> init_manager;
+  Init::TargetHandlePtr init_handle;
+  NiceMock<Event::MockDispatcher> dispatcher;
+  EXPECT_CALL(secret_context, localInfo()).WillRepeatedly(ReturnRef(local_info));
+  EXPECT_CALL(secret_context, api()).WillRepeatedly(ReturnRef(*api));
+  EXPECT_CALL(secret_context, dispatcher()).WillRepeatedly(ReturnRef(dispatcher));
+  EXPECT_CALL(secret_context, stats()).WillRepeatedly(ReturnRef(stats));
+  EXPECT_CALL(secret_context, initManager()).WillRepeatedly(ReturnRef(init_manager));
+  EXPECT_CALL(init_manager, add(_))
+      .WillRepeatedly(Invoke([&init_handle](const Init::Target& target) {
+        init_handle = target.createHandle("test");
+      }));
+
+  auto client_secret_provider =
+      secret_manager.findOrCreateGenericSecretProvider(config_source, "client", secret_context);
+  auto client_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_;
+  auto token_secret_provider =
+      secret_manager.findOrCreateGenericSecretProvider(config_source, "token", secret_context);
+  auto token_callback = secret_context.cluster_manager_.subscription_factory_.callbacks_;
+
+  SDSSecretReader secret_reader(client_secret_provider, token_secret_provider, *api);
+  EXPECT_TRUE(secret_reader.clientSecret().empty());
+  EXPECT_TRUE(secret_reader.tokenSecret().empty());
+
+  const std::string yaml_client = R"EOF(
+name: client
+generic_secret:
+  secret:
+    inline_string: "client_test"
+)EOF";
+
+  envoy::extensions::transport_sockets::tls::v3::Secret typed_secret;
+  TestUtility::loadFromYaml(yaml_client, typed_secret);
+  const auto decoded_resources_client = TestUtility::decodeResources({typed_secret});
+
+  client_callback->onConfigUpdate(decoded_resources_client.refvec_, "");
+  EXPECT_EQ(secret_reader.clientSecret(), "client_test");
+  EXPECT_EQ(secret_reader.tokenSecret(), "");
+
+  const std::string yaml_token = R"EOF(
+name: token
+generic_secret:
+  secret:
+    inline_string: "token_test"
+)EOF";
+  TestUtility::loadFromYaml(yaml_token, typed_secret);
+  const auto decoded_resources_token = TestUtility::decodeResources({typed_secret});
+
+  token_callback->onConfigUpdate(decoded_resources_token.refvec_, "");
+  EXPECT_EQ(secret_reader.clientSecret(), "client_test");
+  EXPECT_EQ(secret_reader.tokenSecret(), "token_test");
+
+  const std::string yaml_client_recheck = R"EOF(
+name: client
+generic_secret:
+  secret:
+    inline_string: "client_test_recheck"
+)EOF";
+  TestUtility::loadFromYaml(yaml_client_recheck, typed_secret);
+  const auto decoded_resources_client_recheck = TestUtility::decodeResources({typed_secret});
+
+  client_callback->onConfigUpdate(decoded_resources_client_recheck.refvec_, "");
+  EXPECT_EQ(secret_reader.clientSecret(), "client_test_recheck");
+  EXPECT_EQ(secret_reader.tokenSecret(), "token_test");
+}
 // Verifies that we fail constructing the filter if the configured cluster doesn't exist.
 TEST_F(OAuth2Test, InvalidCluster) {
   ON_CALL(factory_context_.cluster_manager_, clusters())