http: switching from XFP to scheme

docs/root/configuration/http/http_conn_man/headers.rst

@@ -25,6 +25,8 @@ This default behavior can be overridden via the :ref:`scheme_header_transformati
 <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.scheme_header_transformation>`
 configuration option.
 
+The *:scheme* header will be used by Envoy over *x-forwarded-proto* where the URI scheme is wanted, for example serving content from cache based on the *:scheme* header rather than X-Forwarded-Proto, or setting the scheme of redirects based on the scheme of the original URI. See :ref:`why_is_envoy_using_xfp_or_scheme` for more details.
+
 .. [1] Edge Envoys often have plaintext HTTP/1.1 listeners. If Envoy trusts absolute URL scheme from fully qualfied URLs, a MiTM can adjust relative URLs to https absolute URLs, and inadvertantly cause the Envoy's upstream to send PII or other sensitive data over what it then believes is a secure connection.
 .. [2] Unlike HTTP/1.1, HTTP/2 is in practice always served over TLS via ALPN for edge Envoys. In mesh networks using insecure HTTP/2, if the downstream is not trusted to set scheme, the :ref:`scheme_header_transformation <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.scheme_header_transformation>` should be used.
 
@@ -368,6 +370,8 @@ If the scheme is changed via the :ref:`scheme_header_transformation
 <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.scheme_header_transformation>`
 configuration option, *x-forwarded-proto* will be updated as well.
 
+The *x-forwarded-proto* header will be used by Envoy over *:scheme* where the underlying encryption is wanted, for example clearing default ports based on *x-forwarded-proto*. See :ref:`why_is_envoy_using_xfp_or_scheme` for more details.
+
 .. _config_http_conn_man_headers_x-request-id:
 
 x-request-id

docs/root/faq/debugging/xfp_vs_scheme.rst

@@ -0,0 +1,14 @@
+.. _why_is_envoy_using_xfp_or_scheme:
+
+Why is Envoy operating on X-Forwarded-Proto instead of :scheme or vice-versa?
+=============================================================================
+
+With almost all requests, the value of the X-Forwarded-Proto header and the :scheme
+header (if present) will be the same. Generally users request https:// resources over
+TLS connections and http:// resources in the clear. However, it is entirely possible
+for a user to request http:// content over a TLS connection or in internal meshes to forward
+https:// requests in cleartext. In these cases Envoy will attempt to use the :scheme
+header when refering to content (say serving a given entity out of cache based on the URL
+scheme) and the X-Forwarded-Proto header when doing operations related to underlying
+encryption (stripping the default port based on if the request was TLS on port 443, or
+cleartext on port 80)

docs/root/faq/overview.rst

@@ -36,6 +36,7 @@ Debugging
   debugging/why_is_envoy_404ing_connect_requests
   debugging/why_is_envoy_sending_413s
   debugging/why_is_my_route_not_found
+  debugging/xfp_vs_scheme
 
 Performance
 -----------

docs/root/version_history/current.rst

@@ -10,6 +10,9 @@ Minor Behavior Changes
 *Changes that may cause incompatibilities for some users, but should not for most*
 
 * grpc: gRPC async client can be cached and shared accross filter instances in the same thread, this feature is turned off by default, can be turned on by setting runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to true.
+* http: correct the use of the ``x-forwarded-proto`` header and the ``:scheme`` header. Where they differ
+  (which is rare) ``:scheme`` will now be used for serving redirect URIs and cached content. This behavior
+  can be reverted by setting runtime guard ``correct_scheme_and_xfp`` to false.
 * http: set the default :ref:`lazy headermap threshold <arch_overview_http_header_map_settings>` to 3,
   which defines the minimal number of headers in a request/response/trailers required for using a
   dictionary in addition to the list. Setting the `envoy.http.headermap.lazy_map_min_size` runtime

source/common/http/conn_manager_utility.cc

