Skip to content

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

New issue
New issue
Closed
Closed
Sanitize and/or provide useful X-Forwarded-For header#7679
Assignees
andraxylia
Labels
area/networkinglifecycle/automatically-closedIndicates a PR or issue that has been closed automatically.lifecycle/staleIndicates a PR or issue hasn't been manipulated by an Istio team member for a while
Milestone
 
Nebulous Future
@PiotrSikora

Description

@PiotrSikora
PiotrSikora
opened on Aug 7, 2018
Contributor

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

Activity

kyessenov

kyessenov commented on Aug 7, 2018

@kyessenov
kyessenov
on Aug 7, 2018
Contributor

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

PiotrSikora

PiotrSikora commented on Aug 7, 2018

@PiotrSikora
PiotrSikora
on Aug 7, 2018 · edited by PiotrSikora
ContributorAuthor

@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

mandarjog commented on Aug 7, 2018

@mandarjog
mandarjog
on Aug 7, 2018
Contributor

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

rshriram commented on Aug 8, 2018

@rshriram
rshriram
on Aug 8, 2018
Member

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

rshriram commented on Aug 8, 2018

@rshriram
rshriram
on Aug 8, 2018
Member

I think source subnet match is easiest way forward.

PiotrSikora

PiotrSikora commented on Aug 8, 2018

@PiotrSikora
PiotrSikora
on Aug 8, 2018
ContributorAuthor

@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

exiaohao commented on Aug 9, 2018

@exiaohao
exiaohao
on Aug 9, 2018
Contributor

@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
self-assigned this
on Aug 14, 2018
andraxylia
added
area/networking
on Aug 14, 2018
andraxylia
added this to the 1.0 milestone on Aug 14, 2018
andraxylia

andraxylia commented on Aug 14, 2018

@andraxylia
andraxylia
on Aug 14, 2018
Contributor

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

duderino commented on Aug 14, 2018

@duderino
duderino
on Aug 14, 2018
Contributor

@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

andraxylia commented on Aug 14, 2018

@andraxylia
andraxylia
on Aug 14, 2018
Contributor

@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

andraxylia commented on Aug 14, 2018

@andraxylia
andraxylia
on Aug 14, 2018
Contributor

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

rkpagadala

rkpagadala commented on Aug 15, 2018

@rkpagadala
rkpagadala
on Aug 15, 2018
Contributor

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

65 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area/networkinglifecycle/automatically-closedIndicates a PR or issue that has been closed automatically.lifecycle/staleIndicates a PR or issue hasn't been manipulated by an Istio team member for a while

Type

No type

Projects

Prioritization

Status

Done

Milestone

Relationships

None yet

    Development

    No branches or pull requests

      Participants

      @rlenglet@PiotrSikora@dermyrddin@howardjohn@mandarjog

      Issue actions
        Sanitize and/or provide useful X-Forwarded-For header · Issue #7679 · istio/istio