Skip to content
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

[nginx] Client remote address is discarded when using TLS and proxy-protocol #672

Closed
dpratt opened this issue Apr 28, 2017 · 46 comments
Closed

Comments

@dpratt
Copy link
Contributor

dpratt commented Apr 28, 2017

It looks like the nginx ingress controller has recently added some bits that directly handle SNI and proxy-protocol for port 443 in the golang wrapper instead of nginx.

This unfortunately has the side effect of discarding the remote address in nginx and messing up the proxy_add_x_forwarded for directives. From nginx's point of view, all incoming requests that come in over it's SSL port (442) have an origin address of 127.0.0.1. This breaks a lot of stuff for us internally, the least of which is HTTP request logs - every SSL request (which is nearly all of them) always shows up with a blank remote address.

The short term solution would be to have the golang post-SNI tcp proxy also start it's new connections to the backend with a proxy-protocol header.

In the long term, this is likely going to be a problem for SSL passthrough containers as well, since they will see a source IP of the nginx ingress controller - might be a good idea to optionally implement proxy-protocol for L4 backend connections as well.

@arjanschaaf
Copy link

I just ran into this issue as well with our setup using an AWS ELB in TCP mode in front of the NGINX Ingress Controller. 0.9.0-beta.3 works as expected but proxy-protocol support in 0.9.0-beta.4 & 0.9.0-beta.5 is broken.

@dpratt
Copy link
Contributor Author

dpratt commented May 1, 2017

I've submitted #675, which fixes this.

@dpratt
Copy link
Contributor Author

dpratt commented May 11, 2017

What's the usual workflow for closing out issues? Since #675 is merged, it would seem this is resolved.

@aledbf
Copy link
Member

aledbf commented May 11, 2017

@arjanschaaf @dpratt please test if the image quay.io/aledbf/nginx-ingress-controller:0.113 solves the issue

@arjanschaaf
Copy link

@aledbf @dpratt first test result is that all my HTTPS requests are directed to the default-backend somehow. I don't directly see why, need to spend a bit more time to investigate. HTTP requests are directed to the correct backend...

@aledbf
Copy link
Member

aledbf commented May 11, 2017

@arjanschaaf do you see the correct IP in the logs?

@aledbf
Copy link
Member

aledbf commented May 11, 2017

@arjanschaaf 0.113 is just a test image with a fix for that

@arjanschaaf
Copy link

Yes I do see the correct ip address in the log file. So that part has been fixed! I wasn't sure if I should expect this image to be fully functional 😄 Like I stated before: this image isn't functioning correctly in my setup. But it does log the correct ip address!
<my-ip-address> - [<my-ip-address>] - - [12/May/2017:07:08:08 +0000] "GET / HTTP/2.0" 404 138 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36" 12 0.001 [upstream-default-backend] 10.200.56.11:8080 21 0.001 404

FYI: my test setup is AWS based with a single ELB (tcp mode with proxy protocol support) in front of the NGINX Ingress Controller.

@aledbf
Copy link
Member

aledbf commented May 25, 2017

Please update the image to gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.6

@aledbf
Copy link
Member

aledbf commented May 25, 2017

Closing. Please reopen if the issue persists after the upgrdade to 0.9.0-beta.6.

@aledbf aledbf closed this as completed May 25, 2017
@arjanschaaf
Copy link

0.9.0-beta.7 works for me: thanks!

@juliohm1978
Copy link
Contributor

0.9.0-beta.7 really works, but the issue is back for 0.9.0-beta.10. Will someone, please reopen?

@nailgun
Copy link
Contributor

nailgun commented Jul 11, 2017

Confirm

@aledbf aledbf reopened this Jul 11, 2017
@juliohm1978
Copy link
Contributor

juliohm1978 commented Jul 19, 2017

Just a follow up, 0.9.0-beta.7 does not actually pass my real user IP

