xff: add support for configuring a list of trusted CIDRs #31831

jamesog · 2024-01-15T19:25:10Z

Commit Message: xff: add support for configuring a list of trusted CIDRs

The original client IP address can be determined from the x-forwarded-for header either by a fixed number of trusted hops, or by evaluating the client IP address against a list of trusted addresses.

This adds support for configuring a list of CIDRs in the xff original IP detection extension. The remote IP address is evaluated against these, and optionally recurses through XFF to find the last non-trusted address.

Additional Description:
This feature is generally used by people with a CDN in front of their edge proxy to ensure that XFF is only parsed when the remote connection comes from a CDN server.

The behaviour of the new functionality should be the same as Nginx's realip module.

Disclaimer: This is my first time writing C++ so I'm not certain my changes are completely idiomatic, but I've tried to stick with existing style in the codebase. Feedback very welcome!

Risk Level: Medium
Testing: Unit tests, manual tests
Docs Changes: Updates to HTTP Connection Manager header manipulation docs, and proto docs.
Release Notes: Added to changelogs/current.yaml
Platform Specific Features: None
Fixes #21639
Relates to #31296

X-Forwarded-For parsing can either trust a number of hops, as Envoy does today, or it can trust by IP address. This enables configuration to add a set of CIDRs to the XFF config, and a recurse option. When recurse is not enabled, HCM should trust a remote IP that is covered by one of the configured CIDRs and use the last IP in XFF as the remote IP. When recurse is enabled, if the remote IP is covered by one of the CIDRs, HCM should also walk backwards through XFF to find the first IP not covered by the trusted CIDR list. Signed-off-by: James O'Gorman <[email protected]>

remoteAddressIsTrustedProxy checks the remote IP against a list of CIDRs. If it is covered by any CIDR the remote is considered a trusted proxy. getLastNonTrustedAddressFromXFF walks backwards through XFF to find the first IP that is not covered by a list of trusted CIDRs. This IP is considered the "true" remote IP. Signed-off-by: James O'Gorman <[email protected]>

If a list of trusted CIDRs is set, rather than checking for a set number of trusted hops, instead check the remote IP against the trusted CIDR list. If recurse is set, XFF is also evaluated to find the last IP in the list that is not covered by the trusted CIDRs. Signed-off-by: James O'Gorman <[email protected]>

Signed-off-by: James O'Gorman <[email protected]>

repokitteh-read-only · 2024-01-15T19:25:15Z

Hi @jamesog, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #31831 was opened by jamesog.

see: more, trace.

repokitteh-read-only · 2024-01-15T19:25:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31831 was opened by jamesog.

see: more, trace.

Signed-off-by: James O'Gorman <[email protected]>

source/extensions/http/original_ip_detection/xff/xff.cc

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto

Override xff_num_trusted_hops in the constructor instead of validating in the proto message when xff_trusted_cidrs is set. Signed-off-by: James O'Gorman <[email protected]>

Signed-off-by: James O'Gorman <[email protected]>

markdroth · 2024-01-17T17:29:32Z

/lgtm api

alyssawilk

So sorry for the review delay. Thanks for the well documented and tested first pass - my thoughts below.
/wait

alyssawilk · 2024-01-23T16:25:40Z

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto

