Tickets you have mentioned and forwarded-for-5807 branch are mostly about parsing X-Forwarded-For in order to obtain correct client IP. While it is valuable task, it is not what strikes me right now.

I'm now more concerned with an absence of API for getting user-visible server's name, not client's ip.

Look, I'm currently porting my app from Django to Klein and noticed strange behavior of Klein. For example: @app.route('/alias', alias=True) @app.route('/path') def path(request): return b'42'

When /alias is requested werkzeug generates a redirect to /path. But Klein is passing Request.getHost() to Werkzeug, so redirect gets internal hostname and exposes backend's internal hostname and port to the user. Seems like Klein is passing incorrect hostname to Werkzeug. But how can we fix that?

There are two methods in Request: • Request.getHost() — "Get my originally requesting transport's host" as doc says. Ok, seems like this method intentionally returns server's internal address. • Request.getRequestHostname() —doc says:

...

"Get the hostname that the user passed in to the request. This will

either use the Host: header (if it is available) or the host we are listening on if the header is unavailable." Cool, but why does this method only returns a hostname without a port? It intentionally strips out the port number from Host header. What is the point of such implementation? This method is used only a couple of times inside Twisted itself, and in both places Twisted gets what getRequestHostname() returned and mixes it with request.getHost().port which is *definitely* incorrect, because the former is user-visible while latter is internal. So if my backend server is using different port than a fronend, it is impossible to use getRequestHostname() to build user-visible URL. I think current getRequestHostname() implementation is broken.

So I have two proposals:

Proposal #1 (fixing current behavior): • Variant #1: Change Request.getRequestHostname() to return b"hostname:port". I think this is the correct thing to do, but this is a backward-incompatible change. - or - • Variant #2: Change Klein to use Request.getHeader(b'Host') with fallback to Request.getHost()

Proposal #2 (adding new feature if Variant #1 is choosed): • Add useXForwardedHost=False argument to Request.getRequestHostname() and useXForwardedProto=False to Request.isSecure(). If True is passed, these methods will obey corresponding request headers that are de-facto standard for reverse proxies. Also add corresponding options to Klein app. This can simplify reverse proxy configuration a bit.

-- ilya

вт, 14 мар. 2017 г. в 10:33, Ilya Skriblovsky ilyaskriblovsky@gmail.com:

Thanks, I will study tickets you mentioned and hopefully fix it. Quick-n-dirty fix gave me only two failed tests and in both cases it seems to be a wrong assumption in tests. So I hope this change won't break the world.

-- ilya

вт, 14 мар. 2017 г. в 10:12, Glyph Lefkowitz glyph@twistedmatrix.com:

On Mar 13, 2017, at 11:01 PM, Ilya Skriblovsky ilyaskriblovsky@gmail.com wrote:

Hi,

I'm using Twisted Web server behind Nginx reverse-proxy and I'm getting backend's internal host:port from Request.getHost().

Seems like Request.host is explicitly set to socket's address (i.e. internal address) here: ​ https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L838 But comment at ​ https://github.com/twisted/twisted/blob/trunk/src/twisted/web/http.py#L1297 and what this method does points that Request.host meant to reflect Host header of the request, i.e. user-visible hostname and port.

This creates problems for me when using Klein because it correctly uses Request.getHost() to create host part of URLs for redirects.

It seems like inconsistency in Twisted code. I'd expect Request.host should be only set from the Host request header to reflect user-visible hostname, not the internal backend server's address. Or may be I'm missing something?

You're absolutely correct! I even filed a ticket for this functionality, 5 years ago: https://twistedmatrix.com/trac/ticket/5807 There's even a branch for it. Oddly enough we *do* have a *private* _XForwardedForRequest, but... it's only used for logging, for some reason.

If you want accurate access logging and request information, https://twistedmatrix.com/trac/ticket/7704 will probably also be of interest to you.

I'm so sorry you've hit this glaring deficiency in Twisted.

On the other hand: I'm so glad that you've hit this glaring deficiency in Twisted! I hope you will be motivated to fix it :-). It's bothered me for quite some time that we don't play nicely with proxying setups, when such setups are so *incredibly* common. If you can write pull requests to fix these issues and put them into review, I'm pretty sure you will find an enthusiastic reviewer quickly.

