From 02cd1e2bd87409c1d1eadcefc37bfcd78b034cb4 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Sat, 31 Jul 2021 07:53:34 -0400 Subject: [PATCH 1/4] hcm: forbid mixing ip detection extensions with use_remote_addr/xff_num_trusted_hops Mixing extensions with previously existing knobs leads to undefined behavior, so this removes the deprecation around xff_num_trusted_hops and ensures that it's not mixed with extensions. Note that a unit test already exists for the original bug report, where use_remote_address is used with xff_num_trusted_hops > 0. However it uses the XFF extension instead of the old knob. Given this is now forbidden, there's no need for additional tests wrt that config combination. Fixes #17554. Signed-off-by: Raul Gutierrez Segales --- .../network/http_connection_manager/v3/BUILD | 1 - .../v3/http_connection_manager.proto | 20 +--------- .../v4alpha/http_connection_manager.proto | 11 ++++- docs/root/version_history/current.rst | 1 + .../v3/http_connection_manager.proto | 19 +-------- .../http_connection_manager/v4alpha/BUILD | 1 - .../v4alpha/http_connection_manager.proto | 20 +--------- .../network/http_connection_manager/config.cc | 10 +++++ .../http_connection_manager/config_test.cc | 40 +++++++++++++++++++ 9 files changed, 63 insertions(+), 60 deletions(-) diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/BUILD b/api/envoy/extensions/filters/network/http_connection_manager/v3/BUILD index 456f4e9e61702..55b63248136ce 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/BUILD +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/BUILD @@ -6,7 +6,6 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ - "//envoy/annotations:pkg", "//envoy/config/accesslog/v3:pkg", "//envoy/config/core/v3:pkg", "//envoy/config/filter/network/http_connection_manager/v2:pkg", diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index fa65ae4bcf757..d5f4c0d7af323 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -19,7 +19,6 @@ import "google/protobuf/any.proto"; import "google/protobuf/duration.proto"; import "google/protobuf/wrappers.proto"; -import "envoy/annotations/deprecation.proto"; import "udpa/annotations/migrate.proto"; import "udpa/annotations/security.proto"; import "udpa/annotations/status.proto"; @@ -501,24 +500,7 @@ message HttpConnectionManager { // determining the origin client's IP address. The default is zero if this option // is not specified. See the documentation for // :ref:`config_http_conn_man_headers_x-forwarded-for` for more information. - // - // .. note:: - // This field is deprecated and instead :ref:`original_ip_detection_extensions - // ` - // should be used to configure the :ref:`xff extension ` - // to configure IP detection using the :ref:`config_http_conn_man_headers_x-forwarded-for` header. To replace - // this field use a config like the following: - // - // .. code-block:: yaml - // - // original_ip_detection_extensions: - // - name: envoy.http.original_ip_detection.xff - // typed_config: - // "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig - // xff_num_trusted_hops: 1 - // - uint32 xff_num_trusted_hops = 19 - [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; + uint32 xff_num_trusted_hops = 19; // The configuration for the original IP detection extensions. // diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index bf3cc9ef34a49..44b02906b990f 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -304,9 +304,9 @@ message HttpConnectionManager { type.http.v3.PathTransformation http_filter_transformation = 2; } - reserved 27, 11, 19; + reserved 27, 11; - reserved "idle_timeout", "xff_num_trusted_hops"; + reserved "idle_timeout"; // Supplies the type of codec that the connection manager should use. CodecType codec_type = 1 [(validate.rules).enum = {defined_only: true}]; @@ -498,6 +498,13 @@ message HttpConnectionManager { google.protobuf.BoolValue use_remote_address = 14 [(udpa.annotations.security).configure_for_untrusted_downstream = true]; + // The number of additional ingress proxy hops from the right side of the + // :ref:`config_http_conn_man_headers_x-forwarded-for` HTTP header to trust when + // determining the origin client's IP address. The default is zero if this option + // is not specified. See the documentation for + // :ref:`config_http_conn_man_headers_x-forwarded-for` for more information. + uint32 xff_num_trusted_hops = 19; + // The configuration for the original IP detection extensions. // // When configured the extensions will be called along with the request headers diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 243facceaed48..eabed32164463 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -40,6 +40,7 @@ Bug Fixes * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. +* hcm: remove deprecation for xff_num_trusted_hops and forbid mixing ip detection extensions with old related knobs. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. Removed Config or Runtime diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index f09aac839adc5..e61a45b298299 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -506,24 +506,7 @@ message HttpConnectionManager { // determining the origin client's IP address. The default is zero if this option // is not specified. See the documentation for // :ref:`config_http_conn_man_headers_x-forwarded-for` for more information. - // - // .. note:: - // This field is deprecated and instead :ref:`original_ip_detection_extensions - // ` - // should be used to configure the :ref:`xff extension ` - // to configure IP detection using the :ref:`config_http_conn_man_headers_x-forwarded-for` header. To replace - // this field use a config like the following: - // - // .. code-block:: yaml - // - // original_ip_detection_extensions: - // - name: envoy.http.original_ip_detection.xff - // typed_config: - // "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig - // xff_num_trusted_hops: 1 - // - uint32 xff_num_trusted_hops = 19 - [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; + uint32 xff_num_trusted_hops = 19; // The configuration for the original IP detection extensions. // diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/BUILD b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/BUILD index 37cbc68f19156..64536cdef30b9 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/BUILD +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/BUILD @@ -6,7 +6,6 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ - "//envoy/annotations:pkg", "//envoy/config/accesslog/v4alpha:pkg", "//envoy/config/core/v4alpha:pkg", "//envoy/config/route/v4alpha:pkg", diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index d2332a1c9bb91..44b02906b990f 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -19,7 +19,6 @@ import "google/protobuf/any.proto"; import "google/protobuf/duration.proto"; import "google/protobuf/wrappers.proto"; -import "envoy/annotations/deprecation.proto"; import "udpa/annotations/security.proto"; import "udpa/annotations/status.proto"; import "udpa/annotations/versioning.proto"; @@ -504,24 +503,7 @@ message HttpConnectionManager { // determining the origin client's IP address. The default is zero if this option // is not specified. See the documentation for // :ref:`config_http_conn_man_headers_x-forwarded-for` for more information. - // - // .. note:: - // This field is deprecated and instead :ref:`original_ip_detection_extensions - // ` - // should be used to configure the :ref:`xff extension ` - // to configure IP detection using the :ref:`config_http_conn_man_headers_x-forwarded-for` header. To replace - // this field use a config like the following: - // - // .. code-block:: yaml - // - // original_ip_detection_extensions: - // - name: envoy.http.original_ip_detection.xff - // typed_config: - // "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig - // xff_num_trusted_hops: 1 - // - uint32 hidden_envoy_deprecated_xff_num_trusted_hops = 19 - [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; + uint32 xff_num_trusted_hops = 19; // The configuration for the original IP detection extensions. // diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index d548646277dd7..fbaf5d5e9bfb1 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -373,6 +373,16 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( auto* extension = ip_detection_extensions.Add(); extension->set_name("envoy.http.original_ip_detection.xff"); extension->mutable_typed_config()->PackFrom(xff_config); + } else { + if (use_remote_address_) { + throw EnvoyException( + "Original IP detection extensions and use_remote_address may not be mixed"); + } + + if (xff_num_trusted_hops_ > 0) { + throw EnvoyException( + "Original IP detection extensions and xff_num_trusted_hops may not be mixed"); + } } original_ip_detection_extensions_.reserve(ip_detection_extensions.size()); diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 8bd62d3564d4e..f6ccecac5856e 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -2075,6 +2075,46 @@ TEST_F(HttpConnectionManagerConfigTest, OriginalIPDetectionExtension) { EXPECT_EQ(1, original_ip_detection_extensions.size()); } +TEST_F(HttpConnectionManagerConfigTest, OriginalIPDetectionExtensionMixedWithUseRemoteAddress) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + use_remote_address: true + original_ip_detection_extensions: + - name: envoy.http.original_ip_detection.custom_header + typed_config: + "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig + header_name: x-ip-header + http_filters: + - name: envoy.filters.http.router + )EOF"; + + EXPECT_THROW_WITH_REGEX( + createHttpConnectionManagerConfig(yaml_string), EnvoyException, + "Original IP detection extensions and use_remote_address may not be mixed"); +} + +TEST_F(HttpConnectionManagerConfigTest, OriginalIPDetectionExtensionMixedWithNumTrustedHops) { + const std::string yaml_string = R"EOF( + stat_prefix: ingress_http + route_config: + name: local_route + xff_num_trusted_hops: 1 + original_ip_detection_extensions: + - name: envoy.http.original_ip_detection.custom_header + typed_config: + "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.custom_header.v3.CustomHeaderConfig + header_name: x-ip-header + http_filters: + - name: envoy.filters.http.router + )EOF"; + + EXPECT_THROW_WITH_REGEX( + createHttpConnectionManagerConfig(yaml_string), EnvoyException, + "Original IP detection extensions and xff_num_trusted_hops may not be mixed"); +} + TEST_F(HttpConnectionManagerConfigTest, DynamicFilterWarmingNoDefault) { const std::string yaml_string = R"EOF( codec_type: http1 From 20c7cb935255c78f7a0b5df241620d0fb34dc4e2 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 2 Aug 2021 22:12:30 -0400 Subject: [PATCH 2/4] Docs Signed-off-by: Raul Gutierrez Segales --- .../http/http_conn_man/headers.rst | 17 ++++++++++------- docs/root/version_history/current.rst | 2 +- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/headers.rst b/docs/root/configuration/http/http_conn_man/headers.rst index 5ea4e7946dc84..b28b0133ebac6 100644 --- a/docs/root/configuration/http/http_conn_man/headers.rst +++ b/docs/root/configuration/http/http_conn_man/headers.rst @@ -210,10 +210,16 @@ Given an HTTP request that has traveled through a series of zero or more proxies Envoy, the trusted client address is the earliest source IP address that is known to be accurate. The source IP address of the immediate downstream node's connection to Envoy is trusted. XFF *sometimes* can be trusted. Malicious clients can forge XFF, but the last -address in XFF can be trusted if it was put there by a trusted proxy. Alternatively, Envoy -supports :ref:`extensions ` +address in XFF can be trusted if it was put there by a trusted proxy. + +Alternatively, Envoy supports +:ref:`extensions ` for determining the *trusted client address* or original IP address. +.. note:: + + The use of such extensions cannot be mixed with *use_remote_address* nor *xff_num_trusted_hops*. + Envoy's default rules for determining the trusted client address (*before* appending anything to XFF) are: @@ -223,11 +229,8 @@ to XFF) are: node's connection to Envoy. In an environment where there are one or more trusted proxies in front of an edge -Envoy instance, the :ref:`XFF extension ` -can be configured via the :ref:`original_ip_detection_extensions field -` -to set the *xff_num_trusted_hops* option which controls the number of additional -addresses that are to be trusted: +Envoy instance, the *xff_num_trusted_hops* configuration option can be used to trust +additional addresses from XFF: * If *use_remote_address* is false and *xff_num_trusted_hops* is set to a value *N* that is greater than zero, the trusted client address is the (N+1)th address from the right end diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index eabed32164463..256687cac974b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -40,7 +40,7 @@ Bug Fixes * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. -* hcm: remove deprecation for xff_num_trusted_hops and forbid mixing ip detection extensions with old related knobs. +* hcm: remove deprecation for :ref:`xff_num_trusted_hops ` and forbid mixing ip detection extensions with old related knobs. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. Removed Config or Runtime From 87d818da9123903a90895f0caa460b3e86ecbb41 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Wed, 4 Aug 2021 18:15:52 -0400 Subject: [PATCH 3/4] Add warning to the original_ip_detection_extensions field Signed-off-by: Raul Gutierrez Segales --- .../v3/http_connection_manager.proto | 6 ++++++ .../v4alpha/http_connection_manager.proto | 6 ++++++ .../v3/http_connection_manager.proto | 6 ++++++ .../v4alpha/http_connection_manager.proto | 6 ++++++ 4 files changed, 24 insertions(+) diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index d5f4c0d7af323..ab51333df5c9f 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -512,6 +512,12 @@ message HttpConnectionManager { // the request. If the request isn't rejected nor any extension succeeds, the HCM will // fallback to using the remote address. // + // .. WARNING:: + // Extensions cannot be used in conjunction with :ref:`use_remote_address + // ` + // nor :ref:`xff_num_trusted_hops + // ` + // nor :ref:`xff_num_trusted_hops + // ` + // nor :ref:`xff_num_trusted_hops + // ` + // nor :ref:`xff_num_trusted_hops + // Date: Wed, 4 Aug 2021 18:39:20 -0400 Subject: [PATCH 4/4] Fixes * fix link * update old integration test to go back to not use extensions Signed-off-by: Raul Gutierrez Segales --- .../v3/http_connection_manager.proto | 2 +- .../v4alpha/http_connection_manager.proto | 2 +- .../v3/http_connection_manager.proto | 2 +- .../v4alpha/http_connection_manager.proto | 2 +- test/integration/header_integration_test.cc | 6 +----- 5 files changed, 5 insertions(+), 9 deletions(-) diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index ab51333df5c9f..3fb4bfa09e206 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -516,7 +516,7 @@ message HttpConnectionManager { // Extensions cannot be used in conjunction with :ref:`use_remote_address // ` // nor :ref:`xff_num_trusted_hops - // `. // // [#extension-category: envoy.http.original_ip_detection] repeated config.core.v3.TypedExtensionConfig original_ip_detection_extensions = 46; diff --git a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 11711e013ce4c..80972e52a0956 100644 --- a/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/api/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -519,7 +519,7 @@ message HttpConnectionManager { // Extensions cannot be used in conjunction with :ref:`use_remote_address // ` // nor :ref:`xff_num_trusted_hops - // `. // // [#extension-category: envoy.http.original_ip_detection] repeated config.core.v4alpha.TypedExtensionConfig original_ip_detection_extensions = 46; diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto index 20ec55f084926..b5544eaa93b7c 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto @@ -522,7 +522,7 @@ message HttpConnectionManager { // Extensions cannot be used in conjunction with :ref:`use_remote_address // ` // nor :ref:`xff_num_trusted_hops - // `. // // [#extension-category: envoy.http.original_ip_detection] repeated config.core.v3.TypedExtensionConfig original_ip_detection_extensions = 46; diff --git a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto index 11711e013ce4c..80972e52a0956 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/http_connection_manager/v4alpha/http_connection_manager.proto @@ -519,7 +519,7 @@ message HttpConnectionManager { // Extensions cannot be used in conjunction with :ref:`use_remote_address // ` // nor :ref:`xff_num_trusted_hops - // `. // // [#extension-category: envoy.http.original_ip_detection] repeated config.core.v4alpha.TypedExtensionConfig original_ip_detection_extensions = 46; diff --git a/test/integration/header_integration_test.cc b/test/integration/header_integration_test.cc index b82ce208251f3..2ceef32088453 100644 --- a/test/integration/header_integration_test.cc +++ b/test/integration/header_integration_test.cc @@ -43,11 +43,7 @@ const std::string http_connection_mgr_config = R"EOF( - name: envoy.filters.http.router codec_type: HTTP1 use_remote_address: false -original_ip_detection_extensions: -- name: envoy.http.original_ip_detection.xff - typed_config: - "@type": type.googleapis.com/envoy.extensions.http.original_ip_detection.xff.v3.XffConfig - xff_num_trusted_hops: 1 +xff_num_trusted_hops: 1 stat_prefix: header_test route_config: virtual_hosts: