Skip to content

Commit

Permalink
Fix: oauth2 state encoding (#37473)
Browse files Browse the repository at this point in the history 
Commit Message: 
* Changes the state pramater to a json object to store the user state
before redirecting the user request to the auth server. Currently, the
json object has two fields: the original url and a nonce for csrf
prevention.
* Changes the state econding to
[Base64URL](https://datatracker.ietf.org/doc/html/rfc4648#section-5) to
fix [ AWS cognito doesn't support url encoded state
value](https://docs.aws.amazon.com/cognito/latest/developerguide/authorization-endpoint.html#get-authorize-request-parameters)
* Change the nonce from a plain timestamp to a random number to enhance
its security robustness. This addresses the "1. Generation" in this
issue #36276
* Small refacor: rename variables related to token_secret to hmac_secret
to improve clarity and consistency.

Example:
original state:
`{"url":"https://localhost:8080/login","nonce":"IPOom6PfIoFS+MmiV04aTJai8vUYlzyO5zUgT2G8mZA="}`
base64url encoded state:
`eyJ1cmwiOiJodHRwczovL2xvY2FsaG9zdDo4MDgwL2xvZ2luIiwibm9uY2UiOiJJUE9vbTZQZklvRlMrTW1pVjA0YVRKYWk4dlVZbHp5TzV6VWdUMkc4bVpBPSJ9`

Additional Description: The nonce in the [state parameter
](https://datatracker.ietf.org/doc/html/rfc6749#section-10.12)is used
for csrf prevention and is applicable for both oauth2 and oidc. Please
note that the OIDC spec defines a seperate [nonce
parameter](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest)
,which is specifically designed to prevent replay attacks and is unique
to OIDC.

More discussion about state and nonce parameters and be found in this
comment:
#37050 (comment)

Risk Level:
Testing: Unit test and integration test
Docs Changes:
Release Notes: Yes
Platform Specific Features:
[Optional Runtime guard:] A runtime gurad
"envoy.reloadable_features.oauth2_enable_state_nonce" has been added for
the new nonce in the state parameter.
[Optional Fixes #37049 #36871]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Related Envoy Gateway issue:
envoyproxy/gateway#4625

I've tested this PR against AWS cognito using Envoy Gateway
SecurityPolicy, and it worked.

cc @missBerg @arkodg

---------

Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: code <[email protected]>
Co-authored-by: code <[email protected]>
Co-authored-by: phlax <[email protected]>
  • Loading branch information
@zhaohuabing @wbpcode @phlax
3 people authored Dec 11, 2024
1 parent 56e6cb6 commit 0c6a1d2
Show file tree
Hide file tree
Showing 7 changed files with 253 additions and 348 deletions.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,10 @@ minor_behavior_changes:
:ref:`use_refresh_token <envoy_v3_api_field_extensions.filters.http.oauth2.v3.OAuth2Config.use_refresh_token>`
is now enabled by default. This behavioral change can be temporarily reverted by setting runtime guard
``envoy.reloadable_features.oauth2_use_refresh_token`` to false.
- area: oauth2
change: |
The ``state`` parameter in the OAuth2 authorization request has been changed to a base64url-encoded JSON object.
The JSON object contains the original request URL and a nonce for CSRF prevention.
- area: quic
change: |
Enable UDP GRO in QUIC client connections by default. This behavior can be reverted by setting
Expand Down
1 change: 1 addition & 0 deletions source/extensions/filters/http/oauth2/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ envoy_cc_library(
":oauth_client",
"//envoy/server:filter_config_interface",
"//source/common/common:assert_lib",
"//source/common/common:base64_lib",
"//source/common/common:empty_string",
"//source/common/common:matchers_lib",
"//source/common/crypto:utility_lib",
Expand Down
13 changes: 7 additions & 6 deletions source/extensions/filters/http/oauth2/config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,15 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
const auto& proto_config = proto.config();
const auto& credentials = proto_config.credentials();

const auto& token_secret = credentials.token_secret();
const auto& client_secret = credentials.token_secret();
const auto& hmac_secret = credentials.hmac_secret();

auto& cluster_manager = context.serverFactoryContext().clusterManager();
auto& secret_manager = cluster_manager.clusterManagerFactory().secretManager();
auto& transport_socket_factory = context.getTransportSocketFactoryContext();
auto secret_provider_token_secret = secretsProvider(
token_secret, secret_manager, transport_socket_factory, context.initManager());
if (secret_provider_token_secret == nullptr) {
auto secret_provider_client_secret = secretsProvider(
client_secret, secret_manager, transport_socket_factory, context.initManager());
if (secret_provider_client_secret == nullptr) {
throw EnvoyException("invalid token secret configuration");
}
auto secret_provider_hmac_secret =
Expand All @@ -72,7 +72,7 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
}

auto secret_reader = std::make_shared<SDSSecretReader>(
std::move(secret_provider_token_secret), std::move(secret_provider_hmac_secret),
std::move(secret_provider_client_secret), std::move(secret_provider_hmac_secret),
context.serverFactoryContext().threadLocal(), context.serverFactoryContext().api());
auto config = std::make_shared<FilterConfig>(proto_config, context.serverFactoryContext(),
secret_reader, context.scope(), stats_prefix);
Expand All @@ -83,7 +83,8 @@ Http::FilterFactoryCb OAuth2Config::createFilterFactoryFromProtoTyped(
std::make_unique<OAuth2ClientImpl>(cluster_manager, config->oauthTokenEndpoint(),
config->retryPolicy(), config->defaultExpiresIn());
callbacks.addStreamFilter(std::make_shared<OAuth2Filter>(
config, std::move(oauth_client), context.serverFactoryContext().timeSource()));
config, std::move(oauth_client), context.serverFactoryContext().timeSource(),
context.serverFactoryContext().api().randomGenerator()));
};
}

Expand Down
128 changes: 72 additions & 56 deletions source/extensions/filters/http/oauth2/filter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <vector>

#include "source/common/common/assert.h"
#include "source/common/common/base64.h"
#include "source/common/common/empty_string.h"
#include "source/common/common/enum_to_int.h"
#include "source/common/common/fmt.h"
Expand Down Expand Up @@ -152,19 +153,24 @@ std::string encodeHmacHexBase64(const std::vector<uint8_t>& secret, absl::string
return encoded_hmac;
}

// Generates a SHA256 HMAC from a secret and a message and returns the result as a base64 encoded
// string.
std::string generateHmacBase64(const std::vector<uint8_t>& secret, std::string& message) {
auto& crypto_util = Envoy::Common::Crypto::UtilitySingleton::get();
std::vector<uint8_t> hmac_result = crypto_util.getSha256Hmac(secret, message);
std::string hmac_string(hmac_result.begin(), hmac_result.end());
std::string base64_encoded_hmac;
absl::Base64Escape(hmac_string, &base64_encoded_hmac);
return base64_encoded_hmac;
}

std::string encodeHmacBase64(const std::vector<uint8_t>& secret, absl::string_view domain,
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 =
std::string hmac_payload =
absl::StrJoin({domain, expires, token, id_token, refresh_token}, HmacPayloadSeparator);

std::string base64_encoded_hmac;
std::vector<uint8_t> hmac_result = crypto_util.getSha256Hmac(secret, hmac_payload);
std::string hmac_string(hmac_result.begin(), hmac_result.end());
absl::Base64Escape(hmac_string, &base64_encoded_hmac);
return base64_encoded_hmac;
return generateHmacBase64(secret, hmac_payload);
}

std::string encodeHmac(const std::vector<uint8_t>& secret, absl::string_view domain,
Expand All @@ -173,19 +179,21 @@ std::string encodeHmac(const std::vector<uint8_t>& secret, absl::string_view dom
return encodeHmacBase64(secret, domain, expires, token, id_token, refresh_token);
}

// Generates a nonce based on the current time
std::string generateFixedLengthNonce(TimeSource& time_source) {
constexpr size_t length = 16;

std::string nonce = fmt::format("{}", time_source.systemTime().time_since_epoch().count());

if (nonce.length() < length) {
nonce.append(length - nonce.length(), '0');
} else if (nonce.length() > length) {
nonce = nonce.substr(0, length);
}
// Generates a non-guessable nonce that can be used to prevent CSRF attacks.
std::string generateNonce(Random::RandomGenerator& random) {
return Hex::uint64ToHex(random.random());
}

return nonce;
/**
* Encodes the state parameter for the OAuth2 flow.
* The state parameter is a base64Url encoded JSON object containing the original request URL and a
* nonce for CSRF protection.
*/
std::string encodeState(const std::string& original_request_url, const std::string& nonce) {
std::string buffer;
absl::string_view sanitized_url = Json::sanitize(buffer, original_request_url);
std::string json = fmt::format(R"({{"url":"{}","nonce":"{}"}})", sanitized_url, nonce);
return Base64Url::encode(json.data(), json.size());
}

} // namespace
Expand Down Expand Up @@ -293,11 +301,12 @@ bool OAuth2CookieValidator::timestampIsValid() const {
bool OAuth2CookieValidator::isValid() const { return hmacIsValid() && timestampIsValid(); }

OAuth2Filter::OAuth2Filter(FilterConfigSharedPtr config,
std::unique_ptr<OAuth2Client>&& oauth_client, TimeSource& time_source)
std::unique_ptr<OAuth2Client>&& oauth_client, TimeSource& time_source,
Random::RandomGenerator& random)
: validator_(std::make_shared<OAuth2CookieValidator>(time_source, config->cookieNames(),
config->cookieDomain())),
oauth_client_(std::move(oauth_client)), config_(std::move(config)),
time_source_(time_source) {
oauth_client_(std::move(oauth_client)), config_(std::move(config)), time_source_(time_source),
random_(random) {

oauth_client_->setCallbacks(*this);
}
Expand Down Expand Up @@ -370,7 +379,6 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he
if (config_->redirectPathMatcher().match(original_request_url.pathAndQueryParams())) {
ENVOY_LOG(debug, "state url query params {} matches the redirect path matcher",
original_request_url.pathAndQueryParams());
// TODO(zhaohuabing): Should return 401 unauthorized or 400 bad request?
sendUnauthorizedResponse();
return Http::FilterHeadersStatus::StopIteration;
}
Expand Down Expand Up @@ -456,7 +464,7 @@ Http::FilterHeadersStatus OAuth2Filter::encodeHeaders(Http::ResponseHeaderMap& h
bool OAuth2Filter::canSkipOAuth(Http::RequestHeaderMap& headers) const {
// We can skip OAuth if the supplied HMAC cookie is valid. Apply the OAuth details as headers
// if we successfully validate the cookie.
validator_->setParams(headers, config_->tokenSecret());
validator_->setParams(headers, config_->hmacSecret());
if (validator_->isValid()) {
config_->stats().oauth_success_.inc();
if (config_->forwardBearerToken() && !validator_->token().empty()) {
Expand All @@ -482,7 +490,6 @@ bool OAuth2Filter::canRedirectToOAuthServer(Http::RequestHeaderMap& headers) con
void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) const {
Http::ResponseHeaderMapPtr response_headers{Http::createHeaderMap<Http::ResponseHeaderMapImpl>(
{{Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::Found))}})};

// Construct the correct scheme. We default to https since this is a requirement for OAuth to
// succeed. However, if a downstream client explicitly declares the "http" scheme for whatever
// reason, we also use "http" when constructing our redirect uri to the authorization server.
Expand All @@ -491,12 +498,11 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) const
if (Http::Utility::schemeIsHttp(headers.getSchemeValue())) {
scheme = Http::Headers::get().SchemeValues.Http;
}

const std::string base_path = absl::StrCat(scheme, "://", host_);
const std::string full_url = absl::StrCat(base_path, headers.Path()->value().getStringView());
const std::string escaped_url = Http::Utility::PercentEncoding::urlEncodeQueryParameter(full_url);
const std::string original_url = absl::StrCat(base_path, headers.Path()->value().getStringView());

// Generate a nonce to prevent CSRF attacks
// Encode the original request URL and the nonce to the state parameter.
// Generate a nonce to prevent CSRF attacks.
std::string nonce;
bool nonce_cookie_exists = false;
const auto nonce_cookie = Http::Utility::parseCookies(headers, [this](absl::string_view key) {
Expand All @@ -506,7 +512,7 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) const
nonce = nonce_cookie.at(config_->cookieNames().oauth_nonce_);
nonce_cookie_exists = true;
} else {
nonce = generateFixedLengthNonce(time_source_);
nonce = generateNonce(random_);
}

// Set the nonce cookie if it does not exist.
Expand All @@ -523,29 +529,25 @@ void OAuth2Filter::redirectToOAuthServer(Http::RequestHeaderMap& headers) const
Http::Headers::get().SetCookie,
absl::StrCat(config_->cookieNames().oauth_nonce_, "=", nonce, cookie_tail_http_only));
}
const std::string state = encodeState(original_url, nonce);

// Encode the original request URL and the nonce to the state parameter
const std::string state =
absl::StrCat(stateParamsUrl, "=", escaped_url, "&", stateParamsNonce, "=", nonce);
const std::string escaped_state = Http::Utility::PercentEncoding::urlEncodeQueryParameter(state);
auto query_params = config_->authorizationQueryParams();
query_params.overwrite(queryParamsState, state);

Formatter::FormatterPtr formatter = THROW_OR_RETURN_VALUE(
Formatter::FormatterImpl::create(config_->redirectUri()), Formatter::FormatterPtr);
const auto redirect_uri =
formatter->formatWithContext({&headers}, decoder_callbacks_->streamInfo());
const std::string escaped_redirect_uri =
Http::Utility::PercentEncoding::urlEncodeQueryParameter(redirect_uri);

auto query_params = config_->authorizationQueryParams();
query_params.overwrite(queryParamsRedirectUri, escaped_redirect_uri);
query_params.overwrite(queryParamsState, escaped_state);

// Copy the authorization endpoint URL to replace its query params.
auto authorization_endpoint_url = config_->authorizationEndpointUrl();
const std::string path_and_query_params = query_params.replaceQueryString(
Http::HeaderString(authorization_endpoint_url.pathAndQueryParams()));
authorization_endpoint_url.setPathAndQueryParams(path_and_query_params);
const std::string new_url = authorization_endpoint_url.toString();

response_headers->setLocation(new_url + config_->encodedResourceQueryParams());

decoder_callbacks_->encodeHeaders(std::move(response_headers), true, REDIRECT_FOR_CREDENTIALS);
Expand Down Expand Up @@ -583,6 +585,10 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap
Http::Headers::get().SetCookie,
absl::StrCat(fmt::format(CookieDeleteFormatString, config_->cookieNames().refresh_token_),
cookie_domain));
response_headers->addReferenceKey(
Http::Headers::get().SetCookie,
absl::StrCat(fmt::format(CookieDeleteFormatString, config_->cookieNames().oauth_nonce_),
cookie_domain));
response_headers->setLocation(new_path);
decoder_callbacks_->encodeHeaders(std::move(response_headers), true, SIGN_OUT);

Expand Down Expand Up @@ -621,7 +627,7 @@ void OAuth2Filter::updateTokens(const std::string& access_token, const std::stri
}

std::string OAuth2Filter::getEncodedToken() const {
auto token_secret = config_->tokenSecret();
auto token_secret = config_->hmacSecret();
std::vector<uint8_t> token_secret_vec(token_secret.begin(), token_secret.end());
std::string encoded_token;

Expand Down Expand Up @@ -848,22 +854,21 @@ CallbackValidationResult OAuth2Filter::validateOAuthCallback(const Http::Request
}

// Return 401 unauthorized if the state query parameter does not contain the original request URL
// and nonce. state is an HTTP URL encoded string that contains the url and nonce, for example:
// state=url%3Dhttp%253A%252F%252Ftraffic.example.com%252Fnot%252F_oauth%26nonce%3D1234567890000000".
std::string state = Http::Utility::PercentEncoding::urlDecodeQueryParameter(stateVal.value());
const auto state_parameters = Http::Utility::QueryParamsMulti::parseParameters(state, 0, true);
auto urlVal = state_parameters.getFirstValue(stateParamsUrl);
auto nonceVal = state_parameters.getFirstValue(stateParamsNonce);
if (!urlVal.has_value() || !nonceVal.has_value()) {
ENVOY_LOG(error, "state query param does not contain url or nonce: \n{}", state);
// or nonce.
// Decode the state parameter to get the original request URL and nonce.
const std::string state = Base64Url::decode(stateVal.value());
bool has_unknown_field;
ProtobufWkt::Struct message;

auto status = MessageUtil::loadFromJsonNoThrow(state, message, has_unknown_field);
if (!status.ok()) {
ENVOY_LOG(error, "state query param is not a valid JSON: \n{}", state);
return {false, "", ""};
}

// Return 401 unauthorized if the URL in the state is not valid.
std::string original_request_url = urlVal.value();
Http::Utility::Url url;
if (!url.initialize(original_request_url, false)) {
ENVOY_LOG(error, "state url {} can not be initialized", original_request_url);
const auto& filed_value_pair = message.fields();
if (!filed_value_pair.contains(stateParamsUrl) || !filed_value_pair.contains(stateParamsNonce)) {
ENVOY_LOG(error, "state query param does not contain url or nonce: \n{}", state);
return {false, "", ""};
}

Expand All @@ -873,15 +878,26 @@ CallbackValidationResult OAuth2Filter::validateOAuthCallback(const Http::Request
// sessions via CSRF attack. The attack can result in victims saving their sensitive data
// in the attacker's account.
// More information can be found at https://datatracker.ietf.org/doc/html/rfc6819#section-5.3.5
if (!validateNonce(headers, nonceVal.value())) {
ENVOY_LOG(error, "nonce cookie does not match nonce query param: \n{}", nonceVal.value());
std::string nonce = filed_value_pair.at(stateParamsNonce).string_value();
if (!validateNonce(headers, nonce)) {
ENVOY_LOG(error, "nonce cookie does not match nonce in the state: \n{}", nonce);
return {false, "", ""};
}
const std::string original_request_url = filed_value_pair.at(stateParamsUrl).string_value();

// Return 401 unauthorized if the URL in the state is not valid.
Http::Utility::Url url;
if (!url.initialize(original_request_url, false)) {
ENVOY_LOG(error, "state url {} can not be initialized", original_request_url);
return {false, "", ""};
}

return {true, codeVal.value(), original_request_url};
}

bool OAuth2Filter::validateNonce(const Http::RequestHeaderMap& headers, const std::string& nonce) {
// Validates the nonce in the state parameter against the nonce in the cookie.
bool OAuth2Filter::validateNonce(const Http::RequestHeaderMap& headers,
const std::string& nonce) const {
const auto nonce_cookie = Http::Utility::parseCookies(headers, [this](absl::string_view key) {
return key == config_->cookieNames().oauth_nonce_;
});
Expand Down
Loading

0 comments on commit 0c6a1d2

Please sign in to comment.