Skip to content
Permalink
Browse files Browse the repository at this point in the history
oauth2: do not blindly accept requests with a token in the Authorizat…
…ion headera (781)

The logic was broken because it assumed an additional call would be
performed to the auth server, which isn't the case. Per the filter
documentation, a request is only considered subsequently authenticated
if there's valid cookie that was set after the access token was received
from the auth server:

https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/oauth2_filter

More info about how to validate an access token (which we don't do, per
above):

https://www.oauth.com/oauth2-servers/token-introspection-endpoint/
https://datatracker.ietf.org/doc/html/rfc7662

Also fix the fact that ee shouldn't be calling continueDecoding() after
decoder_callbacks_->encodeHeaders().

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Pradeep Rao <pcrao@google.com>
  • Loading branch information
pradeepcrao committed Jun 8, 2022
1 parent 9b1c396 commit 7ffda4e
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 88 deletions.
58 changes: 7 additions & 51 deletions source/extensions/filters/http/oauth2/filter.cc
Expand Up @@ -203,31 +203,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
Expand All @@ -236,6 +211,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
Expand Down Expand Up @@ -290,17 +269,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.
Expand Down Expand Up @@ -439,18 +408,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_, id_token_, refresh_token_);
Expand All @@ -472,8 +429,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<Http::ResponseHeaderMapImpl>(
Expand Down Expand Up @@ -509,7 +466,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() {
Expand Down
2 changes: 0 additions & 2 deletions source/extensions/filters/http/oauth2/filter.h
Expand Up @@ -243,7 +243,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<OAuth2Client> oauth_client_;
Expand All @@ -257,7 +256,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
Expand Down
3 changes: 0 additions & 3 deletions source/extensions/filters/http/oauth2/oauth_client.cc
Expand Up @@ -21,9 +21,6 @@ namespace HttpFilters {
namespace Oauth2 {

namespace {
Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
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}";

Expand Down
86 changes: 54 additions & 32 deletions test/extensions/filters/http/oauth2/filter_test.cc
Expand Up @@ -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");
Expand All @@ -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");
Expand Down Expand Up @@ -422,6 +422,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).
Expand Down Expand Up @@ -790,63 +834,41 @@ 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},
{Http::Headers::get().Scheme.get(), "https"},
{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
Expand Down

0 comments on commit 7ffda4e

Please sign in to comment.