diff --git a/bazel/external/quiche.BUILD b/bazel/external/quiche.BUILD index a7cd337706cea..32ee289c4a1af 100644 --- a/bazel/external/quiche.BUILD +++ b/bazel/external/quiche.BUILD @@ -59,6 +59,10 @@ quiche_copts = select({ "-Wno-unused-parameter", "-Wno-unused-function", "-Wno-type-limits", + "-Wno-unused-function", + "-Wno-unknown-warning-option", + "-Wno-deprecated-copy", + "-Wno-range-loop-construct", # quic_inlined_frame.h uses offsetof() to optimize memory usage in frames. "-Wno-invalid-offsetof", "-Wno-type-limits", diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 73bd4a5d574e2..10fb10467371c 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -1,18 +1,25 @@ Version history --------------- -1.12.3 (Pending) -========================== -* buffer: force copy when appending small slices to OwnedImpl buffer to avoid fragmentation. -* listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. -* sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. -* http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`. -1.12.5 (Pending) +1.12.7 (pending) ================ -* http: the :ref:`stream_idle_timeout ` - now also defends against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client. -* listener: add runtime support for `per-listener limits ` on active/accepted connections. -* overload management: add runtime support for :ref:`global limits ` on active/accepted connections. +* 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. + 1.12.6 (July 7, 2020) ===================== * tls: fixed a bug where wilcard matching for "\*.foo.com" also matched domains of the form "a.b.foo.com". This behavior can be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_wildcard_matching` to false. diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index 68a28605c2cdd..065385d106719 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -86,6 +86,9 @@ envoy_cc_library( envoy_cc_library( name = "header_map_interface", hdrs = ["header_map.h"], + external_deps = [ + "abseil_inlined_vector", + ], deps = [ "//source/common/common:assert_lib", "//source/common/common:hash_lib", diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 0fa45cdfd5ea0..34f9d79bfdb15 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -15,6 +15,7 @@ #include "common/common/hash.h" #include "common/common/macros.h" +#include "absl/container/inlined_vector.h" #include "absl/strings/string_view.h" namespace Envoy { @@ -129,6 +130,12 @@ class HeaderString { */ char* buffer() { return buffer_.dynamic_; } + /** + * Trim trailing whitespaces from the HeaderString. + * v1.12 supports both Inline and Dynamic, but not Reference type. + */ + void rtrim(); + /** * Get an absl::string_view. It will NOT be NUL terminated! * @@ -501,6 +508,31 @@ class HeaderMap { virtual const HeaderEntry* get(const LowerCaseString& key) const PURE; virtual HeaderEntry* get(const LowerCaseString& key) 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)) {} + + 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 2466d971fafa6..a9adbc8161b91 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -236,6 +236,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/async_client_impl.h b/source/common/http/async_client_impl.h index 5f45456045692..57f98a9d9e8fb 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -144,9 +144,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, } absl::optional maxInterval() const override { return absl::nullopt; } - const std::vector retriable_status_codes_; - const std::vector retriable_headers_; - const std::vector retriable_request_headers_; + const std::vector retriable_status_codes_{}; + const std::vector retriable_headers_{}; + const std::vector retriable_request_headers_{}; }; struct NullShadowPolicy : public Router::ShadowPolicy { diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index f7af625ee7f31..c3c87535d47b6 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -150,6 +150,15 @@ void HeaderString::append(const char* data, uint32_t size) { string_length_ += size; } +void HeaderString::rtrim() { + ASSERT(type() == Type::Inline || type() == Type::Dynamic); + absl::string_view original = getStringView(); + absl::string_view rtrimmed = StringUtil::rtrim(original); + if (original.size() != rtrimmed.size()) { + string_length_ = rtrimmed.size(); + } +} + void HeaderString::clear() { switch (type_) { case Type::Reference: { @@ -496,13 +505,12 @@ uint64_t HeaderMapImpl::byteSizeInternal() const { } const HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) const { - for (const HeaderEntryImpl& header : headers_) { - if (header.key() == key.get().c_str()) { - return &header; - } - } + const auto result = getAll(key); + return result.empty() ? nullptr : result[0]; +} - return nullptr; +HeaderMap::GetResult HeaderMapImpl::getAll(const LowerCaseString& key) const { + return HeaderMap::GetResult(const_cast(this)->getExisting(key)); } HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) { @@ -512,10 +520,38 @@ HeaderEntry* HeaderMapImpl::get(const LowerCaseString& key) { return &header; } } - return nullptr; } +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; + + EntryCb cb = ConstSingleton::get().find(key.get()); + if (cb) { + StaticLookupResponse ref_lookup_response = cb(*this); + if (*ref_lookup_response.entry_) { + ret.push_back(*ref_lookup_response.entry_); + } + return ret; + } + // If the requested header is not an O(1) header we do a full scan. Doing the trie lookup is + // wasteful in the miss case, but is present for code consistency with other functions that do + // similar things. + // TODO(mattklein123): The full scan here and in remove() are the biggest issues with this + // implementation for certain use cases. We can either replace this with a totally different + // 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()) { + ret.push_back(&header); + } + } + return ret; +} + void HeaderMapImpl::iterate(ConstIterateCb cb, void* context) const { for (const HeaderEntryImpl& header : headers_) { if (cb(header, context) == HeaderMap::Iterate::Break) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 82624331f5a9c..47ca6279be2eb 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -85,6 +85,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { uint64_t byteSizeInternal() const override; const HeaderEntry* get(const LowerCaseString& key) const override; HeaderEntry* get(const LowerCaseString& key) override; + HeaderMap::GetResult getAll(const LowerCaseString& key) const override; void iterate(ConstIterateCb cb, void* context) const override; void iterateReverse(ConstIterateCb cb, void* context) const override; Lookup lookup(const LowerCaseString& key, const HeaderEntry** entry) const override; @@ -203,6 +204,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key); HeaderEntryImpl& maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key, HeaderString&& value); + HeaderMap::NonConstGetResult getExisting(const LowerCaseString& key); HeaderEntryImpl* getExistingInline(absl::string_view key); void removeInline(HeaderEntryImpl** entry); diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 76645b77d5d6c..7b5853ce044ab 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -4,8 +4,10 @@ #include "common/common/utility.h" #include "common/http/header_map_impl.h" #include "common/protobuf/utility.h" +#include "common/runtime/runtime_features.h" #include "absl/strings/match.h" +#include "absl/strings/str_join.h" #include "nghttp2/nghttp2.h" namespace Envoy { @@ -92,36 +94,65 @@ 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; default: NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 3b7c5fa0729f4..932d4dd9c3d3e 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -32,6 +32,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/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index cf718cec06b46..7b310ecba98dd 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -408,6 +408,11 @@ void ConnectionImpl::completeLastHeader() { checkHeaderNameForUnderscores(); if (!current_header_field_.empty()) { toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size()); + // Strip trailing whitespace of the current header value if any. Leading whitespace was trimmed + // in ConnectionImpl::onHeaderValue. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 + current_header_value_.rtrim(); + current_header_map_->addViaMove(std::move(current_header_field_), std::move(current_header_value_)); } @@ -518,9 +523,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { return; } - // Work around a bug in http_parser where trailing whitespace is not trimmed - // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 - const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); + absl::string_view header_value{data, length}; if (strict_header_validation_) { if (!Http::HeaderUtility::headerValueIsValid(header_value)) { @@ -538,6 +541,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } header_parsing_state_ = HeaderParsingState::Value; + if (current_header_value_.empty()) { + // Strip leading whitespace if the current header value input contains the first bytes of the + // encoded header value. Trailing whitespace is stripped once the full header value is known in + // ConnectionImpl::completeLastHeader. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 . + header_value = StringUtil::ltrim(header_value); + } current_header_value_.append(header_value.data(), header_value.length()); checkMaxHeadersSize(); diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index 618a0954bfd17..8a2978433825d 100644 --- a/source/common/runtime/BUILD +++ b/source/common/runtime/BUILD @@ -9,13 +9,32 @@ load( envoy_package() envoy_cc_library( - name = "runtime_lib", + name = "runtime_features_lib", srcs = [ "runtime_features.cc", - "runtime_impl.cc", ], hdrs = [ "runtime_features.h", + ], + deps = [ + "//include/envoy/runtime:runtime_interface", + "//source/common/common:assert_lib", + "//source/common/common:hash_lib", + "//source/common/protobuf:message_validator_lib", + "//source/common/protobuf:utility_lib", + "//source/common/singleton:const_singleton", + "@envoy_api//envoy/api/v2/core:pkg_cc_proto", + "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v2:pkg_cc_proto", + ], +) + +envoy_cc_library( + name = "runtime_lib", + srcs = [ + "runtime_impl.cc", + ], + hdrs = [ "runtime_impl.h", ], external_deps = ["ssl"], @@ -37,6 +56,7 @@ envoy_cc_library( "//source/common/init:target_lib", "//source/common/protobuf:message_validator_lib", "//source/common/protobuf:utility_lib", + "//source/common/runtime:runtime_features_lib", "@envoy_api//envoy/api/v2/core:pkg_cc_proto", "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v2:pkg_cc_proto", diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 4475f88c7bf4a..76b55da27661c 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -1,8 +1,31 @@ #include "common/runtime/runtime_features.h" +#include "common/common/assert.h" + namespace Envoy { namespace Runtime { +bool runtimeFeatureEnabled(absl::string_view feature) { + if (Runtime::LoaderSingleton::getExisting()) { + return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->runtimeFeatureEnabled( + feature); + } + ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn, + "Unable to use runtime singleton for feature {}", feature); + return RuntimeFeaturesDefaults::get().enabledByDefault(feature); +} + +uint64_t getInteger(absl::string_view feature, uint64_t default_value) { + ASSERT(absl::StartsWith(feature, "envoy.")); + if (Runtime::LoaderSingleton::getExisting()) { + return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger( + std::string(feature), default_value); + } + ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn, + "Unable to use runtime singleton for feature {}", feature); + return default_value; +} + // Add additional features here to enable the new code paths by default. // // Per documentation in CONTRIBUTING.md is expected that new high risk code paths be guarded @@ -32,6 +55,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.strict_header_validation", "envoy.reloadable_features.strict_authority_validation", "envoy.reloadable_features.fix_wildcard_matching", + "envoy.reloadable_features.http_match_on_all_headers", }; // This is a list of configuration fields which are disallowed by default in Envoy diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index 2f7cce929f680..be717413ba086 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -7,12 +7,17 @@ #include "common/protobuf/utility.h" #include "common/singleton/const_singleton.h" +#include "common/singleton/threadsafe_singleton.h" #include "absl/container/flat_hash_set.h" +#include "absl/strings/match.h" namespace Envoy { namespace Runtime { +bool runtimeFeatureEnabled(absl::string_view feature); +uint64_t getInteger(absl::string_view feature, uint64_t default_value); + class RuntimeFeatures { public: RuntimeFeatures(); diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index c863bc62dac8c..be134c7454064 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -25,28 +25,6 @@ namespace Envoy { namespace Runtime { -bool runtimeFeatureEnabled(absl::string_view feature) { - ASSERT(absl::StartsWith(feature, "envoy.reloadable_features")); - if (Runtime::LoaderSingleton::getExisting()) { - return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->runtimeFeatureEnabled( - feature); - } - ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn, - "Unable to use runtime singleton for feature {}", feature); - return RuntimeFeaturesDefaults::get().enabledByDefault(feature); -} - -uint64_t getInteger(absl::string_view feature, uint64_t default_value) { - ASSERT(absl::StartsWith(feature, "envoy.")); - if (Runtime::LoaderSingleton::getExisting()) { - return Runtime::LoaderSingleton::getExisting()->threadsafeSnapshot()->getInteger( - std::string(feature), default_value); - } - ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn, - "Unable to use runtime singleton for feature {}", feature); - return default_value; -} - const size_t RandomGeneratorImpl::UUID_LENGTH = 36; uint64_t RandomGeneratorImpl::random() { diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 1ee1fe97c66e0..246cacdcb2393 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -23,6 +23,7 @@ #include "common/common/logger.h" #include "common/common/thread.h" #include "common/init/target_impl.h" +#include "common/runtime/runtime_features.h" #include "common/singleton/threadsafe_singleton.h" #include "spdlog/spdlog.h" @@ -30,9 +31,6 @@ namespace Envoy { namespace Runtime { -bool runtimeFeatureEnabled(absl::string_view feature); -uint64_t getInteger(absl::string_view feature, uint64_t default_value); - using RuntimeSingleton = ThreadSafeSingleton; /** diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index 5e71dd819870d..593aed1372f6c 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -1151,10 +1151,10 @@ WasmResult Context::getProperty(absl::string_view path, std::string* result) { Protobuf::Arena::Create(&arena, info->filterState())); } else if (part == "request") { value = CelValue::CreateMap(Protobuf::Arena::Create( - &arena, request_headers, *info)); + &arena, arena, request_headers, *info)); } else if (part == "response") { value = CelValue::CreateMap(Protobuf::Arena::Create( - &arena, response_headers, response_trailers, *info)); + &arena, arena, response_headers, response_trailers, *info)); } else if (part == "connection") { value = CelValue::CreateMap( Protobuf::Arena::Create(&arena, *info)); diff --git a/source/extensions/filters/common/expr/BUILD b/source/extensions/filters/common/expr/BUILD index 01906b7d2b33c..7d6867b392be4 100644 --- a/source/extensions/filters/common/expr/BUILD +++ b/source/extensions/filters/common/expr/BUILD @@ -1,11 +1,11 @@ -licenses(["notice"]) # Apache 2 - load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package", ) +licenses(["notice"]) # Apache 2 + envoy_package() envoy_cc_library( @@ -28,6 +28,9 @@ envoy_cc_library( srcs = ["context.cc"], hdrs = ["context.h"], deps = [ + "//source/common/grpc:common_lib", + "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/http:utility_lib", "//source/common/stream_info:utility_lib", "@com_google_cel_cpp//eval/public:cel_value", diff --git a/source/extensions/filters/common/expr/context.cc b/source/extensions/filters/common/expr/context.cc index befd2de5af239..a2fbaa7584d79 100644 --- a/source/extensions/filters/common/expr/context.cc +++ b/source/extensions/filters/common/expr/context.cc @@ -18,6 +18,19 @@ absl::optional convertHeaderEntry(const Http::HeaderEntry* header) { return CelValue::CreateString(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::CreateString(result.result().value()); + } +} + absl::optional extractSslInfo(const Ssl::ConnectionInfo& ssl_info, absl::string_view value) { if (value == TLSVersion) { @@ -52,8 +65,9 @@ absl::optional HeadersWrapper::operator[](CelValue key) const { if (value_ == nullptr || !key.IsString()) { return {}; } - auto out = value_->get(Http::LowerCaseString(std::string(key.StringOrDie().value()))); - return convertHeaderEntry(out); + return convertHeaderEntry( + arena_, Http::HeaderUtility::getAllOfHeaderAsString( + *value_, Http::LowerCaseString(std::string(key.StringOrDie().value())))); } absl::optional RequestWrapper::operator[](CelValue key) const { diff --git a/source/extensions/filters/common/expr/context.h b/source/extensions/filters/common/expr/context.h index 77bf9cab802c4..b96c83e1f9ad4 100644 --- a/source/extensions/filters/common/expr/context.h +++ b/source/extensions/filters/common/expr/context.h @@ -2,6 +2,8 @@ #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" @@ -66,7 +68,8 @@ class RequestWrapper; class HeadersWrapper : public google::api::expr::runtime::CelMap { public: - HeadersWrapper(const Http::HeaderMap* value) : value_(value) {} + HeadersWrapper(Protobuf::Arena& arena, const Http::HeaderMap* value) + : arena_(arena), value_(value) {} absl::optional operator[](CelValue key) const override; int size() const override { return value_ == nullptr ? 0 : value_->size(); } bool empty() const override { return value_ == nullptr ? true : value_->empty(); } @@ -77,6 +80,7 @@ class HeadersWrapper : public google::api::expr::runtime::CelMap { private: friend class RequestWrapper; friend class ResponseWrapper; + Protobuf::Arena& arena_; const Http::HeaderMap* value_; }; @@ -91,8 +95,9 @@ class BaseWrapper : public google::api::expr::runtime::CelMap { class RequestWrapper : public BaseWrapper { public: - RequestWrapper(const Http::HeaderMap* headers, const StreamInfo::StreamInfo& info) - : headers_(headers), info_(info) {} + RequestWrapper(Protobuf::Arena& arena, const Http::HeaderMap* headers, + const StreamInfo::StreamInfo& info) + : headers_(arena, headers), info_(info) {} absl::optional operator[](CelValue key) const override; private: @@ -102,9 +107,9 @@ class RequestWrapper : public BaseWrapper { class ResponseWrapper : public BaseWrapper { public: - ResponseWrapper(const Http::HeaderMap* headers, const Http::HeaderMap* trailers, - const StreamInfo::StreamInfo& info) - : headers_(headers), trailers_(trailers), info_(info) {} + ResponseWrapper(Protobuf::Arena& arena, const Http::HeaderMap* headers, + const Http::HeaderMap* 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 4df6d9b52beec..dc683e698ef51 100644 --- a/source/extensions/filters/common/expr/evaluator.cc +++ b/source/extensions/filters/common/expr/evaluator.cc @@ -48,27 +48,27 @@ ExpressionPtr createExpression(Builder& builder, const google::api::expr::v1alph return std::move(cel_expression_status.ValueOrDie()); } -absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena, +absl::optional evaluate(const Expression& expr, Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers, const Http::HeaderMap* response_trailers) { google::api::expr::runtime::Activation activation; - const RequestWrapper request(request_headers, info); - const ResponseWrapper response(response_headers, response_trailers, info); + const RequestWrapper request(arena, request_headers, info); + const ResponseWrapper response(arena, response_headers, response_trailers, info); const ConnectionWrapper connection(info); const UpstreamWrapper upstream(info); const PeerWrapper source(info, false); const PeerWrapper destination(info, true); activation.InsertValue(Request, CelValue::CreateMap(&request)); activation.InsertValue(Response, CelValue::CreateMap(&response)); - activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), arena)); + activation.InsertValue(Metadata, CelValue::CreateMessage(&info.dynamicMetadata(), &arena)); activation.InsertValue(Connection, CelValue::CreateMap(&connection)); activation.InsertValue(Upstream, CelValue::CreateMap(&upstream)); activation.InsertValue(Source, CelValue::CreateMap(&source)); activation.InsertValue(Destination, CelValue::CreateMap(&destination)); - auto eval_status = expr.Evaluate(activation, arena); + auto eval_status = expr.Evaluate(activation, &arena); if (!eval_status.ok()) { return {}; } @@ -79,7 +79,7 @@ absl::optional evaluate(const Expression& expr, Protobuf::Arena* arena bool matches(const Expression& expr, const StreamInfo::StreamInfo& info, const Http::HeaderMap& 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 92ccea420d216..95a0968157515 100644 --- a/source/extensions/filters/common/expr/evaluator.h +++ b/source/extensions/filters/common/expr/evaluator.h @@ -16,11 +16,20 @@ namespace Filters { namespace Common { namespace Expr { +using Activation = google::api::expr::runtime::Activation; +using ActivationPtr = std::unique_ptr; using Builder = google::api::expr::runtime::CelExpressionBuilder; using BuilderPtr = std::unique_ptr; using Expression = google::api::expr::runtime::CelExpression; 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(Protobuf::Arena& arena, const StreamInfo::StreamInfo& info, + const Http::HeaderMap* request_headers, + const Http::HeaderMap* response_headers, + const Http::HeaderMap* response_trailers); + // Creates an expression builder. The optional arena is used to enable constant folding // for intermediate evaluation results. // Throws an exception if fails to construct an expression builder. @@ -32,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::HeaderMap* request_headers, const Http::HeaderMap* response_headers, diff --git a/source/extensions/filters/http/header_to_metadata/BUILD b/source/extensions/filters/http/header_to_metadata/BUILD index 6cce872d79e43..aafeb6beddf60 100644 --- a/source/extensions/filters/http/header_to_metadata/BUILD +++ b/source/extensions/filters/http/header_to_metadata/BUILD @@ -18,6 +18,8 @@ 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/config/filter/http/header_to_metadata/v2: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 6c4379e7a9d80..3f64fdcc8c7d5 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 @@ -2,6 +2,8 @@ #include "common/common/base64.h" #include "common/config/well_known_names.h" +#include "common/http/header_utility.h" +#include "common/http/utility.h" #include "common/protobuf/protobuf.h" #include "extensions/filters/http/well_known_names.h" @@ -155,13 +157,12 @@ void HeaderToMetadataFilter::writeHeaderToMetadata(Http::HeaderMap& headers, for (const auto& rulePair : rules) { const auto& header = rulePair.first; const auto& rule = rulePair.second; - const Http::HeaderEntry* header_entry = headers.get(header); + const auto header_value = Http::HeaderUtility::getAllOfHeaderAsString(headers, header); - if (header_entry != nullptr && rule.has_on_header_present()) { + if (header_value.result().has_value() && rule.has_on_header_present()) { const auto& keyval = rule.on_header_present(); - absl::string_view value = keyval.value().empty() ? header_entry->value().getStringView() + absl::string_view value = keyval.value().empty() ? header_value.result().value() : absl::string_view(keyval.value()); - if (!value.empty()) { const auto& nspace = decideNamespace(keyval.metadata_namespace()); addMetadata(structs_by_namespace, nspace, keyval.key(), value, keyval.type(), @@ -173,7 +174,8 @@ void HeaderToMetadataFilter::writeHeaderToMetadata(Http::HeaderMap& headers, if (rule.remove()) { headers.remove(header); } - } else if (rule.has_on_header_missing()) { + } + if (!header_value.result().has_value() && rule.has_on_header_missing()) { // Add metadata for the header missing case. const auto& keyval = rule.on_header_missing(); diff --git a/source/extensions/filters/http/jwt_authn/BUILD b/source/extensions/filters/http/jwt_authn/BUILD index e7bbde282f664..f509dec0722a4 100644 --- a/source/extensions/filters/http/jwt_authn/BUILD +++ b/source/extensions/filters/http/jwt_authn/BUILD @@ -13,6 +13,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/config/filter/http/jwt_authn/v2alpha:pkg_cc_proto", ], diff --git a/source/extensions/filters/http/jwt_authn/extractor.cc b/source/extensions/filters/http/jwt_authn/extractor.cc index c6950a2b31c31..a1bbdba06adc2 100644 --- a/source/extensions/filters/http/jwt_authn/extractor.cc +++ b/source/extensions/filters/http/jwt_authn/extractor.cc @@ -3,6 +3,7 @@ #include #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" @@ -180,9 +181,11 @@ std::vector ExtractorImpl::extract(const Http::HeaderMap& h // Check header locations first for (const auto& location_it : header_locations_) { const auto& location_spec = location_it.second; - const Http::HeaderEntry* entry = headers.get(location_spec->header_); - if (entry) { - auto value_str = entry->value().getStringView(); + ENVOY_LOG_MISC(debug, "extract {}", location_it.first); + 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 dacdd39b21988..3910f4ec09430 100644 --- a/source/extensions/filters/http/lua/BUILD +++ b/source/extensions/filters/http/lua/BUILD @@ -39,6 +39,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 4874a716cec99..e2db862c269df 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" @@ -45,10 +46,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/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc index c9559f1652bec..8bc9228663cb0 100644 --- a/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc +++ b/source/extensions/filters/network/dubbo_proxy/hessian_utils.cc @@ -27,7 +27,7 @@ typename std::enable_if::value, T>::type leftShift(T left, uin inline void addByte(Buffer::Instance& buffer, const uint8_t value) { buffer.add(&value, 1); } void addSeq(Buffer::Instance& buffer, const std::initializer_list& values) { - for (const int8_t& value : values) { + for (const int8_t value : values) { buffer.add(&value, 1); } } diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index a0e32b90a5ea5..ac4171deffe68 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -50,7 +50,7 @@ void HystrixSink::addHistogramToStream(const QuantileLatencyMap& latency_map, ab // TODO: Consider if we better use join here ss << ", \"" << key << "\": {"; bool is_first = true; - for (const std::pair& element : latency_map) { + for (const std::pair& element : latency_map) { const std::string quantile = fmt::sprintf("%g", element.first * 100); HystrixSink::addDoubleToStream(quantile, element.second, ss, is_first); is_first = false; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index f5df8e612ce88..2352795089d29 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -246,6 +246,7 @@ envoy_cc_test( deps = [ "//source/common/http:header_map_lib", "//source/common/http:header_utility_lib", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) @@ -282,6 +283,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", ], ) diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 39d594324a8be..ef0f0e0c73f5d 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -139,7 +139,6 @@ TEST_F(AsyncClientImplTest, Basic) { response_decoder_ = &decoder; return nullptr; })); - TestHeaderMapImpl copy(message_->headers()); copy.addCopy("x-envoy-internal", "true"); copy.addCopy("x-forwarded-for", "127.0.0.1"); @@ -149,7 +148,7 @@ TEST_F(AsyncClientImplTest, Basic) { EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); expectSuccess(200); - client_.send(std::move(message_), callbacks_, AsyncClient::RequestOptions()); + client_.send(std::move(message_), callbacks_, AsyncClient::RequestOptions().setSendXff(true)); HeaderMapPtr response_headers(new TestHeaderMapImpl{{":status", "200"}}); response_decoder_->decodeHeaders(std::move(response_headers), false); diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 905cbe8f9894f..3d4bc7d54b67e 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -5,6 +5,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" @@ -105,6 +106,37 @@ TEST(HeaderStringTest, All) { EXPECT_EQ("HELLO", string.getStringView()); } + // Inline rtrim removes trailing whitespace only. + { + // This header string is short enough to fit into Inline type. + const std::string data_with_leading_lws = " \t\f\v data"; + const std::string data_with_leading_and_trailing_lws = data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Inline); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(data_with_leading_lws, string.getStringView()); + } + + // Dynamic rtrim removes trailing whitespace only. + { + // Making this string longer than Inline can fit. + const std::string padding_data_with_leading_lws = " \t\f\v data" + std::string(128, 'a'); + const std::string data_with_leading_and_trailing_lws = + padding_data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(padding_data_with_leading_lws, string.getStringView()); + } + // Static clear() does nothing. { std::string static_string("HELLO"); diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index cdbc796a0315c..5fa23aad0f0f1 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -7,6 +7,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" @@ -20,6 +21,53 @@ envoy::api::v2::route::HeaderMatcher parseHeaderMatcherFromYaml(const std::strin return header_matcher; } +TEST(GetAllOfHeaderAsStringTest, All) { + const LowerCaseString test_header("test"); + { + TestHeaderMapImpl headers; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_FALSE(ret.result().has_value()); + EXPECT_TRUE(ret.backingString().empty()); + } + { + TestHeaderMapImpl headers{{"test", "foo"}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ("foo", ret.result().value()); + EXPECT_TRUE(ret.backingString().empty()); + } + { + TestHeaderMapImpl 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()); + } + { + TestHeaderMapImpl headers{{"test", ""}, {"test", "bar"}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ(",bar", ret.result().value()); + EXPECT_EQ(",bar", ret.backingString()); + } + { + TestHeaderMapImpl headers{{"test", ""}, {"test", ""}}; + const auto ret = HeaderUtility::getAllOfHeaderAsString(headers, test_header); + EXPECT_EQ(",", ret.result().value()); + EXPECT_EQ(",", ret.backingString()); + } + { + TestHeaderMapImpl 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 @@ -170,8 +218,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/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index ee3256290f822..7f29b42839181 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -41,6 +41,16 @@ std::string createHeaderFragment(int num_headers) { } return headers; } + +Buffer::OwnedImpl createBufferWithNByteSlices(absl::string_view input, size_t max_slice_size) { + Buffer::OwnedImpl buffer; + for (size_t offset = 0; offset < input.size(); offset += max_slice_size) { + buffer.appendSliceForTest(input.substr(offset, max_slice_size)); + } + // Verify that the buffer contains the right number of slices. + ASSERT(buffer.getRawSlices(nullptr, 0) == (input.size() + max_slice_size - 1) / max_slice_size); + return buffer; +} } // namespace class Http1ServerConnectionImplTest : public testing::Test { @@ -66,7 +76,13 @@ class Http1ServerConnectionImplTest : public testing::Test { // Then send a response just to clean up. void sendAndValidateRequestAndSendResponse(absl::string_view raw_request, const TestHeaderMapImpl& expected_request_headers) { - NiceMock decoder; + Buffer::OwnedImpl buffer(raw_request); + sendAndValidateRequestAndSendResponse(buffer, expected_request_headers); + } + + void sendAndValidateRequestAndSendResponse(Buffer::Instance& buffer, + const TestHeaderMapImpl& expected_request_headers) { + NiceMock decoder; Http::StreamEncoder* response_encoder = nullptr; EXPECT_CALL(callbacks_, newStream(_, _)) .Times(1) @@ -74,8 +90,7 @@ class Http1ServerConnectionImplTest : public testing::Test { response_encoder = &encoder; return decoder; })); - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1); - Buffer::OwnedImpl buffer(raw_request); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); @@ -206,6 +221,42 @@ TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { "GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270. Linear whitespace at the +// beginning and end of a header value should be stripped. Whitespace in the middle should be +// preserved. +TEST_F(Http1ServerConnectionImplTest, InnerLWSIsPreserved) { + initialize(); + + // Header with many spaces surrounded by non-whitespace characters to ensure that dispatching is + // split across multiple dispatch calls. The threshold used here comes from Envoy preferring 16KB + // reads, but the important part is that the header value is split such that the pieces have + // leading and trailing whitespace characters. + const std::string header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + TestHeaderMapImpl expected_headers{{":authority", "host"}, + {":path", "/"}, + {":method", "GET"}, + {"header_field", header_value_with_inner_lws}}; + + { + // Regression test spaces in the middle are preserved + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + "\r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } + + { + // Regression test spaces before and after are removed + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + + " \r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } +} + TEST_F(Http1ServerConnectionImplTest, Http10) { initialize(); diff --git a/test/extensions/common/wasm/BUILD b/test/extensions/common/wasm/BUILD index 490402dc17848..729f73e60ea8a 100644 --- a/test/extensions/common/wasm/BUILD +++ b/test/extensions/common/wasm/BUILD @@ -12,6 +12,9 @@ envoy_package() envoy_cc_test( name = "wasm_vm_test", srcs = ["wasm_vm_test.cc"], + tags = [ + "manual", + ], deps = [ "//source/extensions/common/wasm:wasm_vm_lib", "//test/test_common:utility_lib", diff --git a/test/extensions/filters/common/expr/context_test.cc b/test/extensions/filters/common/expr/context_test.cc index 75971170c0de0..c75fd658059d4 100644 --- a/test/extensions/filters/common/expr/context_test.cc +++ b/test/extensions/filters/common/expr/context_test.cc @@ -23,7 +23,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::CreateString(Referer)]; EXPECT_FALSE(header.has_value()); EXPECT_EQ(0, headers.size()); @@ -33,13 +34,14 @@ TEST(Context, EmptyHeadersAttributes) { TEST(Context, RequestAttributes) { NiceMock info; NiceMock empty_info; - Http::TestHeaderMapImpl 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); + Http::TestHeaderMapImpl 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"}, + {"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". @@ -51,7 +53,6 @@ TEST(Context, RequestAttributes) { // stub methods EXPECT_EQ(0, request.size()); EXPECT_FALSE(request.empty()); - { auto value = request[CelValue::CreateString(Undefined)]; EXPECT_FALSE(value.has_value()); @@ -135,7 +136,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()); } { @@ -159,12 +160,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::CreateString(Referer)]; EXPECT_TRUE(header.has_value()); ASSERT_TRUE(header.value().IsString()); EXPECT_EQ("dogs.com", header.value().StringOrDie().value()); + + auto header2 = map[CelValue::CreateString("double-header")]; + EXPECT_TRUE(header2.has_value()); + ASSERT_TRUE(header2.value().IsString()); + EXPECT_EQ("foo,bar", header2.value().StringOrDie().value()); } { @@ -182,7 +188,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)); @@ -208,8 +215,10 @@ TEST(Context, ResponseAttributes) { const std::string trailer_name = "test-trailer"; Http::TestHeaderMapImpl header_map{{header_name, "a"}}; Http::TestHeaderMapImpl trailer_map{{trailer_name, "b"}}; - 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)); diff --git a/test/extensions/filters/http/buffer/config_test.cc b/test/extensions/filters/http/buffer/config_test.cc index 572d6b21b9611..29b5044686be2 100644 --- a/test/extensions/filters/http/buffer/config_test.cc +++ b/test/extensions/filters/http/buffer/config_test.cc @@ -77,10 +77,9 @@ TEST(BufferFilterFactoryTest, BufferFilterEmptyProto) { TEST(BufferFilterFactoryTest, BufferFilterEmptyRouteProto) { BufferFilterFactory factory; EXPECT_NO_THROW({ - envoy::config::filter::http::buffer::v2::BufferPerRoute* config = - dynamic_cast( - factory.createEmptyRouteConfigProto().get()); - EXPECT_NE(nullptr, config); + auto config = factory.createEmptyRouteConfigProto(); + EXPECT_NE(nullptr, + dynamic_cast(config.get())); }); } diff --git a/test/extensions/filters/http/dynamo/config_test.cc b/test/extensions/filters/http/dynamo/config_test.cc index fb8b7b3824a2d..7657065f44044 100644 --- a/test/extensions/filters/http/dynamo/config_test.cc +++ b/test/extensions/filters/http/dynamo/config_test.cc @@ -16,7 +16,7 @@ namespace { TEST(DynamoFilterConfigTest, DynamoFilter) { NiceMock context; DynamoFilterConfig factory; - const auto proto_config = factory.createEmptyConfigProto().get(); + const auto proto_config = factory.createEmptyConfigProto(); Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(*proto_config, "stats", context); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamFilter(_)); 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 9674e22e8217a..bfc0be2bf4e2f 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 @@ -98,6 +98,24 @@ TEST_F(HeaderToMetadataTest, BasicRequestTest) { filter_->onDestroy(); } +// Verify concatenation works. +TEST_F(HeaderToMetadataTest, BasicRequestDoubleHeadersTest) { + initializeFilter(request_config_yaml); + Http::TestHeaderMapImpl 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::TestHeaderMapImpl incoming_trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(incoming_trailers)); + filter_->onDestroy(); +} + /** * X-version not set, the on missing value should be set. */ diff --git a/test/extensions/filters/http/jwt_authn/extractor_test.cc b/test/extensions/filters/http/jwt_authn/extractor_test.cc index 944e06bce06a8..737d53719bfce 100644 --- a/test/extensions/filters/http/jwt_authn/extractor_test.cc +++ b/test/extensions/filters/http/jwt_authn/extractor_test.cc @@ -164,6 +164,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 = TestHeaderMapImpl{{"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 2ce294f854d19..e842a22b8a5dc 100644 --- a/test/extensions/filters/http/lua/wrappers_test.cc +++ b/test/extensions/filters/http/lua/wrappers_test.cc @@ -42,6 +42,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"}; @@ -56,6 +60,7 @@ TEST_F(LuaHeaderMapWrapperTest, Methods) { EXPECT_CALL(*this, testPrint("'header2' 'foo'")); EXPECT_CALL(*this, testPrint("'hello' 'WORLD'")); EXPECT_CALL(*this, testPrint("'header2' 'foo'")); + EXPECT_CALL(*this, testPrint("foo,bar")); start("callMe"); } diff --git a/test/extensions/filters/http/rbac/config_test.cc b/test/extensions/filters/http/rbac/config_test.cc index 7ab2a39ffe0af..2edd85b153c26 100644 --- a/test/extensions/filters/http/rbac/config_test.cc +++ b/test/extensions/filters/http/rbac/config_test.cc @@ -32,16 +32,19 @@ TEST(RoleBasedAccessControlFilterConfigFactoryTest, ValidProto) { TEST(RoleBasedAccessControlFilterConfigFactoryTest, EmptyProto) { RoleBasedAccessControlFilterConfigFactory factory; - auto* config = dynamic_cast( - factory.createEmptyConfigProto().get()); - EXPECT_NE(nullptr, config); + auto config = + + factory.createEmptyConfigProto(); + + EXPECT_NE(nullptr, dynamic_cast(config.get())); } TEST(RoleBasedAccessControlFilterConfigFactoryTest, EmptyRouteProto) { RoleBasedAccessControlFilterConfigFactory factory; - auto* config = dynamic_cast( - factory.createEmptyRouteConfigProto().get()); - EXPECT_NE(nullptr, config); + auto config = factory.createEmptyRouteConfigProto(); + + EXPECT_NE(nullptr, + dynamic_cast(config.get())); } TEST(RoleBasedAccessControlFilterConfigFactoryTest, RouteSpecificConfig) { 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 1d44ceecf98e0..15733438cfc54 100644 --- a/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc +++ b/test/extensions/filters/http/rbac/rbac_filter_integration_test.cc @@ -63,6 +63,33 @@ name: envoy.filters.http.rbac - any: true )EOF"; +const std::string RBAC_CONFIG_HEADER_MATCH_CONDITION = R"EOF( +name: envoy.filters.http.rbac +typed_config: + "@type": type.googleapis.com/envoy.config.filter.http.rbac.v2.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, @@ -277,5 +304,78 @@ TEST_P(RBACIntegrationTest, PathIgnoreCase) { } } +// 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::TestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + }, + 1024); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); +} + +// 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::TestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + {"xxx", "zzz"}, + }, + 1024); + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("403", response->headers().Status()->value().getStringView()); +} + +// 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::TestHeaderMapImpl{ + {":method", "POST"}, + {":path", "/path"}, + {":scheme", "http"}, + {":authority", "host"}, + {"xxx", "yyy"}, + {"xxx", "zzz"}, + }, + 1024); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); + + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); +} + } // namespace } // namespace Envoy diff --git a/test/extensions/filters/network/rbac/config_test.cc b/test/extensions/filters/network/rbac/config_test.cc index 8577982491ab5..a975e9fe75818 100644 --- a/test/extensions/filters/network/rbac/config_test.cc +++ b/test/extensions/filters/network/rbac/config_test.cc @@ -66,9 +66,8 @@ TEST_F(RoleBasedAccessControlNetworkFilterConfigFactoryTest, ValidProto) { TEST_F(RoleBasedAccessControlNetworkFilterConfigFactoryTest, EmptyProto) { RoleBasedAccessControlNetworkFilterConfigFactory factory; - auto* config = dynamic_cast( - factory.createEmptyConfigProto().get()); - EXPECT_NE(nullptr, config); + auto config = factory.createEmptyConfigProto(); + EXPECT_NE(nullptr, dynamic_cast(config.get())); } TEST_F(RoleBasedAccessControlNetworkFilterConfigFactoryTest, InvalidPermission) { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d94fe99d50576..012ee6178cb15 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -230,6 +230,39 @@ TEST_P(ProtocolIntegrationTest, DrainClose) { test_server_->drainManager().draining_ = false; } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270 +TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { + // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that + // dispatching is split across 2 dispatch calls. This threshold comes from Envoy preferring 16KB + // reads, which the buffer rounds up to about 20KB when allocating slices in + // Buffer::OwnedImpl::reserve(). + const std::string long_header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"longrequestvalue", long_header_value_with_inner_lws}}); + waitForNextUpstreamRequest(); + EXPECT_EQ(long_header_value_with_inner_lws, upstream_request_->headers() + .get(Http::LowerCaseString("longrequestvalue")) + ->value() + .getStringView()); + upstream_request_->encodeHeaders( + Http::TestHeaderMapImpl{{":status", "200"}, + {"longresponsevalue", long_header_value_with_inner_lws}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ( + long_header_value_with_inner_lws, + response->headers().get(Http::LowerCaseString("longresponsevalue"))->value().getStringView()); +} + TEST_P(ProtocolIntegrationTest, Retry) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 13fa984dcb9c6..1593114c4436d 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -19,6 +19,7 @@ BSON CAS CB CDS +CEL CHACHA CHLO CHLOS