diff --git a/api/envoy/config/filter/accesslog/v2/accesslog.proto b/api/envoy/config/filter/accesslog/v2/accesslog.proto index fffd2251eff3f..4eec60d7442db 100644 --- a/api/envoy/config/filter/accesslog/v2/accesslog.proto +++ b/api/envoy/config/filter/accesslog/v2/accesslog.proto @@ -191,7 +191,8 @@ message ResponseFlagFilter { "RLSE", "DC", "URX", - "SI" + "SI", + "IH" ] }]; } diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index 02115cb81f734..e776756733571 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -11,6 +11,8 @@ import "envoy/config/filter/accesslog/v2/accesslog.proto"; import "google/protobuf/wrappers.proto"; +import "validate/validate.proto"; + // [#protodoc-title: Router] // Router :ref:`configuration overview `. @@ -36,4 +38,30 @@ message Router { // `, other Envoy filters and the HTTP // connection manager may continue to set *x-envoy-* headers. bool suppress_envoy_headers = 4; + + // Specifies a list of HTTP headers to strictly validate. Envoy will reject a + // request and respond with HTTP status 400 if the request contains an invalid + // value for any of the headers listed in this field. Strict header checking + // is only supported for the following headers: + // + // Value must be a ','-delimited list (i.e. no spaces) of supported retry + // policy values: + // + // * :ref:`config_http_filters_router_x-envoy-retry-grpc-on` + // * :ref:`config_http_filters_router_x-envoy-retry-on` + // + // Value must be an integer: + // + // * :ref:`config_http_filters_router_x-envoy-max-retries` + // * :ref:`config_http_filters_router_x-envoy-upstream-rq-timeout-ms` + // * :ref:`config_http_filters_router_x-envoy-upstream-rq-per-try-timeout-ms` + repeated string strict_check_headers = 5 [(validate.rules).repeated .items.string = { + in: [ + "x-envoy-upstream-rq-timeout-ms", + "x-envoy-upstream-rq-per-try-timeout-ms", + "x-envoy-max-retries", + "x-envoy-retry-grpc-on", + "x-envoy-retry-on" + ] + }]; } diff --git a/api/envoy/data/accesslog/v2/accesslog.proto b/api/envoy/data/accesslog/v2/accesslog.proto index 478d7b03b5a01..1396c729f88e5 100644 --- a/api/envoy/data/accesslog/v2/accesslog.proto +++ b/api/envoy/data/accesslog/v2/accesslog.proto @@ -210,6 +210,10 @@ message ResponseFlags { // Indicates that the stream idle timeout was hit, resulting in a downstream 408. bool stream_idle_timeout = 17; + + // Indicates that the request was rejected because an envoy request header failed strict + // validation. + bool invalid_envoy_request_headers = 18; } // Properties of a negotiated TLS connection. diff --git a/docs/root/configuration/access_log.rst b/docs/root/configuration/access_log.rst index 367d27eaf64f1..9350cc5fb5227 100644 --- a/docs/root/configuration/access_log.rst +++ b/docs/root/configuration/access_log.rst @@ -218,6 +218,8 @@ The following command operators are supported: * **RL**: The request was ratelimited locally by the :ref:`HTTP rate limit filter ` in addition to 429 response code. * **UAEX**: The request was denied by the external authorization service. * **RLSE**: The request was rejected because there was an error in rate limit service. + * **IH**: The request was rejected because it set an invalid value for a + :ref:`strictly-checked header ` in addition to 400 response code. * **SI**: Stream idle timeout in addition to 408 response code. %RESPONSE_TX_DURATION% diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index ed32dd6ca384c..5c8a7270d9956 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -7,6 +7,7 @@ Version history * access log: added a new field for route name to file and gRPC access logger. * access log: added a new field for response code details in :ref:`file access logger` and :ref:`gRPC access logger`. * access log: added several new variables for exposing information about the downstream TLS connection to :ref:`file access logger` and :ref:`gRPC access logger`. +* access log: added a new flag for request rejected due to failed strict header check. * admin: the administration interface now includes a :ref:`/ready endpoint ` for easier readiness checks. * admin: extend :ref:`/runtime_modify endpoint ` to support parameters within the request body. * admin: the :ref:`/listener endpoint ` now returns :ref:`listeners.proto` which includes listener names and ports. @@ -59,6 +60,8 @@ Version history * router: added :ref:`RouteAction's auto_host_rewrite_header ` to allow upstream host header substitution with some other header's value * router: added support for UPSTREAM_REMOTE_ADDRESS :ref:`header formatter `. +* router: add ability to reject a request that includes invalid values for + headers configured in :ref:`strict_check_headers ` * runtime: added support for :ref:`flexible layering configuration `. * runtime: added support for statically :ref:`specifying the runtime in the bootstrap configuration diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index fa2a0ffd92ed2..1558503ccfe52 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -61,8 +61,10 @@ enum ResponseFlag { UpstreamRetryLimitExceeded = 0x8000, // Request hit the stream idle timeout, triggering a 408. StreamIdleTimeout = 0x10000, + // Request specified x-envoy-* header values that failed strict header checks. + InvalidEnvoyRequestHeaders = 0x20000, // ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG. - LastFlag = StreamIdleTimeout + LastFlag = InvalidEnvoyRequestHeaders }; /** @@ -95,6 +97,8 @@ struct ResponseCodeDetailValues { const std::string MissingHost = "missing_host_header"; // The request was rejected due to the request headers being larger than the configured limit. const std::string RequestHeadersTooLarge = "request_headers_too_large"; + // The request was rejected due to x-envoy-* headers failing strict header validation. + const std::string InvalidEnvoyRequestHeaders = "request_headers_failed_strict_check"; // The request was rejected due to the Path or :path header field missing. const std::string MissingPath = "missing_path_rejected"; // The request was rejected due to using an absolute path on a route not supporting them. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index f4364032a12c7..a54b9eea5a568 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -36,9 +36,9 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterInfoConstSharedPtr cluster, Runtime::RandomGenerator& random, Router::ShadowWriterPtr&& shadow_writer, Http::Context& http_context) - : cluster_(cluster), - config_("http.async-client.", local_info, stats_store, cm, runtime, random, - std::move(shadow_writer), true, false, false, dispatcher.timeSource(), http_context), + : cluster_(cluster), config_("http.async-client.", local_info, stats_store, cm, runtime, random, + std::move(shadow_writer), true, false, false, {}, + dispatcher.timeSource(), http_context), dispatcher_(dispatcher) {} AsyncClientImpl::~AsyncClientImpl() { diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 25cfea0397a78..bde0a26ed4c4d 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -70,8 +70,8 @@ RetryPolicyImpl::RetryPolicyImpl(const envoy::api::v2::route::RetryPolicy& retry per_try_timeout_ = std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(retry_policy, per_try_timeout, 0)); num_retries_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(retry_policy, num_retries, 1); - retry_on_ = RetryStateImpl::parseRetryOn(retry_policy.retry_on()); - retry_on_ |= RetryStateImpl::parseRetryGrpcOn(retry_policy.retry_on()); + retry_on_ = RetryStateImpl::parseRetryOn(retry_policy.retry_on()).first; + retry_on_ |= RetryStateImpl::parseRetryGrpcOn(retry_policy.retry_on()).first; for (const auto& host_predicate : retry_policy.retry_host_predicate()) { auto& factory = Envoy::Config::Utility::getAndCheckFactory( diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 8badaa382cfa6..c09f485a1c7a2 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -79,10 +79,11 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap& // Merge in the headers. if (request_headers.EnvoyRetryOn()) { - retry_on_ |= parseRetryOn(request_headers.EnvoyRetryOn()->value().getStringView()); + retry_on_ |= parseRetryOn(request_headers.EnvoyRetryOn()->value().getStringView()).first; } if (request_headers.EnvoyRetryGrpcOn()) { - retry_on_ |= parseRetryGrpcOn(request_headers.EnvoyRetryGrpcOn()->value().getStringView()); + retry_on_ |= + parseRetryGrpcOn(request_headers.EnvoyRetryGrpcOn()->value().getStringView()).first; } if (retry_on_ != 0 && request_headers.EnvoyMaxRetries()) { uint64_t temp; @@ -113,8 +114,9 @@ void RetryStateImpl::enableBackoffTimer() { retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); } -uint32_t RetryStateImpl::parseRetryOn(absl::string_view config) { +std::pair RetryStateImpl::parseRetryOn(absl::string_view config) { uint32_t ret = 0; + bool all_fields_valid = true; for (const auto retry_on : StringUtil::splitToken(config, ",")) { if (retry_on == Http::Headers::get().EnvoyRetryOnValues._5xx) { ret |= RetryPolicy::RETRY_ON_5XX; @@ -128,14 +130,17 @@ uint32_t RetryStateImpl::parseRetryOn(absl::string_view config) { ret |= RetryPolicy::RETRY_ON_REFUSED_STREAM; } else if (retry_on == Http::Headers::get().EnvoyRetryOnValues.RetriableStatusCodes) { ret |= RetryPolicy::RETRY_ON_RETRIABLE_STATUS_CODES; + } else { + all_fields_valid = false; } } - return ret; + return {ret, all_fields_valid}; } -uint32_t RetryStateImpl::parseRetryGrpcOn(absl::string_view retry_grpc_on_header) { +std::pair RetryStateImpl::parseRetryGrpcOn(absl::string_view retry_grpc_on_header) { uint32_t ret = 0; + bool all_fields_valid = true; for (const auto retry_on : StringUtil::splitToken(retry_grpc_on_header, ",")) { if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Cancelled) { ret |= RetryPolicy::RETRY_ON_GRPC_CANCELLED; @@ -147,10 +152,12 @@ uint32_t RetryStateImpl::parseRetryGrpcOn(absl::string_view retry_grpc_on_header ret |= RetryPolicy::RETRY_ON_GRPC_UNAVAILABLE; } else if (retry_on == Http::Headers::get().EnvoyRetryOnGrpcValues.Internal) { ret |= RetryPolicy::RETRY_ON_GRPC_INTERNAL; + } else { + all_fields_valid = false; } } - return ret; + return {ret, all_fields_valid}; } void RetryStateImpl::resetRetry() { diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 647680da4f2f8..7177a3f2909f5 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -29,10 +29,23 @@ class RetryStateImpl : public RetryState { Upstream::ResourcePriority priority); ~RetryStateImpl(); - static uint32_t parseRetryOn(absl::string_view config); - - // Returns the RetryPolicy extracted from the x-envoy-retry-grpc-on header. - static uint32_t parseRetryGrpcOn(absl::string_view retry_grpc_on_header); + /** + * Returns the RetryPolicy extracted from the x-envoy-retry-on header. + * @param config is the value of the header. + * @return std::pair the uint32_t is a bitset representing the + * valid retry policies in @param config. The bool is TRUE iff all the + * policies specified in @param config are valid. + */ + static std::pair parseRetryOn(absl::string_view config); + + /** + * Returns the RetryPolicy extracted from the x-envoy-retry-grpc-on header. + * @param config is the value of the header. + * @return std::pair the uint32_t is a bitset representing the + * valid retry policies in @param config. The bool is TRUE iff all the + * policies specified in @param config are valid. + */ + static std::pair parseRetryGrpcOn(absl::string_view retry_grpc_on_header); // Router::RetryState bool enabled() override { return retry_on_ != 0; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index dbdf35a19708c..026cb807e860b 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -222,6 +222,25 @@ Filter::~Filter() { ASSERT(!retry_state_); } +const FilterUtility::StrictHeaderChecker::HeaderCheckResult +FilterUtility::StrictHeaderChecker::checkHeader(Http::HeaderMap& headers, + const Http::LowerCaseString& target_header) { + if (target_header == Http::Headers::get().EnvoyUpstreamRequestTimeoutMs) { + return isInteger(headers.EnvoyUpstreamRequestTimeoutMs()); + } else if (target_header == Http::Headers::get().EnvoyUpstreamRequestPerTryTimeoutMs) { + return isInteger(headers.EnvoyUpstreamRequestPerTryTimeoutMs()); + } else if (target_header == Http::Headers::get().EnvoyMaxRetries) { + return isInteger(headers.EnvoyMaxRetries()); + } else if (target_header == Http::Headers::get().EnvoyRetryOn) { + return hasValidRetryFields(headers.EnvoyRetryOn(), &Router::RetryStateImpl::parseRetryOn); + } else if (target_header == Http::Headers::get().EnvoyRetryGrpcOn) { + return hasValidRetryFields(headers.EnvoyRetryGrpcOn(), + &Router::RetryStateImpl::parseRetryGrpcOn); + } + // Should only validate headers for which we have implemented a validator. + NOT_REACHED_GCOVR_EXCL_LINE +} + Stats::StatName Filter::upstreamZone(Upstream::HostDescriptionConstSharedPtr upstream_host) { return upstream_host ? upstream_host->localityZoneStatName() : config_.empty_stat_name_; } @@ -381,6 +400,24 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e ENVOY_STREAM_LOG(debug, "cluster '{}' match for URL '{}'", *callbacks_, route_entry_->clusterName(), headers.Path()->value().getStringView()); + if (config_.strict_check_headers_ != nullptr) { + for (const auto& header : *config_.strict_check_headers_) { + const auto res = FilterUtility::StrictHeaderChecker::checkHeader(headers, header); + if (!res.valid_) { + callbacks_->streamInfo().setResponseFlag( + StreamInfo::ResponseFlag::InvalidEnvoyRequestHeaders); + const std::string body = fmt::format("invalid header '{}' with value '{}'", + std::string(res.entry_->key().getStringView()), + std::string(res.entry_->value().getStringView())); + const std::string details = + absl::StrCat(StreamInfo::ResponseCodeDetails::get().InvalidEnvoyRequestHeaders, "{", + res.entry_->key().getStringView(), "}"); + callbacks_->sendLocalReply(Http::Code::BadRequest, body, nullptr, absl::nullopt, details); + return Http::FilterHeadersStatus::StopIteration; + } + } + } + const Http::HeaderEntry* request_alt_name = headers.EnvoyUpstreamAltStatName(); if (request_alt_name) { // TODO(#7003): converting this header value into a StatName requires diff --git a/source/common/router/router.h b/source/common/router/router.h index 0fc6f816332e6..b4351d7e2299e 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -67,6 +67,51 @@ class FilterUtility { bool hedge_on_per_try_timeout_; }; + class StrictHeaderChecker { + public: + struct HeaderCheckResult { + bool valid_ = true; + const Http::HeaderEntry* entry_; + }; + + /** + * Determine whether a given header's value passes the strict validation + * defined for that header. + * @param headers supplies the headers from which to get the target header. + * @param target_header is the header to be validated. + * @return HeaderCheckResult containing the entry for @param target_header + * and valid_ set to FALSE if @param target_header is set to an + * invalid value. If @param target_header doesn't appear in + * @param headers, return a result with valid_ set to TRUE. + */ + static const HeaderCheckResult checkHeader(Http::HeaderMap& headers, + const Http::LowerCaseString& target_header); + + using ParseRetryFlagsFunc = std::function(absl::string_view)>; + + private: + static HeaderCheckResult hasValidRetryFields(Http::HeaderEntry* header_entry, + const ParseRetryFlagsFunc& parseFn) { + HeaderCheckResult r; + if (header_entry) { + const auto flags_and_validity = parseFn(header_entry->value().getStringView()); + r.valid_ = flags_and_validity.second; + r.entry_ = header_entry; + } + return r; + } + + static HeaderCheckResult isInteger(Http::HeaderEntry* header_entry) { + HeaderCheckResult r; + if (header_entry) { + uint64_t out; + r.valid_ = absl::SimpleAtoi(header_entry->value().getStringView(), &out); + r.entry_ = header_entry; + } + return r; + } + }; + /** * Set the :scheme header based on the properties of the upstream cluster. */ @@ -115,6 +160,7 @@ class FilterConfig { Stats::Scope& scope, Upstream::ClusterManager& cm, Runtime::Loader& runtime, Runtime::RandomGenerator& random, ShadowWriterPtr&& shadow_writer, bool emit_dynamic_stats, bool start_child_span, bool suppress_envoy_headers, + const Protobuf::RepeatedPtrField& strict_check_headers, TimeSource& time_source, Http::Context& http_context) : scope_(scope), local_info_(local_info), cm_(cm), runtime_(runtime), random_(random), stats_{ALL_ROUTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, @@ -123,7 +169,14 @@ class FilterConfig { stat_name_pool_(scope_.symbolTable()), retry_(stat_name_pool_.add("retry")), zone_name_(stat_name_pool_.add(local_info_.zoneName())), empty_stat_name_(stat_name_pool_.add("")), shadow_writer_(std::move(shadow_writer)), - time_source_(time_source) {} + time_source_(time_source) { + if (!strict_check_headers.empty()) { + strict_check_headers_ = std::make_unique(); + for (const auto& header : strict_check_headers) { + strict_check_headers_->emplace_back(Http::LowerCaseString(header)); + } + } + } FilterConfig(const std::string& stat_prefix, Server::Configuration::FactoryContext& context, ShadowWriterPtr&& shadow_writer, @@ -132,11 +185,14 @@ class FilterConfig { context.runtime(), context.random(), std::move(shadow_writer), PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, dynamic_stats, true), config.start_child_span(), config.suppress_envoy_headers(), - context.api().timeSource(), context.httpContext()) { + config.strict_check_headers(), context.api().timeSource(), + context.httpContext()) { for (const auto& upstream_log : config.upstream_log()) { upstream_logs_.push_back(AccessLog::AccessLogFactory::fromProto(upstream_log, context)); } } + using HeaderVector = std::vector; + using HeaderVectorPtr = std::unique_ptr; ShadowWriter& shadowWriter() { return *shadow_writer_; } TimeSource& timeSource() { return time_source_; } @@ -150,6 +206,8 @@ class FilterConfig { const bool emit_dynamic_stats_; const bool start_child_span_; const bool suppress_envoy_headers_; + // TODO(xyu-stripe): Make this a bitset to keep cluster memory footprint down. + HeaderVectorPtr strict_check_headers_; std::list upstream_logs_; Http::Context& http_context_; Stats::StatNamePool stat_name_pool_; diff --git a/source/common/stream_info/utility.cc b/source/common/stream_info/utility.cc index 7821c8caab829..339094eacecc6 100644 --- a/source/common/stream_info/utility.cc +++ b/source/common/stream_info/utility.cc @@ -23,6 +23,7 @@ const std::string ResponseFlagUtils::RATE_LIMITED = "RL"; const std::string ResponseFlagUtils::UNAUTHORIZED_EXTERNAL_SERVICE = "UAEX"; const std::string ResponseFlagUtils::RATELIMIT_SERVICE_ERROR = "RLSE"; const std::string ResponseFlagUtils::STREAM_IDLE_TIMEOUT = "SI"; +const std::string ResponseFlagUtils::INVALID_ENVOY_REQUEST_HEADERS = "IH"; void ResponseFlagUtils::appendString(std::string& result, const std::string& append) { if (result.empty()) { @@ -35,7 +36,7 @@ void ResponseFlagUtils::appendString(std::string& result, const std::string& app const std::string ResponseFlagUtils::toShortString(const StreamInfo& stream_info) { std::string result; - static_assert(ResponseFlag::LastFlag == 0x10000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x20000, "A flag has been added. Fix this code."); if (stream_info.hasResponseFlag(ResponseFlag::FailedLocalHealthCheck)) { appendString(result, FAILED_LOCAL_HEALTH_CHECK); @@ -105,6 +106,10 @@ const std::string ResponseFlagUtils::toShortString(const StreamInfo& stream_info appendString(result, STREAM_IDLE_TIMEOUT); } + if (stream_info.hasResponseFlag(ResponseFlag::InvalidEnvoyRequestHeaders)) { + appendString(result, INVALID_ENVOY_REQUEST_HEADERS); + } + return result.empty() ? NONE : result; } @@ -129,6 +134,7 @@ absl::optional ResponseFlagUtils::toResponseFlag(const std::string ResponseFlag::DownstreamConnectionTermination}, {ResponseFlagUtils::UPSTREAM_RETRY_LIMIT_EXCEEDED, ResponseFlag::UpstreamRetryLimitExceeded}, {ResponseFlagUtils::STREAM_IDLE_TIMEOUT, ResponseFlag::StreamIdleTimeout}, + {ResponseFlagUtils::INVALID_ENVOY_REQUEST_HEADERS, ResponseFlag::InvalidEnvoyRequestHeaders}, }; const auto& it = map.find(flag); if (it != map.end()) { diff --git a/source/common/stream_info/utility.h b/source/common/stream_info/utility.h index bf588a7aa72da..c808d7e8aec13 100644 --- a/source/common/stream_info/utility.h +++ b/source/common/stream_info/utility.h @@ -38,6 +38,7 @@ class ResponseFlagUtils { const static std::string UNAUTHORIZED_EXTERNAL_SERVICE; const static std::string RATELIMIT_SERVICE_ERROR; const static std::string STREAM_IDLE_TIMEOUT; + const static std::string INVALID_ENVOY_REQUEST_HEADERS; }; /** diff --git a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc index 36c6584d2e090..c9c94146ef9c3 100644 --- a/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc +++ b/source/extensions/access_loggers/http_grpc/grpc_access_log_impl.cc @@ -108,7 +108,7 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( envoy::data::accesslog::v2::AccessLogCommon& common_access_log, const StreamInfo::StreamInfo& stream_info) { - static_assert(StreamInfo::ResponseFlag::LastFlag == 0x10000, + static_assert(StreamInfo::ResponseFlag::LastFlag == 0x20000, "A flag has been added. Fix this code."); if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::FailedLocalHealthCheck)) { @@ -180,6 +180,10 @@ void HttpGrpcAccessLog::responseFlagsToAccessLogResponseFlags( if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout)) { common_access_log.mutable_response_flags()->set_stream_idle_timeout(true); } + + if (stream_info.hasResponseFlag(StreamInfo::ResponseFlag::InvalidEnvoyRequestHeaders)) { + common_access_log.mutable_response_flags()->set_invalid_envoy_request_headers(true); + } } void HttpGrpcAccessLog::log(const Http::HeaderMap* request_headers, diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc index 468b0f9dbe1d7..5e34f5c0e33d8 100644 --- a/test/common/access_log/access_log_impl_test.cc +++ b/test/common/access_log/access_log_impl_test.cc @@ -869,11 +869,12 @@ name: envoy.file_access_log - DC - URX - SI + - IH config: path: /dev/null )EOF"; - static_assert(StreamInfo::ResponseFlag::LastFlag == 0x10000, + static_assert(StreamInfo::ResponseFlag::LastFlag == 0x20000, "A flag has been added. Fix this code."); std::vector all_response_flags = { @@ -894,6 +895,7 @@ name: envoy.file_access_log StreamInfo::ResponseFlag::DownstreamConnectionTermination, StreamInfo::ResponseFlag::UpstreamRetryLimitExceeded, StreamInfo::ResponseFlag::StreamIdleTimeout, + StreamInfo::ResponseFlag::InvalidEnvoyRequestHeaders, }; InstanceSharedPtr log = AccessLogFactory::fromProto(parseAccessLogFromV2Yaml(yaml), context_); @@ -924,7 +926,7 @@ name: envoy.file_access_log "[\"embedded message failed validation\"] | caused by " "ResponseFlagFilterValidationError.Flags[i]: [\"value must be in list \" [\"LH\" \"UH\" " "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\" \"RLSE\" " - "\"DC\" \"URX\" \"SI\"]]): " + "\"DC\" \"URX\" \"SI\" \"IH\"]]): " "response_flag_filter {\n flags: \"UnsupportedFlag\"\n}\n"); } @@ -947,7 +949,7 @@ name: envoy.file_access_log "[\"embedded message failed validation\"] | caused by " "ResponseFlagFilterValidationError.Flags[i]: [\"value must be in list \" [\"LH\" \"UH\" " "\"UT\" \"LR\" \"UR\" \"UF\" \"UC\" \"UO\" \"NR\" \"DI\" \"FI\" \"RL\" \"UAEX\" \"RLSE\" " - "\"DC\" \"URX\" \"SI\"]]): " + "\"DC\" \"URX\" \"SI\" \"IH\"]]): " "response_flag_filter {\n flags: \"UnsupportedFlag\"\n}\n"); } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 256bb7926fc3c..11ebabe128ea3 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -48,6 +48,7 @@ using testing::Return; using testing::ReturnPointee; using testing::ReturnRef; using testing::SaveArg; +using testing::StartsWith; namespace Envoy { namespace Router { @@ -79,11 +80,12 @@ class TestFilter : public Filter { class RouterTestBase : public testing::Test { public: - RouterTestBase(bool start_child_span, bool suppress_envoy_headers) + RouterTestBase(bool start_child_span, bool suppress_envoy_headers, + Protobuf::RepeatedPtrField strict_headers_to_check) : http_context_(stats_store_.symbolTable()), shadow_writer_(new MockShadowWriter()), config_("test.", local_info_, stats_store_, cm_, runtime_, random_, ShadowWriterPtr{shadow_writer_}, true, start_child_span, suppress_envoy_headers, - test_time_.timeSystem(), http_context_), + std::move(strict_headers_to_check), test_time_.timeSystem(), http_context_), router_(config_) { router_.setDecoderFilterCallbacks(callbacks_); upstream_locality_.set_zone("to_az"); @@ -256,14 +258,15 @@ class RouterTestBase : public testing::Test { class RouterTest : public RouterTestBase { public: - RouterTest() : RouterTestBase(false, false) { + RouterTest() : RouterTestBase(false, false, Protobuf::RepeatedPtrField{}) { EXPECT_CALL(callbacks_, activeSpan()).WillRepeatedly(ReturnRef(span_)); }; }; class RouterTestSuppressEnvoyHeaders : public RouterTestBase { public: - RouterTestSuppressEnvoyHeaders() : RouterTestBase(false, true) {} + RouterTestSuppressEnvoyHeaders() + : RouterTestBase(false, true, Protobuf::RepeatedPtrField{}) {} }; TEST_F(RouterTest, RouteNotFound) { @@ -4013,7 +4016,7 @@ TEST_F(WatermarkTest, RetryRequestNotComplete) { class RouterTestChildSpan : public RouterTestBase { public: - RouterTestChildSpan() : RouterTestBase(true, false) {} + RouterTestChildSpan() : RouterTestBase(true, false, Protobuf::RepeatedPtrField{}) {} }; // Make sure child spans start/inject/finish with a normal flow. @@ -4082,5 +4085,168 @@ TEST_F(RouterTestChildSpan, ResetFlow) { encoder.stream_.resetStream(Http::StreamResetReason::RemoteReset); } +Protobuf::RepeatedPtrField protobufStrList(const std::vector& v) { + Protobuf::RepeatedPtrField res; + for (auto& field : v) { + *res.Add() = field; + } + + return res; +} + +class RouterTestStrictCheckOneHeader : public RouterTestBase, + public testing::WithParamInterface { +public: + RouterTestStrictCheckOneHeader() : RouterTestBase(false, false, protobufStrList({GetParam()})){}; +}; + +INSTANTIATE_TEST_SUITE_P(StrictHeaderCheck, RouterTestStrictCheckOneHeader, + testing::Values("x-envoy-upstream-rq-timeout-ms", + "x-envoy-upstream-rq-per-try-timeout-ms", + "x-envoy-max-retries", "x-envoy-retry-on", + "x-envoy-retry-grpc-on")); + +// Each test param instantiates a router that strict-checks one particular header. +// This test decodes a set of headers with invalid values and asserts that the +// strict header check only fails for the single header specified by the test param +TEST_P(RouterTestStrictCheckOneHeader, SingleInvalidHeader) { + Http::TestHeaderMapImpl req_headers{ + {"X-envoy-Upstream-rq-timeout-ms", "10.0"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "1.0"}, + {"x-envoy-max-retries", "2.0"}, + {"x-envoy-retry-on", "5xx,cancelled"}, // 'cancelled' is an invalid entry + {"x-envoy-retry-grpc-on", "cancelled, internal"}, // spaces are considered errors + }; + HttpTestUtility::addDefaultHeaders(req_headers); + auto checked_header = GetParam(); + + EXPECT_CALL(callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::InvalidEnvoyRequestHeaders)); + + EXPECT_CALL(callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](Http::HeaderMap& response_headers, bool end_stream) -> void { + EXPECT_EQ(enumToInt(Http::Code::BadRequest), + Envoy::Http::Utility::getResponseStatus(response_headers)); + EXPECT_FALSE(end_stream); + })); + + EXPECT_CALL(callbacks_, encodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance& data, bool end_stream) -> void { + EXPECT_THAT(data.toString(), + StartsWith(fmt::format("invalid header '{}' with value ", checked_header))); + EXPECT_TRUE(end_stream); + })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, router_.decodeHeaders(req_headers, true)); + EXPECT_EQ(callbacks_.details_, + fmt::format("request_headers_failed_strict_check{{{}}}", checked_header)); +} + +class RouterTestStrictCheckSomeHeaders + : public RouterTestBase, + public testing::WithParamInterface> { +public: + RouterTestStrictCheckSomeHeaders() : RouterTestBase(false, false, protobufStrList(GetParam())){}; +}; + +INSTANTIATE_TEST_SUITE_P(StrictHeaderCheck, RouterTestStrictCheckSomeHeaders, + testing::Values(std::vector{"x-envoy-upstream-rq-timeout-ms", + "x-envoy-max-retries"}, + std::vector{})); + +// Request has headers with invalid values, but headers are *excluded* from the +// set to which strict-checks apply. Assert that these headers are not rejected. +TEST_P(RouterTestStrictCheckSomeHeaders, IgnoreOmittedHeaders) { + // Invalid, but excluded from the configured set of headers to strictly-check + Http::TestHeaderMapImpl headers{ + {"x-envoy-upstream-rq-per-try-timeout-ms", "1.0"}, + {"x-envoy-upstream-rq-timeout-ms", "5000"}, + {"x-envoy-retry-on", "5xx,cancelled"}, + }; + HttpTestUtility::addDefaultHeaders(headers); + + expectResponseTimerCreate(); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, router_.decodeHeaders(headers, true)); + router_.onDestroy(); +} + +const std::vector SUPPORTED_STRICT_CHECKED_HEADERS = { + "x-envoy-upstream-rq-timeout-ms", "x-envoy-upstream-rq-per-try-timeout-ms", "x-envoy-retry-on", + "x-envoy-retry-grpc-on", "x-envoy-max-retries"}; + +class RouterTestStrictCheckAllHeaders + : public RouterTestBase, + public testing::WithParamInterface> { +public: + RouterTestStrictCheckAllHeaders() + : RouterTestBase(false, false, protobufStrList(SUPPORTED_STRICT_CHECKED_HEADERS)){}; +}; + +INSTANTIATE_TEST_SUITE_P(StrictHeaderCheck, RouterTestStrictCheckAllHeaders, + testing::Combine(testing::ValuesIn(SUPPORTED_STRICT_CHECKED_HEADERS), + testing::ValuesIn(SUPPORTED_STRICT_CHECKED_HEADERS))); + +// Each instance of this test configures a router to strict-validate all +// supported headers and asserts that a request with invalid values set for some +// *pair* of headers is rejected. +TEST_P(RouterTestStrictCheckAllHeaders, MultipleInvalidHeaders) { + const auto& header1 = std::get<0>(GetParam()); + const auto& header2 = std::get<1>(GetParam()); + Http::TestHeaderMapImpl headers{{header1, "invalid"}, {header2, "invalid"}}; + HttpTestUtility::addDefaultHeaders(headers); + + EXPECT_CALL(callbacks_.stream_info_, + setResponseFlag(StreamInfo::ResponseFlag::InvalidEnvoyRequestHeaders)); + + EXPECT_CALL(callbacks_, encodeHeaders_(_, _)) + .WillOnce(Invoke([&](Http::HeaderMap& response_headers, bool end_stream) -> void { + EXPECT_EQ(enumToInt(Http::Code::BadRequest), + Envoy::Http::Utility::getResponseStatus(response_headers)); + EXPECT_FALSE(end_stream); + })); + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, router_.decodeHeaders(headers, true)); + EXPECT_THAT(callbacks_.details_, + StartsWith(fmt::format("request_headers_failed_strict_check{{"))); + router_.onDestroy(); +} + +// Request has headers with invalid values, but headers are *excluded* from the +// set to which strict-checks apply. Assert that these headers are not rejected. +TEST(RouterFilterUtilityTest, StrictCheckValidHeaders) { + Http::TestHeaderMapImpl headers{ + {"X-envoy-Upstream-rq-timeout-ms", "100"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "100"}, + {"x-envoy-max-retries", "2"}, + {"not-checked", "always passes"}, + {"x-envoy-retry-on", + "5xx,gateway-error,retriable-4xx,refused-stream,connect-failure,retriable-status-codes"}, + {"x-envoy-retry-grpc-on", + "cancelled,internal,deadline-exceeded,resource-exhausted,unavailable"}, + }; + + for (const auto& target : SUPPORTED_STRICT_CHECKED_HEADERS) { + EXPECT_TRUE( + FilterUtility::StrictHeaderChecker::checkHeader(headers, Http::LowerCaseString(target)) + .valid_) + << fmt::format("'{}' should have passed strict validation", target); + } + + Http::TestHeaderMapImpl failing_headers{ + {"X-envoy-Upstream-rq-timeout-ms", "10.0"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "1.0"}, + {"x-envoy-max-retries", "2.0"}, + {"x-envoy-retry-on", "5xx,cancelled"}, // 'cancelled' is an invalid entry + {"x-envoy-retry-grpc-on", "cancelled, internal"}, // spaces are considered errors + }; + + for (const auto& target : SUPPORTED_STRICT_CHECKED_HEADERS) { + EXPECT_FALSE(FilterUtility::StrictHeaderChecker::checkHeader(failing_headers, + Http::LowerCaseString(target)) + .valid_) + << fmt::format("'{}' should have failed strict validation", target); + } +} + } // namespace Router } // namespace Envoy diff --git a/test/common/stream_info/utility_test.cc b/test/common/stream_info/utility_test.cc index dead803874177..abcf167e985db 100644 --- a/test/common/stream_info/utility_test.cc +++ b/test/common/stream_info/utility_test.cc @@ -15,7 +15,7 @@ namespace StreamInfo { namespace { TEST(ResponseFlagUtilsTest, toShortStringConversion) { - static_assert(ResponseFlag::LastFlag == 0x10000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x20000, "A flag has been added. Fix this code."); std::vector> expected = { std::make_pair(ResponseFlag::FailedLocalHealthCheck, "LH"), @@ -35,6 +35,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { std::make_pair(ResponseFlag::DownstreamConnectionTermination, "DC"), std::make_pair(ResponseFlag::UpstreamRetryLimitExceeded, "URX"), std::make_pair(ResponseFlag::StreamIdleTimeout, "SI"), + std::make_pair(ResponseFlag::InvalidEnvoyRequestHeaders, "IH"), }; for (const auto& test_case : expected) { @@ -63,7 +64,7 @@ TEST(ResponseFlagUtilsTest, toShortStringConversion) { } TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) { - static_assert(ResponseFlag::LastFlag == 0x10000, "A flag has been added. Fix this code."); + static_assert(ResponseFlag::LastFlag == 0x20000, "A flag has been added. Fix this code."); std::vector> expected = { std::make_pair("LH", ResponseFlag::FailedLocalHealthCheck), @@ -83,6 +84,7 @@ TEST(ResponseFlagsUtilsTest, toResponseFlagConversion) { std::make_pair("DC", ResponseFlag::DownstreamConnectionTermination), std::make_pair("URX", ResponseFlag::UpstreamRetryLimitExceeded), std::make_pair("SI", ResponseFlag::StreamIdleTimeout), + std::make_pair("IH", ResponseFlag::InvalidEnvoyRequestHeaders), }; EXPECT_FALSE(ResponseFlagUtils::toResponseFlag("NonExistentFlag").has_value()); diff --git a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc index abb92c9b05299..960bddb1b96f8 100644 --- a/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/http_grpc/grpc_access_log_impl_test.cc @@ -716,6 +716,7 @@ TEST(responseFlagsToAccessLogResponseFlagsTest, All) { common_access_log_expected.mutable_response_flags()->set_downstream_connection_termination(true); common_access_log_expected.mutable_response_flags()->set_upstream_retry_limit_exceeded(true); common_access_log_expected.mutable_response_flags()->set_stream_idle_timeout(true); + common_access_log_expected.mutable_response_flags()->set_invalid_envoy_request_headers(true); EXPECT_EQ(common_access_log_expected.DebugString(), common_access_log.DebugString()); } diff --git a/test/extensions/filters/http/router/config_test.cc b/test/extensions/filters/http/router/config_test.cc index a1d8e5a7dd03d..70e0079558a03 100644 --- a/test/extensions/filters/http/router/config_test.cc +++ b/test/extensions/filters/http/router/config_test.cc @@ -47,6 +47,30 @@ TEST(RouterFilterConfigTest, BadRouterFilterConfig) { EXPECT_THROW(factory.createFilterFactory(*json_config, "stats", context), Json::Exception); } +TEST(RouterFilterConfigTest, RouterFilterWithUnsupportedStrictHeaderCheck) { + const std::string yaml = R"EOF( + strict_check_headers: + - unsupportedHeader + )EOF"; + + envoy::config::filter::http::router::v2::Router router_config; + TestUtility::loadFromYaml(yaml, router_config); + + NiceMock context; + RouterFilterConfig factory; + EXPECT_THROW_WITH_MESSAGE( + factory.createFilterFactoryFromProto(router_config, "stats", context), + ProtoValidationException, + "Proto constraint validation failed (RouterValidationError.StrictCheckHeaders[i]: " + "[\"value must be in list \" [" + "\"x-envoy-upstream-rq-timeout-ms\" " + "\"x-envoy-upstream-rq-per-try-timeout-ms\" " + "\"x-envoy-max-retries\" " + "\"x-envoy-retry-grpc-on\" " + "\"x-envoy-retry-on\"" + "]]): strict_check_headers: \"unsupportedHeader\"\n"); +} + TEST(RouterFilterConfigTest, RouterV2Filter) { envoy::config::filter::http::router::v2::Router router_config; router_config.mutable_dynamic_stats()->set_value(true);