Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
70893bc
api: Add trusted CIDRs to XFF config
jamesog Jan 10, 2024
37f82b8
http: Utility functions for parsing XFF by CIDR
jamesog Jan 10, 2024
5f390fd
xff: Allow parsing XFF using trusted CIDRs
jamesog Jan 10, 2024
831057e
docs: Note about xff_trusted_cidrs.
jamesog Jan 14, 2024
b32b10d
changelog: Document xff trusted CIDR addition
jamesog Jan 14, 2024
b37899d
Format fixes
jamesog Jan 15, 2024
34e7d33
Fix spell check issues
jamesog Jan 15, 2024
61ade56
api: Remove use of oneof in XffConfig
jamesog Jan 16, 2024
0cf5c6c
xff: Remove unused constructor
jamesog Jan 16, 2024
4c0ff3f
Spelling: quote symbols
jamesog Jan 16, 2024
d9f1a64
Autoformat
jamesog Jan 16, 2024
5842f38
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Jan 16, 2024
2c95c9f
api: Remove unused import
jamesog Jan 16, 2024
a016a6c
http: Use last address in XFF when all are trusted
jamesog Jan 23, 2024
eb2f7ff
api: Note about n*m operation with xff_trusted_cidrs
jamesog Jan 24, 2024
1f76584
api: Clarify when recurse may want to be used
jamesog Jan 24, 2024
994d4be
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Jan 24, 2024
ebb1391
http: Use clearer variable name
jamesog Feb 11, 2024
d976354
xff: Throw error when setting both options
jamesog Feb 11, 2024
666757e
hcm: Support returning whether to append XFF
jamesog Feb 11, 2024
4193f48
docs: Add examples for xff_trusted_cidrs
jamesog Feb 11, 2024
a587a23
hcm: Only treat as internal when XFF has a single address
jamesog Feb 11, 2024
06eccf5
api: Use bool for explicitly false defaults
jamesog Feb 25, 2024
87bf560
docs: Apply suggestion from Matt
jamesog Feb 25, 2024
2c15af4
hcm: Append XFF in original IP detection, add tests
jamesog Feb 25, 2024
901335a
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Feb 25, 2024
6833c32
xff: Fix bool usage after proto change
jamesog Feb 25, 2024
26756eb
test: Fix getXFFExtension usage
jamesog Feb 25, 2024
6671ece
api: Move N*M comment
jamesog Feb 27, 2024
5ed4660
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Feb 29, 2024
c69df1c
xff: xff_trusted_cidrs should take precedence
jamesog Jul 13, 2024
eab825a
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Jul 13, 2024
9cfe9d6
xff: Unwrap Network::Address::CidrRange
jamesog Jul 13, 2024
774cacc
xff: Remove recurse option, fail on invalid XFF entry
jamesog Aug 5, 2024
5c2cbf2
hcm: Factor out code to append to XFF
jamesog Aug 5, 2024
e4ff49e
xff: Rename append_xff to skip_xff_append
jamesog Aug 5, 2024
e2fdae2
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Aug 6, 2024
9a625aa
xff: Use getForwardedForValue
jamesog Aug 8, 2024
0267706
hcm: Test empty XFF header value
jamesog Aug 8, 2024
3dd64cc
xff: Remove internal/loopback constraint
jamesog Aug 8, 2024
a9432b4
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Aug 12, 2024
bfaa8b3
hcm: Style fix
jamesog Aug 13, 2024
1055b01
xff: Restrict XFF header to 20 entries
jamesog Aug 13, 2024
7721581
hcm: Address feedback
jamesog Aug 21, 2024
837cd5f
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Aug 21, 2024
a80178e
xff: Set skip_xff_append to true by default
jamesog Aug 31, 2024
aec2db5
hcm: Use absl::InlinedVector
jamesog Aug 31, 2024
57baab7
xff: Revert c69df1c556e4d71dee0ad7460261eefb6df5a58d
jamesog Aug 31, 2024
7344aa8
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Aug 31, 2024
277fefd
Merge remote-tracking branch 'upstream/main' into xff_trusted_cidrs
jamesog Sep 3, 2024
91903c9
xff: Don't use smart pointer
jamesog Sep 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion api/envoy/extensions/http/original_ip_detection/xff/v3/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@ load("@envoy_api//bazel:api_build_system.bzl", "api_proto_package")
licenses(["notice"]) # Apache 2