-glyph _______________________________________________ Twisted-web mailing list Twisted-web@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web

Ok, so in the sort term you are suggesting to change Request.URLPath (uppercased method? Hmm) to use Host header instead of getRequestHostname and to change Klein to use it instead of Request.getHost(), right? Sounds wise and reasonable :)

But I'd like to add one more thing. In order to build correct external URL we need to know is it http or https. Currently URLPath is using Request.isSecure(), but it is not sufficient since Request.isSecure() just checks if backend connection is SSL while encryption is often terminated at a reverse proxy. Can we add "useXForwardedProto=False" argument to Request.URLPath() and check X-Forwarded-Proto header if it is true? And may be add "useXForwardedHost=False" too to simplify setting up a reverse proxy (with a bold red warning in docstring that it can be set to True only if reverse proxy is correctly configured to drop corresponding client-specified headers). What do you think?

-- ilya

ср, 15 мар. 2017 г. в 9:10, Glyph Lefkowitz glyph@twistedmatrix.com:

On Mar 14, 2017, at 3:00 PM, Ilya Skriblovsky ilyaskriblovsky@gmail.com wrote:

Tickets you have mentioned and forwarded-for-5807 branch are mostly about parsing X-Forwarded-For in order to obtain correct client IP. While it is valuable task, it is not what strikes me right now.

Sorry, I was pretty tired when I wrote that message, and I realize that I was getting server identification and client identification mixed up.

I'm now more concerned with an absence of API for getting user-visible server's name, not client's ip.

Yes. My mistake. (Although totally fix those other bugs too. They're also bad. :))

Look, I'm currently porting my app from Django to Klein and noticed strange behavior of Klein. For example: @app.route('/alias', alias=True) @app.route('/path') def path(request): return b'42'

When /alias is requested werkzeug generates a redirect to /path. But Klein is passing Request.getHost() to Werkzeug, so redirect gets internal hostname and exposes backend's internal hostname and port to the user. Seems like Klein is passing incorrect hostname to Werkzeug. But how can we fix that?

There are two methods in Request: • Request.getHost() — "Get my originally requesting transport's host" as doc says. Ok, seems like this method intentionally returns server's internal address. • Request.getRequestHostname() —doc says:

...

...

"Get the hostname that the user passed in to the request. This will

either use the Host: header (if it is available) or the host we are listening on if the header is unavailable."

Cool, but why does this method only returns a hostname without a port? It intentionally strips out the port number from Host header. What is the point of such implementation?

Request is one of the oldest parts of Twisted, so the likely reason is "it looked like a good idea at the time". Request predates the requirement for test coverage, documentation coverage, and, in many cases, the author (me) having any idea what they were doing :). If you find something that looks bad, it's probably just bad, there is unlikely any deeper reason.

Long term, we need to overhaul the API to have fewer methods and be generally less confusing. See for example the infamous https://twistedmatrix.com/trac/ticket/288 ticket. However, before we do that, we should make all the stuff that is there already behave correctly and be documented even in its weird shape; then we can transition to a new good thing confident in the knowledge that no old applications will break and that users can move over to the new APIs without massive disruption.

This method is used only a couple of times inside Twisted itself, and in both places Twisted gets what getRequestHostname() returned and mixes it with request.getHost().port which is *definitely* incorrect, because the former is user-visible while latter is internal. So if my backend server is using different port than a fronend, it is impossible to use getRequestHostname() to build user-visible URL. I think current getRequestHostname() implementation is broken.

So I have two proposals:

Proposal #1 (fixing current behavior): • Variant #1: Change Request.getRequestHostname() to return b"hostname:port". I think this is the correct thing to do, but this is a backward-incompatible change.

• or -

• Variant #2: Change Klein to use Request.getHeader(b'Host') with fallback to Request.getHost()

Proposal #2 (adding new feature if Variant #1 is choosed): • Add useXForwardedHost=False argument to Request.getRequestHostname() and useXForwardedProto=False to Request.isSecure(). If True is passed, these methods will obey corresponding request headers that are de-facto standard for reverse proxies. Also add corresponding options to Klein app. This can simplify reverse proxy configuration a bit.

