From fb9e01db7cbe76235139740f6f9be37dd700d512 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 14 Jul 2021 16:44:27 -0400 Subject: [PATCH 1/4] http: switching from XFP to scheme Signed-off-by: Alyssa Wilk --- envoy/http/header_map.h | 4 +-- source/common/http/conn_manager_utility.cc | 16 +++++---- source/common/http/headers.h | 2 +- source/common/http/utility.cc | 12 +++++-- source/common/http/utility.h | 9 +++++ source/common/router/config_impl.cc | 34 ++++++++++++------ source/common/router/router.cc | 9 ++--- source/common/runtime/runtime_features.cc | 1 + source/common/tracing/http_tracer_impl.cc | 1 + source/extensions/common/aws/utility.cc | 2 +- .../filters/http/cache/cacheability_utils.cc | 6 ++-- .../filters/http/cache/http_cache.cc | 12 +++---- .../extensions/filters/http/oauth2/filter.cc | 3 +- test/common/http/async_client_impl_test.cc | 3 +- test/common/http/common.cc | 19 ++++++---- test/common/http/common.h | 3 +- test/common/http/conn_manager_impl_test.cc | 6 ++-- test/common/http/conn_manager_impl_test_2.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 12 +++---- test/common/http/utility_test.cc | 6 ++-- .../config_impl_headermap_benchmark_test.cc | 6 ++-- test/common/router/config_impl_test.cc | 34 +++++++++--------- test/common/router/rds_impl_test.cc | 2 +- test/common/router/router_ratelimit_test.cc | 2 +- test/common/router/router_test.cc | 9 ++--- test/common/router/router_test_base.cc | 2 +- test/common/tracing/http_tracer_impl_test.cc | 36 +++++++------------ .../http/cache/cache_custom_headers_test.cc | 2 +- .../filters/http/cache/cache_filter_test.cc | 2 +- .../http/cache/cacheability_utils_test.cc | 18 ++++------ .../filters/http/cache/http_cache_test.cc | 6 ++-- .../simple_http_cache_test.cc | 2 +- .../compressor_integration_tests.cc | 6 ++-- .../filters/http/ext_proc/filter_test.cc | 30 ++-------------- .../ext_proc/streaming_integration_test.cc | 4 +-- .../filters/http/oauth2/filter_test.cc | 26 +++++++------- test/integration/integration_test.cc | 1 + .../multiplexed_upstream_integration_test.cc | 4 +-- .../integration/websocket_integration_test.cc | 6 ++-- test/tools/router_check/router.cc | 2 +- 40 files changed, 179 insertions(+), 183 deletions(-) diff --git a/envoy/http/header_map.h b/envoy/http/header_map.h index a3f1fd855b417..3d7e2978526cc 100644 --- a/envoy/http/header_map.h +++ b/envoy/http/header_map.h @@ -304,7 +304,6 @@ class HeaderEntry { HEADER_FUNC(Expect) \ HEADER_FUNC(ForwardedClientCert) \ HEADER_FUNC(ForwardedFor) \ - HEADER_FUNC(ForwardedProto) \ HEADER_FUNC(GrpcTimeout) \ HEADER_FUNC(Host) \ HEADER_FUNC(Method) \ @@ -312,7 +311,8 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) + HEADER_FUNC(UserAgent) \ + HEADER_FUNC(XForwardedProto) #define INLINE_REQ_NUMERIC_HEADERS(HEADER_FUNC) \ HEADER_FUNC(EnvoyExpectedRequestTimeoutMs) \ diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 0b288a022bdb7..246c65bfc6c53 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -131,8 +131,8 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m } // 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.setReferenceForwardedProto( + if (xff_num_trusted_hops == 0 || request_headers.XForwardedProto() == nullptr) { + request_headers.setReferenceXForwardedProto( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); } } else { @@ -161,21 +161,23 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m // 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.setReferenceForwardedProto(connection.ssl() ? Headers::get().SchemeValues.Https - : Headers::get().SchemeValues.Http); + if (!request_headers.XForwardedProto()) { + request_headers.setReferenceXForwardedProto( + connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); } + if (config.schemeToSet().has_value()) { request_headers.setScheme(config.schemeToSet().value()); - request_headers.setForwardedProto(config.schemeToSet().value()); + request_headers.setXForwardedProto(config.schemeToSet().value()); } + // 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 if different values are sent from downstream. if (!request_headers.Scheme() && Runtime::runtimeFeatureEnabled("envoy.reloadable_features.add_and_validate_scheme_header")) { request_headers.setScheme( - getScheme(request_headers.getForwardedProtoValue(), connection.ssl() != nullptr)); + getScheme(request_headers.getXForwardedProtoValue(), connection.ssl() != nullptr)); } // At this point we can determine whether this is an internal or external request. The diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 50edd1fa92546..9f479ebdb7041 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -188,7 +188,6 @@ class HeaderValues { const LowerCaseString ForwardedClientCert{"x-forwarded-client-cert"}; const LowerCaseString ForwardedFor{"x-forwarded-for"}; const LowerCaseString ForwardedHost{"x-forwarded-host"}; - const LowerCaseString ForwardedProto{"x-forwarded-proto"}; const LowerCaseString GrpcMessage{"grpc-message"}; const LowerCaseString GrpcStatus{"grpc-status"}; const LowerCaseString GrpcTimeout{"grpc-timeout"}; @@ -215,6 +214,7 @@ class HeaderValues { const LowerCaseString Via{"via"}; const LowerCaseString WWWAuthenticate{"www-authenticate"}; const LowerCaseString XContentTypeOptions{"x-content-type-options"}; + const LowerCaseString XForwardedProto{"x-forwarded-proto"}; const LowerCaseString XSquashDebug{"x-squash-debug"}; struct { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 3320090c1c3a6..b64573f549ac7 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -680,7 +680,7 @@ bool Utility::sanitizeConnectionHeader(Http::RequestHeaderMap& headers) { } else if ((lcs_header_to_remove == Http::Headers::get().ForwardedFor) || (lcs_header_to_remove == Http::Headers::get().ForwardedHost) || - (lcs_header_to_remove == Http::Headers::get().ForwardedProto) || + (lcs_header_to_remove == Http::Headers::get().XForwardedProto) || !token_sv.find(':')) { // An attacker could nominate an X-Forwarded* header, and its removal may mask @@ -768,6 +768,14 @@ const std::string& Utility::getProtocolString(const Protocol protocol) { NOT_REACHED_GCOVR_EXCL_LINE; } +absl::string_view Utility::getScheme(const RequestHeaderMap& headers) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_scheme_header") && + !headers.getSchemeValue().empty()) { + return headers.getSchemeValue(); + } + return headers.getXForwardedProtoValue(); +} + std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_headers, const absl::optional max_path_length) { if (!request_headers.Path()) { @@ -781,7 +789,7 @@ std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_head path = path.substr(0, max_path_length.value()); } - return absl::StrCat(request_headers.getForwardedProtoValue(), "://", + return absl::StrCat(Http::Utility::getScheme(request_headers), "://", request_headers.getHostValue(), path); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index c5dd2e270d0cb..a0f422b01fb71 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -395,6 +395,15 @@ bool sanitizeConnectionHeader(Http::RequestHeaderMap& headers); */ const std::string& getProtocolString(const Protocol p); +/** + * Return the scheme of the request. + * For legacy code (envoy.reloadable_features.use_scheme_header == false) this + * will be the value of the X-Forwarded-Proto header value. By default it will + * return the scheme if present, otherwise the value of X-Forwarded-Proto if + * present. + */ +absl::string_view getScheme(const RequestHeaderMap& headers); + /** * Constructs the original URI sent from the client from * the request headers. diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index b1432fa02bbf7..de46456f33c6d 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -45,6 +45,11 @@ namespace Envoy { namespace Router { namespace { +absl::string_view getXFPOrScheme(const Http::RequestHeaderMap& headers) { + absl::string_view xfp = headers.getXForwardedProtoValue(); + return xfp.empty() ? headers.getSchemeValue() : xfp; +} + const std::string DEPRECATED_ROUTER_NAME = "envoy.router"; constexpr uint32_t DEFAULT_MAX_DIRECT_RESPONSE_BODY_SIZE_BYTES = 4096; @@ -750,7 +755,10 @@ absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHead if (host_end != absl::string_view::npos) { absl::string_view request_port = request_host.substr(host_end); - absl::string_view request_protocol = headers.getForwardedProtoValue(); + // In the rare case that XFP and scheme disagree (say http URL over an HTTPs + // connection), do port stripping based on XFP so http://foo.com:80 won't + // have the port stripped when served over TLS. + absl::string_view request_protocol = getXFPOrScheme(headers); bool remove_port = !new_port.empty(); if (new_scheme != request_protocol) { @@ -781,8 +789,9 @@ std::string RouteEntryImplBase::newPath(const Http::RequestHeaderMap& headers) c } else if (https_redirect_) { final_scheme = Http::Headers::get().SchemeValues.Https; } else { - ASSERT(headers.ForwardedProto()); - final_scheme = headers.getForwardedProtoValue(); + // Serve the redirect URL based on the scheme of the original URL, not the + // security of the underlying connection. + final_scheme = Http::Utility::getScheme(headers); } if (!port_redirect_.empty()) { @@ -1367,18 +1376,23 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { - // No x-forwarded-proto header. This normally only happens when ActiveStream::decodeHeaders - // bails early (as it rejects a request), so there is no routing is going to happen anyway. - const auto* forwarded_proto_header = headers.ForwardedProto(); - if (forwarded_proto_header == nullptr) { + // In the rare case that XFP and scheme disagree (say http URL over an HTTPs + // connection), force a redirect based on underlying protocol, rather than URL + // scheme, so don't force a redirect for a http:// url served over a TLS + // connection. + // TODO(alyssar, mattklein123) is the goal here to force TLS, or force https: + // scheme? Maybe upstreams will get confused if seeing an http url. + const absl::string_view scheme = getXFPOrScheme(headers); + if (scheme.empty()) { + // No scheme header. This normally only happens when ActiveStream::decodeHeaders + // bails early (as it rejects a request), so there is no routing is going to happen anyway. return nullptr; } // First check for ssl redirect. - if (ssl_requirements_ == SslRequirements::All && forwarded_proto_header->value() != "https") { + if (ssl_requirements_ == SslRequirements::All && scheme != "https") { return SSL_REDIRECT_ROUTE; - } else if (ssl_requirements_ == SslRequirements::ExternalOnly && - forwarded_proto_header->value() != "https" && + } else if (ssl_requirements_ == SslRequirements::ExternalOnly && scheme != "https" && !Http::HeaderUtility::isEnvoyInternalRequest(headers)) { return SSL_REDIRECT_ROUTE; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 79ea7b165065e..8e3f137093ac6 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -51,7 +51,7 @@ uint32_t getLength(const Buffer::Instance* instance) { return instance ? instanc bool schemeIsHttp(const Http::RequestHeaderMap& downstream_headers, const Network::Connection& connection) { - if (downstream_headers.getForwardedProtoValue() == Http::Headers::get().SchemeValues.Http) { + if (Http::Utility::getScheme(downstream_headers) == Http::Headers::get().SchemeValues.Http) { return true; } if (!connection.ssl()) { @@ -83,8 +83,9 @@ void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool down if (Http::HeaderUtility::schemeIsValid(headers.getSchemeValue())) { return; } - if (Http::HeaderUtility::schemeIsValid(headers.getForwardedProtoValue())) { - headers.setScheme(headers.getForwardedProtoValue()); + absl::string_view xfp = headers.getXForwardedProtoValue(); + if (Http::HeaderUtility::schemeIsValid(xfp)) { + headers.setScheme(xfp); return; } } @@ -1516,7 +1517,7 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do } const auto& policy = route_entry_->internalRedirectPolicy(); - // Don't allow serving TLS responses over plaintext unless allowed by policy. + // Don't change the scheme from the original request const bool scheme_is_http = schemeIsHttp(downstream_headers, *callbacks_->connection()); const bool target_is_http = absolute_url.scheme() == Http::Headers::get().SchemeValues.Http; if (!policy.isCrossSchemeRedirectAllowed() && scheme_is_http != target_is_http) { diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 6b21ac719f92f..b354c05a7a157 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -94,6 +94,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.vhds_heartbeats", "envoy.reloadable_features.wasm_cluster_name_envoy_grpc", "envoy.reloadable_features.upstream_http2_flood_checks", + "envoy.reloadable_features.use_scheme_header", "envoy.restart_features.use_apple_api_for_dns_lookups", "envoy.reloadable_features.header_map_correctly_coalesce_cookies", }; diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 35263ac7ce052..7df66b8189d18 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -16,6 +16,7 @@ #include "source/common/grpc/common.h" #include "source/common/http/codes.h" #include "source/common/http/header_map_impl.h" +#include "source/common/http/header_utility.h" #include "source/common/http/headers.h" #include "source/common/http/utility.h" #include "source/common/protobuf/utility.h" diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc index 497e37f409628..f4336362ea214 100644 --- a/source/extensions/common/aws/utility.cc +++ b/source/extensions/common/aws/utility.cc @@ -26,7 +26,7 @@ Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) { // Skip headers that are likely to mutate, when crossing proxies const auto key = entry.key().getStringView(); if (key == Http::Headers::get().ForwardedFor.get() || - key == Http::Headers::get().ForwardedProto.get() || key == "x-amzn-trace-id") { + key == Http::Headers::get().XForwardedProto.get() || key == "x-amzn-trace-id") { return Http::HeaderMap::Iterate::Continue; } diff --git a/source/extensions/filters/http/cache/cacheability_utils.cc b/source/extensions/filters/http/cache/cacheability_utils.cc index 20fe9107ebfe5..5a34dcb87c949 100644 --- a/source/extensions/filters/http/cache/cacheability_utils.cc +++ b/source/extensions/filters/http/cache/cacheability_utils.cc @@ -4,6 +4,7 @@ #include "source/common/common/macros.h" #include "source/common/common/utility.h" +#include "source/common/http/utility.h" #include "source/extensions/filters/http/cache/cache_custom_headers.h" namespace Envoy { @@ -33,7 +34,7 @@ const std::vector& conditionalHeaders() { bool CacheabilityUtils::canServeRequestFromCache(const Http::RequestHeaderMap& headers) { const absl::string_view method = headers.getMethodValue(); - const absl::string_view forwarded_proto = headers.getForwardedProtoValue(); + const absl::string_view scheme = Http::Utility::getScheme(headers); const Http::HeaderValues& header_values = Http::Headers::get(); // Check if the request contains any conditional headers. @@ -52,8 +53,7 @@ bool CacheabilityUtils::canServeRequestFromCache(const Http::RequestHeaderMap& h return headers.Path() && headers.Host() && !headers.getInline(CacheCustomHeaders::authorization()) && (method == header_values.MethodValues.Get || method == header_values.MethodValues.Head) && - (forwarded_proto == header_values.SchemeValues.Http || - forwarded_proto == header_values.SchemeValues.Https); + (scheme == header_values.SchemeValues.Http || scheme == header_values.SchemeValues.Https); } bool CacheabilityUtils::isCacheableResponse(const Http::ResponseHeaderMap& headers, diff --git a/source/extensions/filters/http/cache/http_cache.cc b/source/extensions/filters/http/cache/http_cache.cc index 2fef083676bfb..3a24900fb9822 100644 --- a/source/extensions/filters/http/cache/http_cache.cc +++ b/source/extensions/filters/http/cache/http_cache.cc @@ -9,6 +9,7 @@ #include "source/common/http/header_utility.h" #include "source/common/http/headers.h" +#include "source/common/http/utility.h" #include "source/common/protobuf/utility.h" #include "source/extensions/filters/http/cache/cache_custom_headers.h" #include "source/extensions/filters/http/cache/cache_headers_utils.h" @@ -29,17 +30,14 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst // CacheFilter doesn't create LookupRequests for such requests. ASSERT(request_headers.Path(), "Can't form cache lookup key for malformed Http::RequestHeaderMap " "with null Path."); - ASSERT( - request_headers.ForwardedProto(), - "Can't form cache lookup key for malformed Http::RequestHeaderMap with null ForwardedProto."); ASSERT(request_headers.Host(), "Can't form cache lookup key for malformed Http::RequestHeaderMap " "with null Host."); - const Http::HeaderString& forwarded_proto = request_headers.ForwardedProto()->value(); + absl::string_view scheme = Http::Utility::getScheme(request_headers); const auto& scheme_values = Http::Headers::get().SchemeValues; - ASSERT(forwarded_proto == scheme_values.Http || forwarded_proto == scheme_values.Https); + ASSERT(scheme == scheme_values.Http || scheme == scheme_values.Https); initializeRequestCacheControl(request_headers); - // TODO(toddmgreer): Let config determine whether to include forwarded_proto, host, and + // TODO(toddmgreer): Let config determine whether to include scheme, host, and // query params. // TODO(toddmgreer): get cluster name. if (request_headers.getMethodValue() == Http::Headers::get().MethodValues.Get) { @@ -51,7 +49,7 @@ LookupRequest::LookupRequest(const Http::RequestHeaderMap& request_headers, Syst key_.set_cluster_name("cluster_name_goes_here"); key_.set_host(std::string(request_headers.getHostValue())); key_.set_path(std::string(request_headers.getPathValue())); - key_.set_clear_http(forwarded_proto == scheme_values.Http); + key_.set_clear_http(scheme == scheme_values.Http); vary_headers_ = vary_allow_list.possibleVariedHeaders(request_headers); } diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index f7731ee44d7a2..8a42db24cd90e 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -386,8 +386,7 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap Http::ResponseHeaderMapPtr response_headers{Http::createHeaderMap( {{Http::Headers::get().Status, std::to_string(enumToInt(Http::Code::Found))}})}; - const std::string new_path = - absl::StrCat(headers.ForwardedProto()->value().getStringView(), "://", host_, "/"); + const std::string new_path = absl::StrCat(Http::Utility::getScheme(headers), "://", host_, "/"); response_headers->addReference(Http::Headers::get().SetCookie, SignoutCookieValue); response_headers->addReference(Http::Headers::get().SetCookie, SignoutBearerTokenValue); response_headers->setLocation(new_path); diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index ea81164f1cc42..2512daf88d25a 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1304,7 +1304,8 @@ TEST_F(AsyncClientImplTest, StreamTimeoutHeadReply) { })); RequestMessagePtr message{new RequestMessageImpl()}; - HttpTestUtility::addDefaultHeaders(message->headers(), "HEAD"); + message->headers().setMethod("HEAD"); + HttpTestUtility::addDefaultHeaders(message->headers(), false); EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(&message->headers()), true)); timer_ = new NiceMock(&dispatcher_); EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(40), _)); diff --git a/test/common/http/common.cc b/test/common/http/common.cc index cd37ab9b48453..d289b021b94ba 100644 --- a/test/common/http/common.cc +++ b/test/common/http/common.cc @@ -5,11 +5,18 @@ #include "envoy/http/header_map.h" namespace Envoy { -void HttpTestUtility::addDefaultHeaders(Http::RequestHeaderMap& headers, - const std::string default_method) { - headers.setScheme("http"); - headers.setMethod(default_method); - headers.setHost("host"); - headers.setPath("/"); +void HttpTestUtility::addDefaultHeaders(Http::RequestHeaderMap& headers, bool overwrite) { + if (overwrite || headers.getSchemeValue().empty()) { + headers.setScheme("http"); + } + if (overwrite || headers.getMethodValue().empty()) { + headers.setMethod("GET"); + } + if (overwrite || headers.getHostValue().empty()) { + headers.setHost("host"); + } + if (overwrite || headers.getPathValue().empty()) { + headers.setPath("/"); + } } } // namespace Envoy diff --git a/test/common/http/common.h b/test/common/http/common.h index b23c14535845c..2e78fb57e84c8 100644 --- a/test/common/http/common.h +++ b/test/common/http/common.h @@ -66,7 +66,6 @@ struct ConnPoolCallbacks : public Http::ConnectionPool::Callbacks { */ class HttpTestUtility { public: - static void addDefaultHeaders(Http::RequestHeaderMap& headers, - const std::string default_method = "GET"); + static void addDefaultHeaders(Http::RequestHeaderMap& headers, bool overwrite = true); }; } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 69894daf187aa..86df5da19f732 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -27,7 +27,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) { .Times(2) .WillRepeatedly(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getForwardedProtoValue()); + EXPECT_EQ("http", headers.getXForwardedProtoValue()); if (headers.Path()->value() == "/healthcheck") { filter->callbacks_->streamInfo().healthCheck(true); } @@ -92,7 +92,7 @@ TEST_F(HttpConnectionManagerImplTest, 100ContinueResponse) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillRepeatedly(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getForwardedProtoValue()); + EXPECT_EQ("http", headers.getXForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); @@ -3662,7 +3662,7 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillOnce(Invoke([](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("https", headers.getForwardedProtoValue()); + EXPECT_EQ("https", headers.getXForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index 82e09cee54e91..ffe48e846c168 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -2643,7 +2643,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponseUsingHttp3) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillOnce(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getForwardedProtoValue()); + EXPECT_EQ("http", headers.getXForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index ec66b39ad50d4..ffbcf490e0319 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -277,12 +277,12 @@ TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternal) { TestRequestHeaderMapImpl headers{{"x-forwarded-proto", "https"}}; callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("https", headers.getForwardedProtoValue()); + EXPECT_EQ("https", headers.getXForwardedProtoValue()); // Given :scheme was not set, it will be set to X-Forwarded-Proto EXPECT_EQ("https", headers.getSchemeValue()); // Make sure if x-forwarded-proto changes it doesn't cause problems. - headers.setForwardedProto("ftp"); + headers.setXForwardedProto("ftp"); EXPECT_EQ("https", headers.getSchemeValue()); } @@ -296,7 +296,7 @@ TEST_F(ConnectionManagerUtilityTest, OverwriteForwardedProtoWhenExternal) { ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address)); callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("http", headers.getForwardedProtoValue()); + EXPECT_EQ("http", headers.getXForwardedProtoValue()); // Given :scheme was not set, it will be set to X-Forwarded-Proto EXPECT_EQ("http", headers.getSchemeValue()); } @@ -313,7 +313,7 @@ TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternalButSetSch TestRequestHeaderMapImpl headers{{"x-forwarded-proto", "foo"}}; callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("foo", headers.getForwardedProtoValue()); + EXPECT_EQ("foo", headers.getXForwardedProtoValue()); // Given :scheme was not set, but X-Forwarded-Proto is not a valid scheme, // scheme will be set based on encryption level. EXPECT_EQ("http", headers.getSchemeValue()); @@ -329,7 +329,7 @@ TEST_F(ConnectionManagerUtilityTest, SchemeIsRespected) { ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address)); callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("http", headers.getForwardedProtoValue()); + EXPECT_EQ("http", headers.getXForwardedProtoValue()); // Given :scheme was set, it will not be changed. EXPECT_EQ("https", headers.getSchemeValue()); } @@ -348,7 +348,7 @@ TEST_F(ConnectionManagerUtilityTest, SchemeOverwrite) { config_.scheme_ = "https"; callMutateRequestHeaders(headers, Protocol::Http2); EXPECT_EQ("https", headers.getSchemeValue()); - EXPECT_EQ("https", headers.getForwardedProtoValue()); + EXPECT_EQ("https", headers.getXForwardedProtoValue()); } // Verify internal request and XFF is set when we are using remote address and the address is diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index f86935dece080..28accb0622446 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1317,7 +1317,7 @@ TEST(HttpUtility, TestRejectTeHeaderTooLong) { TEST(HttpUtility, TestRejectUriWithNoPath) { Http::TestRequestHeaderMapImpl request_headers_no_path = { - {":method", "GET"}, {":authority", "example.com"}, {"x-forwarded-proto", "http"}}; + {":method", "GET"}, {":authority", "example.com"}, {":scheme", "http"}}; EXPECT_EQ(Utility::buildOriginalUri(request_headers_no_path, {}), ""); } @@ -1325,7 +1325,7 @@ TEST(HttpUtility, TestTruncateUri) { Http::TestRequestHeaderMapImpl request_headers_truncated_path = {{":method", "GET"}, {":path", "/hello_world"}, {":authority", "example.com"}, - {"x-forwarded-proto", "http"}}; + {":scheme", "http"}}; EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, 2), "http://example.com/h"); } @@ -1334,7 +1334,7 @@ TEST(HttpUtility, TestUriUsesOriginalPath) { {":method", "GET"}, {":path", "/hello_world"}, {":authority", "example.com"}, - {"x-forwarded-proto", "http"}, + {":scheme", "http"}, {"x-envoy-original-path", "/goodbye_world"}}; EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, {}), "http://example.com/goodbye_world"); diff --git a/test/common/router/config_impl_headermap_benchmark_test.cc b/test/common/router/config_impl_headermap_benchmark_test.cc index 755f858c88378..365e7794b6c4f 100644 --- a/test/common/router/config_impl_headermap_benchmark_test.cc +++ b/test/common/router/config_impl_headermap_benchmark_test.cc @@ -60,10 +60,8 @@ static void manyCountryRoutesLongHeaders(benchmark::State& state) { ProtobufMessage::getNullValidationVisitor(), true); const auto stream_info = NiceMock(); - auto req_headers = Http::TestRequestHeaderMapImpl{{":authority", "www.lyft.com"}, - {":path", "/"}, - {":method", "GET"}, - {"x-forwarded-proto", "http"}}; + auto req_headers = Http::TestRequestHeaderMapImpl{ + {":authority", "www.lyft.com"}, {":path", "/"}, {":method", "GET"}, {":scheme", "http"}}; // Add dummy headers to reach ~100 headers (limit per request). for (int i = 0; i < 90; i++) { req_headers.addCopy(Http::LowerCaseString(absl::StrCat("dummyheader", i)), "some_value"); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index a403b857474a6..a3164809c904a 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -114,23 +114,21 @@ class TestConfigImpl : public ConfigImpl { Http::TestRequestHeaderMapImpl genPathlessHeaders(const std::string& host, const std::string& method) { - return Http::TestRequestHeaderMapImpl{{":authority", host}, {":method", method}, - {"x-safe", "safe"}, {"x-global-nope", "global"}, - {"x-vhost-nope", "vhost"}, {"x-route-nope", "route"}, - {"x-forwarded-proto", "http"}}; + return Http::TestRequestHeaderMapImpl{{":authority", host}, {":method", method}, + {"x-safe", "safe"}, {"x-global-nope", "global"}, + {"x-vhost-nope", "vhost"}, {"x-route-nope", "route"}, + {":scheme", "http"}}; } Http::TestRequestHeaderMapImpl genHeaders(const std::string& host, const std::string& path, - const std::string& method, - const std::string& forwarded_proto) { - auto hdrs = Http::TestRequestHeaderMapImpl{ - {":authority", host}, {":path", path}, - {":method", method}, {"x-safe", "safe"}, - {"x-global-nope", "global"}, {"x-vhost-nope", "vhost"}, - {"x-route-nope", "route"}, {"x-forwarded-proto", forwarded_proto}}; + const std::string& method, const std::string& scheme) { + auto hdrs = Http::TestRequestHeaderMapImpl{{":authority", host}, {":path", path}, + {":method", method}, {"x-safe", "safe"}, + {"x-global-nope", "global"}, {"x-vhost-nope", "vhost"}, + {"x-route-nope", "route"}, {":scheme", scheme}}; - if (forwarded_proto.empty()) { - hdrs.remove("x-forwarded-proto"); + if (scheme.empty()) { + hdrs.remove(":scheme"); } return hdrs; @@ -731,7 +729,7 @@ TEST_F(RouteMatcherTest, TestRoutes) { {}); TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); - // No host header, no x-forwarded-proto and no path header testing. + // No host header, no scheme and no path header testing. EXPECT_EQ(nullptr, config.route(Http::TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, 0)); EXPECT_EQ(nullptr, config.route(Http::TestRequestHeaderMapImpl{{":authority", "foo"}, @@ -740,7 +738,7 @@ TEST_F(RouteMatcherTest, TestRoutes) { 0)); EXPECT_EQ(nullptr, config.route(Http::TestRequestHeaderMapImpl{{":authority", "foo"}, {":method", "CONNECT"}, - {"x-forwarded-proto", "http"}}, + {":scheme", "http"}}, 0)); // Base routing testing. @@ -4179,14 +4177,14 @@ TEST_F(RouteMatcherTest, NoProtocolInHeadersWhenTlsIsRequired) { factory_context_.cluster_manager_.initializeClusters({"www"}, {}); TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true); - // route may be called early in some edge cases and "x-forwarded-proto" will not be set. + // route may be called early in some edge cases and ":scheme" will not be set. Http::TestRequestHeaderMapImpl headers{{":authority", "www.lyft.com"}, {":path", "/"}}; EXPECT_EQ(nullptr, config.route(headers, 0)); } /** * @brief Generate headers for testing - * @param ssl set true to insert "x-forwarded-proto: https", else "x-forwarded-proto: http" + * @param ssl set true to insert "":scheme: https", else ":scheme http" * @param internal nullopt for no such "x-envoy-internal" header, or explicit "true/false" * @return Http::TestRequestHeaderMapImpl */ @@ -4194,7 +4192,7 @@ static Http::TestRequestHeaderMapImpl genRedirectHeaders(const std::string& host const std::string& path, bool ssl, absl::optional internal) { Http::TestRequestHeaderMapImpl headers{ - {":authority", host}, {":path", path}, {"x-forwarded-proto", ssl ? "https" : "http"}}; + {":authority", host}, {":path", path}, {":scheme", ssl ? "https" : "http"}}; if (internal.has_value()) { headers.addCopy("x-envoy-internal", internal.value() ? "true" : "false"); } diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 6ed9e8f1985f9..2e268c585a06f 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -121,7 +121,7 @@ stat_prefix: foo RouteConstSharedPtr route(Http::TestRequestHeaderMapImpl headers) { NiceMock stream_info; - headers.addCopy("x-forwarded-proto", "http"); + headers.addCopy(":scheme", "http"); return rds_->config()->route(headers, stream_info, 0); } diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index 01127c1a07971..a6d4780e35729 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -72,7 +72,7 @@ TEST(BadRateLimitConfiguration, ActionsMissingRequiredFields) { static Http::TestRequestHeaderMapImpl genHeaders(const std::string& host, const std::string& path, const std::string& method) { return Http::TestRequestHeaderMapImpl{ - {":authority", host}, {":path", path}, {":method", method}, {"x-forwarded-proto", "http"}}; + {":authority", host}, {":path", path}, {":method", method}, {":scheme", "http"}}; } class RateLimitConfiguration : public testing::Test { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 1f4bc9d6b64b8..fea3c9bca0f84 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4467,7 +4467,6 @@ TEST_F(RouterTest, InternalRedirectRejectedByPredicate) { TEST_F(RouterTest, HttpInternalRedirectSucceeded) { enableRedirects(3); setNumPreviousRedirect(2); - default_request_headers_.setForwardedProto("http"); sendRequest(); EXPECT_CALL(callbacks_, clearRouteCache()); @@ -4489,6 +4488,7 @@ TEST_F(RouterTest, HttpsInternalRedirectSucceeded) { auto ssl_connection = std::make_shared(); enableRedirects(3); setNumPreviousRedirect(1); + default_request_headers_.setScheme("https"); sendRequest(); @@ -4508,6 +4508,7 @@ TEST_F(RouterTest, HttpsInternalRedirectSucceeded) { TEST_F(RouterTest, CrossSchemeRedirectAllowedByPolicy) { auto ssl_connection = std::make_shared(); enableRedirects(); + default_request_headers_.setScheme("https"); sendRequest(); @@ -5560,7 +5561,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { // With invalid x-forwarded-proto, still use scheme. { Http::TestRequestHeaderMapImpl headers; - headers.setForwardedProto("foo"); + headers.setXForwardedProto("foo"); FilterUtility::setUpstreamScheme(headers, true, true); EXPECT_EQ("https", headers.get_(":scheme")); } @@ -5568,7 +5569,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { // Use valid x-forwarded-proto. { Http::TestRequestHeaderMapImpl headers; - headers.setForwardedProto(Http::Headers::get().SchemeValues.Http); + headers.setXForwardedProto(Http::Headers::get().SchemeValues.Http); FilterUtility::setUpstreamScheme(headers, true, true); EXPECT_EQ("http", headers.get_(":scheme")); } @@ -5577,7 +5578,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { { Http::TestRequestHeaderMapImpl headers; headers.setScheme(Http::Headers::get().SchemeValues.Https); - headers.setForwardedProto(Http::Headers::get().SchemeValues.Http); + headers.setXForwardedProto(Http::Headers::get().SchemeValues.Http); FilterUtility::setUpstreamScheme(headers, false, false); EXPECT_EQ("https", headers.get_(":scheme")); } diff --git a/test/common/router/router_test_base.cc b/test/common/router/router_test_base.cc index 6a8a3d0c2446a..11a23de807f1b 100644 --- a/test/common/router/router_test_base.cc +++ b/test/common/router/router_test_base.cc @@ -219,7 +219,7 @@ void RouterTestBase::sendRequest(bool end_stream) { upstream_stream_info_, Http::Protocol::Http10); return nullptr; })); - HttpTestUtility::addDefaultHeaders(default_request_headers_); + HttpTestUtility::addDefaultHeaders(default_request_headers_, false); router_.decodeHeaders(default_request_headers_, end_stream); } diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index e3289d042f3f7..158e39f9073e6 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -131,7 +131,7 @@ TEST_F(HttpConnManFinalizerImplTest, OriginalAndLongPath) { {"x-envoy-original-path", path}, {":method", "GET"}, {":path", ""}, - {"x-forwarded-proto", "http"}}; + {":scheme", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; @@ -161,10 +161,8 @@ TEST_F(HttpConnManFinalizerImplTest, NoGeneratedId) { const auto remote_address = Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv4Instance(expected_ip, 0, nullptr)}; - Http::TestRequestHeaderMapImpl request_headers{{":path", ""}, - {"x-envoy-original-path", path}, - {":method", "GET"}, - {"x-forwarded-proto", "http"}}; + Http::TestRequestHeaderMapImpl request_headers{ + {":path", ""}, {"x-envoy-original-path", path}, {":method", "GET"}, {":scheme", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; @@ -194,8 +192,7 @@ TEST_F(HttpConnManFinalizerImplTest, Connect) { const auto remote_address = Network::Address::InstanceConstSharedPtr{ new Network::Address::Ipv4Instance(expected_ip, 0, nullptr)}; - Http::TestRequestHeaderMapImpl request_headers{{":method", "CONNECT"}, - {"x-forwarded-proto", "http"}}; + Http::TestRequestHeaderMapImpl request_headers{{":method", "CONNECT"}, {":scheme", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; @@ -318,10 +315,8 @@ TEST_F(HttpConnManFinalizerImplTest, UpstreamClusterTagSet) { } TEST_F(HttpConnManFinalizerImplTest, SpanOptionalHeaders) { - Http::TestRequestHeaderMapImpl request_headers{{"x-request-id", "id"}, - {":path", "/test"}, - {":method", "GET"}, - {"x-forwarded-proto", "https"}}; + Http::TestRequestHeaderMapImpl request_headers{ + {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}, {":scheme", "https"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; const std::string expected_ip = "10.0.0.100"; @@ -362,10 +357,8 @@ TEST_F(HttpConnManFinalizerImplTest, SpanOptionalHeaders) { } TEST_F(HttpConnManFinalizerImplTest, UnixDomainSocketPeerAddressTag) { - Http::TestRequestHeaderMapImpl request_headers{{"x-request-id", "id"}, - {":path", "/test"}, - {":method", "GET"}, - {"x-forwarded-proto", "https"}}; + Http::TestRequestHeaderMapImpl request_headers{ + {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}, {":scheme", "https"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; const std::string path_{TestEnvironment::unixDomainSocketPath("foo")}; @@ -388,7 +381,7 @@ TEST_F(HttpConnManFinalizerImplTest, SpanCustomTags) { Http::TestRequestHeaderMapImpl request_headers{{"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}, - {"x-forwarded-proto", "https"}, + {":scheme", "https"}, {"x-bb", "b"}}; ProtobufWkt::Struct fake_struct; @@ -502,10 +495,8 @@ tag: dd-10, } TEST_F(HttpConnManFinalizerImplTest, SpanPopulatedFailureResponse) { - Http::TestRequestHeaderMapImpl request_headers{{"x-request-id", "id"}, - {":path", "/test"}, - {":method", "GET"}, - {"x-forwarded-proto", "http"}}; + Http::TestRequestHeaderMapImpl request_headers{ + {"x-request-id", "id"}, {":path", "/test"}, {":method", "GET"}, {":scheme", "http"}}; Http::TestResponseHeaderMapImpl response_headers; Http::TestResponseTrailerMapImpl response_trailers; const std::string expected_ip = "10.0.0.100"; @@ -567,7 +558,6 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcOkStatus) { {":path", "/pb.Foo/Bar"}, {":authority", "example.com:80"}, {"content-type", "application/grpc"}, - {"x-forwarded-proto", "http"}, {"te", "trailers"}}; Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, @@ -618,7 +608,7 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcErrorTag) { {":authority", "example.com:80"}, {"content-type", "application/grpc"}, {"grpc-timeout", "10s"}, - {"x-forwarded-proto", "http"}, + {":scheme", "http"}, {"te", "trailers"}}; Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, @@ -662,7 +652,7 @@ TEST_F(HttpConnManFinalizerImplTest, GrpcTrailersOnly) { {":path", "/pb.Foo/Bar"}, {":authority", "example.com:80"}, {"content-type", "application/grpc"}, - {"x-forwarded-proto", "http"}, + {":scheme", "http"}, {"te", "trailers"}}; Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}, diff --git a/test/extensions/filters/http/cache/cache_custom_headers_test.cc b/test/extensions/filters/http/cache/cache_custom_headers_test.cc index f7d54b16cb43a..593da5921f765 100644 --- a/test/extensions/filters/http/cache/cache_custom_headers_test.cc +++ b/test/extensions/filters/http/cache/cache_custom_headers_test.cc @@ -19,7 +19,7 @@ TEST(CacheCustomHeadersTest, EnsureCacheCustomHeadersGettersDoNotFail) { Http::TestRequestHeaderMapImpl request_headers_{ {":path", "/"}, {":method", "GET"}, - {"x-forwarded-proto", "https"}, + {":scheme", "https"}, {":authority", "example.com"}, {"authorization", "Basic abc123def456"}, {"pragma", "no-cache"}, diff --git a/test/extensions/filters/http/cache/cache_filter_test.cc b/test/extensions/filters/http/cache/cache_filter_test.cc index ff8357b0d4f00..3fecae678b985 100644 --- a/test/extensions/filters/http/cache/cache_filter_test.cc +++ b/test/extensions/filters/http/cache/cache_filter_test.cc @@ -123,7 +123,7 @@ class CacheFilterTest : public ::testing::Test { Event::SimulatedTimeSystem time_source_; DateFormatter formatter_{"%a, %d %b %Y %H:%M:%S GMT"}; Http::TestRequestHeaderMapImpl request_headers_{ - {":path", "/"}, {":method", "GET"}, {"x-forwarded-proto", "https"}}; + {":path", "/"}, {":method", "GET"}, {":scheme", "https"}}; Http::TestResponseHeaderMapImpl response_headers_{{":status", "200"}, {"cache-control", "public,max-age=3600"}}; NiceMock decoder_callbacks_; diff --git a/test/extensions/filters/http/cache/cacheability_utils_test.cc b/test/extensions/filters/http/cache/cacheability_utils_test.cc index 79b4b3f836a95..7c208861712dd 100644 --- a/test/extensions/filters/http/cache/cacheability_utils_test.cc +++ b/test/extensions/filters/http/cache/cacheability_utils_test.cc @@ -14,18 +14,14 @@ namespace { class CanServeRequestFromCacheTest : public testing::Test { protected: - Http::TestRequestHeaderMapImpl request_headers_ = {{":path", "/"}, - {":method", "GET"}, - {"x-forwarded-proto", "http"}, - {":authority", "test.com"}}; + Http::TestRequestHeaderMapImpl request_headers_ = { + {":path", "/"}, {":method", "GET"}, {":scheme", "http"}, {":authority", "test.com"}}; }; class RequestConditionalHeadersTest : public testing::TestWithParam { protected: - Http::TestRequestHeaderMapImpl request_headers_ = {{":path", "/"}, - {":method", "GET"}, - {"x-forwarded-proto", "http"}, - {":authority", "test.com"}}; + Http::TestRequestHeaderMapImpl request_headers_ = { + {":path", "/"}, {":method", "GET"}, {":scheme", "http"}, {":authority", "test.com"}}; std::string conditionalHeader() const { return GetParam(); } }; @@ -78,11 +74,11 @@ TEST_F(CanServeRequestFromCacheTest, MethodHeader) { EXPECT_FALSE(CacheabilityUtils::canServeRequestFromCache(request_headers_)); } -TEST_F(CanServeRequestFromCacheTest, ForwardedProtoHeader) { +TEST_F(CanServeRequestFromCacheTest, SchemeHeader) { EXPECT_TRUE(CacheabilityUtils::canServeRequestFromCache(request_headers_)); - request_headers_.setForwardedProto("ftp"); + request_headers_.setScheme("ftp"); EXPECT_FALSE(CacheabilityUtils::canServeRequestFromCache(request_headers_)); - request_headers_.removeForwardedProto(); + request_headers_.removeScheme(); EXPECT_FALSE(CacheabilityUtils::canServeRequestFromCache(request_headers_)); } diff --git a/test/extensions/filters/http/cache/http_cache_test.cc b/test/extensions/filters/http/cache/http_cache_test.cc index 9ccc413033f9e..3ffc8089b47ec 100644 --- a/test/extensions/filters/http/cache/http_cache_test.cc +++ b/test/extensions/filters/http/cache/http_cache_test.cc @@ -43,10 +43,8 @@ class LookupRequestTest : public testing::TestWithParam { LookupRequestTest() : vary_allow_list_(getConfig().allowed_vary_headers()) {} DateFormatter formatter_{"%a, %d %b %Y %H:%M:%S GMT"}; - Http::TestRequestHeaderMapImpl request_headers_{{":path", "/"}, - {":method", "GET"}, - {"x-forwarded-proto", "https"}, - {":authority", "example.com"}}; + Http::TestRequestHeaderMapImpl request_headers_{ + {":path", "/"}, {":method", "GET"}, {":scheme", "https"}, {":authority", "example.com"}}; VaryHeader vary_allow_list_; diff --git a/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc b/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc index 0e895d0c6efae..eaad85d95d162 100644 --- a/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc +++ b/test/extensions/filters/http/cache/simple_http_cache/simple_http_cache_test.cc @@ -32,7 +32,7 @@ class SimpleHttpCacheTest : public testing::Test { SimpleHttpCacheTest() : vary_allow_list_(getConfig().allowed_vary_headers()) { request_headers_.setMethod("GET"); request_headers_.setHost("example.com"); - request_headers_.setForwardedProto("https"); + request_headers_.setScheme("https"); request_headers_.setCopy(Http::CustomHeaders::get().CacheControl, "max-age=3600"); } diff --git a/test/extensions/filters/http/compressor/compressor_integration_tests.cc b/test/extensions/filters/http/compressor/compressor_integration_tests.cc index e15001246962c..e3723df2fa604 100644 --- a/test/extensions/filters/http/compressor/compressor_integration_tests.cc +++ b/test/extensions/filters/http/compressor/compressor_integration_tests.cc @@ -71,9 +71,9 @@ void WebsocketWithCompressorIntegrationTest::validateUpgradeRequestHeaders( const Http::RequestHeaderMap& original_proxied_request_headers, const Http::RequestHeaderMap& original_request_headers) { Http::TestRequestHeaderMapImpl proxied_request_headers(original_proxied_request_headers); - if (proxied_request_headers.ForwardedProto()) { - ASSERT_EQ(proxied_request_headers.getForwardedProtoValue(), "http"); - proxied_request_headers.removeForwardedProto(); + if (proxied_request_headers.XForwardedProto()) { + ASSERT_EQ(proxied_request_headers.getXForwardedProtoValue(), "http"); + proxied_request_headers.removeXForwardedProto(); } // Check for and remove headers added by default for HTTP requests. diff --git a/test/extensions/filters/http/ext_proc/filter_test.cc b/test/extensions/filters/http/ext_proc/filter_test.cc index 73640c79af78b..cde78e9e16be0 100644 --- a/test/extensions/filters/http/ext_proc/filter_test.cc +++ b/test/extensions/filters/http/ext_proc/filter_test.cc @@ -64,6 +64,8 @@ class HttpFilterTest : public testing::Test { filter_ = std::make_unique(config_, std::move(client_)); filter_->setEncoderFilterCallbacks(encoder_callbacks_); filter_->setDecoderFilterCallbacks(decoder_callbacks_); + HttpTestUtility::addDefaultHeaders(request_headers_); + request_headers_.setMethod("POST"); } ExternalProcessorStreamPtr doStart(ExternalProcessorCallbacks& callbacks) { @@ -211,7 +213,6 @@ TEST_F(HttpFilterTest, SimplestPost) { EXPECT_TRUE(config_->failureModeAllow()); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 10); request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes"); @@ -272,7 +273,6 @@ TEST_F(HttpFilterTest, PostAndChangeHeaders) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("x-some-other-header"), "yes"); request_headers_.addCopy(LowerCaseString("x-do-we-want-this"), "no"); @@ -356,8 +356,6 @@ TEST_F(HttpFilterTest, PostAndRespondImmediately) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); Http::TestResponseHeaderMapImpl immediate_response_headers; @@ -419,8 +417,6 @@ TEST_F(HttpFilterTest, PostAndRespondImmediatelyOnResponse) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); processRequestHeaders(false, absl::nullopt); @@ -481,7 +477,6 @@ TEST_F(HttpFilterTest, PostAndChangeRequestBodyBuffered) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -546,7 +541,6 @@ TEST_F(HttpFilterTest, PostAndChangeRequestBodyBufferedComesFast) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -614,7 +608,6 @@ TEST_F(HttpFilterTest, PostAndChangeRequestBodyBufferedComesALittleFast) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -682,7 +675,6 @@ TEST_F(HttpFilterTest, PostAndChangeBothBodiesBufferedOneChunk) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -756,7 +748,6 @@ TEST_F(HttpFilterTest, PostAndChangeBothBodiesBufferedMultiChunk) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -843,7 +834,6 @@ TEST_F(HttpFilterTest, PostAndIgnoreStreamedBodiesUntilImplemented) { )EOF"); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); request_headers_.addCopy(LowerCaseString("content-type"), "text/plain"); request_headers_.addCopy(LowerCaseString("content-length"), 100); @@ -885,8 +875,6 @@ TEST_F(HttpFilterTest, RespondImmediatelyDefault) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); Http::TestResponseHeaderMapImpl immediate_response_headers; @@ -923,8 +911,6 @@ TEST_F(HttpFilterTest, RespondImmediatelyGrpcError) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); Http::TestResponseHeaderMapImpl immediate_response_headers; @@ -965,7 +951,6 @@ TEST_F(HttpFilterTest, PostAndFail) { EXPECT_FALSE(config_->failureModeAllow()); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); // Oh no! The remote server had a failure! @@ -1010,7 +995,6 @@ TEST_F(HttpFilterTest, PostAndFailOnResponse) { EXPECT_FALSE(config_->failureModeAllow()); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); EXPECT_FALSE(last_request_.async_mode()); @@ -1069,7 +1053,6 @@ TEST_F(HttpFilterTest, PostAndIgnoreFailure) { EXPECT_TRUE(config_->failureModeAllow()); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); // Oh no! The remote server had a failure which we will ignore @@ -1107,7 +1090,6 @@ TEST_F(HttpFilterTest, PostAndClose) { EXPECT_FALSE(config_->failureModeAllow()); // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); EXPECT_FALSE(last_request_.async_mode()); @@ -1151,7 +1133,6 @@ TEST_F(HttpFilterTest, ProcessingModeRequestHeadersOnly) { response_trailer_mode: "SKIP" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); EXPECT_FALSE(last_request_.async_mode()); @@ -1198,7 +1179,6 @@ TEST_F(HttpFilterTest, ProcessingModeOverrideResponseHeaders) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, false)); processRequestHeaders( @@ -1248,7 +1228,6 @@ TEST_F(HttpFilterTest, ProcessingModeResponseHeadersOnly) { response_trailer_mode: "SKIP" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_, "POST"); EXPECT_EQ(FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers_, false)); Buffer::OwnedImpl first_chunk("foo"); @@ -1290,9 +1269,6 @@ TEST_F(HttpFilterTest, ClearRouteCache) { response_body_mode: "BUFFERED" )EOF"); - // Create synthetic HTTP request - HttpTestUtility::addDefaultHeaders(request_headers_, "GET"); - EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); EXPECT_CALL(decoder_callbacks_, clearRouteCache()); @@ -1330,7 +1306,7 @@ TEST_F(HttpFilterTest, ReplaceRequest) { cluster_name: "ext_proc_server" )EOF"); - HttpTestUtility::addDefaultHeaders(request_headers_); + request_headers_.setMethod("GET"); EXPECT_EQ(FilterHeadersStatus::StopIteration, filter_->decodeHeaders(request_headers_, true)); Buffer::OwnedImpl req_buffer; diff --git a/test/extensions/filters/http/ext_proc/streaming_integration_test.cc b/test/extensions/filters/http/ext_proc/streaming_integration_test.cc index 4fd5d7a581246..59e03fe215773 100644 --- a/test/extensions/filters/http/ext_proc/streaming_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/streaming_integration_test.cc @@ -85,8 +85,8 @@ class StreamingIntegrationTest : public HttpIntegrationTest, sendClientRequestHeaders(absl::optional> cb) { auto conn = makeClientConnection(lookupPort("http")); codec_client_ = makeHttpConnection(std::move(conn)); - Http::TestRequestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers, std::string("POST")); + Http::TestRequestHeaderMapImpl headers{{":method", "POST"}}; + HttpTestUtility::addDefaultHeaders(headers, false); if (cb) { (*cb)(headers); } diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index 9179db9d6686f..b4232ae2bccba 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -101,7 +101,7 @@ class OAuth2Test : public testing::Test { endpoint->set_cluster("auth.example.com"); endpoint->set_uri("auth.example.com/_oauth"); endpoint->mutable_timeout()->set_seconds(1); - p.set_redirect_uri("%REQ(x-forwarded-proto)%://%REQ(:authority)%" + TEST_CALLBACK); + p.set_redirect_uri("%REQ(:scheme)%://%REQ(:authority)%" + TEST_CALLBACK); p.mutable_redirect_path_matcher()->mutable_path()->set_exact(TEST_CALLBACK); p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); @@ -250,7 +250,7 @@ TEST_F(OAuth2Test, DefaultAuthScope) { endpoint->set_cluster("auth.example.com"); endpoint->set_uri("auth.example.com/_oauth"); endpoint->mutable_timeout()->set_seconds(1); - p.set_redirect_uri("%REQ(x-forwarded-proto)%://%REQ(:authority)%" + TEST_CALLBACK); + p.set_redirect_uri("%REQ(:scheme)%://%REQ(:authority)%" + TEST_CALLBACK); p.mutable_redirect_path_matcher()->mutable_path()->set_exact(TEST_CALLBACK); p.set_authorization_endpoint("https://auth.example.com/oauth/authorize/"); p.mutable_signout_path()->mutable_path()->set_exact("/_signout"); @@ -286,7 +286,6 @@ TEST_F(OAuth2Test, DefaultAuthScope) { {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, {Http::Headers::get().Scheme.get(), "http"}, - {Http::Headers::get().ForwardedProto.get(), "http"}, }; Http::TestResponseHeaderMapImpl response_headers{ @@ -320,7 +319,7 @@ TEST_F(OAuth2Test, RequestSignout) { {Http::Headers::get().Path.get(), "/_signout"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, }; Http::TestResponseHeaderMapImpl response_headers{ @@ -350,7 +349,7 @@ TEST_F(OAuth2Test, OAuthOkPass) { {Http::Headers::get().Path.get(), "/anypath"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::CustomHeaders::get().Authorization.get(), "Bearer injected_malice!"}, }; @@ -358,7 +357,7 @@ TEST_F(OAuth2Test, OAuthOkPass) { {Http::Headers::get().Path.get(), "/anypath"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::CustomHeaders::get().Authorization.get(), "Bearer legit_token"}, }; @@ -394,7 +393,6 @@ TEST_F(OAuth2Test, OAuthErrorNonOAuthHttpCallback) { {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, {Http::Headers::get().Scheme.get(), "http"}, - {Http::Headers::get().ForwardedProto.get(), "http"}, }; Http::TestResponseHeaderMapImpl response_headers{ @@ -459,7 +457,7 @@ TEST_F(OAuth2Test, OAuthCallbackStartsAuthentication) { Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/_oauth?code=123&state=https://asdf&method=GET"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, }; @@ -704,7 +702,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { {Http::Headers::get().Path.get(), "/test?name=admin&level=trace"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Post}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, }; // This is the immediate response - a redirect to the auth cluster. @@ -739,7 +737,7 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { "2Ftest%3Fname%3Dadmin%26level%3Dtrace"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, }; // Deliberately fail the HMAC validation check. @@ -783,7 +781,7 @@ TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { {Http::Headers::get().Path.get(), "/test?role=bearer"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}, }; // Expected decoded headers after the callback & validation of the bearer token is complete. @@ -791,7 +789,7 @@ TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { {Http::Headers::get().Path.get(), "/test?role=bearer"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-header-token"}, }; @@ -811,13 +809,13 @@ TEST_F(OAuth2Test, OAuthBearerTokenFlowFromQueryParameters) { {Http::Headers::get().Path.get(), "/test?role=bearer&token=xyz-queryparam-token"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, }; Http::TestRequestHeaderMapImpl request_headers_after{ {Http::Headers::get().Path.get(), "/test?role=bearer&token=xyz-queryparam-token"}, {Http::Headers::get().Host.get(), "traffic.example.com"}, {Http::Headers::get().Method.get(), Http::Headers::get().MethodValues.Get}, - {Http::Headers::get().ForwardedProto.get(), "https"}, + {Http::Headers::get().Scheme.get(), "https"}, {Http::CustomHeaders::get().Authorization.get(), "Bearer xyz-queryparam-token"}, }; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 10c57fa1fac6d..eea87b3854d6c 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -1076,6 +1076,7 @@ TEST_P(IntegrationTest, AbsolutePathUsingHttpsDisallowedAtFrontline) { } TEST_P(IntegrationTest, AbsolutePathUsingHttpsAllowedInternally) { + autonomous_upstream_ = true; // Sent an HTTPS request over non-TLS. It will be allowed for non-front-line Envoys // and match the configured redirect. auto host = config_helper_.createVirtualHost("www.redirect.com", "/"); diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index 694aebb9d41c7..f1dafa9f271cd 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -72,14 +72,14 @@ TEST_P(Http2UpstreamIntegrationTest, TestSchemeAndXFP) { auto check_preserved = ([&](absl::string_view scheme, absl::string_view xff) { { default_request_headers_.setScheme(scheme); - default_request_headers_.setForwardedProto(xff); + default_request_headers_.setXForwardedProto(xff); auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); ASSERT_TRUE(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); + EXPECT_EQ(headers->getXForwardedProtoValue(), xff); } }); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index c7159200ab9e0..8f2576fe79036 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -59,9 +59,9 @@ void WebsocketIntegrationTest::validateUpgradeRequestHeaders( const Http::RequestHeaderMap& original_proxied_request_headers, const Http::RequestHeaderMap& original_request_headers) { Http::TestRequestHeaderMapImpl proxied_request_headers(original_proxied_request_headers); - if (proxied_request_headers.ForwardedProto()) { - ASSERT_EQ(proxied_request_headers.getForwardedProtoValue(), "http"); - proxied_request_headers.removeForwardedProto(); + if (proxied_request_headers.XForwardedProto()) { + ASSERT_EQ(proxied_request_headers.getXForwardedProtoValue(), "http"); + proxied_request_headers.removeXForwardedProto(); } // Check for and remove headers added by default for HTTP requests. diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index 7da0acffffa7b..d657457d36874 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -70,7 +70,7 @@ ToolConfig ToolConfig::create(const envoy::RouterCheckToolSchema::ValidationItem request_headers->addCopy(":authority", check_config.input().authority()); request_headers->addCopy(":path", check_config.input().path()); request_headers->addCopy(":method", check_config.input().method()); - request_headers->addCopy("x-forwarded-proto", check_config.input().ssl() ? "https" : "http"); + request_headers->addCopy(":scheme", check_config.input().ssl() ? "https" : "http"); if (check_config.input().internal()) { request_headers->addCopy("x-envoy-internal", "true"); From 9427dfcc244c1020fd592e9d43b4233c0ae5c4bd Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 15 Jul 2021 17:26:21 -0400 Subject: [PATCH 2/4] spelling Signed-off-by: Alyssa Wilk --- source/common/router/config_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index de46456f33c6d..e418cd5898a38 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -755,8 +755,8 @@ absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHead if (host_end != absl::string_view::npos) { absl::string_view request_port = request_host.substr(host_end); - // In the rare case that XFP and scheme disagree (say http URL over an HTTPs - // connection), do port stripping based on XFP so http://foo.com:80 won't + // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPs + // connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't // have the port stripped when served over TLS. absl::string_view request_protocol = getXFPOrScheme(headers); bool remove_port = !new_port.empty(); @@ -1376,7 +1376,7 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { - // In the rare case that XFP and scheme disagree (say http URL over an HTTPs + // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPs // connection), force a redirect based on underlying protocol, rather than URL // scheme, so don't force a redirect for a http:// url served over a TLS // connection. From 8f69b06e12cb00d05b4bb0d376da729640d0be7f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Jul 2021 12:27:23 -0400 Subject: [PATCH 3/4] comments Signed-off-by: Alyssa Wilk --- .../http/http_conn_man/headers.rst | 4 ++++ docs/root/faq/debugging/xfp_vs_scheme.rst | 14 +++++++++++++ docs/root/faq/overview.rst | 1 + docs/root/version_history/current.rst | 3 +++ envoy/http/header_map.h | 4 ++-- source/common/http/conn_manager_utility.cc | 14 ++++++------- source/common/http/headers.h | 2 +- source/common/http/utility.cc | 7 +++---- source/common/http/utility.h | 2 +- source/common/router/config_impl.cc | 13 +++--------- source/common/router/router.cc | 5 ++++- source/common/runtime/runtime_features.cc | 2 +- source/extensions/common/aws/utility.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 6 +++--- test/common/http/conn_manager_impl_test_2.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 12 +++++------ test/common/http/utility_test.cc | 2 +- test/common/router/config_impl_test.cc | 21 +++++++++++-------- test/common/router/rds_impl_test.cc | 2 +- test/common/router/router_ratelimit_test.cc | 7 +++++-- test/common/router/router_test.cc | 6 +++--- .../compressor_integration_tests.cc | 6 +++--- .../multiplexed_upstream_integration_test.cc | 4 ++-- .../integration/websocket_integration_test.cc | 6 +++--- test/tools/router_check/router.cc | 1 + 25 files changed, 86 insertions(+), 62 deletions(-) create mode 100644 docs/root/faq/debugging/xfp_vs_scheme.rst diff --git a/docs/root/configuration/http/http_conn_man/headers.rst b/docs/root/configuration/http/http_conn_man/headers.rst index 9fd0e2fa0c0b9..5ea4e7946dc84 100644 --- a/docs/root/configuration/http/http_conn_man/headers.rst +++ b/docs/root/configuration/http/http_conn_man/headers.rst @@ -25,6 +25,8 @@ This default behavior can be overridden via the :ref:`scheme_header_transformati ` configuration option. +The *:scheme* header will be used by Envoy over *x-forwarded-proto* where the URI scheme is wanted, for example serving content from cache based on the *:scheme* header rather than X-Forwarded-Proto, or setting the scheme of redirects based on the scheme of the original URI. See :ref:`why_is_envoy_using_xfp_or_scheme` for more details. + .. [1] Edge Envoys often have plaintext HTTP/1.1 listeners. If Envoy trusts absolute URL scheme from fully qualfied URLs, a MiTM can adjust relative URLs to https absolute URLs, and inadvertantly cause the Envoy's upstream to send PII or other sensitive data over what it then believes is a secure connection. .. [2] Unlike HTTP/1.1, HTTP/2 is in practice always served over TLS via ALPN for edge Envoys. In mesh networks using insecure HTTP/2, if the downstream is not trusted to set scheme, the :ref:`scheme_header_transformation ` should be used. @@ -368,6 +370,8 @@ If the scheme is changed via the :ref:`scheme_header_transformation ` configuration option, *x-forwarded-proto* will be updated as well. +The *x-forwarded-proto* header will be used by Envoy over *:scheme* where the underlying encryption is wanted, for example clearing default ports based on *x-forwarded-proto*. See :ref:`why_is_envoy_using_xfp_or_scheme` for more details. + .. _config_http_conn_man_headers_x-request-id: x-request-id diff --git a/docs/root/faq/debugging/xfp_vs_scheme.rst b/docs/root/faq/debugging/xfp_vs_scheme.rst new file mode 100644 index 0000000000000..6bdb4550dc996 --- /dev/null +++ b/docs/root/faq/debugging/xfp_vs_scheme.rst @@ -0,0 +1,14 @@ +.. _why_is_envoy_using_xfp_or_scheme: + +Why is Envoy operating on X-Forwarded-Proto instead of :scheme or vice-versa? +============================================================================= + +With almost all requests, the value of the X-Forwarded-Proto header and the :scheme +header (if present) will be the same. Generally users request https:// resources over +TLS connections and http:// resources in the clear. However, it is entirely possible +for a user to request http:// content over a TLS connection or in internal meshes to forward +https:// requests in cleartext. In these cases Envoy will attempt to use the :scheme +header when refering to content (say serving a given entity out of cache based on the URL +scheme) and the X-Forwarded-Proto header when doing operations related to underlying +encryption (stripping the default port based on if the request was TLS on port 443, or +cleartext on port 80) diff --git a/docs/root/faq/overview.rst b/docs/root/faq/overview.rst index 33d8f906f0eaf..4da3fd0dde41d 100644 --- a/docs/root/faq/overview.rst +++ b/docs/root/faq/overview.rst @@ -36,6 +36,7 @@ Debugging debugging/why_is_envoy_404ing_connect_requests debugging/why_is_envoy_sending_413s debugging/why_is_my_route_not_found + debugging/xfp_vs_scheme Performance ----------- diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 20d5236f703d8..c2e67c21f9a11 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,6 +13,9 @@ Minor Behavior Changes which defines the minimal number of headers in a request/response/trailers required for using a dictionary in addition to the list. Setting the `envoy.http.headermap.lazy_map_min_size` runtime feature to a non-negative number will override the default value. +* http: correct the use of the ``x-forwarded-proto`` header and the ``:scheme`` header. Where they differ + (which is rare) ``:scheme`` will now be used for serving redirect URIs and cached content. This behavior + can be reverted by setting runtime guard ``correct_scheme_and_xfp`` to false. Bug Fixes --------- diff --git a/envoy/http/header_map.h b/envoy/http/header_map.h index 3d7e2978526cc..a3f1fd855b417 100644 --- a/envoy/http/header_map.h +++ b/envoy/http/header_map.h @@ -304,6 +304,7 @@ class HeaderEntry { HEADER_FUNC(Expect) \ HEADER_FUNC(ForwardedClientCert) \ HEADER_FUNC(ForwardedFor) \ + HEADER_FUNC(ForwardedProto) \ HEADER_FUNC(GrpcTimeout) \ HEADER_FUNC(Host) \ HEADER_FUNC(Method) \ @@ -311,8 +312,7 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) \ - HEADER_FUNC(XForwardedProto) + HEADER_FUNC(UserAgent) #define INLINE_REQ_NUMERIC_HEADERS(HEADER_FUNC) \ HEADER_FUNC(EnvoyExpectedRequestTimeoutMs) \ diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 246c65bfc6c53..ba1abb7f7461c 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -131,8 +131,8 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m } // 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.XForwardedProto() == nullptr) { - request_headers.setReferenceXForwardedProto( + if (xff_num_trusted_hops == 0 || request_headers.ForwardedProto() == nullptr) { + request_headers.setReferenceForwardedProto( connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); } } else { @@ -161,14 +161,14 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m // 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.XForwardedProto()) { - request_headers.setReferenceXForwardedProto( - connection.ssl() ? Headers::get().SchemeValues.Https : Headers::get().SchemeValues.Http); + if (!request_headers.ForwardedProto()) { + request_headers.setReferenceForwardedProto(connection.ssl() ? Headers::get().SchemeValues.Https + : Headers::get().SchemeValues.Http); } if (config.schemeToSet().has_value()) { request_headers.setScheme(config.schemeToSet().value()); - request_headers.setXForwardedProto(config.schemeToSet().value()); + request_headers.setForwardedProto(config.schemeToSet().value()); } // If :scheme is not set, sets :scheme based on X-Forwarded-Proto if a valid scheme, @@ -177,7 +177,7 @@ ConnectionManagerUtility::MutateRequestHeadersResult ConnectionManagerUtility::m if (!request_headers.Scheme() && Runtime::runtimeFeatureEnabled("envoy.reloadable_features.add_and_validate_scheme_header")) { request_headers.setScheme( - getScheme(request_headers.getXForwardedProtoValue(), connection.ssl() != nullptr)); + getScheme(request_headers.getForwardedProtoValue(), connection.ssl() != nullptr)); } // At this point we can determine whether this is an internal or external request. The diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 9f479ebdb7041..50edd1fa92546 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -188,6 +188,7 @@ class HeaderValues { const LowerCaseString ForwardedClientCert{"x-forwarded-client-cert"}; const LowerCaseString ForwardedFor{"x-forwarded-for"}; const LowerCaseString ForwardedHost{"x-forwarded-host"}; + const LowerCaseString ForwardedProto{"x-forwarded-proto"}; const LowerCaseString GrpcMessage{"grpc-message"}; const LowerCaseString GrpcStatus{"grpc-status"}; const LowerCaseString GrpcTimeout{"grpc-timeout"}; @@ -214,7 +215,6 @@ class HeaderValues { const LowerCaseString Via{"via"}; const LowerCaseString WWWAuthenticate{"www-authenticate"}; const LowerCaseString XContentTypeOptions{"x-content-type-options"}; - const LowerCaseString XForwardedProto{"x-forwarded-proto"}; const LowerCaseString XSquashDebug{"x-squash-debug"}; struct { diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index b64573f549ac7..cc472f8e1cecd 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -680,7 +680,7 @@ bool Utility::sanitizeConnectionHeader(Http::RequestHeaderMap& headers) { } else if ((lcs_header_to_remove == Http::Headers::get().ForwardedFor) || (lcs_header_to_remove == Http::Headers::get().ForwardedHost) || - (lcs_header_to_remove == Http::Headers::get().XForwardedProto) || + (lcs_header_to_remove == Http::Headers::get().ForwardedProto) || !token_sv.find(':')) { // An attacker could nominate an X-Forwarded* header, and its removal may mask @@ -769,11 +769,10 @@ const std::string& Utility::getProtocolString(const Protocol protocol) { } absl::string_view Utility::getScheme(const RequestHeaderMap& headers) { - if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_scheme_header") && - !headers.getSchemeValue().empty()) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correct_scheme_and_xfp")) { return headers.getSchemeValue(); } - return headers.getXForwardedProtoValue(); + return headers.getForwardedProtoValue(); } std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_headers, diff --git a/source/common/http/utility.h b/source/common/http/utility.h index a0f422b01fb71..20d5ae98b223e 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -397,7 +397,7 @@ const std::string& getProtocolString(const Protocol p); /** * Return the scheme of the request. - * For legacy code (envoy.reloadable_features.use_scheme_header == false) this + * For legacy code (envoy.reloadable_features.correct_scheme_and_xfp == false) this * will be the value of the X-Forwarded-Proto header value. By default it will * return the scheme if present, otherwise the value of X-Forwarded-Proto if * present. diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index e418cd5898a38..33474aea19dfc 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -45,11 +45,6 @@ namespace Envoy { namespace Router { namespace { -absl::string_view getXFPOrScheme(const Http::RequestHeaderMap& headers) { - absl::string_view xfp = headers.getXForwardedProtoValue(); - return xfp.empty() ? headers.getSchemeValue() : xfp; -} - const std::string DEPRECATED_ROUTER_NAME = "envoy.router"; constexpr uint32_t DEFAULT_MAX_DIRECT_RESPONSE_BODY_SIZE_BYTES = 4096; @@ -758,7 +753,7 @@ absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHead // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPs // connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't // have the port stripped when served over TLS. - absl::string_view request_protocol = getXFPOrScheme(headers); + absl::string_view request_protocol = headers.getForwardedProtoValue(); bool remove_port = !new_port.empty(); if (new_scheme != request_protocol) { @@ -1380,12 +1375,10 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb // connection), force a redirect based on underlying protocol, rather than URL // scheme, so don't force a redirect for a http:// url served over a TLS // connection. - // TODO(alyssar, mattklein123) is the goal here to force TLS, or force https: - // scheme? Maybe upstreams will get confused if seeing an http url. - const absl::string_view scheme = getXFPOrScheme(headers); + const absl::string_view scheme = headers.getForwardedProtoValue(); if (scheme.empty()) { // No scheme header. This normally only happens when ActiveStream::decodeHeaders - // bails early (as it rejects a request), so there is no routing is going to happen anyway. + // bails early (as it rejects a request), or a buggy filter removes the :scheme header. return nullptr; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 8e3f137093ac6..0a5bb031136bb 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -83,7 +83,10 @@ void FilterUtility::setUpstreamScheme(Http::RequestHeaderMap& headers, bool down if (Http::HeaderUtility::schemeIsValid(headers.getSchemeValue())) { return; } - absl::string_view xfp = headers.getXForwardedProtoValue(); + // After all the changes in https://github.com/envoyproxy/envoy/issues/14587 + // this path should only occur if a buggy filter has removed the :scheme + // header. In that case best-effort set from X-Forwarded-Proto. + absl::string_view xfp = headers.getForwardedProtoValue(); if (Http::HeaderUtility::schemeIsValid(xfp)) { headers.setScheme(xfp); return; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index b354c05a7a157..b7be319b38cc6 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -60,6 +60,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_response_for_timeout", "envoy.reloadable_features.check_unsupported_typed_per_filter_config", "envoy.reloadable_features.check_ocsp_policy", + "envoy.reloadable_features.correct_scheme_and_xfp", "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.dont_add_content_length_for_bodiless_requests", "envoy.reloadable_features.enable_compression_without_content_length_header", @@ -94,7 +95,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.vhds_heartbeats", "envoy.reloadable_features.wasm_cluster_name_envoy_grpc", "envoy.reloadable_features.upstream_http2_flood_checks", - "envoy.reloadable_features.use_scheme_header", "envoy.restart_features.use_apple_api_for_dns_lookups", "envoy.reloadable_features.header_map_correctly_coalesce_cookies", }; diff --git a/source/extensions/common/aws/utility.cc b/source/extensions/common/aws/utility.cc index f4336362ea214..497e37f409628 100644 --- a/source/extensions/common/aws/utility.cc +++ b/source/extensions/common/aws/utility.cc @@ -26,7 +26,7 @@ Utility::canonicalizeHeaders(const Http::RequestHeaderMap& headers) { // Skip headers that are likely to mutate, when crossing proxies const auto key = entry.key().getStringView(); if (key == Http::Headers::get().ForwardedFor.get() || - key == Http::Headers::get().XForwardedProto.get() || key == "x-amzn-trace-id") { + key == Http::Headers::get().ForwardedProto.get() || key == "x-amzn-trace-id") { return Http::HeaderMap::Iterate::Continue; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 86df5da19f732..69894daf187aa 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -27,7 +27,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponse) { .Times(2) .WillRepeatedly(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getXForwardedProtoValue()); + EXPECT_EQ("http", headers.getForwardedProtoValue()); if (headers.Path()->value() == "/healthcheck") { filter->callbacks_->streamInfo().healthCheck(true); } @@ -92,7 +92,7 @@ TEST_F(HttpConnectionManagerImplTest, 100ContinueResponse) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillRepeatedly(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getXForwardedProtoValue()); + EXPECT_EQ("http", headers.getForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); @@ -3662,7 +3662,7 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillOnce(Invoke([](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("https", headers.getXForwardedProtoValue()); + EXPECT_EQ("https", headers.getForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc index ffe48e846c168..82e09cee54e91 100644 --- a/test/common/http/conn_manager_impl_test_2.cc +++ b/test/common/http/conn_manager_impl_test_2.cc @@ -2643,7 +2643,7 @@ TEST_F(HttpConnectionManagerImplTest, HeaderOnlyRequestAndResponseUsingHttp3) { EXPECT_CALL(*filter, decodeHeaders(_, true)) .WillOnce(Invoke([&](RequestHeaderMap& headers, bool) -> FilterHeadersStatus { EXPECT_NE(nullptr, headers.ForwardedFor()); - EXPECT_EQ("http", headers.getXForwardedProtoValue()); + EXPECT_EQ("http", headers.getForwardedProtoValue()); return FilterHeadersStatus::StopIteration; })); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index ffbcf490e0319..ec66b39ad50d4 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -277,12 +277,12 @@ TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternal) { TestRequestHeaderMapImpl headers{{"x-forwarded-proto", "https"}}; callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("https", headers.getXForwardedProtoValue()); + EXPECT_EQ("https", headers.getForwardedProtoValue()); // Given :scheme was not set, it will be set to X-Forwarded-Proto EXPECT_EQ("https", headers.getSchemeValue()); // Make sure if x-forwarded-proto changes it doesn't cause problems. - headers.setXForwardedProto("ftp"); + headers.setForwardedProto("ftp"); EXPECT_EQ("https", headers.getSchemeValue()); } @@ -296,7 +296,7 @@ TEST_F(ConnectionManagerUtilityTest, OverwriteForwardedProtoWhenExternal) { ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address)); callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("http", headers.getXForwardedProtoValue()); + EXPECT_EQ("http", headers.getForwardedProtoValue()); // Given :scheme was not set, it will be set to X-Forwarded-Proto EXPECT_EQ("http", headers.getSchemeValue()); } @@ -313,7 +313,7 @@ TEST_F(ConnectionManagerUtilityTest, PreserveForwardedProtoWhenInternalButSetSch TestRequestHeaderMapImpl headers{{"x-forwarded-proto", "foo"}}; callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("foo", headers.getXForwardedProtoValue()); + EXPECT_EQ("foo", headers.getForwardedProtoValue()); // Given :scheme was not set, but X-Forwarded-Proto is not a valid scheme, // scheme will be set based on encryption level. EXPECT_EQ("http", headers.getSchemeValue()); @@ -329,7 +329,7 @@ TEST_F(ConnectionManagerUtilityTest, SchemeIsRespected) { ON_CALL(config_, localAddress()).WillByDefault(ReturnRef(local_address)); callMutateRequestHeaders(headers, Protocol::Http2); - EXPECT_EQ("http", headers.getXForwardedProtoValue()); + EXPECT_EQ("http", headers.getForwardedProtoValue()); // Given :scheme was set, it will not be changed. EXPECT_EQ("https", headers.getSchemeValue()); } @@ -348,7 +348,7 @@ TEST_F(ConnectionManagerUtilityTest, SchemeOverwrite) { config_.scheme_ = "https"; callMutateRequestHeaders(headers, Protocol::Http2); EXPECT_EQ("https", headers.getSchemeValue()); - EXPECT_EQ("https", headers.getXForwardedProtoValue()); + EXPECT_EQ("https", headers.getForwardedProtoValue()); } // Verify internal request and XFF is set when we are using remote address and the address is diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 28accb0622446..62fcd5f17ff0b 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1233,7 +1233,7 @@ TEST(HttpUtility, TestRejectNominatedXForwardedHost) { EXPECT_EQ(sanitized_headers, request_headers); } -TEST(HttpUtility, TestRejectNominatedXForwardedProto) { +TEST(HttpUtility, TestRejectNominatedForwardedProto) { Http::TestRequestHeaderMapImpl request_headers = { {":method", "GET"}, {":path", "/"}, diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index a3164809c904a..0afc3a4f799c8 100644 --- a/test/common/router/config_impl_test.cc +++ b/test/common/router/config_impl_test.cc @@ -114,18 +114,20 @@ class TestConfigImpl : public ConfigImpl { Http::TestRequestHeaderMapImpl genPathlessHeaders(const std::string& host, const std::string& method) { - return Http::TestRequestHeaderMapImpl{{":authority", host}, {":method", method}, - {"x-safe", "safe"}, {"x-global-nope", "global"}, - {"x-vhost-nope", "vhost"}, {"x-route-nope", "route"}, - {":scheme", "http"}}; + return Http::TestRequestHeaderMapImpl{ + {":authority", host}, {":method", method}, {"x-safe", "safe"}, + {"x-global-nope", "global"}, {"x-vhost-nope", "vhost"}, {"x-route-nope", "route"}, + {"x-forwarded-proto", "http"}, {":scheme", "http"}}; } Http::TestRequestHeaderMapImpl genHeaders(const std::string& host, const std::string& path, const std::string& method, const std::string& scheme) { - auto hdrs = Http::TestRequestHeaderMapImpl{{":authority", host}, {":path", path}, - {":method", method}, {"x-safe", "safe"}, - {"x-global-nope", "global"}, {"x-vhost-nope", "vhost"}, - {"x-route-nope", "route"}, {":scheme", scheme}}; + auto hdrs = + Http::TestRequestHeaderMapImpl{{":authority", host}, {":path", path}, + {":method", method}, {"x-safe", "safe"}, + {"x-global-nope", "global"}, {"x-vhost-nope", "vhost"}, + {"x-route-nope", "route"}, {":scheme", scheme}, + {"x-forwarded-proto", scheme}}; if (scheme.empty()) { hdrs.remove(":scheme"); @@ -4191,8 +4193,9 @@ TEST_F(RouteMatcherTest, NoProtocolInHeadersWhenTlsIsRequired) { static Http::TestRequestHeaderMapImpl genRedirectHeaders(const std::string& host, const std::string& path, bool ssl, absl::optional internal) { + std::string scheme = ssl ? "https" : "http"; Http::TestRequestHeaderMapImpl headers{ - {":authority", host}, {":path", path}, {":scheme", ssl ? "https" : "http"}}; + {":authority", host}, {":path", path}, {":scheme", scheme}, {"x-forwarded-proto", scheme}}; if (internal.has_value()) { headers.addCopy("x-envoy-internal", internal.value() ? "true" : "false"); } diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 2e268c585a06f..6ed9e8f1985f9 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -121,7 +121,7 @@ stat_prefix: foo RouteConstSharedPtr route(Http::TestRequestHeaderMapImpl headers) { NiceMock stream_info; - headers.addCopy(":scheme", "http"); + headers.addCopy("x-forwarded-proto", "http"); return rds_->config()->route(headers, stream_info, 0); } diff --git a/test/common/router/router_ratelimit_test.cc b/test/common/router/router_ratelimit_test.cc index a6d4780e35729..75009618c20c3 100644 --- a/test/common/router/router_ratelimit_test.cc +++ b/test/common/router/router_ratelimit_test.cc @@ -71,8 +71,11 @@ TEST(BadRateLimitConfiguration, ActionsMissingRequiredFields) { static Http::TestRequestHeaderMapImpl genHeaders(const std::string& host, const std::string& path, const std::string& method) { - return Http::TestRequestHeaderMapImpl{ - {":authority", host}, {":path", path}, {":method", method}, {":scheme", "http"}}; + return Http::TestRequestHeaderMapImpl{{":authority", host}, + {":path", path}, + {":method", method}, + {"x-forwarded-proto", "http"}, + {":scheme", "http"}}; } class RateLimitConfiguration : public testing::Test { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index fea3c9bca0f84..badfe10da7024 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -5561,7 +5561,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { // With invalid x-forwarded-proto, still use scheme. { Http::TestRequestHeaderMapImpl headers; - headers.setXForwardedProto("foo"); + headers.setForwardedProto("foo"); FilterUtility::setUpstreamScheme(headers, true, true); EXPECT_EQ("https", headers.get_(":scheme")); } @@ -5569,7 +5569,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { // Use valid x-forwarded-proto. { Http::TestRequestHeaderMapImpl headers; - headers.setXForwardedProto(Http::Headers::get().SchemeValues.Http); + headers.setForwardedProto(Http::Headers::get().SchemeValues.Http); FilterUtility::setUpstreamScheme(headers, true, true); EXPECT_EQ("http", headers.get_(":scheme")); } @@ -5578,7 +5578,7 @@ TEST(RouterFilterUtilityTest, SetUpstreamScheme) { { Http::TestRequestHeaderMapImpl headers; headers.setScheme(Http::Headers::get().SchemeValues.Https); - headers.setXForwardedProto(Http::Headers::get().SchemeValues.Http); + headers.setForwardedProto(Http::Headers::get().SchemeValues.Http); FilterUtility::setUpstreamScheme(headers, false, false); EXPECT_EQ("https", headers.get_(":scheme")); } diff --git a/test/extensions/filters/http/compressor/compressor_integration_tests.cc b/test/extensions/filters/http/compressor/compressor_integration_tests.cc index e3723df2fa604..e15001246962c 100644 --- a/test/extensions/filters/http/compressor/compressor_integration_tests.cc +++ b/test/extensions/filters/http/compressor/compressor_integration_tests.cc @@ -71,9 +71,9 @@ void WebsocketWithCompressorIntegrationTest::validateUpgradeRequestHeaders( const Http::RequestHeaderMap& original_proxied_request_headers, const Http::RequestHeaderMap& original_request_headers) { Http::TestRequestHeaderMapImpl proxied_request_headers(original_proxied_request_headers); - if (proxied_request_headers.XForwardedProto()) { - ASSERT_EQ(proxied_request_headers.getXForwardedProtoValue(), "http"); - proxied_request_headers.removeXForwardedProto(); + if (proxied_request_headers.ForwardedProto()) { + ASSERT_EQ(proxied_request_headers.getForwardedProtoValue(), "http"); + proxied_request_headers.removeForwardedProto(); } // Check for and remove headers added by default for HTTP requests. diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index f1dafa9f271cd..694aebb9d41c7 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -72,14 +72,14 @@ TEST_P(Http2UpstreamIntegrationTest, TestSchemeAndXFP) { auto check_preserved = ([&](absl::string_view scheme, absl::string_view xff) { { default_request_headers_.setScheme(scheme); - default_request_headers_.setXForwardedProto(xff); + default_request_headers_.setForwardedProto(xff); auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); ASSERT_TRUE(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->getXForwardedProtoValue(), xff); + EXPECT_EQ(headers->getForwardedProtoValue(), xff); } }); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 8f2576fe79036..c7159200ab9e0 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -59,9 +59,9 @@ void WebsocketIntegrationTest::validateUpgradeRequestHeaders( const Http::RequestHeaderMap& original_proxied_request_headers, const Http::RequestHeaderMap& original_request_headers) { Http::TestRequestHeaderMapImpl proxied_request_headers(original_proxied_request_headers); - if (proxied_request_headers.XForwardedProto()) { - ASSERT_EQ(proxied_request_headers.getXForwardedProtoValue(), "http"); - proxied_request_headers.removeXForwardedProto(); + if (proxied_request_headers.ForwardedProto()) { + ASSERT_EQ(proxied_request_headers.getForwardedProtoValue(), "http"); + proxied_request_headers.removeForwardedProto(); } // Check for and remove headers added by default for HTTP requests. diff --git a/test/tools/router_check/router.cc b/test/tools/router_check/router.cc index d657457d36874..615f2a43dff45 100644 --- a/test/tools/router_check/router.cc +++ b/test/tools/router_check/router.cc @@ -70,6 +70,7 @@ ToolConfig ToolConfig::create(const envoy::RouterCheckToolSchema::ValidationItem request_headers->addCopy(":authority", check_config.input().authority()); request_headers->addCopy(":path", check_config.input().path()); request_headers->addCopy(":method", check_config.input().method()); + request_headers->addCopy("x-forwarded-proto", check_config.input().ssl() ? "https" : "http"); request_headers->addCopy(":scheme", check_config.input().ssl() ? "https" : "http"); if (check_config.input().internal()) { From 7e3f85172b0627d86a9c03647df3cc8ba2ee0c3b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Jul 2021 17:06:13 -0400 Subject: [PATCH 4/4] merge fix Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 6 +++--- source/common/router/config_impl.cc | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 5dec37efd693b..e8649bf5b5c38 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -10,13 +10,13 @@ Minor Behavior Changes *Changes that may cause incompatibilities for some users, but should not for most* * grpc: gRPC async client can be cached and shared accross filter instances in the same thread, this feature is turned off by default, can be turned on by setting runtime guard ``envoy.reloadable_features.enable_grpc_async_client_cache`` to true. +* http: correct the use of the ``x-forwarded-proto`` header and the ``:scheme`` header. Where they differ + (which is rare) ``:scheme`` will now be used for serving redirect URIs and cached content. This behavior + can be reverted by setting runtime guard ``correct_scheme_and_xfp`` to false. * http: set the default :ref:`lazy headermap threshold ` to 3, which defines the minimal number of headers in a request/response/trailers required for using a dictionary in addition to the list. Setting the `envoy.http.headermap.lazy_map_min_size` runtime feature to a non-negative number will override the default value. -* http: correct the use of the ``x-forwarded-proto`` header and the ``:scheme`` header. Where they differ - (which is rare) ``:scheme`` will now be used for serving redirect URIs and cached content. This behavior - can be reverted by setting runtime guard ``correct_scheme_and_xfp`` to false. * listener: added the :ref:`enable_reuse_port ` field and changed the default for reuse_port from false to true, as the feature is now well supported on the majority of production Linux kernels in use. The default change is aware of hot diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 33474aea19dfc..79ffc9aa09bb6 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -750,7 +750,7 @@ absl::string_view RouteEntryImplBase::processRequestHost(const Http::RequestHead if (host_end != absl::string_view::npos) { absl::string_view request_port = request_host.substr(host_end); - // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPs + // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS // connection), do port stripping based on X-Forwarded-Proto so http://foo.com:80 won't // have the port stripped when served over TLS. absl::string_view request_protocol = headers.getForwardedProtoValue(); @@ -1371,7 +1371,7 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb const Http::RequestHeaderMap& headers, const StreamInfo::StreamInfo& stream_info, uint64_t random_value) const { - // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPs + // In the rare case that X-Forwarded-Proto and scheme disagree (say http URL over an HTTPS // connection), force a redirect based on underlying protocol, rather than URL // scheme, so don't force a redirect for a http:// url served over a TLS // connection.