http: forwarding x-forwarded-proto from trusted proxies #7995
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,9 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest | |
| Network::Address::InstanceConstSharedPtr final_remote_address; | ||
| bool single_xff_address; | ||
| const uint32_t xff_num_trusted_hops = config.xffNumTrustedHops(); | ||
| const bool trusted_forwarded_proto = | ||
| Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto"); | ||
|
There was a problem hiding this comment. Why not make this true by default? I guess do we have a documented procedure of the sequence for these type of features? false by default -> true by default -> remove old code path? There was a problem hiding this comment. Yeah, generally false -> true -> remove, but for bugfixes and low risk code I think it's fine to set true by default so done. There was a problem hiding this comment. Not related to this PR, but do your scripts track this somehow? I think this is all awesome, I just want to make sure that we don't forget about the sequence of things we need to do upon each release? There was a problem hiding this comment. Yep, the scripts grep through the whole code base and add any flags which aren't already defaulted true. |
||
|
|
||
| if (config.useRemoteAddress()) { | ||
| single_xff_address = request_headers.ForwardedFor() == nullptr; | ||
| // If there are any trusted proxies in front of this Envoy instance (as indicated by | ||
|
|
@@ -107,8 +110,21 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest | |
| Utility::appendXff(request_headers, *connection.remoteAddress()); | ||
| } | ||
| } | ||
| if (trusted_forwarded_proto) { | ||
mattklein123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 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) { | ||
|
There was a problem hiding this comment. Should this respect There was a problem hiding this comment. I don't think so. There was a problem hiding this comment. My thinking was that But if you think that's fine, then it's fine. I just wanted to make sure you didn't miss this. The code is definitely an improvement over what we have now. Thanks! |
||
| 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); | ||
| } | ||
| } else { | ||
| // If we are not using remote address, attempt to pull a valid IPv4 or IPv6 address out of XFF. | ||
| // If we find one, it will be used as the downstream address for logging. It may or may not be | ||
|
|
@@ -118,8 +134,8 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest | |
| single_xff_address = ret.single_address_; | ||
| } | ||
|
|
||
| // 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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| // A simple test utility to easily allow for runtime feature overloads in unit tests. | ||
| // | ||
| // As long as this class is in scope one can do runtime feature overrides: | ||
| // | ||
| // 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 | ||
|
|
||
| #include "common/runtime/runtime_impl.h" | ||
| #include "common/stats/isolated_store_impl.h" | ||
|
|
||
| #include "test/mocks/event/mocks.h" | ||
| #include "test/mocks/init/mocks.h" | ||
| #include "test/mocks/local_info/mocks.h" | ||
| #include "test/mocks/protobuf/mocks.h" | ||
| #include "test/mocks/runtime/mocks.h" | ||
| #include "test/mocks/thread_local/mocks.h" | ||
|
|
||
| #include "gmock/gmock.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| // TODO(alyssawilk) move existing runtime tests over to using this. | ||
| class TestScopedRuntime { | ||
| public: | ||
| TestScopedRuntime() : api_(Api::createApiForTest()) { | ||
| envoy::config::bootstrap::v2::LayeredRuntime config; | ||
| // The existence of an admin layer is required for mergeValues() to work. | ||
| config.add_layers()->mutable_admin_layer(); | ||
|
There was a problem hiding this comment. Can you add a small comment that this is required for mergeValues() to work? This wasn't immediately clear to me. There was a problem hiding this comment. Sorry I meant to add a comment above this line along the lines of "the existence of an admin layer is required for mergeValues() to work" |
||
|
|
||
| loader_ = std::make_unique<Runtime::ScopedLoaderSingleton>( | ||
| std::make_unique<Runtime::LoaderImpl>(dispatcher_, tls_, config, local_info_, init_manager_, | ||
| store_, generator_, validation_visitor_, *api_)); | ||
| } | ||
|
|
||
| private: | ||
| Event::MockDispatcher dispatcher_; | ||
| testing::NiceMock<ThreadLocal::MockInstance> tls_; | ||
| Stats::IsolatedStoreImpl store_; | ||
| Runtime::MockRandomGenerator generator_; | ||
| Api::ApiPtr api_; | ||
| testing::NiceMock<LocalInfo::MockLocalInfo> local_info_; | ||
| Init::MockManager init_manager_; | ||
| testing::NiceMock<ProtobufMessage::MockValidationVisitor> validation_visitor_; | ||
| std::unique_ptr<Runtime::ScopedLoaderSingleton> loader_; | ||
| }; | ||
|
|
||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm shouldn't this be:
"from downstream trusted proxies"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if so: #8021