HEADERS RECEIVED:
Connection=close
Host=lab.tjpr.net
X-Forwarded-For=10.46.0.0
X-Forwarded-Host=lab.tjpr.net
X-Forwarded-Port=443
X-Forwarded-Proto=https
X-Original-URI=/servicoA
X-Real-IP=10.46.0.0
X-Real-IP=10.46.0.0
X-Scheme=https
accept=text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
accept-encoding=gzip, deflate, br
accept-language=en-US,en;q=0.5
cache-control=max-age=0
upgrade-insecure-requests=1
user-agent=Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0

10.46.0.0 is still some internal k8s address.

Also trying 0.9.0-beta.11, the real IP header is back to 127.0.0.1. So, the problem persists.

@dragonsmith
Copy link

+1
I also confirm 0.9.0-beta.11 has this issue. Shoot myself in the leg today after updating to beta.11 because of Nginx CVE.

@juliohm1978
Copy link
Contributor

@dragonsmith

Could you tell me which version you were using that works?

@grantwnorman
Copy link

I can confirm that 0.9.0-beta.7 works, I had to roll back to that version

@juliohm1978
Copy link
Contributor

That's strange. I just tested 0.9.0-beta.7 this week and, even though it's not passing 127.0.0.1, it's still passing some k8s internal IP.

X-Forwarded-For=10.46.0.0
X-Real-IP=10.46.0.0

In my case, it only works setting hostNetwork: true. We deploy the pods as a DaemonSet, and keep them restrained to a group of nodes with apropriate labels.

@nailgun
Copy link
Contributor

nailgun commented Jul 22, 2017

Latest working version is 0.9.0-beta.8

@dragonsmith
Copy link

dragonsmith commented Jul 24, 2017

@juliohm1978 Sorry, was out for a few days.

I can confirm both 0.9.0-beta.7 and 0.9.0-beta.8 work fine in my case (hostNetwork: true and no more balancers in front of them)

10 & 11 are broken.

@n1koo
Copy link

n1koo commented Jul 24, 2017

I was looking in to this myself. What changed after b8 was #890 fixing #885

The somewhat bad thing about #890 is that many people were relying on that funny default for this functionality. But for quick fix just add use-proxy-protocol: "true" to you config. Obviously this assumes you can use proxy-protocol.

$remote_addr on the other hand is always localhost when using HTTPS

The reasoning behind the previous is the 443->442 trick we do for TLS SNI

@aledbf I don't think the remote_addr will ever work for https with this setup. In #890 we probably should have kept the logic for https or did I miss something?

@dragonsmith
Copy link

dragonsmith commented Jul 27, 2017

The reasoning behind the previous is the 443->442 trick we do for TLS SNI

@n1koo Sorry for a little off-topic, but, please, can you point me somewhere where I can read more about this hack? Because, as I understand, Nginx has good TLS SNI support itself and I'm confused why it was implemented here the way it is now. It would be really great if you can. Thank you!

just add use-proxy-protocol: "true"

BTW, in my case, that breaks http support, obviously, as I do not have any load balancers in front of Nignx Ingress controller which can proxy http connections to me via proxy-protocol.

@arjanschaaf
Copy link

I just wanted to confirm that 0.9.0-beta.11 seems to work just as good as 0.9.0-beta.7 in our setup. We have an ELB in TCP mode in front of the k8s cluster and use-proxy-protocol: "true" is added to the configmap.

@juliohm1978
Copy link
Contributor

@arjanschaaf

Could you, please, provide an example of your configuration? Service/Deployment objects you used? I'm interested to compare it with my yaml. I'm also using 0.9.0-beta.11, but I'm still getting an invalid header x-real-ip=10.40.0.0.

@arjanschaaf
Copy link

@juliohm1978 sure! Please note that an important detail in my setup is that I have a classic ELB in front of my NGINX Ingress Controller with proxy-protocol support enabled. Enabling proxy-protocol support isn't possible in the AWS webui: http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html

For the configuration of my ingress controller I have a ConfigMap with some additional configuration which looks like:

{
    "apiVersion": "v1",
    "data": {
        <other config properties>
        "use-proxy-protocol": "true"
    },
    "kind": "ConfigMap",
    "metadata": {
        "name": "nginx-ingress-custom-configuration",
        "namespace": "xxx"
    }
}

