nginxinc / kubernetes-ingress

NGINX and NGINX Plus Ingress Controllers for Kubernetes
https://docs.nginx.com/nginx-ingress-controller
Apache License 2.0
4.63k stars 1.96k forks source link

HTTP > HTTPS not working with NLB with TLS termination and Proxy Protocol V2 #1209

Open sgasquet opened 3 years ago

sgasquet commented 3 years ago

Describe the bug Hi there. We are facing a weird issue regarding HTTPS redirection with this ingress on this context:

To Reproduce Our configuration in values.yaml file :

 controller:
    kind: daemonset
    config:
      entries:
        hsts: "True"
        proxy-protocol: "True"
        real-ip-header: proxy_protocol
        redirect-to-https: "True"
        server-tokens: "False"
        set-real-ip-from: 0.0.0.0/0
    service:
      type: LoadBalancer
      externalTrafficPolicy: Cluster
      annotations:
        service.beta.kubernetes.io/aws-load-balancer-type: nlb
        service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: "*"
        service.beta.kubernetes.io/aws-load-balancer-backend-protocol: http
        service.beta.kubernetes.io/aws-load-balancer-ssl-cert-ports: https
        service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"
        service.beta.kubernetes.io/aws-load-balancer-ssl-negotiation-policy: "ELBSecurityPolicy-TLS-1-2-2017-01"
        service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arn:aws:acm:xxxxxxxxxxxxxxxxxx
      httpPort:
        enable: true
        port: 80
        targetPort: http
      httpsPort:
        enable: true
        port: 443
        targetPort: http

Expected behavior In ingress of a web backend application, HTTPS redirect should occur with this kind of annotations : nginx.org/redirect-to-https ingress.kubernetes.io/ssl-redirect: "True" But none of this works actually.

Debug To debug this behavior, we played with tcpdump inside a POD and see what Headers are saw. First thing, the header X-FORWARDED-PROTO is always the same, reaching the endpoint in HTTP or HTTPS won't change that. And still no redirection is done. Here are the headers we can see : X-Real-IP: X.X.X.X OK X-Forwarded-For: X.X.X.X OK X-Forwarded-Host: host.blabla OK X-Forwarded-Port: 80 (always) X-Forwarded-Proto: HTTPS (always)

So all forwarded headers are good but not X-Forwarded-Port and X-Forwarded-Proto which never change.

Is anything we are doing wrong in the configuration ? Is it related to the ProxyProtocol which forwards the protocol defined here "aws-load-balancer-backend-protocol: http" and not the real protocol used by client ? Any help appreciated :)

Thanks for your help !

Aha! Link: https://nginx.aha.io/features/IC-103

pleshakov commented 3 years ago

Hi @sgasquet

It should be possible to support an SSL redirect based on $proxy_protocol_server_port variable, assuming that when NLB terminates SSL, the value is 443, and 80 otherwise.

Would it be possible if you try use a custom Ingress template for the Ingress Controller with the following change and let us know if it works?

    if ($http_x_forwarded_proto = 'http') {
        return 301 https://$host$request_uri;
    }

->

    if ($proxy_protocol_server_port != '443') {
        return 301 https://$host$request_uri;
    }

In the ConfigMap, ssl-redirect should be "false" and redirect-to-https - "true".

sgasquet commented 3 years ago

Hey @pleshakov !

It's working !! The second problem you solve in two days :D. So the fix in my helm configuration / values.yaml is "simply":

    ingress-template: |
        {{- if $server.RedirectToHTTPS}}
        if ($proxy_protocol_server_port != '443') {
          return 301 https://$host$request_uri;
        }
        {{- end}}

Now everything is redirected smoothly thanks again for your help ! As TLS termination & Proxy Protocol V2 are very new with NLB i guess this will be useful for others soon or later.

Have a nice day !

sgasquet commented 3 years ago

Hi again !

So ! The settings are working only on reload with nginx, which keeps the entire nginx.ingress template in memory. But after reboot the entire template file is reduced to the short code described in my last post (and obviously Nginx wont start).

