Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override 'wsgi.url_scheme' via a request header, 'X_WSGI_URL_SCHEME'. #42

Merged
merged 10 commits into from
Mar 10, 2014

Conversation

tseaver
Copy link
Member

@tseaver tseaver commented Sep 3, 2013

Allows proxies which serve mixed HTTP / HTTPS requests to control signal
which are served as HTTPS.

Allows proxies which serve mixed HTTP / HTTPS requests to control signal
which are served as HTTPS.
@tseaver
Copy link
Member Author

tseaver commented Sep 3, 2013

Removes one more reason to mention / explain Paste. The only question I have is whether waitress should expect the same 'X_Forwarded_Scheme' header as Paste; I chose not, but it would be easy to switch back.

@mmerickel
Copy link
Member

Ah and as you mentioned the prefix middleware supports both X-Forwarded-Proto and X-Forwarded-Scheme, both of which are (of course) undocumented.

To be clear the prefix middleware is in PasteDeploy, so mentioning it is not so bad. It's py3k compatible.

@@ -13,6 +13,9 @@ Differences from ``zope.server``

- Calls "close()" on the app_iter object returned by the WSGI application.

- ALlows per-request override of ``wsgi.url_scheme`` by the value of the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a minor typo here: s/ALlows/Allows/

@kgaughan
Copy link
Member

kgaughan commented Sep 7, 2013

I was thinking: might it be worthwhile ensuring that X-Forwarded-Proto can't be used to override the value explicitly set in waitress.adjustments.Adjustments.url_scheme when waitress is spun up?

@tseaver
Copy link
Member Author

tseaver commented Oct 24, 2013

@kgaughan: Zope's configuration exposes the notion of a "trusted proxy": if the upstream IP matched, then the headers passed by the proxy would be used unconditionally. Otherwise, the configured defaults would be used. Maybe we need to do the same?

@kgaughan
Copy link
Member

That definitely seems worthwhile.

@sontek
Copy link
Member

sontek commented Mar 9, 2014

@tseaver whats holding up the merge of this?

@sontek
Copy link
Member

sontek commented Mar 10, 2014

I think it'd be good to support X-Forwarded-For and X-Forwarded-Proto, thpose are the standard headers that things like nginx and gunicorn use. X-Forwarded-For would be the client IP and -Proto would be http/https.

@mmerickel
Copy link
Member

This should be opt-in, and should also include the X-Forwarded-For support that @sontek mentioned. It'd be unwise to publish this as-is since it's a large vulnerability for any waitress server that is not behind a properly-configured reverse proxy to trust a random HTTP header.

@tseaver
Copy link
Member Author

tseaver commented Mar 10, 2014

@mmerickel @sontek : 'X-Forwarded-For' isn't relevant to this PR, which is about allowing mixed-mode proxies to signal to the application which scheme was used in the original request, so that they can generate absolute URLs using the same scheme.

But yes, I haven't merged this because of the concern @mmerickel mentions about folks running w/o proxies.

@mmerickel
Copy link
Member

I think the thought process is that both headers are very common in a reverse proxy configuration and may be opted-in by the same option, but of course maybe not.

allow_forwarded_scheme = yes
allow_forwarded_ip = yes

@mmerickel
Copy link
Member

I wish this was done via the allow* options I mentioned rather than this trusted_proxy ip address because now I have to mess with my deployment setup to make my app know about the proxy it's running behind, while previously my wsgi server was completely agnostic and just as secure.

@tseaver
Copy link
Member Author

tseaver commented Mar 10, 2014

Feel free to update it after I merge the work I've already done.

tseaver added a commit that referenced this pull request Mar 10, 2014
Override 'wsgi.url_scheme' via a request header, 'X_WSGI_URL_SCHEME'.
@tseaver tseaver merged commit 09f25b3 into master Mar 10, 2014
@@ -28,8 +28,14 @@ threads
number of threads used to process application logic (integer), default
``4``

trusted_proxy
IP addreess of a client allowed to override ``url_scheme`` via the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling

@mmerickel
Copy link
Member

I'd happily update it, but I don't like to change code without talking about it first. By that, I can't quite tell if you think the concerns are not relevant or you just really wanted to merge this branch?

@tseaver
Copy link
Member Author

tseaver commented Mar 11, 2014

I would prefer the 'trusted_proxy' approach myself (less convenient, but also less susceptible to being abused if the appserver is accidentally exposed to public requests). I can live with it either way.

@pwaller
Copy link

pwaller commented Mar 12, 2014

Huh, I just followed the latest documentation and hadn't realised that this isn't released yet. Surprised such a feature is so new. What's the best way to configure it before the new release, then?

With respect to the issue at hand, what if there are multiple proxies in a load balancing arrangement? It seems like hard-coding IPs into application configuration files is not the ideal place to do it and that this configuration should really live in the runtime environment architecture or in a firewall. If I was deploying an application I would make it listen on an interface where only trusted proxies could talk to it in the first place.

@pwaller
Copy link

pwaller commented Mar 12, 2014

(I meant to give a shout-out to @mmerickel's allow_* suggestion)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants