Sanitize and/or provide useful X-Forwarded-For header

[PiotrSikora]

Describe the bug
X-Forwarded-For header isn't sanitized before being forwarded by ingress gateway, which means that malicious client can easily spoof its source address by sending X-Forwarded-For header with fake IP address, and while the fake IP address won't be used by ingress gateway (because of the use_remote_address: true setting), it might be accepted by workloads trusting ingress to pass client's IP using this header.

Note that the presence of spoofed X-Forwarded-For header results in either X-Envoy-Internal or X-Envoy-External-Address being forwarded to the workload.

Furthermore, even without spoofing, both X-Forwarded-For and X-Envoy-External-Address headers contain IP address of kube-proxy (e.g. 10.128.0.5) and not client's IP, rendering it pretty useless.

External request:

curl -v -H"Host: httpbin.example.com" http://$INGRESS_HOST:$INGRESS_PORT/get?show_env=1

< HTTP/1.1 200 OK
< server: envoy
< date: Tue, 07 Aug 2018 00:29:34 GMT
< content-type: application/json
< access-control-allow-origin: *
< access-control-allow-credentials: true
< content-length: 534
< x-envoy-upstream-service-time: 6
<
{
  "args": {
    "show_env": "1"
  },
  "headers": {
    "Accept": "*/*",
    "Content-Length": "0",
    "Host": "httpbin.example.com",
    "User-Agent": "curl/7.60.0",
    "X-B3-Sampled": "1",
    "X-B3-Spanid": "f318863530d246b3",
    "X-B3-Traceid": "f318863530d246b3",
    "X-Envoy-Internal": "true",
    "X-Forwarded-For": "10.128.0.5",
    "X-Forwarded-Proto": "http",
    "X-Request-Id": "a53a017d-dc07-9784-a4ec-5957a1915800"
  },
  "origin": "10.128.0.5",
  "url": "http://httpbin.example.com/get?show_env=1"
}

External request with spoofed X-Forwarded-For header:

curl -v -H"Host: httpbin.example.com" -H"X-Forwarded-For: 1.2.3.4" http://$INGRESS_HOST:$INGRESS_PORT/get?show_env=1

< HTTP/1.1 200 OK
< server: envoy
< date: Tue, 07 Aug 2018 00:29:50 GMT
< content-type: application/json
< access-control-allow-origin: *
< access-control-allow-credentials: true
< content-length: 566
< x-envoy-upstream-service-time: 13
<
{
  "args": {
    "show_env": "1"
  },
  "headers": {
    "Accept": "*/*",
    "Content-Length": "0",
    "Host": "httpbin.example.com",
    "User-Agent": "curl/7.60.0",
    "X-B3-Sampled": "1",
    "X-B3-Spanid": "384e7075880e36c8",
    "X-B3-Traceid": "384e7075880e36c8",
    "X-Envoy-External-Address": "10.128.0.5",
    "X-Forwarded-For": "1.2.3.4, 10.128.0.5",
    "X-Forwarded-Proto": "http",
    "X-Request-Id": "38bab7c1-5691-9825-ba76-a3a654b83842"
  },
  "origin": "1.2.3.4, 10.128.0.5",
  "url": "http://httpbin.example.com/get?show_env=1"
}

Expected behavior
X-Forwarded-For header should contain valid client's IP, and ingress gateway should sanitize and/or strip untrusted values before forwarding requests to backend services.

Note that we should account for trusted load balancers being deployed in front of ingress gateway and forwarding valid client's IP using this header.

Steps to reproduce the bug
Deploy sample httpbin service and expose it via ingress gateway from the official guide and issue curl commands mentioned above.

Version

$ istioctl version
Version: 1.0.0
GitRevision: 3a136c90ec5e308f236e0d7ebb5c4c5e405217f4
User: root@71a9470ea93c
Hub: gcr.io/istio-release
GolangVersion: go1.10.1
BuildStatus: Clean

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.1", GitCommit:"b1b29978270dc22fecc592ac55d903350454310a", GitTreeState:"clean", BuildDate:"2018-07-17T18:53:20Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"10+", GitVersion:"v1.10.5-gke.3", GitCommit:"6265b9797fc8680c8395abeab12c1e3bad14069a", GitTreeState:"clean", BuildDate:"2018-07-19T23:02:51Z", GoVersion:"go1.9.3b4", Compiler:"gc", Platform:"linux/amd64"}

Is Istio Auth enabled or not?
n/a

Environment
GKE

cc @louiscryan @costinm @rshriram

[kyessenov]