api_proto_package(
deps = ["@com_github_cncf_xds//udpa/annotations:pkg"],
deps = [
"//envoy/config/core/v3:pkg",
"@com_github_cncf_xds//udpa/annotations:pkg",
],
)
36 changes: 36 additions & 0 deletions api/envoy/extensions/http/original_ip_detection/xff/v3/xff.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ syntax = "proto3";

package envoy.extensions.http.original_ip_detection.xff.v3;

import "envoy/config/core/v3/address.proto";

import "google/protobuf/wrappers.proto";

import "udpa/annotations/status.proto";

option java_package = "io.envoyproxy.envoy.extensions.http.original_ip_detection.xff.v3";
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

uint32 xff_num_trusted_hops = 1;

// The `CIDR <https://tools.ietf.org/html/rfc4632>`_ ranges to trust when
// evaluating the remote IP address to determine the origin client's IP address.
// This is used instead of
// :ref:`use_remote_address <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.use_remote_address>`.
// When the remote IP address matches a trusted CIDR and the
// :ref:`config_http_conn_man_headers_x-forwarded-for` header was sent, the last
// IP address from ``x-forwarded-for`` is used as the original client address.
// If no ``x-forwarded-for`` header was sent, the remote IP is considered the real IP.
//
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

ooc when would we not want to recurse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

//
// This is typically used when requests are proxied by a
// `CDN <https://en.wikipedia.org/wiki/Content_delivery_network>`_.
//
// If this field is configured, ``xff_num_trusted_hops`` is not used.
XffTrustedCidrs xff_trusted_cidrs = 2;
}

message XffTrustedCidrs {
// The list of `CIDRs <https://tools.ietf.org/html/rfc4632>`_ from which remote
// connections are considered trusted.
repeated config.core.v3.CidrRange cidrs = 1;

// If ``true``, recurse through the :ref:`config_http_conn_man_headers_x-forwarded-for`
// HTTP header to find the first (from the right) non-trusted IP address; otherwise
// the last address in ``x-forwarded-for`` is used.
google.protobuf.BoolValue recurse = 2;
Copy link
Member

Choose a reason for hiding this comment

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

This can just be a bool as the default is false. If the default is actually true this should be indicated in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

}
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,9 @@ removed_config_or_runtime:
# *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

new_features:
- area: original_ip_detection extension
change: |
The :ref:`xff <envoy_v3_api_msg_extensions.http.original_ip_detection.xff.v3.XffConfig>`
original IP detection method now supports using a list of trusted CIDRs when parsing ``x-forwarded-for``.

deprecated:
6 changes: 6 additions & 0 deletions docs/root/configuration/http/http_conn_man/headers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,12 @@ additional addresses from XFF:
the XFF contains fewer than N addresses, Envoy falls back to using the immediate downstream
connection's source address as trusted client address.)

.. note::

If the trusted client address should be determined from a list of known CIDRs, use the
:ref:`xff <envoy_v3_api_msg_extensions.http.original_ip_detection.xff.v3.XffConfig>` original IP
detection extension instead.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
detection extension instead.
detection option instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


Envoy uses the trusted client address contents to determine whether a request originated
externally or internally. This influences whether the
:ref:`config_http_conn_man_headers_x-envoy-internal` header is set.
Expand Down
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@ envoy_cc_library(
"//source/common/common:enum_to_int",
"//source/common/common:utility_lib",
"//source/common/grpc:status_lib",
"//source/common/network:cidr_range_lib",
"//source/common/network:utility_lib",
"//source/common/protobuf:utility_lib",
"//source/common/runtime:runtime_features_lib",
Expand Down
56 changes: 56 additions & 0 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "source/common/http/header_map_impl.h"
#include "source/common/http/headers.h"
#include "source/common/http/message_impl.h"
#include "source/common/network/cidr_range.h"
#include "source/common/network/utility.h"
#include "source/common/protobuf/utility.h"
#include "source/common/runtime/runtime_features.h"
Expand Down Expand Up @@ -781,6 +782,61 @@ void Utility::sendLocalReply(const bool& is_reset, const EncodeFunctions& encode
encodeLocalReply(is_reset, std::move(prepared_local_reply));
}

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;
}
Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, these have been updated.


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

