Add behind proxy middleware

[veryrusty]

Collects the request header munging that was being done when building the Request object into a Plack middleware (wraps existing Plack middlewares for request munging behind proxies).

Simplifies the request object (before tackling #589) and adds support for reverse proxies from non-root paths (#571). Devs can (conditionally) apply the middleware (via Plack::Builder) to provide more flexibility than was previously possible (eg BehindProxy -> MultiLang middleware-> Dancer app ).

I'm not 100% convinced that behind_proxy should be a global config item though - yell if you have opinions :D

[xsawyerx]

This is a whole lot. Can we have a discussion about it first? I like some ideas here, but others scare me, to be honest.

[veryrusty]

The Pr was to initiate that discussion. Have I opened a can of worms? ;)

[xsawyerx]

So a rough assessment from me:

• Generally it makes sense.
• Using Plack middlewares is a good direction.
• Plack::Middleware::ReverseProxyPath has a bad name and terrible documentation.
• Commit https://github.com/PerlDancer/Dancer2/commit/fea1cd0271cff0006d030d96ce8506637b23d62b helps understand it better.
• Re: #571: I don't like HTTP_REQUEST_BASE.
• If we're writing a middleware, put it under Plack::Middleware, not Dancer2::Middleware. It leads to a slope that I'm not sure we want to take.
• Are any of these header munging standardized? X-FORWARDED-FOR, X-FORWARDED-HOST, X-FORWARDED-PROTOCOL, X-FORWARDED-PROTO, HTTP-REQUEST-BASE, X-FORWARDED-SCRIPT-NAME, etc. I'm getting totally lost here.
• I like the idea of moving all "behind proxy" logic into a self-contained middleware.
• I like moving "behind_proxy" as global.
• I like moving is_behind_proxy out of the host and scheme attributes.

So that's my quick brain dump. :)

[veryrusty]

Thanks @xsawyerx ! Lots of positives there (and nothing really scary). I suspect that if the issue that prompted adding behing_proxy to Dancer1 arose today (https://github.com/PerlDancer/Dancer/issues/505), we'd all be saying "use the existing middleware".

As far as I'm aware there are no standards for those headers. Plack::Middleware::ReverseProxy handles all the de facto standard headers (for host/port/protocol). There are NO standards for the path handling that ReverseProxyPath implements.

Is it worth considering Plack::Middleware::BehindProxy into a separate package?

[xsawyerx]

I forgot to mention (since I wrote it quickly) that the work you've done here is really good. I was really happy you sunk your teeth in it, and it seems very well thought-out.

I agree that we would just say people should use the existing middleware. That's definitely the direction for us too.

I'm still not sure about the situation of ReverseProxyPath. Are we sure it's something that shouldn't be done in Plack::App::URLMap?

[veryrusty]

ReverseProxyPath is more general than URLMap, I can go into details if needed..

We could take the approach of using the behind_proxy setting to wrap the app only with ReverseProxy middleware, which is equivalent to current support in the Request class (and also what Catalyst's reverse proxy support does). Then leave path mappings with ReverseProxyPath and/or URLMap to a Cookbook entry. This way the core wouldn't have a dependency on the ReverseProxyPath middleware.

@xsawyerx I'm happy to prepare another PR with this approach if you think its a better fit.

[xsawyerx]

Maybe I'm being too presumptuous, but isn't it also possible to add this solution to the cookbook as something that can be done using Plack::Builder in the PSGI file?

Either way, getting behind_proxy as a global would be something we could accept right away before we resolve this entire issue. Want to try that?

[xsawyerx]

I want to rebase and merge it, but I have one question: Could this simply be a Plack::Middleware namespaced-middleware? Is there a distinction in putting it under the Dancer2 namespace? (okay, I guess these are two questions)

[veryrusty]

The only reason I put it under the Dancer2 namespace is that is is kinda Dancer2 specific; some of the headers that are checked are non-standard ones that only Dancer1/2 have core support for.

I'm happy to release it as a separate package; just need a sane name. Maybe Plack::Middleware::Dancer::BehindProxy ?

[xsawyerx]

Are they really Dancer-specific?

Considering people are hitting these problems on production, wouldn't it mean they should be theoretically supported by others?

My question is rather, is it simply a matter of others not noticing them or only us recognizing them? Is it our special brand of HTTP or is it simply a convention few follow? Ya know what I mean?

[veryrusty]

Here's a (not so short) rundown of the three "non-standard" parts to the middleware in this Pr.

1. The proxied request may use a different protocol to the original. Dancer2::Core::Request has support for using any one of these headers: X-Forwarded-Proto, X-Forwarded-Protocol and Forwarded-Proto. The defacto standard is the X-Forwarded-Proto header, which is what Plack::Middleware::ReverseProxy uses. My preference would be to only supported the "defacto" standard, but that will introduce an incompatibility when some users upgrade if they were previously using one of the other options. This part can be released as a stand-alone middleware if needed.
2. If you do not proxy to the root path, you need addition path info to construct URLs. In Dancer1 the non-standard Request-Base header was used for this, whereas Plack::Middleware::ReverseProxyPath uses X-Forwarded-Script-Name. Since there is no current support in D2 for this, we can document what's needed and move on.
3. #503 added support for the X-Forwarded-Host header containing multiple values. In that case, Plack::Middleware::ReverseProxy takes the last (most recent) whereas that Pr503 takes the first. Since this header can be "spoofed", some may view taking the first as a security risk. eg. you typically only have the last server in the list under your control. The required adjustments here can be wrapped up into a standalone middleware. If someone did release this, then I'd suggest NOT to apply it by default.

I think the summary of that is; If we stick to "defacto" headers that the ReverseProxy and ReverseProxyPath middleware use, we do not need to release further middleware, but will need to help some users in that migration. Did I just volunteer to write some docs..?

[racke]

This is a very old pull request with a complicated discussion. So if you have a compelling reason to keep it open, please speak up. Otherwise I will consider to close it soon.

[veryrusty]

@racke yes its old, but please leave the PR as is.

[racke]

:shrug:

[xsawyerx]

Reviewed this with @cromedome. Here's a summary of our thoughts:

1. If you are setting behind_proxy to true, we can add another middleware that checks whether you are using one of the non-defacto headers and override the defacto header with their values. The middleware would warn each time it detects such a header so users stop using unsupported headers, and it will also warn if there are multiple headers with different values - giving priority to the defacto header.
2. We never supported Request-Base in D2 so we can just use the Plack middlewares' header and document we don't support Request-Base in the Migration document.
3. Considering it's a security risk to use the first address in a series of proxies in a header, we should switch to the last one (essentially letting the Plack middleware do what it's supposed to). We can't think of a way to make this transition easy or seamless, so we'll just document it clearly in the release.