http: sending the original url scheme (or best guess) upstream. (#15321)

This replaces prior logic where the :scheme header was consistently overwritten based on the encryption level of the upstream connection.

Risk Level: High (l7 change)
Testing: new integration tests, unit tests
Docs Changes: api docs updated
Release Notes: inline
Runtime guard: envoy.reloadable_features.preserve_downstream_scheme
Part of #14587
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

api/envoy/service/auth/v3/attribute_context.proto

@@ -123,9 +123,7 @@ message AttributeContext {
     // The HTTP request `Host` or 'Authority` header value.
     string host = 5;
 
-    // The HTTP URL scheme, such as `http` and `https`. This is set for HTTP/2
-    // requests only. For HTTP/1.1, use "x-forwarded-for" header value to lookup
-    // the scheme of the request.
+    // The HTTP URL scheme, such as `http` and `https`.
     string scheme = 6;
 
     // This field is always empty, and exists for compatibility reasons. The HTTP URL query is

docs/root/version_history/current.rst

@@ -6,6 +6,7 @@ Incompatible Behavior Changes
 *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required*
 
 * grpc_stats: the default value for :ref:`stats_for_all_methods <envoy_v3_api_field_extensions.filters.http.grpc_stats.v3.FilterConfig.stats_for_all_methods>` is switched from true to false, in order to avoid possible memory exhaustion due to an untrusted downstream sending a large number of unique method names. The previous default value was deprecated in version 1.14.0. This only changes the behavior when the value is not set. The previous behavior can be used by setting the value to true. This behavior change by be overridden by setting runtime feature `envoy.deprecated_features.grpc_stats_filter_enable_stats_for_all_methods_by_default`.
+* http: fixing a standards compliance issue with :scheme. The :scheme header sent upstream is now based on the original URL scheme, rather than set based on the security of the upstream connection. This behavior can be temporarily reverted by setting `envoy.reloadable_features.preserve_downstream_scheme` to false.
 * http: resolving inconsistencies between :scheme and X-Forwarded-Proto. :scheme will now be set for all HTTP/1.1 requests. This changes the behavior of the gRPC access logger, Wasm filters, CSRF filter and oath2 filter for HTTP/1 traffic, where :scheme was previously not set. This change also validates that for front-line Envoys (Envoys configured with  :ref:`xff_num_trusted_hops <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.xff_num_trusted_hops>` set to 0 and :ref:`use_remote_address <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.use_remote_address>` set to true) that HTTP/1.1 https schemed requests can not be sent over non-TLS connections. All behavioral changes listed here can be temporarily reverted by setting `envoy.reloadable_features.add_and_validate_scheme_header` to false.
 
 Minor Behavior Changes

source/common/http/conn_manager_utility.cc

@@ -152,8 +152,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
   }
   // 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, especially in the case of L2
-  // Envoys, where :scheme is set based on transport security.
+  // X-Forwarded-Proto and :scheme may still differ if different values are sent from downstream.
   if (!request_headers.Scheme() &&
       Runtime::runtimeFeatureEnabled("envoy.reloadable_features.add_and_validate_scheme_header")) {
     request_headers.setScheme(

source/common/router/router.cc

@@ -79,8 +79,23 @@ uint64_t FilterUtility::percentageOfTimeout(const std::chrono::milliseconds resp
   return static_cast<uint64_t>(response_time.count() * TimeoutPrecisionFactor / timeout.count());
 }
 
-void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool use_secure_transport) {
-  if (use_secure_transport) {
+void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool downstream_secure,
+                                      bool upstream_secure) {
+  if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.preserve_downstream_scheme")) {
+    if (Http::HeaderUtility::schemeIsValid(headers.getSchemeValue())) {
+      return;
+    }
+    if (Http::HeaderUtility::schemeIsValid(headers.getForwardedProtoValue())) {
+      headers.setScheme(headers.getForwardedProtoValue());
+      return;
+    }
+  }
+  const bool transport_secure =
+      Runtime::runtimeFeatureEnabled("envoy.reloadable_features.preserve_downstream_scheme")
+          ? downstream_secure
+          : upstream_secure;
+
+  if (transport_secure) {
     headers.setReferenceScheme(Http::Headers::get().SchemeValues.Https);
   } else {
     headers.setReferenceScheme(Http::Headers::get().SchemeValues.Http);
@@ -591,6 +606,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers,
   route_entry_->finalizeRequestHeaders(headers, callbacks_->streamInfo(),
                                        !config_.suppress_envoy_headers_);
   FilterUtility::setUpstreamScheme(headers,
+                                   callbacks_->streamInfo().downstreamSslConnection() != nullptr,
                                    host->transportSocketFactory().implementsSecureTransport());
 
   // Ensure an http transport scheme is selected before continuing with decoding.

source/common/router/router.h

@@ -113,9 +113,13 @@ class FilterUtility {
                                       const std::chrono::milliseconds timeout);
 
   /**
-   * Set the :scheme header based on whether the underline transport is secure.
+   * Set the :scheme header using the best information available. In order this is
+   * - existing scheme header if valid
+   * - x-forwarded-proto header if valid
+   * - security of downstream connection
    */
-  static void setUpstreamScheme(Http::RequestHeaderMap& headers, bool use_secure_transport);
+  static void setUpstreamScheme(Http::RequestHeaderMap& headers, bool downstream_secure,
+                                bool upstream_secure);
 
   /**
    * Determine whether a request should be shadowed.

source/common/runtime/runtime_features.cc

@@ -79,6 +79,7 @@ constexpr const char* runtime_features[] = {
     "envoy.reloadable_features.improved_stream_limit_handling",
     "envoy.reloadable_features.overload_manager_disable_keepalive_drain_http2",
     "envoy.reloadable_features.prefer_quic_kernel_bpf_packet_routing",
+    "envoy.reloadable_features.preserve_downstream_scheme",
     "envoy.reloadable_features.remove_forked_chromium_url",
     "envoy.reloadable_features.require_ocsp_response_for_must_staple_certs",
     "envoy.reloadable_features.stop_faking_paths",

source/common/upstream/health_checker_impl.cc

@@ -274,7 +274,11 @@ void HttpHealthCheckerImpl::HttpActiveHealthCheckSession::onInterval() {
        {Http::Headers::get().Path, parent_.path_},
        {Http::Headers::get().UserAgent, Http::Headers::get().UserAgentValues.EnvoyHealthChecker}});
   Router::FilterUtility::setUpstreamScheme(
-      *request_headers, host_->transportSocketFactory().implementsSecureTransport());
+      *request_headers,
+      // Here there is no downstream connection so scheme will be based on
+      // upstream crypto
+      host_->transportSocketFactory().implementsSecureTransport(),
+      host_->transportSocketFactory().implementsSecureTransport());
   StreamInfo::StreamInfoImpl stream_info(protocol_, parent_.dispatcher_.timeSource(),
                                          local_address_provider_);
   stream_info.onUpstreamHostSelected(host_);
@@ -720,7 +724,11 @@ void GrpcHealthCheckerImpl::GrpcActiveHealthCheckSession::onInterval() {
   Grpc::Common::toGrpcTimeout(parent_.timeout_, headers_message->headers());
 
   Router::FilterUtility::setUpstreamScheme(
-      headers_message->headers(), host_->transportSocketFactory().implementsSecureTransport());
+      headers_message->headers(),
+      // Here there is no downstream connection so scheme will be based on
+      // upstream crypto
+      host_->transportSocketFactory().implementsSecureTransport(),
+      host_->transportSocketFactory().implementsSecureTransport());
 
   auto status = request_encoder_->encodeHeaders(headers_message->headers(), false);
   // Encoding will only fail if required headers are missing.

test/common/router/router_test.cc

@@ -5424,14 +5424,58 @@ TEST(RouterFilterUtilityTest, FinalTimeoutSupressEnvoyHeaders) {
 }
 
 TEST(RouterFilterUtilityTest, SetUpstreamScheme) {
+  TestScopedRuntime scoped_runtime;
+
+  // With no scheme and x-forwarded-proto, set scheme based on encryption level
+  {
+    Http::TestRequestHeaderMapImpl headers;
+    FilterUtility::setUpstreamScheme(headers, false, false);
+    EXPECT_EQ("http", headers.get_(":scheme"));
+  }
+  {
+    Http::TestRequestHeaderMapImpl headers;
+    FilterUtility::setUpstreamScheme(headers, true, true);
+    EXPECT_EQ("https", headers.get_(":scheme"));
+  }
+
+  // With invalid x-forwarded-proto, still use scheme.
+  {
+    Http::TestRequestHeaderMapImpl headers;
+    headers.setForwardedProto("foo");
+    FilterUtility::setUpstreamScheme(headers, true, true);
+    EXPECT_EQ("https", headers.get_(":scheme"));
+  }
+
+  // Use valid x-forwarded-proto.
+  {
+    Http::TestRequestHeaderMapImpl headers;
+    headers.setForwardedProto(Http::Headers::get().SchemeValues.Http);
+    FilterUtility::setUpstreamScheme(headers, true, true);
+    EXPECT_EQ("http", headers.get_(":scheme"));
+  }
+
+  // Trust scheme over x-forwarded-proto.
   {
     Http::TestRequestHeaderMapImpl headers;
-    FilterUtility::setUpstreamScheme(headers, false);
+    headers.setScheme(Http::Headers::get().SchemeValues.Https);
+    headers.setForwardedProto(Http::Headers::get().SchemeValues.Http);
+    FilterUtility::setUpstreamScheme(headers, false, false);
+    EXPECT_EQ("https", headers.get_(":scheme"));
+  }
+
+  // New logic uses downstream crypto
+  {
+    Http::TestRequestHeaderMapImpl headers;
+    FilterUtility::setUpstreamScheme(headers, false, true);
     EXPECT_EQ("http", headers.get_(":scheme"));
   }
+
+  // Legacy logic uses upstream crypto
+  Runtime::LoaderSingleton::getExisting()->mergeValues(
+      {{"envoy.reloadable_features.preserve_downstream_scheme", "false"}});
   {
     Http::TestRequestHeaderMapImpl headers;
-    FilterUtility::setUpstreamScheme(headers, true);
+    FilterUtility::setUpstreamScheme(headers, false, true);
     EXPECT_EQ("https", headers.get_(":scheme"));
   }
 }

test/common/upstream/health_checker_impl_test.cc

@@ -1099,7 +1099,7 @@ TEST_F(HttpHealthCheckerImplTest, TlsOptions) {
     )EOF";
 
   auto socket_factory = new Network::MockTransportSocketFactory();
-  EXPECT_CALL(*socket_factory, implementsSecureTransport()).WillOnce(Return(true));
+  EXPECT_CALL(*socket_factory, implementsSecureTransport()).Times(2).WillRepeatedly(Return(true));
   auto transport_socket_match = new NiceMock<Upstream::MockTransportSocketMatcher>(
       Network::TransportSocketFactoryPtr(socket_factory));
   cluster_->info_->transport_socket_matcher_.reset(transport_socket_match);
@@ -2564,7 +2564,9 @@ TEST_F(HttpHealthCheckerImplTest, TransportSocketMatchCriteria) {
   // We expect that this default_socket_factory will NOT be used to create a transport socket for
   // the health check connection.
   EXPECT_CALL(*default_socket_factory, createTransportSocket(_)).Times(0);
-  EXPECT_CALL(*default_socket_factory, implementsSecureTransport());
+  EXPECT_CALL(*default_socket_factory, implementsSecureTransport())
+      .Times(2)
+      .WillRepeatedly(Return(true));
   auto transport_socket_match =
       std::make_unique<Upstream::MockTransportSocketMatcher>(std::move(default_socket_factory));
 
@@ -2625,7 +2627,9 @@ TEST_F(HttpHealthCheckerImplTest, NoTransportSocketMatchCriteria) {
   // The default_socket_factory should be used to create a transport socket for the health check
   // connection.
   EXPECT_CALL(*default_socket_factory, createTransportSocket(_));
-  EXPECT_CALL(*default_socket_factory, implementsSecureTransport());
+  EXPECT_CALL(*default_socket_factory, implementsSecureTransport())
+      .Times(2)
+      .WillRepeatedly(Return(true));
   auto transport_socket_match =
       std::make_unique<Upstream::MockTransportSocketMatcher>(std::move(default_socket_factory));
   // We expect resolve() to be called exactly once for endpoint socket matching. We should not

test/integration/http2_upstream_integration_test.cc

@@ -57,6 +57,33 @@ TEST_P(Http2UpstreamIntegrationTest, GrpcRetry) { testGrpcRetry(); }
 
 TEST_P(Http2UpstreamIntegrationTest, Trailers) { testTrailers(1024, 2048, true, true); }
 
+TEST_P(Http2UpstreamIntegrationTest, TestSchemeAndXFP) {
+  autonomous_upstream_ = true;
+  initialize();
+
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+
+  auto check_preserved = ([&](absl::string_view scheme, absl::string_view xff) {
+    {
+      default_request_headers_.setScheme(scheme);
+      default_request_headers_.setForwardedProto(xff);
+      auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+      response->waitForEndStream();
+      auto headers = reinterpret_cast<AutonomousUpstream*>(fake_upstreams_.front().get())
+                         ->lastRequestHeaders();
+      // Ensure original scheme and x-forwarded-proto are preserved.
+      EXPECT_EQ(headers->getSchemeValue(), scheme);
+      EXPECT_EQ(headers->getForwardedProtoValue(), xff);
+    }
+  });
+
+  // Ensure regardless of value, scheme and x-forwarded-proto are independently preserved.
+  check_preserved("http", "https");
+  check_preserved("https", "http");
+
+  codec_client_->close();
+}
+
 // Ensure Envoy handles streaming requests and responses simultaneously.
 void Http2UpstreamIntegrationTest::bidirectionalStreaming(uint32_t bytes) {
   initialize();