The service configuration:

{
  "kind": "Service",
  "apiVersion": "v1",
  "metadata": {
    "name": "nginx-ingress",
    "namespace": "xxx",
    "annotations": {
        "service.beta.kubernetes.io/external-traffic": "OnlyLocal"
    }
  },
  "spec": {
    "ports": [
        {"port": 80, "name": "http", "nodePort": 31111},
        {"port": 443, "name": "https", "nodePort": 31112},
        {"port": 10254, "name": "healthcheck", "nodePort": 31113}],
    "selector": {
      "app": "nginx-ingress-lb"
    },
    "type": "NodePort"
  }
}

port 80 & 443 are exposed by the ELB, the healthcheck port is used by the ELB to determine which nodes in the EC2 autoscaling group actually run a Ingress Controller. The ELB only sents traffic to the nodes which run such a pod.

The Ingress Controller Deployment config:

{
    "kind": "Deployment",
    "metadata": {
        "name": "nginx-ingress-controller",
        "namespace": "xxx",
        "labels": {
            "app": "nginx-ingress-lb"
        }
    },
    "spec": {
        "replicas": 1,
        "selector": {
            "matchLabels": {
                "app": "nginx-ingress-lb"
            }
        },
        "template": {
            "metadata": {
                "labels": {
                    "app": "nginx-ingress-lb"
                }
            },
            "spec": {
                "containers": [
                    {
                        "name": "nginx-ingress-controller",
                        "image": "gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.11",
                        "ports": [
                            {
                                "containerPort": 80
                            },
                            {
                                "containerPort": 443
                            },
                            {
                                "containerPort": 10254
                            }
                        ],
                        "livenessProbe": {
                            "httpGet": {
                                "path": "/healthz",
                                "port": 10254
                            },
                            "initialDelaySeconds": 10,
                            "timeoutSeconds": 5
                        },
                        "readinessProbe": {
                            "httpGet": {
                                "path": "/healthz",
                                "port": 10254
                            }
                        },
                        "args": [
                            "/nginx-ingress-controller",
                            "--apiserver-host=http://xxx.example.com:8080",
                            "--configmap=$(POD_NAMESPACE)/nginx-ingress-custom-configuration",
                            "--default-backend-service=$(POD_NAMESPACE)/default-http-backend",
                            "--default-ssl-certificate=$(POD_NAMESPACE)/tls-cloud-rti-com-certificate"
                        ],
                        "env": [
                            {
                                "name": "POD_NAME",
                                "valueFrom": {"fieldRef": {"fieldPath": "metadata.name"}}
                            },
                            {
                                "name": "POD_NAMESPACE",
                                "valueFrom": {"fieldRef": {"fieldPath": "metadata.namespace"}}
                            }
						],
                        "resources": {
                            "requests": {
                                "cpu": "1",
                                "memory": "2Gi"
                            },
                            "limits": {
                                "cpu": "2",
                                "memory": "2Gi"
                            }

                        },
                        "volumeMounts": [
                            {
                                "name": "tls-dhparam-vol",
                                "mountPath": "/etc/nginx-ssl/dhparam"
                            }
                        ],
                        "imagePullPolicy": "Always"
                    }
                ],
                "volumes": [
                    {
                        "name": "tls-dhparam-vol",
                        "secret": {
                            "secretName": "tls-dhparam"
                        }
                    }
                ],
                "restartPolicy": "Always"
            }
        },
        "strategy": {
            "type": "Recreate"
        }
    }
}

Let me know if this helps out.

@dragonsmith
Copy link

I have a classic ELB in front of my NGINX Ingress Controller with proxy-protocol support enabled

I was also able to "fix" the problem by adding a LB (non proxy-protocol though) in front of Nginx Ingress controller. But it still remains if Nginx Ingress controller is the last frontier.

@juliohm1978
Copy link
Contributor

juliohm1978 commented Aug 7, 2017

