Opened 5 years ago

Last modified 5 months ago

#5807 enhancement new

something in twisted.web should respect (x-)forwarded-for on the server side

Reported by: Glyph Owned by: Evilham
Priority: normal Milestone:
Component: web Keywords:
Cc: Allister MacLeod, github@… Branch: branches/forwarded-for-5807
branch-diff, diff-cov, branch-cov, buildbot
Author: sirgolan

Description (last modified by Evilham)

This should work vaguely the same way that twisted.web.vhost.VHostMonsterResource works.

  • Request.getHost() should return the forwarded-for host rather than the Host:.
  • Request.isSecure should return the security of the proto parameter

Like VHostMonster, there should be some configuration required to get into this mode. One mechanism for doing that would be to have a ForwardedForParserResource; however, since the connecting address is quite important, it may also be reasonable to build this directly into Site. Trusting random forwarded-for headers off the internet would not be good, so it should be easy to specify what the address of the expected terminating proxy is.

Also, forwarded-for is a bit more expressive than the vhostmonster idiom in that it can describe multiple hops. This additional information should be exposed through an explicit API - perhaps a new forwardedFor method on Resource that returns an iterable of objects describing the hosts that it was forwarded through.

See #5806, https://tools.ietf.org/html/rfc7239

Change History (11)

comment:1 Changed 5 years ago by Glyph

Component: coreweb

comment:2 Changed 5 years ago by Allister MacLeod

Cc: Allister MacLeod added

comment:3 Changed 5 years ago by Glyph

I need to fill this out with some more details; if you want to implement it please comment first so I can fill you in.

comment:4 Changed 5 years ago by Glyph

Description: modified (diff)

I think this should be sufficient for a spec. Please comment if you think I should be more specific.

comment:5 Changed 4 years ago by Mike Handverger

Author: sirgolan
Branch: branches/forwarded-for-5807

(In [39085]) Branching to 'forwarded-for-5807'.

comment:6 Changed 4 years ago by Mike Handverger

Owner: set to Mike Handverger

comment:7 Changed 4 years ago by Mike Handverger

(In [39187]) Implement Resource subclass for automatically updating the client IP taking into account X-Forwarded-For headers. Also add a getForwarders method to the Request class. Refs: #5807

comment:8 Changed 4 years ago by Mike Handverger

(In [39188]) Renamed things to match the ticket. Refs: #5807

comment:9 Changed 14 months ago by Julian Berman

FWIW the vocabulary that e.g. werkzeug uses here (for what is being called forwardedFor) is "access route".

No comment on which is better than the other.

comment:10 Changed 5 months ago by Evilham

Description: modified (diff)
Owner: changed from Mike Handverger to Evilham

I am necring this, since I'm touching code of mine with very ugly workarounds for this ticket.

I see some work had been started: https://github.com/twisted/twisted/commit/4a40d4d21d73b5accc2a7e0411367da12977b474

However, this does seem to assume HTTP / HTTPS connections are established only on ports 80 and 443 respectively (http.Request.setClientIP). I want to point out, that's not always the case, furthermore, it is possible to proxy requests for twisted using UNIX sockets, which means the request has no IP information.

Would it be a terrible idea to have this functionality directly in the t.w.Resource class? I envision a allowedForwarders attribute that when set to a list of IP combinations, performs the IP changing magic and when not set behaves as usual.

That way usage would be something like:

    root = resource.Resource()
    root.allowedForwarders = ['127.0.0.1']
    site = server.Site(root)

With no behaviour changes if line 2 is absent. For a UNIX Socket Forwarder, maybe a allowAnyForwarder flag in resource.Resource with sufficient documentation warnings would do the trick (in this case, the Forwarder should be configured to strip any header it does not trust).

Since the header is now part of the "Permanent Message Header Field Names", it's now "Forwarded: for=value;proto=value", and so on; as opposed to X-Forwarded-For, X-Forwarded-Proto and friends.

Is it worth it to support both? Otherwise I'd go for the one in RFC7239.

AFAICT, there is no defined behaviour for when a requests hits the server with Forwarded Headers from an IP that is not trusted, what would be more sensitive here?

Two options come to mind, where I'd think option 1 makes more sense

  1. Ignore the headers
  2. Return a 403

Thoughts?

PS: This is Gunicorn's deployment documentation, which covers this topic: http://docs.gunicorn.org/en/stable/deploy.html

Last edited 5 months ago by Evilham (previous) (diff)

comment:11 Changed 5 months ago by Evilham

Cc: github@… added
Note: See TracTickets for help on using tickets.