Skip to content

Provide blacklist functionality for IPs which have been rate-limited

stephen requested to merge block-ip-limiter-offenders into main

A conversation in #135 (closed) brought to light the fact that donate-neo is only rate-limiting connections but not blocklisting them, and that the ultimate intention is to have one result in the other, and that perhaps the question of where and how, precisely, the blocklist would be implemented was not fully hashed out before sitelaunch. This draft MR provides an implementation which fully implements the ratelimit-triggered blocklist, though it will require some light devops orchestration and a conversation about security to be production-ready.

The biggest problem with our work on keeping users from card-testing on donate-neo until now has, frankly, been with the particular way that django-ratelimit is built. It does not provide nearly any surface upon which to perform logging or generate metrics after a rate-limit event, and it has seemed to provide even less for the Django developer looking to pivot from the rate-limit event to anything more. A lot of effort has been put into merely logging against it and ensuring it uses the right IP address in the first place.

As it happens, there is a "proper" way to use django-ratelimit as the kickoff point for further action - it is "writing a second method decorator, and explicitly telling django-ratelimit decorator to hand the rate-limited request off to the method or decorator next in line." We did not do this for the simple reason that it would likely result in an entirely separate app to manage.

Enter separate app django-blacklist, which somehow does exactly what we need: it provides a decorator which explicitly accepts that django-ratelimit rate-limited request, and uses it as the impetus to blocklist the offending IP address.

Setting up django-blacklist is straightforward; there are a handful of settings to add to settings.py, most of which are on/off switches or which govern simple things, like how often to update the DB, or where the template for displaying the custom "You have been blocklisted" lives. The last setting is quite interesting, though - BLACKLIST_ADDRESS_SOURCE is intended to contain the location where django-blacklist should look for the actual IP address to block. According to the docs:

[This] can be a key in request.META, a callable that receives the request object, or the dotted string path to such a callable.

@anarcat wrote a clever IP address-retrieving utility in tordonate.util.get_client_ip, but passing BLACKLIST_ADDRESS_SOURCE that dotted string path threw a bunch of WSGIRequest errors, so it seems django-blacklist operates closer to the core of the server traffic than django-ratelimit.

I have added a second utility method, get_wsgi_client_ip, which attempts to address the differences between the two formats of HTTPRequest. It has worked well in my development environment through several test donations, but I'm not generating HTTP_X_FORWADED_FOR headers with my puny 127.0.0.1, so this is worth thorough re-testing on staging. (WSGIRequest is technically a subclass of HTTPRequest` but it manually re-implements the bits of its parent class it wants rather than calling "super", which is why it required a mild rewrite; fair warning that if you want to look deeper into this morass you are going to hit the Common Gateway Interface spec before too long, so pour yourself a big mug of something before venturing forth.)

I have re-implemented the rate-limiting on our main route, where the donate form lives, in the manner that django-blacklist requires. Rather than wrapping the URL route definition in a rate-limiter call in tordonate.urls, we instead stack decorators directly on top of tordonate.views.DonateFormView.

It's possible to decorate class methods directly, but we want to decorate DonateFormView's inherited get() method, and since our class only uses it implicitly, we instead use Django's method_decorator; this allows us to wrap our decorator call in a tuple whose pair is the name of the class method we target.

We can stack multiple @ratelimit decorators on top of one another to define differing rate-limiting qualifiers. To this end - and in response to some specific behavior we saw via metrics - I have renamed the settings variable RATE_LIMITING_RATE to RATE_LIMITING_RATE_SLOW and RATE_LIMITING_RATE_FAST, and set them to "10/m" and "50/h" respectively. This will block any traffic from one IP which meets either of these qualifiers.

Underneath them, the @blacklist_ratelimited decorator checks the request to see whether @ratelimit has marked them as having been rate-limited - if so, it adds the IP address to the Django database with the datetime.timedelta denoting how long the IP will have to wait to regain site access. (I set this to "ten years" by default.)