I have a third proposal.

Ideally if we want to know about the URL for the request, we could ask the request to just give us the URL. And in fact the URL does have a method, URLPath(), which is both (A) *unambiguously* broken (the case could be made that getRequestHostname() is supposed to really just be a host, not for URL generation, and maybe there is even a case where that makes sense; origin comparisons perhaps) and (B) returning a data structure which could be fixed to be correct without concern for client compatibility.

In the long term, we should get rid of all these methods and have a single 'request.url()' method which cleanly and correctly returns a https://twistedmatrix.com/documents/17.1.0/api/twisted.python.url.URL.html object, which is better than a string or a URLPath (basically, it's what URLPath should have been if we had designed it carefully). In the meanwhile, without adding a bunch of new API surface and abandoning existing methods, Request.URLPath() is the easiest place to put this fix.

getRequestHostname is, as you correctly called out, probably useless, but we should just adjust its docstring to direct users to the URLPath method instead.

Klein should then be changed to use Request.URLPath() to build any URLs.

What do you think of this proposal? Does my reasoning make sense?

-glyph

---

Twisted-web mailing list Twisted-web@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web

The code calling request.URLPath(), in a given Resource, or application,

is highly unlikely to know whether it wants to honor (x-)forwarded-for. You are right, I haven't thought about it. But I'm in doubt whether trusting X-Forwarded-* by default can damage security if Twisted app is running with naked HTTP(S) port exposed without reverse proxy that handles these headers. There are three headers: 1. X-Forwarded-For specifying original client IP and IPs of proxies 2. X-Forwarded-Host specifying original Host header from the client 3. X-Forwarded-Proto specifying original client's scheme (there is also new-style "Forwarded:" header but it is not widely used yet, AFAIK)

X-Forwarded-For definetly can't be trusted if comes from untrusted client client. Fortunately we don't need it at all for generating URLs :) It will be in question when refactoring getClientIP() somewhen later.

But can we trust X-Forwarded-Host & X-Forwarded-Proto? From the first glance it isn't a problem since we are using them to display URLs for the same client, so nasty client will get his nasty URLs, that's it. But if app is doing something like storing URL in DB or (more likely) sending an email with a link to another client, this would be an issue.

-- ilya

пт, 17 мар. 2017 г. в 11:18, Glyph glyph@twistedmatrix.com:

On Mar 15, 2017, at 1:20 AM, Ilya Skriblovsky ilyaskriblovsky@gmail.com wrote:

Ok, so in the sort term you are suggesting to change Request.URLPath

Yes.

(uppercased method? Hmm)

Like I said, not a great interface, overall :-).

to use Host header instead of getRequestHostname and to change Klein to use it instead of Request.getHost(), right? Sounds wise and reasonable :)

OK, glad you agree :).

But I'd like to add one more thing. In order to build correct external URL we need to know is it http or https. Currently URLPath is using Request.isSecure(), but it is not sufficient since Request.isSecure() just checks if backend connection is SSL while encryption is often terminated at a reverse proxy. Can we add "useXForwardedProto=False" argument to Request.URLPath() and check X-Forwarded-Proto header if it is true? And may be add "useXForwardedHost=False" too to simplify setting up a reverse proxy (with a bold red warning in docstring that it can be set to True only if reverse proxy is correctly configured to drop corresponding client-specified headers). What do you think?

I think that for starters, it would make more sense to just fix it to *always* honor forwarded-for and x-forwarded-for headers. The code calling request.URLPath(), in a given Resource, or application, is highly unlikely to know whether it wants to honor (x-)forwarded-for. The code that might know about this sort of configuration would be the thing that constructs the Site object, but I'd be much happier to just get a change that always honors it first, and then figure out how to customize it later.

-glyph

_______________________________________________ Twisted-web mailing list Twisted-web@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web

This thread is mostly about X-Forwarded-Host & X-Forwarded-Proto because the original issue was inability of Twisted Web server to obtain it's public hostname. X-Forwarded-For is another (and probably more complex) story.

Django indeed dropped support for X-Forwarded-For, but it does support X-Forwarded-Host [1] and X-Forwarded-Proto [2] on opt-in basis.

