Skip to content

Commit

Permalink
oauth2: add a separator for HMAC calculation
Browse files Browse the repository at this point in the history 
Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Boteng Yao <[email protected]>
  • Loading branch information
@lizan @howardjohn
lizan authored and howardjohn committed Jul 18, 2023
1 parent 884e129 commit da77eab
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 23 deletions.
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ bug_fixes:
server scope for stats instead of the listener's global scope. This fixes a
use-after-free that can occur if the listener is drained but the cached
gRPC access logger uses the listener's global scope for stats.
- area: oauth2
change: |
Fixed a cookie validator bug that meant the HMAC calculation could be the same for different payloads.
This prevents malicious clients from constructing credentials with permanent validity in some specific scenarios.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
40 changes: 21 additions & 19 deletions source/extensions/filters/http/oauth2/filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ constexpr absl::string_view REDIRECT_FOR_CREDENTIALS = "oauth.missing_credential
constexpr absl::string_view SIGN_OUT = "oauth.sign_out";
constexpr absl::string_view DEFAULT_AUTH_SCOPE = "user";

constexpr absl::string_view HmacPayloadSeparator = "\n";

template <class T>
std::vector<Http::HeaderUtility::HeaderData> headerMatchers(const T& matcher_protos) {
std::vector<Http::HeaderUtility::HeaderData> matchers;
Expand Down Expand Up @@ -143,6 +145,18 @@ Http::Utility::QueryParams buildAutorizationQueryParams(
absl::StrJoin(authScopesList(proto_config.auth_scopes()), " "), ":/=&? ");
return query_params;
}

std::string encodeHmac(const std::vector<uint8_t>& secret, absl::string_view host,
absl::string_view expires, absl::string_view token = "",
absl::string_view id_token = "", absl::string_view refresh_token = "") {
auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get();
const auto hmac_payload =
absl::StrJoin({host, expires, token, id_token, refresh_token}, HmacPayloadSeparator);
std::string encoded_hmac;
absl::Base64Escape(Hex::encode(crypto_util.getSha256Hmac(secret, hmac_payload)), &encoded_hmac);
return encoded_hmac;
}

} // namespace

