http: auditing Path() calls for safety with Pathless CONNECT (envoypr…

…oxy#10851) This should result in all Path() calls not altered in envoyproxy#10720 being safe for path-less CONNECT. The major change for this PR is that requests without a path will not be considered gRPC requests. They're still currently rejected at the HCM, but when they are allowed through they will simply not be gRPC rather than causing crashes. Risk Level: medium (L7 code refactor) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Part of envoyproxy#1630 envoyproxy#1451 Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: pengg <[email protected]>
penguingao · Apr 22, 2020 · 2f01698 · 2f01698
1 parent fe915e0
commit 2f01698
Show file tree

Hide file tree

Showing 28 changed files with 195 additions and 75 deletions.
diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD
@@ -94,6 +94,7 @@ envoy_cc_library(
         "//source/common/common:macros",
         "//source/common/common:utility_lib",
         "//source/common/grpc:status_lib",
+        "//source/common/http:header_utility_lib",
         "//source/common/http:headers_lib",
         "//source/common/http:message_lib",
         "//source/common/http:utility_lib",

diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -14,6 +14,7 @@
 #include "common/common/fmt.h"
 #include "common/common/macros.h"
 #include "common/common/utility.h"
+#include "common/http/header_utility.h"
 #include "common/http/headers.h"
 #include "common/http/message_impl.h"
 #include "common/http/utility.h"
@@ -37,7 +38,14 @@ bool Common::hasGrpcContentType(const Http::RequestOrResponseHeaderMap& headers)
                   .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+');
 }
 
-bool Common::isGrpcResponseHeader(const Http::ResponseHeaderMap& headers, bool end_stream) {
+bool Common::isGrpcRequestHeaders(const Http::RequestHeaderMap& headers) {
+  if (!headers.Path()) {
+    return false;
+  }
+  return hasGrpcContentType(headers);
+}
+
+bool Common::isGrpcResponseHeaders(const Http::ResponseHeaderMap& headers, bool end_stream) {
   if (end_stream) {
     // Trailers-only response, only grpc-status is required.
     return headers.GrpcStatus() != nullptr;

diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h
@@ -36,12 +36,20 @@ class Common {
    */
   static bool hasGrpcContentType(const Http::RequestOrResponseHeaderMap& headers);
 
+  /**
+   * @param headers the headers to parse.
+   * @return bool indicating whether the header is a gRPC request header.
+   * Currently headers are considered gRPC request headers if they have the gRPC
+   * content type, and have a path header.
+   */
+  static bool isGrpcRequestHeaders(const Http::RequestHeaderMap& headers);
+
   /**
    * @param headers the headers to parse.
    * @param bool indicating whether the header is at end_stream.
    * @return bool indicating whether the header is a gRPC response header
    */
-  static bool isGrpcResponseHeader(const Http::ResponseHeaderMap& headers, bool end_stream);
+  static bool isGrpcResponseHeaders(const Http::ResponseHeaderMap& headers, bool end_stream);
 
   /**
    * Returns the GrpcStatus code from a given set of trailers, if present.

diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc
@@ -134,7 +134,7 @@ void AsyncStreamImpl::sendHeaders(RequestHeaderMap& headers, bool end_stream) {
     is_head_request_ = true;
   }
 
-  is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers);
+  is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers);
   headers.setReferenceEnvoyInternalRequest(Headers::get().EnvoyInternalRequestValues.True);
   if (send_xff_) {
     Utility::appendXff(headers, *parent_.config_.local_info_.address());

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -671,15 +671,16 @@ void ConnectionManagerImpl::ActiveStream::onIdleTimeout() {
   } else {
     stream_info_.setResponseFlag(StreamInfo::ResponseFlag::StreamIdleTimeout);
     sendLocalReply(request_headers_ != nullptr &&
-                       Grpc::Common::hasGrpcContentType(*request_headers_),
+                       Grpc::Common::isGrpcRequestHeaders(*request_headers_),
                    Http::Code::RequestTimeout, "stream timeout", nullptr, state_.is_head_request_,
                    absl::nullopt, StreamInfo::ResponseCodeDetails::get().StreamIdleTimeout);
   }
 }
 
 void ConnectionManagerImpl::ActiveStream::onRequestTimeout() {
   connection_manager_.stats_.named_.downstream_rq_timeout_.inc();
-  sendLocalReply(request_headers_ != nullptr && Grpc::Common::hasGrpcContentType(*request_headers_),
+  sendLocalReply(request_headers_ != nullptr &&
+                     Grpc::Common::isGrpcRequestHeaders(*request_headers_),
                  Http::Code::RequestTimeout, "request timeout", nullptr, state_.is_head_request_,
                  absl::nullopt, StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout);
 }
@@ -799,7 +800,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
     // overload it is more important to avoid unnecessary allocation than to create the filters.
     state_.created_filter_chain_ = true;
     connection_manager_.stats_.named_.downstream_rq_overload_close_.inc();
-    sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_),
+    sendLocalReply(Grpc::Common::isGrpcRequestHeaders(*request_headers_),
                    Http::Code::ServiceUnavailable, "envoy overloaded", nullptr,
                    state_.is_head_request_, absl::nullopt,
                    StreamInfo::ResponseCodeDetails::get().Overload);

diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h
@@ -313,7 +313,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
     // so that we can issue gRPC local responses to gRPC requests. Filter's decodeHeaders()
     // called here may change the content type, so we must check it before the call.
     FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream) {
-      is_grpc_request_ = Grpc::Common::hasGrpcContentType(headers);
+      is_grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers);
       FilterHeadersStatus status = handle_->decodeHeaders(headers, end_stream);
       if (end_stream) {
         handle_->decodeComplete();

diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -404,7 +404,9 @@ void ConnectionManagerUtility::mutateResponseHeaders(
 
 bool ConnectionManagerUtility::maybeNormalizePath(RequestHeaderMap& request_headers,
                                                   const ConnectionManagerConfig& config) {
-  ASSERT(request_headers.Path());
+  if (!request_headers.Path()) {
+    return true; // It's as valid as it is going to get.
+  }
   bool is_valid_path = true;
   if (config.shouldNormalizePath()) {
     is_valid_path = PathUtil::canonicalPath(request_headers);

diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h
@@ -63,9 +63,10 @@ class ConnectionManagerUtility {
                                     const RequestIDExtensionSharedPtr& rid_extension,
                                     const std::string& via);
 
-  // Sanitize the path in the header map if forced by config.
+  // Sanitize the path in the header map if the path exists and it is forced by config.
   // Side affect: the string view of Path header is invalidated.
   // Return false if error happens during the sanitization.
+  // Returns true if there is no path.
   static bool maybeNormalizePath(RequestHeaderMap& request_headers,
                                  const ConnectionManagerConfig& config);
 

diff --git a/source/common/http/path_utility.cc b/source/common/http/path_utility.cc
@@ -29,6 +29,7 @@ absl::optional<std::string> canonicalizePath(absl::string_view original_path) {
 
 /* static */
 bool PathUtil::canonicalPath(RequestHeaderMap& headers) {
+  ASSERT(headers.Path());
   const auto original_path = headers.Path()->value().getStringView();
   // canonicalPath is supposed to apply on path component in URL instead of :path header
   const auto query_pos = original_path.find('?');
@@ -54,6 +55,7 @@ bool PathUtil::canonicalPath(RequestHeaderMap& headers) {
 }
 
 void PathUtil::mergeSlashes(RequestHeaderMap& headers) {
+  ASSERT(headers.Path());
   const auto original_path = headers.Path()->value().getStringView();
   // Only operate on path component in URL.
   const absl::string_view::size_type query_start = original_path.find('?');

diff --git a/source/common/http/path_utility.h b/source/common/http/path_utility.h
@@ -14,8 +14,10 @@ class PathUtil {
 public:
   // Returns if the normalization succeeds.
   // If it is successful, the path header in header path will be updated with the normalized path.
+  // Requires the Path header be present.
   static bool canonicalPath(RequestHeaderMap& headers);
   // Merges two or more adjacent slashes in path part of URI into one.
+  // Requires the Path header be present.
   static void mergeSlashes(RequestHeaderMap& headers);
   // Removes the query and/or fragment string (if present) from the input path.
   // For example, this function returns "/data" for the input path "/data#fragment?param=value".

diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -47,6 +47,10 @@ namespace {
 
 const std::string DEPRECATED_ROUTER_NAME = "envoy.router";
 
+const absl::string_view getPath(const Http::RequestHeaderMap& headers) {
+  return headers.Path() ? headers.Path()->value().getStringView() : "";
+}
+
 } // namespace
 
 std::string SslRedirector::newPath(const Http::RequestHeaderMap& headers) const {
@@ -490,26 +494,19 @@ bool RouteEntryImplBase::matchRoute(const Http::RequestHeaderMap& headers,
                                     uint64_t random_value) const {
   bool matches = true;
 
-  // TODO(mattklein123): Currently all match types require a path header. When we support CONNECT
-  // we will need to figure out how to safely relax this.
-  if (headers.Path() == nullptr) {
-    return false;
-  }
-
   matches &= evaluateRuntimeMatch(random_value);
   if (!matches) {
     // No need to waste further cycles calculating a route match.
     return false;
   }
 
   if (match_grpc_) {
-    matches &= Grpc::Common::hasGrpcContentType(headers);
+    matches &= Grpc::Common::isGrpcRequestHeaders(headers);
   }
 
   matches &= Http::HeaderUtility::matchHeaders(headers, config_headers_);
   if (!config_query_parameters_.empty()) {
-    Http::Utility::QueryParams query_parameters =
-        Http::Utility::parseQueryString(headers.Path()->value().getStringView());
+    Http::Utility::QueryParams query_parameters = Http::Utility::parseQueryString(getPath(headers));
     matches &= ConfigUtility::matchQueryParams(query_parameters, config_query_parameters_);
   }
 
@@ -600,7 +597,7 @@ void RouteEntryImplBase::finalizePathHeader(Http::RequestHeaderMap& headers,
     return;
   }
 
-  std::string path(headers.Path()->value().getStringView());
+  std::string path(getPath(headers));
   if (insert_envoy_original_path) {
     headers.setEnvoyOriginalPath(path);
   }
@@ -693,8 +690,7 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c
   if (!path_redirect_.empty()) {
     final_path = path_redirect_.c_str();
   } else {
-    ASSERT(headers.Path());
-    final_path = headers.Path()->value().getStringView();
+    final_path = getPath(headers);
     if (strip_query_) {
       size_t path_end = final_path.find("?");
       if (path_end != absl::string_view::npos) {
@@ -939,7 +935,7 @@ RouteConstSharedPtr PrefixRouteEntryImpl::matches(const Http::RequestHeaderMap&
                                                   const StreamInfo::StreamInfo& stream_info,
                                                   uint64_t random_value) const {
   if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value) &&
-      path_matcher_->match(headers.Path()->value().getStringView())) {
+      path_matcher_->match(getPath(headers))) {
     return clusterEntry(headers, random_value);
   }
   return nullptr;
@@ -961,7 +957,7 @@ RouteConstSharedPtr PathRouteEntryImpl::matches(const Http::RequestHeaderMap& he
                                                 const StreamInfo::StreamInfo& stream_info,
                                                 uint64_t random_value) const {
   if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value) &&
-      path_matcher_->match(headers.Path()->value().getStringView())) {
+      path_matcher_->match(getPath(headers))) {
     return clusterEntry(headers, random_value);
   }
 
@@ -989,8 +985,7 @@ RegexRouteEntryImpl::RegexRouteEntryImpl(
 
 void RegexRouteEntryImpl::rewritePathHeader(Http::RequestHeaderMap& headers,
                                             bool insert_envoy_original_path) const {
-  const absl::string_view path =
-      Http::PathUtil::removeQueryAndFragment(headers.Path()->value().getStringView());
+  const absl::string_view path = Http::PathUtil::removeQueryAndFragment(getPath(headers));
   // TODO(yuval-k): This ASSERT can happen if the path was changed by a filter without clearing the
   // route cache. We should consider if ASSERT-ing is the desired behavior in this case.
   ASSERT(regex_->match(path));
@@ -1001,8 +996,7 @@ RouteConstSharedPtr RegexRouteEntryImpl::matches(const Http::RequestHeaderMap& h
                                                  const StreamInfo::StreamInfo& stream_info,
                                                  uint64_t random_value) const {
   if (RouteEntryImplBase::matchRoute(headers, stream_info, random_value)) {
-    const absl::string_view path =
-        Http::PathUtil::removeQueryAndFragment(headers.Path()->value().getStringView());
+    const absl::string_view path = Http::PathUtil::removeQueryAndFragment(getPath(headers));
     if (regex_->match(path)) {
       return clusterEntry(headers, random_value);
     }
@@ -1211,6 +1205,10 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const Http::RequestHead
 
   // Check for a route that matches the request.
   for (const RouteEntryImplBaseConstSharedPtr& route : routes_) {
+    if (!headers.Path()) {
+      // TODO(alyssawilk) allow specifically for kConnectMatcher routes.
+      return nullptr;
+    }
     RouteConstSharedPtr route_entry = route->matches(headers, stream_info, random_value);
     if (nullptr != route_entry) {
       return route_entry;

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -68,6 +68,9 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream
   if (internal_redirect.value().getStringView().length() == 0) {
     return false;
   }
+  if (!downstream_headers.Path()) {
+    return false;
+  }
 
   Http::Utility::Url absolute_url;
   if (!absolute_url.initialize(internal_redirect.value().getStringView(), false)) {
@@ -129,6 +132,10 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream
 
 constexpr uint64_t TimeoutPrecisionFactor = 100;
 
+const absl::string_view getPath(const Http::RequestHeaderMap& headers) {
+  return headers.Path() ? headers.Path()->value().getStringView() : "";
+}
+
 } // namespace
 
 // Express percentage as [0, TimeoutPrecisionFactor] because stats do not accept floating point
@@ -396,7 +403,6 @@ void Filter::chargeUpstreamCode(Http::Code code,
 Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, bool end_stream) {
   // Do a common header check. We make sure that all outgoing requests have all HTTP/2 headers.
   // These get stripped by HTTP/1 codec where applicable.
-  ASSERT(headers.Path());
   ASSERT(headers.Method());
   ASSERT(headers.Host());
 
@@ -412,7 +418,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
           : nullptr;
 
   // TODO: Maybe add a filter API for this.
-  grpc_request_ = Grpc::Common::hasGrpcContentType(headers);
+  grpc_request_ = Grpc::Common::isGrpcRequestHeaders(headers);
 
   // Only increment rq total stat if we actually decode headers here. This does not count requests
   // that get handled by earlier filters.
@@ -427,8 +433,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
   route_ = callbacks_->route();
   if (!route_) {
     config_.stats_.no_route_.inc();
-    ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_,
-                     headers.Path()->value().getStringView());
+    ENVOY_STREAM_LOG(debug, "no cluster match for URL '{}'", *callbacks_, getPath(headers));
 
     callbacks_->streamInfo().setResponseFlag(StreamInfo::ResponseFlag::NoRouteFound);
     callbacks_->sendLocalReply(Http::Code::NotFound, "", modify_headers, absl::nullopt,
@@ -445,7 +450,10 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
         direct_response->responseCode(), direct_response->responseBody(),
         [this, direct_response,
          &request_headers = headers](Http::ResponseHeaderMap& response_headers) -> void {
-          const auto new_path = direct_response->newPath(request_headers);
+          std::string new_path;
+          if (request_headers.Path()) {
+            new_path = direct_response->newPath(request_headers);
+          }
           // See https://tools.ietf.org/html/rfc7231#section-7.1.2.
           const auto add_location =
               direct_response->responseCode() == Http::Code::Created ||
@@ -490,7 +498,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
   // Set up stat prefixes, etc.
   request_vcluster_ = route_entry_->virtualCluster(headers);
   ENVOY_STREAM_LOG(debug, "cluster '{}' match for URL '{}'", *callbacks_,
-                   route_entry_->clusterName(), headers.Path()->value().getStringView());
+                   route_entry_->clusterName(), getPath(headers));
 
   if (config_.strict_check_headers_ != nullptr) {
     for (const auto& header : *config_.strict_check_headers_) {

diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc
@@ -36,6 +36,9 @@ static std::string valueOrDefault(const Http::HeaderEntry* header, const char* d
 
 static std::string buildUrl(const Http::RequestHeaderMap& request_headers,
                             const uint32_t max_path_length) {
+  if (!request_headers.Path()) {
+    return "";
+  }
   std::string path(request_headers.EnvoyOriginalPath()
                        ? request_headers.EnvoyOriginalPath()->value().getStringView()
                        : request_headers.Path()->value().getStringView());
@@ -184,7 +187,7 @@ void HttpTracerUtility::finalizeDownstreamSpan(Span& span,
                   std::string(request_headers->ClientTraceId()->value().getStringView()));
     }
 
-    if (Grpc::Common::hasGrpcContentType(*request_headers)) {
+    if (Grpc::Common::isGrpcRequestHeaders(*request_headers)) {
       addGrpcRequestTags(span, *request_headers);
     }
   }

diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -583,8 +583,8 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::decodeHeaders(
                   end_stream);
     return;
   }
-  if (!Grpc::Common::hasGrpcContentType(*headers)) {
-    onRpcComplete(Grpc::Status::WellKnownGrpcStatus::Internal, "invalid gRPC content-type", false);
+  if (!Grpc::Common::isGrpcResponseHeaders(*headers, end_stream)) {
+    onRpcComplete(Grpc::Status::WellKnownGrpcStatus::Internal, "not a gRPC request", false);
     return;
   }
   if (end_stream) {

diff --git a/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc b/source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc
@@ -298,8 +298,11 @@ void Filter::jsonizeRequest(Http::RequestHeaderMap const& headers, const Buffer:
       &json_req);
 
   // Wrap the Query String
-  for (auto&& kv_pair : Http::Utility::parseQueryString(headers.Path()->value().getStringView())) {
-    json_req.mutable_query_string_parameters()->insert({kv_pair.first, kv_pair.second});
+  if (headers.Path()) {
+    for (auto&& kv_pair :
+         Http::Utility::parseQueryString(headers.Path()->value().getStringView())) {
+      json_req.mutable_query_string_parameters()->insert({kv_pair.first, kv_pair.second});
+    }
   }
 
   // Wrap the body

diff --git a/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc b/source/extensions/filters/http/grpc_http1_bridge/http1_bridge_filter.cc
@@ -24,7 +24,7 @@ void Http1BridgeFilter::chargeStat(const Http::ResponseHeaderOrTrailerMap& heade
 }
 
 Http::FilterHeadersStatus Http1BridgeFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) {
-  const bool grpc_request = Grpc::Common::hasGrpcContentType(headers);
+  const bool grpc_request = Grpc::Common::isGrpcRequestHeaders(headers);
   if (grpc_request) {
     setupStatTracking(headers);
   }

diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.cc
@@ -84,7 +84,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
   // If this is a gRPC request we:
   //  - mark this request as being gRPC
   //  - change the content-type to application/x-protobuf
-  if (Envoy::Grpc::Common::hasGrpcContentType(headers)) {
+  if (Envoy::Grpc::Common::isGrpcRequestHeaders(headers)) {
     enabled_ = true;
 
     // We keep track of the original content-type to ensure that we handle