@arjanschaaf, Thank you. Sorry for the late response, it's been busy around here.

The main difference I see is that I'm using a ClusterIP Service, instead of NodePort.

apiVersion: v1
kind: Service
metadata:
  name: frontend
spec:
  selector:
    app: frontend.nginx
  type: ClusterIP

  ports:
    - name: http
      port: 80
      targetPort: 80
    - name: https
      port: 443
      targetPort: 443
  externalIPs:
    - x.x.x.x
    - y.y.y.y

I'll admit some ignorance on my part. In the case of ClusterIP Service, clients get NATed into the cluster through the host's externalIPs. Is that the case?

I'm having trouble finding out the correct way to expose ports 443 and 80 to the external world using NodePort. What kind of ELB am I suppsed to setup outside the k8s cluster that understands which hsot ports represent 80 and 443? Can anyone give me an example?

@arjanschaaf
Copy link

@juliohm1978 I'm using a classic ELB which points http traffic coming in on port 80 towards nodePort 31111 and https traffic coming in on 443 towards nodeport 31112.
That classic ELB is coupled with an autoscaling group which I use for the kubernetes nodes (minions). So in this basic setup the ELB will think that every node in the autoscaling group will have a running NGINX Controller, which is more or less correct as Kubernetes will direct any requests coming in on any node for a NodePort towards the node actually running the service. So this will work perfectly fine although it will introduce a bit of overhead when you have more nodes running then the nr of Ingress controllers because of the potential redirect in k8s. In order to prevent that you have to configure "service.beta.kubernetes.io/external-traffic": "OnlyLocal" annotation in the service, like I did in the example configuration. Using this trick the ELB healthcheck will fail on any node NOT running a Ingress Controller and it will be successful on node that is running a healthy Ingress Controller pod. That way the ELB is able to only send requests to the relevant nodes and thus prevent unnecessary NodePort related redirects.

@arjanschaaf
Copy link

arjanschaaf commented Aug 10, 2017

I was also able to "fix" the problem by adding a LB (non proxy-protocol though) in front of Nginx Ingress controller.

@dragonsmith are you sure about this?
I just disabled proxy-protocol support on my ELB using the method described here: http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-proxy-protocol.html#proxy-protocol-disable-policy-cli
Once proxy-protocol was disabled on the ELB (which is the default) I saw a local Flannel network IP-address in the X-Forwarded-For header instead of the ip-address of my browser client that made the request.

@karanthukral
Copy link

I have been looking into this too. We currently run on GKE and after the change in #890 we lost the ability to access the real client-ip. Enabling use-proxy-protocol: "true" doesn't work for us since we don't use proxy-protocol.

We can get the old behaviour back by simply using proxy_protocol_addr instead of remote_addr which basically reverts #890.

@aledbf since #890 didn’t have too much information attached to it, would you be able to provide more insight into the change? In its current state, without proxy-protocol enabled, the client-ip on https will just default to localhost.

@aledbf
Copy link
Member

aledbf commented Aug 10, 2017

@arjanschaaf @dragonsmith when we create a service type=Loadbalancer by default the ports are TCP and that's the reason why we don't see the real source IP.
Using proxy-protocol fixes this. I understand this is not an option in GCE/GKE.

In AWS the solution is to add additional annotations to the service:

kind: Service
apiVersion: v1
metadata:
  name: ingress-nginx
  labels:
    k8s-addon: ingress-nginx.addons.k8s.io
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "http"
    # Map port 443
    service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "https"
spec:
  type: LoadBalancer
  selector:
    app: ingress-nginx
  ports:
  - name: http
    port: 80
    targetPort: http
  - name: https
    port: 443
    targetPort: https

@arjanschaaf
Copy link

In AWS the solution is to add additional annotations to the service:

@aledbf not sure if I understand your solution for AWS. Will this lead to an ELB being created by the k8s LoadBalancer service with HTTP based ports instead of TCP based ports?

Personally I bypass the creation of ELB instances by the k8s LoadBalancer services. I created our ELB "by hand" with TCP based ports and proxy-protocol enabled and coupled it with a simple NodePort based k8s service.

