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] whitelist-source-range doesn’t work on ssl port #727

Closed
cheungpat opened this issue May 17, 2017 · 11 comments · Fixed by #740
Closed

[nginx] whitelist-source-range doesn’t work on ssl port #727

cheungpat opened this issue May 17, 2017 · 11 comments · Fixed by #740

Comments

@cheungpat
Copy link
Contributor

cheungpat commented May 17, 2017

It appears that the setting whitelist-source-range on an ingress does not work as expected on the 443 port but it does work on the 80 port.

The ingress controller is running in GKE with Kubernetes 1.6.2. The ingress service has the service.beta.kubernetes.io/external-traffic annotation set to OnlyLocal.

I have an ingress that looks like this:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    ingress.kubernetes.io/ssl-redirect: "false"
    ingress.kubernetes.io/whitelist-source-range: 192.30.252.0/22,185.199.108.0/22
  name: web-server
  namespace: default
spec:
  rules:
  - host: www.example.com
    http:
      paths:
      - backend:
          serviceName: web-server
          servicePort: 80
        path: /
  tls:
  - hosts:
    - www.example.com

curl https://www.example.com (https) doesn't work no matter the source IP is in the white-listed range or not. (My IP is 1.2.3.4 here)

nginx-ingress-controller-31897685-5ljx5 nginx-ingress-lb 2017/05/17 15:35:15 [error] 227#227: *11 access forbidden by rule, client: 127.0.0.1, server: www.example.com, request: "GET / HTTP/2.0", host: "www.example.com"
nginx-ingress-controller-31897685-5ljx5 nginx-ingress-lb 1.2.3.4 - [1.2.3.4] - - [17/May/2017:15:35:15 +0000] "GET / HTTP/2.0" 403 234 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/603.2.4 (KHTML, like Gecko) Version/10.1.1 Safari/603.2.4" 177 0.000 [web-server-80] - - - -

Note that the client IP address becomes the 127.0.0.1, which is not expected.

curl http://www.example.com (http) works as expected. If outside the white-listed range, nginx is able to identify the client IP address:

nginx-ingress-controller-31897685-5ljx5 nginx-ingress-lb 2017/05/17 15:38:23 [error] 306#306: *19 access forbidden by rule, client: 1.2.3.4, server: www.example.com, request: "GET / HTTP/1.1", host: "www.example.com"
nginx-ingress-controller-31897685-5ljx5 nginx-ingress-lb 1.2.3.4 - [1.2.3.4] - - [17/May/2017:15:38:23 +0000] "GET / HTTP/1.1" 403 170 "-" "curl/7.52.1" 98 0.000 [web-server-80] - - - -

I am not sure why nginx is able to identify the correct client IP address in the request log, but it doesn’t use this client IP to determine if the request should be allowed.

I think this is related to #614

@aledbf
Copy link
Member

aledbf commented May 17, 2017

@cheungpat this is fixed in master. Please use the image quay.io/aledbf/nginx-ingress-controller:0.116

@aledbf aledbf closed this as completed May 17, 2017
@cheungpat
Copy link
Contributor Author

@aledbf Thanks for the quick reply! It is also nice to know that there is a image that is frequently updated. : )

I updated the ingress controller to use the image quay.io/aledbf/nginx-ingress-controller:0.116 but the problem still persist.

The same error is printed in the log:

nginx-ingress-controller-3378685672-fch2x nginx-ingress-lb 2017/05/18 01:02:09 [error] 68#68: *2 access forbidden by rule, client: 127.0.0.1, server: www.example.com, request: "GET / HTTP/2.0", host: "www.example.com"

Perhaps it is my config that is causing problem? Here is my ingress controller deployment and configmap:

# deployment
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    k8s-app: nginx-ingress-lb
  name: nginx-ingress-controller
  namespace: kube-system
