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 270d9d92b53b..d6e4a55eced7 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -760,6 +760,13 @@ 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. We check for path because HTTP/2 connect requests may have a + // path. + 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) { @@ -921,9 +928,14 @@ 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()) { + // 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 + // 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 + // contains a smuggled HTTP request. + state_.saw_connection_close_ = true; 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, @@ -2003,7 +2015,12 @@ 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_ && HeaderUtility::isConnect(*request_headers_)) { + 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 5da6106261a9..72560b73e076 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 { @@ -183,11 +187,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/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 cc2f53d6b3a2..9f5fb06f53d1 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(); } @@ -165,14 +167,15 @@ 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. // // 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,18 +358,29 @@ 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) { + const HeaderEntry* host = headers.Host(); + bool is_connect = HeaderUtility::isConnect(headers); + + if (!method || (!path && !is_connect)) { 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; } if (Utility::isUpgrade(headers)) { upgrade_request_ = true; } + 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); @@ -607,6 +625,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. @@ -727,7 +749,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_)); @@ -736,18 +758,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"); } @@ -758,9 +777,11 @@ 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()); - headers.setPath(absolute_url.path_and_query_params()); + if (!absolute_url.pathAndQueryParams().empty()) { + headers.setPath(absolute_url.pathAndQueryParams()); + } active_request.request_url_.clear(); } @@ -788,10 +809,9 @@ 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_.setIsResponseToHeadRequest(parser_.method == HTTP_HEAD); + active_request.response_encoder_.setIsResponseToConnectRequest(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()); @@ -857,6 +877,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; @@ -990,6 +1011,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. @@ -999,7 +1026,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 ddb7a746015c..12901e447e9a 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -71,7 +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 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: @@ -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; @@ -154,6 +157,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 14560492e5ae..721da9af8431 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -161,6 +161,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(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 fe9393574a32..544b1295b97b 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); @@ -211,12 +210,11 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { // 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(); + uint64_t path_beginning = u.field_data[UF_HOST].off + hostAndPort().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; @@ -382,7 +380,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() != Headers::get().ProtocolValues.Bytestream; } bool Utility::isWebSocketUpgradeRequest(const RequestHeaderMap& headers) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2daa237894d1..241962466c82 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -101,10 +101,10 @@ namespace Utility { */ class Url { public: - bool initialize(absl::string_view absolute_url); + 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 host_and_port_; } + absl::string_view pathAndQueryParams() { return path_and_query_params_; } private: absl::string_view scheme_; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index c18bb58fda69..de4278259e65 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; } @@ -104,8 +104,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/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 5eb9143cc55e..1ddc80c12f3d 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -59,7 +59,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/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/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index d18f95de35df..396dc056c87d 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -35,8 +35,8 @@ 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())) { - return absolute_url.host_and_port(); + if (absolute_url.initialize(header->value().getStringView(), false)) { + return absolute_url.hostAndPort(); } 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..e9175eccf711 100644 --- a/test/common/http/codec_impl_corpus/connect +++ b/test/common/http/codec_impl_corpus/connect @@ -6,13 +6,14 @@ actions { value: "CONNECT" } headers { - key: ":path" + key: ":authority" value: "foo.com:80" } } end_stream: false } } +actions { quiesce_drain {} } actions { stream_action { stream_id: 0 diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 99fa69b05acb..324f2c58259f 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 dd19b620354b..5110bd6ccd93 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); @@ -1537,6 +1537,89 @@ TEST_F(Http1ServerConnectionImplTest, UpgradeRequestWithNoBody) { codec_->dispatch(buffer); } +TEST_F(Http1ServerConnectionImplTest, ConnectRequestNoContentLength) { + initialize(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + 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); + + Buffer::OwnedImpl expected_data("abcd"); + Buffer::OwnedImpl connect_payload("abcd"); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + 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(); + + InSequence sequence; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl expected_data("abcd"); + 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); +} + +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. + // 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)); + Buffer::OwnedImpl buffer( + "CONNECT host:80 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)); + 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); +} + TEST_F(Http1ServerConnectionImplTest, WatermarkTest) { EXPECT_CALL(connection_, bufferLimit()).WillOnce(Return(10)); initialize(); @@ -1882,6 +1965,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)); + 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)); + 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)); + 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 e44c29198a2e..8ad4de81ccd7 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1786,6 +1786,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); +} + class TestNghttp2SessionFactory; // Test client for H/2 METADATA frame edge cases. diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 8c5c4d55efb6..20d7e97305a5 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 = { @@ -1073,74 +1078,89 @@ 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)); } -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)) << "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); + 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.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"); } +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/BUILD b/test/integration/BUILD index 24fbc6f4b806..7ec27ca352f5 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -577,6 +577,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 a38c97817102..1ae41217b1a0 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1280,4 +1280,82 @@ TEST_P(IntegrationTest, TestUpgradeHeaderInResponse) { cleanupUpstreamAndDownstream(); } +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 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 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; + + 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, 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 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)); + // 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()); +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e1bc4a9ef727..f3251638f4a7 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1739,12 +1739,12 @@ 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(); - 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..2851fd79a494 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_{}; +}; + +// 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(); + + 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:80"}}; + IntegrationStreamDecoderPtr response_; +}; + +INSTANTIATE_TEST_SUITE_P(Protocols, ProxyingConnectIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +TEST_P(ProxyingConnectIntegrationTest, 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