Skip to content

fix oauth filter sds update #16253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
snowp merged 13 commits into from
May 14, 2021
Merged

fix oauth filter sds update #16253

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5108686
fix oauth filter sds update
Apr 30, 2021
948a0f4
fix format check
Apr 30, 2021
6ee2773
code refactoring
Apr 30, 2021
d4b5c12
add test for oauth dynamic sds
May 2, 2021
f8c5d39
small changes in the test
May 3, 2021
f4b3755
more changes in the tests
May 4, 2021
d166c84
intefration test for the dynamic secret
May 7, 2021
cd71d20
fix secret update race condition
May 7, 2021
e87f907
delete unuse test config
May 7, 2021
4ac587a
set resource_api_version explicitly
May 8, 2021
05cee4f
fix tsan build
May 8, 2021
21e7131
review code fix
May 11, 2021
4da1c5b
add tests for parseSetCookie
May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions source/extensions/filters/http/oauth2/filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
};

/**
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/oauth2/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,15 @@ 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",
"//source/extensions/filters/http/oauth2:oauth_lib",
"//test/integration:http_integration_lib",
"//test/mocks/server:server_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:environment_lib",
"@envoy_api//envoy/extensions/filters/http/oauth2/v3alpha:pkg_cc_proto",
],
)
Expand Down
65 changes: 65 additions & 0 deletions test/extensions/filters/http/oauth2/filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
#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"

#include "test/mocks/http/mocks.h"
#include "test/mocks/server/mocks.h"
#include "test/mocks/upstream/mocks.h"
#include "test/test_common/environment.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
Expand Down Expand Up @@ -155,6 +157,69 @@ class OAuth2Test : public testing::Test {
Event::SimulatedTimeSystem test_time_;
};

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

@snowp snowp May 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: end in period to keep comments using proper grammar

@Inode1 Inode1 May 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

@snowp snowp May 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Inode1 Inode1 May 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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;

@snowp snowp May 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Inode1 Inode1 May 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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(TestEnvironment::substitute(yaml_client), typed_secret);

@snowp snowp May 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Inode1 Inode1 May 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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(TestEnvironment::substitute(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");

@dmitri-d dmitri-d May 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
// 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())
Expand Down