http: downstream connect support (#10720)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

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

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;

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;
 }
 

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

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"};

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<ResponseHeaderMapPtr>(nullptr);
-    } else if (cannotHaveBody()) {
+    } else if (cannotHaveBody() && !handling_upgrade_) {
       deferred_end_stream_headers_ = true;
     } else {
       pending_response_.value().decoder_->decodeHeaders(std::move(headers), false);

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_{};
 };
 
 /**

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<RequestHeaderMapImpl>(headers);
+    modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
+    buildHeaders(final_headers, *modified_headers);
   } else {
     buildHeaders(final_headers, headers);
   }

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) {

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_;

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;
 }