diff --git a/api/envoy/service/auth/v3/attribute_context.proto b/api/envoy/service/auth/v3/attribute_context.proto index 8dfecd3ac958..cdf3ee9f96e4 100644 --- a/api/envoy/service/auth/v3/attribute_context.proto +++ b/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 diff --git a/api/envoy/service/auth/v4alpha/attribute_context.proto b/api/envoy/service/auth/v4alpha/attribute_context.proto index 2af2aa0741ad..a1bf9c9c62cb 100644 --- a/api/envoy/service/auth/v4alpha/attribute_context.proto +++ b/api/envoy/service/auth/v4alpha/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 diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f118851ef7be..3c868585fc29 100644 --- a/docs/root/version_history/current.rst +++ b/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 ` 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 ` set to 0 and :ref:`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 diff --git a/generated_api_shadow/envoy/service/auth/v3/attribute_context.proto b/generated_api_shadow/envoy/service/auth/v3/attribute_context.proto index 8dfecd3ac958..cdf3ee9f96e4 100644 --- a/generated_api_shadow/envoy/service/auth/v3/attribute_context.proto +++ b/generated_api_shadow/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 diff --git a/generated_api_shadow/envoy/service/auth/v4alpha/attribute_context.proto b/generated_api_shadow/envoy/service/auth/v4alpha/attribute_context.proto index 2af2aa0741ad..a1bf9c9c62cb 100644 --- a/generated_api_shadow/envoy/service/auth/v4alpha/attribute_context.proto +++ b/generated_api_shadow/envoy/service/auth/v4alpha/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 diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 0b2b8a82149f..1d9d008a3bcc 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/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( diff --git a/source/common/router/router.cc b/source/common/router/router.cc index df46bed5f9b0..297042abc768 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -79,8 +79,23 @@ uint64_t FilterUtility::percentageOfTimeout(const std::chrono::milliseconds resp return static_cast(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. diff --git a/source/common/router/router.h b/source/common/router/router.h index 46db2fe32364..28d287c217b5 100644 --- a/source/common/router/router.h +++ b/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. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 282cff166213..be68b6260617 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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", diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc index 099e71a7c37e..7b64ed5fda85 100644 --- a/source/common/upstream/health_checker_impl.cc +++ b/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. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 5d982b354596..366f45aeba86 100644 --- a/test/common/router/router_test.cc +++ b/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")); } } diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index e57fe6104857..0b629193f47e 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/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( 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(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(std::move(default_socket_factory)); // We expect resolve() to be called exactly once for endpoint socket matching. We should not diff --git a/test/integration/http2_upstream_integration_test.cc b/test/integration/http2_upstream_integration_test.cc index 61b374609f1b..2eb725d5283b 100644 --- a/test/integration/http2_upstream_integration_test.cc +++ b/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(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();