-
Notifications
You must be signed in to change notification settings - Fork 5.5k
oauth2 filter: Make OAuth scopes configurable. #14168
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.
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
Changes from 37 commits
80acc89
c276e54
d26dd3a
1577ed6
47f1fa4
906a91f
94fa8b1
2fdf64a
7c689b9
6549060
5e709f9
c2567ec
657e21f
a7acd87
5381f74
4d9eab9
5e126b9
b64e322
7359ad3
62f1ff4
b3c2071
a18a184
2aa7274
a097339
47598a2
6cca203
a5d64d0
5aef557
b956a6e
e8393a7
9fa051d
ee4cb01
40fe4b8
966e04c
5109f3e
0e05c16
ecf6616
21b9d65
527a144
f4226a4
82fd52b
4625f33
c936cbb
aedc096
897b290
9705b0d
a83fd6f
ab0a563
f9f9f7a
f0efcf4
be36649
6c9c0d9
8da1892
0a33292
e63ec2b
f2cdd88
96ebfcd
9fc7acb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ Minor Behavior Changes | |
| * jwt_authn filter: added support of Jwt time constraint verification with a clock skew (default to 60 seconds) and added a filter config field :ref:`clock_skew_seconds <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.clock_skew_seconds>` to configure it. | ||
| * memory: enable new tcmalloc with restartable sequences for aarch64 builds. | ||
| * mongo proxy metrics: swapped network connection remote and local closed counters previously set reversed (`cx_destroy_local_with_active_rq` and `cx_destroy_remote_with_active_rq`). | ||
| * oauth filter: added the optional parameter :ref:`auth_scopes <config_http_filters_oauth>` with default value of 'user' if not provided. Enables this value to be overridden in the Authorization request to the OAuth provider. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you have this link to the field? Should be lots of examples in this file |
||
| * tls: removed RSA key transport and SHA-1 cipher suites from the client-side defaults. | ||
| * watchdog: the watchdog action :ref:`abort_action <envoy_v3_api_msg_watchdog.v3alpha.AbortActionConfig>` is now the default action to terminate the process if watchdog kill / multikill is enabled. | ||
| * xds: to support TTLs, heartbeating has been added to xDS. As a result, responses that contain empty resources without updating the version will no longer be propagated to the | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ secretsProvider(const envoy::extensions::transport_sockets::tls::v3::SdsSecretCo | |
| return secret_manager.findStaticGenericSecretProvider(config.name()); | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| } // namespace | ||
|
|
||
| Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ constexpr const char* CookieTailHttpOnlyFormatString = | |
| ";version=1;path=/;Max-Age={};secure;HttpOnly"; | ||
|
|
||
| const char* AuthorizationEndpointFormat = | ||
| "{}?client_id={}&scope=user&response_type=code&redirect_uri={}&state={}"; | ||
| "{}?client_id={}&scope={}&response_type=code&redirect_uri={}&state={}"; | ||
|
|
||
| constexpr absl::string_view UnauthorizedBodyMessage = "OAuth flow failed."; | ||
|
|
||
|
|
@@ -60,6 +60,7 @@ constexpr absl::string_view REDIRECT_RACE = "oauth.race_redirect"; | |
| constexpr absl::string_view REDIRECT_LOGGED_IN = "oauth.logged_in"; | ||
| constexpr absl::string_view REDIRECT_FOR_CREDENTIALS = "oauth.missing_credentials"; | ||
| constexpr absl::string_view SIGN_OUT = "oauth.sign_out"; | ||
| constexpr absl::string_view DEFAULT_AUTH_SCOPE = "user"; | ||
|
|
||
| template <class T> | ||
| std::vector<Http::HeaderUtility::HeaderData> headerMatchers(const T& matcher_protos) { | ||
|
|
@@ -73,6 +74,25 @@ std::vector<Http::HeaderUtility::HeaderData> headerMatchers(const T& matcher_pro | |
| return matchers; | ||
| } | ||
|
|
||
| // Transforms the proto list of 'auth_scopes' into a vector of std::string, also | ||
| // handling the default value logic | ||
| template <class T> std::vector<std::string> authScopesList(const T& auth_scopes_protos) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just use the actual type (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snowp Should I change to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snowp is that what you mean?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking to remove the T from the paramter list and use the real type instead, so If we need to convert to vectorstd::string` then we'd keep that on the class, if not we don't even need this conversion function
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. Added a commit to fix this. |
||
| std::vector<std::string> scopes; | ||
|
|
||
| // if 'auth_scopes' is empty it must return a list with the default value. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: proper grammar in comments, so
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| if (auth_scopes_protos.empty()) { | ||
| scopes.reserve(1); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reserving one seems redundant, I imagine it's only useful to avoid multiple allocations from happening by telling the vector that multiple entries will be inserted
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @snowp do you think just removing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
| scopes.emplace_back(DEFAULT_AUTH_SCOPE); | ||
| } else { | ||
| scopes.reserve(auth_scopes_protos.size()); | ||
|
|
||
| for (const auto& scope : auth_scopes_protos) { | ||
| scopes.emplace_back(scope); | ||
| } | ||
| } | ||
| return scopes; | ||
| } | ||
|
|
||
|
snowp marked this conversation as resolved.
|
||
| // Sets the auth token as the Bearer token in the authorization header. | ||
| void setBearerToken(Http::RequestHeaderMap& headers, const std::string& token) { | ||
| headers.setInline(authorization_handle.handle(), absl::StrCat("Bearer ", token)); | ||
|
|
@@ -90,6 +110,7 @@ FilterConfig::FilterConfig( | |
| redirect_matcher_(proto_config.redirect_path_matcher()), | ||
| signout_path_(proto_config.signout_path()), secret_reader_(secret_reader), | ||
| stats_(FilterConfig::generateStats(stats_prefix, scope)), | ||
| auth_scopes_(authScopesList(proto_config.auth_scopes())), | ||
| forward_bearer_token_(proto_config.forward_bearer_token()), | ||
| pass_through_header_matchers_(headerMatchers(proto_config.pass_through_matcher())) { | ||
| if (!cluster_manager.get(oauth_token_endpoint_.cluster())) { | ||
|
|
@@ -275,9 +296,11 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he | |
| const std::string escaped_redirect_uri = | ||
| Http::Utility::PercentEncoding::encode(redirect_uri, ":/=&?"); | ||
|
|
||
| const std::string escaped_auth_scopes = absl::StrJoin(config_->authScopes(), "%20"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the scopes also be escaped (i.e., joined with spaces and using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adisuissa I tried to join the list with whitespace and use PercentEncoding::encode, but this function seems to do not replace the whitespace character with percent. Then I just used %20 instead. Do you mean something different?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the names of the auth scopes require escaping as well? You should be able to include |
||
|
|
||
| const std::string new_url = | ||
| fmt::format(AuthorizationEndpointFormat, config_->authorizationEndpoint(), | ||
| config_->clientId(), escaped_redirect_uri, escaped_state); | ||
| config_->clientId(), escaped_auth_scopes, escaped_redirect_uri, escaped_state); | ||
| response_headers->setLocation(new_url); | ||
| decoder_callbacks_->encodeHeaders(std::move(response_headers), true, REDIRECT_FOR_CREDENTIALS); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,7 @@ class FilterConfig { | |
| std::string clientSecret() const { return secret_reader_->clientSecret(); } | ||
| std::string tokenSecret() const { return secret_reader_->tokenSecret(); } | ||
| FilterStats& stats() { return stats_; } | ||
| const std::vector<std::string>& authScopes() const { return auth_scopes_; } | ||
|
|
||
| private: | ||
| static FilterStats generateStats(const std::string& prefix, Stats::Scope& scope); | ||
|
|
@@ -135,6 +136,7 @@ class FilterConfig { | |
| const Matchers::PathMatcher signout_path_; | ||
| std::shared_ptr<SecretReader> secret_reader_; | ||
| FilterStats stats_; | ||
| const std::vector<std::string> auth_scopes_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is no longer used
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, just removed it |
||
| const bool forward_bearer_token_ : 1; | ||
| const std::vector<Http::HeaderUtility::HeaderData> pass_through_header_matchers_; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ void OAuth2ClientImpl::onSuccess(const Http::AsyncClient::Request&, | |
| const auto response_code = message->headers().Status()->value().getStringView(); | ||
| if (response_code != "200") { | ||
| ENVOY_LOG(debug, "Oauth response code: {}", response_code); | ||
| ENVOY_LOG(debug, "Oauth response body: {}", message->bodyAsString()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid of the amount of data that would be logged here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me clarify this @joaovitor. What is being logged here is the response body of the request to the OAuth provider. This is not the upstream request (to service being proxied in the end). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great @andreyprezotto ... Hope to see this PR merged. |
||
| parent_->sendUnauthorizedResponse(); | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ static const std::string TEST_CALLBACK = "/_oauth"; | |
| static const std::string TEST_CLIENT_ID = "1"; | ||
| static const std::string TEST_CLIENT_SECRET_ID = "MyClientSecretKnoxID"; | ||
| static const std::string TEST_TOKEN_SECRET_ID = "MyTokenSecretKnoxID"; | ||
| static const std::string TEST_ENCODED_AUTH_SCOPES = "user%20openid%20email"; | ||
|
|
||
| namespace { | ||
| Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders> | ||
|
|
@@ -91,6 +92,9 @@ class OAuth2Test : public testing::Test { | |
| p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); | ||
| p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); | ||
| p.set_forward_bearer_token(true); | ||
| p.add_auth_scopes("user"); | ||
| p.add_auth_scopes("openid"); | ||
| p.add_auth_scopes("email"); | ||
| auto* matcher = p.add_pass_through_matcher(); | ||
| matcher->set_name(":method"); | ||
| matcher->set_exact_match("OPTIONS"); | ||
|
|
@@ -147,6 +151,43 @@ TEST_F(OAuth2Test, InvalidCluster) { | |
| "specify which cluster to direct OAuth requests to."); | ||
| } | ||
|
|
||
| // Verifies that the OAuth config is created with a default value for auth_scopes field when it is | ||
| // not set in proto/yaml. | ||
| TEST_F(OAuth2Test, DefaultAuthScope) { | ||
|
|
||
| // Set up proto fields | ||
| envoy::extensions::filters::http::oauth2::v3alpha::OAuth2Config p; | ||
| auto* endpoint = p.mutable_token_endpoint(); | ||
| endpoint->set_cluster("auth.example.com"); | ||
| endpoint->set_uri("auth.example.com/_oauth"); | ||
| endpoint->mutable_timeout()->set_seconds(1); | ||
| p.set_redirect_uri("%REQ(x-forwarded-proto)%://%REQ(:authority)%" + TEST_CALLBACK); | ||
| p.mutable_redirect_path_matcher()->mutable_path()->set_exact(TEST_CALLBACK); | ||
| p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); | ||
| p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); | ||
| p.set_forward_bearer_token(true); | ||
| auto* matcher = p.add_pass_through_matcher(); | ||
| matcher->set_name(":method"); | ||
| matcher->set_exact_match("OPTIONS"); | ||
|
|
||
| auto credentials = p.mutable_credentials(); | ||
| credentials->set_client_id(TEST_CLIENT_ID); | ||
| credentials->mutable_token_secret()->set_name("secret"); | ||
| credentials->mutable_hmac_secret()->set_name("hmac"); | ||
|
|
||
| MessageUtil::validate(p, ProtobufMessage::getStrictValidationVisitor()); | ||
|
|
||
| // Create the OAuth config. | ||
| auto secret_reader = std::make_shared<MockSecretReader>(); | ||
| FilterConfigSharedPtr test_config_; | ||
| test_config_ = std::make_shared<FilterConfig>(p, factory_context_.cluster_manager_, secret_reader, | ||
| scope_, "test."); | ||
|
|
||
| // auth_scopes was not set, should return default value | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: same comment on comment grammar: proper case and end in period. Bonus point for complete sentence
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| std::vector<std::string> default_scope = {"user"}; | ||
| EXPECT_EQ(test_config_->authScopes(), default_scope); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like the only use case of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
| } | ||
|
|
||
| /** | ||
| * Scenario: The OAuth filter receives a sign out request. | ||
| * | ||
|
|
@@ -239,8 +280,8 @@ TEST_F(OAuth2Test, OAuthErrorNonOAuthHttpCallback) { | |
| {Http::Headers::get().Location.get(), | ||
| "https://auth.example.com/oauth/" | ||
| "authorize/?client_id=" + | ||
| TEST_CLIENT_ID + | ||
| "&scope=user&response_type=code&" | ||
| TEST_CLIENT_ID + "&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&response_type=code&" | ||
| "redirect_uri=http%3A%2F%2Ftraffic.example.com%2F" | ||
| "_oauth&state=http%3A%2F%2Ftraffic.example.com%2Fnot%2F_oauth"}, | ||
| }; | ||
|
|
@@ -392,8 +433,8 @@ TEST_F(OAuth2Test, OAuthTestInvalidUrlInStateQueryParam) { | |
| Http::TestRequestHeaderMapImpl request_headers{ | ||
| {Http::Headers::get().Host.get(), "traffic.example.com"}, | ||
| {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, | ||
| {Http::Headers::get().Path.get(), "/_oauth?code=abcdefxyz123&scope=user&" | ||
| "state=blah"}, | ||
| {Http::Headers::get().Path.get(), | ||
| "/_oauth?code=abcdefxyz123&scope=" + TEST_ENCODED_AUTH_SCOPES + "&state=blah"}, | ||
| {Http::Headers::get().Cookie.get(), "OauthExpires=123;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), "BearerToken=legit_token;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), | ||
|
|
@@ -426,8 +467,9 @@ TEST_F(OAuth2Test, OAuthTestCallbackUrlInStateQueryParam) { | |
| Http::TestRequestHeaderMapImpl request_headers{ | ||
| {Http::Headers::get().Host.get(), "traffic.example.com"}, | ||
| {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, | ||
| {Http::Headers::get().Path.get(), "/_oauth?code=abcdefxyz123&scope=user&" | ||
| "state=https%3A%2F%2Ftraffic.example.com%2F_oauth"}, | ||
| {Http::Headers::get().Path.get(), | ||
| "/_oauth?code=abcdefxyz123&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&state=https%3A%2F%2Ftraffic.example.com%2F_oauth"}, | ||
| {Http::Headers::get().Cookie.get(), "OauthExpires=123;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), "BearerToken=legit_token;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), | ||
|
|
@@ -457,8 +499,9 @@ TEST_F(OAuth2Test, OAuthTestCallbackUrlInStateQueryParam) { | |
| Http::TestRequestHeaderMapImpl final_request_headers{ | ||
| {Http::Headers::get().Host.get(), "traffic.example.com"}, | ||
| {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, | ||
| {Http::Headers::get().Path.get(), "/_oauth?code=abcdefxyz123&scope=user&" | ||
| "state=https%3A%2F%2Ftraffic.example.com%2F_oauth"}, | ||
| {Http::Headers::get().Path.get(), | ||
| "/_oauth?code=abcdefxyz123&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&state=https%3A%2F%2Ftraffic.example.com%2F_oauth"}, | ||
| {Http::Headers::get().Cookie.get(), "OauthExpires=123;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), "BearerToken=legit_token;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), | ||
|
|
@@ -482,8 +525,9 @@ TEST_F(OAuth2Test, OAuthTestUpdatePathAfterSuccess) { | |
| Http::TestRequestHeaderMapImpl request_headers{ | ||
| {Http::Headers::get().Host.get(), "traffic.example.com"}, | ||
| {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, | ||
| {Http::Headers::get().Path.get(), "/_oauth?code=abcdefxyz123&scope=user&" | ||
| "state=https%3A%2F%2Ftraffic.example.com%2Foriginal_path"}, | ||
| {Http::Headers::get().Path.get(), | ||
| "/_oauth?code=abcdefxyz123&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&state=https%3A%2F%2Ftraffic.example.com%2Foriginal_path"}, | ||
| {Http::Headers::get().Cookie.get(), "OauthExpires=123;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), "BearerToken=legit_token;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), | ||
|
|
@@ -511,8 +555,9 @@ TEST_F(OAuth2Test, OAuthTestUpdatePathAfterSuccess) { | |
| Http::TestRequestHeaderMapImpl final_request_headers{ | ||
| {Http::Headers::get().Host.get(), "traffic.example.com"}, | ||
| {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, | ||
| {Http::Headers::get().Path.get(), "/_oauth?code=abcdefxyz123&scope=user&" | ||
| "state=https%3A%2F%2Ftraffic.example.com%2Foriginal_path"}, | ||
| {Http::Headers::get().Path.get(), | ||
| "/_oauth?code=abcdefxyz123&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&state=https%3A%2F%2Ftraffic.example.com%2Foriginal_path"}, | ||
| {Http::Headers::get().Cookie.get(), "OauthExpires=123;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), "BearerToken=legit_token;version=test"}, | ||
| {Http::Headers::get().Cookie.get(), | ||
|
|
@@ -545,8 +590,8 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { | |
| {Http::Headers::get().Location.get(), | ||
| "https://auth.example.com/oauth/" | ||
| "authorize/?client_id=" + | ||
| TEST_CLIENT_ID + | ||
| "&scope=user&response_type=code&" | ||
| TEST_CLIENT_ID + "&scope=" + TEST_ENCODED_AUTH_SCOPES + | ||
| "&response_type=code&" | ||
| "redirect_uri=https%3A%2F%2Ftraffic.example.com%2F" | ||
| "_oauth&state=https%3A%2F%2Ftraffic.example.com%2Ftest%" | ||
| "3Fname%3Dadmin%26level%3Dtrace"}, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.