Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: forwarding x-forwarded-proto from trusted proxies #7995

Merged
merged 4 commits into from
Aug 22, 2019
Merged

http: forwarding x-forwarded-proto from trusted proxies #7995

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54c956e
http: forwarding x-forwarded-proto from trusted proxies
alyssawilk Aug 20, 2019
02fce9f
fixing asan test bug
alyssawilk Aug 21, 2019
8dc7cbc
reviewer comments
alyssawilk Aug 22, 2019
6e3142d
comment
alyssawilk Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +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: 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>`.
Expand Down
16 changes: 14 additions & 2 deletions source/common/http/conn_manager_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
bool trusted_forwarded_proto =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const

Runtime::runtimeFeatureEnabled("envoy.reloadable_features.trusted_forwarded_proto");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, generally false -> true -> remove, but for bugfixes and low risk code I think it's fine to set true by default so done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand All @@ -107,8 +110,17 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest
Utility::appendXff(request_headers, *connection.remoteAddress());
}
}
request_headers.insertForwardedProto().value().setReference(
connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http);
if (trusted_forwarded_proto) {
mattklein123 marked this conversation as resolved.
Show resolved Hide resolved
// Set x-forwarded-proto unless it already exists and was forwarded on by a trusted proxy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you rephrase this comment to read more in line with the logic that follows (i.e. or vs. and, etc.). Normally I wouldn't be picky like this but this code is so hard to understand as it is that any comments hopefully should follow pretty closely. (I'm having a bit of a hard time groking what is happening here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Also note that I could just overwrite if num_trusted_hops is 0 and allow the logic at :139 to set proto if absent if you prefer that. I felt it was more clear to have it in one place than fall through, but that may just be me :-)

if (xff_num_trusted_hops == 0 || request_headers.ForwardedProto() == nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this respect skip_xff_append?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.
skip_xff_append is to make the proxy invisible from a fowarding perspective. We don't have a general "stop appending headers" config, we do them one at a time (there's already config knobs to avoid adding Via and I'm adding one for Server shortly). Meanwhile if we skip adding it at all we'd break things in the router, so all this code does is decide if we set an original value or trust the downstream value, which I think is orthogonal from if we want this proxy to be a visible hop in xff at the usptream server.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that X-Forwarded-For and X-Forwarded-Proto are kind of coupled together (and they are part of the same "entry" in the Forwarded (RFC7239) header), so it would be weird seeing X-Forwarded-Proto alone...

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 {
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
Expand Down
1 change: 1 addition & 0 deletions test/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ envoy_cc_test(
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/ssl:ssl_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:test_runtime_lib",
"//test/test_common:utility_lib",
],
)
Expand Down
45 changes: 45 additions & 0 deletions test/common/http/conn_manager_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "test/mocks/runtime/mocks.h"
#include "test/mocks/ssl/mocks.h"
#include "test/test_common/printers.h"
#include "test/test_common/test_runtime.h"
#include "test/test_common/utility.h"

#include "gmock/gmock.h"
Expand Down Expand Up @@ -226,6 +227,50 @@ TEST_F(ConnectionManagerUtilityTest, SkipXffAppendPassThruUseRemoteAddress) {
EXPECT_EQ("198.51.100.1", headers.ForwardedFor()->value().getStringView());
}

TEST_F(ConnectionManagerUtilityTest, ForwardedProtoLegacyBehavior) {
TestScopedRuntime scoped_runtime;
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.trusted_forwarded_proto", "false"}});

ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1));
EXPECT_CALL(config_, skipXffAppend()).WillOnce(Return(true));
connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("12.12.12.12");
ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};

callMutateRequestHeaders(headers, Protocol::Http2);
EXPECT_EQ("http", headers.ForwardedProto()->value().getStringView());
}

TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternal) {
TestScopedRuntime scoped_runtime;
Runtime::LoaderSingleton::getExisting()->mergeValues(
{{"envoy.reloadable_features.trusted_forwarded_proto", "true"}});

ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(1));
EXPECT_CALL(config_, skipXffAppend()).WillOnce(Return(true));
connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("12.12.12.12");
ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};

callMutateRequestHeaders(headers, Protocol::Http2);
EXPECT_EQ("https", headers.ForwardedProto()->value().getStringView());
}

TEST_F(ConnectionManagerUtilityTest, OverwriteForwardedProtoWhenExternal) {
ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
ON_CALL(config_, xffNumTrustedHops()).WillByDefault(Return(0));
connection_.remote_address_ = std::make_shared<Network::Address::Ipv4Instance>("127.0.0.1");
TestHeaderMapImpl headers{{"x-forwarded-proto", "https"}};
Network::Address::Ipv4Instance local_address("10.3.2.1");
ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address));

callMutateRequestHeaders(headers, Protocol::Http2);
EXPECT_EQ("http", headers.ForwardedProto()->value().getStringView());
}

// Verify internal request and XFF is set when we are using remote address and the address is
// internal according to user configuration.
TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenUserConfiguredRemoteAddress) {
Expand Down
15 changes: 15 additions & 0 deletions test/test_common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@ envoy_cc_test_library(
],
)

envoy_cc_test_library(
name = "test_runtime_lib",
hdrs = ["test_runtime.h"],
deps = [
"//source/common/runtime:runtime_lib",
"//source/common/stats:isolated_store_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/init:init_mocks",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/protobuf:protobuf_mocks",
"//test/mocks/runtime:runtime_mocks",
"//test/mocks/thread_local:thread_local_mocks",
],
)

envoy_cc_test_library(
name = "thread_factory_for_test_lib",
srcs = ["thread_factory_for_test.cc"],
Expand Down
49 changes: 49 additions & 0 deletions test/test_common/test_runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// 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"}});

#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;
config.add_layers()->mutable_admin_layer();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a small comment that this is required for mergeValues() to work? This wasn't immediately clear to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>(Runtime::LoaderPtr{
new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make_unique

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