From 390aa32199a10dbf290e3dac4eecc0458ca024db Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 30 Mar 2021 17:29:47 -0400 Subject: [PATCH] quic: turn up status tests (#15648) Tagging a bunch of quic test failures with more details. Adapting the status code tests to QUIC and fixing QUIC code to validate. minor refactors to core code. Risk Level: low (minor core refactors) Testing: new integration tests Docs Changes: n/a Release Notes: n/a part of #14829 Signed-off-by: Alyssa Wilk --- source/common/http/utility.cc | 10 ++- source/common/http/utility.h | 7 ++ .../common/quic/envoy_quic_client_stream.cc | 13 +++- source/common/quic/envoy_quic_utils.cc | 2 + .../quic/envoy_quic_client_stream_test.cc | 2 +- test/integration/protocol_integration_test.cc | 76 ++++++++++--------- 6 files changed, 70 insertions(+), 40 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 6ba08a599446..e10ab97fe0a2 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -442,10 +442,18 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin } uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) { + auto status = Utility::getResponseStatusNoThrow(headers); + if (!status.has_value()) { + throw CodecClientException(":status must be specified and a valid unsigned long"); + } + return status.value(); +} + +absl::optional Utility::getResponseStatusNoThrow(const ResponseHeaderMap& headers) { const HeaderEntry* header = headers.Status(); uint64_t response_code; if (!header || !absl::SimpleAtoi(headers.getStatusValue(), &response_code)) { - throw CodecClientException(":status must be specified and a valid unsigned long"); + return absl::nullopt; } return response_code; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2af7e49e0a4b..27f3300e777f 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -259,6 +259,13 @@ std::string makeSetCookieValue(const std::string& key, const std::string& value, */ uint64_t getResponseStatus(const ResponseHeaderMap& headers); +/** + * Get the response status from the response headers. + * @param headers supplies the headers to get the status from. + * @return absl::optional the response code or absl::nullopt if the headers are invalid. + */ +absl::optional getResponseStatusNoThrow(const ResponseHeaderMap& headers); + /** * Determine whether these headers are a valid Upgrade request or response. * This function returns true if the following HTTP headers and values are present: diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index feb2b81bcfe9..b9aae50c0c24 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -126,7 +126,10 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, return; } quic::QuicSpdyStream::OnInitialHeadersComplete(fin, frame_len, header_list); - ASSERT(headers_decompressed() && !header_list.empty()); + if (!headers_decompressed() || header_list.empty()) { + Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + return; + } ENVOY_STREAM_LOG(debug, "Received headers: {}.", *this, header_list.DebugString()); if (fin) { @@ -134,7 +137,13 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, } std::unique_ptr headers = quicHeadersToEnvoyHeaders(header_list); - const uint64_t status = Http::Utility::getResponseStatus(*headers); + const absl::optional optional_status = + Http::Utility::getResponseStatusNoThrow(*headers); + if (!optional_status.has_value()) { + Reset(quic::QUIC_BAD_APPLICATION_PAYLOAD); + return; + } + const uint64_t status = optional_status.value(); if (Http::CodeUtility::is1xx(status)) { if (status == enumToInt(Http::Code::SwitchingProtocols)) { // HTTP3 doesn't support the HTTP Upgrade mechanism or 101 (Switching Protocols) status code. diff --git a/source/common/quic/envoy_quic_utils.cc b/source/common/quic/envoy_quic_utils.cc index 3a1574763b18..cf2d4c83b9a9 100644 --- a/source/common/quic/envoy_quic_utils.cc +++ b/source/common/quic/envoy_quic_utils.cc @@ -82,6 +82,8 @@ Http::StreamResetReason quicRstErrorToEnvoyLocalResetReason(quic::QuicRstStreamE return Http::StreamResetReason::LocalRefusedStreamReset; case quic::QUIC_STREAM_CONNECTION_ERROR: return Http::StreamResetReason::ConnectionFailure; + case quic::QUIC_BAD_APPLICATION_PAYLOAD: + return Http::StreamResetReason::ProtocolError; default: return Http::StreamResetReason::LocalReset; } diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 5f31ea232e95..62acbef678b6 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -253,7 +253,7 @@ TEST_P(EnvoyQuicClientStreamTest, ResetUpon101SwitchProtocol) { const auto result = quic_stream_->encodeHeaders(request_headers_, false); EXPECT_TRUE(result.ok()); - EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::LocalReset, _)); + EXPECT_CALL(stream_callbacks_, onResetStream(Http::StreamResetReason::ProtocolError, _)); // Receive several 10x headers, only the first 100 Continue header should be // delivered. if (quic::VersionUsesHttp3(quic_version_.transport_version)) { diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index aa05eaba4388..1e5687ae79f9 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -341,7 +341,7 @@ TEST_P(ProtocolIntegrationTest, ResponseWithHostHeader) { // Tests missing headers needed for H/1 codec first line. TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) { - EXCLUDE_UPSTREAM_HTTP3; // buffer bug. + EXCLUDE_UPSTREAM_HTTP3; // tsan flake? useAccessLog("%RESPONSE_CODE_DETAILS%"); config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " "type.googleapis.com/google.protobuf.Empty } }"); @@ -377,7 +377,7 @@ TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) { // TODO(danzh) re-enable after plumbing through http2 option // "allow_connect". EXCLUDE_DOWNSTREAM_HTTP3; - EXCLUDE_UPSTREAM_HTTP3; // buffer bug. + EXCLUDE_UPSTREAM_HTTP3; // use after free. // Faulty filter that removed host in a CONNECT request. config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& @@ -1051,7 +1051,7 @@ TEST_P(ProtocolIntegrationTest, HittingEncoderFilterLimit) { // connection error is not detected under these circumstances. #if !defined(__APPLE__) TEST_P(ProtocolIntegrationTest, 100ContinueAndClose) { - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // CI fails with 503 in access logs. testEnvoyHandling100Continue(false, "", true); } #endif @@ -1081,7 +1081,8 @@ TEST_P(ProtocolIntegrationTest, EnvoyProxyingLateMultiple1xx) { TEST_P(ProtocolIntegrationTest, TwoRequests) { testTwoRequests(); } TEST_P(ProtocolIntegrationTest, TwoRequestsWithForcedBackup) { - EXCLUDE_UPSTREAM_HTTP3; // Hits assert in switchStreamBlockState. + // hits assert "Upper stream buffer limit is reached before response body is delivered." + EXCLUDE_UPSTREAM_HTTP3; testTwoRequests(true); } @@ -1260,57 +1261,54 @@ TEST_P(ProtocolIntegrationTest, 304WithBody) { } TEST_P(ProtocolIntegrationTest, OverflowingResponseCode) { - EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing. + useAccessLog("%RESPONSE_CODE_DETAILS%"); initialize(); - FakeRawConnectionPtr fake_upstream_connection; - // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames codec_client_ = makeHttpConnection(lookupPort("http")); auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); - ASSERT(fake_upstream_connection != nullptr); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + // The http1 codec won't send illegal status codes so send raw HTTP/1.1 + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); ASSERT_TRUE(fake_upstream_connection->write( "HTTP/1.1 11111111111111111111111111111111111111111111111111111111111111111 OK\r\n", false)); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); } else { - Http::Http2::Http2Frame::SettingsFlags settings_flags = - static_cast(0); - Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); - // Ack settings - settings_flags = static_cast(1); // ack - Http2Frame ack_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); - ASSERT(fake_upstream_connection->write(std::string(setting_frame))); // empty settings - ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting - Http::Http2::Http2Frame overflowed_status = Http::Http2::Http2Frame::makeHeadersFrameWithStatus( - "11111111111111111111111111111111111111111111111111111111111111111", 0); - ASSERT_TRUE(fake_upstream_connection->write(std::string(overflowed_status))); + waitForNextUpstreamRequest(); + default_response_headers_.setStatus( + "11111111111111111111111111111111111111111111111111111111111111111"); + upstream_request_->encodeHeaders(default_response_headers_, false); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) { + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } else { + // TODO(#14829) QUIC won't disconnect on invalid messaging. + ASSERT_TRUE(upstream_request_->waitForReset()); + } } - ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); response->waitForEndStream(); EXPECT_EQ("502", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("protocol_error")); } TEST_P(ProtocolIntegrationTest, MissingStatus) { - EXCLUDE_UPSTREAM_HTTP3; // Protocol-specific framing. initialize(); - FakeRawConnectionPtr fake_upstream_connection; // HTTP1, uses a defined protocol which doesn't split up messages into raw byte frames codec_client_ = makeHttpConnection(lookupPort("http")); auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); - ASSERT(fake_upstream_connection != nullptr); - if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT(fake_upstream_connection != nullptr); ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 OK\r\n", false)); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); - } else { + } else if (upstreamProtocol() == FakeHttpConnection::Type::HTTP2) { + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); Http::Http2::Http2Frame::SettingsFlags settings_flags = static_cast(0); Http2Frame setting_frame = Http::Http2::Http2Frame::makeEmptySettingsFrame(settings_flags); @@ -1321,9 +1319,15 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) { ASSERT(fake_upstream_connection->write(std::string(ack_frame))); // ack setting Http::Http2::Http2Frame missing_status = Http::Http2::Http2Frame::makeHeadersFrameNoStatus(0); ASSERT_TRUE(fake_upstream_connection->write(std::string(missing_status))); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + } else { + waitForNextUpstreamRequest(); + default_response_headers_.removeStatus(); + upstream_request_->encodeHeaders(default_response_headers_, false); + // TODO(#14829) QUIC won't disconnect on invalid messaging. + ASSERT_TRUE(upstream_request_->waitForReset()); } - ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); response->waitForEndStream(); EXPECT_EQ("502", response->headers().getStatusValue()); } @@ -1332,7 +1336,7 @@ TEST_P(ProtocolIntegrationTest, MissingStatus) { TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) { // TODO(danzh) re-enable this test after quic headers size become configurable. EXCLUDE_DOWNSTREAM_HTTP3 - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -1357,7 +1361,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingConcatenated) { TEST_P(DownstreamProtocolIntegrationTest, LargeCookieParsingMany) { // TODO(danzh) re-enable this test after quic headers size become configurable. EXCLUDE_DOWNSTREAM_HTTP3 - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // QUIC_STREAM_EXCESSIVE_LOAD // Set header count limit to 2010. uint32_t max_count = 2010; config_helper_.addConfigModifier( @@ -1570,7 +1574,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlRejected) { TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlAccepted) { // TODO(danzh) re-enable this test after quic headers size become configurable. EXCLUDE_DOWNSTREAM_HTTP3 - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits. // Send one 95 kB URL with limit 96 kB headers. testLargeRequestUrl(95, 96); } @@ -1582,7 +1586,7 @@ TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) { TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersAccepted) { EXCLUDE_DOWNSTREAM_HTTP3 - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // requires configurable header limits. // Send one 95 kB header with limit 96 kB and 100 headers. testLargeRequestHeaders(95, 1, 96, 100); } @@ -1602,7 +1606,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) { TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { // QUICHE doesn't limit number of headers. EXCLUDE_DOWNSTREAM_HTTP3 - EXCLUDE_UPSTREAM_HTTP3; // buffer bug. + EXCLUDE_UPSTREAM_HTTP3; // CI asan use-after-free // Default header (and trailer) count limit is 100. config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); @@ -1630,7 +1634,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { } TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) { - EXCLUDE_UPSTREAM_HTTP3; + EXCLUDE_UPSTREAM_HTTP3; // assert failure: validHeaderString // Set header (and trailer) count limit to 200. uint32_t max_count = 200; config_helper_.addConfigModifier(