diff --git a/docs/root/configuration/http/http_conn_man/response_code_details.rst b/docs/root/configuration/http/http_conn_man/response_code_details.rst index def0da6cbba23..342350c6a4f8c 100644 --- a/docs/root/configuration/http/http_conn_man/response_code_details.rst +++ b/docs/root/configuration/http/http_conn_man/response_code_details.rst @@ -26,7 +26,8 @@ Below are the list of reasons the HttpConnectionManager or Router filter may sen duration_timeout, The max connection duration was exceeded. direct_response, A direct response was generated by the router filter. filter_chain_not_found, The request was rejected due to no matching filter chain. - filter_removed_required_headers, The request was rejected in the filter manager because a configured filter removed required headers. + filter_removed_required_request_headers, The request was rejected in the filter manager because a configured filter removed required request headers. + filter_removed_required_response_headers, The response was rejected in the filter manager because a configured filter removed required response headers or these values were invalid (e.g. overflown status). internal_redirect, The original stream was replaced with an internal redirect. low_version, The HTTP/1.0 or HTTP/0.9 request was rejected due to HTTP/1.0 support not being configured. maintenance_mode, The request was rejected by the router filter because the cluster was in maintenance mode. diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index acf84800d7303..ecb04a7bf555b 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -179,8 +179,13 @@ struct ResponseCodeDetailValues { const std::string AdminFilterResponse = "admin_filter_response"; // The original stream was replaced with an internal redirect. const std::string InternalRedirect = "internal_redirect"; - // The request was rejected because configured filters erroneously removed required headers. - const std::string FilterRemovedRequiredHeaders = "filter_removed_required_headers"; + // The request was rejected because configured filters erroneously removed required request + // headers. + const std::string FilterRemovedRequiredRequestHeaders = "filter_removed_required_request_headers"; + // The request was rejected because configured filters erroneously removed required response + // headers. + const std::string FilterRemovedRequiredResponseHeaders = + "filter_removed_required_response_headers"; // The request was rejected because the original IP couldn't be detected. const std::string OriginalIPDetectionFailed = "rejecting because detection failed"; // Changes or additions to details should be reflected in diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index a962365e10f36..f5aa68f53052f 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1084,6 +1084,21 @@ void FilterManager::encodeHeaders(ActiveStreamEncoderFilter* filter, ResponseHea } } + // Check if the filter chain above did not remove critical headers or set malformed header values. + // We could do this at the codec in order to prevent other places than the filter chain from + // removing critical headers, but it will come with the implementation complexity. + // See the previous attempt (#15658) for detail, and for now we choose to protect only against + // filter chains. + const auto status = HeaderUtility::checkRequiredResponseHeaders(headers); + if (!status.ok()) { + // If the check failed, then we reply with BadGateway, and stop the further processing. + sendLocalReply( + Http::Code::BadGateway, status.message(), nullptr, absl::nullopt, + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredResponseHeaders, + "{", status.message(), "}")); + return; + } + const bool modified_end_stream = (end_stream && continue_data_entry == encoder_filters_.end()); state_.non_100_response_headers_encoded_ = true; filter_manager_callbacks_.encodeHeaders(headers, modified_end_stream); diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index d019a1799175c..5bbc372ffb382 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -322,7 +322,7 @@ bool HeaderUtility::shouldCloseConnection(Http::Protocol protocol, return false; } -Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& headers) { +Http::Status HeaderUtility::checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers) { if (!headers.Method()) { return absl::InvalidArgumentError( absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Method.get())); @@ -344,6 +344,15 @@ Http::Status HeaderUtility::checkRequiredHeaders(const Http::RequestHeaderMap& h return Http::okStatus(); } +Http::Status HeaderUtility::checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers) { + const absl::optional status = Utility::getResponseStatusNoThrow(headers); + if (!status.has_value()) { + return absl::InvalidArgumentError( + absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Status.get())); + } + return Http::okStatus(); +} + bool HeaderUtility::isRemovableHeader(absl::string_view header) { return (header.empty() || header[0] != ':') && !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()); diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 04d1201d61ecf..a7ab5ab2783e0 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -198,13 +198,20 @@ class HeaderUtility { */ static absl::string_view::size_type getPortStart(absl::string_view host); - /* Does a common header check ensuring required headers are present. + /* Does a common header check ensuring required request headers are present. * Required request headers include :method header, :path for non-CONNECT requests, and * host/authority for HTTP/1.1 or CONNECT requests. * @return Status containing the result. If failed, message includes details on which header was * missing. */ - static Http::Status checkRequiredHeaders(const Http::RequestHeaderMap& headers); + static Http::Status checkRequiredRequestHeaders(const Http::RequestHeaderMap& headers); + + /* Does a common header check ensuring required response headers are present. + * Current required response headers only includes :status. + * @return Status containing the result. If failed, message includes details on which header was + * missing. + */ + static Http::Status checkRequiredResponseHeaders(const Http::ResponseHeaderMap& headers); /** * Returns true if a header may be safely removed without causing additional diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 23859ac380a2c..f6ad7dfe7f0fe 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -402,7 +402,7 @@ static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; Status RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers)); const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 5f3cc901a7684..ec188c386cf31 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -210,7 +210,7 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(HeaderUtility::checkRequiredRequestHeaders(headers)); // This must exist outside of the scope of isUpgrade as the underlying memory is // needed until encodeHeadersBase has been called. std::vector final_headers; diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 1098a39b79fd5..581ebfb6ae594 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -58,7 +58,7 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& bool end_stream) { // Required headers must be present. This can only happen by some erroneous processing after the // downstream codecs decode. - RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredHeaders(headers)); + RETURN_IF_ERROR(Http::HeaderUtility::checkRequiredRequestHeaders(headers)); ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers); local_end_stream_ = end_stream; diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 355f577a38bc7..99e1fe0b33a68 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -449,8 +449,8 @@ void UpstreamRequest::onPoolReady( // erroneously remove required headers. stream_info_.setResponseFlag(StreamInfo::ResponseFlag::DownstreamProtocolError); const std::string details = - absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredHeaders, "{", - status.message(), "}"); + absl::StrCat(StreamInfo::ResponseCodeDetails::get().FilterRemovedRequiredRequestHeaders, + "{", status.message(), "}"); parent_.callbacks()->sendLocalReply(Http::Code::ServiceUnavailable, status.message(), nullptr, absl::nullopt, details); return; diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index c8c9d311af5fc..ae92213908284 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1477,5 +1477,30 @@ TEST(PercentEncoding, Encoding) { EXPECT_EQ(Utility::PercentEncoding::encode("too%!large/", "%!/"), "too%25%21large%2F"); } +TEST(CheckRequiredHeaders, Request) { + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders( + TestRequestHeaderMapImpl{{":method", "GET"}, {":path", "/"}})); + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{ + {":method", "CONNECT"}, {":authority", "localhost:1234"}})); + EXPECT_EQ(absl::InvalidArgumentError("missing required header: :method"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{})); + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :path"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "GET"}})); + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :authority"), + HeaderUtility::checkRequiredRequestHeaders(TestRequestHeaderMapImpl{{":method", "CONNECT"}})); +} + +TEST(CheckRequiredHeaders, Response) { + EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredResponseHeaders( + TestResponseHeaderMapImpl{{":status", "200"}})); + EXPECT_EQ(absl::InvalidArgumentError("missing required header: :status"), + HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{})); + EXPECT_EQ( + absl::InvalidArgumentError("missing required header: :status"), + HeaderUtility::checkRequiredResponseHeaders(TestResponseHeaderMapImpl{{":status", "abcd"}})); +} + } // namespace Http } // namespace Envoy diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index dfac7ba12c36b..11addb7c2d0d4 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -165,12 +165,13 @@ TEST_F(RouterTest, MissingRequiredHeaders) { EXPECT_CALL(encoder, encodeHeaders(_, _)) .WillOnce(Invoke([](const Http::RequestHeaderMap& headers, bool) -> Http::Status { - return Http::HeaderUtility::checkRequiredHeaders(headers); + return Http::HeaderUtility::checkRequiredRequestHeaders(headers); })); - EXPECT_CALL(callbacks_, - sendLocalReply(Http::Code::ServiceUnavailable, - testing::Eq("missing required header: :method"), _, _, - "filter_removed_required_headers{missing required header: :method}")) + EXPECT_CALL( + callbacks_, + sendLocalReply(Http::Code::ServiceUnavailable, + testing::Eq("missing required header: :method"), _, _, + "filter_removed_required_request_headers{missing required header: :method}")) .WillOnce(testing::InvokeWithoutArgs([] {})); router_.decodeHeaders(headers, true); router_.onDestroy(); diff --git a/test/integration/BUILD b/test/integration/BUILD index 1dfbc527eae75..1e23aeb4a1d67 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -486,6 +486,7 @@ envoy_cc_test_library( "//test/integration/filters:local_reply_during_encoding_filter_lib", "//test/integration/filters:local_reply_with_metadata_filter_lib", "//test/integration/filters:random_pause_filter_lib", + "//test/integration/filters:remove_response_headers_lib", "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 7285345d40549..b7ec16e374bf3 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -527,3 +527,18 @@ envoy_cc_test_library( "//test/test_common:delegating_route_utility_lib", ], ) + +envoy_cc_test_library( + name = "remove_response_headers_lib", + srcs = [ + "remove_response_headers.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) diff --git a/test/integration/filters/remove_response_headers.cc b/test/integration/filters/remove_response_headers.cc new file mode 100644 index 0000000000000..20043cf7fd4b2 --- /dev/null +++ b/test/integration/filters/remove_response_headers.cc @@ -0,0 +1,33 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "test/integration/filters/common.h" + +namespace Envoy { + +// Registers the misbehaving filter which removes all response headers. +class RemoveResponseHeadersFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "remove-response-headers-filter"; + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override { + std::vector keys; + headers.iterate([&keys](const Http::HeaderEntry& header) -> Http::HeaderMap::Iterate { + keys.push_back(std::string(header.key().getStringView())); + return Http::HeaderMap::Iterate::Continue; + }); + for (auto& k : keys) { + const Http::LowerCaseString lower_key{k}; + headers.remove(lower_key); + } + return Http::FilterHeadersStatus::Continue; + } +}; + +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 13798f7509f0b..7ef450f2596fe 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2400,4 +2400,44 @@ TEST_P(DownstreamProtocolIntegrationTest, DisableStripTrailingHostDot) { EXPECT_EQ("404", response->headers().getStatusValue()); } +static std::string remove_response_headers_filter = R"EOF( +name: remove-response-headers-filter +typed_config: + "@type": type.googleapis.com/google.protobuf.Empty +)EOF"; + +TEST_P(ProtocolIntegrationTest, HeadersOnlyRequestWithRemoveResponseHeadersFilter) { + config_helper_.addFilter(remove_response_headers_filter); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + ASSERT_TRUE(response->waitForEndStream()); + // If a filter chain removes :status from the response headers, then Envoy must reply with + // BadGateway and must not crash. + ASSERT_TRUE(codec_client_->connected()); + EXPECT_EQ("502", response->headers().getStatusValue()); + EXPECT_THAT(response->body(), HasSubstr("missing required header: :status")); +} + +TEST_P(ProtocolIntegrationTest, RemoveResponseHeadersFilter) { + config_helper_.addFilter(remove_response_headers_filter); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + IntegrationStreamDecoderPtr response = + codec_client_->makeRequestWithBody(default_request_headers_, 10); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + ASSERT_TRUE(response->waitForEndStream()); + // If a filter chain removes :status from the response headers, then Envoy must reply with + // BadGateway and not crash. + ASSERT_TRUE(codec_client_->connected()); + EXPECT_EQ("502", response->headers().getStatusValue()); + EXPECT_THAT(response->body(), HasSubstr("missing required header: :status")); +} + } // namespace Envoy diff --git a/test/mocks/http/stream_encoder.cc b/test/mocks/http/stream_encoder.cc index 5d872c64db15f..39970b10d3881 100644 --- a/test/mocks/http/stream_encoder.cc +++ b/test/mocks/http/stream_encoder.cc @@ -17,7 +17,7 @@ MockRequestEncoder::MockRequestEncoder() { .WillByDefault(Invoke([](const RequestHeaderMap& headers, bool) -> Status { // Check to see that method is not-null. Path can be null for CONNECT and authority can be // null at the codec level. - ASSERT(HeaderUtility::checkRequiredHeaders(headers).ok()); + ASSERT(HeaderUtility::checkRequiredRequestHeaders(headers).ok()); return okStatus(); })); }