From 0ac56ec6e27d9b6c56f13eaa62777333001d83d5 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Wed, 8 Jun 2022 22:43:12 +0000 Subject: [PATCH] oauth2: do not blindly accept requests with a token in the Authorization header (781) Signed-off-by: Raul Gutierrez Segales Signed-off-by: Matt Klein Signed-off-by: Pradeep Rao --- .../extensions/filters/http/oauth2/filter.cc | 58 ++----------- .../extensions/filters/http/oauth2/filter.h | 2 - .../filters/http/oauth2/oauth_client.cc | 3 - .../filters/http/oauth2/filter_test.cc | 86 ++++++++++++------- 4 files changed, 61 insertions(+), 88 deletions(-) diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 78c8a16d83e9..41d62f1e6965 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -191,31 +191,6 @@ const std::string& OAuth2Filter::bearerPrefix() const { CONSTRUCT_ON_FIRST_USE(std::string, "bearer "); } -std::string OAuth2Filter::extractAccessToken(const Http::RequestHeaderMap& headers) const { - ASSERT(headers.Path() != nullptr); - - // Start by looking for a bearer token in the Authorization header. - const Http::HeaderEntry* authorization = headers.getInline(authorization_handle.handle()); - if (authorization != nullptr) { - const auto value = StringUtil::trim(authorization->value().getStringView()); - const auto& bearer_prefix = bearerPrefix(); - if (absl::StartsWithIgnoreCase(value, bearer_prefix)) { - const size_t start = bearer_prefix.length(); - return std::string(StringUtil::ltrim(value.substr(start))); - } - } - - // Check for the named query string parameter. - const auto path = headers.Path()->value().getStringView(); - const auto params = Http::Utility::parseQueryString(path); - const auto param = params.find("token"); - if (param != params.end()) { - return param->second; - } - - return EMPTY_STRING; -} - /** * primary cases: * 1) user is signing out @@ -224,6 +199,10 @@ std::string OAuth2Filter::extractAccessToken(const Http::RequestHeaderMap& heade * 4) user is unauthorized */ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { + // Sanitize the Authorization header, since we have no way to validate its content. Also, + // if token forwarding is enabled, this header will be set based on what is on the HMAC cookie + // before forwarding the request upstream. + headers.removeInline(authorization_handle.handle()); // The following 2 headers are guaranteed for regular requests. The asserts are helpful when // writing test code to not forget these important variables in mock requests @@ -278,17 +257,7 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he request_headers_ = &headers; } - // If a bearer token is supplied as a header or param, we ingest it here and kick off the - // user resolution immediately. Note this comes after HMAC validation, so technically this - // header is sanitized in a way, as the validation check forces the correct Bearer Cookie value. - access_token_ = extractAccessToken(headers); - if (!access_token_.empty()) { - found_bearer_token_ = true; - finishFlow(); - return Http::FilterHeadersStatus::Continue; - } - - // If no access token and this isn't the callback URI, redirect to acquire credentials. + // If this isn't the callback URI, redirect to acquire credentials. // // The following conditional could be replaced with a regex pattern-match, // if we're concerned about strict matching against the callback path. @@ -417,18 +386,6 @@ void OAuth2Filter::onGetAccessTokenSuccess(const std::string& access_code, } void OAuth2Filter::finishFlow() { - - // We have fully completed the entire OAuth flow, whether through Authorization header or from - // user redirection to the auth server. - if (found_bearer_token_) { - if (config_->forwardBearerToken()) { - setBearerToken(*request_headers_, access_token_); - } - config_->stats().oauth_success_.inc(); - decoder_callbacks_->continueDecoding(); - return; - } - std::string token_payload; if (config_->forwardBearerToken()) { token_payload = absl::StrCat(host_, new_expires_, access_token_); @@ -450,8 +407,8 @@ void OAuth2Filter::finishFlow() { const std::string cookie_tail_http_only = fmt::format(CookieTailHttpOnlyFormatString, new_expires_); - // At this point we have all of the pieces needed to authorize a user that did not originally - // have a bearer access token. Now, we construct a redirect request to return the user to their + // At this point we have all of the pieces needed to authorize a user. + // Now, we construct a redirect request to return the user to their // previous state and additionally set the OAuth cookies in browser. // The redirection should result in successfully passing this filter. Http::ResponseHeaderMapPtr response_headers{Http::createHeaderMap( @@ -475,7 +432,6 @@ void OAuth2Filter::finishFlow() { decoder_callbacks_->encodeHeaders(std::move(response_headers), true, REDIRECT_LOGGED_IN); config_->stats().oauth_success_.inc(); - decoder_callbacks_->continueDecoding(); } void OAuth2Filter::sendUnauthorizedResponse() { diff --git a/source/extensions/filters/http/oauth2/filter.h b/source/extensions/filters/http/oauth2/filter.h index 6722845fddaa..4b8db382a2d7 100644 --- a/source/extensions/filters/http/oauth2/filter.h +++ b/source/extensions/filters/http/oauth2/filter.h @@ -214,7 +214,6 @@ class OAuth2Filter : public Http::PassThroughDecoderFilter, public FilterCallbac std::string new_expires_; absl::string_view host_; std::string state_; - bool found_bearer_token_{false}; Http::RequestHeaderMap* request_headers_{nullptr}; std::unique_ptr oauth_client_; @@ -228,7 +227,6 @@ class OAuth2Filter : public Http::PassThroughDecoderFilter, public FilterCallbac Http::FilterHeadersStatus signOutUser(const Http::RequestHeaderMap& headers); const std::string& bearerPrefix() const; - std::string extractAccessToken(const Http::RequestHeaderMap& headers) const; }; } // namespace Oauth2 diff --git a/source/extensions/filters/http/oauth2/oauth_client.cc b/source/extensions/filters/http/oauth2/oauth_client.cc index 24335ff6c390..7c6913df362c 100644 --- a/source/extensions/filters/http/oauth2/oauth_client.cc +++ b/source/extensions/filters/http/oauth2/oauth_client.cc @@ -20,9 +20,6 @@ namespace HttpFilters { namespace Oauth2 { namespace { -Http::RegisterCustomInlineHeader - authorization_handle(Http::CustomHeaders::get().Authorization); - constexpr const char* GetAccessTokenBodyFormatString = "grant_type=authorization_code&code={0}&client_id={1}&client_secret={2}&redirect_uri={3}"; diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 129c0ec0fdfc..44487866735e 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -95,7 +95,7 @@ class OAuth2Test : public testing::Test { } // Set up proto fields with standard config. - FilterConfigSharedPtr getConfig() { + FilterConfigSharedPtr getConfig(bool forward_bearer_token = true) { envoy::extensions::filters::http::oauth2::v3::OAuth2Config p; auto* endpoint = p.mutable_token_endpoint(); endpoint->set_cluster("auth.example.com"); @@ -105,7 +105,7 @@ class OAuth2Test : public testing::Test { 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); + p.set_forward_bearer_token(forward_bearer_token); p.add_auth_scopes("user"); p.add_auth_scopes("openid"); p.add_auth_scopes("email"); @@ -379,6 +379,50 @@ TEST_F(OAuth2Test, OAuthOkPass) { EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 1); } +/** + * Scenario: The OAuth filter receives a request to an arbitrary path with valid OAuth cookies + * (cookie values and validation are mocked out), but with an invalid token in the Authorization + * header and forwarding bearer token is disabled. + * + * Expected behavior: the filter should sanitize the Authorization header and let the request + * proceed. + */ +TEST_F(OAuth2Test, OAuthOkPassButInvalidToken) { + init(getConfig(false /* forward_bearer_token */)); + + Http::TestRequestHeaderMapImpl mock_request_headers{ + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + {Http::CustomHeaders::get().Authorization.get(), "Bearer injected_malice!"}, + }; + + Http::TestRequestHeaderMapImpl expected_headers{ + {Http::Headers::get().Path.get(), "/anypath"}, + {Http::Headers::get().Host.get(), "traffic.example.com"}, + {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, + {Http::Headers::get().Scheme.get(), "https"}, + }; + + // cookie-validation mocking + EXPECT_CALL(*validator_, setParams(_, _)); + EXPECT_CALL(*validator_, isValid()).WillOnce(Return(true)); + + // Sanitized return reference mocking + std::string legit_token{"legit_token"}; + EXPECT_CALL(*validator_, token()).WillRepeatedly(ReturnRef(legit_token)); + + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_->decodeHeaders(mock_request_headers, false)); + + // Ensure that existing OAuth forwarded headers got sanitized. + EXPECT_EQ(mock_request_headers, expected_headers); + + EXPECT_EQ(scope_.counterFromString("test.oauth_failure").value(), 0); + EXPECT_EQ(scope_.counterFromString("test.oauth_success").value(), 1); +} + /** * Scenario: The OAuth filter receives a request without valid OAuth cookies to a non-callback URL * (indicating that the user needs to re-validate cookies or get 401'd). @@ -771,21 +815,12 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&second_response_headers), true)); - EXPECT_CALL(decoder_callbacks_, continueDecoding()); filter_->finishFlow(); } TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { - Http::TestRequestHeaderMapImpl request_headers_before{ - {Http::Headers::get().Path.get(), "/test?role=bearer"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "https"}, - {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}, - }; - // Expected decoded headers after the callback & validation of the bearer token is complete. - Http::TestRequestHeaderMapImpl request_headers_after{ + Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/test?role=bearer"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, @@ -793,41 +828,28 @@ TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}, }; - // Fail the validation to trigger the OAuth flow. + // Fail the validation. EXPECT_CALL(*validator_, setParams(_, _)); EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_->decodeHeaders(request_headers_before, false)); - - // Finally, expect that the header map had OAuth information appended to it. - EXPECT_EQ(request_headers_before, request_headers_after); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers, false)); } TEST_F(OAuth2Test, OAuthBearerTokenFlowFromQueryParameters) { - Http::TestRequestHeaderMapImpl request_headers_before{ - {Http::Headers::get().Path.get(), "/test?role=bearer&token=xyz-queryparam-token"}, - {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().Scheme.get(), "https"}, - }; - Http::TestRequestHeaderMapImpl request_headers_after{ + Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/test?role=bearer&token=xyz-queryparam-token"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, {Http::Headers::get().Scheme.get(), "https"}, - {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-queryparam-token"}, }; - // Fail the validation to trigger the OAuth flow. + // Fail the validation. EXPECT_CALL(*validator_, setParams(_, _)); EXPECT_CALL(*validator_, isValid()).WillOnce(Return(false)); - EXPECT_EQ(Http::FilterHeadersStatus::Continue, - filter_->decodeHeaders(request_headers_before, false)); - - // Expected decoded headers after the callback & validation of the bearer token is complete. - EXPECT_EQ(request_headers_before, request_headers_after); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_->decodeHeaders(request_headers, false)); } } // namespace Oauth2