Choose a reason for hiding this comment

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

ditto. Please use span or vector reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

const auto xff_header = request_headers.ForwardedFor();
if (xff_header == nullptr) {
return {nullptr, false};
}

absl::string_view xff_string(xff_header->value().getStringView());
Copy link
Contributor

Choose a reason for hiding this comment

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

request_headers.getForwardedForValue() and you can skip the null pointer checks above

static constexpr absl::string_view separator(",");

const auto xff_entries = StringUtil::splitToken(xff_string, separator, false, true);

for (auto it = xff_entries.rbegin(); it != xff_entries.rend(); it++) {
auto addr = Network::Utility::parseInternetAddressNoThrow(std::string(*it));
if (addr == nullptr) {
continue;
}

bool trusted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

remoteAddressIsTrustedProxy?

for (const auto& cidr : trusted_cidrs) {
if (cidr.isInRange(*addr.get())) {
trusted = true;
break;
}
}

if (trusted) {
continue;
}

// If we reach here we found a non-trusted address
if (Network::Utility::isInternalAddress(*addr.get()) ||
Network::Utility::isLoopbackAddress(*addr.get())) {
// If the next IP after skipping all trusted IPs is internal or loopback
// we cannot determine the true remote IP: Either a proxy didn't add its IP,
// or XFF is malformed.
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry probably naive here but if every hop is trusted do we not trust the whole set?
Agree if we have trusted, untrusted, trusted I wouldn't trust the final trusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The aim is to find the first non-trusted public IP. The condition here says if we treated all previous IPs as proxies we trust, but we then hit a private IP, we can't determine the "real" client IP so we bail out.

Choose a reason for hiding this comment

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

I think in that case the "real" IP would be the private IP, right? I think that's a valid case, where the end client that the traffic originated from was an internal IP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "real real" IP may well be a private IP, yes, but a private IP isn't useful to an Internet-facing proxy and, AFAIK, no implementation will consider private IPs. The exception I've seen is Apache's mod_remoteip which has a separate CIDR list for internal proxies.

If we want to consider allowing private IPs as being a real client IP I would prefer to gate that behind another setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about we at least call that out in the API docs. I've done my fair share of telneting into localhost for debug so at least we can hopefully avoid surprising anyone that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I another round of consideration. First, I was wrong on

AFAIK, no implementation will consider private IPs

I couldn't remember where I got that from, so I went and checked a few implementations and none of them have a check like this. I suspect I got myself muddled over the general view that private IPs aren't useful (for Internet-facing proxies).

Second, on balance having this check is far too restrictive and makes assumptions about the use cases. As Alyssa alluded to, if we trust a proxy, we should trust that it validated the header.

Therefore, I've removed this block and updated the API docs to remove mention of that restriction.

return {nullptr, false};
}
return {addr, true};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to just return true here or below. I believe the final boolean here is "do we trust the whole chain such that we can say this is an internal request". I don't think we want to equate "we have a chain of trusted cidr addresses" with "we have a trusted chain of internal addresses". I think we should only allow treating the chain as internal iff the direct connected host is internal and every hop is not only cidr-trusted but internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, you're right, this is definitely the wrong thing to do here. And now the more I read on what "internal" means in Envoy the more confused I'm getting 😅. The return in getLastAddressFromXFF is also pretty confusing.

I've read through some history in #2559 and #2590 to try and figure out what the correct answer is here. What should "internal" mean when you're saying "I trust these CIDRs"? Internal doesn't always have to mean RFC1918 or ULA, for example.

To give a concrete example for my use-case for using this feature, I want to add my CDN's ranges, and our own internal proxy IPs (RFC1918), but not the whole internal network; anything else internal should be untrusted.

If Envoy's definition of "internal" is simply "RFC1918 or ULA", perhaps while looping through trusted_cidrs:

  • Set a boolean marking if any CIDR is not internal;
  • On this line I can exchange true with isInternalAddress(addr);
  • On the final line use the bool from point 1

WDYT? I'm still not convinced I've got this entirely right.

Copy link
Contributor

Choose a reason for hiding this comment

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

for "internal" I usually read "sent by someone in my company", like I trust them to hit up /quitquitquit
for "trusted" here that's "trusted to set XFF header"
so unless you trust your CDN to verify "this is actually someone from my company sending traffic through the CDN" I'd say "not internal" :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you think we should mark that with a list of CIDRs? I could split it into "internal_cidrs" and "external_cidrs", which feels a bit clunky but would do the job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've dug through how the internal address config works, and how the detect() method is called on the xff extension, and my understanding is:

  • The bool here is allow_trusted_address_checks, not "do we trust this address":
    // Is the detected address trusted (e.g.: can it be used to determine if this is an internal
    // request).
    bool allow_trusted_address_checks;
  • The allow_trusted_address_checks bool determines whether to compare the address against internal_address_config in HCM:
    // At this point we can determine whether this is an internal or external request. The
    // determination of internal status uses the following:
    // 1) After remote address/XFF appending, the XFF header must contain a *single* address.
    // 2) The single address must be an internal address.
    // 3) If configured to not use remote address, but no XFF header is available, even if the real
    // remote is internal, the request is considered external.
    // HUGE WARNING: The way we do this is not optimal but is how it worked "from the beginning" so
    // we can't change it at this point. In the future we will likely need to add
    // additional inference modes and make this mode legacy.
    const bool internal_request =
    allow_trusted_address_checks && final_remote_address != nullptr &&
    config.internalAddressConfig().isInternalAddress(*final_remote_address);

So given that, I think returning true is the correct thing to do, and then HCM will compare the address returned against its internal address config to really determine if it's internal. Given that, it fits my use-case described above perfectly and there's no need to split the list or do extra checks here.

Does my reasoning seem sound?

}
return {nullptr, false};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :-)

}