spec:
  selector:
    matchLabels:
      k8s-app: nginx-ingress-lb
  template:
    metadata:
      labels:
        k8s-app: nginx-ingress-lb
    spec:
      containers:
      - args:
        - /nginx-ingress-controller
        - --default-backend-service=$(POD_NAMESPACE)/default-http-backend
        - --default-ssl-certificate=$(POD_NAMESPACE)/ssl-cert
        - --configmap=$(POD_NAMESPACE)/nginx-configmap
        - --publish-service=$(POD_NAMESPACE)/nginx-ingress-controller
        env:
        - name: POD_NAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.name
        - name: POD_NAMESPACE
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
        image: quay.io/aledbf/nginx-ingress-controller:0.116
        imagePullPolicy: IfNotPresent
        name: nginx-ingress-lb
        ports:
        - containerPort: 80
          hostPort: 80
          protocol: TCP
        - containerPort: 443
          hostPort: 443
          protocol: TCP
---
# configmap
apiVersion: v1
data:
  body-size: 50m
  hsts: "false"
kind: ConfigMap
metadata:
  name: nginx-configmap
  namespace: kube-system

@aledbf
Copy link
Member

aledbf commented May 18, 2017

@cheungpat please publish the service nginx-ingress-controller.
I am not sure the behavior of the annotation service.beta.kubernetes.io/external-traffic with the nginx ingress controller.

@aledbf aledbf reopened this May 18, 2017
@cheungpat
Copy link
Contributor Author

cheungpat commented May 18, 2017

The behavior of the service.beta.kubernetes.io/external-traffic annotation is described here: https://kubernetes.io/docs/tutorials/services/source-ip/

In short, the load balancer will only forward traffic to the node that has the pod running in order to preserve source IP.

The service manifest looks like this:

apiVersion: v1
kind: Service
metadata:
  annotations:
    service.beta.kubernetes.io/external-traffic: OnlyLocal
    service.beta.kubernetes.io/healthcheck-nodeport: "31557"
  name: nginx-ingress-controller
  namespace: kube-system
spec:
  clusterIP: 10.7.241.1
  ports:
  - name: http
    nodePort: 30427
    port: 80
    protocol: TCP
    targetPort: 80
  - name: https
    nodePort: 30077
    port: 443
    protocol: TCP
    targetPort: 443
  selector:
    k8s-app: nginx-ingress-lb
  sessionAffinity: None
  type: LoadBalancer

@sigxcpu76
Copy link

sigxcpu76 commented May 19, 2017

The issue is the forward from 443 to 442 that sees the source IP as "127.0.0.1". This was solved in beta3 (by not enabling that forward if there is no specific rule that requires it and keeping 443 as SSL) and now it is broken again (just tested beta.4 and there is no way to disable the 443->442 thing.

Here's the commit that broke it again:
de14e2f#diff-b7803798d356c6c17a90d93cc58bdbaa

@aledbf
Copy link
Member

aledbf commented May 19, 2017

@sigxcpu76 right and that is why I am refactoring the template to use the real IP address (that you can see in the log) and not the configured by set_real_ip_from where is not possible to reflect the redirect between ports

Edit: I am sorry for the troubles this issue introduces but the ssl passthrough feature is something we really need and make it work in all the different use cases is not an easy task.

@sigxcpu76
Copy link

sigxcpu76 commented May 19, 2017

So this means anything above beta.3 is not working? The issue is that appRoot is a nice feature and it is not in beta.3

@cheungpat
Copy link
Contributor Author

I am speculating that the nginx config is set to real_ip_header X-Forwarded-For; but the traffic sent to 442 port is actually using proxy protocol, so nginx does not get the real IP.

Perhaps we can set real_ip_header proxy_protocol and have HTTP traffic go through the golang proxy as well?

@aledbf
Copy link
Member

aledbf commented May 20, 2017

@cheungpat is not possible to set the value in real_ip_header, what I am testing is the use of a nginx map.

@aledbf
Copy link
Member

aledbf commented May 20, 2017

@cheungpat @sigxcpu76 please check if the image quay.io/aledbf/nginx-ingress-controller:0.118 solves the issue

@cheungpat
Copy link
Contributor Author

@aledbf It works!

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

Successfully merging a pull request may close this issue.

3 participants