Allow multiple headers to be used for ForwardedRequestCustomizer

[joakime]

There are environments where the server can be hit by legacy proxies, and new proxies, with possibly different request headers for things like the remote-address and the scheme (https)

This is a feature request to allow ForwardedRequestCustomizer to utilize multiple headers (search order should be important) for each individual possible value.

[joakime]

One wrinkle is if the header value is different.

Eg:

Standard: X-Forwarded-Proto: https
Legacy 1: X-Proxied-Https: on
Legacy 2: X-Https-Terminated: true

[gregw]

We should also implement rfc7239

[joakime]

How would the Servlet API's HttpServletRequest.getRemoteAddr() work if RFC7239's Obfuscated form is used?

https://tools.ietf.org/html/rfc7239#section-6.3

Example from RFC: Forwarded: for=_hidden, for=_SEVKISEK

[sbordet]

@gregw if you're working on this, please look also at #185.

[gregw]

Currently planning to ignore _hidden and unknown headers

[gregw]

I have implemented #185 RFC7239. I also implemented legacy support for either X-Proxied-Https or X-Https-Terminated if configured.

I have not implemented a fallback between legacy headers, as that can be implemented by adding multiple request customizers in the order you wish to check.

[sbordet]

@gregw then #185 can be closed too ?

[gregw]

I've reopened as I want to consider if multiple customizers is really the right solution. What if we have both sets of headers?

[joakime]

Perhaps we establish a search order, and javadoc it.

Proposal: RFC7239 -> Old/Standard Proxy Headers -> Custom Proxy Headers

[gregw]

Closing again. I've added some tests, but I'm not going to add any more ordering than there already is. Currently it has RFC7239 -> OtherHeaders, where the other headers can be defacto standard or anything you want to set. It's too much to come up with something that will search 3 deep.

You can always have 2 customizers and the second will overwrite the first, if your really need 2 or more legacy headers.

[sbordet]

@gregw, what's your take on using the new headers ?

For example, AbstractProxyServlet adds X-Forwarded-XXX, should it also add the Forwarded header ?

And in all the places we reference X-Forwarded-XXX should we try Forwarded first and then fallback to X-Forwarded-XXX ? For example in AbstractNCSARequestLog ?

[gregw]

@sbordet Oh I didn't know we were using X-Forwarded from AbstractNCSARequestLog - that _preferProxiedForAddress option should be deprecated - if you want to log the forwarded addresses, then the ForwardedRequestCustomizer should be used and the normal APIs will report the forwarded addresses.

So all server side usage should hang off the ForwardedRequestCustomizer.

Client/proxy side is more difficult. If we are forwarding to ourselves, we should default to using the RFC7239 header. But we may forward to all sorts of different servers and I think that RFC7239 takeup appears moderately low. So I think we need to have configuration between at least RFC7239 and the defacto standard headers. Perhaps this needs to be fully configurable?

I note that our proxy servlet does add the defactor standard headers, but the middleman servlet does not. We should probably develop some configurable common logic and use for both.

I'll create a new issue to track this.

[sbordet]

Reopening to track the changes required to AbstractNCSARequestLog.

[joakime]

Closing this as we have a more specific issue for both the Proxy / Middleman #842 feature set and AbstractNCSARequestLog #1052