fix oauth filter sds update

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/extensions/filters/http/oauth2/BUILD

@@ -39,6 +39,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())

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

@@ -1,9 +1,11 @@
+#include "common/crypto/utility.h"
 #include "common/protobuf/utility.h"
 
 #include "source/extensions/filters/http/oauth2/oauth_response.pb.h"
 
 #include "test/integration/http_integration.h"
 
+#include "absl/strings/escaping.h"
 #include "gtest/gtest.h"
 
 namespace Envoy {
@@ -35,7 +37,43 @@ class OauthIntegrationTest : public testing::Test, public HttpIntegrationTest {
   void initialize() override {
     setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
 
-    config_helper_.addFilter(R"EOF(
+    TestEnvironment::writeStringToFileForTest("token_secret.yaml", R"EOF(
+resources:
+  - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"
+    name: token
+    generic_secret:
+      secret:
+        inline_string: "token_secret")EOF",
+                                              false);
+
+    TestEnvironment::writeStringToFileForTest("token_secret_1.yaml", R"EOF(
+resources:
+  - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"
+    name: token
+    generic_secret:
+      secret:
+        inline_string: "token_secret_1")EOF",
+                                              false);
+
+    TestEnvironment::writeStringToFileForTest("hmac_secret.yaml", R"EOF(
+resources:
+  - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"
+    name: hmac
+    generic_secret:
+      secret:
+        inline_string: "hmac_secret")EOF",
+                                              false);
+
+    TestEnvironment::writeStringToFileForTest("hmac_secret_1.yaml", R"EOF(
+resources:
+  - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"
+    name: hmac
+    generic_secret:
+      secret:
+        inline_string: "hmac_secret_1")EOF",
+                                              false);
+
+    config_helper_.addFilter(TestEnvironment::substitute(R"EOF(
 name: oauth
 typed_config:
   "@type": type.googleapis.com/envoy.extensions.filters.http.oauth2.v3alpha.OAuth2
@@ -56,8 +94,14 @@ name: oauth
       client_id: foo
       token_secret:
         name: token
+        sds_config:
+          path: "{{ test_tmpdir }}/token_secret.yaml"
+          resource_api_version: V3
       hmac_secret:
         name: hmac
+        sds_config:
+          path: "{{ test_tmpdir }}/hmac_secret.yaml"
+          resource_api_version: V3
     auth_scopes:
     - user
     - openid
@@ -66,20 +110,12 @@ name: oauth
     - oauth2-resource
     - http://example.com
     - https://example.com
-)EOF");
+)EOF"));
 
     // Add the OAuth cluster.
     config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
       *bootstrap.mutable_static_resources()->add_clusters() =
           config_helper_.buildStaticCluster("oauth", 0, "127.0.0.1");
-
-      auto* token_secret = bootstrap.mutable_static_resources()->add_secrets();
-      token_secret->set_name("token");
-      token_secret->mutable_generic_secret()->mutable_secret()->set_inline_bytes("token_secret");
-
-      auto* hmac_secret = bootstrap.mutable_static_resources()->add_secrets();
-      hmac_secret->set_name("hmac");
-      hmac_secret->mutable_generic_secret()->mutable_secret()->set_inline_bytes("hmac_secret");
     });
 
     setUpstreamCount(2);
@@ -107,9 +143,66 @@ TEST_F(OauthIntegrationTest, UnauthenticatedFlow) {
   EXPECT_EQ("302", response->headers().getStatusValue());
 }
 
+std::string parseSetCookieValue(const Http::HeaderMap& headers, const std::string& key) {
+
+  std::string ret;
+
+  headers.iterateReverse([&key, &ret](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate {
+    // Find the cookie headers in the request (typically, there's only one).
+    if (header.key() == Http::Headers::get().SetCookie.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 Http::HeaderMap::Iterate::Break;
+        }
+      }
+    }
+    return Http::HeaderMap::Iterate::Continue;
+  });
+
+  return ret;
+}
+
 TEST_F(OauthIntegrationTest, AuthenticationFlow) {
   initialize();
 
+  Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher();
+  Filesystem::Watcher::OnChangedCb on_changed_cb_;
+  watcher->addWatch(TestEnvironment::temporaryPath("token_secret.yaml"),
+                    Filesystem::Watcher::Events::MovedTo,
+                    [&](uint32_t) -> void { dispatcher_->exit(); });
+
+  watcher->addWatch(TestEnvironment::temporaryPath("hmac_secret.yaml"),
+                    Filesystem::Watcher::Events::MovedTo,
+                    [&](uint32_t) -> void { dispatcher_->exit(); });
+
+  TestEnvironment::renameFile(TestEnvironment::temporaryPath("token_secret_1.yaml"),
+                              TestEnvironment::temporaryPath("token_secret.yaml"));
+  dispatcher_->run(Event::Dispatcher::RunType::Block);
+  TestEnvironment::renameFile(TestEnvironment::temporaryPath("hmac_secret_1.yaml"),
+                              TestEnvironment::temporaryPath("hmac_secret.yaml"));
+
+  dispatcher_->run(Event::Dispatcher::RunType::Block);
+
   codec_client_ = makeHttpConnection(lookupPort("http"));
 
   Http::TestRequestHeaderMapImpl headers{
@@ -119,6 +212,8 @@ TEST_F(OauthIntegrationTest, AuthenticationFlow) {
       {"x-forwarded-proto", "http"},
       {":authority", "authority"},
       {"authority", "Bearer token"}};
+  absl::string_view host_ = headers.Host()->value().getStringView();
+
   auto encoder_decoder = codec_client_->startRequest(headers);
   request_encoder_ = &encoder_decoder.first;
   auto response = std::move(encoder_decoder.second);
@@ -127,6 +222,13 @@ TEST_F(OauthIntegrationTest, AuthenticationFlow) {
 
   ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
 
+  std::string request_body = upstream_request_->body().toString();
+  const auto query_parameters = Http::Utility::parseFromBody(request_body);
+  auto it = query_parameters.find("client_secret");
+
+  ASSERT_TRUE(it != query_parameters.end());
+  EXPECT_EQ(it->second, "token_secret_1");
+
   upstream_request_->encodeHeaders(
       Http::TestRequestHeaderMapImpl{{":status", "200"}, {"content-type", "application/json"}},
       false);
@@ -140,6 +242,22 @@ TEST_F(OauthIntegrationTest, AuthenticationFlow) {
 
   // We should get an immediate redirect back.
   response->waitForHeaders();
+
+  const Http::ResponseHeaderMap& headers_ = response->headers();
+  std::string expires_ = parseSetCookieValue(headers_, "OauthExpires");
+  std::string token_ = parseSetCookieValue(headers_, "BearerToken");
+  std::string hmac_ = parseSetCookieValue(headers_, "OauthHMAC");
+  auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get();
+  const auto hmac_payload = absl::StrCat(host_, expires_, token_);
+  std::string hmac_secret = "hmac_secret_1";
+  std::vector<uint8_t> hmac_secret_vec(hmac_secret.begin(), hmac_secret.end());
+  const auto pre_encoded_hmac =
+      Hex::encode(crypto_util.getSha256Hmac(hmac_secret_vec, hmac_payload));
+  std::string encoded_hmac;
+  absl::Base64Escape(pre_encoded_hmac, &encoded_hmac);
+
+  EXPECT_EQ(encoded_hmac, hmac_) << response->headers();
+
   EXPECT_EQ("302", response->headers().getStatusValue());
 }