Skip to content
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

add support for additional cookie attributes #27529

Merged
merged 21 commits into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
15 changes: 15 additions & 0 deletions api/envoy/config/route/v3/route_components.proto
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,18 @@ message RouteAction {
type.matcher.v3.RegexMatchAndSubstitute regex_rewrite = 2;
}

// CookieAttribute defines an API for adding additional attributes for a HTTP cookie.
message CookieAttribute {
// The name of the cookie attribute.
string name = 1
[(validate.rules).string =
{min_len: 1 max_bytes: 16384 well_known_regex: HTTP_HEADER_NAME strict: false}];
Comment on lines +839 to +841
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the HTTP header name format is appropriate for the cookie attributes. If there are only some enumatable values, maybe in could be used here rather than well_known_regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of it is make this generic rather than adding the specific list. But If you prefer list, can you please propose what that list would be a good default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wbpcode @mattklein123 any suggestions to take this forward?

Copy link
Member

Choose a reason for hiding this comment

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

Can you figure out where in the RFC it specifies valid characters in cookie names? cc @RyanTheOptimist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cookie names follow token I guess same as Http Header names as per https://www.rfc-editor.org/rfc/rfc6265#section-4.1 and https://www.rfc-editor.org/rfc/rfc2616#section-2.2

Copy link
Member

Choose a reason for hiding this comment

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

OK sounds good then seems like this is fine for now. Thanks for looking.


// The optional value of the cookie attribute.
string value = 2 [(validate.rules).string =
{max_bytes: 16384 well_known_regex: HTTP_HEADER_VALUE strict: false}];
}

// Envoy supports two types of cookie affinity:
//
// 1. Passive. Envoy takes a cookie that's present in the cookies header and
Expand Down Expand Up @@ -864,6 +876,9 @@ message RouteAction {
// The name of the path for the cookie. If no path is specified here, no path
// will be set for the cookie.
string path = 3;

// Additional attributes for the cookie. They will be used when generating a new cookie.
repeated CookieAttribute attributes = 4;
}

