Re: internal vs. external hostname in Request.getHost9)
Ilya Skriblovsky <[email protected]> Wed, 15 Mar 2017 08:20:59 +0000
| Newsgroups | gmane.comp.python.twisted.web |
|---|---|
| Message-ID | <CAOG7vkx5hDm_Mb9v12rMKnva=wYbB328Zxux_CesdSSnZQXq6w@mail.gmail.com> |
--===============2595636242705384547== Content-Type: multipart/alternative; boundary=001a113cec0280b165054ac0a06c --001a113cec0280b165054ac0a06c Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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=3DFalse" argument to Request.URLPath() and check X-Forwarded-Proto header if it is true? And may be add "useXForwardedHost=3DFalse" too to simplify setting up a reverse pro= xy (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 =D1=81=D1=80, 15 =D0=BC=D0=B0=D1=80. 2017 =D0=B3. =D0=B2 9:10, Glyph Lefkow= itz <[email protected]>: > On Mar 14, 2017, at 3:00 PM, Ilya Skriblovsky <[email protected]> > 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=3DTrue) > @app.route('/path') > def path(request): return b'42' > > When /alias is requested werkzeug generates a redirect to /path. But Klei= n > 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 w= e > fix that? > > There are two methods in Request: > =E2=80=A2 Request.getHost() =E2=80=94 "Get my originally requesting trans= port'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 "i= t > looked like a good idea at the time". Request predates the requirement f= or > test coverage, documentation coverage, and, in many cases, the author (me= ) > having any idea what they were doing :). If you find something that look= s > 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 ne= w > 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): > =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() > > Proposal #2 (adding new feature if Variant #1 is choosed): > =E2=80=A2 Add useXForwardedHost=3DFalse argument to Request.getRequestHos= tname() and > useXForwardedProto=3DFalse to Request.isSecure(). If True is passed, thes= e > methods will obey corresponding request headers that are de-facto standar= d > for reverse proxies. Also add corresponding options to Klein app. This ca= n > 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 th= e > 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 coul= d > 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 singl= e > 'request.url()' method which cleanly and correctly returns a > https://twistedmatrix.com/documents/17.1.0/api/twisted.python.url.URL.htm= l > 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 > [email protected] > http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web > --001a113cec0280b165054ac0a06c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Ok, so in the sort term you are suggesting to change Reque= st.URLPath (uppercased method? Hmm) to use Host header instead of getReques= tHostname and to change Klein to use it instead of Request.getHost(), right= ?<div>Sounds wise and reasonable :)</div><div><br></div><div>But I'd li= ke 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(), b= ut it is not sufficient since Request.isSecure() just checks if backend con= nection is SSL while encryption is often terminated at a reverse proxy. Can= we add "useXForwardedProto=3DFalse" argument to Request.URLPath(= ) and check X-Forwarded-Proto header if it is true? And may be add "us= eXForwardedHost=3DFalse" too to simplify setting up a reverse proxy (w= ith a bold red warning in docstring that it can be set to True only if reve= rse proxy is correctly configured to drop corresponding client-specified he= aders). What do you think?</div><div><br></div><div>-- ilya</div></div><br>= <div class=3D"gmail_quote"><div dir=3D"ltr">=D1=81=D1=80, 15 =D0=BC=D0=B0= =D1=80. 2017 =D0=B3. =D0=B2 9:10, Glyph Lefkowitz <<a href=3D"mailto:gly= [email protected]">[email protected]</a>>:<br></div><blockquote= class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px #ccc soli= d;padding-left:1ex"><div style=3D"word-wrap:break-word" class=3D"gmail_msg"= ><div class=3D"gmail_msg"><blockquote type=3D"cite" class=3D"gmail_msg"><di= v class=3D"gmail_msg">On Mar 14, 2017, at 3:00 PM, Ilya Skriblovsky <<a = href=3D"mailto:[email protected]" class=3D"gmail_msg" target=3D"_bl= ank">[email protected]</a>> wrote:</div><br class=3D"m_794912322= 9638861122Apple-interchange-newline gmail_msg"><div class=3D"gmail_msg"><di= v dir=3D"ltr" class=3D"gmail_msg">Tickets you have mentioned and=C2=A0<span= style=3D"font-family:"helvetica neue","bitstream vera sans&= quot;,helvetica,arial,sans-serif" class=3D"gmail_msg">forwarded-for-5807 br= anch</span>=C2=A0are mostly about parsing X-Forwarded-For in order to obtai= n correct client IP. While it is valuable task, it is not what strikes me r= ight now.</div></div></blockquote><div class=3D"gmail_msg"><br class=3D"gma= il_msg"></div></div></div><div style=3D"word-wrap:break-word" class=3D"gmai= l_msg"><div class=3D"gmail_msg"><div class=3D"gmail_msg">Sorry, I was prett= y tired when I wrote that message, and I realize that I was getting server = identification and client identification mixed up.</div></div></div><div st= yle=3D"word-wrap:break-word" class=3D"gmail_msg"><div class=3D"gmail_msg"><= br class=3D"gmail_msg"><blockquote type=3D"cite" class=3D"gmail_msg"><div c= lass=3D"gmail_msg"><div dir=3D"ltr" class=3D"gmail_msg"><div class=3D"gmail= _msg">I'm now more concerned with an absence of API for getting user-vi= sible server's name, not client's ip.</div></div></div></blockquote= ><div class=3D"gmail_msg"><br class=3D"gmail_msg"></div></div></div><div st= yle=3D"word-wrap:break-word" class=3D"gmail_msg"><div class=3D"gmail_msg"><= div class=3D"gmail_msg">Yes.=C2=A0 My mistake. =C2=A0(Although totally fix = those other bugs too. They're also bad. :))</div></div></div><div style= =3D"word-wrap:break-word" class=3D"gmail_msg"><div class=3D"gmail_msg"><br = class=3D"gmail_msg"><blockquote type=3D"cite" class=3D"gmail_msg"><div clas= s=3D"gmail_msg"><div dir=3D"ltr" class=3D"gmail_msg"><div class=3D"gmail_ms= g"><div class=3D"gmail_msg">Look, I'm currently porting my app from Dja= ngo to Klein and noticed strange behavior of Klein. For example:</div><div = class=3D"gmail_msg">@app.route('/alias', alias=3DTrue)</div><div cl= ass=3D"gmail_msg">@app.route('/path')</div><div class=3D"gmail_msg"= >def path(request): return b'42'</div><div class=3D"gmail_msg"><br = class=3D"gmail_msg"></div><div class=3D"gmail_msg">When /alias is requested= werkzeug generates a redirect to /path. But Klein is passing Request.getHo= st() to Werkzeug, so redirect gets internal hostname and exposes backend= 9;s internal hostname and port to the user. Seems like Klein is passing inc= orrect hostname to Werkzeug. But how can we fix that?</div></div><div class= =3D"gmail_msg"><br class=3D"gmail_msg"></div><div class=3D"gmail_msg">There= are two methods in Request:</div><div class=3D"gmail_msg">=E2=80=A2 Reques= t.getHost() =E2=80=94 "Get my originally requesting transport's ho= st" as doc says. Ok, seems like this method intentionally returns serv= er's internal address.</div><div class=3D"gmail_msg">=E2=80=A2 Request.= getRequestHostname() =E2=80=94doc says:</div><div class=3D"gmail_msg">>&= gt; "Get the hostname that the user passed in to the request. This wil= l either use the Host: header (if it is available) or the host we are liste= ning on if the header is unavailable."</div></div></div></blockquote><= blockquote type=3D"cite" class=3D"gmail_msg"><div class=3D"gmail_msg"><div = dir=3D"ltr" class=3D"gmail_msg"><div class=3D"gmail_msg">Cool, but why does= this method only returns a hostname without a port? It intentionally strip= s out the port number from Host header. What is the point of such implement= ation?</div></div></div></blockquote><div class=3D"gmail_msg"><br class=3D"= gmail_msg"></div></div></div><div style=3D"word-wrap:break-word" class=3D"g= mail_msg"><div class=3D"gmail_msg"><div class=3D"gmail_msg">Request is one = of the oldest parts of Twisted, so the likely reason is "it looked lik= e a good idea at the time".=C2=A0 Request predates the requirement for= test coverage, documentation coverage, and, in many cases, the author (me)= having any idea what they were doing :).=C2=A0 If you find something that = looks bad, it's probably just bad, there is unlikely any deeper reason.= </div><div class=3D"gmail_msg"><br class=3D"gmail_msg"></div><div class=3D"= gmail_msg">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"gmail_msg" target=3D"_blank"= >https://twistedmatrix.com/trac/ticket/288</a> ticket.=C2=A0 However, befor= e we do that, we should make all the stuff that is there already behave cor= rectly 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 disrupti= on.</div></div></div><div style=3D"word-wrap:break-word" class=3D"gmail_msg= "><div class=3D"gmail_msg"><br class=3D"gmail_msg"><blockquote type=3D"cite= " class=3D"gmail_msg"><div class=3D"gmail_msg"><div dir=3D"ltr" class=3D"gm= ail_msg"><div class=3D"gmail_msg">This method is used only a couple of time= s inside Twisted itself, and in both places Twisted gets what getRequestHos= tname() returned and mixes it with request.getHost().port which is *definit= ely* 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 imp= ossible to use getRequestHostname() to build user-visible URL. I think curr= ent getRequestHostname() implementation is broken.</div></div></div></block= quote><blockquote type=3D"cite" class=3D"gmail_msg"><div dir=3D"ltr" class= =3D"gmail_msg"><div class=3D"gmail_msg"><br class=3D"gmail_msg"></div><div = class=3D"gmail_msg">So I have two proposals:</div><div class=3D"gmail_msg">= <br class=3D"gmail_msg"></div><div class=3D"gmail_msg">Proposal #1 (fixing = current behavior):</div><div class=3D"gmail_msg">=E2=80=A2 Variant #1: Chan= ge Request.getRequestHostname() to return b"hostname:port". I thi= nk this is the correct thing to do, but this is a backward-incompatible cha= nge.</div><div class=3D"gmail_msg">- or -</div><div class=3D"gmail_msg">=E2= =80=A2 Variant #2: Change Klein to use Request.getHeader(b'Host') w= ith fallback to Request.getHost()</div><div class=3D"gmail_msg"><br class= =3D"gmail_msg"></div><div class=3D"gmail_msg">Proposal #2 (adding new featu= re if Variant #1 is choosed):</div><div class=3D"gmail_msg">=E2=80=A2 Add u= seXForwardedHost=3DFalse argument to Request.getRequestHostname() and useXF= orwardedProto=3DFalse to Request.isSecure(). If True is passed, these metho= ds will obey corresponding request headers that are de-facto standard for r= everse proxies. Also add corresponding options to Klein app. This can simpl= ify reverse proxy configuration a bit.</div></div></blockquote><br class=3D= "gmail_msg"></div></div><div style=3D"word-wrap:break-word" class=3D"gmail_= msg"><div class=3D"gmail_msg"></div><div class=3D"gmail_msg">I have a third= proposal.</div><div class=3D"gmail_msg"><br class=3D"gmail_msg"></div><div= class=3D"gmail_msg">Ideally if we want to know about the URL for the reque= st, we could ask the request to just give us the URL.=C2=A0 And in fact the= URL does have a method, URLPath(), which is both (A)=C2=A0<i class=3D"gmai= l_msg">unambiguously</i>=C2=A0broken (the case could be made that getReques= tHostname() is supposed to really just be a host, not for URL generation, a= nd maybe there is even a case where that makes sense; origin comparisons pe= rhaps) and (B) returning a data structure which could be fixed to be correc= t without concern for client compatibility.</div><div class=3D"gmail_msg"><= br class=3D"gmail_msg"></div><div class=3D"gmail_msg">In the long term, we = should get rid of all these methods and have a single 'request.url()= 9; method which cleanly and correctly returns a=C2=A0<a href=3D"https://twi= stedmatrix.com/documents/17.1.0/api/twisted.python.url.URL.html" class=3D"g= mail_msg" target=3D"_blank">https://twistedmatrix.com/documents/17.1.0/api/= twisted.python.url.URL.html</a> object, which is better than a string or a = URLPath (basically, it's what URLPath should have been if we had design= ed it carefully).=C2=A0 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 class=3D"gmail_msg"><br class=3D"gmail_msg= "></div><div class=3D"gmail_msg">getRequestHostname is, as you correctly ca= lled out, probably useless, but we should just adjust its docstring to dire= ct users to the URLPath method instead.</div><div class=3D"gmail_msg"><br c= lass=3D"gmail_msg"></div><div class=3D"gmail_msg">Klein should then be chan= ged to use Request.URLPath() to build any URLs.</div><div class=3D"gmail_ms= g"><br class=3D"gmail_msg"></div><div class=3D"gmail_msg">What do you think= of this proposal?=C2=A0 Does my reasoning make sense?</div></div><div styl= e=3D"word-wrap:break-word" class=3D"gmail_msg"><div class=3D"gmail_msg"><br= class=3D"gmail_msg"></div><div class=3D"gmail_msg">-glyph</div><div class= =3D"gmail_msg"><br class=3D"gmail_msg"></div></div>________________________= _______________________<br class=3D"gmail_msg"> Twisted-web mailing list<br class=3D"gmail_msg"> <a href=3D"mailto:[email protected]" class=3D"gmail_msg" target= =3D"_blank">[email protected]</a><br class=3D"gmail_msg"> <a href=3D"http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web" r= el=3D"noreferrer" class=3D"gmail_msg" target=3D"_blank">http://twistedmatri= x.com/cgi-bin/mailman/listinfo/twisted-web</a><br class=3D"gmail_msg"> </blockquote></div> --001a113cec0280b165054ac0a06c-- --===============2595636242705384547== 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 --===============2595636242705384547==--