@@ -165,10 +165,12 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m
     request_headers.setReferenceForwardedProto(connection.ssl() ? Headers::get().SchemeValues.Https
                                                                 : Headers::get().SchemeValues.Http);
   }
+
   if (config.schemeToSet().has_value()) {
     request_headers.setScheme(config.schemeToSet().value());
     request_headers.setForwardedProto(config.schemeToSet().value());
   }
+
   // If :scheme is not set, sets :scheme based on X-Forwarded-Proto if a valid scheme,
   // else encryption level.
   // X-Forwarded-Proto and :scheme may still differ if different values are sent from downstream.

source/common/http/utility.cc

@@ -768,6 +768,13 @@ const std::string& Utility::getProtocolString(const Protocol protocol) {
   NOT_REACHED_GCOVR_EXCL_LINE;
 }
 
+absl::string_view Utility::getScheme(const RequestHeaderMap& headers) {
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correct_scheme_and_xfp")) {
+    return headers.getSchemeValue();
+  }
+  return headers.getForwardedProtoValue();
+}
+
 std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_headers,
                                       const absl::optional<uint32_t> max_path_length) {
   if (!request_headers.Path()) {
@@ -781,7 +788,7 @@ std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_head
     path = path.substr(0, max_path_length.value());
   }
 
-  return absl::StrCat(request_headers.getForwardedProtoValue(), "://",
+  return absl::StrCat(Http::Utility::getScheme(request_headers), "://",
                       request_headers.getHostValue(), path);
 }
 

source/common/http/utility.h

@@ -395,6 +395,15 @@ bool sanitizeConnectionHeader(Http::RequestHeaderMap& headers);
  */
 const std::string& getProtocolString(const Protocol p);
 
+/**
+ * Return the scheme of the request.
+ * For legacy code (envoy.reloadable_features.correct_scheme_and_xfp == false) this
+ * will be the value of the X-Forwarded-Proto header value. By default it will
+ * return the scheme if present, otherwise the value of X-Forwarded-Proto if
+ * present.
+ */
+absl::string_view getScheme(const RequestHeaderMap& headers);
+
 /**
  * Constructs the original URI sent from the client from
  * the request headers.

source/common/router/config_impl.cc

@@ -750,6 +750,9 @@ absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHead
 
   if (host_end != absl::string_view::npos) {
     absl::string_view request_port = request_host.substr(host_end);
+    // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS
+    // connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't
+    // have the port stripped when served over TLS.
     absl::string_view request_protocol = headers.getForwardedProtoValue();
     bool remove_port = !new_port.empty();
 
@@ -781,8 +784,9 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c
   } else if (https_redirect_) {
     final_scheme = Http::Headers::get().SchemeValues.Https;
   } else {
-    ASSERT(headers.ForwardedProto());
-    final_scheme = headers.getForwardedProtoValue();
+    // Serve the redirect URL based on the scheme of the original URL, not the
+    // security of the underlying connection.
+    final_scheme = Http::Utility::getScheme(headers);
   }
 
   if (!port_redirect_.empty()) {
@@ -1367,18 +1371,21 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb
                                                          const Http::RequestHeaderMap& headers,
                                                          const StreamInfo::StreamInfo& stream_info,
                                                          uint64_t random_value) const {
-  // No x-forwarded-proto header. This normally only happens when ActiveStream::decodeHeaders
-  // bails early (as it rejects a request), so there is no routing is going to happen anyway.
-  const auto* forwarded_proto_header = headers.ForwardedProto();
-  if (forwarded_proto_header == nullptr) {
+  // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS
+  // connection), force a redirect based on underlying protocol, rather than URL
+  // scheme, so don't force a redirect for a http:// url served over a TLS
+  // connection.
+  const absl::string_view scheme = headers.getForwardedProtoValue();
+  if (scheme.empty()) {
+    // No scheme header. This normally only happens when ActiveStream::decodeHeaders
+    // bails early (as it rejects a request), or a buggy filter removes the :scheme header.
     return nullptr;
   }
 
   // First check for ssl redirect.
-  if (ssl_requirements_ == SslRequirements::All && forwarded_proto_header->value() != "https") {
+  if (ssl_requirements_ == SslRequirements::All && scheme != "https") {
     return SSL_REDIRECT_ROUTE;
-  } else if (ssl_requirements_ == SslRequirements::ExternalOnly &&
-             forwarded_proto_header->value() != "https" &&
+  } else if (ssl_requirements_ == SslRequirements::ExternalOnly && scheme != "https" &&
              !Http::HeaderUtility::isEnvoyInternalRequest(headers)) {
     return SSL_REDIRECT_ROUTE;
   }

source/common/router/router.cc

@@ -51,7 +51,7 @@ uint32_t getLength(const Buffer::Instance* instance) { return instance ? instanc
 
 bool schemeIsHttp(const Http::RequestHeaderMap& downstream_headers,
                   const Network::Connection& connection) {
-  if (downstream_headers.getForwardedProtoValue() == Http::Headers::get().SchemeValues.Http) {
+  if (Http::Utility::getScheme(downstream_headers) == Http::Headers::get().SchemeValues.Http) {
     return true;
   }
   if (!connection.ssl()) {
@@ -83,8 +83,12 @@ void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool down
     if (Http::HeaderUtility::schemeIsValid(headers.getSchemeValue())) {
       return;
     }
-    if (Http::HeaderUtility::schemeIsValid(headers.getForwardedProtoValue())) {
-      headers.setScheme(headers.getForwardedProtoValue());
+    // After all the changes in https://github.com/envoyproxy/envoy/issues/14587
+    // this path should only occur if a buggy filter has removed the :scheme
+    // header. In that case best-effort set from X-Forwarded-Proto.
+    absl::string_view xfp = headers.getForwardedProtoValue();
+    if (Http::HeaderUtility::schemeIsValid(xfp)) {
+      headers.setScheme(xfp);
       return;
     }
   }
@@ -1516,7 +1520,7 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do
   }
 
   const auto& policy = route_entry_->internalRedirectPolicy();
-  // Don't allow serving TLS responses over plaintext unless allowed by policy.
+  // Don't change the scheme from the original request
   const bool scheme_is_http = schemeIsHttp(downstream_headers, *callbacks_->connection());
   const bool target_is_http = absolute_url.scheme() == Http::Headers::get().SchemeValues.Http;
   if (!policy.isCrossSchemeRedirectAllowed() && scheme_is_http != target_is_http) {

source/common/runtime/runtime_features.cc

@@ -60,6 +60,8 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.allow_response_for_timeout",
     "envoy.reloadable_features.check_unsupported_typed_per_filter_config",
     "envoy.reloadable_features.check_ocsp_policy",
+    "envoy.reloadable_features.correct_scheme_and_xfp",
+    "envoy.reloadable_features.disable_tls_inspector_injection",
     "envoy.reloadable_features.dont_add_content_length_for_bodiless_requests",
     "envoy.reloadable_features.enable_compression_without_content_length_header",
     "envoy.reloadable_features.grpc_bridge_stats_disabled",

source/common/tracing/http_tracer_impl.cc

@@ -16,6 +16,7 @@
 #include "source/common/grpc/common.h"
 #include "source/common/http/codes.h"
 #include "source/common/http/header_map_impl.h"
+#include "source/common/http/header_utility.h"
 #include "source/common/http/headers.h"
 #include "source/common/http/utility.h"
 #include "source/common/protobuf/utility.h"

source/extensions/filters/http/cache/cacheability_utils.cc

@@ -4,6 +4,7 @@
 
 #include "source/common/common/macros.h"
 #include "source/common/common/utility.h"
+#include "source/common/http/utility.h"
 #include "source/extensions/filters/http/cache/cache_custom_headers.h"
 
 namespace Envoy {
@@ -33,7 +34,7 @@ const std::vector<const Http::LowerCaseString*>& conditionalHeaders() {
 
 bool CacheabilityUtils::canServeRequestFromCache(const Http::RequestHeaderMap& headers) {
   const absl::string_view method = headers.getMethodValue();
-  const absl::string_view forwarded_proto = headers.getForwardedProtoValue();
+  const absl::string_view scheme = Http::Utility::getScheme(headers);
   const Http::HeaderValues& header_values = Http::Headers::get();
 
   // Check if the request contains any conditional headers.
@@ -52,8 +53,7 @@ bool CacheabilityUtils::canServeRequestFromCache(const Http::RequestHeaderMap& h
   return headers.Path() && headers.Host() &&
          !headers.getInline(CacheCustomHeaders::authorization()) &&
          (method == header_values.MethodValues.Get || method == header_values.MethodValues.Head) &&
-         (forwarded_proto == header_values.SchemeValues.Http ||
-          forwarded_proto == header_values.SchemeValues.Https);
+         (scheme == header_values.SchemeValues.Http || scheme == header_values.SchemeValues.Https);
 }
 
 bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& headers,

source/extensions/filters/http/cache/http_cache.cc

@@ -9,6 +9,7 @@
 
 #include "source/common/http/header_utility.h"
 #include "source/common/http/headers.h"
+#include "source/common/http/utility.h"
 #include "source/common/protobuf/utility.h"
 #include "source/extensions/filters/http/cache/cache_custom_headers.h"
 #include "source/extensions/filters/http/cache/cache_headers_utils.h"
@@ -29,17 +30,14 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst
   // CacheFilter doesn't create LookupRequests for such requests.
   ASSERT(request_headers.Path(), "Can't form cache lookup key for malformed Http::RequestHeaderMap "
                                  "with null Path.");
-  ASSERT(
-      request_headers.ForwardedProto(),
-      "Can't form cache lookup key for malformed Http::RequestHeaderMap with null ForwardedProto.");
   ASSERT(request_headers.Host(), "Can't form cache lookup key for malformed Http::RequestHeaderMap "
                                  "with null Host.");
-  const Http::HeaderString& forwarded_proto = request_headers.ForwardedProto()->value();
+  absl::string_view scheme = Http::Utility::getScheme(request_headers);
   const auto& scheme_values = Http::Headers::get().SchemeValues;
-  ASSERT(forwarded_proto == scheme_values.Http || forwarded_proto == scheme_values.Https);
+  ASSERT(scheme == scheme_values.Http || scheme == scheme_values.Https);
 
   initializeRequestCacheControl(request_headers);
-  // TODO(toddmgreer): Let config determine whether to include forwarded_proto, host, and
+  // TODO(toddmgreer): Let config determine whether to include scheme, host, and
   // query params.
   // TODO(toddmgreer): get cluster name.
   if (request_headers.getMethodValue() == Http::Headers::get().MethodValues.Get) {
@@ -51,7 +49,7 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst
   key_.set_cluster_name("cluster_name_goes_here");
   key_.set_host(std::string(request_headers.getHostValue()));
   key_.set_path(std::string(request_headers.getPathValue()));
-  key_.set_clear_http(forwarded_proto == scheme_values.Http);
+  key_.set_clear_http(scheme == scheme_values.Http);
 
   vary_headers_ = vary_allow_list.possibleVariedHeaders(request_headers);
 }

source/extensions/filters/http/oauth2/filter.cc

@@ -386,8 +386,7 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap
   Http::ResponseHeaderMapPtr response_headers{Http::createHeaderMap<Http::ResponseHeaderMapImpl>(
       {{Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::Found))}})};
 
-  const std::string new_path =
-      absl::StrCat(headers.ForwardedProto()->value().getStringView(), "://", host_, "/");
+  const std::string new_path = absl::StrCat(Http::Utility::getScheme(headers), "://", host_, "/");
   response_headers->addReference(Http::Headers::get().SetCookie, SignoutCookieValue);
   response_headers->addReference(Http::Headers::get().SetCookie, SignoutBearerTokenValue);
   response_headers->setLocation(new_path);

test/common/http/async_client_impl_test.cc

@@ -1304,7 +1304,8 @@ TEST_F(AsyncClientImplTest, StreamTimeoutHeadReply) {
       }));
 
   RequestMessagePtr message{new RequestMessageImpl()};
-  HttpTestUtility::addDefaultHeaders(message->headers(), "HEAD");
+  message->headers().setMethod("HEAD");
+  HttpTestUtility::addDefaultHeaders(message->headers(), false);
   EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(&message->headers()), true));
   timer_ = new NiceMock<Event::MockTimer>(&dispatcher_);
   EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(40), _));

test/common/http/common.cc

@@ -5,11 +5,18 @@
 #include "envoy/http/header_map.h"
 
 namespace Envoy {
-void HttpTestUtility::addDefaultHeaders(Http::RequestHeaderMap& headers,
-                                        const std::string default_method) {
-  headers.setScheme("http");
-  headers.setMethod(default_method);
-  headers.setHost("host");
-  headers.setPath("/");
+void HttpTestUtility::addDefaultHeaders(Http::RequestHeaderMap& headers, bool overwrite) {
+  if (overwrite || headers.getSchemeValue().empty()) {
+    headers.setScheme("http");
+  }
+  if (overwrite || headers.getMethodValue().empty()) {
+    headers.setMethod("GET");
+  }
+  if (overwrite || headers.getHostValue().empty()) {
+    headers.setHost("host");
+  }
+  if (overwrite || headers.getPathValue().empty()) {
+    headers.setPath("/");
+  }
 }
 } // namespace Envoy

test/common/http/common.h

@@ -66,7 +66,6 @@ struct ConnPoolCallbacks : public Http::ConnectionPool::Callbacks {
  */
 class HttpTestUtility {
 public:
-  static void addDefaultHeaders(Http::RequestHeaderMap& headers,
-                                const std::string default_method = "GET");
+  static void addDefaultHeaders(Http::RequestHeaderMap& headers, bool overwrite = true);
 };
 } // namespace Envoy