message ConnectionProperties {
Expand Down
5 changes: 4 additions & 1 deletion changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ new_features:
- area: ext_proc
change: |
added new field ``filter_metadata <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExtProc.filter_metadata`` to aid in logging.
Metadata will be stored in StreamInfo filter state under a namespace corresponding to the name of the ext proc filter.
Metadata will be stored in StreamInfo filter metadata under a namespace corresponding to the name of the ext proc filter.
- area: matching
change: |
added CEL(Common Expression Language) matcher support :ref:`CEL data input <extension_envoy.matching.inputs.cel_data_input>`
Expand All @@ -309,6 +309,9 @@ new_features:
This works with dynamic secrets when
:ref:`CertificateValidationContext <envoy_v3_api_msg_extensions.transport_sockets.tls.v3.CertificateValidationContext>`
is delivered via SDS.
- area: http
change: |
added support for configuring additional :ref:`cookie attributes <envoy_v3_api_msg_config.route.v3.RouteAction.HashPolicy.cookie>`.

deprecated:
- area: access_log
Expand Down
20 changes: 19 additions & 1 deletion envoy/http/hash_policy.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,23 @@
namespace Envoy {
namespace Http {

/**
* CookieAttribute that stores the name and value of a cookie.
*/
class CookieAttribute {
public:
CookieAttribute(const std::string& name, const std::string& value) : name_(name), value_(value) {}

std::string name() const { return name_; }
std::string value() const { return value_; }

private:
std::string name_;
std::string value_;
};

using CookieAttributeRefVector = std::vector<std::reference_wrapper<const CookieAttribute>>;

/**
* Request hash policy. I.e., if using a hashing load balancer, how a request should be hashed onto
* an upstream host.
Expand All @@ -25,7 +42,8 @@ class HashPolicy {
* @return std::string the opaque value of the cookie that will be set
*/
using AddCookieCallback = std::function<std::string(
const std::string& key, const std::string& path, std::chrono::seconds ttl)>;
const std::string& key, const std::string& path, std::chrono::seconds ttl,
const CookieAttributeRefVector attributes)>;

/**
* @param downstream_address is the address of the connected client host, or nullptr if the
Expand Down
18 changes: 14 additions & 4 deletions source/common/http/hash_policy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ class HeaderHashMethod : public HashMethodImplBase {
class CookieHashMethod : public HashMethodImplBase {
public:
CookieHashMethod(const std::string& key, const std::string& path,
const absl::optional<std::chrono::seconds>& ttl, bool terminal)
: HashMethodImplBase(terminal), key_(key), path_(path), ttl_(ttl) {}
const absl::optional<std::chrono::seconds>& ttl, bool terminal,
const CookieAttributeRefVector attributes)
: HashMethodImplBase(terminal), key_(key), path_(path), ttl_(ttl), attributes_(attributes) {}

absl::optional<uint64_t> evaluate(const Network::Address::Instance*,
const RequestHeaderMap& headers,
Expand All @@ -90,7 +91,7 @@ class CookieHashMethod : public HashMethodImplBase {
absl::optional<uint64_t> hash;
std::string value = Utility::parseCookieValue(headers, key_);
if (value.empty() && ttl_.has_value()) {
value = add_cookie(key_, path_, ttl_.value());
value = add_cookie(key_, path_, ttl_.value(), attributes_);
hash = HashUtil::xxHash64(value);

} else if (!value.empty()) {
Expand All @@ -103,6 +104,7 @@ class CookieHashMethod : public HashMethodImplBase {
const std::string key_;
const std::string path_;
const absl::optional<std::chrono::seconds> ttl_;
const CookieAttributeRefVector attributes_;
};

class IpHashMethod : public HashMethodImplBase {
Expand Down Expand Up @@ -188,9 +190,17 @@ HashPolicyImpl::HashPolicyImpl(
if (hash_policy->cookie().has_ttl()) {
ttl = std::chrono::seconds(hash_policy->cookie().ttl().seconds());
}
std::vector<CookieAttribute> attributes;
for (const auto& attribute : hash_policy->cookie().attributes()) {
attributes.push_back({attribute.name(), attribute.value()});
}
CookieAttributeRefVector ref_attributes;
for (const auto& attribute : attributes) {
ref_attributes.push_back(attribute);
}
hash_impls_.emplace_back(new CookieHashMethod(hash_policy->cookie().name(),
hash_policy->cookie().path(), ttl,
hash_policy->terminal()));
hash_policy->terminal(), ref_attributes));
break;
}
case envoy::config::route::v3::RouteAction::HashPolicy::PolicySpecifierCase::
Expand Down
12 changes: 11 additions & 1 deletion source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,8 @@ std::string Utility::parseSetCookieValue(const Http::HeaderMap& headers, const s

std::string Utility::makeSetCookieValue(const std::string& key, const std::string& value,
const std::string& path, const std::chrono::seconds max_age,
bool httponly) {
bool httponly,
const Http::CookieAttributeRefVector attributes) {
std::string cookie_value;
// Best effort attempt to avoid numerous string copies.
cookie_value.reserve(value.size() + path.size() + 30);
Expand All @@ -600,6 +601,15 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
if (!path.empty()) {
absl::StrAppend(&cookie_value, "; Path=", path);
}

for (auto const& attribute : attributes) {
if (attribute.get().value().empty()) {
absl::StrAppend(&cookie_value, "; ", attribute.get().name());
} else {
absl::StrAppend(&cookie_value, "; ", attribute.get().name(), "=", attribute.get().value());
}
}

if (httponly) {
absl::StrAppend(&cookie_value, "; HttpOnly");
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ std::string parseSetCookieValue(const HeaderMap& headers, const std::string& key
*/
std::string makeSetCookieValue(const std::string& key, const std::string& value,
const std::string& path, const std::chrono::seconds max_age,
bool httponly);
bool httponly, const Http::CookieAttributeRefVector attributes);

/**
* Get the response status from the response headers.
Expand Down
11 changes: 7 additions & 4 deletions source/common/router/router.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "envoy/http/codes.h"
#include "envoy/http/filter.h"
#include "envoy/http/filter_factory.h"
#include "envoy/http/hash_policy.h"
#include "envoy/http/stateful_session.h"
#include "envoy/local_info/local_info.h"
#include "envoy/router/shadow_writer.h"
Expand Down Expand Up @@ -408,8 +409,9 @@ class Filter : Logger::Loggable<Logger::Id::router>,
return hash_policy->generateHash(
callbacks_->streamInfo().downstreamAddressProvider().remoteAddress().get(),
*downstream_headers_,
[this](const std::string& key, const std::string& path, std::chrono::seconds max_age) {
return addDownstreamSetCookie(key, path, max_age);
[this](const std::string& key, const std::string& path, std::chrono::seconds max_age,
Http::CookieAttributeRefVector attributes) {
return addDownstreamSetCookie(key, path, max_age, attributes);
},
callbacks_->streamInfo().filterState());
}
Expand Down Expand Up @@ -500,7 +502,8 @@ class Filter : Logger::Loggable<Logger::Id::router>,
* @return std::string the value of the new cookie
*/
std::string addDownstreamSetCookie(const std::string& key, const std::string& path,
std::chrono::seconds max_age) {
std::chrono::seconds max_age,
Http::CookieAttributeRefVector attributes) {
// The cookie value should be the same per connection so that if multiple
// streams race on the same path, they all receive the same cookie.
// Since the downstream port is part of the hashed value, multiple HTTP1
Expand All @@ -513,7 +516,7 @@ class Filter : Logger::Loggable<Logger::Id::router>,

const std::string cookie_value = Hex::uint64ToHex(HashUtil::xxHash64(value));
downstream_set_cookies_.emplace_back(
Http::Utility::makeSetCookieValue(key, cookie_value, path, max_age, true));
Http::Utility::makeSetCookieValue(key, cookie_value, path, max_age, true, attributes));
return cookie_value;
}