@aledbf
Copy link
Member

aledbf commented Aug 11, 2017

I created our ELB "by hand" with TCP based ports and proxy-protocol enabled and coupled it with a simple NodePort based k8s service.

That works too :)
If you manually create the elb and use HTTP for both ports the service receives the X-Forwarded-For header and you not need to enable proxy-protocol.

@arjanschaaf
Copy link

@aledbf one of the disadvantages of using ELB in HTTP mode instead of TCP is that you need to configure your SSL certificate on the ELB. Where we currently use the NGINX ingress controller for the SSL offloading. It all depends on your specific situation of course but we currently use multiple wildcard certificates for different domains simultaneously. This works perfectly in one Ingress Controller + one ELB in TCP mode but when I would have to facilitate this with Amazon ELB I would need to create an ELB for every SSL certificate I need to deploy because ELB only supports one SSL certificate per ELB instance. So in our situation we would need create many ELB's and incur the additional costs (and hassle).

@rolftimmermans
Copy link

This is still a problem for people using HTTPS and a TCP load balancer. I have described the details that I have found in my own testing in #1067 (comment)

@fredriklindberg
Copy link

Wouldn't it be possible to skip the 442/443 port hack for the nginx controller and just use ssl_preread instead?

@aledbf
Copy link
Member

aledbf commented Aug 14, 2017

Wouldn't it be possible to skip the 442/443 port hack for the nginx controller and just use ssl_preread instead?

We tried that without success.

@YouriT
Copy link

YouriT commented Aug 18, 2017

I confirm that @arjanschaaf suggestion fixes the issue in beta.11 by adding "use-proxy-protocol": "true" to the configmap, the redirect from http -> https makes the IP address juste fine.

Note: I'm using it on GKE with a LoadBalancer service and a static IP.

@dragonsmith
Copy link

dragonsmith commented Aug 28, 2017

are you sure about this?

@arjanschaaf Sorry, I was inaccurate in my terms. In my case, there is no AWS at all - I'm running clusters on DigitalOcean & bare-metal. So adding one more Nginx as an LB in front of Nginx Ingress Controller solves the problem as it can set X-Forwarded-For header for us. But this solution seems a bit crippled to me (let's install one more Nginx in front of Nginx).

@aledbf The problem still presents even if there is no Service in front of the pods with Nginx Ingress Controller (when they are the last bastion and open their ports straight to the internet).

Sorry again, I've concentrated too much on my end of the problem and did not mention clearly I'm not talking in an AWS context.

@aledbf
Copy link
Member

aledbf commented Aug 28, 2017

Closing. Current master code disables ssl-passthrough feature (behind a flag now) and by default it's disabled.

@aledbf aledbf closed this as completed Aug 28, 2017
@aledbf
Copy link
Member

aledbf commented Aug 28, 2017

@dragonsmith please open a new issues describing the issue you have. Please include the k8s version, where you are running the cluster and how to reproduce the issue

@dragonsmith
Copy link

@aledbf I'll test beta-12 release first. At first glance, it seems ssl-passthrough should resolve my problem. Thank you very much!

@Karthickarumugam
Copy link

Karthickarumugam commented Mar 5, 2018

I have set use-proxy-protocol: "true" and in the log-format i have "remote_addr": "$proxy_protocol_addr" in the ingress controller version gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.15 inside GKE kubernetes. But i am not seeing client IP and it shows only private IP. Could you help here?

GKE cluster version is 1.9.2

@YouriT
Copy link

YouriT commented Mar 6, 2018

@Karthickarumugam checkout the quay repo and upgrade that's already quite old version you have there.

@Karthickarumugam
Copy link

Karthickarumugam commented Mar 6, 2018

Thanks @YouriT
I found alternative way to capture the IP via service. That does the trick. Because service was hitting the pod, hence we were getting private IP.

externalTrafficPolicy: Local

@gustavomc
Copy link

hi @Karthickarumugam how could you get it to work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests