fix oauth filter sds update by Inode1 · Pull Request #16253 · envoyproxy/envoy

Inode1 · 2021-04-30T14:38:18Z

Additional Description:
The client_secret field was not updated on configuration changes.

Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: inline
Fixes #16126

repokitteh-read-only · 2021-04-30T14:38:21Z

Hi @Inode1, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16253 was opened by Inode1.

see: more, trace.

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

dmitri-d · 2021-04-30T16:53:55Z

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

 private:
-  void readAndWatchSecret(std::string& value,
-                          Secret::GenericSecretConfigProvider& secret_provider) {
+  void readAndWatchSecret(std::string& value, Secret::GenericSecretConfigProvider& secret_provider,


Could you return the update_callback from the method instead of using a parameter? I think it would be more readable...

Good point.

dmitri-d · 2021-04-30T16:55:37Z

Could you provide a brief explanation of how this fixes the problem in #16126? Also, could you add tests?

Inode1 · 2021-04-30T19:35:22Z

Yup. This method secret_provider->addUpdateCallback calls this at the end. And if we don't save results in unique_ptr it will be destroyed and deleted from the callbacks list according to this. We need to create two objects here. If we don't do it the update_callback_ will be rewrited and callback will be deleted.

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

dmitri-d · 2021-04-30T21:30:56Z

Nice catch, @Inode1. Could you add a test?

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 · 2021-05-02T08:02:50Z

Here you are, check this out.

Inode1 · 2021-05-03T10:56:42Z

/retest

repokitteh-read-only · 2021-05-03T10:56:46Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16253 (comment) was created by @Inode1.

see: more, trace.

dmitri-d · 2021-05-03T18:07:06Z

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

+
+  token_callback->onConfigUpdate(decoded_resources_token.refvec_, "");
+  EXPECT_EQ(secret_reader.clientSecret(), "client_test");
+  EXPECT_EQ(secret_reader.tokenSecret(), "token_test");


Could you add another check where token stays the same, but the client secret is updated again? To make sure that an update to client secret doesn't overwrite the token.

dmitri-d · 2021-05-03T18:07:53Z

Lgtm, just a small ask in the test.

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

snowp

Thanks! Could you include a bit more information in the PR description that describes that the bug is and how this fixes it?

Also, do you think it would be possible to write an integration test for this? Would be good to have a test that verifies this end to end.

snowp · 2021-05-03T20:33:35Z

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

  Event::SimulatedTimeSystem test_time_;
 };

+// Verifies that the OAuth SDSSecretReader correctly updates dynamic generic secret


nit: end in period to keep comments using proper grammar

snowp · 2021-05-03T20:59:22Z

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

+)EOF";
+
+  envoy::extensions::transport_sockets::tls::v3::Secret typed_secret;
+  TestUtility::loadFromYaml(TestEnvironment::substitute(yaml_client), typed_secret);


I don't think you need to use substitute here and elsewhere since the input doesn't use any of the substitution formats.

snowp · 2021-05-03T21:05:08Z

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

+TEST_F(OAuth2Test, SdsDynamicGenericSecret) {
+  NiceMock<Server::MockConfigTracker> config_tracker;
+  std::unique_ptr<Secret::SecretManager> secret_manager(
+      new Secret::SecretManagerImpl(config_tracker));


You can probably get by here by allocating this on the stack?

snowp · 2021-05-03T21:10:33Z

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

+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> secret_context;
+  NiceMock<LocalInfo::MockLocalInfo> local_info;
+  Api::ApiPtr api = Api::createApiForTest();
+  Stats::IsolatedStoreImpl stats;


At least this one can use the store_ defined on the fixture class instead of redefining one here

I think it will be more readable if we leave it in the function scope to save the context.

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 · 2021-05-04T11:46:50Z

I don't know how to update secrets in integration tests, so if you give me a hint, I think I can do it.

snowp · 2021-05-06T16:19:57Z

Maybe try looking at sds_static_integration_test or sds_dynamic_integration_test for how that sets up SDS triggers?

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 · 2021-05-08T13:06:33Z

/retest

repokitteh-read-only · 2021-05-08T13:06:36Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16253 (comment) was created by @Inode1.

see: more, trace.

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 · 2021-05-09T10:25:03Z

@snowp could you review the test and could you check this and this I think it's not thread safe to use it this way.

snowp

Thanks for getting an integration test together!

Re your question on thread safety it's hard to say without deeper inspection. What specifically makes you think that it's not thread safe? At a quick look it does seem problematic that we're just updating the secrets from the main thread and then presumably read from the filters? We might want to make use of a TLS slot here to avoid that, but if we can get an integration test reproducing the issue first that would be awesome.

snowp · 2021-05-10T16:40:22Z

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

  EXPECT_EQ("302", response->headers().getStatusValue());
 }