Expand Down
1 change: 1 addition & 0 deletions source/extensions/http/stateful_session/cookie/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ envoy_cc_library(
hdrs = ["cookie.h"],
deps = [
":cookie_encoding_cc_proto",
"//envoy/http:hash_policy_interface",
"//envoy/http:stateful_session_interface",
"//source/common/common:base64_lib",
"//source/common/http:headers_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/extensions/http/stateful_session/cookie/cookie.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <memory>

#include "envoy/extensions/http/stateful_session/cookie/v3/cookie.pb.h"
#include "envoy/http/hash_policy.h"
#include "envoy/http/stateful_session.h"

#include "source/common/common/base64.h"
Expand Down Expand Up @@ -90,12 +91,13 @@ class CookieBasedSessionStateFactory : public Envoy::Http::SessionStateFactory {
}

std::string makeSetCookie(const std::string& address) const {
return Envoy::Http::Utility::makeSetCookieValue(name_, address, path_, ttl_, true);
return Envoy::Http::Utility::makeSetCookieValue(name_, address, path_, ttl_, true, attributes_);
}

const std::string name_;
const std::chrono::seconds ttl_;
const std::string path_;
const Envoy::Http::CookieAttributeRefVector attributes_;
TimeSource& time_source_;

std::function<bool(absl::string_view)> path_matcher_;
Expand Down
3 changes: 2 additions & 1 deletion test/common/http/utility_fuzz_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ DEFINE_PROTO_FUZZER(const test::common::http::UtilityTestCase& input) {
case test::common::http::UtilityTestCase::kMakeSetCookieValue: {
const auto& cookie_value = input.make_set_cookie_value();
std::chrono::seconds max_age(cookie_value.max_age());
Http::CookieAttributeRefVector cookie_attributes;
Http::Utility::makeSetCookieValue(cookie_value.key(), cookie_value.value(), cookie_value.path(),
max_age, cookie_value.httponly());
max_age, cookie_value.httponly(), cookie_attributes);
break;
}
case test::common::http::UtilityTestCase::kParseAuthorityString: {
Expand Down
37 changes: 29 additions & 8 deletions test/common/http/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -912,23 +912,44 @@ TEST(HttpUtility, TestParseSetCookieWithQuotes) {
}

TEST(HttpUtility, TestMakeSetCookieValue) {
CookieAttributeRefVector ref_attributes;
EXPECT_EQ("name=\"value\"; Max-Age=10",
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), false));
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), false,
ref_attributes));
EXPECT_EQ("name=\"value\"",
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds::zero(), false));
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds::zero(), false,
ref_attributes));
EXPECT_EQ("name=\"value\"; Max-Age=10; HttpOnly",
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), true));
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds(10), true,
ref_attributes));
EXPECT_EQ("name=\"value\"; HttpOnly",
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds::zero(), true));
Utility::makeSetCookieValue("name", "value", "", std::chrono::seconds::zero(), true,
ref_attributes));

EXPECT_EQ("name=\"value\"; Max-Age=10; Path=/",
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds(10), false));
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds(10), false,
ref_attributes));
EXPECT_EQ("name=\"value\"; Path=/",
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds::zero(), false));
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds::zero(), false,
ref_attributes));
EXPECT_EQ("name=\"value\"; Max-Age=10; Path=/; HttpOnly",
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds(10), true));
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds(10), true,
ref_attributes));
EXPECT_EQ("name=\"value\"; Path=/; HttpOnly",
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds::zero(), true));
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds::zero(), true,
ref_attributes));

std::vector<CookieAttribute> attributes;
attributes.push_back({"SameSite", "None"});
attributes.push_back({"Secure", ""});
attributes.push_back({"Partitioned", ""});
for (const auto& attribute : attributes) {
ref_attributes.push_back(attribute);
}

EXPECT_EQ("name=\"value\"; Path=/; SameSite=None; Secure; Partitioned; HttpOnly",
Utility::makeSetCookieValue("name", "value", "/", std::chrono::seconds::zero(), true,
ref_attributes));
}

TEST(HttpUtility, SendLocalReply) {
Expand Down
Loading