Re: internal vs. external hostname in Request.getHost9)

--===============1110597621251517107==
Content-Type: multipart/alternative;
 boundary="Apple-Mail=_B4AE43CE-3654-4CCA-AACC-7A03B41F260F"


--Apple-Mail=_B4AE43CE-3654-4CCA-AACC-7A03B41F260F
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> On Mar 14, 2017, at 3:00 PM, Ilya Skriblovsky =
<[email protected]> wrote:
>=20
> 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=3DTrue)
> @app.route('/path')
> def path(request): return b'42'
>=20
> 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?
>=20
> There are two methods in Request:
> =E2=80=A2 Request.getHost() =E2=80=94 "Get my originally requesting =
transport's host" as doc says. Ok, seems like this method intentionally =
returns server's internal address.
> =E2=80=A2 Request.getRequestHostname() =E2=80=94doc 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.
>=20
> So I have two proposals:
>=20
> Proposal #1 (fixing current behavior):
> =E2=80=A2 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 -
> =E2=80=A2 Variant #2: Change Klein to use Request.getHeader(b'Host') =
with fallback to Request.getHost()
>=20
> Proposal #2 (adding new feature if Variant #1 is choosed):
> =E2=80=A2 Add useXForwardedHost=3DFalse argument to =
Request.getRequestHostname() and useXForwardedProto=3DFalse 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


--Apple-Mail=_B4AE43CE-3654-4CCA-AACC-7A03B41F260F
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html><head><meta http-equiv=3D"Content-Type" content=3D"text/html =
charset=3Dutf-8"></head><body style=3D"word-wrap: break-word; =
-webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" =
class=3D""><br class=3D""><div><blockquote type=3D"cite" class=3D""><div =
class=3D"">On Mar 14, 2017, at 3:00 PM, Ilya Skriblovsky &lt;<a =
href=3D"mailto:[email protected]" =
class=3D"">[email protected]</a>&gt; wrote:</div><br =
class=3D"Apple-interchange-newline"><div class=3D""><div dir=3D"ltr" =
class=3D"">Tickets you have mentioned and&nbsp;<span =
style=3D"font-family:&quot;helvetica neue&quot;,&quot;bitstream vera =
sans&quot;,helvetica,arial,sans-serif" class=3D"">forwarded-for-5807 =
branch</span>&nbsp;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.</div></div></blockquote><div><br =
class=3D""></div><div>Sorry, I was pretty tired when I wrote that =
message, and I realize that I was getting server identification and =
client identification mixed up.</div><br class=3D""><blockquote =
type=3D"cite" class=3D""><div class=3D""><div dir=3D"ltr" class=3D""><div =
class=3D"">I'm now more concerned with an absence of API for getting =
user-visible server's name, not client's =
ip.</div></div></div></blockquote><div><br class=3D""></div><div>Yes. =
&nbsp;My mistake. &nbsp;(Although totally fix those other bugs too. =
They're also bad. :))</div><br class=3D""><blockquote type=3D"cite" =
class=3D""><div class=3D""><div dir=3D"ltr" class=3D""><div =
class=3D""><div class=3D"">Look, I'm currently porting my app from =
Django to Klein and noticed strange behavior of Klein. For =
example:</div><div class=3D"">@app.route('/alias', alias=3DTrue)</div><div=
 class=3D"">@app.route('/path')</div><div class=3D"">def path(request): =
return b'42'</div><div class=3D""><br class=3D""></div><div =
class=3D"">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?</div></div><div class=3D""><br =
class=3D""></div><div class=3D"">There are two methods in =
Request:</div><div class=3D"">=E2=80=A2 Request.getHost() =E2=80=94 "Get =
my originally requesting transport's host" as doc says. Ok, seems like =
this method intentionally returns server's internal address.</div><div =
class=3D"">=E2=80=A2 Request.getRequestHostname() =E2=80=94doc =
says:</div><div class=3D"">&gt;&gt; "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."</div></div></div></blockquote><blockquote type=3D"cite" =
class=3D""><div class=3D""><div dir=3D"ltr" class=3D""><div =
class=3D"">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?</div></div></div></blockquote><div><br =
class=3D""></div><div>Request is one of the oldest parts of Twisted, so =
the likely reason is "it looked like a good idea at the time". =
&nbsp;Request predates the requirement for test coverage, documentation =
coverage, and, in many cases, the author (me) having any idea what they =
were doing :). &nbsp;If you find something that looks bad, it's probably =
just bad, there is unlikely any deeper reason.</div><div><br =
class=3D""></div><div>Long term, we need to overhaul the API to have =
fewer methods and be generally less confusing. See for example the =
infamous <a href=3D"https://twistedmatrix.com/trac/ticket/288" =
class=3D"">https://twistedmatrix.com/trac/ticket/288</a> ticket. =
&nbsp;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.</div><br =
class=3D""><blockquote type=3D"cite" class=3D""><div class=3D""><div =
dir=3D"ltr" class=3D""><div class=3D"">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.</div></div></div></blockquote><blockquote =
type=3D"cite" class=3D""><div dir=3D"ltr" class=3D""><div class=3D""><br =
class=3D""></div><div class=3D"">So I have two proposals:</div><div =
class=3D""><br class=3D""></div><div class=3D"">Proposal #1 (fixing =
current behavior):</div><div class=3D"">=E2=80=A2 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.</div><div class=3D"">- or -</div><div class=3D"">=E2=80=A2 =
Variant #2: Change Klein to use Request.getHeader(b'Host') with fallback =
to Request.getHost()</div><div class=3D""><br class=3D""></div><div =
class=3D"">Proposal #2 (adding new feature if Variant #1 is =
choosed):</div><div class=3D"">=E2=80=A2 Add useXForwardedHost=3DFalse =
argument to Request.getRequestHostname() and useXForwardedProto=3DFalse =
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.</div></div></blockquote><br =
class=3D""></div><div>I have a third proposal.</div><div><br =
class=3D""></div><div>Ideally if we want to know about the URL for the =
request, we could ask the request to just give us the URL. &nbsp;And in =
fact the URL does have a method, URLPath(), which is both (A)&nbsp;<i =
class=3D"">unambiguously</i>&nbsp;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.</div><div><br class=3D""></div><div>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&nbsp;<a =
href=3D"https://twistedmatrix.com/documents/17.1.0/api/twisted.python.url.=
URL.html" =
class=3D"">https://twistedmatrix.com/documents/17.1.0/api/twisted.python.u=
rl.URL.html</a> object, which is better than a string or a URLPath =
(basically, it's what URLPath should have been if we had designed it =
carefully). &nbsp;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.</div><div><br =
class=3D""></div><div>getRequestHostname is, as you correctly called =
out, probably useless, but we should just adjust its docstring to direct =
users to the URLPath method instead.</div><div><br =
class=3D""></div><div>Klein should then be changed to use =
Request.URLPath() to build any URLs.</div><div><br =
class=3D""></div><div>What do you think of this proposal? &nbsp;Does my =
reasoning make sense?</div><div><br =
class=3D""></div><div>-glyph</div><div><br class=3D""></div></body></html>=

--Apple-Mail=_B4AE43CE-3654-4CCA-AACC-7A03B41F260F--


--===============1110597621251517107==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Twisted-web mailing list
[email protected]
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web

--===============1110597621251517107==--