@@ -22,5 +26,37 @@ message XffConfig {
  // 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.
+  //
+  // If ``xff_trusted_cidrs`` is configured, this field is not used.


can we disallow it being set and reject config if it is?

I originally had this change as using a oneof encapsulating the original xff_num_trusted_hops and the new xff_trusted_cidrs fields, but Mark told me that would be a breaking API change and to do it this way instead. Do you know if there's another way to handle rejecting it without it being a breaking change?

I don't think we want to disallow if both the old and new fields are set, since that makes it hard for control planes to start using this new feature.

If we reject when both fields are set, then a control plane must know whether any given client supports the new field. If it sets only the new field, old clients will not work properly, and if it sets both, new clients will break. So the only way to transition from the old field to the new field is to either (a) wait for all clients to be upgraded before starting to use the new field or (b) have some way for the control plane to know which client supports the new field and which clients support the old field so that it can send the right config to each one -- and doing that in a way that does not break cacheability requires some dances with dynamic parameter support, which means that the clients have to be configured to tell the control plane whether or not they support the new field.

In contrast, if we allow both fields to be set and specify the precedence order, then this becomes much easier. Control planes can simply set both fields. Old clients will use the old field, and new clients will prefer the new field.

I think the latter approach is much cleaner and should be preferred any time we add this kind of new field in xDS.

How do you feel about the change I made since Alyssa's feedback? d976354

She suggested it should either log or reject; I chose to reject by throwing an error with a message that clearly informs that only one field can be set.

What I'm saying is that I don't think that's the behavior we want. I think the way you had this originally was the right thing to do.

In that case I'm getting conflicting advice from two different maintainers :-)

@alyssawilk perhaps you can help decide which way I should take this based on Mark's feedback?

alyssawilk · 2024-01-23T16:27:41Z

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto

+  //
+  // If ``recurse`` is set the :ref:`config_http_conn_man_headers_x-forwarded-for`
+  // header is also evaluated against the trusted CIDR list, and the last non-trusted
+  // address is used as the original client address.


ooc when would we not want to recurse?

It's a good question! I added some commentary to the docs in 1f76584.

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto

alyssawilk · 2024-01-23T16:33:46Z

source/common/http/utility.cc

+    }
+    return {addr, true};
+  }
+  return {nullptr, false};


if every address in the XFF is trusted don't we want to return the last address in the list?

This one made me think! Originally I was assuming this might be some kind of undefined behaviour and so it was safer to return null, but after talking about this with a colleague, and looking over what other proxy servers do, I implemented your suggestion in a016a6c. Thanks for questioning my logic :-)

When parsing XFF using a list of trusted CIDRs, if all IP in XFF match the trusted CIDRs (there are no untrusted IPs), return the first IP in XFF (the last evaluated) - and a test to confirm the behaviour. Signed-off-by: James O'Gorman <[email protected]>

Signed-off-by: James O'Gorman <[email protected]>

adisuissa · 2024-08-20T20:38:26Z

/retest

wbpcode · 2024-08-21T12:28:44Z

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto

+  //   For proper proxy behaviour it is not recommended to set this option.
+  //   For backwards compatibility, if this option is unset and
+  //   ``xff_num_trusted_hops`` is set, it defaults to true. When
+  //   ``xff_trusted_cidrs`` is set, it defaults to false.


Different default values only brings confusion. Let's only use a default value with true.

Please see the related conversation below: #31831 (comment)

@alyssawilk seemed half inclined to go with this split default, but let me know if you still think we should really keep one default.

true is a bad default, to be honest, but I also don't want to break any existing users of xff_num_trusted_hops.

From API's perspective, explicit and predictable behavior is always better. Different default values is always not recommend for our API.

Got it. I've updated this to default to true.

wbpcode · 2024-08-21T12:38:47Z

source/common/http/utility.cc

+bool Utility::remoteAddressIsTrustedProxy(
+    const Envoy::Network::Address::InstanceConstSharedPtr& remote,
+    const std::vector<Network::Address::CidrRange> trusted_cidrs) {
+  for (const auto& cidr : trusted_cidrs) {
+    if (cidr.isInRange(*remote.get())) {
+      return true;
+    }
+  }
+  return false;
+}


const Envoy::Network::Address::Instance& remote: const reference to shared pointer here make no sense.
absl::Span<const Network::Address::CidrRange>: to avoid copy whole vector. (or vector reference should be used.)

Thank you, these have been updated.

wbpcode

Thanks for the contribution. Some comments are added.

wbpcode · 2024-08-21T12:40:28Z

source/common/http/utility.cc

+
+Utility::GetLastAddressFromXffInfo Utility::getLastNonTrustedAddressFromXFF(
+    const Http::RequestHeaderMap& request_headers,
+    const std::vector<Network::Address::CidrRange> trusted_cidrs) {


ditto. Please use span or vector reference.

Updated, thanks.

wbpcode · 2024-08-21T12:43:41Z

source/common/http/utility.cc

+  static constexpr absl::string_view separator(",");
+  static constexpr size_t MAX_ALLOWED_XFF_ENTRIES = 20;
+
+  const auto xff_entries = StringUtil::splitToken(xff_header, separator, false, true);


which type is used here to store the splitted tokens? Seems auto is not appropriate here. Maybe you should specify a type here. ~~InlineVector<absl::string_view, 8>~~ InlinedVector<absl::string_view, 8> will be recommend.

Updated. I used std::vector<absl::string_view> as this seemed to be the style elsewhere and I couldn't find any reference of InlineVector.

absl::InlinedVector provide better performance when we only has limited hops (in most cases?). And sorry for the spelling error.

Thanks for the explanation. I've updated to use InlinedVector now.

wbpcode · 2024-08-21T12:47:56Z

source/extensions/http/original_ip_detection/xff/xff.cc

+  if (config.has_xff_trusted_cidrs() && config.xff_num_trusted_hops() > 0) {
+    ENVOY_LOG(warn, "Both xff_num_trusted_hops and xff_trusted_cidrs are configured; only "
+                    "xff_trusted_cidrs will be used");
+  }


It should be safe to reject the configuration?

I had previously tried to do that by throwing, but this causes failures for Mobile. Is there another way to reject the configuration?

Maybe you can should use throwExceptionOrPanic method for now.

Is panicking acceptable for Mobile?

I've added the throwExceptionOrPanic. Hopefully this won't cause the Mobile tests to fail this time. Thanks for the pointer.

wbpcode · 2024-08-21T12:49:07Z

/wait

Signed-off-by: James O'Gorman <[email protected]>

jamesog · 2024-08-23T11:34:08Z

/retest

wbpcode · 2024-08-31T15:16:07Z

Sorry for the delay. Some times I may ignore the notification from tools. It's OK to ping me directly on slack.

wbpcode · 2024-08-31T15:16:57Z

/wait

The API should provide a consistent default value, and we cannot set this to false without causing a breaking change. Instead we have a warning in the API docs recommending to disable this option. Signed-off-by: James O'Gorman <[email protected]>

Signed-off-by: James O'Gorman <[email protected]>

This reverts c69df1c and changes to use throwEnvoyExceptionOrPanic, which will cause a config error in Envoy, and a panic in Envoy Mobile. Signed-off-by: James O'Gorman <[email protected]>

Signed-off-by: James O'Gorman <[email protected]>

wbpcode

Thanks for the great contribution. Only last comment, I think.

wbpcode · 2024-09-04T03:47:26Z

source/common/http/utility.cc

+bool Utility::remoteAddressIsTrustedProxy(
+    const Envoy::Network::Address::InstanceConstSharedPtr remote,


See, basically we only use the smart point as input parameter when we wants to change the ownership. But in this case, if we ensure this remote always be valid, then const Envoy::Network::Address::Instance& should be used.

If we can ensure this remote always be valid, then OptRef<const Envoy::Network::Address::Instance> should be used.

I think there should be the first case.

I think I understood you correctly. This is getting beyond my knowledge here so I wasn't quite sure. Please see the latest commit that now uses const Network::Address:Instance&.

Signed-off-by: James O'Gorman <[email protected]>

wbpcode

LGTM. Thanks.

jamesog added 5 commits January 15, 2024 12:34

docs: Note about xff_trusted_cidrs.

831057e

Signed-off-by: James O'Gorman <[email protected]>

changelog: Document xff trusted CIDR addition

b32b10d

Signed-off-by: James O'Gorman <[email protected]>

jamesog requested review from alyssawilk and mattklein123 as code owners January 15, 2024 19:25

repokitteh-read-only bot added the api label Jan 15, 2024

repokitteh-read-only bot assigned markdroth Jan 15, 2024

jamesog added 2 commits January 15, 2024 19:53

Format fixes

b37899d

Signed-off-by: James O'Gorman <[email protected]>

Fix spell check issues

34e7d33

Signed-off-by: James O'Gorman <[email protected]>

jamesog commented Jan 15, 2024

View reviewed changes

source/extensions/http/original_ip_detection/xff/xff.cc Outdated Show resolved Hide resolved

jamesog mentioned this pull request Jan 16, 2024

Configurable filter of trusted proxies to handle x-forwarded-for header #21639

Closed

markdroth reviewed Jan 16, 2024

View reviewed changes

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto Outdated Show resolved Hide resolved

api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto Outdated Show resolved Hide resolved

jamesog added 6 commits January 16, 2024 20:22

api: Remove use of oneof in XffConfig

61ade56

Override xff_num_trusted_hops in the constructor instead of validating in the proto message when xff_trusted_cidrs is set. Signed-off-by: James O'Gorman <[email protected]>

xff: Remove unused constructor

0cf5c6c

Signed-off-by: James O'Gorman <[email protected]>

Spelling: quote symbols

4c0ff3f

Signed-off-by: James O'Gorman <[email protected]>

Autoformat

d9f1a64

Signed-off-by: James O'Gorman <[email protected]>

Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs

5842f38

Signed-off-by: James O'Gorman <[email protected]>

api: Remove unused import

2c95c9f

Signed-off-by: James O'Gorman <[email protected]>

repokitteh-read-only bot removed the api label Jan 17, 2024

ravenblackx assigned alyssawilk Jan 19, 2024

alyssawilk reviewed Jan 23, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 23, 2024

jamesog added 3 commits January 24, 2024 17:18

api: Note about n*m operation with xff_trusted_cidrs

eb2f7ff

Signed-off-by: James O'Gorman <[email protected]>

api: Clarify when recurse may want to be used

1f76584

Signed-off-by: James O'Gorman <[email protected]>

This comment was marked as outdated.

Sign in to view

alyssawilk removed their assignment Aug 20, 2024

wbpcode reviewed Aug 21, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 21, 2024

jamesog added 2 commits August 21, 2024 20:47

hcm: Address feedback

7721581

Signed-off-by: James O'Gorman <[email protected]>

Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs

837cd5f

Signed-off-by: James O'Gorman <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 21, 2024

repokitteh-read-only bot added the waiting label Aug 31, 2024

jamesog added 4 commits August 31, 2024 18:49

hcm: Use absl::InlinedVector

aec2db5

Signed-off-by: James O'Gorman <[email protected]>

xff: Revert c69df1c

57baab7

This reverts c69df1c and changes to use throwEnvoyExceptionOrPanic, which will cause a config error in Envoy, and a panic in Envoy Mobile. Signed-off-by: James O'Gorman <[email protected]>

Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs

7344aa8

Signed-off-by: James O'Gorman <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 31, 2024

Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs

277fefd

Signed-off-by: James O'Gorman <[email protected]>

wbpcode reviewed Sep 4, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 4, 2024

xff: Don't use smart pointer

91903c9

Signed-off-by: James O'Gorman <[email protected]>

repokitteh-read-only bot removed the waiting label Sep 4, 2024

wbpcode approved these changes Sep 9, 2024

View reviewed changes

repokitteh-read-only bot removed the api label Sep 9, 2024

wbpcode merged commit fbc6ee2 into envoyproxy:main Sep 9, 2024
39 of 40 checks passed

larhauga mentioned this pull request Sep 18, 2024

Support envoy X-Forwarded-For trusted CIDRs istio/istio#53185

Open

robd003 mentioned this pull request Oct 21, 2024

Allow setting of trusted CIDR blocks for X-Forwarded-For envoyproxy/gateway#4489

Open

		bool Utility::remoteAddressIsTrustedProxy(
		const Envoy::Network::Address::InstanceConstSharedPtr remote,

xff: add support for configuring a list of trusted CIDRs #31831

xff: add support for configuring a list of trusted CIDRs #31831

Conversation

jamesog commented Jan 15, 2024

repokitteh-read-only bot commented Jan 15, 2024

repokitteh-read-only bot commented Jan 15, 2024

markdroth commented Jan 17, 2024

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

adisuissa commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Aug 21, 2024

jamesog commented Aug 23, 2024

wbpcode commented Aug 31, 2024

wbpcode commented Aug 31, 2024

wbpcode left a comment

Choose a reason for hiding this comment

wbpcode Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment

Choose a reason for hiding this comment

wbpcode Aug 21, 2024 •

edited

Loading

wbpcode Sep 4, 2024 •

edited

Loading