Django-rest-framework: Request Throttler before authentication?

[cristianocca]

Is it possible to have the request throttling code to run before running authentication? Or have it run even if authentication fails?

Current scenario: Endpoint with authentication and throttling enabled, endpoint can still be abused with unauthenticated requests since it will skip throttling. This might not seem important, but if authentication is something like basic or api key, someone could technically brute force that endpoint if it doesn't have throttling until it gets a response.

[tomchristie]

Not really no - in general we need the request to be authenticated before throttling so that we can apply per-user throttling if needed. You can bake the behavior into the authentication class if you want and raise an exception if authentication is called too often, that's probably the best approach.

[dabrowne]

This seems like a pretty big potential vulnerability IMO as it basically leaves endpoints open to having auth credentials brute forced, which is surely one of the major use cases for throttling. I'd expect that either this behavior would be explicitly documented, or could be changed. I don't understand the argument that you need an authenticated user to apply throttling because surely in the case that auth fails, you can still fall back to an anonymous throttle.

I think this could be fixed by replacing:
https://github.com/encode/django-rest-framework/blob/fe54575e6a0b1abc43a84814cc1b8625e6187a8b/rest_framework/views.py#L387-L390

With:

try:
    self.perform_authentication(request)
except exceptions.AuthenticationFailed:
    self.check_throttles(request)
    raise

self.check_permissions(request)
self.check_throttles(request)

@tomchristie Is your stance on this still the same? I'm happy to PR either some extra documentation or the change suggested above.

[rpkilby]

I'm reopening to make sure this issue is addressed by someone with more knowledge in this area. My off the cuff response is that @dabrowne is correct. Throttling is per-user, but does seem to take into account anonymous users. See:

https://github.com/encode/django-rest-framework/blob/fe54575e6a0b1abc43a84814cc1b8625e6187a8b/rest_framework/throttling.py#L185-L204

[xordoquy]

Anonymous user is different from failed authentication. Former means "I don't have credentials" while latter is "I didn't use valid credential - though I did use some".
I think @dabrowne got a valid point here, thanks for bringing it up.

[dabrowne]

I've since done a bit of testing and I realize that my proposed solution is unfortunately naive:

It doesn't actually stop brute force attacks for throttles that use different keys for authenticated and unauthenticated users (i.e. all of the out-of-the-box throttle classes). Requests with invalid auth _will_ get a 429 response once throttled, however as soon as an attacker guesses a correct set of credentials, the request will be allowed as the throttle key will change due to the successful authentication.

The only real way to protect against this sort of attack would be to use a throttle that is always 'anonymous' (i.e. identified by IP address) in combination with the code I suggested above. Otherwise as initially suggested by @tomchristie, it could be baked into the actual authentication class.

[dabrowne]

I got around this by implementing a throttling mixin for Authentication classes, largely derived from the throttles codebase:

import time

from django.core.cache import cache as default_cache
from django.core.exceptions import ImproperlyConfigured

from rest_framework.settings import api_settings
from rest_framework.exceptions import Throttled, AuthenticationFailed


class ThrottledAuthenticationMixin(object):
    """
    Mixin for authentication classes that will throttle clients that make
    too many requests which fail authentication.

    By default clients are identified by IP address. This behavior can be
    changed by overriding `get_ident`, much like standard Throttle classes.

    """
    cache = default_cache
    timer = time.time
    cache_format = 'throttle_%(scope)s_%(ident)s'
    rate = None

    THROTTLE_RATES = api_settings.DEFAULT_THROTTLE_RATES

    @property
    def scope(self):
        return self.__class__.__name__

    def __init__(self):
        if self.rate is None:
            self.rate = self.get_rate()

        self.num_requests, self.duration = self.parse_rate(self.rate)

    def get_rate(self):
        """
        Determine the string representation of the allowed request rate.
        """
        try:
            return self.THROTTLE_RATES[self.scope]
        except KeyError:
            msg = "No default throttle rate set for '%s' scope" % self.scope
            raise ImproperlyConfigured(msg)

    def parse_rate(self, rate):
        """
        Given the request rate string, return a two tuple of:
        <allowed number of requests>, <period of time in seconds>
        """
        if rate is None:
            return (None, None)
        num, period = rate.split('/')
        num_requests = int(num)
        duration = {'s': 1, 'm': 60, 'h': 3600, 'd': 86400}[period[0]]
        return (num_requests, duration)

    def get_ident(self, request):
        """
        Identify the machine making the request by parsing HTTP_X_FORWARDED_FOR
        if present and number of proxies is > 0. If not use all of
        HTTP_X_FORWARDED_FOR if it is available, if not use REMOTE_ADDR.
        """
        xff = request.META.get('HTTP_X_FORWARDED_FOR')
        remote_addr = request.META.get('REMOTE_ADDR')
        num_proxies = api_settings.NUM_PROXIES

        if num_proxies is not None:
            if num_proxies == 0 or xff is None:
                return remote_addr
            addrs = xff.split(',')
            client_addr = addrs[-min(num_proxies, len(addrs))]
            return client_addr.strip()

        return ''.join(xff.split()) if xff else remote_addr

    def get_cache_key(self, request):
        return self.cache_format % {
            'scope': self.scope,
            'ident': self.get_ident(request)
        }

    def authenticate(self, request):
        self.initialize_request(request)

        if self.throttle_is_active:
            raise Throttled(wait=self._wait())

        try:
            return super().authenticate(request)
        except AuthenticationFailed:
            self._authentication_failed()
            raise

    def initialize_request(self, request):
        self.key = self.get_cache_key(request)
        self.now = self.timer()
        self.history = self.cache.get(self.key, [])

        # Drop any requests from the history which have now passed the throttle duration
        history_truncated = False
        while self.history and self.history[-1] <= self.now - self.duration:
            self.history.pop()
            history_truncated = True

        # Update cache with truncated history
        if history_truncated:
            self.cache.set(self.key, self.history, timeout=self.duration)

        self.throttle_is_active = len(self.history) >= self.num_requests

    def _authentication_failed(self):
        """
        Add an entry to the throttle history when authentication fails
        """
        # Reduce the chance of race condition by re-querying the history
        self.history = self.cache.get(self.key, [])
        self.history.insert(0, self.now)
        self.cache.set(self.key, self.history, timeout=self.duration)

    def _wait(self):
        """
        Returns the recommended next request time in seconds.
        """
        if self.history:
            remaining_duration = self.duration - (self.now - self.history[-1])
        else:
            remaining_duration = self.duration

        available_requests = self.num_requests - len(self.history) + 1
        if available_requests <= 0:
            return None

        return remaining_duration / float(available_requests)

Which can be applied like this:

class ThrottledBasicAuthentication(ThrottledAuthenticationMixin, BasicAuthentication):
    rate = '10/min'

[cristianocca]

Why not just override SimpleRateThrottle instead? You would just need to define the get_cache_key method and the authenticate with the same code you used here but with a call to allow_request instead.

Edit: I see that you would also need to switch the behavior of throttle_success and throttle_failure so failures are throttled instead

[dabrowne]

I did think about subclassing SimpleRateThrottle, but as you've noted the process for throttling is slightly different and I didn't want a bunch of inherited methods hanging around that don't make sense or would be broken if someone tried to use them.

[tomchristie]

I'd strongly suggest a custom auth class for preventing multiple authentication attempts against a single user, since there's a different set of requirements vs. throttling (eg. block for X minutes once Y different incorrect authentication attempts are made) which doesn't fit in with the rest of the throttling API.

API keys are less susceptible to brute forcing than user/password (which in our BasicAuthentication implementation get's Django's default hashing behavior as a protection mechanism.