Open cskarby opened 5 months ago
arkodg
commented
5 months ago this makes sense, the API can live under ClientIPDetection https://github.com/envoyproxy/gateway/blob/0abdda7a033f0e37647177a588a904c90b783149/api/v1alpha1/clienttrafficpolicy_types.go#L220 @cskarby the workarounds are to use EnvoyPatchPolicy or update bootstrap
cc @zhaohuabing @davidalger
zhaohuabing
commented
5 months ago BTW, looks like it's only configurable at the HCM level, so probably can't use update bootstrap.
cskarby
commented
5 months ago I'm new to this project, and not so familiar with envoy configuration, however I hope my draft could be useful. Just let me know if it is not... Probably the API should be bumped when adding new fields? And further we probably should add some tests
arkodg
commented
5 months ago @cskarby there may be two ways of representing this in the API
1) allow private subnet IPs to be populated in XFF - this can be represented as a bool *AllowPrivateIPs and internally EG can go ahead and configure 10.0.0.0/8,........ in InternalAddressConfig in Envoy
2) only populate IPs from these trusted CIDRs in XFF - this can be represented as []string CIDR which can be implemented once we have https://github.com/envoyproxy/envoy/pull/31831
wdyt @envoyproxy/gateway-maintainers
cskarby
commented
5 months ago @cskarby there may be two ways of representing this in the API
- allow private subnet IPs to be populated in XFF - this can be represented as a
bool *AllowPrivateIPsand internally EG can go ahead and configure10.0.0.0/8,........in InternalAddressConfig in Envoy- only populate IPs from these trusted CIDRs in XFF - this can be represented as
[]string CIDRwhich can be implemented once we have xff: add support for configuring a list of trusted CIDRs envoy#31831wdyt @envoyproxy/gateway-maintainers
Thank you for your quick response! Personally I like the second option best, see proposal below, but it diverges from the defaults in vanilla envoy. The first option is already the default behavior, but actually I need a way to not treat these addresses as internally trusted sources. To override it we would need to set some other addresses, what about localhost?
Actually my problem is different to that of envoy#31831, which is about trusting existing XFF headers. I would like envoy-gateway to set XFF headers so that I can see the client IP in a new XFF header when doing upstream processing. I have no proxy between the client and envoy-gateway, so there is no legitimate requests originating with an existing XFF header.
Envoy will only append to XFF if the
use_remote_addressHTTP connection manager option is set totrueand theskip_xff_appendis setfalse. This means that ifuse_remote_addressisfalse(which is the default) orskip_xff_appendistrue, the connection manager operates in a transparent mode where it does not modify XFF.
Even when use_remote_address is true, Envoy flags incoming requests from RFC1918 ranges (10.0.0.0/8, 172.16.0.0/12 and 192.168.0.0/16) and the RFC4193 range (fc00::/7) as internal.
From A few very important notes about XFF:
- XFF is what Envoy uses to determine whether a request is internal origin or external origin. If
use_remote_addressis set totrue, the request is internal if and only if the request contains no XFF and the immediate downstream node’s connection to Envoy has an internal (RFC1918 or RFC4193) source address. [...]
So in my case I need to set use_remote_address =true, but then the request from any private IP range that comes without an existing XFF will be flagged as internal. This is not desired for two reasons:
Regarding the first point in your feedback: Envoy already considers connections from these addresses as internal, unless overridden by explicit configuration. The configuration API is already in envoy, but not envoy-gateway. This is why we need a way to configure it in envoy-gateway.
Perhaps the default for envoy-gateway should be to treat all IP addresses as untrusted? I believe we could achive this by default setting HttpConnectionManager.InternalAddressConfig to 127.0.0.0/8 and ::1/128 - then only requests from localhost ranges are considered internal by default. I think it is more intuitive and a safer default, but it would diverge from vanilla envoy, which could cause confusion for experienced envoy admins.
Later when xff: add support for configuring a list of trusted CIDRs envoy#31831 is merged, we could extend the API to look something like this (which I believe is in line with what you write in your feedback):
apiVersion: gateway.envoyproxy.io/v1alpha2-proposal
kind: ClientTrafficPolicy
metadata:
name: enable-tcp-keepalive-policy
namespace: default
spec:
targetRef:
group: gateway.networking.k8s.io
kind: Gateway
name: eg
namespace: default
clientIPDetection:
xForwardedFor:
xff_trusted_cidrs:
- 10.0.10.0/24
- 192.88.99.128/30When we have a matching ClientTrafficPolicy, we could also add all ranges from xff_trusted_cidrs to HttpConnectionManager.InternalAddressConfig in additon to the localhost ranges? (If it is still needed after xff: add support for configuring a list of trusted CIDRs envoy#31831?)
zhaohuabing
commented
5 months ago Actually my problem is different to that of envoy#31831, which is about trusting existing XFF headers. I would like envoy-gateway to set XFF headers so that I can see the client IP in a new XFF header when doing upstream processing. I have no proxy between the client and envoy-gateway, so there is no legitimate requests originating with an existing XFF header.
@cskarby Looks like whether the IP range is internal or not doesn't impact Envoy's decision on appending XXF header. Of course I may miss something here - Envoy's configuration around XXF is a bit messy.
if (config.useRemoteAddress()) {
....
if (!config.skipXffAppend()) {
if (Network::Utility::isLoopbackAddress(
*connection.connectionInfoProvider().remoteAddress())) {
Utility::appendXff(request_headers, config.localAddress());
} else {
Utility::appendXff(request_headers, *connection.connectionInfoProvider().remoteAddress());
}
}
...
}Indeed not so easy to navigate, so I just tested a minimal diff to see if it makes a difference
diff --git a/internal/xds/translator/listener.go b/internal/xds/translator/listener.go
index 427cca26..bff7e0eb 100644
--- a/internal/xds/translator/listener.go
+++ b/internal/xds/translator/listener.go
@@ -205,6 +205,10 @@ func (t *Translator) addXdsHTTPFilterChain(xdsListener *listenerv3.Listener, irL
if originalIPDetectionExtensions != nil {
useRemoteAddress = false
}
+
+ var localhostCidrRanges []*corev3.CidrRange
+ localhostCidrRanges = append(localhostCidrRanges, &corev3.CidrRange{AddressPrefix: "127.0.0.1", PrefixLen: &wrapperspb.UInt32Value{Value: 8}})
+ localhostCidrRanges = append(localhostCidrRanges, &corev3.CidrRange{AddressPrefix: "::1", PrefixLen: &wrapperspb.UInt32Value{Value: 128}})
mgr := &hcmv3.HttpConnectionManager{
AccessLog: al,
@@ -225,6 +229,7 @@ func (t *Translator) addXdsHTTPFilterChain(xdsListener *listenerv3.Listener, irL
Http2ProtocolOptions: http2ProtocolOptions(),
// https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_conn_man/headers#x-forwarded-for
UseRemoteAddress: &wrappers.BoolValue{Value: useRemoteAddress},
+ InternalAddressConfig: &hcmv3.HttpConnectionManager_InternalAddressConfig{UnixSockets: true, CidrRanges: localhostCidrRanges},
XffNumTrustedHops: xffNumTrustedHops(irListener.ClientIPDetection),
OriginalIpDetectionExtensions: originalIPDetectionExtensions,
// normalize paths according to RFC 3986and the patch actually modifies the behaviour of envoy when it comes to sending the XFF header upstream. I will investigate further...
Description The http request header
x-forwarded-for(XFF) is not beeing set when the request is origination from ip-ranges that envoy detects as internal. By default any request from private ip ranges (RFC1918 / RFC4193) are considered internal. Envoy's list of internal CIDR ranges can be configured with the internal address configuration in the http connection manager.When running the envoy gateway in an internal network, where the client will connect directly to the envoy-gateway without network address translation from a private ip addresses, it would still be interesting to know the client's IP address. When the XFF header is not set, the backend will only have the ip address of the proxying gateway, and not the actual client.
Please provide guidance for how to configure the envoy-gateway with custom CIDR ranges to consider as internal. Ideally it should be possible to set this via helm values, or at least via relevant kubernetes configuration objects. In the meantime I would also be interested in ways to configure this via low level interfaces, e.g. overriding the bootstrap configuration or any other way, if that is possible?
Relevant Links