proxy header handling

[smancke]

I had to add the X-Forwarded-Proto header to get fabio working as reverse proxy for the docker-registry with tls. While doing this, I made a more broaden overwork on the proxy headers.

So, here is a proposal for additional handling of proxy headers:

1. I changed the Host header for outgoing calls to reflect the host of the target service. This is e.g. essential if the target host serves different virtual hosts on the same ip.
2. I added the X-Real-Ip, X-Forwarded-Host, X-Forwarded-Proto headers, as they are the widely used.
3. I added the host attribute to Forwarded header.
4. I removed the X-Forwaded-For header in the addHeaders method, because the golang reverse proxy already adds this header and we should not double the entries. (This changes the existing behavior, since the LocalIp is no longer part in the X-Forwaded-For, now.)

Please let me know, what you think about this. If you have any remarks on this, feel free to formulate your wishes and I will overwork the pull request.

(I'm also thinking about an integration test for the headers, which show the combined result of the addHeaders() and the Golang Reverse Proxy - but I was not sure, if you would like tests which test more than one unit.)

[magiconair]

Hi @smancke,

Thanks for working on this. A couple of comments on the style:

In general, it might be better if you separate refactorings (like the change in the test structure) from additions. Also, I think that splitting the addHeader() function up into smaller pieces does not make a lot of sense here since you're not adding a lot of code. I'd prefer if you leave the style for both the addHeader() method and the tests the way it was and just add your code and test cases. This makes it easier for me to review the change.

Also:

• Please use IP and URL in uppercase consistently in the variable names and remove extra blank lines
• Please check all errors also in tests (see commit 61f6883)
• Since you have already added a comment to the test case in the table driven test I would prefer if you wouldn't split up the table into smaller segments. Also, please stick to the if got, want := x, y; got != want {...} idiom where possible. Consistency is key. Don't call it expected once and want somewhere else. Since I prefer that you revert to the previous style consider this a general guideline.
• Please check the guidelines on documenting Go code for godoc, i.e. that the comment should start with the function name
• Please sort the imports as the rest of the files: stdlib, blank line, fabio libs, blank line, rest

Can you please explain which behavior regarding X-Forwarded-For changes exactly (provide an example) since breaking existing applications is a big no-no. This may need to become configurable with the current behavior as a default.

[smancke]

Hi @magiconair,

ok - thanks for your comments.
I'll close this pull request and do the overwork in a clean branch with a new pull request

The current behaviour for the X-Forwarded-For has in my opinion two errors, leading to strange behaviour:

1. The LocalIP has nothing to do in X-Forwarded-For. If needed, it could be placed in a X-Forwarded-Server.
2. The double implementation of addHeaders and the go Proxy brings some errors.

I have collected the cases, how it is and how it should behave. In my opinion, it's nothing worth to preserve the current behavior by a configuration flag, since it currently is to buggy and strange.

case A:

config.Proxy{LocalIP: "", ClientIPHeader: ""})
req.RemoteAddr = "2.2.2.2:..." 
X-Forwarded-For: 2.2.2.2
--> Correct, but not expected when I read read the documentation of fabio.properties

case B:

config.Proxy{LocalIP: "", ClientIPHeader: "X-Forwarded-For"})
req.RemoteAddr = "2.2.2.2:..." 
X-Forwarded-For: 2.2.2.2, 2.2.2.2
--> Wrong: Doubled
--> Should be 2.2.2.2

case C:

config.Proxy{LocalIP: "1.1.1.1", ClientIPHeader: "X-Forwarded-For"})
req.RemoteAddr = "2.2.2.2:..." 
X-Forwarded-For: 2.2.2.2, 1.1.1.1, 2.2.2.2
--> Doubled and mixed up with the local ip 
--> Should be  2.2.2.2

case D:

config.Proxy{LocalIP: "1.1.1.1", ClientIPHeader: ""})
req.RemoteAddr = "2.2.2.2:..." 
req.Header.Add("X-Forwarded-For", "3.3.3.3")
X-Forwarded-For:  3.3.3.3, 1.1.1.1, 2.2.2.2
-->  mixed up with the local ip 
--> Should be  3.3.3.3, 2.2.2.2

case E:

config.Proxy{LocalIP: "1.1.1.1", ClientIPHeader: "X-Forwarded-For"})
req.RemoteAddr = "2.2.2.2:..." 
req.Header.Add("X-Forwarded-For", "3.3.3.3")
X-Forwarded-For:  2.2.2.2, 1.1.1.1, 2.2.2.2
-->  mixed up with the local ip, and the original client address got los 
--> Should be  3.3.3.3, 2.2.2.2

[magiconair]

Hmm, I've implemented this in #10 after reading this: https://en.wikipedia.org/wiki/X-Forwarded-For. According to that article the format is X-Forwarded-For: client, proxy1, proxy2 which is the reason I've put the proxy.localIP there. The Forwarded header should have a by parameter with the proxy ip if I understand this correctly. So what am I missing?

[smancke]

Here is an example. Lets think about the following case of proxy servers:

browser -> nginx -> apache -> fabio -> tomcat-app

Then fabio would see:
X-Forwarded-For: browser-ip, nginx-ip
req.RemoteAddr: apache-ip

And fabio has to add the apache-ip to the X-Forwarded-For list.

So that tomcat would see:
X-Forwarded-For: browser-ip, nginx-ip, apache-ip
RemoteAddr: fabio-ip

So, the LocalIP of fabio is send to the tomcat as RemoteAddr, not forwarded for.

[magiconair]

Right, the proxy list is the list of the previous proxies. OK, makes sense. Thx for clarifying.