Add x-forwarded-host to the nginx config in the deis-router for ruby apps issue

[dmcnaught]

Here are descriptions of the issue that we see in our Ruby apps after adding
router.deis.io/nginx.useProxyProtocol: "true"

https://stackoverflow.com/a/51111144/2112497
rails/rails#22965

Env:
K8s 1.12.9, workflow-v2.20.0
We are terminating our SSL certs on the AWS ELB in front of the deis-router.

[dmcnaught]

Thread from teamhephy slack channel:

We’d like to add x-forwarded-host to the nginx config in the deis-router. Doesn’t look this this is currently supported.
https://stackoverflow.com/a/51111144/2112497
rails/rails#22965

cryptophobia 7 days ago
Should be possible. This seems to be a header set per application, right?

cryptophobia 7 days ago
Let me know if you need help with this but should be a few lines to add to the deis-router configuration as an annotation on each application deployment.

duncanmcnaught 7 days ago
I’m not sure how to add custom nginx.conf settings. I think I just want to add this line proxy_set_header X-Forwarded-Host $host;
@Cryptophobia (edited)

duncanmcnaught 7 days ago
I thought you could just add the settings listed: https://github.com/teamhephy/router (edited)

duncanmcnaught 7 days ago
Do you have time to help @Cryptophobia?

cryptophobia 7 days ago
We will need to make a PR and make it configurable?

cryptophobia 7 days ago
I can take a look sometime tomorrow. Do you want this proxy_set_header to be set default for every app or to be configurable?