FilterConfig::FilterConfig(
Expand Down Expand Up @@ -197,13 +211,7 @@ void OAuth2CookieValidator::setParams(const Http::RequestHeaderMap& headers,
}

bool OAuth2CookieValidator::hmacIsValid() const {
auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get();
const auto hmac_payload = absl::StrCat(host_, expires_, token_, id_token_, refresh_token_);
const auto pre_encoded_hmac = Hex::encode(crypto_util.getSha256Hmac(secret_, hmac_payload));
std::string encoded_hmac;
absl::Base64Escape(pre_encoded_hmac, &encoded_hmac);

return encoded_hmac == hmac_;
return encodeHmac(secret_, host_, expires_, token_, id_token_, refresh_token_) == hmac_;
}

bool OAuth2CookieValidator::timestampIsValid() const {
Expand Down Expand Up @@ -458,21 +466,15 @@ void OAuth2Filter::onGetAccessTokenSuccess(const std::string& access_code,
}

void OAuth2Filter::finishFlow() {
std::string token_payload;
if (config_->forwardBearerToken()) {
token_payload = absl::StrCat(host_, new_expires_, access_token_, id_token_, refresh_token_);
} else {
token_payload = absl::StrCat(host_, new_expires_);
}

auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get();

auto token_secret = config_->tokenSecret();
std::vector<uint8_t> token_secret_vec(token_secret.begin(), token_secret.end());
const std::string pre_encoded_token =
Hex::encode(crypto_util.getSha256Hmac(token_secret_vec, token_payload));
std::string encoded_token;
absl::Base64Escape(pre_encoded_token, &encoded_token);
if (config_->forwardBearerToken()) {
encoded_token =
encodeHmac(token_secret_vec, host_, new_expires_, access_token_, id_token_, refresh_token_);
} else {
encoded_token = encodeHmac(token_secret_vec, host_, new_expires_);
}

// We use HTTP Only cookies for the HMAC and Expiry.
const std::string cookie_tail = fmt::format(CookieTailFormatString, new_expires_);
Expand Down
119 changes: 115 additions & 4 deletions test/extensions/filters/http/oauth2/filter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ class OAuth2Test : public testing::Test {
absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")},
{Http::Headers::get().Cookie.get(),
absl::StrCat(cookie_names.oauth_hmac_, "="
"NGQ3MzVjZGExNGM5NTFiZGJjODBkMjBmYjAyYjNiOTFjMmNjYj"
"IxMTUzNmNiNWU0NjQzMmMxMWUzZmE2ZWJjYg=="
"NzQyYmI0YTJkMzFjMmU4Njg2MTdiZGUzYWQzZjkxZjJiMTgwZD"
"I5OWQ2YzhjODBkN2Y4Zjg2OGFmMWFiMWM0Mg=="
";version=test")},
};

Expand Down Expand Up @@ -694,6 +694,75 @@ TEST_F(OAuth2Test, CookieValidatorWithCustomNames) {
expectValidCookies(CookieNames{"CustomBearerToken", "CustomOauthHMAC", "CustomOauthExpires"});
}

// Validates the behavior of the cookie validator when the combination of some fields could be same.
TEST_F(OAuth2Test, CookieValidatorSame) {
test_time_.setSystemTime(SystemTime(std::chrono::seconds(0)));
auto cookie_names = CookieNames{"BearerToken", "OauthHMAC", "OauthExpires"};
const auto expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 5;

// Host name is `traffic.example.com:101` and the expire time is 5.
Http::TestRequestHeaderMapImpl request_headers{
{Http::Headers::get().Host.get(), "traffic.example.com:101"},
{Http::Headers::get().Path.get(), "/anypath"},
{Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get},
{Http::Headers::get().Cookie.get(),
fmt::format("{}={};version=test", cookie_names.oauth_expires_, expires_at_s)},
{Http::Headers::get().Cookie.get(),
absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")},
{Http::Headers::get().Cookie.get(),
absl::StrCat(cookie_names.oauth_hmac_, "="
"MzEyYWJjOWE0MzUwMTlkNWYxZDhiMjg2OTRiMWNjYzEyMjIzZj"
"JiMmQ5NDY3YWM3MTNhMTE2YmVmNGQ0MTcxZA=="
";version=test")},
};

auto cookie_validator = std::make_shared<OAuth2CookieValidator>(test_time_, cookie_names);
EXPECT_EQ(cookie_validator->token(), "");
cookie_validator->setParams(request_headers, "mock-secret");

EXPECT_TRUE(cookie_validator->hmacIsValid());
EXPECT_TRUE(cookie_validator->timestampIsValid());
EXPECT_TRUE(cookie_validator->isValid());

// If we advance time beyond 5s the timestamp should no longer be valid.
test_time_.advanceTimeWait(std::chrono::seconds(6));

EXPECT_FALSE(cookie_validator->timestampIsValid());
EXPECT_FALSE(cookie_validator->isValid());

test_time_.setSystemTime(SystemTime(std::chrono::seconds(0)));
const auto new_expires_at_s = DateUtil::nowToSeconds(test_time_.timeSystem()) + 15;

// Host name is `traffic.example.com:10` and the expire time is 15.
// HMAC should be different from the above one with the separator fix.
Http::TestRequestHeaderMapImpl request_headers_second{
{Http::Headers::get().Host.get(), "traffic.example.com:10"},
{Http::Headers::get().Path.get(), "/anypath"},
{Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get},
{Http::Headers::get().Cookie.get(),
fmt::format("{}={};version=test", cookie_names.oauth_expires_, new_expires_at_s)},
{Http::Headers::get().Cookie.get(),
absl::StrCat(cookie_names.bearer_token_, "=xyztoken;version=test")},
{Http::Headers::get().Cookie.get(),
absl::StrCat(cookie_names.oauth_hmac_, "="
"NzViOTc0ZTAyNGFiZTllNTg1ZTc2YzFkMzQzMDkxYjdmNTMwZT"
"gwZTMyZTM1YzFiYTY2YjU0NTkxYzgzZDg1YQ=="
";version=test")},
};

cookie_validator->setParams(request_headers_second, "mock-secret");

EXPECT_TRUE(cookie_validator->hmacIsValid());
EXPECT_TRUE(cookie_validator->timestampIsValid());
EXPECT_TRUE(cookie_validator->isValid());

// If we advance time beyond 15s the timestamp should no longer be valid.
test_time_.advanceTimeWait(std::chrono::seconds(16));

EXPECT_FALSE(cookie_validator->timestampIsValid());
EXPECT_FALSE(cookie_validator->isValid());
}

// Validates the behavior of the cookie validator when the expires_at value is not a valid integer.
TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) {
Http::TestRequestHeaderMapImpl request_headers{
Expand All @@ -704,7 +773,7 @@ TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) {
{Http::Headers::get().Cookie.get(), "BearerToken=xyztoken;version=test"},
{Http::Headers::get().Cookie.get(),
"OauthHMAC="
"M2NjZmIxYWE0NzQzOGZlZTJjMjQwMzBiZTU5OTdkN2Y0NDRhZjE5MjZiOWNhY2YzNjM0MWRmMTNkMDVmZWFlOQ=="
"NzNlZDZhY2YyYWNjOWFhMWJjZjhlZTFkOWZiNmY2ZjBlYmNkMzQzNTljNmY0ZTMyMjVmMzViNjQyMTM1Y2Q4MQ=="
";version=test"},
};

Expand Down Expand Up @@ -931,7 +1000,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) {
{Http::Headers::get().Status.get(), "302"},
{Http::Headers::get().SetCookie.get(),
"OauthHMAC="
"NWUzNzE5MWQwYTg0ZjA2NjIyMjVjMzk3MzY3MzMyZmE0NjZmMWI2MjI1NWFhNDhkYjQ4NDFlZmRiMTVmMTk0MQ==;"
"N2Q1ZWI2M2EwMmUyYTQyODUzNDEwMGI3NTA1ODAzYTdlOTc5YjAyODkyNmY3Y2VkZWU3MGE4MjYyNTYyYmQ2Yw==;"
"version=1;path=/;Max-Age=;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"OauthExpires=;version=1;path=/;Max-Age=;secure;HttpOnly"},
Expand All @@ -946,6 +1015,48 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) {
filter_->finishFlow();
}

/**
* Testing oauth response after tokens are set.
*
* Expected behavior: cookies are set.
*/
TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens) {

// Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs.
test_time_.setSystemTime(SystemTime(std::chrono::seconds(1000)));

// host_ must be set, which is guaranteed (ASAN).
Http::TestRequestHeaderMapImpl request_headers{
{Http::Headers::get().Host.get(), "traffic.example.com"},
{Http::Headers::get().Path.get(), "/_signout"},
{Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get},
};
filter_->decodeHeaders(request_headers, false);

// Expected response after the callback is complete.
Http::TestRequestHeaderMapImpl expected_headers{
{Http::Headers::get().Status.get(), "302"},
{Http::Headers::get().SetCookie.get(),
"OauthHMAC="
"ZTEzMmIyYzRmNTdmMTdiY2IyYmViZDE3ODA5ZDliOTE2MTRlNzNjYjc4MjBlMTVlOWY1OTM2ZjViZjM4MzAwNA==;"
"version=1;path=/;Max-Age=1600;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"OauthExpires=1600;version=1;path=/;Max-Age=1600;secure;HttpOnly"},
{Http::Headers::get().SetCookie.get(),
"BearerToken=access_code;version=1;path=/;Max-Age=1600;secure"},
{Http::Headers::get().SetCookie.get(),
"IdToken=some-id-token;version=1;path=/;Max-Age=1600;secure"},
{Http::Headers::get().SetCookie.get(),
"RefreshToken=some-refresh-token;version=1;path=/;Max-Age=1600;secure"},
{Http::Headers::get().Location.get(), ""},
};

EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true));

filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token",
std::chrono::seconds(600));
}

TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) {
Http::TestRequestHeaderMapImpl request_headers{
{Http::Headers::get().Path.get(), "/test?role=bearer"},
Expand Down

0 comments on commit da77eab

Please sign in to comment.