http: downstream connect support

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,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) {
@@ -921,9 +927,13 @@ 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 (upgrade_rejected) {
- // Do not allow upgrades if the route does not support it.
+ 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
+ // 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 +2013,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,31 @@ 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 (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) {
  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 +627,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 +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_));
@@ -736,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");
  }
@@ -758,9 +779,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 +811,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 +879,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 +1013,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 +1028,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

@@ -162,6 +162,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;
  }
 
@@ -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;
 }