Ingress gateways should run with externalTrafficPolicy set to Local (see #7328)

[PiotrSikora]

@kyessenov Good find, thanks! Indeed, changing externalTrafficPolicy from Cluster to Local results in real client IP being passed via X-Forwarded-For header. This is also what ingress-nginx seems to be using in order to preserve client IP.

That leaves us with sanitization that won't break X-Forwarded-For passed by load balancers.

[mandarjog]

It depends on which lbs are in the front of ingress. If there is a trusted lb like gcp lb or elb is in front, then you want the left most address set by in xff, if the fronting lb already sanitizes. this should be configurable by deployment.
Is there a way to configure trusted downstream in envoy?

[rshriram]

There is a load balancer source ip range in kube. Not sure if that helps. Else we can look into ip tagging filter or add a source subnet field in the filter chain match so that we can enable it via api (users specify source ip ranges in the gateway spec).

[rshriram]

I think source subnet match is easiest way forward.

[PiotrSikora]

@mandarjog you can configure number of trusted proxies (xff_num_trusted_hops), which, while not perfect, works for the "behind cloud load balancer" use case.

[exiaohao]

@kyessenov
I've changed externalTrafficPolicy from Cluster to Local, Ingressgateway seems stopped working. envoy is still listening port but the port doesn't response any request.

my istio-ingressgateway config

apiVersion: v1
kind: Service
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Service","metadata":{"annotations":{},"labels":{"app":"istio-ingressgateway","chart":"gateways-1.0.0","heritage":"Tiller","istio":"ingressgateway","release":"istio"},"name":"istio-ingressgateway","namespace":"istio-system"},"spec":{"ports":[{"name":"http2","nodePort":31380,"port":80,"targetPort":80},{"name":"https","nodePort":31390,"port":443},{"name":"tcp","nodePort":31400,"port":31400},{"name":"tcp-pilot-grpc-tls","port":15011,"targetPort":15011},{"name":"tcp-citadel-grpc-tls","port":8060,"targetPort":8060},{"name":"http2-prometheus","port":15030,"targetPort":15030},{"name":"http2-grafana","port":15031,"targetPort":15031}],"selector":{"app":"istio-ingressgateway","istio":"ingressgateway"},"type":"LoadBalancer"}}
  creationTimestamp: 2018-08-09T09:07:09Z
  labels:
    app: istio-ingressgateway
    chart: gateways-1.0.0
    heritage: Tiller
    istio: ingressgateway
    release: istio
  name: istio-ingressgateway
  namespace: istio-system
  resourceVersion: "1430477"
  selfLink: /api/v1/namespaces/istio-system/services/istio-ingressgateway
  uid: 98cb3523-9bb3-11e8-91cb-000c29469214
spec:
  clusterIP: 172.21.188.124
  externalIPs:
  - 10.7.0.194
  externalTrafficPolicy: Cluster
  ports:
  - name: http2
    nodePort: 31380
    port: 80
    protocol: TCP
    targetPort: 80
  - name: https
    nodePort: 31390
    port: 443
    protocol: TCP
    targetPort: 443
  - name: tcp
    nodePort: 31400
    port: 31400
    protocol: TCP
    targetPort: 31400
  - name: tcp-pilot-grpc-tls
    nodePort: 30833
    port: 15011
    protocol: TCP
    targetPort: 15011
  - name: tcp-citadel-grpc-tls
    nodePort: 32514
    port: 8060
    protocol: TCP
    targetPort: 8060
  - name: http2-prometheus
    nodePort: 30431
    port: 15030
    protocol: TCP
    targetPort: 15030
  - name: http2-grafana
    nodePort: 31506
    port: 15031
    protocol: TCP
    targetPort: 15031
  selector:
    app: istio-ingressgateway
    istio: ingressgateway
  sessionAffinity: None
  type: LoadBalancer
status:
  loadBalancer: {}

[andraxylia]

We can add a helm deployment option in the istio-ingressgateway proxy, where the user can specify if the gateway is an edge proxy, or it is located behind a trusted load balancer. This will get passed via proxy node Metadata to Pilot, which will generate the correct Envoy config with the correct combination of use_remote_address and xff_num_trusted_hops as described here:

https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/headers#config-http-conn-man-headers-x-forwarded-for

Let me know if you see issues with this approach for 1.0.1.

[duderino]

@rshriram's proposal to filter on source IP with a CIDR or similar would handle cases where an ingress gateway wants to strip an xff header from some but not all of its clients. The current approach is all or nothing - the ingress gateway trusts all of its clients or none of its clients.

Is that flexible enough?

[andraxylia]

@duderino Envoy does not have support for filtering x-forwarded-for , it would need to be added.

What are the best practices in the industry to prevent spoofing and how do other proxies handle it? I wonder why Envoy went with the num_hops solely. @PiotrSikora

[andraxylia]

filtering based on IP is discussed here also as a possible fix: cloudfoundry/gorouter#179

[rkpagadala]

@PiotrSikora We want to cut a build on Friday, what is the status of this bug

[merlineus]

Should this work with AWS ELB?
I've added externalTrafficPolicy: Local to istio-ingressgateway service but can see only the balancer's IP in nginx logs.

[andraxylia]

@rkpagadala I am working on this as per this proposal: #7679 (comment)

[PiotrSikora]

@andraxylia I thought the plan for 1.0.1 was to only strip X-Forwarded-For, since we don't have externalTrafficPolicy: Local merged yet (see: #7918), so the info there is pretty useless.

[andraxylia]

@PiotrSikora the agreement was to have a flag for edge proxy, on by default, and to strip only for edge proxies. This way, when externalTrafficPolicy is fixed, we don't need to revisit this issue anymore, it will just work.

[Dagon-]

@exiaohao Did you find a fix for this? I'm seeing the same thing.

[osswangxining]

@exiaohao Did you find a fix for this? I'm seeing the same thing.

You can specify the parameter gateways.istio-ingressgateway.externalTrafficPolicy to Local if using Helm to deploy.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[tx19980520]

@kyessenov
@exiaohao Have you solved the problems? Or maybe other params need change.
Or change the param in Helm?

[tx19980520]

@exiaohao Did you find a fix for this? I'm seeing the same thing.

You can specify the parameter gateways.istio-ingressgateway.externalTrafficPolicy to Local if using Helm to deploy.

@osswangxining
I upgrade from 1.1 => 1.2.5 by using helm template and changed the externalTrafficPolicy to Local but the ingressgateway is not working. kubectl edit svc istio-ingressgateway -n istio-system change it to local it works well.
I just wanna get a real client IP to have a dynamic blacklist to limit rate by IP.

[haddadianhamed1]

for anyone who had the issue with setting externalTrafficPolicy from Cluster to Local you would need to make sure istio-ingress is running on the node you are connecting to. for example in my case I had 4 ingress gateway when I changed externalTrafficPolicy on the service from Cluster to Local I couldnt get access to my internal containers. so what I had to do was add podaffinity to make sure istio-ingressgateway is running on the hosts that traffic is going to. when you are changing Cluster to Local it basically stops kubernetes service to proxy traffic to another host. for example if you have host A and B and you are accessing host B but istio-ingressgateway is running on host A changing externalTrafficPolicy Cluster to Local would break the traffic. that was the issue I had.

or change ingressgateway to run as DaemonSet

      - preference:

          matchExpressions:
          - key: kubernetes.io/network
            operator: In
            values:
            - public
        weight: 2

[PiotrSikora]

cc @nrjpoddar @costinm

[incfly]

/cc @incfly

[howardjohn]

Does #23292 fix this or is it just a part of it?

[Arpeet-gupta]

https://blog.opstree.com/2020/04/07/preserve-source-ip-in-aws-classic-load-balancer-and-istios-envoy-using-proxy-protocol/

[erSitzt]

for anyone who had the issue with setting externalTrafficPolicy from Cluster to Local you would need to make sure istio-ingress is running on the node you are connecting to. for example in my case I had 4 ingress gateway when I changed externalTrafficPolicy on the service from Cluster to Local I couldnt get access to my internal containers. so what I had to do was add podaffinity to make sure istio-ingressgateway is running on the hosts that traffic is going to. when you are changing Cluster to Local it basically stops kubernetes service to proxy traffic to another host. for example if you have host A and B and you are accessing host B but istio-ingressgateway is running on host A changing externalTrafficPolicy Cluster to Local would break the traffic. that was the issue I had.

or change ingressgateway to run as DaemonSet

      - preference:

          matchExpressions:
          - key: kubernetes.io/network
            operator: In
            values:
            - public
        weight: 2

Is there a way to configure the ingress-gateway to run on all nodes via the helm values ?
I manually scaled it up to run on all nodes, but an update of istio in Rancher reverted my changes... and it took me some time to remember this problem.

Whats the correct way to do this ?

[nrjpoddar]

Does #23292 fix this or is it just a part of it?

Envoy doesn't sanitize XFF headers, you can only set which XFF to use and the trusted proxies in front is supposed to sanitize if needed. The Gateway topology API allows users to easily specify which XFF to choose but doesn't guarantee sanitization.

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-07-27. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.