Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 47 additions & 24 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -254,46 +254,69 @@ bool maybeAdjustForIpv6(absl::string_view absolute_url, uint64_t& offset, uint64
return true;
}

absl::string_view parseCookie(absl::string_view cookie_value, absl::string_view key) {
// Split the cookie header into individual cookies.
for (const auto& s : StringUtil::splitToken(cookie_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);
// If the key matches, parse the value from the rest of the cookie string.
if (k == key) {
void forEachCookie(const HeaderMap& headers, const LowerCaseString& cookie_header,
const std::function<bool (const absl::string_view&, const absl::string_view&)> cookie_consumer) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view should be passed by value as the spec suggests. No need to add const and &.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out! I have dropped const and & as recommended in the spec

const Http::HeaderMap::GetResult cookie_headers = headers.get(cookie_header);

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);
}
return v;

bool continue_iteration = cookie_consumer(k, v);
if (!continue_iteration) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!continue_iteration) {
if (!cookie_consumer(k, v)) {

then you don't need continue_iteration.

@theshubhamp theshubhamp Aug 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, moved the invocation inside if condition itself

return;
}
}
}
return EMPTY_STRING;
}

std::string parseCookie(const HeaderMap& headers, const std::string& key,
const LowerCaseString& cookie) {
const Http::HeaderMap::GetResult cookie_headers = headers.get(cookie);
std::string value;

for (size_t index = 0; index < cookie_headers.size(); index++) {
auto cookie_header_value = cookie_headers[index]->value().getStringView();
absl::string_view result = parseCookie(cookie_header_value, key);
if (!result.empty()) {
return std::string{result};
// Iterate over each cookie & return if its value is not empty.
forEachCookie(headers, cookie, [&key, &value] (const absl::string_view& k, const absl::string_view& v) -> bool {
if (key == k && !v.empty()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this new code implements different behavior and theoretically may break certain setups. Consider the case of

Cookie: a=; b=1; a=2
Cookie: a=3

The old parseCookie(headers, "a", "cookie") would return 3. The new code returns 2 if I'm not mistaken.

I think it makes sense to add a test for this case and make sure it passes for both versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, this new version is not equivalent to the old one. I'll try to make both the variants (single-cookie and map) compatible with the old behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a related note it appears that even the old behaviours haven't been consistent over time.

For example if we use this new test as a reference:

TEST(HttpUtility, TestParseCookieDuplicates) {
  TestRequestHeaderMapImpl headers{
      {"someheader", "10.0.0.1"},
      {"cookie", "a=; b=1; a=2"},
      {"cookie", "a=3; b=2"}};

  EXPECT_EQ(Utility::parseCookieValue(headers, "a"), "3");
  EXPECT_EQ(Utility::parseCookieValue(headers, "b"), "1");
}

It fails before #17560 landed (changed header iter. from reverse to natural order)

# git status
HEAD detached at b57827c20

# bazel test //test/common/http:utility_test
...
NFO: 12 processes: 1 internal, 11 darwin-sandbox.
INFO: Build completed, 1 test FAILED, 12 total actions
//test/common/http:utility_test                                          FAILED in 2.5s

# less /private/var/tmp/_bazel_shubhamp/306aac4fa57fcfade8ca38e576399a40/execroot/envoy/bazel-out/darwin-fastbuild/testlogs/test/common/http/utility_test/test.log
...
[ RUN      ] HttpUtility.TestParseCookieDuplicates
test/common/http/utility_test.cc:559: Failure
Expected equality of these values:
  Utility::parseCookieValue(headers, "b")
    Which is: "2"
  "1"
Stack trace:
  0x10b50f3aa: Envoy::Http::HttpUtility_TestParseCookieDuplicates_Test::TestBody()
  0x1148c1fc4: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x114897a2b: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x114897963: testing::Test::Run()
  0x114898977: testing::TestInfo::Run()
... Google Test internal frames ...

But passes on main:

# less less /private/var/tmp/_bazel_shubhamp/306aac4fa57fcfade8ca38e576399a40/execroot/envoy/bazel-out/darwin-fastbuild/testlogs/test/common/http/utility_test/test.log
...
[ RUN      ] HttpUtility.TestParseCookieDuplicates
[       OK ] HttpUtility.TestParseCookieDuplicates (0 ms)

@theshubhamp theshubhamp Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behaviour on main (and the older version before it got changed) is a little inconsistent because it:

  • picks the first matching cookie from a single cookie header (i.e. resolves a's value to an empty string if the header value looks like: "a=; b=1; a=2")
  • but exhaustively searches remaining cookie headers until a non empty value is found.

And this ^ makes parsing out duplicate cookies spilt between multiple cookie headers fun.

@theshubhamp theshubhamp Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions on how to proceed here ?

  1. Keep the behaviour from main as-is
  2. Pick the first cookie value OR first non-empty cookie value
  3. Pick the last cookie value OR last non-empty cookie value

I'm inclined towards approaches 2 or 3 because it keeps the parsing simple in the long run - but this can break some setups.

RFC 6265 - HTTP State Management Mechanism itself is pretty ambiguous about duplicate cookies and ordering:

   Although cookies are serialized linearly in the Cookie header,
   servers SHOULD NOT rely upon the serialization order.  In particular,
   if the Cookie header contains two cookies with the same name (e.g.,
   that were set with different Path or Domain attributes), servers
   SHOULD NOT rely upon the order in which these cookies appear in the
   header.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... That's a good catch! If the spec explicitly states that the order is unimportant then just pick the first value irrespective of its emptiness for the sake of perf.

A second reviewer may give a second opinion on it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to take the first value for perf.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, switched to picking up the first occurrence of a cookie!

Added tests so that this behavioural quirk has coverage.

value = std::string{v};
return false;
}
}

return EMPTY_STRING;
// continue iterating until a cookie that matches `key` is found.
return true;
});

return value;
}

std::map<std::string, std::string> Utility::parseCookies(const RequestHeaderMap& headers) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use absl::flat_hash_map here.

@theshubhamp theshubhamp Aug 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to using absl::flat_hash_map

For my curiosity: Why should it be preferred over std::map ? I tried looking at the abseil docs and other sources but could not get a simple answer!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Algorithmic complexity of inserts and lookups is logarithmic for std::map. For hashes it's basically O(1).

std::map<std::string, std::string> cookies;

forEachCookie(headers, Http::Headers::get().Cookie, [&cookies] (const absl::string_view& k, const absl::string_view& v) -> bool {
cookies.emplace(std::string{k}, std::string{v});

// continue iterating until all cookies are processed.
return true;
});

return cookies;
}

bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) {
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,13 @@ std::string stripQueryString(const HeaderString& path);
**/
std::string parseCookieValue(const HeaderMap& headers, const std::string& key);

/**
* Parse cookies from header into a map.
* @param headers supplies the headers to get cookies from.
* @return std::map cookie map.
**/
std::map<std::string, std::string> parseCookies(const RequestHeaderMap& headers);

/**
* Parse a particular value out of a set-cookie
* @param headers supplies the headers to get the set-cookie from.
Expand Down
14 changes: 11 additions & 3 deletions source/extensions/filters/http/oauth2/filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,17 @@ FilterStats FilterConfig::generateStats(const std::string& prefix, Stats::Scope&

void OAuth2CookieValidator::setParams(const Http::RequestHeaderMap& headers,
const std::string& secret) {
expires_ = Http::Utility::parseCookieValue(headers, "OauthExpires");
token_ = Http::Utility::parseCookieValue(headers, "BearerToken");
hmac_ = Http::Utility::parseCookieValue(headers, "OauthHMAC");
const auto& cookies = Http::Utility::parseCookies(headers);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation parses all cookies into a map even when we just need a subset.

Should parseCookies(...) be enhanced to accept a filter to filter down on keys at the time of parsing ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if possible performance gain can balance increased code complexity. I'd rather keep it as it is for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that instantiates many small strings, I think this should pass a lambda like the following to forEachCookies:

if (k == "OauthExpires") {
  expires_ = v;
}
...

@theshubhamp theshubhamp Aug 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (k == "OauthExpires") {
  expires_ = v;
}
...

This may require consumers of forEachCookies to worry about duplicate cookie semantics discussed on this thread #17811 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this variant of parseCookies that accepts a key filter lambda ? It can support both static checks like these ^ OR something more dynamic like checking set / map membership.

absl::flat_hash_map<std::string, std::string>
Utility::parseCookies(const RequestHeaderMap& headers, const std::function<bool(absl::string_view)>& key_filter) {
  absl::flat_hash_map<std::string, std::string> cookies;

  forEachCookie(headers, Http::Headers::get().Cookie,
                [&cookies, &key_filter](absl::string_view k, absl::string_view v) -> bool {
                  if (key_filter(k)) {
                    cookies.emplace(k, v);
                  }

                  // continue iterating until all cookies are processed.
                  return true;
                });

  return cookies;
}

An overload can be added that that defaults key_filter to a lambda that always returns true to provide a parse all cookies fallback.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the latter sounds good to me if we care about uniform handling of duplicate cookies. Checking for set membership may be even faster than the static string comparisons.

@theshubhamp theshubhamp Aug 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Its being used like this in the oauth2 filter now:

...
  const auto& cookies = Http::Utility::parseCookies(headers, [](absl::string_view key) -> bool {
    return key == "OauthExpires" || key == "BearerToken" || key == "OauthHMAC";
  });
...

Kept static comparisons inside the predicate thinking that serial = comparisons should be (a little ?) faster than using a set which requires hash computation before every check (if I am not mistaken)


const auto expires_it = cookies.find("OauthExpires");
expires_ = expires_it != cookies.end() ? expires_it->second : EMPTY_STRING;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace it with a function defined in the anonymous namespace and call the function also for token_ and hmac_.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added std::string findValue(...) to unpack the hash_map as we need it here.


const auto token_it = cookies.find("BearerToken");
token_ = token_it != cookies.end() ? token_it->second : EMPTY_STRING;

const auto hmac_it = cookies.find("OauthHMAC");
hmac_ = hmac_it != cookies.end() ? hmac_it->second : EMPTY_STRING;

host_ = headers.Host()->value().getStringView();

secret_.assign(secret.begin(), secret.end());
Expand Down
15 changes: 15 additions & 0 deletions test/common/http/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,21 @@ TEST(HttpUtility, TestParseCookieWithQuotes) {
EXPECT_EQ(Utility::parseCookieValue(headers, "leadingdquote"), "\"foobar");
}

TEST(HttpUtility, TestParseCookies) {
TestRequestHeaderMapImpl headers{
{"someheader", "10.0.0.1"},
{"cookie", "dquote=\"; quoteddquote=\"\"\""},
{"cookie", "leadingdquote=\"foobar;"},
{"cookie", "abc=def; token=\"abc123\"; Expires=Wed, 09 Jun 2021 10:18:14 GMT"}};

const auto& cookies = Utility::parseCookies(headers);

EXPECT_EQ(cookies.at("token"), "abc123");
EXPECT_EQ(cookies.at("dquote"), "\"");
EXPECT_EQ(cookies.at("quoteddquote"), "\"");
EXPECT_EQ(cookies.at("leadingdquote"), "\"foobar");
}

TEST(HttpUtility, TestParseSetCookieWithQuotes) {
TestRequestHeaderMapImpl headers{
{"someheader", "10.0.0.1"},
Expand Down