test/common/http/utility_test.cc

@@ -1233,7 +1233,7 @@ TEST(HttpUtility, TestRejectNominatedXForwardedHost) {
   EXPECT_EQ(sanitized_headers, request_headers);
 }
 
-TEST(HttpUtility, TestRejectNominatedXForwardedProto) {
+TEST(HttpUtility, TestRejectNominatedForwardedProto) {
   Http::TestRequestHeaderMapImpl request_headers = {
       {":method", "GET"},
       {":path", "/"},
@@ -1317,15 +1317,15 @@ TEST(HttpUtility, TestRejectTeHeaderTooLong) {
 
 TEST(HttpUtility, TestRejectUriWithNoPath) {
   Http::TestRequestHeaderMapImpl request_headers_no_path = {
-      {":method", "GET"}, {":authority", "example.com"}, {"x-forwarded-proto", "http"}};
+      {":method", "GET"}, {":authority", "example.com"}, {":scheme", "http"}};
   EXPECT_EQ(Utility::buildOriginalUri(request_headers_no_path, {}), "");
 }
 
 TEST(HttpUtility, TestTruncateUri) {
   Http::TestRequestHeaderMapImpl request_headers_truncated_path = {{":method", "GET"},
                                                                    {":path", "/hello_world"},
                                                                    {":authority", "example.com"},
-                                                                   {"x-forwarded-proto", "http"}};
+                                                                   {":scheme", "http"}};
   EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, 2), "http://example.com/h");
 }
 
@@ -1334,7 +1334,7 @@ TEST(HttpUtility, TestUriUsesOriginalPath) {
       {":method", "GET"},
       {":path", "/hello_world"},
       {":authority", "example.com"},
-      {"x-forwarded-proto", "http"},
+      {":scheme", "http"},
       {"x-envoy-original-path", "/goodbye_world"}};
   EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, {}),
             "http://example.com/goodbye_world");