Utility::GetLastAddressFromXffInfo
Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers,
uint32_t num_to_skip) {
Expand Down
21 changes: 21 additions & 0 deletions source/common/http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <cstdint>
#include <memory>
#include <string>
#include <vector>

#include "envoy/common/regex.h"
#include "envoy/config/core/v3/http_uri.pb.h"
Expand Down Expand Up @@ -437,6 +438,26 @@ struct GetLastAddressFromXffInfo {
bool allow_trusted_address_checks_;
};

/**
* Checks if the remote address is contained by one of the trusted proxy CIDRs.
* @param remote the remote address
* @param trusted_cidrs the list of CIDRs which are considered trusted proxies
* @return whether the remote address is a trusted proxy
*/
bool remoteAddressIsTrustedProxy(const Envoy::Network::Address::InstanceConstSharedPtr& remote,
const std::vector<Network::Address::CidrRange> trusted_cidrs);

/**
* Retrieves the last address in the x-forwarded-header after removing all trusted proxy addresses.
* @param request_headers supplies the request headers
* @param trusted_cidrs the list of CIDRs which are considered trusted proxies
* @return GetLastAddressFromXffInfo information about the last address in the XFF header.
* @see GetLastAddressFromXffInfo for more information.
*/
GetLastAddressFromXffInfo
getLastNonTrustedAddressFromXFF(const Http::RequestHeaderMap& request_headers,
const std::vector<Network::Address::CidrRange> trusted_cidrs);

/**
* Retrieves the last IPv4/IPv6 address in the x-forwarded-for header.
* @param request_headers supplies the request headers.
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/http/original_ip_detection/xff/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ envoy_cc_library(
deps = [
"//envoy/http:original_ip_detection_interface",
"//source/common/http:utility_lib",
"//source/common/network:cidr_range_lib",
"//source/common/protobuf",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/http/original_ip_detection/xff/v3:pkg_cc_proto",
],
)
Expand All @@ -33,6 +36,7 @@ envoy_cc_extension(
"//envoy/http:original_ip_detection_interface",
"//envoy/registry",
"//source/common/config:utility_lib",
"//source/common/network:cidr_range_lib",
"@envoy_api//envoy/extensions/http/original_ip_detection/xff/v3:pkg_cc_proto",
],
)
25 changes: 23 additions & 2 deletions source/extensions/http/original_ip_detection/xff/xff.cc
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "source/extensions/http/original_ip_detection/xff/xff.h"

#include "source/common/http/utility.h"
#include "source/common/network/cidr_range.h"

namespace Envoy {
namespace Extensions {
Expand All @@ -10,13 +11,33 @@ namespace Xff {

XffIPDetection::XffIPDetection(
const envoy::extensions::http::original_ip_detection::xff::v3::XffConfig& config)
: xff_num_trusted_hops_(config.xff_num_trusted_hops()) {}
: xff_num_trusted_hops_(!config.has_xff_trusted_cidrs() ? config.xff_num_trusted_hops() : 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so you're normalizing here instead of rejecting. So I'd either then ENVOY_LOG a warning that you're ignoring any non-zero configured value of xff_num_trusted_hops or reject the config by throwing an error here. Basically it's annoying to be debugging why setting X to 5 or 6 makes no difference because both 5 and 6 are being ignored due to some other field you weren't looking at resulting in both being discarded. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks!

recurse_(config.xff_trusted_cidrs().recurse().value()) {
xff_trusted_cidrs_.reserve(config.xff_trusted_cidrs().cidrs().size());
for (const envoy::config::core::v3::CidrRange& entry : config.xff_trusted_cidrs().cidrs()) {
Network::Address::CidrRange cidr = Network::Address::CidrRange::create(entry);
xff_trusted_cidrs_.push_back(cidr);
}
}

XffIPDetection::XffIPDetection(uint32_t xff_num_trusted_hops)
: xff_num_trusted_hops_(xff_num_trusted_hops) {}
: xff_num_trusted_hops_(xff_num_trusted_hops), recurse_(false) {}

Envoy::Http::OriginalIPDetectionResult
XffIPDetection::detect(Envoy::Http::OriginalIPDetectionParams& params) {
if (!xff_trusted_cidrs_.empty()) {
if (!Envoy::Http::Utility::remoteAddressIsTrustedProxy(params.downstream_remote_address,
xff_trusted_cidrs_)) {
return {nullptr, false, absl::nullopt};
}
if (recurse_) {
// Check XFF for last IP that isn't in `xff_trusted_cidrs`
auto ret = Envoy::Http::Utility::getLastNonTrustedAddressFromXFF(params.request_headers,
xff_trusted_cidrs_);
return {ret.address_, ret.allow_trusted_address_checks_, absl::nullopt};
}
}

auto ret =
Envoy::Http::Utility::getLastAddressFromXFF(params.request_headers, xff_num_trusted_hops_);
return {ret.address_, ret.allow_trusted_address_checks_, absl::nullopt};
Expand Down
6 changes: 6 additions & 0 deletions source/extensions/http/original_ip_detection/xff/xff.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
#pragma once

#include "envoy/config/core/v3/address.pb.h"
#include "envoy/extensions/http/original_ip_detection/xff/v3/xff.pb.h"
#include "envoy/http/original_ip_detection.h"

#include "source/common/network/cidr_range.h"
#include "source/common/protobuf/protobuf.h"

namespace Envoy {
namespace Extensions {
namespace Http {
Expand All @@ -22,6 +26,8 @@ class XffIPDetection : public Envoy::Http::OriginalIPDetection {

private:
const uint32_t xff_num_trusted_hops_;
std::vector<Network::Address::CidrRange> xff_trusted_cidrs_;
const bool recurse_;
};

} // namespace Xff
Expand Down
4 changes: 4 additions & 0 deletions test/extensions/http/original_ip_detection/xff/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ envoy_extension_cc_test(
extension_names = ["envoy.http.original_ip_detection.xff"],
deps = [
"//source/common/http:utility_lib",
"//source/common/network:address_lib",
"//source/common/protobuf:utility_lib",
"//source/extensions/http/original_ip_detection/xff:xff_lib",
"//test/test_common:utility_lib",
"@envoy_api//envoy/extensions/http/original_ip_detection/xff/v3:pkg_cc_proto",
Expand All @@ -29,6 +31,8 @@ envoy_extension_cc_test(
extension_names = ["envoy.http.original_ip_detection.xff"],
deps = [
"//envoy/registry",
"//source/common/network:address_lib",
"//source/common/protobuf:utility_lib",
"//source/extensions/http/original_ip_detection/xff:config",
"//source/extensions/http/original_ip_detection/xff:xff_lib",
"//test/mocks/server:factory_context_mocks",
Expand Down
Loading