From 8dc7cbc3eadc74fff20fd9f8ae38bad54422e7c3 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk <alyssar@chromium.org> Date: Thu, 22 Aug 2019 10:38:21 -0400 Subject: [PATCH] reviewer comments Signed-off-by: Alyssa Wilk <alyssar@chromium.org> --- docs/root/intro/version_history.rst | 2 +- source/common/http/conn_manager_utility.cc | 12 ++++++++---- source/common/runtime/runtime_features.cc | 1 + test/test_common/test_runtime.h | 9 ++++++--- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index a9520ce49345..91addd15eb92 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -25,7 +25,7 @@ Version history * grpc-json: added support for :ref:`ignoring unknown query parameters<envoy_api_field_config.filter.http.transcoder.v2.GrpcJsonTranscoder.ignore_unknown_query_parameters>`. * header to metadata: added :ref:`PROTOBUF_VALUE <envoy_api_enum_value_config.filter.http.header_to_metadata.v2.Config.ValueType.PROTOBUF_VALUE>` and :ref:`ValueEncode <envoy_api_enum_config.filter.http.header_to_metadata.v2.Config.ValueEncode>` to support protobuf Value and Base64 encoding. * http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`. -* http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto`. +* http: changed Envoy to forward existing x-forwarded-proto from upstream trusted proxies. Guarded by `envoy.reloadable_features.trusted_forwarded_proto` which defaults true. * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path. * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`. diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index b64112cc835a..9c1b8a3b1ea6 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -85,7 +85,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest Network::Address::InstanceConstSharedPtr final_remote_address; bool single_xff_address; const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops(); - bool trusted_forwarded_proto = + const bool trusted_forwarded_proto = Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto"); if (config.useRemoteAddress()) { @@ -111,13 +111,17 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest } } if (trusted_forwarded_proto) { - // Set x-forwarded-proto unless it already exists and was forwarded on by a trusted proxy. + // If the prior hop is not a trusted proxy, overwrite any x-forwarded-proto value it set as + // untrusted. Alternately if no x-forwarded-proto header exists, add one. if (xff_num_trusted_hops == 0 || request_headers.ForwardedProto() == nullptr) { request_headers.insertForwardedProto().value().setReference( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); } } else { + // Previously, before the trusted_forwarded_proto logic, Envoy would always overwrite the + // x-forwarded-proto header even if it was set by a trusted proxy. This code path is + // deprecated and will be removed. request_headers.insertForwardedProto().value().setReference( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); } @@ -130,8 +134,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest single_xff_address = ret.single_address_; } - // If we didn't already replace x-forwarded-proto because we are using the remote address, and - // remote hasn't set it (trusted proxy), we set it, since we then use this for setting scheme. + // If the x-forwarded-proto header is not set, set it here, since Envoy uses it for determining + // scheme and communicating it upstream. if (!request_headers.ForwardedProto()) { request_headers.insertForwardedProto().value().setReference( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index e1dc2a53bc91..db42217c8233 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -26,6 +26,7 @@ constexpr const char* runtime_features[] = { // Enabled "envoy.reloadable_features.test_feature_true", "envoy.reloadable_features.buffer_filter_populate_content_length", + "envoy.reloadable_features.trusted_forwarded_proto", }; // This is a list of configuration fields which are disallowed by default in Envoy diff --git a/test/test_common/test_runtime.h b/test/test_common/test_runtime.h index 0d109256cf56..afbdd257babc 100644 --- a/test/test_common/test_runtime.h +++ b/test/test_common/test_runtime.h @@ -5,6 +5,9 @@ // TestScopedRuntime scoped_runtime; // Runtime::LoaderSingleton::getExisting()->mergeValues( // {{"envoy.reloadable_features.test_feature_true", "false"}}); +// +// As long as a TestScopedRuntime exists, Runtime::LoaderSingleton::getExisting()->mergeValues() +// can safely be called to override runtime values. #pragma once @@ -29,9 +32,9 @@ class TestScopedRuntime { envoy::config::bootstrap::v2::LayeredRuntime config; config.add_layers()->mutable_admin_layer(); - loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>(Runtime::LoaderPtr{ - new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_, - generator_, validation_visitor_, *api_)}); + loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>( + std::make_unique<Runtime::LoaderImpl>(dispatcher_, tls_, config, local_info_, init_manager_, + store_, generator_, validation_visitor_, *api_)); } private: