PreDecorationFilter overrides X-Forwarded-Host

[smilasek]

Hi,

Today I have encountered the following issue:
We have an application running behind multiple reverse-proxies. Here is a simple schema:

Client => RP1 => RP2 => Zuul => App

RP1 - reverseproxy.com
RP2 - reverseproxy2.com

Client
===> RP1 (x-forwarded-host: reverseproxy.com)
===> RP2 (x-forwarded-host: reverseproxy.com, reverseproxy2.com)
===> Zuul (x-forwarded-host: reverseproxy2.com)
===> App

I have found that PreDecorationFilter is replacing x-forwarded-host instead checking whether it exists before. Is that a proper behaviour? As far as I understand X-Forwarded-Host is original host header (RP1 hostname) not the previous one.
Below snippet of code reponsible for it:
https://github.com/spring-cloud/spring-cloud-netflix/blob/master/spring-cloud-netflix-core/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java#L101-L102

[dsyer]

I can see why that might be inconvenient, but the Zuul filter only replaces the x-forwarded-host header with what it finds in the request, so if the app is configured normally (for any app behind a proxy) it should pick the right value. There's a section in the Spring Boot user guide about how to configure an app for use behind a proxy (TL;DR set server.use-forward-headers=true).

[smilasek]

Yes, you're right. Zuul always replaces x-forwarded-host with servername:
ctx.addZuulRequestHeader("X-Forwarded-Host",ctx.getRequest().getServerName());
Maybe I am wrong, but as far as I know if request goes throught multiple reverse proxies X-forwarded-Host should keep always at least original server name (usually other reverse proxies just append server names to the end as in RP2 example).

My app is deployed to PCF (by default server.use-forward-headers=true) and I have an issues generating hateoas links. It has some simple configuration without any extraordinary things so I assume it should pick the right value. But from my investigation I have found that hateoas is using x-forwarded-host to build proper link:
https://github.com/spring-projects/spring-hateoas/blob/master/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java#L237

All requests coming to my app are with x-forwarded-host: zuul-address.com instead of reverseproxy.com. so I assume that the issue comes from zuul itself ;/

[spencergibb]

so should we be appending to the list if it already exists?

[dsyer]

No, I don't think so: X-Forwarded-Hist is single valued. Maybe one of the other proxies is messed up, or maybe your Zuul gateway does not have `session.use-forward-headers=true"?

[spencergibb]

I ask because Spring Hateoas treats it like it can be a comma separated list https://github.com/spring-projects/spring-hateoas/blob/master/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilder.java#L237-L243

[smilasek]

From apache mod-proxy documentation:
https://httpd.apache.org/docs/2.4/mod/mod_proxy.html#x-headers

Be careful when using these headers on the origin server, since they will contain more than one (comma-separated) value if the original request already contained one of these headers

So I suppose that it should be comma separated list. Another thing as you mentioned is that Hateoas also treats it as list.

[dsyer]

I've never seen x-forwarded-host multi valued, but anyway, the container should be dealing with it for us so we shouldn't have to care. All we care about is that the servlet request has the right host info, and that's not in our hands AFAIK.

[avarabyeu]

Have the same problem. PreDecorationFilter just overrides header from previous reverse proxy server. I had to disable adding proxy headers using zuul.add-proxy-headers=false, but i believe filter should add one more X-Forwarded-Host header

[odrotbohm]

Would there be anything wrong if the filter only set the header if they weren't already present?

[stefanocke]

@dsyer Not only Spring Hateoas, but Spring in general supports multi valued X-Forwarded-Host header. See for instance https://jira.spring.io/browse/SPR-11140 .
So I think, Zuul could / should support it as well.
I don't understand why you say you should not have to care, since as far as I understand @smilasek (#959 (comment)) , you always overwrite the Header with a single value instead of appending to it and there is nothing the container could do to prevent this.

The same applies for X-Forwarded-Port and X-Forwarded-Proto.

[dsyer]

I think when I said you didn't have to care it was from the point of view of the downstream (and I was wrong anyway). The simplest fix would be to only set the header if it is not already present. An equivalent workaround with no changes would be to set zuul.add-proxy-headers=false.

[dsyer]

Judging by the implementation of UriComponentsBuilder there is considerable variety in the behaviour of existing proxies. Appending to a comma-separated list is probably too simple in general. E.g. the X-Forwarded-Host often contains the port as well. If it does then maybe we should strip the X-Forwarded-Port header? Or maybe re-write the X-Forwarded-Host so it doesn't embed the ports? Or neither.

[dsyer]

P.S. There's another, more complete workaround in that duplicate #1286 (a custom filter).

[stefanocke]

Thanks for the workarounds and for reconsidering.

[dsyer]

Fixed in a38b7b7 (but see comment there about the "Forwarded" header, which is recognized by UriComponentsBuilder, but not able to convey port information).