-
Notifications
You must be signed in to change notification settings - Fork 5.1k
xff: add support for configuring a list of trusted CIDRs #31831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
70893bc
37f82b8
5f390fd
831057e
b32b10d
b37899d
34e7d33
61ade56
0cf5c6c
4c0ff3f
d9f1a64
5842f38
2c95c9f
a016a6c
eb2f7ff
1f76584
994d4be
ebb1391
d976354
666757e
4193f48
a587a23
06eccf5
87bf560
2c15af4
901335a
6833c32
26756eb
6671ece
5ed4660
c69df1c
eab825a
9cfe9d6
774cacc
5c2cbf2
e4ff49e
e2fdae2
9a625aa
0267706
3dd64cc
a9432b4
bfaa8b3
1055b01
7721581
837cd5f
a80178e
aec2db5
57baab7
7344aa8
277fefd
91903c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,10 @@ syntax = "proto3"; | |
|
|
||
| package envoy.extensions.http.original_ip_detection.xff.v3; | ||
|
|
||
| import "envoy/config/core/v3/address.proto"; | ||
|
|
||
| import "udpa/annotations/status.proto"; | ||
| import "validate/validate.proto"; | ||
|
|
||
| option java_package = "io.envoyproxy.envoy.extensions.http.original_ip_detection.xff.v3"; | ||
| option java_outer_classname = "XffProto"; | ||
|
|
@@ -17,10 +20,42 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |
| // | ||
| // [#extension: envoy.http.original_ip_detection.xff] | ||
| message XffConfig { | ||
| // 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 = 1; | ||
| oneof xff_trusted_config { | ||
| option (validate.required) = 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 = 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. | ||
| // | ||
| // This is typically used when requests are proxied by a | ||
| // `CDN <https://en.wikipedia.org/wiki/Content_delivery_network>`_. | ||
| XffTrustedCidrs xff_trusted_cidrs = 2; | ||
| } | ||
| } | ||
|
|
||
| message XffTrustedCidrs { | ||
mattklein123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // 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. | ||
| bool recurse = 2; | ||
jamesog marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||
|
||||||
| detection extension instead. | |
| detection option instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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" | ||||||||||||||||||||||||||||||||
|
|
@@ -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; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| const auto xff_header = request_headers.ForwardedFor(); | ||||||||||||||||||||||||||||||||
| if (xff_header == nullptr) { | ||||||||||||||||||||||||||||||||
| return {nullptr, false}; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| absl::string_view xff_string(xff_header->value().getStringView()); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| 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)); | ||||||||||||||||||||||||||||||||
alyssawilk marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||
| if (addr == nullptr) { | ||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| bool trusted = false; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| 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. | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| return {nullptr, false}; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return {addr, true}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| // Is the detected address trusted (e.g.: can it be used to determine if this is an internal | |
| // request). | |
| bool allow_trusted_address_checks; |
allow_trusted_address_checks bool determines whether to compare the address against internal_address_config in HCM: envoy/source/common/http/conn_manager_utility.cc
Lines 217 to 228 in ac42a62
| // 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?
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :-)
Uh oh!
There was an error while loading. Please reload this page.