+std::string parseSetCookieValue(const Http::HeaderMap& headers, const std::string& key) {


Can you use the helper in http/utlity.h instead of redefining this here?

snowp · 2021-05-10T16:41:40Z

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

  initialize();

+  Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher();
+  Filesystem::Watcher::OnChangedCb on_changed_cb_;


snowp · 2021-05-10T16:43:50Z

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

 TEST_F(OauthIntegrationTest, AuthenticationFlow) {
  initialize();

+  Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher();


Maybe extract this stuff into a setup function? Just to declutter the test file

Also: does it make sense to do one authentication flow, reload the secrets, then do another one? That way we're verifying a real world secret rotation scenario

snowp · 2021-05-10T16:44:31Z

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

+  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);


It might be simpler to wait for a stat to update that reflects the secret reload, assuming we have one?

snowp · 2021-05-10T16:45:26Z

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

      {"x-forwarded-proto", "http"},
      {":authority", "authority"},
      {"authority", "Bearer token"}};
+  absl::string_view host_ = headers.Host()->value().getStringView();


The _ suffix is reserved for field members, so I'd call this host instead

snowp · 2021-05-10T16:46:23Z

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

+  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);


Putting this in a helper function might help readability, its quite a bit of code just to verify the hmac

snowp · 2021-05-10T16:46:50Z

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

+  std::string expires_ = parseSetCookieValue(headers_, "OauthExpires");
+  std::string token_ = parseSetCookieValue(headers_, "BearerToken");
+  std::string hmac_ = parseSetCookieValue(headers_, "OauthHMAC");


Remove the _ suffix since these aren't field variables

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 · 2021-05-11T19:58:33Z

Thank you, @snowp your remarks were taken into consideration. So I will recheck my suspicions about thread safe and if this is confirmed I will open a new issue.

Inode1 · 2021-05-12T04:11:11Z

/retest

repokitteh-read-only · 2021-05-12T04:11:15Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16253 (comment) was created by @Inode1.

see: more, trace.

snowp

Thanks this looks great, just a few more comments and then I think we're good to go

snowp · 2021-05-12T17:36:18Z

source/common/http/utility.h

+ * @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);


Mind adding a test for this function? Ideally we have coverage for this outside of extensions. Thanks!

I can copy-past some tests from utility_test.cpp with parseCookieValue check and trasnform them for parseSetCookieValue. Will it be okay?

snowp · 2021-05-12T17:37:38Z

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

+
+    OAuth2CookieValidator validator{api_->timeSource()};
+    validator.setParams(validate_headers, std::string(hmac_secret));
+    ASSERT_TRUE(validator.isValid());


if you return validator.isValid() here instead you can do an EXPECT_TRUE at call site and provide better error messaging

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

snowp

Thanks!

The client_secret field was not updated on configuration changes. Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru> Signed-off-by: James Mulcahy <jmulcahy@netflix.com>

fix oauth filter sds update

5108686

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 force-pushed the oauth-sds-fix branch from d80cf93 to 5108686 Compare April 30, 2021 14:49

fix format check

948a0f4

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 force-pushed the oauth-sds-fix branch from c435241 to 948a0f4 Compare April 30, 2021 15:56

dmitri-d reviewed Apr 30, 2021

View reviewed changes

code refactoring

6ee2773

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

add test for oauth dynamic sds

d4b5c12

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

ggreenway assigned snowp May 3, 2021

dmitri-d reviewed May 3, 2021

View reviewed changes

small changes in the test

f8c5d39

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

snowp suggested changes May 3, 2021

View reviewed changes

more changes in the tests

f4b3755

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 requested a review from snowp May 4, 2021 12:37

i.makarychev added 3 commits May 7, 2021 14:19

intefration test for the dynamic secret

d166c84

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

fix secret update race condition

cd71d20

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

delete unuse test config

e87f907

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

i.makarychev added 2 commits May 8, 2021 19:23

set resource_api_version explicitly

4ac587a

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

fix tsan build

05cee4f

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

snowp suggested changes May 10, 2021

View reviewed changes

review code fix

21e7131

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

snowp suggested changes May 12, 2021

View reviewed changes

add tests for parseSetCookie

4da1c5b

Signed-off-by: i.makarychev <i.makarychev@tinkoff.ru>

Inode1 force-pushed the oauth-sds-fix branch from 4c8c33a to 4da1c5b Compare May 12, 2021 19:02

snowp approved these changes May 14, 2021

View reviewed changes

snowp merged commit 1f4ca4c into envoyproxy:main May 14, 2021

Conversation

Inode1 commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d commented Apr 30, 2021

Uh oh!

Inode1 commented Apr 30, 2021

Uh oh!

dmitri-d commented Apr 30, 2021

Uh oh!

Inode1 commented May 2, 2021

Uh oh!

Inode1 commented May 3, 2021

Uh oh!

repokitteh-read-only bot commented May 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitri-d commented May 3, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Inode1 commented May 4, 2021

Uh oh!

snowp commented May 6, 2021

Uh oh!

Inode1 commented May 8, 2021

Uh oh!

repokitteh-read-only bot commented May 8, 2021

Uh oh!

Inode1 commented May 9, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Inode1 commented May 11, 2021

Uh oh!

Inode1 commented May 12, 2021

Uh oh!

repokitteh-read-only bot commented May 12, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Inode1 commented Apr 30, 2021 •

edited

Loading

Inode1 May 12, 2021 •

edited

Loading