989055 - django rate limiting should honor X-Forwarded-For header

Status: RESOLVED FIXED

(Socorro :: Webapp, task)

Description

[Robert Helmer [:rhelmer]]

Requests to Mozilla's crash-stats instances appear to come from the IP address of our load balancer, so our rate limit (which is per-IP) is way lower than it should be.

The django rate limiter should honor the X-Forwarded HTTP header, and fall back to what it is doing now if that does not exist.

Comment 1

[Peter Bengtsson [:peterbe]]

Now we depend on this:
https://github.com/jsocol/django-ratelimit/pull/41

Comment 2

[me]

This is not going to be "fixed" in django-ratelimit. 3rd party tools like django-ratelimit are the wrong place to implement this:

* If REMOTE_ADDR is wrong, it's wrong, and django-ratelimit is not the only tool that needs to touch this.
* If you use X-Client-IP (or use a tproxy load balancer, or don't explicitly exclude setting X-Forwarded-For, or X-Forwarded-For is a list) this exposes a serious security issue.

Django dropped SetRemoteAddressFromForwardedFor middleware because this behavior is far too deployment-dependent, and the correct solution is to write middleware that sets REMOTE_ADDR correctly, and for all 3rd-party packages like django-ratelimit to use REMOTE_ADDR as the source of truth.

Comment 3

[Peter Bengtsson [:peterbe]]

PR https://github.com/mozilla/socorro/pull/1972

Comment 4

[Peter Bengtsson [:peterbe]]

Who or how would we be able to figure out if `HTTP_X_CLUSTER_CLIENT_IP` is a good header we can trust? (that's what kitsune is using)

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Peter Bengtsson [:peterbe] from comment #4)
> Who or how would we be able to figure out if `HTTP_X_CLUSTER_CLIENT_IP` is a
> good header we can trust? (that's what kitsune is using)

Any header can be spoofed by the client, though I might be misunderstanding which sense you mean "trust" in!

If you mean e.g. a header we can depend upon, I guess we should ask web ops (may phrawzty knows? I would like to know if there's a particular reason Kitsune chose that one over X-Forwarded-For.

Comment 6

[Daniel Maher [:phrawzty]]

(In reply to me from comment #2)
> * If you use X-Client-IP (or use a tproxy load balancer, or don't explicitly
> exclude setting X-Forwarded-For, or X-Forwarded-For is a list) this exposes
> a serious security issue.

Hi James,

Could you please expand on this statement?  Inquiring minds want to know. :)

Comment 7

[Daniel Maher [:phrawzty]]

(In reply to Robert Helmer [:rhelmer] from comment #5)
> (In reply to Peter Bengtsson [:peterbe] from comment #4)
> > Who or how would we be able to figure out if `HTTP_X_CLUSTER_CLIENT_IP` is a
> > good header we can trust? (that's what kitsune is using)
> 
> Any header can be spoofed by the client, though I might be misunderstanding
> which sense you mean "trust" in!
> 
> If you mean e.g. a header we can depend upon, I guess we should ask web ops
> (may phrawzty knows? I would like to know if there's a particular reason
> Kitsune chose that one over X-Forwarded-For.

I can't speak to Kitsune; however, I can tell you that X-Forwarded-For is what is used for most of the web properties at Mozilla, and that it functions as intended.  If the client passes that header in their request, Zeus will ignore it entirely, and write out its own X-Forwarded-For in the ensuing request to the web head.  In other words, this header cannot be successfully spoofed in our environment.

By way of an example test, this:
$ curl -H "X-Forwarded-For: 1.2.3.4" https://support-dev.allizom.org/

Produces this in the logs:
"[MY_ACTUAL_IP], 10.8.81.216" - - [01/Apr/2014:09:09:21 -0700] "GET / HTTP/1.1" 302 - "-" "curl/7.30.0"

If Zeus had honoured the header that I set, "1.2.3.4" would have appeared in the first field of that log entry instead of my actual IP.  (Yes, I realise this is SUMO - it was the first one-node cluster I could think of, heh).

Comment 8

[Peter Bengtsson [:peterbe]]

I think this is what James was referring to
https://docs.djangoproject.com/en/dev/releases/1.1/#removed-setremoteaddrfromforwardedfor-middleware

Our situation is different *thanks* to Zeus being slightly smarter.

Comment 9

[Peter Bengtsson [:peterbe]]

PR https://github.com/mozilla/socorro/pull/1976

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/socorro

https://github.com/mozilla/socorro/commit/d40843a7cfd34d3b4b172dc5962cf4f05bcef927
Merge pull request #1976 from peterbe/bug-989055-django-rate-limiting-should-honor-x-forwarded-for-header-2

fixes bug 989055 - django rate limiting should honor x-forwarded-for

Comment 11

[Peter Bengtsson [:peterbe]]

The necessary [config changes] are:

* In settings/local.py,
copy from settings/local.py-dist the last three lines::

base.MIDDLEWARE_CLASSES += (
    'crashstats.crashstats.middleware.SetRemoteAddrFromForwardedFor',
)

* You also need to change the 5th line and un-comment::

from . import base

Comment 12

[Peter Bengtsson [:peterbe]]

this is resolved. no need for input from James.