From dd36508d6ca4e4e44d0c7b75364fb445761e2cd6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 9 Apr 2020 09:28:47 -0400 Subject: [PATCH 01/12] http: downstream connect support Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 21 +- source/common/http/header_utility.cc | 5 - source/common/http/http1/codec_impl.cc | 32 ++- source/common/http/http1/codec_impl.h | 4 + source/common/http/http2/codec_impl.cc | 4 + source/common/http/utility.cc | 3 +- source/common/runtime/runtime_features.cc | 1 - test/common/http/conn_manager_impl_test.cc | 23 ++ test/common/http/http1/codec_impl_test.cc | 129 ++++++++++ test/common/http/http2/codec_impl_test.cc | 20 ++ test/common/http/utility_test.cc | 5 + test/integration/BUILD | 2 + test/integration/integration_test.cc | 113 +++++++++ test/integration/protocol_integration_test.cc | 2 +- .../tcp_tunneling_integration_test.cc | 230 +++++++++++++++++- 15 files changed, 579 insertions(+), 15 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2b3946187846..e81eb5cf5c5b 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -857,6 +857,16 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // Verify header sanity checks which should have been performed by the codec. ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); + // For CONNECT requests there may not be a path. Add a placeholder path just so filters don't have + // to special case. + // TODO(alyssawilk, mattklein123) do we want to require path? It'd probably cause interop + // problems. + // TODO(alyssawilk, mattklein123) do we need to strip outbound or is adding one OK? + if (Http::Headers::get().MethodValues.Connect == + request_headers_->Method()->value().getStringView() && + !request_headers_->Path()) { + request_headers_->setPath("/"); + } // Currently we only support relative paths at the application layer. We expect the codec to have // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this // when the allow_absolute_url flag is enabled on the HCM. @@ -921,9 +931,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // TODO if there are no filters when starting a filter iteration, the connection manager // should return 404. The current returns no response if there is no router filter. - if (protocol == Protocol::Http11 && hasCachedRoute()) { + if (hasCachedRoute()) { if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. + state_.saw_connection_close_ = true; + // TODO(alyssawilk) can we rename this or should we just comment that it's + // for all upgrades? connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", nullptr, state_.is_head_request_, absl::nullopt, @@ -2004,6 +2017,12 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() { } bool upgrade_rejected = false; auto upgrade = request_headers_ ? request_headers_->Upgrade() : nullptr; + // Treat CONNECT requests as a special upgrade case. + if (!upgrade && request_headers_ && request_headers_->Method() && + Http::Headers::get().MethodValues.Connect == + request_headers_->Method()->value().getStringView()) { + upgrade = request_headers_->Method(); + } state_.created_filter_chain_ = true; if (upgrade != nullptr) { const Router::RouteEntry::UpgradeMap* upgrade_map = nullptr; diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 9772f3556683..5e6aae992bdc 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -183,11 +183,6 @@ HeaderUtility::requestHeadersValid(const RequestHeaderMap& headers) { headers.Host() && !HeaderUtility::authorityIsValid(headers.Host()->value().getStringView())) { return SharedResponseCodeDetails::get().InvalidAuthority; } - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_method_validation") && - headers.Method() && - Http::Headers::get().MethodValues.Connect == headers.Method()->value().getStringView()) { - return SharedResponseCodeDetails::get().ConnectUnsupported; - } return absl::nullopt; } diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 85efa2c11102..3cee92ea70db 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -57,6 +57,7 @@ HeaderKeyFormatterPtr formatter(const Http::Http1Settings& settings) { return nullptr; } + } // namespace const std::string StreamEncoderImpl::CRLF = "\r\n"; @@ -67,7 +68,8 @@ StreamEncoderImpl::StreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter) : connection_(connection), disable_chunk_encoding_(false), chunk_encoding_(true), processing_100_continue_(false), is_response_to_head_request_(false), - is_content_length_allowed_(true), header_key_formatter_(header_key_formatter) { + is_response_to_connect_request_(false), is_content_length_allowed_(true), + header_key_formatter_(header_key_formatter) { if (connection_.connection().aboveHighWatermark()) { runHighWatermarkCallbacks(); } @@ -172,7 +174,8 @@ void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& head // // When sending a response to a HEAD request Envoy may send an informational // "Transfer-Encoding: chunked" header, but should not send a chunk encoded body. - chunk_encoding_ = !Utility::isUpgrade(headers) && !is_response_to_head_request_; + chunk_encoding_ = !Utility::isUpgrade(headers) && !is_response_to_head_request_ && + !is_response_to_connect_request_; } } @@ -342,6 +345,10 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e // set is_content_length_allowed_ back to true. setIsContentLengthAllowed(true); } + if (numeric_status >= 300) { + // Don't do special CONNECT logic if the CONNECT was rejected. + is_response_to_connect_request_ = false; + } encodeHeadersBase(headers, end_stream); } @@ -351,12 +358,17 @@ static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); + if (!method || !path) { throw CodecClientException(":method and :path must be specified"); } if (method->value() == Headers::get().MethodValues.Head) { head_request_ = true; + } else if (method->value() == Headers::get().MethodValues.Connect) { + disableChunkEncoding(); + connect_request_ = true; } + connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); @@ -604,6 +616,10 @@ int ConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } } + if (parser_.method == HTTP_CONNECT) { + ENVOY_CONN_LOG(trace, "codec entering upgrade mode for CONNECT request.", connection_); + handling_upgrade_ = true; + } // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject // transfer-codings it does not understand. @@ -786,9 +802,8 @@ int ServerConnectionImpl::onHeadersComplete() { // Inform the response encoder about any HEAD method, so it can set content // length and transfer encoding headers correctly. active_request.response_encoder_.isResponseToHeadRequest(parser_.method == HTTP_HEAD); + active_request.response_encoder_.isResponseToConnectRequest(parser_.method == HTTP_CONNECT); - // Currently, CONNECT is not supported, however; http_parser_parse_url needs to know about - // CONNECT handlePath(*headers, parser_.method); ASSERT(active_request.request_url_.empty()); @@ -854,6 +869,7 @@ void ServerConnectionImpl::onBody(Buffer::Instance& data) { } void ServerConnectionImpl::onMessageComplete() { + ASSERT(!handling_upgrade_); if (active_request_.has_value()) { auto& active_request = active_request_.value(); active_request.remote_complete_ = true; @@ -986,6 +1002,12 @@ int ClientConnectionImpl::onHeadersComplete() { ENVOY_CONN_LOG(trace, "Client: onHeadersComplete size={}", connection_, headers->size()); headers->setStatus(parser_.status_code); + if (parser_.status_code >= 200 && parser_.status_code < 300 && + pending_response_.value().encoder_.connectRequest()) { + ENVOY_CONN_LOG(trace, "codec entering upgrade mode for CONNECT response.", connection_); + handling_upgrade_ = true; + } + if (parser_.status_code == 100) { // http-parser treats 100 continue headers as their own complete response. // Swallow the spurious onMessageComplete and continue processing. @@ -995,7 +1017,7 @@ int ClientConnectionImpl::onHeadersComplete() { // Reset to ensure no information from the continue headers is used for the response headers // in case the callee does not move the headers out. headers_or_trailers_.emplace(nullptr); - } else if (cannotHaveBody()) { + } else if (cannotHaveBody() && !handling_upgrade_) { deferred_end_stream_headers_ = true; } else { pending_response_.value().decoder_->decodeHeaders(std::move(headers), false); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index cdc651c19766..e535ec21183e 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -72,6 +72,7 @@ class StreamEncoderImpl : public virtual StreamEncoder, const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override; void isResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; } + void isResponseToConnectRequest(bool value) { is_response_to_connect_request_ = value; } void setDetails(absl::string_view details) { details_ = details; } protected: @@ -88,6 +89,7 @@ class StreamEncoderImpl : public virtual StreamEncoder, bool chunk_encoding_ : 1; bool processing_100_continue_ : 1; bool is_response_to_head_request_ : 1; + bool is_response_to_connect_request_ : 1; bool is_content_length_allowed_ : 1; private: @@ -145,6 +147,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { RequestEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter) : StreamEncoderImpl(connection, header_key_formatter) {} bool headRequest() { return head_request_; } + bool connectRequest() { return connect_request_; } // Http::RequestEncoder void encodeHeaders(const RequestHeaderMap& headers, bool end_stream) override; @@ -152,6 +155,7 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { private: bool head_request_{}; + bool connect_request_{}; }; /** diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 2d54b0b1a413..e28007c7837a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -149,6 +149,10 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea upgrade_type_ = std::string(headers.Upgrade()->value().getStringView()); Http::Utility::transformUpgradeRequestFromH1toH2(*modified_headers); buildHeaders(final_headers, *modified_headers); + } else if (headers.Method() && headers.Method()->value() == "CONNECT") { + modified_headers = createHeaderMap(headers); + modified_headers->setProtocol("bytestream"); + buildHeaders(final_headers, *modified_headers); } else { buildHeaders(final_headers, headers); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index fe9393574a32..d2cd3e9eff2a 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -382,7 +382,8 @@ bool Utility::isUpgrade(const RequestOrResponseHeaderMap& headers) { bool Utility::isH2UpgradeRequest(const RequestHeaderMap& headers) { return headers.Method() && headers.Method()->value().getStringView() == Http::Headers::get().MethodValues.Connect && - headers.Protocol() && !headers.Protocol()->value().empty(); + headers.Protocol() && !headers.Protocol()->value().empty() && + headers.Protocol()->value() != "bytestream"; } bool Utility::isWebSocketUpgradeRequest(const RequestHeaderMap& headers) { diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a78868b3ff3c..2589e08c8605 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -30,7 +30,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.connection_header_sanitization", "envoy.reloadable_features.strict_authority_validation", "envoy.reloadable_features.reject_unsupported_transfer_encodings", - "envoy.reloadable_features.strict_method_validation", "envoy.reloadable_features.new_http1_connection_pool_behavior", "envoy.reloadable_features.new_http2_connection_pool_behavior", "envoy.deprecated_features.allow_deprecated_extension_names", diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a820323b4286..49e80a51ab4d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -2564,6 +2564,29 @@ TEST_F(HttpConnectionManagerImplTest, FooUpgradeDrainClose) { conn_manager_->onData(fake_input, false); } +// Make sure CONNECT requests hit the upgrade filter path. +TEST_F(HttpConnectionManagerImplTest, ConnectAsUpgrade) { + setup(false, "envoy-custom-server", false); + + NiceMock encoder; + RequestDecoder* decoder = nullptr; + + EXPECT_CALL(filter_factory_, createUpgradeFilterChain("CONNECT", _, _)) + .WillRepeatedly(Return(true)); + + EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + decoder = &conn_manager_->newStream(encoder); + RequestHeaderMapPtr headers{ + new TestRequestHeaderMapImpl{{":authority", "host"}, {":method", "CONNECT"}}}; + decoder->decodeHeaders(std::move(headers), false); + data.drain(4); + })); + + // Kick off the incoming data. Use extra data which should cause a redispatch. + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); +} + // Regression test for https://github.com/envoyproxy/envoy/issues/10138 TEST_F(HttpConnectionManagerImplTest, DrainCloseRaceWithClose) { InSequence s; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 0c26f621f9e8..04ca96062048 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1537,6 +1537,70 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithNoBody) { codec_->dispatch(buffer); } +TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); + Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + + Buffer::OwnedImpl expected_data("abcd"); + Buffer::OwnedImpl connect_payload("abcd"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + codec_->dispatch(connect_payload); +} + +TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithEarlyData) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl expected_data("abcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\n\r\nabcd"); + codec_->dispatch(buffer); +} + +TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithTEChunked) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + // Connect with body is technically illegal, but Envoy does not inspect the + // body to see if there is a non-zero byte chunk. It will instead pass it + // through. + Buffer::OwnedImpl expected_data("12345abcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n12345abcd"); + codec_->dispatch(buffer); +} + +TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithContentLength) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + // Make sure we avoid the deferred_end_stream_headers_ optimization for + // requests-with-no-body. + Buffer::OwnedImpl expected_data("abcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\ncontent-length: 0\r\n\r\nabcd"); + codec_->dispatch(buffer); +} + TEST_F(Http1ServerConnectionImplTest, WatermarkTest) { EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); @@ -1865,6 +1929,71 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponseWithEarlyData) { codec_->dispatch(response); } +TEST_F(Http1ClientConnectionImplTest, ConnectResponse) { + initialize(); + + InSequence s; + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + // Send response headers + EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"); + codec_->dispatch(response); + + // Send body payload + Buffer::OwnedImpl expected_data1("12345"); + Buffer::OwnedImpl body("12345"); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + codec_->dispatch(body); + + // Send connect payload + Buffer::OwnedImpl expected_data2("abcd"); + Buffer::OwnedImpl connect_payload("abcd"); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); + codec_->dispatch(connect_payload); +} + +// Same data as above, but make sure directDispatch immediately hands off any +// outstanding data. +TEST_F(Http1ClientConnectionImplTest, ConnectResponseWithEarlyData) { + initialize(); + + InSequence s; + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + // Send response headers and payload + EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); + Buffer::OwnedImpl expected_data("12345abcd"); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\n\r\n12345abcd"); + codec_->dispatch(response); +} + +TEST_F(Http1ClientConnectionImplTest, ConnectRejected) { + initialize(); + + InSequence s; + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); + Buffer::OwnedImpl expected_data("12345abcd"); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + Buffer::OwnedImpl response("HTTP/1.1 400 OK\r\n\r\n12345abcd"); + codec_->dispatch(response); +} + TEST_F(Http1ClientConnectionImplTest, WatermarkTest) { EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 69c3e47db3cc..e4dec9210970 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1779,6 +1779,26 @@ TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) { EXPECT_NO_THROW(server_wrapper_.dispatch(data, *server_)); } +// CONNECT without upgrade type gets tagged with "bytestream" +TEST_P(Http2CodecImplTest, ConnectTest) { + client_http2_options_.set_allow_connect(true); + server_http2_options_.set_allow_connect(true); + initialize(); + MockStreamCallbacks callbacks; + request_encoder_->getStream().addCallbacks(callbacks); + + TestRequestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + request_headers.setReferenceKey(Headers::get().Method, Http::Headers::get().MethodValues.Connect); + TestRequestHeaderMapImpl expected_headers; + HttpTestUtility::addDefaultHeaders(expected_headers); + expected_headers.setReferenceKey(Headers::get().Method, + Http::Headers::get().MethodValues.Connect); + expected_headers.setReferenceKey(Headers::get().Protocol, "bytestream"); + EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + request_encoder_->encodeHeaders(request_headers, false); +} + } // namespace Http2 } // namespace Http } // namespace Envoy diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 8c5c4d55efb6..85b99c718276 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -123,6 +123,11 @@ TEST(HttpUtility, H2H1H2Request) { ASSERT_EQ(converted_headers, original_headers); } +TEST(HttpUtility, ConnectBytestreamSpecialCased) { + TestRequestHeaderMapImpl headers = {{":method", "CONNECT"}, {":protocol", "bytestream"}}; + ASSERT_FALSE(Utility::isH2UpgradeRequest(headers)); +} + // Start with H1 style websocket response headers. Transform to H2 and back. TEST(HttpUtility, H1H2H1Response) { TestResponseHeaderMapImpl converted_headers = { diff --git a/test/integration/BUILD b/test/integration/BUILD index 5ef3aa5d2a43..6eedd177d678 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -578,6 +578,7 @@ envoy_cc_test( "integration_test.cc", "integration_test.h", ], + shard_count = 2, deps = [ ":http_integration_lib", "//source/common/http:header_map_lib", @@ -888,6 +889,7 @@ envoy_cc_test( ], deps = [ ":http_integration_lib", + ":http_protocol_integration_lib", "//source/extensions/filters/network/tcp_proxy:config", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/filter/network/tcp_proxy/v2:pkg_cc_proto", diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index c18599476f5f..31027e45a059 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -25,6 +25,7 @@ using Envoy::Http::Headers; using Envoy::Http::HeaderValueOf; using Envoy::Http::HttpStatusIs; +using testing::Contains; using testing::EndsWith; using testing::HasSubstr; using testing::Not; @@ -1254,4 +1255,116 @@ TEST_P(IntegrationTest, TestManyBadRequests) { EXPECT_EQ(0, test_server_->counter("http1.response_flood")->value()); } +TEST_P(IntegrationTest, ConnectWithNoBody) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); + }); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + tcp_client->write("CONNECT / HTTP/1.1\r\nHost: host\r\n\r\n", false); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + EXPECT_TRUE(absl::StartsWith(data, "CONNECT / HTTP/1.1")); + // No transfer-encoding: chunked or connection: close + EXPECT_FALSE(absl::StrContains(data, "hunked")) << data; + EXPECT_FALSE(absl::StrContains(data, "onnection")) << data; + + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 200 OK\r\nContent-length: 0\r\n\r\n")); + tcp_client->waitForData("\r\n\r\n", false); + EXPECT_TRUE(absl::StartsWith(tcp_client->data(), "HTTP/1.1 200 OK\r\n")) << tcp_client->data(); + // Make sure the following payload is proxied without chunks or any other modifications. + tcp_client->write("payload"); + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\npayload"), &data)); + + ASSERT_TRUE(fake_upstream_connection->write("return-payload")); + tcp_client->waitForData("\r\n\r\nreturn-payload", false); + + tcp_client->close(); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); +} + +TEST_P(IntegrationTest, DISABLED_ConnectWithChunkedBody) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); + }); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + tcp_client->write("CONNECT / HTTP/1.1\r\nHost: host\r\n\r\n", false); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + EXPECT_TRUE(absl::StartsWith(data, "CONNECT / HTTP/1.1")); + // No transfer-encoding: chunked or connection: close + EXPECT_FALSE(absl::StrContains(data, "hunked")) << data; + EXPECT_FALSE(absl::StrContains(data, "onnection")) << data; + + ASSERT_TRUE(fake_upstream_connection->write( + "HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n")); + tcp_client->waitForData("0\r\n\r\n", false); + EXPECT_TRUE(absl::StartsWith(tcp_client->data(), "HTTP/1.1 200 OK\r\n")); + EXPECT_TRUE(absl::StrContains(tcp_client->data(), "hunked")) << tcp_client->data(); + EXPECT_TRUE(absl::StrContains(tcp_client->data(), "\r\n\r\nb\r\nHello World\r\n0\r\n\r\n")) + << tcp_client->data(); + + // Make sure the following payload is proxied without chunks or any other modifications. + tcp_client->write("payload"); + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\npayload"))); + + ASSERT_TRUE(fake_upstream_connection->write("return-payload")); + tcp_client->waitForData("\r\n\r\nreturn-payload", false); + + tcp_client->close(); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); +} + +TEST_P(IntegrationTest, DISABLED_ConnectRejectionWithChunkedBody) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); + }); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + tcp_client->write("CONNECT / HTTP/1.1\r\nHost: host\r\n\r\n", false); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + EXPECT_TRUE(absl::StartsWith(data, "CONNECT / HTTP/1.1")); + // No transfer-encoding: chunked or connection: close + EXPECT_FALSE(absl::StrContains(data, "hunked")) << data; + EXPECT_FALSE(absl::StrContains(data, "onnection")) << data; + + ASSERT_TRUE(fake_upstream_connection->write( + "HTTP/1.1 403 NO\r\ntransfer-encoding: chunked\r\n\r\nb\r\nNo connects\r\n0\r\n\r\n")); + tcp_client->waitForData("0\r\n\r\n", false); + EXPECT_TRUE(absl::StartsWith(tcp_client->data(), "HTTP/1.1 403 Forbidden\r\n")); + EXPECT_TRUE(absl::StrContains(tcp_client->data(), "hunked")) << tcp_client->data(); + EXPECT_TRUE(absl::StrContains(tcp_client->data(), "\r\n\r\nb\r\nNo connects\r\n0\r\n\r\n")) + << tcp_client->data(); + + tcp_client->waitForDisconnect(); +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d051884e2629..036ceed371a2 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1633,7 +1633,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) { if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { response->waitForEndStream(); - EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + EXPECT_EQ("403", response->headers().Status()->value().getStringView()); EXPECT_TRUE(response->complete()); } else { response->waitForReset(); diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index a7c54d68ce05..6ed9ea43d921 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -4,12 +4,239 @@ #include "envoy/config/filter/network/tcp_proxy/v2/tcp_proxy.pb.h" #include "test/integration/http_integration.h" +#include "test/integration/http_protocol_integration.h" #include "gtest/gtest.h" namespace Envoy { namespace { +// Terminating CONNECT and sending raw TCP upstream. +class ConnectTerminationIntegrationTest + : public testing::TestWithParam, + public HttpIntegrationTest { +public: + ConnectTerminationIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) { + enable_half_close_ = true; + } + + void initialize() override { + auto host = config_helper_.createVirtualHost("host", "/"); + // host.mutable_proxying_config(); + config_helper_.addVirtualHost(host); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); + + if (enable_timeout_) { + hcm.mutable_stream_idle_timeout()->set_seconds(0); + hcm.mutable_stream_idle_timeout()->set_nanos(200 * 1000 * 1000); + } + }); + HttpIntegrationTest::initialize(); + } + + void setUpConnection() { + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(connect_headers_); + request_encoder_ = &encoder_decoder.first; + response_ = std::move(encoder_decoder.second); + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_raw_upstream_connection_)); + } + + void sendBidirectionalData(const char* downstream_send_data = "hello", + const char* upstream_received_data = "hello", + const char* upstream_send_data = "there!", + const char* downstream_received_data = "there!") { + // Send some data upstream. + codec_client_->sendData(*request_encoder_, downstream_send_data, false); + ASSERT_TRUE(fake_raw_upstream_connection_->waitForData( + FakeRawConnection::waitForInexactMatch(upstream_received_data))); + + // Send some data downstream. + ASSERT_TRUE(fake_raw_upstream_connection_->write(upstream_send_data)); + response_->waitForBodyData(strlen(downstream_received_data)); + EXPECT_EQ(downstream_received_data, response_->body()); + } + + Http::TestRequestHeaderMapImpl connect_headers_{{":method", "CONNECT"}, + {":path", "/"}, + {":protocol", "bytestream"}, + {":scheme", "https"}, + {":authority", "host"}}; + FakeRawConnectionPtr fake_raw_upstream_connection_; + IntegrationStreamDecoderPtr response_; + bool enable_timeout_{}; +}; + +// FIXME make sure that if data is sent with the connect it does not go upstream +// until the 200 headers are sent. +TEST_P(ConnectTerminationIntegrationTest, DISABLED_Basic) { + initialize(); + + setUpConnection(); + sendBidirectionalData("hello", "hello", "there!", "there!"); + // Send a second set of data to make sure for example headers are only sent once. + sendBidirectionalData(",bye", "hello,bye", "ack", "there!ack"); + + // Send an end stream. This should result in half close upstream. + codec_client_->sendData(*request_encoder_, "", true); + ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose()); + + // Now send a FIN from upstream. This should result in clean shutdown downstream. + ASSERT_TRUE(fake_raw_upstream_connection_->close()); + response_->waitForEndStream(); + ASSERT_FALSE(response_->reset()); +} + +TEST_P(ConnectTerminationIntegrationTest, DISABLED_DownstreamClose) { + initialize(); + + setUpConnection(); + sendBidirectionalData(); + + // Tear down by closing the client connection. + codec_client_->close(); + ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose()); +} + +TEST_P(ConnectTerminationIntegrationTest, DISABLED_DownstreamReset) { + initialize(); + + setUpConnection(); + sendBidirectionalData(); + + // Tear down by resetting the client stream. + codec_client_->sendReset(*request_encoder_); + ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose()); +} + +TEST_P(ConnectTerminationIntegrationTest, DISABLED_UpstreamClose) { + initialize(); + + setUpConnection(); + sendBidirectionalData(); + + // Tear down by closing the upstream connection. + ASSERT_TRUE(fake_raw_upstream_connection_->close()); + response_->waitForReset(); +} + +TEST_P(ConnectTerminationIntegrationTest, DISABLED_TestTimeout) { + enable_timeout_ = true; + initialize(); + + setUpConnection(); + + // Wait for the timeout to close the connection. + response_->waitForEndStream(); + ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose()); +} + +TEST_P(ConnectTerminationIntegrationTest, DISABLED_BuggyHeaders) { + initialize(); + // It's possible that the FIN is received before we set half close on the + // upstream connection, so allow unexpected disconnects. + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + // Sending a header-only request is probably buggy, but rather than having a + // special corner case it is treated as a regular half close. + codec_client_ = makeHttpConnection(lookupPort("http")); + response_ = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, + {":path", "/"}, + {":protocol", "bytestream"}, + {":scheme", "https"}, + {":authority", "host"}}); + // If the connection is established (created, set to half close, and then the + // FIN arrives), make sure the FIN arrives, and send a FIN from upstream. + if (fake_upstreams_[0]->waitForRawConnection(fake_raw_upstream_connection_) && + fake_raw_upstream_connection_->connected()) { + ASSERT_TRUE(fake_raw_upstream_connection_->waitForHalfClose()); + ASSERT_TRUE(fake_raw_upstream_connection_->close()); + } + + // Either with early close, or half close, the FIN from upstream should result + // in clean stream teardown. + response_->waitForEndStream(); + ASSERT_FALSE(response_->reset()); +} + +// For this class, forward the CONNECT request upstream +class ProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest { +public: + void initialize() override { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.add_upgrade_configs()->set_upgrade_type("CONNECT"); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); + }); + HttpProtocolIntegrationTest::initialize(); + } + + Http::TestRequestHeaderMapImpl connect_headers_{{":method", "CONNECT"}, + {":path", "/"}, + {":protocol", "bytestream"}, + {":scheme", "https"}, + {":authority", "host"}}; + IntegrationStreamDecoderPtr response_; +}; + +INSTANTIATE_TEST_SUITE_P(Protocols, ProxyingConnectIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +TEST_P(ProxyingConnectIntegrationTest, DISABLED_ProxyConnect) { + initialize(); + + // Send request headers. + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(connect_headers_); + request_encoder_ = &encoder_decoder.first; + response_ = std::move(encoder_decoder.second); + + // Wait for them to arrive upstream. + AssertionResult result = + fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_); + RELEASE_ASSERT(result, result.message()); + result = fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_); + RELEASE_ASSERT(result, result.message()); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Method)->value(), "CONNECT"); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol) == nullptr); + } else { + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Protocol)->value(), + "bytestream"); + } + + // Send response headers + upstream_request_->encodeHeaders(default_response_headers_, false); + + // Wait for them to arrive downstream. + response_->waitForHeaders(); + EXPECT_EQ("200", response_->headers().Status()->value().getStringView()); + + // Make sure that even once the response has started, that data can continue to go upstream. + codec_client_->sendData(*request_encoder_, "hello", false); + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, 5)); + + // Also test upstream to downstream data. + upstream_request_->encodeData(12, false); + response_->waitForBodyData(12); + + cleanupUpstreamAndDownstream(); +} + +INSTANTIATE_TEST_SUITE_P(IpVersions, ConnectTerminationIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +// Tunneling downstream TCP over an upstream HTTP channel. class TcpTunnelingIntegrationTest : public testing::TestWithParam, public HttpIntegrationTest { public: @@ -81,7 +308,7 @@ TEST_P(TcpTunnelingIntegrationTest, InvalidResponseHeaders) { ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); - // Send invalid response headers, and verify that the client disconnects and + // Send invalid response_ headers, and verify that the client disconnects and // upstream gets a stream reset. default_response_headers_.setStatus(enumToInt(Http::Code::ServiceUnavailable)); upstream_request_->encodeHeaders(default_response_headers_, false); @@ -236,5 +463,6 @@ TEST_P(TcpTunnelingIntegrationTest, TcpProxyUpstreamFlush) { INSTANTIATE_TEST_SUITE_P(IpVersions, TcpTunnelingIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); + } // namespace } // namespace Envoy From 0c0601008662a860e276a9c312da4d64d5165a58 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 9 Apr 2020 10:45:52 -0400 Subject: [PATCH 02/12] Fixing spelling on my questions to Matt Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 4 ++-- test/integration/tcp_tunneling_integration_test.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index e81eb5cf5c5b..054b3148e95e 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -859,8 +859,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // For CONNECT requests there may not be a path. Add a placeholder path just so filters don't have // to special case. - // TODO(alyssawilk, mattklein123) do we want to require path? It'd probably cause interop - // problems. + // TODO(alyssawilk, mattklein123) do we want to require path? Alyssa guesses it would cause + // problems for non-Envoy uses to do so but we could launch and iterate // TODO(alyssawilk, mattklein123) do we need to strip outbound or is adding one OK? if (Http::Headers::get().MethodValues.Connect == request_headers_->Method()->value().getStringView() && diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index 6ed9ea43d921..5d06a1ef8486 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -72,8 +72,8 @@ class ConnectTerminationIntegrationTest bool enable_timeout_{}; }; -// FIXME make sure that if data is sent with the connect it does not go upstream -// until the 200 headers are sent. +// TODO(alyssawilk) make sure that if data is sent with the connect it does not go upstream +// until the 200 headers are sent before unhiding ANY config. TEST_P(ConnectTerminationIntegrationTest, DISABLED_Basic) { initialize(); From 5b1a13a7c556e97f880a393f0e0754abf30e1e55 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Apr 2020 10:52:24 -0400 Subject: [PATCH 03/12] altering TODO Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 054b3148e95e..378ef300d466 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -857,11 +857,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // Verify header sanity checks which should have been performed by the codec. ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); - // For CONNECT requests there may not be a path. Add a placeholder path just so filters don't have - // to special case. - // TODO(alyssawilk, mattklein123) do we want to require path? Alyssa guesses it would cause - // problems for non-Envoy uses to do so but we could launch and iterate - // TODO(alyssawilk, mattklein123) do we need to strip outbound or is adding one OK? + // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including + // auditing of empty path headers. if (Http::Headers::get().MethodValues.Connect == request_headers_->Method()->value().getStringView() && !request_headers_->Path()) { From b60e8034e8cd7f2b0f18384da2ba0a86c6c08f25 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Apr 2020 15:58:16 -0400 Subject: [PATCH 04/12] reviewer comments Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 20 ++++++------- source/common/http/header_utility.cc | 4 +++ source/common/http/header_utility.h | 5 ++++ source/common/http/headers.h | 5 ++++ source/common/http/http1/codec_impl.cc | 8 +++--- source/common/http/http1/codec_impl.h | 4 +-- source/common/http/http2/codec_impl.cc | 2 +- source/common/http/utility.cc | 2 +- source/common/tcp_proxy/upstream.cc | 2 +- test/common/http/http1/codec_impl_test.cc | 35 ++++++++++++----------- 10 files changed, 51 insertions(+), 36 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 378ef300d466..2482476f18de 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -858,10 +858,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including - // auditing of empty path headers. - if (Http::Headers::get().MethodValues.Connect == - request_headers_->Method()->value().getStringView() && - !request_headers_->Path()) { + // auditing of empty path headers, and make sure the CONNECT firstline is a host:port. + if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) { request_headers_->setPath("/"); } // Currently we only support relative paths at the application layer. We expect the codec to have @@ -929,8 +927,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // TODO if there are no filters when starting a filter iteration, the connection manager // should return 404. The current returns no response if there is no router filter. if (hasCachedRoute()) { - if (upgrade_rejected) { - // Do not allow upgrades if the route does not support it. + if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. + // While downstream servers should not send upgrade payload without the upgrade being + // accepeted, err on the side of caution and refuse to process any further requests on this + // connection, to avoid a class of HTTP/1.1 smuggling bugs where Upgrade or CONNECT payload + // contians a smuggled HTTP request. state_.saw_connection_close_ = true; // TODO(alyssawilk) can we rename this or should we just comment that it's // for all upgrades? @@ -2013,11 +2014,10 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() { return false; } bool upgrade_rejected = false; - auto upgrade = request_headers_ ? request_headers_->Upgrade() : nullptr; + const Envoy::Http::HeaderEntry* upgrade = + request_headers_ ? request_headers_->Upgrade() : nullptr; // Treat CONNECT requests as a special upgrade case. - if (!upgrade && request_headers_ && request_headers_->Method() && - Http::Headers::get().MethodValues.Connect == - request_headers_->Method()->value().getStringView()) { + if (!upgrade && request_headers_ && HeaderUtility::isConnect(*request_headers_)) { upgrade = request_headers_->Method(); } state_.created_filter_chain_ = true; diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 5e6aae992bdc..123d5d32402c 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -157,6 +157,10 @@ bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { header_value.size()) != 0; } +bool HeaderUtility::isConnect(const RequestHeaderMap& headers) { + return headers.Method() && headers.Method()->value() == Http::Headers::get().MethodValues.Connect; +} + void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) { headers_to_add.iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index e349a2ee97d0..b061a5a31d0e 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -111,6 +111,11 @@ class HeaderUtility { */ static bool authorityIsValid(const absl::string_view authority_value); + /** + * @brief a helper function to determine if the headers represent a CONNECT request. + */ + static bool isConnect(const RequestHeaderMap& headers); + /** * Add headers from one HeaderMap to another * @param headers target where headers will be added diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 2698501edff8..10b87e3da092 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -239,6 +239,11 @@ class HeaderValues { const std::string Trace{"TRACE"}; } MethodValues; + struct { + // per https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02 + const std::string Bytestream{"bytestream"}; + } ProtocolValues; + struct { const std::string Http{"http"}; const std::string Https{"https"}; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 5bed358fba94..2d7ac2d000a2 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -167,8 +167,8 @@ void StreamEncoderImpl::encodeHeadersBase(const RequestOrResponseHeaderMap& head } else { encodeFormattedHeader(Headers::get().TransferEncoding.get(), Headers::get().TransferEncodingValues.Chunked); - // We do not apply chunk encoding for HTTP upgrades. - // If there is a body in a WebSocket Upgrade response, the chunks will be + // We do not apply chunk encoding for HTTP upgrades, including CONNECT style upgrades. + // If there is a body in a response on the upgrade path, the chunks will be // passed through via maybeDirectDispatch so we need to avoid appending // extra chunk boundaries. // @@ -804,8 +804,8 @@ int ServerConnectionImpl::onHeadersComplete() { // Inform the response encoder about any HEAD method, so it can set content // length and transfer encoding headers correctly. - active_request.response_encoder_.isResponseToHeadRequest(parser_.method == HTTP_HEAD); - active_request.response_encoder_.isResponseToConnectRequest(parser_.method == HTTP_CONNECT); + active_request.response_encoder_.setIsResponseToHeadRequest(parser_.method == HTTP_HEAD); + active_request.response_encoder_.setIsResponseToConnectRequest(parser_.method == HTTP_CONNECT); handlePath(*headers, parser_.method); ASSERT(active_request.request_url_.empty()); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index e24cc14e69be..12901e447e9a 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -71,8 +71,8 @@ class StreamEncoderImpl : public virtual StreamEncoder, absl::string_view responseDetails() override { return details_; } const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override; - void isResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; } - void isResponseToConnectRequest(bool value) { is_response_to_connect_request_ = value; } + void setIsResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; } + void setIsResponseToConnectRequest(bool value) { is_response_to_connect_request_ = value; } void setDetails(absl::string_view details) { details_ = details; } protected: diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e28007c7837a..abbe99113f80 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -151,7 +151,7 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea buildHeaders(final_headers, *modified_headers); } else if (headers.Method() && headers.Method()->value() == "CONNECT") { modified_headers = createHeaderMap(headers); - modified_headers->setProtocol("bytestream"); + modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream); buildHeaders(final_headers, *modified_headers); } else { buildHeaders(final_headers, headers); diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index d2cd3e9eff2a..0ce5c22e5623 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -383,7 +383,7 @@ bool Utility::isH2UpgradeRequest(const RequestHeaderMap& headers) { return headers.Method() && headers.Method()->value().getStringView() == Http::Headers::get().MethodValues.Connect && headers.Protocol() && !headers.Protocol()->value().empty() && - headers.Protocol()->value() != "bytestream"; + headers.Protocol()->value() != Headers::get().ProtocolValues.Bytestream; } bool Utility::isWebSocketUpgradeRequest(const RequestHeaderMap& headers) { diff --git a/source/common/tcp_proxy/upstream.cc b/source/common/tcp_proxy/upstream.cc index fcf50e80290d..451a277e0865 100644 --- a/source/common/tcp_proxy/upstream.cc +++ b/source/common/tcp_proxy/upstream.cc @@ -117,7 +117,7 @@ void HttpUpstream::setRequestEncoder(Http::RequestEncoder& request_encoder, bool is_ssl ? Http::Headers::get().SchemeValues.Https : Http::Headers::get().SchemeValues.Http; auto headers = Http::createHeaderMap( {{Http::Headers::get().Method, "CONNECT"}, - {Http::Headers::get().Protocol, "bytestream"}, + {Http::Headers::get().Protocol, Http::Headers::get().ProtocolValues.Bytestream}, {Http::Headers::get().Scheme, scheme}, {Http::Headers::get().Path, "/"}, {Http::Headers::get().Host, hostname_}}); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 8029c32def62..78f72801089d 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -907,7 +907,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAllowed) { {":method", "GET"}, {"foo_bar", "bar"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer(absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo_bar: bar\r\n\r\n")); codec_->dispatch(buffer); @@ -929,7 +929,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderNameWithUnderscoreAreDropped) { {":path", "/"}, {":method", "GET"}, }; - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)).Times(1); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); Buffer::OwnedImpl buffer(absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo_bar: bar\r\n\r\n")); codec_->dispatch(buffer); @@ -1544,13 +1544,13 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) { NiceMock decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\n\r\n"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + Buffer::OwnedImpl buffer("CONNECT host:80 HTTP/1.1\r\n\r\n"); codec_->dispatch(buffer); Buffer::OwnedImpl expected_data("abcd"); Buffer::OwnedImpl connect_payload("abcd"); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); codec_->dispatch(connect_payload); } @@ -1562,9 +1562,9 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithEarlyData) { EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); Buffer::OwnedImpl expected_data("abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\n\r\nabcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + Buffer::OwnedImpl buffer("CONNECT host:80 HTTP/1.1\r\n\r\nabcd"); codec_->dispatch(buffer); } @@ -1579,9 +1579,10 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithTEChunked) { // body to see if there is a non-zero byte chunk. It will instead pass it // through. Buffer::OwnedImpl expected_data("12345abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n12345abcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + Buffer::OwnedImpl buffer( + "CONNECT host:80 HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n12345abcd"); codec_->dispatch(buffer); } @@ -1595,9 +1596,9 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithContentLength) { // Make sure we avoid the deferred_end_stream_headers_ optimization for // requests-with-no-body. Buffer::OwnedImpl expected_data("abcd"); - EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1); - EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); - Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\ncontent-length: 0\r\n\r\nabcd"); + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + Buffer::OwnedImpl buffer("CONNECT host:80 HTTP/1.1\r\ncontent-length: 0\r\n\r\nabcd"); codec_->dispatch(buffer); } @@ -1964,13 +1965,13 @@ TEST_F(Http1ClientConnectionImplTest, ConnectResponse) { // Send body payload Buffer::OwnedImpl expected_data1("12345"); Buffer::OwnedImpl body("12345"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data1), false)); codec_->dispatch(body); // Send connect payload Buffer::OwnedImpl expected_data2("abcd"); Buffer::OwnedImpl connect_payload("abcd"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data2), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data2), false)); codec_->dispatch(connect_payload); } @@ -2006,7 +2007,7 @@ TEST_F(Http1ClientConnectionImplTest, ConnectRejected) { EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); Buffer::OwnedImpl expected_data("12345abcd"); - EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)).Times(1); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); Buffer::OwnedImpl response("HTTP/1.1 400 OK\r\n\r\n12345abcd"); codec_->dispatch(response); } From 74c932599cc939785f92b3ae62deede0d85d180a Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 15 Apr 2020 10:04:42 -0400 Subject: [PATCH 05/12] fixed never-working connect parsing Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 15 ++++----- source/common/http/http1/codec_impl.cc | 29 ++++++++++------- source/common/http/utility.cc | 5 ++- source/common/http/utility.h | 2 +- test/common/http/http1/codec_impl_test.cc | 31 ++++++++++++++++++- test/common/http/utility_test.cc | 15 +++++++++ test/integration/integration_test.cc | 9 +++--- test/integration/protocol_integration_test.cc | 4 +-- 8 files changed, 80 insertions(+), 30 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2482476f18de..a3d74ef9bd0a 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -760,6 +760,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he connection_manager_.read_callbacks_->connection().dispatcher()); request_headers_ = std::move(headers); + // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including + // auditing of empty path headers. + if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) { + request_headers_->setPath("/"); + } + // We need to snap snapped_route_config_ here as it's used in mutateRequestHeaders later. if (connection_manager_.config_.isRoutable()) { if (connection_manager_.config_.routeConfigProvider() != nullptr) { @@ -857,11 +863,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // Verify header sanity checks which should have been performed by the codec. ASSERT(HeaderUtility::requestHeadersValid(*request_headers_).has_value() == false); - // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including - // auditing of empty path headers, and make sure the CONNECT firstline is a host:port. - if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) { - request_headers_->setPath("/"); - } // Currently we only support relative paths at the application layer. We expect the codec to have // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this // when the allow_absolute_url flag is enabled on the HCM. @@ -929,9 +930,9 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he if (hasCachedRoute()) { if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. // While downstream servers should not send upgrade payload without the upgrade being - // accepeted, err on the side of caution and refuse to process any further requests on this + // accepted, err on the side of caution and refuse to process any further requests on this // connection, to avoid a class of HTTP/1.1 smuggling bugs where Upgrade or CONNECT payload - // contians a smuggled HTTP request. + // contains a smuggled HTTP request. state_.saw_connection_close_ = true; // TODO(alyssawilk) can we rename this or should we just comment that it's // for all upgrades? diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 2d7ac2d000a2..2cd62b782def 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -358,8 +358,12 @@ static const char REQUEST_POSTFIX[] = " HTTP/1.1\r\n"; void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end_stream) { const HeaderEntry* method = headers.Method(); const HeaderEntry* path = headers.Path(); + const HeaderEntry* host = headers.Host(); + bool is_connect = HeaderUtility::isConnect(headers); - if (!method || !path) { + if (is_connect && !host) { + throw CodecClientException("Host must be specified for CONNECT requests"); + } else if (!method || (!path && !is_connect)) { throw CodecClientException(":method and :path must be specified"); } if (method->value() == Headers::get().MethodValues.Head) { @@ -374,7 +378,11 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end connection_.copyToBuffer(method->value().getStringView().data(), method->value().size()); connection_.addCharToBuffer(' '); - connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); + if (is_connect) { + connection_.copyToBuffer(host->value().getStringView().data(), host->value().size()); + } else { + connection_.copyToBuffer(path->value().getStringView().data(), path->value().size()); + } connection_.copyToBuffer(REQUEST_POSTFIX, sizeof(REQUEST_POSTFIX) - 1); encodeHeadersBase(headers, end_stream); @@ -743,7 +751,7 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // The url is relative or a wildcard when the method is OPTIONS. Nothing to do here. auto& active_request = active_request_.value(); - if (!active_request.request_url_.getStringView().empty() && + if (!is_connect && !active_request.request_url_.getStringView().empty() && (active_request.request_url_.getStringView()[0] == '/' || ((method == HTTP_OPTIONS) && active_request.request_url_.getStringView()[0] == '*'))) { headers.addViaMove(std::move(path), std::move(active_request.request_url_)); @@ -752,18 +760,15 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // If absolute_urls and/or connect are not going be handled, copy the url and return. // This forces the behavior to be backwards compatible with the old codec behavior. - if (!codec_settings_.allow_absolute_url_) { - headers.addViaMove(std::move(path), std::move(active_request.request_url_)); - return; - } - - if (is_connect) { + // CONNECT "urls" are actually host:port so look like absolute URLs to the above checks. + // Absolute URLS in CONNECT requests will be rejected below by the URL class validation. + if (!codec_settings_.allow_absolute_url_ && !is_connect) { headers.addViaMove(std::move(path), std::move(active_request.request_url_)); return; } Utility::Url absolute_url; - if (!absolute_url.initialize(active_request.request_url_.getStringView())) { + if (!absolute_url.initialize(active_request.request_url_.getStringView(), is_connect)) { sendProtocolError(Http1ResponseCodeDetails::get().InvalidUrl); throw CodecProtocolException("http/1.1 protocol error: invalid url in request line"); } @@ -776,7 +781,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // forward the received Host field-value. headers.setHost(absolute_url.host_and_port()); - headers.setPath(absolute_url.path_and_query_params()); + if (!absolute_url.path_and_query_params().empty()) { + headers.setPath(absolute_url.path_and_query_params()); + } active_request.request_url_.clear(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 0ce5c22e5623..5270c4539b71 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -185,9 +185,8 @@ namespace Http { static const char kDefaultPath[] = "/"; -bool Utility::Url::initialize(absl::string_view absolute_url) { +bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { struct http_parser_url u; - const bool is_connect = false; http_parser_url_init(&u); const int result = http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); @@ -216,7 +215,7 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { if (path_len > 0) { uint64_t path_beginning = u.field_data[UF_HOST].off + host_and_port().length(); path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_len); - } else { + } else if (!is_connect) { path_and_query_params_ = absl::string_view(kDefaultPath, 1); } return true; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2daa237894d1..d2db23db71f0 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -101,7 +101,7 @@ namespace Utility { */ class Url { public: - bool initialize(absl::string_view absolute_url); + bool initialize(absl::string_view absolute_url, bool is_connect_request = false); absl::string_view scheme() { return scheme_; } absl::string_view host_and_port() { return host_and_port_; } absl::string_view path_and_query_params() { return path_and_query_params_; } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 78f72801089d..a865a96a3ead 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1544,7 +1544,11 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) { NiceMock decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - EXPECT_CALL(decoder, decodeHeaders_(_, false)); + TestHeaderMapImpl expected_headers{ + {":authority", "host:80"}, + {":method", "CONNECT"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); Buffer::OwnedImpl buffer("CONNECT host:80 HTTP/1.1\r\n\r\n"); codec_->dispatch(buffer); @@ -1554,6 +1558,19 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) { codec_->dispatch(connect_payload); } +// We use the absolute URL parsing code for CONNECT requests, but it does not +// actually allow absolute URLs. +TEST_F(Http1ServerConnectionImplTest, ConnectRequestAbsoluteURLNotallowed) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer("CONNECT http://host:80 HTTP/1.1\r\n\r\n"); + EXPECT_THROW(codec_->dispatch(buffer), CodecProtocolException); +} + TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithEarlyData) { initialize(); @@ -2012,6 +2029,18 @@ TEST_F(Http1ClientConnectionImplTest, ConnectRejected) { codec_->dispatch(response); } +TEST_F(Http1ClientConnectionImplTest, ConnectWithoutHost) { + initialize(); + + InSequence s; + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}}; + EXPECT_THROW_WITH_MESSAGE(request_encoder.encodeHeaders(headers, true), CodecClientException, + "Host must be specified for CONNECT requests"); +} + TEST_F(Http1ClientConnectionImplTest, WatermarkTest) { EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 85b99c718276..62e8def641e0 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1082,6 +1082,8 @@ TEST(Url, ParsingFails) { EXPECT_FALSE(url.initialize("foo")); EXPECT_FALSE(url.initialize("http://")); EXPECT_FALSE(url.initialize("random_scheme://host.com/path")); + EXPECT_FALSE(url.initialize("http://www.foo.com", true)); + EXPECT_FALSE(url.initialize("foo.com", true)); } void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, @@ -1093,6 +1095,14 @@ void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, EXPECT_EQ(url.path_and_query_params(), expected_path); } +void ValidateConnectUrl(absl::string_view raw_url, absl::string_view expected_host_port) { + Utility::Url url; + ASSERT_TRUE(url.initialize(raw_url, true)) << "Failed to initialize " << raw_url; + EXPECT_TRUE(url.scheme().empty()); + EXPECT_TRUE(url.path_and_query_params().empty()); + EXPECT_EQ(url.host_and_port(), expected_host_port); +} + TEST(Url, ParsingTest) { // Test url with no explicit path (with and without port) ValidateUrl("http://www.host.com", "http", "www.host.com", "/"); @@ -1146,6 +1156,11 @@ TEST(Url, ParsingTest) { "/path?query=param&query2=param2#fragment"); } +TEST(Url, ParsingForConnectTest) { + ValidateConnectUrl("host.com:443", "host.com:443"); + ValidateConnectUrl("host.com:80", "host.com:80"); +} + void validatePercentEncodingEncodeDecode(absl::string_view source, absl::string_view expected_encoded) { EXPECT_EQ(Utility::PercentEncoding::encode(source), expected_encoded); diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 2ad6c294c907..bc5ef53146cf 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1291,14 +1291,14 @@ TEST_P(IntegrationTest, ConnectWithNoBody) { initialize(); IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); - tcp_client->write("CONNECT / HTTP/1.1\r\nHost: host\r\n\r\n", false); + tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\nHost: host\r\n\r\n", false); FakeRawConnectionPtr fake_upstream_connection; ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData( FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); - EXPECT_TRUE(absl::StartsWith(data, "CONNECT / HTTP/1.1")); + EXPECT_TRUE(absl::StartsWith(data, "CONNECT host.com:80 HTTP/1.1")); // No transfer-encoding: chunked or connection: close EXPECT_FALSE(absl::StrContains(data, "hunked")) << data; EXPECT_FALSE(absl::StrContains(data, "onnection")) << data; @@ -1318,7 +1318,7 @@ TEST_P(IntegrationTest, ConnectWithNoBody) { ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); } -TEST_P(IntegrationTest, DISABLED_ConnectWithChunkedBody) { +TEST_P(IntegrationTest, ConnectWithChunkedBody) { config_helper_.addConfigModifier( [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) -> void { @@ -1328,14 +1328,13 @@ TEST_P(IntegrationTest, DISABLED_ConnectWithChunkedBody) { initialize(); IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); - tcp_client->write("CONNECT / HTTP/1.1\r\nHost: host\r\n\r\n", false); + tcp_client->write("CONNECT host.com:80 HTTP/1.1\r\nHost: host\r\n\r\n", false); FakeRawConnectionPtr fake_upstream_connection; ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); std::string data; ASSERT_TRUE(fake_upstream_connection->waitForData( FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); - EXPECT_TRUE(absl::StartsWith(data, "CONNECT / HTTP/1.1")); // No transfer-encoding: chunked or connection: close EXPECT_FALSE(absl::StrContains(data, "hunked")) << data; EXPECT_FALSE(absl::StrContains(data, "onnection")) << data; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 036ceed371a2..b73cc2e6013b 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1628,8 +1628,8 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidAuthority) { TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{ - {":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}}); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", "host.com:80"}}); if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { response->waitForEndStream(); From bdba8ba06e1612186a8e73a98bf11809cc1a07e3 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 15 Apr 2020 11:44:53 -0400 Subject: [PATCH 06/12] cleanup + enabling a now working e2e test Signed-off-by: Alyssa Wilk --- docs/root/configuration/http/http_conn_man/stats.rst | 2 +- source/common/http/conn_manager_impl.cc | 2 -- test/integration/tcp_tunneling_integration_test.cc | 4 ++-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index cc8b9800d8b6..d68abc8ce8e6 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/docs/root/configuration/http/http_conn_man/stats.rst @@ -53,7 +53,7 @@ statistics: downstream_rq_3xx, Counter, Total 3xx responses downstream_rq_4xx, Counter, Total 4xx responses downstream_rq_5xx, Counter, Total 5xx responses - downstream_rq_ws_on_non_ws_route, Counter, Total WebSocket upgrade requests rejected by non WebSocket routes + downstream_rq_ws_on_non_ws_route, Counter, Total upgrade requests rejected by non upgrade routes. This now applies both to WebSocket and non-WebSocket upgrades downstream_rq_time, Histogram, Total time for request and response (milliseconds) downstream_rq_idle_timeout, Counter, Total requests closed due to idle timeout downstream_rq_max_duration_reached, Counter, Total requests closed due to max duration reached diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index a3d74ef9bd0a..c5801e7409d8 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -934,8 +934,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // connection, to avoid a class of HTTP/1.1 smuggling bugs where Upgrade or CONNECT payload // contains a smuggled HTTP request. state_.saw_connection_close_ = true; - // TODO(alyssawilk) can we rename this or should we just comment that it's - // for all upgrades? connection_manager_.stats_.named_.downstream_rq_ws_on_non_ws_route_.inc(); sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::Forbidden, "", nullptr, state_.is_head_request_, absl::nullopt, diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index 5d06a1ef8486..2851fd79a494 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -182,7 +182,7 @@ class ProxyingConnectIntegrationTest : public HttpProtocolIntegrationTest { {":path", "/"}, {":protocol", "bytestream"}, {":scheme", "https"}, - {":authority", "host"}}; + {":authority", "host:80"}}; IntegrationStreamDecoderPtr response_; }; @@ -190,7 +190,7 @@ INSTANTIATE_TEST_SUITE_P(Protocols, ProxyingConnectIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); -TEST_P(ProxyingConnectIntegrationTest, DISABLED_ProxyConnect) { +TEST_P(ProxyingConnectIntegrationTest, ProxyConnect) { initialize(); // Send request headers. From 2674cf6031e13274e17899c9910c51115c9046a8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 15 Apr 2020 13:50:39 -0400 Subject: [PATCH 07/12] reviewer comments Signed-off-by: Alyssa Wilk --- source/common/http/utility.h | 2 +- source/common/router/router.cc | 2 +- source/extensions/filters/http/csrf/csrf_filter.cc | 2 +- .../quiche/platform/quic_hostname_utils_impl.cc | 2 +- test/common/http/codec_impl_corpus/connect | 2 +- test/common/http/http1/codec_impl_test.cc | 13 +------------ test/common/http/utility_test.cc | 10 +++++----- 7 files changed, 11 insertions(+), 22 deletions(-) diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d2db23db71f0..d3e476ad97a0 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -101,7 +101,7 @@ namespace Utility { */ class Url { public: - bool initialize(absl::string_view absolute_url, bool is_connect_request = false); + bool initialize(absl::string_view absolute_url, bool is_connect_request); absl::string_view scheme() { return scheme_; } absl::string_view host_and_port() { return host_and_port_; } absl::string_view path_and_query_params() { return path_and_query_params_; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4171f18440be..98c2ec509f8c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -70,7 +70,7 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream } Http::Utility::Url absolute_url; - if (!absolute_url.initialize(internal_redirect.value().getStringView())) { + if (!absolute_url.initialize(internal_redirect.value().getStringView(), false)) { return false; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index d18f95de35df..0109f782b5a4 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -35,7 +35,7 @@ bool isModifyMethod(const Http::RequestHeaderMap& headers) { absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { - if (absolute_url.initialize(header->value().getStringView())) { + if (absolute_url.initialize(header->value().getStringView(), false)) { return absolute_url.host_and_port(); } return header->value().getStringView(); diff --git a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc index 4b7b40744977..bcbafb56639e 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc @@ -23,7 +23,7 @@ bool QuicHostnameUtilsImpl::IsValidSNI(quiche::QuicheStringPiece sni) { // TODO(wub): Implement it on top of GoogleUrl, once it is available. return sni.find_last_of('.') != std::string::npos && - Envoy::Http::Utility::Url().initialize(absl::StrCat("http://", sni)); + Envoy::Http::Utility::Url().initialize(absl::StrCat("http://", sni), false); } // static diff --git a/test/common/http/codec_impl_corpus/connect b/test/common/http/codec_impl_corpus/connect index 63f0b9c42a85..807a1f2f0abb 100644 --- a/test/common/http/codec_impl_corpus/connect +++ b/test/common/http/codec_impl_corpus/connect @@ -6,7 +6,7 @@ actions { value: "CONNECT" } headers { - key: ":path" + key: ":authority" value: "foo.com:80" } } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index a865a96a3ead..5110bd6ccd93 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1595,6 +1595,7 @@ TEST_F(Http1ServerConnectionImplTest, ConnectRequestWithTEChunked) { // Connect with body is technically illegal, but Envoy does not inspect the // body to see if there is a non-zero byte chunk. It will instead pass it // through. + // TODO(alyssawilk) track connect payload and block if this happens. Buffer::OwnedImpl expected_data("12345abcd"); EXPECT_CALL(decoder, decodeHeaders_(_, false)); EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); @@ -2029,18 +2030,6 @@ TEST_F(Http1ClientConnectionImplTest, ConnectRejected) { codec_->dispatch(response); } -TEST_F(Http1ClientConnectionImplTest, ConnectWithoutHost) { - initialize(); - - InSequence s; - - NiceMock response_decoder; - Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); - TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}}; - EXPECT_THROW_WITH_MESSAGE(request_encoder.encodeHeaders(headers, true), CodecClientException, - "Host must be specified for CONNECT requests"); -} - TEST_F(Http1ClientConnectionImplTest, WatermarkTest) { EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 62e8def641e0..f841e9da9387 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1078,10 +1078,10 @@ TEST(HttpUtility, TestRejectTeHeaderTooLong) { TEST(Url, ParsingFails) { Utility::Url url; - EXPECT_FALSE(url.initialize("")); - EXPECT_FALSE(url.initialize("foo")); - EXPECT_FALSE(url.initialize("http://")); - EXPECT_FALSE(url.initialize("random_scheme://host.com/path")); + EXPECT_FALSE(url.initialize("", false)); + EXPECT_FALSE(url.initialize("foo", false)); + EXPECT_FALSE(url.initialize("http://", false)); + EXPECT_FALSE(url.initialize("random_scheme://host.com/path", false)); EXPECT_FALSE(url.initialize("http://www.foo.com", true)); EXPECT_FALSE(url.initialize("foo.com", true)); } @@ -1089,7 +1089,7 @@ TEST(Url, ParsingFails) { void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, absl::string_view expected_host_port, absl::string_view expected_path) { Utility::Url url; - ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; + ASSERT_TRUE(url.initialize(raw_url, false)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); EXPECT_EQ(url.host_and_port(), expected_host_port); EXPECT_EQ(url.path_and_query_params(), expected_path); From fa767672b0a7bd4bfb294a33cd66acd7d7c9b1b5 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 15 Apr 2020 16:29:33 -0400 Subject: [PATCH 08/12] fuzzing requests and responses Signed-off-by: Alyssa Wilk --- test/common/http/codec_impl_corpus/connect | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/http/codec_impl_corpus/connect b/test/common/http/codec_impl_corpus/connect index 807a1f2f0abb..e9175eccf711 100644 --- a/test/common/http/codec_impl_corpus/connect +++ b/test/common/http/codec_impl_corpus/connect @@ -13,6 +13,7 @@ actions { end_stream: false } } +actions { quiesce_drain {} } actions { stream_action { stream_id: 0 From e0725a1b500ee7f50e2bad3a1dce5d9a33536dfe Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 16 Apr 2020 09:55:58 -0400 Subject: [PATCH 09/12] preexisting clang_tidy :-( Signed-off-by: Alyssa Wilk --- source/common/http/http1/codec_impl.cc | 6 +- source/common/http/utility.cc | 12 ++-- source/common/http/utility.h | 8 +-- source/common/router/router.cc | 4 +- .../filters/http/csrf/csrf_filter.cc | 2 +- test/common/http/utility_test.cc | 56 +++++++++---------- test/integration/integration_test.cc | 1 - 7 files changed, 43 insertions(+), 46 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 2cd62b782def..a1df747f0002 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -779,10 +779,10 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // request-target. A proxy that forwards such a request MUST generate a // new Host field-value based on the received request-target rather than // forward the received Host field-value. - headers.setHost(absolute_url.host_and_port()); + headers.setHost(absolute_url.hostAndPort()); - if (!absolute_url.path_and_query_params().empty()) { - headers.setPath(absolute_url.path_and_query_params()); + if (!absolute_url.pathAndQueryParams().empty()) { + headers.setPath(absolute_url.pathAndQueryParams()); } active_request.request_url_.clear(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 5270c4539b71..a493704a79a0 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -205,18 +205,16 @@ bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { if ((u.field_set & (1 << UF_PORT)) == (1 << UF_PORT)) { authority_len = authority_len + u.field_data[UF_PORT].len + 1; } - host_and_port_ = - absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); + hostAndPort_ = absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); // RFC allows the absolute-uri to not end in /, but the absolute path form // must start with - uint64_t path_len = - absolute_url.length() - (u.field_data[UF_HOST].off + host_and_port().length()); + uint64_t path_len = absolute_url.length() - (u.field_data[UF_HOST].off + hostAndPort().length()); if (path_len > 0) { - uint64_t path_beginning = u.field_data[UF_HOST].off + host_and_port().length(); - path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_len); + uint64_t path_beginning = u.field_data[UF_HOST].off + hostAndPort().length(); + pathAndQueryParams_ = absl::string_view(absolute_url.data() + path_beginning, path_len); } else if (!is_connect) { - path_and_query_params_ = absl::string_view(kDefaultPath, 1); + pathAndQueryParams_ = absl::string_view(kDefaultPath, 1); } return true; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index d3e476ad97a0..604bab40d271 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -103,13 +103,13 @@ class Url { public: bool initialize(absl::string_view absolute_url, bool is_connect_request); absl::string_view scheme() { return scheme_; } - absl::string_view host_and_port() { return host_and_port_; } - absl::string_view path_and_query_params() { return path_and_query_params_; } + absl::string_view hostAndPort() { return hostAndPort_; } + absl::string_view pathAndQueryParams() { return pathAndQueryParams_; } private: absl::string_view scheme_; - absl::string_view host_and_port_; - absl::string_view path_and_query_params_; + absl::string_view hostAndPort_; + absl::string_view pathAndQueryParams_; }; class PercentEncoding { diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 98c2ec509f8c..f2a75ddb998f 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -105,8 +105,8 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Replace the original host, scheme and path. downstream_headers.setScheme(absolute_url.scheme()); - downstream_headers.setHost(absolute_url.host_and_port()); - downstream_headers.setPath(absolute_url.path_and_query_params()); + downstream_headers.setHost(absolute_url.hostAndPort()); + downstream_headers.setPath(absolute_url.pathAndQueryParams()); return true; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index 0109f782b5a4..396dc056c87d 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -36,7 +36,7 @@ absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { if (absolute_url.initialize(header->value().getStringView(), false)) { - return absolute_url.host_and_port(); + return absolute_url.hostAndPort(); } return header->value().getStringView(); } diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index f841e9da9387..010a8c07be17 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1086,73 +1086,73 @@ TEST(Url, ParsingFails) { EXPECT_FALSE(url.initialize("foo.com", true)); } -void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, +void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, absl::string_view expected_host_port, absl::string_view expected_path) { Utility::Url url; ASSERT_TRUE(url.initialize(raw_url, false)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); - EXPECT_EQ(url.host_and_port(), expected_host_port); - EXPECT_EQ(url.path_and_query_params(), expected_path); + EXPECT_EQ(url.hostAndPort(), expected_host_port); + EXPECT_EQ(url.pathAndQueryParams(), expected_path); } void ValidateConnectUrl(absl::string_view raw_url, absl::string_view expected_host_port) { Utility::Url url; ASSERT_TRUE(url.initialize(raw_url, true)) << "Failed to initialize " << raw_url; EXPECT_TRUE(url.scheme().empty()); - EXPECT_TRUE(url.path_and_query_params().empty()); - EXPECT_EQ(url.host_and_port(), expected_host_port); + EXPECT_TRUE(url.pathAndQueryParams().empty()); + EXPECT_EQ(url.hostAndPort(), expected_host_port); } TEST(Url, ParsingTest) { // Test url with no explicit path (with and without port) - ValidateUrl("http://www.host.com", "http", "www.host.com", "/"); - ValidateUrl("http://www.host.com:80", "http", "www.host.com:80", "/"); + validateUrl("http://www.host.com", "http", "www.host.com", "/"); + validateUrl("http://www.host.com:80", "http", "www.host.com:80", "/"); // Test url with "/" path. - ValidateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); - ValidateUrl("http://www.host.com/", "http", "www.host.com", "/"); + validateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); + validateUrl("http://www.host.com/", "http", "www.host.com", "/"); // Test url with "?". - ValidateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/?"); - ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/?"); + validateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/?"); + validateUrl("http://www.host.com/?", "http", "www.host.com", "/?"); // Test url with "?" but without slash. - ValidateUrl("http://www.host.com:80?", "http", "www.host.com:80", "?"); - ValidateUrl("http://www.host.com?", "http", "www.host.com", "?"); + validateUrl("http://www.host.com:80?", "http", "www.host.com:80", "?"); + validateUrl("http://www.host.com?", "http", "www.host.com", "?"); // Test url with multi-character path - ValidateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); - ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); + validateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); + validateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); // Test url with multi-character path and ? at the end - ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path?"); - ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?"); + validateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path?"); + validateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?"); // Test https scheme - ValidateUrl("https://www.host.com", "https", "www.host.com", "/"); + validateUrl("https://www.host.com", "https", "www.host.com", "/"); // Test url with query parameter - ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); - ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + validateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); + validateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); // Test url with query parameter but without slash - ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); - ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); + validateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); + validateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); // Test url with multi-character path and query parameter - ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", + validateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", "/path?query=param"); - ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); + validateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); // Test url with multi-character path and more than one query parameter - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", + validateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", "/path?query=param&query2=param2"); - ValidateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", "/path?query=param&query2=param2"); // Test url with multi-character path, more than one query parameter and fragment - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", + validateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", "www.host.com:80", "/path?query=param&query2=param2#fragment"); - ValidateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", "/path?query=param&query2=param2#fragment"); } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index bc5ef53146cf..9688ef252305 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -25,7 +25,6 @@ using Envoy::Http::Headers; using Envoy::Http::HeaderValueOf; using Envoy::Http::HttpStatusIs; -using testing::Contains; using testing::EndsWith; using testing::HasSubstr; using testing::Not; From 6a3ca237692cf462c73d9a8a8da89ada1fb1a70b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 16 Apr 2020 12:02:45 -0400 Subject: [PATCH 10/12] fixing the fixes Signed-off-by: Alyssa Wilk --- source/common/http/utility.cc | 7 ++++--- source/common/http/utility.h | 8 ++++---- test/common/http/utility_test.cc | 6 +++--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index a493704a79a0..544b1295b97b 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -205,16 +205,17 @@ bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { if ((u.field_set & (1 << UF_PORT)) == (1 << UF_PORT)) { authority_len = authority_len + u.field_data[UF_PORT].len + 1; } - hostAndPort_ = absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); + host_and_port_ = + absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); // RFC allows the absolute-uri to not end in /, but the absolute path form // must start with uint64_t path_len = absolute_url.length() - (u.field_data[UF_HOST].off + hostAndPort().length()); if (path_len > 0) { uint64_t path_beginning = u.field_data[UF_HOST].off + hostAndPort().length(); - pathAndQueryParams_ = absl::string_view(absolute_url.data() + path_beginning, path_len); + path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_len); } else if (!is_connect) { - pathAndQueryParams_ = absl::string_view(kDefaultPath, 1); + path_and_query_params_ = absl::string_view(kDefaultPath, 1); } return true; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 604bab40d271..241962466c82 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -103,13 +103,13 @@ class Url { public: bool initialize(absl::string_view absolute_url, bool is_connect_request); absl::string_view scheme() { return scheme_; } - absl::string_view hostAndPort() { return hostAndPort_; } - absl::string_view pathAndQueryParams() { return pathAndQueryParams_; } + absl::string_view hostAndPort() { return host_and_port_; } + absl::string_view pathAndQueryParams() { return path_and_query_params_; } private: absl::string_view scheme_; - absl::string_view hostAndPort_; - absl::string_view pathAndQueryParams_; + absl::string_view host_and_port_; + absl::string_view path_and_query_params_; }; class PercentEncoding { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 010a8c07be17..20d7e97305a5 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1095,7 +1095,7 @@ void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, EXPECT_EQ(url.pathAndQueryParams(), expected_path); } -void ValidateConnectUrl(absl::string_view raw_url, absl::string_view expected_host_port) { +void validateConnectUrl(absl::string_view raw_url, absl::string_view expected_host_port) { Utility::Url url; ASSERT_TRUE(url.initialize(raw_url, true)) << "Failed to initialize " << raw_url; EXPECT_TRUE(url.scheme().empty()); @@ -1157,8 +1157,8 @@ TEST(Url, ParsingTest) { } TEST(Url, ParsingForConnectTest) { - ValidateConnectUrl("host.com:443", "host.com:443"); - ValidateConnectUrl("host.com:80", "host.com:80"); + validateConnectUrl("host.com:443", "host.com:443"); + validateConnectUrl("host.com:80", "host.com:80"); } void validatePercentEncodingEncodeDecode(absl::string_view source, From f391f827450aa0abf51ceaf1c989ac34f49d379b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 16 Apr 2020 19:18:52 -0400 Subject: [PATCH 11/12] reviewer comments Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 6 ++++-- source/common/http/http1/codec_impl.cc | 4 +--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c5801e7409d8..8fcc96a14ed8 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -761,7 +761,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he request_headers_ = std::move(headers); // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including - // auditing of empty path headers. + // auditing of empty path headers. We check for path because HTTP/2 connect requests may have a + // path. if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) { request_headers_->setPath("/"); } @@ -928,7 +929,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he // TODO if there are no filters when starting a filter iteration, the connection manager // should return 404. The current returns no response if there is no router filter. if (hasCachedRoute()) { - if (upgrade_rejected) { // Do not allow upgrades if the route does not support it. + // Do not allow upgrades if the route does not support it. + if (upgrade_rejected) { // While downstream servers should not send upgrade payload without the upgrade being // accepted, err on the side of caution and refuse to process any further requests on this // connection, to avoid a class of HTTP/1.1 smuggling bugs where Upgrade or CONNECT payload diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index a1df747f0002..9a15b70838ec 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -361,9 +361,7 @@ void RequestEncoderImpl::encodeHeaders(const RequestHeaderMap& headers, bool end const HeaderEntry* host = headers.Host(); bool is_connect = HeaderUtility::isConnect(headers); - if (is_connect && !host) { - throw CodecClientException("Host must be specified for CONNECT requests"); - } else if (!method || (!path && !is_connect)) { + if (!method || (!path && !is_connect)) { throw CodecClientException(":method and :path must be specified"); } if (method->value() == Headers::get().MethodValues.Head) { From 840406e63fe3da9f2fabaa55a521e552d14d2e69 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 20 Apr 2020 08:38:01 -0400 Subject: [PATCH 12/12] spaces Signed-off-by: Alyssa Wilk --- source/common/http/conn_manager_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 8fcc96a14ed8..ded7daf13f11 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -761,7 +761,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he request_headers_ = std::move(headers); // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including - // auditing of empty path headers. We check for path because HTTP/2 connect requests may have a + // auditing of empty path headers. We check for path because HTTP/2 connect requests may have a // path. if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) { request_headers_->setPath("/");