From 2c60632d41555ec8b3d9ef5246242be637a2db0f Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 27 Aug 2020 19:20:54 -0700 Subject: [PATCH] http: header map security fixes for duplicate headers (#197) Previously header matching did not match on all headers for non-inline headers. This patch changes the default behavior to always logically match on all headers. Multiple individual headers will be logically concatenated with ',' similar to what is done with inline headers. This makes the behavior effectively consistent. This behavior can be temporary reverted by setting the runtime value "envoy.reloadable_features.header_match_on_all_headers" to "false". Targeted fixes have been additionally performed on the following extensions which make them consider all duplicate headers by default as a comma concatenated list: 1) Any extension using CEL matching on headers. 2) The header to metadata filter. 3) The JWT filter. 4) The Lua filter. Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. Finally, the setCopy() header map API previously only set the first header in the case of duplicate non-inline headers. setCopy() now behaves similiarly to the other set*() APIs and replaces all found headers with a single value. This may have had security implications in the extauth filter which uses this API. This behavior can be disabled by setting the runtime value "envoy.reloadable_features.http_set_copy_replace_all_headers" to false. Fixes https://github.com/envoyproxy/envoy-setec/issues/188 Signed-off-by: Matt Klein --- docs/root/version_history/v1.15.1.rst | 27 +++++ docs/root/version_history/version_history.rst | 1 + include/envoy/http/header_map.h | 26 +++++ source/common/http/BUILD | 1 + source/common/http/header_map_impl.cc | 52 +++++---- source/common/http/header_map_impl.h | 6 +- source/common/http/header_utility.cc | 51 +++++++-- source/common/http/header_utility.h | 29 +++++ source/common/runtime/runtime_features.cc | 2 + .../extensions/filters/common/expr/context.cc | 13 +++ .../extensions/filters/common/expr/context.h | 22 ++-- .../filters/common/expr/evaluator.cc | 18 ++-- .../filters/common/expr/evaluator.h | 4 +- .../filters/http/header_to_metadata/BUILD | 1 + .../header_to_metadata_filter.cc | 7 +- .../extensions/filters/http/jwt_authn/BUILD | 1 + .../filters/http/jwt_authn/extractor.cc | 8 +- source/extensions/filters/http/lua/BUILD | 1 + .../extensions/filters/http/lua/wrappers.cc | 9 +- test/common/http/BUILD | 2 + test/common/http/header_map_impl_test.cc | 51 ++++++++- test/common/http/header_utility_test.cc | 72 +++++++++++++ .../filters/common/expr/context_test.cc | 41 ++++--- .../common/expr/evaluator_fuzz_test.cc | 2 +- .../header_to_metadata_filter_test.cc | 18 ++++ .../filters/http/jwt_authn/extractor_test.cc | 8 ++ .../filters/http/lua/wrappers_test.cc | 5 + .../http/rbac/rbac_filter_integration_test.cc | 100 ++++++++++++++++++ test/test_common/utility.h | 3 + 29 files changed, 503 insertions(+), 78 deletions(-) create mode 100644 docs/root/version_history/v1.15.1.rst diff --git a/docs/root/version_history/v1.15.1.rst b/docs/root/version_history/v1.15.1.rst new file mode 100644 index 000000000000..57b2acb3f307 --- /dev/null +++ b/docs/root/version_history/v1.15.1.rst @@ -0,0 +1,27 @@ +1.15.1 (TBD) +============ + +Changes +------- +* http: fixed CVE-2020-25017. Previously header matching did not match on all headers for non-inline + headers. This patch changes the default behavior to always logically match on all headers. + Multiple individual headers will be logically concatenated with ',' similar to what is done with + inline headers. This makes the behavior effectively consistent. This behavior can be temporary + reverted by setting the runtime value "envoy.reloadable_features.header_match_on_all_headers" to + "false". + + Targeted fixes have been additionally performed on the following extensions which make them + consider all duplicate headers by default as a comma concatenated list: + + 1. Any extension using CEL matching on headers. + 2. The header to metadata filter. + 3. The JWT filter. + 4. The Lua filter. + + Like primary header matching used in routing, RBAC, etc. this behavior can be disabled by setting + the runtime value "envoy.reloadable_features.header_match_on_all_headers" to false. +* http: The setCopy() header map API previously only set the first header in the case of duplicate + non-inline headers. setCopy() now behaves similarly to the other set*() APIs and replaces all found + headers with a single value. This may have had security implications in the extauth filter which + uses this API. This behavior can be disabled by setting the runtime value + "envoy.reloadable_features.http_set_copy_replace_all_headers" to false. diff --git a/docs/root/version_history/version_history.rst b/docs/root/version_history/version_history.rst index 07db664892d4..d9d9993e500c 100644 --- a/docs/root/version_history/version_history.rst +++ b/docs/root/version_history/version_history.rst @@ -7,6 +7,7 @@ Version history :titlesonly: current + v1.15.1 v1.15.0 v1.14.3 v1.14.2 diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 358b642ed4d1..daa5bbeb1988 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -512,6 +512,32 @@ class HeaderMap { */ virtual const HeaderEntry* get(const LowerCaseString& key) const PURE; + /** + * This is a wrapper for the return result from getAll(). It avoids a copy when translating from + * non-const HeaderEntry to const HeaderEntry and only provides const access to the result. + */ + using NonConstGetResult = absl::InlinedVector; + class GetResult { + public: + GetResult() = default; + explicit GetResult(NonConstGetResult&& result) : result_(std::move(result)) {} + void operator=(GetResult&& rhs) noexcept { result_ = std::move(rhs.result_); } + + bool empty() const { return result_.empty(); } + size_t size() const { return result_.size(); } + const HeaderEntry* operator[](size_t i) const { return result_[i]; } + + private: + NonConstGetResult result_; + }; + + /** + * Get a header by key. + * @param key supplies the header key. + * @return all header entries matching the key. + */ + virtual GetResult getAll(const LowerCaseString& key) const PURE; + // aliases to make iterate() and iterateReverse() callbacks easier to read enum class Iterate { Continue, Break }; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 580a30ac64bc..fcc08663dcc2 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -306,6 +306,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:non_copyable", "//source/common/common:utility_lib", + "//source/common/runtime:runtime_features_lib", "//source/common/singleton:const_singleton", ], ) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ce63493486b7..2f6dec47dd22 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -10,6 +10,7 @@ #include "common/common/assert.h" #include "common/common/dump_state_utils.h" #include "common/common/empty_string.h" +#include "common/runtime/runtime_features.h" #include "common/singleton/const_singleton.h" #include "absl/strings/match.h" @@ -383,9 +384,9 @@ void HeaderMapImpl::addCopy(const LowerCaseString& key, absl::string_view value) void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view value) { // TODO(#9221): converge on and document a policy for coalescing multiple headers. - auto* entry = getExisting(key); - if (entry) { - const uint64_t added_size = appendToHeader(entry->value(), value); + auto entry = getExisting(key); + if (!entry.empty()) { + const uint64_t added_size = appendToHeader(entry[0]->value(), value); addSize(added_size); } else { addCopy(key, value); @@ -393,29 +394,27 @@ void HeaderMapImpl::appendCopy(const LowerCaseString& key, absl::string_view val } void HeaderMapImpl::setReference(const LowerCaseString& key, absl::string_view value) { - HeaderString ref_key(key); - HeaderString ref_value(value); remove(key); - insertByKey(std::move(ref_key), std::move(ref_value)); + addReference(key, value); } void HeaderMapImpl::setReferenceKey(const LowerCaseString& key, absl::string_view value) { - HeaderString ref_key(key); - HeaderString new_value; - new_value.setCopy(value); remove(key); - insertByKey(std::move(ref_key), std::move(new_value)); - ASSERT(new_value.empty()); // NOLINT(bugprone-use-after-move) + addReferenceKey(key, value); } void HeaderMapImpl::setCopy(const LowerCaseString& key, absl::string_view value) { - // Replaces the first occurrence of a header if it exists, otherwise adds by copy. - // TODO(#9221): converge on and document a policy for coalescing multiple headers. - auto* entry = getExisting(key); - if (entry) { - updateSize(entry->value().size(), value.size()); - entry->value(value); + if (!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http_set_copy_replace_all_headers")) { + auto entry = getExisting(key); + if (!entry.empty()) { + updateSize(entry[0]->value().size(), value.size()); + entry[0]->value(value); + } else { + addCopy(key, value); + } } else { + remove(key); addCopy(key, value); } } @@ -433,17 +432,26 @@ void HeaderMapImpl::verifyByteSizeInternalForTest() const { } const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { - return const_cast(this)->getExisting(key); + const auto result = getAll(key); + return result.empty() ? nullptr : result[0]; } -HeaderEntry* HeaderMapImpl::getExisting(const LowerCaseString& key) { +HeaderMap::GetResult HeaderMapImpl::getAll(const LowerCaseString& key) const { + return HeaderMap::GetResult(const_cast(this)->getExisting(key)); +} + +HeaderMap::NonConstGetResult HeaderMapImpl::getExisting(const LowerCaseString& key) { // Attempt a trie lookup first to see if the user is requesting an O(1) header. This may be // relatively common in certain header matching / routing patterns. // TODO(mattklein123): Add inline handle support directly to the header matcher code to support // this use case more directly. + HeaderMap::NonConstGetResult ret; auto lookup = staticLookup(key.get()); if (lookup.has_value()) { - return *lookup.value().entry_; + if (*lookup.value().entry_ != nullptr) { + ret.push_back(*lookup.value().entry_); + } + return ret; } // If the requested header is not an O(1) header we do a full scan. Doing the trie lookup is @@ -454,11 +462,11 @@ HeaderEntry* HeaderMapImpl::getExisting(const LowerCaseString& key) { // implementation or potentially create a lazy map if the size of the map is above a threshold. for (HeaderEntryImpl& header : headers_) { if (header.key() == key.get().c_str()) { - return &header; + ret.push_back(&header); } } - return nullptr; + return ret; } void HeaderMapImpl::iterate(HeaderMap::ConstIterateCb cb) const { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index d4cb88bdcacb..4ac2839b2a64 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -86,6 +86,7 @@ class HeaderMapImpl : NonCopyable { void setCopy(const LowerCaseString& key, absl::string_view value); uint64_t byteSize() const; const HeaderEntry* get(const LowerCaseString& key) const; + HeaderMap::GetResult getAll(const LowerCaseString& key) const; void iterate(HeaderMap::ConstIterateCb cb) const; void iterateReverse(HeaderMap::ConstIterateCb cb) const; void clear(); @@ -241,7 +242,7 @@ class HeaderMapImpl : NonCopyable { HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key, HeaderString&& value); - HeaderEntry* getExisting(const LowerCaseString& key); + HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key); size_t removeInline(HeaderEntryImpl** entry); void updateSize(uint64_t from_size, uint64_t to_size); void addSize(uint64_t size); @@ -299,6 +300,9 @@ template class TypedHeaderMapImpl : public HeaderMapImpl, publ const HeaderEntry* get(const LowerCaseString& key) const override { return HeaderMapImpl::get(key); } + HeaderMap::GetResult getAll(const LowerCaseString& key) const override { + return HeaderMapImpl::getAll(key); + } void iterate(HeaderMap::ConstIterateCb cb) const override { HeaderMapImpl::iterate(cb); } void iterateReverse(HeaderMap::ConstIterateCb cb) const override { HeaderMapImpl::iterateReverse(cb); diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 3b1726d0304e..91f15b4aeb0d 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -105,39 +105,68 @@ bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, return true; } +HeaderUtility::GetAllOfHeaderAsStringResult +HeaderUtility::getAllOfHeaderAsString(const HeaderMap& headers, const Http::LowerCaseString& key) { + GetAllOfHeaderAsStringResult result; + const auto header_value = headers.getAll(key); + + if (header_value.empty()) { + // Empty for clarity. Avoid handling the empty case in the block below if the runtime feature + // is disabled. + } else if (header_value.size() == 1 || + !Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.http_match_on_all_headers")) { + result.result_ = header_value[0]->value().getStringView(); + } else { + // In this case we concatenate all found headers using a ',' delimiter before performing the + // final match. We use an InlinedVector of absl::string_view to invoke the optimized join + // algorithm. This requires a copying phase before we invoke join. The 3 used as the inline + // size has been arbitrarily chosen. + // TODO(mattklein123): Do we need to normalize any whitespace here? + absl::InlinedVector string_view_vector; + string_view_vector.reserve(header_value.size()); + for (size_t i = 0; i < header_value.size(); i++) { + string_view_vector.push_back(header_value[i]->value().getStringView()); + } + result.result_backing_string_ = absl::StrJoin(string_view_vector, ","); + } + + return result; +} + bool HeaderUtility::matchHeaders(const HeaderMap& request_headers, const HeaderData& header_data) { - const HeaderEntry* header = request_headers.get(header_data.name_); + const auto header_value = getAllOfHeaderAsString(request_headers, header_data.name_); - if (header == nullptr) { + if (!header_value.result().has_value()) { return header_data.invert_match_ && header_data.header_match_type_ == HeaderMatchType::Present; } bool match; - const absl::string_view header_view = header->value().getStringView(); switch (header_data.header_match_type_) { case HeaderMatchType::Value: - match = header_data.value_.empty() || header_view == header_data.value_; + match = header_data.value_.empty() || header_value.result().value() == header_data.value_; break; case HeaderMatchType::Regex: - match = header_data.regex_->match(header_view); + match = header_data.regex_->match(header_value.result().value()); break; case HeaderMatchType::Range: { - int64_t header_value = 0; - match = absl::SimpleAtoi(header_view, &header_value) && - header_value >= header_data.range_.start() && header_value < header_data.range_.end(); + int64_t header_int_value = 0; + match = absl::SimpleAtoi(header_value.result().value(), &header_int_value) && + header_int_value >= header_data.range_.start() && + header_int_value < header_data.range_.end(); break; } case HeaderMatchType::Present: match = true; break; case HeaderMatchType::Prefix: - match = absl::StartsWith(header_view, header_data.value_); + match = absl::StartsWith(header_value.result().value(), header_data.value_); break; case HeaderMatchType::Suffix: - match = absl::EndsWith(header_view, header_data.value_); + match = absl::EndsWith(header_value.result().value(), header_data.value_); break; case HeaderMatchType::Contains: - match = absl::StrContains(header_view, header_data.value_); + match = absl::StrContains(header_value.result().value(), header_data.value_); break; default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 27d2b9907361..cd9669efcf14 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -33,6 +33,35 @@ class HeaderUtility { static void getAllOfHeader(const HeaderMap& headers, absl::string_view key, std::vector& out); + /** + * Get all header values as a single string. Multiple headers are concatenated with ','. + */ + class GetAllOfHeaderAsStringResult { + public: + // The ultimate result of the concatenation. If absl::nullopt, no header values were found. + // If the final string required a string allocation, the memory is held in + // backingString(). This allows zero allocation in the common case of a single header + // value. + absl::optional result() const { + // This is safe for move/copy of this class as the backing string will be moved or copied. + // Otherwise result_ is valid. The assert verifies that both are empty or only 1 is set. + ASSERT((!result_.has_value() && result_backing_string_.empty()) || + (result_.has_value() ^ !result_backing_string_.empty())); + return !result_backing_string_.empty() ? result_backing_string_ : result_; + } + + const std::string& backingString() const { return result_backing_string_; } + + private: + absl::optional result_; + // Valid only if result_ relies on memory allocation that must live beyond the call. See above. + std::string result_backing_string_; + + friend class HeaderUtility; + }; + static GetAllOfHeaderAsStringResult getAllOfHeaderAsString(const HeaderMap& headers, + const Http::LowerCaseString& key); + // A HeaderData specifies one of exact value or regex or range element // to match in a request's header, specified in the header_match_type_ member. // It is the runtime equivalent of the HeaderMatchSpecifier proto in RDS API. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 3415be06f081..173286c1489b 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -76,6 +76,8 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.fixed_connection_close", "envoy.reloadable_features.hcm_stream_error_on_invalid_message", "envoy.reloadable_features.http_default_alpn", + "envoy.reloadable_features.http_match_on_all_headers", + "envoy.reloadable_features.http_set_copy_replace_all_headers", "envoy.reloadable_features.http_transport_failure_reason_in_body", "envoy.reloadable_features.http2_skip_encoding_empty_trailers", "envoy.reloadable_features.listener_in_place_filterchain_update", diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index 0655b1c735d8..a33228bcd73a 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -23,6 +23,19 @@ absl::optional convertHeaderEntry(const Http::HeaderEntry* header) { return CelValue::CreateStringView(header->value().getStringView()); } +absl::optional +convertHeaderEntry(Protobuf::Arena& arena, + Http::HeaderUtility::GetAllOfHeaderAsStringResult&& result) { + if (!result.result().has_value()) { + return {}; + } else if (!result.backingString().empty()) { + return CelValue::CreateString( + Protobuf::Arena::Create(&arena, result.backingString())); + } else { + return CelValue::CreateStringView(result.result().value()); + } +} + namespace { absl::optional extractSslInfo(const Ssl::ConnectionInfo& ssl_info, diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 09bf8d6a559e..fd4b386a9a32 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -4,6 +4,7 @@ #include "envoy/stream_info/stream_info.h" #include "common/grpc/status.h" +#include "common/http/header_utility.h" #include "common/http/headers.h" #include "eval/public/cel_value.h" @@ -76,10 +77,13 @@ constexpr absl::string_view UpstreamTransportFailureReason = "transport_failure_ class RequestWrapper; absl::optional convertHeaderEntry(const Http::HeaderEntry* header); +absl::optional +convertHeaderEntry(Protobuf::Arena& arena, + Http::HeaderUtility::GetAllOfHeaderAsStringResult&& result); template class HeadersWrapper : public google::api::expr::runtime::CelMap { public: - HeadersWrapper(const T* value) : value_(value) {} + HeadersWrapper(Protobuf::Arena& arena, const T* value) : arena_(arena), value_(value) {} absl::optional operator[](CelValue key) const override { if (value_ == nullptr || !key.IsString()) { return {}; @@ -89,8 +93,8 @@ template class HeadersWrapper : public google::api::expr::runtime::Cel // Reject key if it is an invalid header string return {}; } - auto out = value_->get(Http::LowerCaseString(str)); - return convertHeaderEntry(out); + return convertHeaderEntry( + arena_, Http::HeaderUtility::getAllOfHeaderAsString(*value_, Http::LowerCaseString(str))); } int size() const override { return value_ == nullptr ? 0 : value_->size(); } bool empty() const override { return value_ == nullptr ? true : value_->empty(); } @@ -101,6 +105,7 @@ template class HeadersWrapper : public google::api::expr::runtime::Cel private: friend class RequestWrapper; friend class ResponseWrapper; + Protobuf::Arena& arena_; const T* value_; }; @@ -127,8 +132,9 @@ class BaseWrapper : public google::api::expr::runtime::CelMap, class RequestWrapper : public BaseWrapper { public: - RequestWrapper(const Http::RequestHeaderMap* headers, const StreamInfo::StreamInfo& info) - : headers_(headers), info_(info) {} + RequestWrapper(Protobuf::Arena& arena, const Http::RequestHeaderMap* headers, + const StreamInfo::StreamInfo& info) + : headers_(arena, headers), info_(info) {} absl::optional operator[](CelValue key) const override; private: @@ -138,9 +144,9 @@ class RequestWrapper : public BaseWrapper { class ResponseWrapper : public BaseWrapper { public: - ResponseWrapper(const Http::ResponseHeaderMap* headers, const Http::ResponseTrailerMap* trailers, - const StreamInfo::StreamInfo& info) - : headers_(headers), trailers_(trailers), info_(info) {} + ResponseWrapper(Protobuf::Arena& arena, const Http::ResponseHeaderMap* headers, + const Http::ResponseTrailerMap* trailers, const StreamInfo::StreamInfo& info) + : headers_(arena, headers), trailers_(arena, trailers), info_(info) {} absl::optional operator[](CelValue key) const override; private: diff --git a/source/extensions/filters/common/expr/evaluator.cc b/source/extensions/filters/common/expr/evaluator.cc index fee62c0ad9fa..d2c376684f72 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -11,14 +11,15 @@ namespace Filters { namespace Common { namespace Expr { -ActivationPtr createActivation(const StreamInfo::StreamInfo& info, +ActivationPtr createActivation(Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, const Http::RequestHeaderMap* request_headers, const Http::ResponseHeaderMap* response_headers, const Http::ResponseTrailerMap* response_trailers) { auto activation = std::make_unique(); - activation->InsertValueProducer(Request, std::make_unique(request_headers, info)); - activation->InsertValueProducer( - Response, std::make_unique(response_headers, response_trailers, info)); + activation->InsertValueProducer(Request, + std::make_unique(arena, request_headers, info)); + activation->InsertValueProducer(Response, std::make_unique( + arena, response_headers, response_trailers, info)); activation->InsertValueProducer(Connection, std::make_unique(info)); activation->InsertValueProducer(Upstream, std::make_unique(info)); activation->InsertValueProducer(Source, std::make_unique(info, false)); @@ -67,13 +68,14 @@ ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alph return std::move(cel_expression_status.value()); } -absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, +absl::optional evaluate(const Expression& expr, Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, const Http::RequestHeaderMap* request_headers, const Http::ResponseHeaderMap* response_headers, const Http::ResponseTrailerMap* response_trailers) { - auto activation = createActivation(info, request_headers, response_headers, response_trailers); - auto eval_status = expr.Evaluate(*activation, arena); + auto activation = + createActivation(arena, info, request_headers, response_headers, response_trailers); + auto eval_status = expr.Evaluate(*activation, &arena); if (!eval_status.ok()) { return {}; } @@ -84,7 +86,7 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena bool matches(const Expression& expr, const StreamInfo::StreamInfo& info, const Http::RequestHeaderMap& headers) { Protobuf::Arena arena; - auto eval_status = Expr::evaluate(expr, &arena, info, &headers, nullptr, nullptr); + auto eval_status = Expr::evaluate(expr, arena, info, &headers, nullptr, nullptr); if (!eval_status.has_value()) { return false; } diff --git a/source/extensions/filters/common/expr/evaluator.h b/source/extensions/filters/common/expr/evaluator.h index 116239fde1a5..37fdf63ab1bf 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -25,7 +25,7 @@ using ExpressionPtr = std::unique_ptr; // Creates an activation providing the common context attributes. // The activation lazily creates wrappers during an evaluation using the evaluation arena. -ActivationPtr createActivation(const StreamInfo::StreamInfo& info, +ActivationPtr createActivation(Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, const Http::RequestHeaderMap* request_headers, const Http::ResponseHeaderMap* response_headers, const Http::ResponseTrailerMap* response_trailers); @@ -41,7 +41,7 @@ ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alph // Evaluates an expression for a request. The arena is used to hold intermediate computational // results and potentially the final value. -absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, +absl::optional evaluate(const Expression& expr, Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, const Http::RequestHeaderMap* request_headers, const Http::ResponseHeaderMap* response_headers, diff --git a/source/extensions/filters/http/header_to_metadata/BUILD b/source/extensions/filters/http/header_to_metadata/BUILD index 1bbe574312e6..ad4f9bcf8cfe 100644 --- a/source/extensions/filters/http/header_to_metadata/BUILD +++ b/source/extensions/filters/http/header_to_metadata/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( deps = [ "//include/envoy/server:filter_config_interface", "//source/common/common:base64_lib", + "//source/common/http:header_utility_lib", "//source/common/http:utility_lib", "//source/extensions/filters/http:well_known_names", "@envoy_api//envoy/extensions/filters/http/header_to_metadata/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc b/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc index 9550e243c6a1..2d5358f89aeb 100644 --- a/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc +++ b/source/extensions/filters/http/header_to_metadata/header_to_metadata_filter.cc @@ -5,6 +5,7 @@ #include "common/common/base64.h" #include "common/common/regex.h" #include "common/config/well_known_names.h" +#include "common/http/header_utility.h" #include "common/http/utility.h" #include "common/protobuf/protobuf.h" @@ -20,12 +21,12 @@ namespace HeaderToMetadataFilter { // Extract the value of the header. absl::optional HeaderValueSelector::extract(Http::HeaderMap& map) const { - const Http::HeaderEntry* header_entry = map.get(header_); - if (header_entry == nullptr) { + const auto header_value = Http::HeaderUtility::getAllOfHeaderAsString(map, header_); + if (!header_value.result().has_value()) { return absl::nullopt; } // Catch the value in the header before removing. - absl::optional value = std::string(header_entry->value().getStringView()); + absl::optional value = std::string(header_value.result().value()); if (remove_) { map.remove(header_); } diff --git a/source/extensions/filters/http/jwt_authn/BUILD b/source/extensions/filters/http/jwt_authn/BUILD index f0249b014ea1..1df6425b585f 100644 --- a/source/extensions/filters/http/jwt_authn/BUILD +++ b/source/extensions/filters/http/jwt_authn/BUILD @@ -14,6 +14,7 @@ envoy_cc_library( srcs = ["extractor.cc"], hdrs = ["extractor.h"], deps = [ + "//source/common/http:header_utility_lib", "//source/common/http:utility_lib", "@envoy_api//envoy/extensions/filters/http/jwt_authn/v3:pkg_cc_proto", ], diff --git a/source/extensions/filters/http/jwt_authn/extractor.cc b/source/extensions/filters/http/jwt_authn/extractor.cc index b84f9fb4178f..b1ec12058ea9 100644 --- a/source/extensions/filters/http/jwt_authn/extractor.cc +++ b/source/extensions/filters/http/jwt_authn/extractor.cc @@ -5,6 +5,7 @@ #include "envoy/extensions/filters/http/jwt_authn/v3/config.pb.h" #include "common/common/utility.h" +#include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/utility.h" #include "common/singleton/const_singleton.h" @@ -187,9 +188,10 @@ ExtractorImpl::extract(const Http::RequestHeaderMap& headers) const { for (const auto& location_it : header_locations_) { const auto& location_spec = location_it.second; ENVOY_LOG(debug, "extract {}", location_it.first); - const Http::HeaderEntry* entry = headers.get(location_spec->header_); - if (entry) { - auto value_str = entry->value().getStringView(); + const auto result = + Http::HeaderUtility::getAllOfHeaderAsString(headers, location_spec->header_); + if (result.result().has_value()) { + auto value_str = result.result().value(); if (!location_spec->value_prefix_.empty()) { const auto pos = value_str.find(location_spec->value_prefix_); if (pos == absl::string_view::npos) { diff --git a/source/extensions/filters/http/lua/BUILD b/source/extensions/filters/http/lua/BUILD index 25b02828a28f..188b0c484752 100644 --- a/source/extensions/filters/http/lua/BUILD +++ b/source/extensions/filters/http/lua/BUILD @@ -43,6 +43,7 @@ envoy_cc_library( "//include/envoy/http:header_map_interface", "//include/envoy/stream_info:stream_info_interface", "//source/common/crypto:utility_lib", + "//source/common/http:header_utility_lib", "//source/common/http:utility_lib", "//source/extensions/common/crypto:utility_lib", "//source/extensions/filters/common/lua:lua_lib", diff --git a/source/extensions/filters/http/lua/wrappers.cc b/source/extensions/filters/http/lua/wrappers.cc index f716519dbefb..cb31e695f8be 100644 --- a/source/extensions/filters/http/lua/wrappers.cc +++ b/source/extensions/filters/http/lua/wrappers.cc @@ -1,5 +1,6 @@ #include "extensions/filters/http/lua/wrappers.h" +#include "common/http/header_utility.h" #include "common/http/utility.h" #include "extensions/filters/common/lua/wrappers.h" @@ -42,10 +43,10 @@ int HeaderMapWrapper::luaAdd(lua_State* state) { int HeaderMapWrapper::luaGet(lua_State* state) { const char* key = luaL_checkstring(state, 2); - const Http::HeaderEntry* entry = headers_.get(Http::LowerCaseString(key)); - if (entry != nullptr) { - lua_pushlstring(state, entry->value().getStringView().data(), - entry->value().getStringView().length()); + const auto value = + Http::HeaderUtility::getAllOfHeaderAsString(headers_, Http::LowerCaseString(key)); + if (value.result().has_value()) { + lua_pushlstring(state, value.result().value().data(), value.result().value().length()); return 1; } else { return 0; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 83668959f511..9acc034717dc 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -263,6 +263,7 @@ envoy_cc_test( "//source/common/http:header_list_view_lib", "//source/common/http:header_map_lib", "//source/common/http:header_utility_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) @@ -304,6 +305,7 @@ envoy_cc_test( srcs = ["header_utility_test.cc"], deps = [ "//source/common/http:header_utility_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/route/v3:pkg_cc_proto", ], diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 0e5b0c3df8cd..94995001405f 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -7,6 +7,7 @@ #include "common/http/header_utility.h" #include "test/test_common/printers.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -728,7 +729,11 @@ TEST(HeaderMapImplTest, SetReferenceKey) { EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); } -TEST(HeaderMapImplTest, SetCopy) { +TEST(HeaderMapImplTest, SetCopyOldBehavior) { + TestScopedRuntime runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.http_set_copy_replace_all_headers", "false"}}); + TestRequestHeaderMapImpl headers; LowerCaseString foo("hello"); headers.setCopy(foo, "world"); @@ -773,6 +778,50 @@ TEST(HeaderMapImplTest, SetCopy) { EXPECT_EQ(headers.getPathValue(), "/foo"); } +TEST(HeaderMapImplTest, SetCopyNewBehavior) { + TestRequestHeaderMapImpl headers; + LowerCaseString foo("hello"); + headers.setCopy(foo, "world"); + EXPECT_EQ("world", headers.get(foo)->value().getStringView()); + + // Overwrite value. + headers.setCopy(foo, "monde"); + EXPECT_EQ("monde", headers.get(foo)->value().getStringView()); + + // Add another foo header. + headers.addCopy(foo, "monde2"); + EXPECT_EQ(headers.size(), 2); + + // The foo header is overridden. + headers.setCopy(foo, "override-monde"); + EXPECT_EQ(headers.size(), 1); + + HeaderAndValueCb cb; + + InSequence seq; + EXPECT_CALL(cb, Call("hello", "override-monde")); + headers.iterate(cb.asIterateCb()); + + // Test setting an empty string and then overriding. + EXPECT_EQ(1UL, headers.remove(foo)); + EXPECT_EQ(headers.size(), 0); + const std::string empty; + headers.setCopy(foo, empty); + EXPECT_EQ(headers.size(), 1); + headers.setCopy(foo, "not-empty"); + EXPECT_EQ(headers.get(foo)->value().getStringView(), "not-empty"); + + // Use setCopy with inline headers both indirectly and directly. + headers.clear(); + EXPECT_EQ(headers.size(), 0); + headers.setCopy(Headers::get().Path, "/"); + EXPECT_EQ(headers.size(), 1); + EXPECT_EQ(headers.getPathValue(), "/"); + headers.setPath("/foo"); + EXPECT_EQ(headers.size(), 1); + EXPECT_EQ(headers.getPathValue(), "/foo"); +} + TEST(HeaderMapImplTest, AddCopy) { TestRequestHeaderMapImpl headers; diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index ae0aaba39c42..068eb67eec8b 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -8,6 +8,7 @@ #include "common/http/header_utility.h" #include "common/json/json_loader.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -71,6 +72,53 @@ TEST_F(HeaderUtilityTest, RemovePortsFromHostConnect) { } } +TEST(GetAllOfHeaderAsStringTest, All) { + const LowerCaseString test_header("test"); + { + TestRequestHeaderMapImpl headers; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_FALSE(ret.result().has_value()); + EXPECT_TRUE(ret.backingString().empty()); + } + { + TestRequestHeaderMapImpl headers{{"test", "foo"}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ("foo", ret.result().value()); + EXPECT_TRUE(ret.backingString().empty()); + } + { + TestRequestHeaderMapImpl headers{{"test", "foo"}, {"test", "bar"}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ("foo,bar", ret.result().value()); + EXPECT_EQ("foo,bar", ret.backingString()); + } + { + TestRequestHeaderMapImpl headers{{"test", ""}, {"test", "bar"}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ(",bar", ret.result().value()); + EXPECT_EQ(",bar", ret.backingString()); + } + { + TestRequestHeaderMapImpl headers{{"test", ""}, {"test", ""}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ(",", ret.result().value()); + EXPECT_EQ(",", ret.backingString()); + } + { + TestRequestHeaderMapImpl headers{ + {"test", "a"}, {"test", "b"}, {"test", "c"}, {"test", ""}, {"test", ""}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ("a,b,c,,", ret.result().value()); + EXPECT_EQ("a,b,c,,", ret.backingString()); + // Make sure copying the return value works correctly. + const auto ret2 = ret; // NOLINT(performance-unnecessary-copy-initialization) + EXPECT_EQ(ret2.result(), ret.result()); + EXPECT_EQ(ret2.backingString(), ret.backingString()); + EXPECT_EQ(ret2.result().value().data(), ret2.backingString().data()); + EXPECT_NE(ret2.result().value().data(), ret.backingString().data()); + } +} + TEST(HeaderDataConstructorTest, NoSpecifierSet) { const std::string yaml = R"EOF( name: test-header @@ -236,8 +284,32 @@ regex_match: (a|b) EXPECT_FALSE(HeaderUtility::matchHeaders(headers, header_data)); headers.addCopy("match-header", "a"); + // With a single "match-header" this regex will match. EXPECT_TRUE(HeaderUtility::matchHeaders(headers, header_data)); + headers.addCopy("match-header", "b"); + // With two "match-header" we now logically have "a,b" as the value, so the regex will not match. + EXPECT_FALSE(HeaderUtility::matchHeaders(headers, header_data)); + + header_data[0] = std::make_unique(parseHeaderMatcherFromYaml(R"EOF( +name: match-header +exact_match: a,b + )EOF")); + // Make sure that an exact match on "a,b" does in fact work. + EXPECT_TRUE(HeaderUtility::matchHeaders(headers, header_data)); + + TestScopedRuntime runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.http_match_on_all_headers", "false"}}); + // Flipping runtime to false should make "a,b" no longer match because we will match on the first + // header only. + EXPECT_FALSE(HeaderUtility::matchHeaders(headers, header_data)); + + header_data[0] = std::make_unique(parseHeaderMatcherFromYaml(R"EOF( +name: match-header +exact_match: a + )EOF")); + // With runtime off, exact match on "a" should pass. EXPECT_TRUE(HeaderUtility::matchHeaders(headers, header_data)); } diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index bdcb4cef091d..0ac086e93603 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -25,7 +25,8 @@ namespace { constexpr absl::string_view Undefined = "undefined"; TEST(Context, EmptyHeadersAttributes) { - HeadersWrapper headers(nullptr); + Protobuf::Arena arena; + HeadersWrapper headers(arena, nullptr); auto header = headers[CelValue::CreateStringView(Referer)]; EXPECT_FALSE(header.has_value()); EXPECT_EQ(0, headers.size()); @@ -34,7 +35,8 @@ TEST(Context, EmptyHeadersAttributes) { TEST(Context, InvalidRequest) { Http::TestRequestHeaderMapImpl header_map{{"referer", "dogs.com"}}; - HeadersWrapper headers(&header_map); + Protobuf::Arena arena; + HeadersWrapper headers(arena, &header_map); auto header = headers[CelValue::CreateStringView("dogs.com\n")]; EXPECT_FALSE(header.has_value()); } @@ -45,10 +47,11 @@ TEST(Context, RequestAttributes) { Http::TestRequestHeaderMapImpl header_map{ {":method", "POST"}, {":scheme", "http"}, {":path", "/meow?yes=1"}, {":authority", "kittens.com"}, {"referer", "dogs.com"}, {"user-agent", "envoy-mobile"}, - {"content-length", "10"}, {"x-request-id", "blah"}, - }; - RequestWrapper request(&header_map, info); - RequestWrapper empty_request(nullptr, empty_info); + {"content-length", "10"}, {"x-request-id", "blah"}, {"double-header", "foo"}, + {"double-header", "bar"}}; + Protobuf::Arena arena; + RequestWrapper request(arena, &header_map, info); + RequestWrapper empty_request(arena, nullptr, empty_info); EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); // "2018-04-03T23:06:09.123Z". @@ -145,7 +148,7 @@ TEST(Context, RequestAttributes) { EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); // this includes the headers size - EXPECT_EQ(138, value.value().Int64OrDie()); + EXPECT_EQ(170, value.value().Int64OrDie()); } { @@ -169,12 +172,17 @@ TEST(Context, RequestAttributes) { ASSERT_TRUE(value.value().IsMap()); auto& map = *value.value().MapOrDie(); EXPECT_FALSE(map.empty()); - EXPECT_EQ(8, map.size()); + EXPECT_EQ(10, map.size()); auto header = map[CelValue::CreateStringView(Referer)]; EXPECT_TRUE(header.has_value()); ASSERT_TRUE(header.value().IsString()); EXPECT_EQ("dogs.com", header.value().StringOrDie().value()); + + auto header2 = map[CelValue::CreateStringView("double-header")]; + EXPECT_TRUE(header2.has_value()); + ASSERT_TRUE(header2.value().IsString()); + EXPECT_EQ("foo,bar", header2.value().StringOrDie().value()); } { @@ -209,7 +217,8 @@ TEST(Context, RequestFallbackAttributes) { {":scheme", "http"}, {":path", "/meow?yes=1"}, }; - RequestWrapper request(&header_map, info); + Protobuf::Arena arena; + RequestWrapper request(arena, &header_map, info); EXPECT_CALL(info, bytesReceived()).WillRepeatedly(Return(10)); @@ -236,8 +245,9 @@ TEST(Context, ResponseAttributes) { const std::string grpc_status = "grpc-status"; Http::TestResponseHeaderMapImpl header_map{{header_name, "a"}}; Http::TestResponseTrailerMapImpl trailer_map{{trailer_name, "b"}, {grpc_status, "8"}}; - ResponseWrapper response(&header_map, &trailer_map, info); - ResponseWrapper empty_response(nullptr, nullptr, empty_info); + Protobuf::Arena arena; + ResponseWrapper response(arena, &header_map, &trailer_map, info); + ResponseWrapper empty_response(arena, nullptr, nullptr, empty_info); EXPECT_CALL(info, responseCode()).WillRepeatedly(Return(404)); EXPECT_CALL(info, bytesSent()).WillRepeatedly(Return(123)); @@ -354,7 +364,8 @@ TEST(Context, ResponseAttributes) { { Http::TestResponseHeaderMapImpl header_map{{header_name, "a"}, {grpc_status, "7"}}; Http::TestResponseTrailerMapImpl trailer_map{{trailer_name, "b"}}; - ResponseWrapper response_header_status(&header_map, &trailer_map, info); + Protobuf::Arena arena; + ResponseWrapper response_header_status(arena, &header_map, &trailer_map, info); auto value = response_header_status[CelValue::CreateStringView(GrpcStatus)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); @@ -363,7 +374,8 @@ TEST(Context, ResponseAttributes) { { Http::TestResponseHeaderMapImpl header_map{{header_name, "a"}}; Http::TestResponseTrailerMapImpl trailer_map{{trailer_name, "b"}}; - ResponseWrapper response_no_status(&header_map, &trailer_map, info); + Protobuf::Arena arena; + ResponseWrapper response_no_status(arena, &header_map, &trailer_map, info); auto value = response_no_status[CelValue::CreateStringView(GrpcStatus)]; EXPECT_TRUE(value.has_value()); ASSERT_TRUE(value.value().IsInt64()); @@ -373,7 +385,8 @@ TEST(Context, ResponseAttributes) { NiceMock info_without_code; Http::TestResponseHeaderMapImpl header_map{{header_name, "a"}}; Http::TestResponseTrailerMapImpl trailer_map{{trailer_name, "b"}}; - ResponseWrapper response_no_status(&header_map, &trailer_map, info_without_code); + Protobuf::Arena arena; + ResponseWrapper response_no_status(arena, &header_map, &trailer_map, info_without_code); auto value = response_no_status[CelValue::CreateStringView(GrpcStatus)]; EXPECT_FALSE(value.has_value()); } diff --git a/test/extensions/filters/common/expr/evaluator_fuzz_test.cc b/test/extensions/filters/common/expr/evaluator_fuzz_test.cc index 2f29c9f0023c..6c3867747ede 100644 --- a/test/extensions/filters/common/expr/evaluator_fuzz_test.cc +++ b/test/extensions/filters/common/expr/evaluator_fuzz_test.cc @@ -44,7 +44,7 @@ DEFINE_PROTO_FUZZER(const test::extensions::filters::common::expr::EvaluatorTest // Evaluate the CEL expression. Protobuf::Arena arena; - Expr::evaluate(*expr, &arena, *stream_info, &request_headers, &response_headers, + Expr::evaluate(*expr, arena, *stream_info, &request_headers, &response_headers, &response_trailers); } catch (const CelException& e) { ENVOY_LOG_MISC(debug, "CelException: {}", e.what()); diff --git a/test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc b/test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc index fc9c81431ae1..9a88da9ee050 100644 --- a/test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc +++ b/test/extensions/filters/http/header_to_metadata/header_to_metadata_filter_test.cc @@ -109,6 +109,24 @@ TEST_F(HeaderToMetadataTest, BasicRequestTest) { filter_->onDestroy(); } +// Verify concatenation works. +TEST_F(HeaderToMetadataTest, BasicRequestDoubleHeadersTest) { + initializeFilter(request_config_yaml); + Http::TestRequestHeaderMapImpl incoming_headers{{"X-VERSION", "foo"}, {"X-VERSION", "bar"}}; + std::map expected = {{"version", "foo,bar"}}; + + EXPECT_CALL(decoder_callbacks_, streamInfo()).WillRepeatedly(ReturnRef(req_info_)); + EXPECT_CALL(req_info_, setDynamicMetadata("envoy.lb", MapEq(expected))); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(incoming_headers, false)); + Http::MetadataMap metadata_map{{"metadata", "metadata"}}; + EXPECT_EQ(Http::FilterMetadataStatus::Continue, filter_->decodeMetadata(metadata_map)); + Buffer::OwnedImpl data("data"); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false)); + Http::TestRequestTrailerMapImpl incoming_trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(incoming_trailers)); + filter_->onDestroy(); +} + TEST_F(HeaderToMetadataTest, PerRouteOverride) { // Global config is empty. initializeFilter("{}"); diff --git a/test/extensions/filters/http/jwt_authn/extractor_test.cc b/test/extensions/filters/http/jwt_authn/extractor_test.cc index d91f2c7dfee1..a32d819fbeae 100644 --- a/test/extensions/filters/http/jwt_authn/extractor_test.cc +++ b/test/extensions/filters/http/jwt_authn/extractor_test.cc @@ -170,6 +170,14 @@ TEST_F(ExtractorTest, TestCustomHeaderToken) { EXPECT_FALSE(headers.get(Http::LowerCaseString("token-header"))); } +// Make sure a double custom header concatenates the token +TEST_F(ExtractorTest, TestDoubleCustomHeaderToken) { + auto headers = TestRequestHeaderMapImpl{{"token-header", "jwt_token"}, {"token-header", "foo"}}; + auto tokens = extractor_->extract(headers); + EXPECT_EQ(tokens.size(), 1); + EXPECT_EQ(tokens[0]->token(), "jwt_token,foo"); +} + // Test extracting token from the custom header: "prefix-header" // value prefix doesn't match. It has to be either "AAA" or "AAABBB". TEST_F(ExtractorTest, TestPrefixHeaderNotMatch) { diff --git a/test/extensions/filters/http/lua/wrappers_test.cc b/test/extensions/filters/http/lua/wrappers_test.cc index 7ccf297bf343..e3b03f538eaf 100644 --- a/test/extensions/filters/http/lua/wrappers_test.cc +++ b/test/extensions/filters/http/lua/wrappers_test.cc @@ -44,6 +44,10 @@ TEST_F(LuaHeaderMapWrapperTest, Methods) { for key, value in pairs(object) do testPrint(string.format("'%s' '%s'", key, value)) end + + object:add("header3", "foo") + object:add("header3", "bar") + testPrint(object:get("header3")) end )EOF"}; @@ -58,6 +62,7 @@ TEST_F(LuaHeaderMapWrapperTest, Methods) { EXPECT_CALL(printer_, testPrint("'header2' 'foo'")); EXPECT_CALL(printer_, testPrint("'hello' 'WORLD'")); EXPECT_CALL(printer_, testPrint("'header2' 'foo'")); + EXPECT_CALL(printer_, testPrint("foo,bar")); start("callMe"); } diff --git a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc index 49455bef6789..89b73bbef081 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc @@ -92,6 +92,33 @@ name: rbac - any: true )EOF"; +const std::string RBAC_CONFIG_HEADER_MATCH_CONDITION = R"EOF( +name: rbac +typed_config: + "@type": type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC + rules: + policies: + foo: + permissions: + - any: true + principals: + - any: true + condition: + call_expr: + function: _==_ + args: + - select_expr: + operand: + select_expr: + operand: + ident_expr: + name: request + field: headers + field: xxx + - const_expr: + string_value: {} +)EOF"; + using RBACIntegrationTest = HttpProtocolIntegrationTest; INSTANTIATE_TEST_SUITE_P(Protocols, RBACIntegrationTest, @@ -357,5 +384,78 @@ TEST_P(RBACIntegrationTest, LogConnectionAllow) { EXPECT_EQ("200", response->headers().getStatusValue()); } +// Basic CEL match on a header value. +TEST_P(RBACIntegrationTest, HeaderMatchCondition) { + config_helper_.addFilter(fmt::format(RBAC_CONFIG_HEADER_MATCH_CONDITION, "yyy")); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + }, + 1024); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); +} + +// CEL match on a header value in which the header is a duplicate. Verifies we handle string +// copying correctly inside the CEL expression. +TEST_P(RBACIntegrationTest, HeaderMatchConditionDuplicateHeaderNoMatch) { + config_helper_.addFilter(fmt::format(RBAC_CONFIG_HEADER_MATCH_CONDITION, "yyy")); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + {"xxx", "zzz"}, + }, + 1024); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("403", response->headers().getStatusValue()); +} + +// CEL match on a header value in which the header is a duplicate. Verifies we handle string +// copying correctly inside the CEL expression. +TEST_P(RBACIntegrationTest, HeaderMatchConditionDuplicateHeaderMatch) { + config_helper_.addFilter(fmt::format(RBAC_CONFIG_HEADER_MATCH_CONDITION, "yyy,zzz")); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto response = codec_client_->makeRequestWithBody( + Http::TestRequestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + {"xxx", "zzz"}, + }, + 1024); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); +} + } // namespace } // namespace Envoy diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 20a20246ef82..309c5e3b9993 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -937,6 +937,9 @@ template class TestHeaderMapImplBase : public Inte const HeaderEntry* get(const LowerCaseString& key) const override { return header_map_->get(key); } + HeaderMap::GetResult getAll(const LowerCaseString& key) const override { + return header_map_->getAll(key); + } void iterate(HeaderMap::ConstIterateCb cb) const override { header_map_->iterate(cb); } void iterateReverse(HeaderMap::ConstIterateCb cb) const override { header_map_->iterateReverse(cb);