-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
jwt_authn: Support extraction of JWT from Cookies in JWT Extension #17721
Changes from 5 commits
65f7e61
3d37b65
09eaa9b
c9f88ef
72ee06c
1a958af
4c78c4a
e61bab0
883eb21
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.
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 |
---|---|---|
|
@@ -296,6 +296,41 @@ std::string parseCookie(const HeaderMap& headers, const std::string& key, | |
return EMPTY_STRING; | ||
} | ||
|
||
std::map<std::string, std::string> Utility::parseCookies(const RequestHeaderMap& headers) { | ||
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 very similar to the existing code in source/common/http/utility.cc #L257-L282 Could not use it as-is because that'd scan over cookies multiple times. Any suggestions on merging them together ? 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. Perhaps you could introduce something like 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. Thanks for the suggestion! Was able to get this working locally 👍 I would prefer to do this change in a follow-up PR. I hope that's okay. 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. Yes, a follow-up would be Ok. Just put a TODO note to the function. 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. The other places using I would suggest you to land the refactoring before this PR. 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. Cool, I'll do that first |
||
// TODO(theshubhamp): Replace this duplicated logic from parseCookie(...) with a cookie iterator. | ||
|
||
std::map<std::string, std::string> cookies; | ||
const Http::HeaderMap::GetResult cookie_headers = headers.get(Http::Headers::get().Cookie); | ||
|
||
for (size_t index = 0; index < cookie_headers.size(); index++) { | ||
auto cookie_header_value = cookie_headers[index]->value().getStringView(); | ||
|
||
// Split the cookie header into individual cookies. | ||
for (const auto& s : StringUtil::splitToken(cookie_header_value, ";")) { | ||
// Find the key part of the cookie (i.e. the name of the cookie). | ||
size_t first_non_space = s.find_first_not_of(' '); | ||
size_t equals_index = s.find('='); | ||
if (equals_index == absl::string_view::npos) { | ||
// The cookie is malformed if it does not have an `=`. Continue | ||
// checking other cookies in this header. | ||
continue; | ||
} | ||
absl::string_view k = s.substr(first_non_space, equals_index - first_non_space); | ||
absl::string_view v = s.substr(equals_index + 1, s.size() - 1); | ||
|
||
// Cookie values may be wrapped in double quotes. | ||
// https://tools.ietf.org/html/rfc6265#section-4.1.1 | ||
if (v.size() >= 2 && v.back() == '"' && v[0] == '"') { | ||
v = v.substr(1, v.size() - 2); | ||
} | ||
|
||
cookies.emplace(std::string{k}, std::string{v}); | ||
} | ||
} | ||
|
||
return cookies; | ||
} | ||
|
||
bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { | ||
struct http_parser_url u; | ||
http_parser_url_init(&u); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,17 @@ class JwtParamLocation : public JwtLocationBase { | |
} | ||
}; | ||
|
||
// The JwtLocation for cookie extraction. | ||
class JwtCookieLocation : public JwtLocationBase { | ||
public: | ||
JwtCookieLocation(const std::string& token, const JwtIssuerChecker& issuer_checker) | ||
: JwtLocationBase(token, issuer_checker) {} | ||
|
||
void removeJwt(Http::HeaderMap&) const override { | ||
// TODO(theshubhamp): remove JWT from cookies. | ||
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. Use 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. Added below the comment |
||
} | ||
}; | ||
|
||
/** | ||
* The class implements Extractor interface | ||
* | ||
|
@@ -133,6 +144,8 @@ class ExtractorImpl : public Logger::Loggable<Logger::Id::jwt>, public Extractor | |
const std::string& value_prefix); | ||
// add a query param config | ||
void addQueryParamConfig(const std::string& issuer, const std::string& param); | ||
// add a query param config | ||
void addCookieConfig(const std::string& issuer, const std::string& cookie); | ||
// ctor helper for a jwt provider config | ||
void addProvider(const JwtProvider& provider); | ||
|
||
|
@@ -164,6 +177,14 @@ class ExtractorImpl : public Logger::Loggable<Logger::Id::jwt>, public Extractor | |
// The map of a parameter key to set of issuers specified the parameter | ||
std::map<std::string, ParamLocationSpec> param_locations_; | ||
|
||
// CookieMap value type to store issuers that specified this cookie. | ||
struct CookieLocationSpec { | ||
// Issuers that specified this param. | ||
JwtIssuerChecker issuer_checker_; | ||
}; | ||
// The map of a cookie key to set of issuers specified the cookie. | ||
std::map<std::string, CookieLocationSpec> cookie_locations_; | ||
|
||
std::vector<LowerCaseString> forward_payload_headers_; | ||
}; | ||
|
||
|
@@ -183,6 +204,9 @@ void ExtractorImpl::addProvider(const JwtProvider& provider) { | |
for (const std::string& param : provider.from_params()) { | ||
addQueryParamConfig(provider.issuer(), param); | ||
} | ||
for (const std::string& cookie : provider.from_cookies()) { | ||
addCookieConfig(provider.issuer(), cookie); | ||
} | ||
// If not specified, use default locations. | ||
if (provider.from_headers().empty() && provider.from_params().empty()) { | ||
addHeaderConfig(provider.issuer(), Http::CustomHeaders::get().Authorization, | ||
|
@@ -210,6 +234,11 @@ void ExtractorImpl::addQueryParamConfig(const std::string& issuer, const std::st | |
param_location_spec.issuer_checker_.add(issuer); | ||
} | ||
|
||
void ExtractorImpl::addCookieConfig(const std::string& issuer, const std::string& cookie) { | ||
auto& cookie_location_spec = cookie_locations_[cookie]; | ||
cookie_location_spec.issuer_checker_.add(issuer); | ||
} | ||
|
||
std::vector<JwtLocationConstPtr> | ||
ExtractorImpl::extract(const Http::RequestHeaderMap& headers) const { | ||
std::vector<JwtLocationConstPtr> tokens; | ||
|
@@ -235,22 +264,36 @@ ExtractorImpl::extract(const Http::RequestHeaderMap& headers) const { | |
} | ||
} | ||
|
||
// If no query parameter locations specified, or Path() is null, bail out | ||
if (param_locations_.empty() || headers.Path() == nullptr) { | ||
return tokens; | ||
// Check query parameter locations only if query parameter locations specified and Path() is not | ||
// null | ||
if (!param_locations_.empty() && headers.Path() != nullptr) { | ||
const auto& params = Http::Utility::parseAndDecodeQueryString(headers.getPathValue()); | ||
for (const auto& location_it : param_locations_) { | ||
const auto& param_key = location_it.first; | ||
const auto& location_spec = location_it.second; | ||
const auto& it = params.find(param_key); | ||
if (it != params.end()) { | ||
tokens.push_back(std::make_unique<const JwtParamLocation>( | ||
it->second, location_spec.issuer_checker_, param_key)); | ||
} | ||
} | ||
} | ||
|
||
// Check query parameter locations. | ||
const auto& params = Http::Utility::parseAndDecodeQueryString(headers.getPathValue()); | ||
for (const auto& location_it : param_locations_) { | ||
const auto& param_key = location_it.first; | ||
const auto& location_spec = location_it.second; | ||
const auto& it = params.find(param_key); | ||
if (it != params.end()) { | ||
tokens.push_back(std::make_unique<const JwtParamLocation>( | ||
it->second, location_spec.issuer_checker_, param_key)); | ||
// Check cookie locations. | ||
if (!cookie_locations_.empty()) { | ||
const auto& cookies = Http::Utility::parseCookies(headers); | ||
|
||
for (const auto& location_it : cookie_locations_) { | ||
const auto& cookie_key = location_it.first; | ||
const auto& location_spec = location_it.second; | ||
const auto& it = cookies.find(cookie_key); | ||
if (it != cookies.end()) { | ||
tokens.push_back( | ||
std::make_unique<const JwtCookieLocation>(it->second, location_spec.issuer_checker_)); | ||
} | ||
} | ||
} | ||
|
||
return tokens; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this block after
from_params
.This doesn't have to be ordered by field tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for pointing this out