Skip to content

Commit

Permalink
Allow upstream to receive x-envoy-original-path headers (#26649)
Browse files Browse the repository at this point in the history 
Signed-off-by: Raven Black <ravenblack@dropbox.com>
  • Loading branch information
@ravenblackx @phlax
ravenblackx authored and phlax committed Apr 13, 2023
1 parent da7b710 commit 05c9c2b
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 10 deletions.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ bug_fixes:
- area: dependency
change: |
update Curl -> 8.0.1 to resolve CVE-2023-27535, CVE-2023-27536, CVE-2023-27538.
- area: http
change: |
amend the fix for ``x-envoy-original-path`` so it removes the header only at edge.
Previously this would also remove the header at any Envoy instance upstream of an external request, including an Envoy instance that added the header.
removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`
Expand Down
11 changes: 8 additions & 3 deletions source/common/http/conn_manager_utility.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,19 @@ void ConnectionManagerUtility::cleanInternalHeaders(
RequestHeaderMap& request_headers, bool edge_request,
const std::list<Http::LowerCaseString>& internal_only_headers) {
if (edge_request) {
// Headers to be stripped from edge requests, i.e. to sanitize so
// clients can't inject values.
request_headers.removeEnvoyDecoratorOperation();
request_headers.removeEnvoyDownstreamServiceCluster();
request_headers.removeEnvoyDownstreamServiceNode();
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_original_path")) {
request_headers.removeEnvoyOriginalPath();
}
}

// Headers to be stripped from edge *and* intermediate-hop external requests.
// TODO: some of these should only be stripped at edge, i.e. moved into
// the block above.
request_headers.removeEnvoyRetriableStatusCodes();
request_headers.removeEnvoyRetriableHeaderNames();
request_headers.removeEnvoyRetryOn();
Expand All @@ -324,9 +332,6 @@ void ConnectionManagerUtility::cleanInternalHeaders(
request_headers.removeEnvoyIpTags();
request_headers.removeEnvoyOriginalUrl();
request_headers.removeEnvoyHedgeOnPerTryTimeout();
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.sanitize_original_path")) {
request_headers.removeEnvoyOriginalPath();
}

for (const LowerCaseString& header : internal_only_headers) {
request_headers.remove(header);
Expand Down
47 changes: 40 additions & 7 deletions test/common/http/conn_manager_utility_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ TEST_F(ConnectionManagerUtilityTest, OverrideRequestIdForExternalRequests) {
TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestUseRemote) {
connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(
std::make_shared<Network::Address::Ipv4Instance>("50.0.0.1"));
ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(true));
EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(true));
route_config_.internal_only_headers_.push_back(LowerCaseString("custom_header"));
TestRequestHeaderMapImpl headers{{"x-envoy-decorator-operation", "foo"},
{"x-envoy-downstream-service-cluster", "foo"},
Expand All @@ -776,6 +776,7 @@ TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestUseRemote) {
EXPECT_EQ("50.0.0.1", headers.get_("x-envoy-external-address"));
EXPECT_FALSE(headers.has("x-envoy-decorator-operation"));
EXPECT_FALSE(headers.has("x-envoy-downstream-service-cluster"));
EXPECT_FALSE(headers.has("x-envoy-original-path"));
EXPECT_FALSE(headers.has("x-envoy-hedge-on-per-try-timeout"));
EXPECT_FALSE(headers.has("x-envoy-retriable-status-codes"));
EXPECT_FALSE(headers.has("x-envoy-retry-on"));
Expand All @@ -787,23 +788,55 @@ TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestUseRemote) {
EXPECT_FALSE(headers.has("x-envoy-expected-rq-timeout-ms"));
EXPECT_FALSE(headers.has("x-envoy-ip-tags"));
EXPECT_FALSE(headers.has("x-envoy-original-url"));
EXPECT_FALSE(headers.has("x-envoy-original-path"));
EXPECT_FALSE(headers.has("custom_header"));
}

// A request that is from an external address, but does not use remote address, should pull the
// address from XFF.
// A request that does not use remote address and is from an external address should pull the
// external address from x-forwarded-for, and should not remove only-edge-sanitized headers.
TEST_F(ConnectionManagerUtilityTest, ExternalAddressExternalRequestDontUseRemote) {
connection_.stream_info_.downstream_connection_info_provider_->setRemoteAddress(
std::make_shared<Network::Address::Ipv4Instance>("60.0.0.2"));
ON_CALL(config_, useRemoteAddress()).WillByDefault(Return(false));
std::make_shared<Network::Address::Ipv4Instance>("50.0.0.1"));
EXPECT_CALL(config_, useRemoteAddress()).WillRepeatedly(Return(false));
route_config_.internal_only_headers_.push_back(LowerCaseString("custom_header"));
TestRequestHeaderMapImpl headers{{"x-envoy-external-address", "60.0.0.1"},
{"x-forwarded-for", "60.0.0.1"}};
{"x-forwarded-for", "60.0.0.1"},
{"x-envoy-decorator-operation", "foo"},
{"x-envoy-downstream-service-cluster", "foo"},
{"x-envoy-hedge-on-per-try-timeout", "foo"},
{"x-envoy-retriable-status-codes", "123,456"},
{"x-envoy-retry-on", "foo"},
{"x-envoy-retry-grpc-on", "foo"},
{"x-envoy-max-retries", "foo"},
{"x-envoy-upstream-alt-stat-name", "foo"},
{"x-envoy-upstream-rq-timeout-alt-response", "204"},
{"x-envoy-upstream-rq-timeout-ms", "foo"},
{"x-envoy-expected-rq-timeout-ms", "10"},
{"x-envoy-ip-tags", "bar"},
{"x-envoy-original-url", "my_url"},
{"x-envoy-original-path", "/my_path"},
{"custom_header", "foo"}};

EXPECT_EQ((MutateRequestRet{"60.0.0.1:0", false, Tracing::Reason::NotTraceable}),
callMutateRequestHeaders(headers, Protocol::Http2));
// Expected external address is the one from the header, not the one from immediate upstream,
// because useRemoteAddress is false.
EXPECT_EQ("60.0.0.1", headers.get_("x-envoy-external-address"));
EXPECT_EQ("60.0.0.1", headers.get_("x-forwarded-for"));
EXPECT_TRUE(headers.has("x-envoy-decorator-operation"));
EXPECT_TRUE(headers.has("x-envoy-downstream-service-cluster"));
EXPECT_TRUE(headers.has("x-envoy-original-path"));
EXPECT_FALSE(headers.has("x-envoy-hedge-on-per-try-timeout"));
EXPECT_FALSE(headers.has("x-envoy-retriable-status-codes"));
EXPECT_FALSE(headers.has("x-envoy-retry-on"));
EXPECT_FALSE(headers.has("x-envoy-retry-grpc-on"));
EXPECT_FALSE(headers.has("x-envoy-max-retries"));
EXPECT_FALSE(headers.has("x-envoy-upstream-alt-stat-name"));
EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-alt-response"));
EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms"));
EXPECT_FALSE(headers.has("x-envoy-expected-rq-timeout-ms"));
EXPECT_FALSE(headers.has("x-envoy-ip-tags"));
EXPECT_FALSE(headers.has("x-envoy-original-url"));
EXPECT_FALSE(headers.has("custom_header"));
}

// Verify that if XFF is invalid we fall back to remote address, even if it is a pipe.
Expand Down

0 comments on commit 05c9c2b

Please sign in to comment.