duncanmcnaught 7 days ago
I’m not sure if we would be ok just adding X-Forwarded-Host and X-Forwarded-Ssl to router.deis.io/nginx.useProxyProtocol (https://github.com/teamhephy/router#use-proxy-protocol) - I’m not sure why they weren’t put there before. Do you know @felixbuenemann? (edited)

cryptophobia 7 days ago
@felixbuenemann should know more about this.

cryptophobia 6 days ago
From what I am reading @duncanmcnaught, it looks like X-Forwarded-Ssl is an extra header that does not always need to be proxied to the backend to make HTTPS requests work. For example look at this issue in Gitlab source code: https://gitlab.com/gitlab-org/gitlab-ce/issues/32937 (edited)

duncanmcnaught 6 days ago
Right - we’re thinking that the X-Forwarded-Host will be enough to fix our ruby apps having the problem linked to start this thread

duncanmcnaught 6 days ago
Do you think we can get a fix in @Cryptophobia?

cryptophobia 6 days ago
Sure, all we need to add is this header if proxyProtocol is set to true, correct?

cryptophobia 6 days ago
I can attempt a PR, should only be a one/two line change. Would need you to test with a router image to verify it works.
👌

cryptophobia 6 days ago
I will have the PR ready tom morning and docker image tag for testing.
👍

duncanmcnaught 6 days ago
Great. Thanks!

cryptophobia 5 days ago
Hey @duncanmcnaught, looks like the X-Forwarded-Host header has a privacy concern that I do not want to enable by default.

cryptophobia 5 days ago
This header is used for debugging, statistics, and generating location-dependent content and by design it exposes privacy sensitive information, such as the IP address of the client. Therefore the user's privacy must be kept in mind when deploying this header.

cryptophobia 5 days ago
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host (edited)

cryptophobia 5 days ago
We should only be concerned with the protocol selection HTTP vs HTTPS. This is likely related to how you have configured your rails application. I will let @felixbuenemann chime in but I don't see a reason to add this header for the above privacy concerns.

cryptophobia 5 days ago
It's clear in the code that useProxyProtocol is not dealing with setting any X-Forwarded-For headers or X-Forwarded-Hosts:
{{ if $routerConfig.UseProxyProtocol -}}
real_ip_header proxy_protocol;
{{- else -}}
real_ip_header X-Forwarded-For;
{{- end }}

cryptophobia 5 days ago
This is the exact behavior from the nginx-ingress config builder. I'm not sure we should deviate from the exact behavior of the nginx-ingress k8s controller just to satisfy some Rails app requirements.

cryptophobia 5 days ago
https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl#L127-L134

cryptophobia 5 days ago
https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#forwarded-for-header

duncanmcnaught 5 days ago
Maybe we can set it as a separate config option - so only rails app users would turn it on? @Cryptophobia

cryptophobia 5 days ago
We could add as a separate config option annotation but this would still deviate from nginx-controller best practices.

duncanmcnaught 5 days ago
It seems like the ingress-nginx for k8s does allow X-Forwarded-Host: https://github.com/kubernetes/ingress-nginx/search?l=Markdown&q=X-Forwarded-Host&type=Code @Cryptophobia

duncanmcnaught 5 days ago
https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl#L1192

duncanmcnaught 5 days ago
With respect to the privacy concerns - we are wanting to set the ProxyProto on the deis router so we can get the client IP, that’s the whole point of our change.

duncanmcnaught 4 days ago
What are your current thoughts on adding this setting to workflow @Cryptophobia?

[felixbuenemann]

I’m currently on vacation and can’t check, but isn’t the original Host header already kept intact?

[felixbuenemann]

I've checked the config and we already have proxy_set_header Host $host; in there, so setting X-Forwarded-Host $host in addition to that doesn't really make sense.

Is it possible that the requests are proxied by another app in your cluster that modifies the Host header?

[dmcnaught]

I don't think so @felixbuenemann , we have AWS ELB -> deis-router -> rails apps
We are terminating certs on the ELB in environments we have tested in.
Why don't we just continue the discussion here - seems we are posting both here and in slack 😄

[felixbuenemann]

Terminating TLS on ELB is problematic, since NGINX can no longer detect if the connection is using HTTPS or HTTP, which is likely the cause of your problem.

Theoretically this could be worked around by inspecting the destination port of the PROXY protocol header, but NGINX does not expose a variable for it ($proxy_protocol_port is the source port).

You can save yourself a lot if trouble by switching to TCP mode on the ELB.

[felixbuenemann]

See also https://trac.nginx.org/nginx/ticket/1206 which has a possible workaround:

There could be a flag in the router to signal external TLS termination and disable the ssl and http2 directives on the ports, that way you could forward TLS connections to these fake HTTPS ports and the router could identify HTTP and HTTPS based on the listening port that accepted the connection.

Your always loosing HTTP/2 Support if your terminating TLS on the ELB, which is another big argument against doing it.

[dmcnaught]

I don't really understand how I would apply the workaround you mention above @felixbuenemann - can you elaborate?

[felixbuenemann]

It would require extending the router code that generates the Nginx config and adding annotations to switch the behavior.

[kingdonb]

Do you have a reference that I can quote regarding the HTTP/2 support on terminating TLS at ELB?

My group is terminating TLS at the ELB and I think we did not know about this limitation. This seems like a decent reason to think about not doing it, as you say. I just didn't see anything about this in the link you posted. Is this an issue with ELBs only, or can ALBs still support the termination with HTTP/2 underneath?

[felixbuenemann]

@kingdonb You cannot handle HTTP/2 when termination TLS on an ELB (or NLB), because it requires ALPN negotiation for HTTP/2 on the ELB and even if that would work, NGINX has no idea what was negotiated.

It's possible to have a TLS terminating load balancer like HAproxy, that can negotiate ALPN and send the traffic to different ports on the origin, one port that handles plain HTTP/1 and one port that handles plain HTTP/2. However this would require a custom NGINX config for the Hephy Router.

ALB on the other hand is a Level 7 load balancer, so it does not support either TCP mode or TLS termination.

ALB does support HTTP/2, so it could be used, but you loose all the certificate management provided by Hephy, since you will have to manually manage certificates through ACM on the ALB load balancer.

[kingdonb]

Thanks for sharing. I had read that ALBs do support HTTP/2 but this is quite a bit more than I had understood before.

[Cryptophobia]

Your always loosing HTTP/2 Support if your terminating TLS on the ELB, which is another big argument against doing it.

I can attest to this. This is a problem that is not very well documented and had to solve this problem a couple of times.