When a user is properly blocked, they see the contents of the blocklisted.html.jinja template returned with HTTP status code 400.

The most handy part of django-blacklist is probably the admin panel interface it provides. To showcase this functionality, I have reenabled the Django admin panel. (Now we're really in "mark as draft" territory.) A "Blacklist" category now appears in the left-hand sidebar, with a clickable "Rules" link. This link takes you to a page showing all current bans, the offending IPs, and the time remaining in the ban.

Since a need for importing the old blocklist was mentioned in #135 (closed), I should note that there is an interface to enter IP bans by hand - but also that IP bans are kept straightforwardly in Django's SQLite3 database and can likely be imported via script. I've generated an example here by banning myself:

sqlite> .mode column
sqlite> select * from blacklist_rule;
id  created                     updated                     address    prefixlen  duration  comments
                   user_id
--  --------------------------  --------------------------  ---------  ---------  --------  -------------------------------------------------------------------------------  -------
1   2024-09-25 21:18:05.727597  2024-09-25 21:18:05.727626  127.0.0.1             60000000  Automatically blacklisted ratelimited client.
Request ID:
Request line: GET /

Now - all of this is well and good - but it still involves reverting our decision to forego having a live admin panel on donate-neo. This would raise additional security concerns that we haven't discussed at all as a working group - and, yes, this is one massive reason this MR is a draft.

I've pre-emptively made one proactive change towards securing the admin panel - the environment variable DJANGO_ADMIN_PATH is now used to define the path of the admin login page, defaulting to /donate-admin/ if no such environment variable is found. (This is not a particularly obfuscated default path, but it is better than the actual Django default, /admin/, and I'm not sure anything would actually be good to use as a default here, given that this codebase is open-source - maybe a random string?)

Locking access to that endpoint to an allowlist of IPs on a server level, if it's something that folks have the capability of doing, would be ideal. There is also a django-two-factor-auth project which enables 2FA on the admin form, and even has Yubikey support - that lives here: https://github.com/jazzband/django-two-factor-auth - but solutions on this level feel riper for a group conversation among wise sysadmins rather than the speculations of a developer up past his bedtime :)

There are still a handful of limitations presented by this approach, some of which rub up against known desired features, so I'll quickly mention what I've noticed so far:

  • The use of decorators as the way to pipe the rate-limit event to the blocklist handler means that we can't rate-limit any of the async methods, at least not without writing handlers for the two third-party app decorators to make them async-friendly. Unfortunately, most of the routes where users submit form data - which could be used to rate-limit against specific form fields containing the same data - have async view methods.
    • I do have an implementation in mind that would get closer - we could rate-limit /validate/. Unfortunately, it only receives one POST variable: a long query-string-encoded representation of the form's contents. However, there are several examples in django-ratelimit's documentation which show that the value of key - normally ip or post:email_address or something, can be substituted with a lambda function instead. (See https://django-ratelimit.readthedocs.io/en/stable/usage.html#examples for a few.) Maybe this is just the JavaScript goblin in me, but perhaps we could pull the form data we'd like out of that query string in the lambda function, and rate-limit against individual repeated form values that way.
    • I believe it's also possible to write a wrapper for a decorator to make it async-compatible, but I'd have to do separate research into the topic - I consciously avoided diving any deeper than "it's probably possible?" in order to put the rest of this draft together.
  • Rendering both the rate-limit template and the blocklist template appears to be necessary for metrics generation and logging to occur on both occasions; if the template contents differ, then a blocklisted user will notice the change from the first to the second. I'm of the opinion that we should offer as little insight to attackers as possible, so that's a little disappointing to me as the out-of-the-box behavior.
    • For now, I've updated the RateLimiter response content to match the django-blacklist repsonse content, but it's not my favorite; django-blacklist will only return a 400 and I preferred returning a 429, but I guess I can live with that if the behavior is consistent in both cases.

I think that's everything! Thoughts, comments, concerns, et al., are welcome and encouraged.

Merge request reports