Is there a way to have this incremental change in config ? Or do we need to apply the whole NGINX INGRESS TEMPLATE file ?

Thanks again for your help :).

pleshakov commented 3 years ago

Hi @sgasquet

Yep, the whole template needs to be applied. Unfortunately, it is not possible to modify a part of the template.

sgasquet commented 3 years ago

Thanks for your help :)

niraj1234567890 commented 3 years ago

@sgasquet Hey I am facing the same issue as you were facing on the redirection from http to https where I am making ssl termination at the load balancer level. I tried to add the custom ingress template in the config-map as seen below:

apiVersion: networking.k8s.io/v1beta1
kind: IngressClass
metadata:
  name: nginx
spec:
  controller: nginx.org/ingress-controller
---
kind: ConfigMap
apiVersion: v1
metadata:
  name: nginx-config
  namespace: nginx-ingress
data:
  proxy-protocol: "True"
  real-ip-header: "proxy_protocol"
  set-real-ip-from: "0.0.0.0/0"
  redirect-to-https: "True"
  ingress-template: |
        {{- if $server.RedirectToHTTPS}}
        if ($proxy_protocol_server_port != '443') {
          return 301 https://$host$request_uri;
        }
        {{- end}}

My other config's are same as your's (BUT not using helm-chart). When I am implementing the above configmap yaml, I am getting this error in the logs: Error: variable server is not defiend I suppose there must be something on the custom ingress template I am missing. Can you please suggest if there is anything you can think of on the above error.

FYI: Http and Https, both are working fine.

pleshakov commented 3 years ago

@niraj1234567890 this is not obvious, but the entire template needs to be added in the ingress-template, not just the excerpt.

niraj1234567890 commented 3 years ago

@pleshakov It worked. It came out that I was not passing the entire nginx-ingress.teml file into the configmap which l did it eventually and now it's working like a charm.

pleshakov commented 3 years ago

reopening this issue, as a proper way to support this should not require a workaround, but rather a built-in feature

pleshakov commented 3 years ago

a full workaround example is mentioned in this comment -- https://github.com/nginxinc/kubernetes-ingress/issues/1250#issuecomment-798940775

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

rnicholas-saatva commented 3 years ago

Is there any plan to fix this issue? Ideally don't want to have to have a values file with a full config in it.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

suroh1994 commented 2 years ago

Are there any news on this end? We're running into the same issue right now and the referenced workaround seems quite daunting to be honest...

rojspencer commented 1 year ago

I was able to get this working without adding a full template to the configMap. Section of values file passed to helm chart for nginx-stabe:

controller:
  ...
  config:
    entries:
      proxy-connect-timeout: "10s"
      proxy-read-timeout: "10s"
      client-max-body-size: "2m"
      proxy-protocol: "True"
      real-ip-header: "proxy_protocol"
      set-real-ip-from: "0.0.0.0/0"
      server-snippets: |
        if ($proxy_protocol_server_port != '443') {
          return 301 https://$host$request_uri;
        }
rojspencer commented 1 year ago

I'm sorry, but I don't see a fix for this issue in that release. #2993 appears to be only for when ssl passthrough is enabled, which isn't in this case.

I'm running 2.4.2 with TLS terminated on a NLB using proxy protocol 2 and cannot get http redirect to work without the work-around I posted.

jasonwilliams14 commented 1 year ago

@rojspencer I'm sorry, my mistake. I was thinking of another item we addressed in 2.4.0. I will review this week.

dheerajappin commented 1 year ago

Hi,

can any one help me on this issue, i have tried everything mentioned above but its not working

shaun-nx commented 2 weeks ago

Hi folks 👋 We are looking into re-evaluating the value, priority and effort of this request.

We encourage everyone to keep an eye on the existing Epic Roadmap and Prioritized Backlog for the project.

I am currently in the process of grooming our existing issues, starting with the oldest ones. As you look at the backlog now, know that it is very likely to change in the near future. Some issues may be added, and some removed 🙂

shaun-nx commented 2 weeks ago

Re-opening. It looks like our automated process closed it since we had originally closed the issue in one of our iterations