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 58acfe7b9c519..642b186557eb1 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 @@ -120,4 +120,5 @@ All http3 details are rooted at *http3.* http3.too_many_trailers, Either incoming request or response trailers contained too many entries. http3.remote_refuse, The peer refused the stream. http3.remote_reset, The peer reset the stream. + http3.inconsistent_content_length, The payload size is different from what was indicated by the content-length header. diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 211aeed6aa1f0..7c87f4af84999 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -405,7 +405,7 @@ HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscor HeaderUtility::HeaderValidationResult HeaderUtility::validateContentLength(absl::string_view header_value, bool override_stream_error_on_invalid_http_message, - bool& should_close_connection) { + bool& should_close_connection, size_t& content_length_output) { should_close_connection = false; std::vector values = absl::StrSplit(header_value, ','); absl::optional content_length; @@ -430,6 +430,7 @@ HeaderUtility::validateContentLength(absl::string_view header_value, return HeaderValidationResult::REJECT; } } + content_length_output = content_length.value(); return HeaderValidationResult::ACCEPT; } diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 1053f893c34f4..8e1ae6decd1c3 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -258,13 +258,14 @@ class HeaderUtility { /** * Check if header_value represents a valid value for HTTP content-length header. - * Return HeaderValidationResult and populate should_close_connection - * according to override_stream_error_on_invalid_http_message. + * Return HeaderValidationResult and populate content_length_output if the value is valid, + * otherwise populate should_close_connection according to + * override_stream_error_on_invalid_http_message. */ static HeaderValidationResult validateContentLength(absl::string_view header_value, bool override_stream_error_on_invalid_http_message, - bool& should_close_connection); + bool& should_close_connection, size_t& content_length_output); }; } // namespace Http diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 1d413afc60974..71038f7ab184b 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -53,15 +53,19 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& local_end_stream_ = end_stream; SendBufferMonitor::ScopedWatermarkBufferUpdater updater(this, this); auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers); - if (headers.Method() && headers.Method()->value() == "CONNECT") { - // It is a bytestream connect and should have :path and :protocol set accordingly - // As HTTP/1.1 does not require a path for CONNECT, we may have to add one - // if shifting codecs. For now, default to "/" - this can be made - // configurable if necessary. - // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02 - spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream; - if (!headers.Path()) { - spdy_headers[":path"] = "/"; + if (headers.Method()) { + if (headers.Method()->value() == "CONNECT") { + // It is a bytestream connect and should have :path and :protocol set accordingly + // As HTTP/1.1 does not require a path for CONNECT, we may have to add one + // if shifting codecs. For now, default to "/" - this can be made + // configurable if necessary. + // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02 + spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream; + if (!headers.Path()) { + spdy_headers[":path"] = "/"; + } + } else if (headers.Method()->value() == "HEAD") { + sent_head_request_ = true; } } WriteHeaders(std::move(spdy_headers), end_stream, nullptr); @@ -190,6 +194,9 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } else if (status != enumToInt(Http::Code::Continue)) { response_decoder_->decodeHeaders(std::move(headers), /*end_stream=*/fin); + if (status == enumToInt(Http::Code::NotModified)) { + got_304_response_ = true; + } } ConsumeHeaderList(); @@ -224,6 +231,11 @@ void EnvoyQuicClientStream::OnBodyAvailable() { if (fin_read_and_no_trailers) { end_stream_decoded_ = true; } + updateReceivedContentBytes(buffer->length(), fin_read_and_no_trailers); + if (stream_error() != quic::QUIC_STREAM_NO_ERROR) { + // A stream error has occurred, stop processing. + return; + } response_decoder_->decodeData(*buffer, fin_read_and_no_trailers); } @@ -255,6 +267,11 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; + updateReceivedContentBytes(0, true); + if (stream_error() != quic::QUIC_STREAM_NO_ERROR) { + // A stream error has occurred, stop processing. + return; + } quic::QuicRstStreamErrorCode transform_rst = quic::QUIC_STREAM_NO_ERROR; auto trailers = spdyHeaderBlockToEnvoyTrailers( received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_, diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index f763121944a1c..70e10c99ce223 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -73,17 +73,15 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, // Http::MultiplexedStreamImplBase bool hasPendingData() override; + void onStreamError(absl::optional should_close_connection, + quic::QuicRstStreamErrorCode rst_code) override; + private: QuicFilterManagerConnectionImpl* filterManagerConnection(); // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); - // Either reset the stream or close the connection according to - // should_close_connection and configured http3 options. - void onStreamError(absl::optional should_close_connection, - quic::QuicRstStreamErrorCode rst_code); - Http::ResponseDecoder* response_decoder_{nullptr}; bool decoded_100_continue_{false}; diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 1e62a22030558..84260b04760d3 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -211,6 +211,11 @@ void EnvoyQuicServerStream::OnBodyAvailable() { if (fin_read_and_no_trailers) { end_stream_decoded_ = true; } + updateReceivedContentBytes(buffer->length(), fin_read_and_no_trailers); + if (stream_error() != quic::QUIC_STREAM_NO_ERROR) { + // A stream error has occurred, stop processing. + return; + } request_decoder_->decodeData(*buffer, fin_read_and_no_trailers); } @@ -248,6 +253,11 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; + updateReceivedContentBytes(0, true); + if (stream_error() != quic::QUIC_STREAM_NO_ERROR) { + // A stream error has occurred, stop processing. + return; + } quic::QuicRstStreamErrorCode rst = quic::QUIC_STREAM_NO_ERROR; auto trailers = spdyHeaderBlockToEnvoyTrailers( received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_, diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index 8693cd197b2bf..6045d321b96b1 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -80,17 +80,16 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, void onPendingFlushTimer() override; bool hasPendingData() override; + void + onStreamError(absl::optional should_close_connection, + quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD) override; + private: QuicFilterManagerConnectionImpl* filterManagerConnection(); // Deliver awaiting trailers if body has been delivered. void maybeDecodeTrailers(); - // Either reset the stream or close the connection according to - // should_close_connection and configured http3 options. - void onStreamError(absl::optional should_close_connection, - quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD); - Http::RequestDecoder* request_decoder_{nullptr}; envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action_; diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index 4f1669f493efb..278f61246ff8c 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -107,9 +107,13 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, return Http::HeaderUtility::HeaderValidationResult::REJECT; } if (header_name == "content-length") { - return Http::HeaderUtility::validateContentLength( - header_value, override_stream_error_on_invalid_http_message, - close_connection_upon_invalid_header_); + size_t content_length = 0; + Http::HeaderUtility::HeaderValidationResult result = + Http::HeaderUtility::validateContentLength( + header_value, override_stream_error_on_invalid_http_message, + close_connection_upon_invalid_header_, content_length); + content_length_ = content_length; + return result; } return Http::HeaderUtility::HeaderValidationResult::ACCEPT; } @@ -122,6 +126,26 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // Needed for ENVOY_STREAM_LOG. virtual uint32_t streamId() PURE; virtual Network::Connection* connection() PURE; + // Either reset the stream or close the connection according to + // should_close_connection and configured http3 options. + virtual void + onStreamError(absl::optional should_close_connection, + quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD) PURE; + + // TODO(danzh) remove this once QUICHE enforces content-length consistency. + void updateReceivedContentBytes(size_t payload_length, bool end_stream) { + received_content_bytes_ += payload_length; + if (!content_length_.has_value()) { + return; + } + if (received_content_bytes_ > content_length_.value() || + (end_stream && received_content_bytes_ != content_length_.value() && + !(got_304_response_ && received_content_bytes_ == 0) && !(sent_head_request_))) { + details_ = Http3ResponseCodeDetailValues::inconsistent_content_length; + // Reset instead of closing the connection to align with nghttp2. + onStreamError(false); + } + } // True once end of stream is propagated to Envoy. Envoy doesn't expect to be // notified more than once about end of stream. So once this is true, no need @@ -141,6 +165,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // TODO(kbaichoo): bind the account to the QUIC buffers to enable tracking of // memory allocated within QUIC buffers. Buffer::BufferMemoryAccountSharedPtr buffer_memory_account_ = nullptr; + bool got_304_response_{false}; + bool sent_head_request_{false}; private: // Keeps track of bytes buffered in the stream send buffer in QUICHE and reacts @@ -157,6 +183,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, // state change in its own call stack. And Envoy upstream doesn't like quic stream to be unblocked // in its callstack either because the stream will push data right away. Event::SchedulableCallbackPtr async_stream_blockage_change_; + absl::optional content_length_; + size_t received_content_bytes_{0}; }; } // namespace Quic diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index b07324b049bcd..338c4bddfec69 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -53,6 +53,9 @@ class Http3ResponseCodeDetailValues { static constexpr absl::string_view too_many_trailers = "http3.too_many_trailers"; // Too many headers were sent. static constexpr absl::string_view too_many_headers = "http3.too_many_headers"; + // The payload size is different from what the content-length header indicated. + static constexpr absl::string_view inconsistent_content_length = + "http3.inconsistent_content_length"; }; // TODO(danzh): this is called on each write. Consider to return an address instance on the stack if diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 5951dced3199d..1fc0b8abeac6e 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -916,20 +916,26 @@ TEST(ValidateHeaders, Connect) { TEST(ValidateHeaders, ContentLength) { bool should_close_connection; - EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT, - HeaderUtility::validateContentLength("1,1", true, should_close_connection)); + size_t content_length{0}; + EXPECT_EQ( + HeaderUtility::HeaderValidationResult::ACCEPT, + HeaderUtility::validateContentLength("1,1", true, should_close_connection, content_length)); EXPECT_FALSE(should_close_connection); + EXPECT_EQ(1, content_length); - EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("1,2", true, should_close_connection)); + EXPECT_EQ( + HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("1,2", true, should_close_connection, content_length)); EXPECT_FALSE(should_close_connection); - EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("1,2", false, should_close_connection)); + EXPECT_EQ( + HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("1,2", false, should_close_connection, content_length)); EXPECT_TRUE(should_close_connection); - EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT, - HeaderUtility::validateContentLength("-1", false, should_close_connection)); + EXPECT_EQ( + HeaderUtility::HeaderValidationResult::REJECT, + HeaderUtility::validateContentLength("-1", false, should_close_connection, content_length)); EXPECT_TRUE(should_close_connection); } diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 4248c858838b9..e0705e8f3865f 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -1880,4 +1880,35 @@ TEST_P(Http2IntegrationTest, InvalidTrailers) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("invalid")); } +TEST_P(Http2IntegrationTest, InconsistentContentLength) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"content-length", "1025"}}); + + auto response = std::move(encoder_decoder.second); + request_encoder_ = &encoder_decoder.first; + codec_client_->sendData(*request_encoder_, 1024, false); + codec_client_->sendTrailers(*request_encoder_, + Http::TestRequestTrailerMapImpl{{"trailer", "value"}}); + + // Inconsistency in content-length header and the actually body length should be treated as a + // stream error. + ASSERT_TRUE(response->waitForReset()); + // http3.inconsistent_content_length. + if (downstreamProtocol() == Http::CodecType::HTTP3) { + EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("inconsistent_content_length")); + } else { + EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->resetReason()); + // http2.violation.of.messaging.rule + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("violation")); + } +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 4db44f4f03dc7..9e4d3f74ca2cf 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -493,6 +493,27 @@ TEST_P(ProtocolIntegrationTest, 304HeadResponseWithoutContentLengthLegacy) { EXPECT_TRUE(response->headers().get(Http::LowerCaseString("content-length")).empty()); } +// Tests that the response to a HEAD request can have content-length header but empty body. +TEST_P(ProtocolIntegrationTest, 200HeadResponseWithContentLength) { + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "HEAD"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"if-none-match", "\"1234567890\""}}); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders( + Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-length", "123"}}, true); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_EQ( + "123", + response->headers().get(Http::LowerCaseString("content-length"))[0]->value().getStringView()); +} + // Tests missing headers needed for H/1 codec first line. TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { if (upstreamProtocol() == Http::CodecType::HTTP3) { @@ -3158,4 +3179,54 @@ TEST_P(ProtocolIntegrationTest, FragmentStrippedFromPathWithOverride) { EXPECT_EQ("200", response->headers().getStatusValue()); } +TEST_P(DownstreamProtocolIntegrationTest, ContentLengthSmallerThanPayload) { + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = + codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"content-length", "123"}}, + 1024); + if (downstreamProtocol() == Http::CodecType::HTTP1) { + waitForNextUpstreamRequest(); + // HTTP/1.x requests get the payload length from Content-Length header. The remaining bytes is + // parsed as another request. + EXPECT_EQ(123u, upstream_request_->body().length()); + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_TRUE(response->complete()); + } else { + // Inconsistency in content-length header and the actually body length should be treated as a + // stream error. + ASSERT_TRUE(response->waitForReset()); + EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason()); + } +} + +TEST_P(DownstreamProtocolIntegrationTest, ContentLengthLargerThanPayload) { + if (downstreamProtocol() == Http::CodecType::HTTP1) { + // HTTP/1.x request rely on Content-Length header to determine payload length. So there is no + // inconsistency but the request will hang there waiting for the rest bytes. + return; + } + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = + codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"content-length", "1025"}}, + 1024); + + // Inconsistency in content-length header and the actually body length should be treated as a + // stream error. + ASSERT_TRUE(response->waitForReset()); + EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason()); +} + } // namespace Envoy