I'm agree that none of the headers should be trusted by default and that opting-in should be done at Site level.

-- ilya

[1]: https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-USE_X_FORWA... [2]: https://docs.djangoproject.com/en/1.10/ref/settings/#std:setting-SECURE_PROX...

пн, 20 мар. 2017 г. в 21:32, Tom Most tommost@gmail.com:

On Fri, Mar 17, 2017 at 2:52 AM, Ilya Skriblovsky < ilyaskriblovsky@gmail.com> wrote:

...

The code calling request.URLPath(), in a given Resource, or

application, is highly unlikely to know whether it wants to honor (x-)forwarded-for. You are right, I haven't thought about it. But I'm in doubt whether trusting X-Forwarded-* by default can damage security if Twisted app is running with naked HTTP(S) port exposed without reverse proxy that handles these headers. There are three headers:

1. X-Forwarded-For specifying original client IP and IPs of proxies
2. X-Forwarded-Host specifying original Host header from the client
3. X-Forwarded-Proto specifying original client's scheme

(there is also new-style "Forwarded:" header but it is not widely used yet, AFAIK)

X-Forwarded-For definetly can't be trusted if comes from untrusted client client. Fortunately we don't need it at all for generating URLs :) It will be in question when refactoring getClientIP() somewhen later.

But can we trust X-Forwarded-Host & X-Forwarded-Proto? From the first glance it isn't a problem since we are using them to display URLs for the same client, so nasty client will get his nasty URLs, that's it. But if app is doing something like storing URL in DB or (more likely) sending an email with a link to another client, this would be an issue.

It's not safe to enable support for X-Forwarded-For and friends by default, since you can't know how application code will use that information and in many configurations it may be spoofed by clients.

A proper deployment which sets these headers looks like this:

1. Configure your frontend reverse-proxy (nginx or whatever) to discard

incoming X-Forwarded-* headers and *set* X-Forwarded-* as appropriate. Note that X-Forwarded-For is actually a *list* of hops[1], so if you do this naively your server may append to the list! [2] 2. Configure your backend services to respect exactly the headers passed by your frontend.

Note that this requires administrative action in two places, and that if you don't do the frontend config it will probably pass the headers through, allowing them to be spoofed.

A non-exhaustive list of possible bad stuff I client could use spoofing for:

1. Evade IP-based access control (a reasonable defense-in-depth measure).
2. Evade pinning of user sessions to IP addresses or subnets.
3. Evade IP-based rate limiting, e.g. as discussed at [3].

It's also probably worth noting that Django used to offer support for X-Forwarded-For and removed it[4]. X-Forwarded-* and friends just too varied in the wild to reasonably support.

If Twisted is to support this in any way, I think that it should be opt-in support for the Forwarded header as specified in RFC 7239. This should be a parameter applicable to all of twisted.web.server rather than per-method call, since it's something the administrator needs to set.

—Tom

[1]: Standardized as Forwarded in https://tools.ietf.org/html/rfc7239 [2]: Your frontend proxy should also validate Host is a domain you control to prevent cookie theft. Plus lots of other stuff. Web security is hard, and every default everywhere sets users up for failure. [3]: http://django-ratelimit.readthedocs.io/en/v1.0.0/security.html [4]: https://www.djangoproject.com/weblog/2009/jul/28/security/#secondary-issue _______________________________________________ Twisted-web mailing list Twisted-web@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web

On Mar 20, 2017, at 11:30 AM, Tom Most tommost@gmail.com wrote:

If Twisted is to support this in any way, I think that it should be opt-in support for the Forwarded header as specified in RFC 7239. This should be a parameter applicable to all of twisted.web.server rather than per-method call, since it's something the administrator needs to set.

I'm generally in agreement with this. Further, we should probably have some notion of authentication, i.e. Site(..., trustForwardedForFrom=[...]), where [...] could be, let's say a twisted.internet.ssl.Certificate representing a client CA to check client connections from, or a list of twisted.internet.address.IPv4Address objects naming servers on a network we can trust. Effectively building in authentication to this layer is important (and since twisted is a web _server_ and not a web framework, more generally possible than e.g. Django).

-glyph