Skip to content

[POC][Security] Added basic login throttling feature #37444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jun 28, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37266
License MIT
Doc PR tbd
security:
  firewalls:
    main:
      form_login: ~
      login_throttling:
        threshold: 3    # max number of allowed failed attempts
        lock_timeout: 1 # minutes until it is possible to login again

This currently only works for invalid passwords (not invalid usernames) due to a limitation in the new security system (see #37436 for an RFC to fix this).

@wouterj wouterj force-pushed the issue-37266/login-throttling branch from 3be6ce1 to 8fe76ea Compare June 28, 2020 18:48
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 28, 2020

Thanks for the PR. Is this effective at all? I mean: it's trivial to drop the session cookie between login attempts, effectively bypassing any throttling.

@wouterj
Copy link
Member Author

wouterj commented Jun 28, 2020

This uses the Symfony cache component, to prevent relying on the session (at least, that's what I tried to do). It's heavily inspired by Laravel's login throttling system.
An alternative would be to add a storage abstraction, but I wanted to keep it as simple as possible (but it's a POC exactly because I'm not sure how we best store locked sessions)

@simonberger
Copy link
Contributor

simonberger commented Jun 28, 2020

The throttling is fairly hard integrated into the login component. Considering just a few lines are needed for the core throttling code I don't know if it is worth it to make parts of it reusable, but I can think of many possible uses of the feature.
A nice follow up task would be for example to integrate this in the password reset functionality of the maker-bundle as well (mainly to prevent spamming or mail server attacks).

@javiereguiluz
Copy link
Member

Somewhat related to @simonberger comment ... what if we consider instead creating a "RateLimiter" component?

These are popular in comparable projects (e.g. is native in Go: https://pkg.go.dev/golang.org/x/time/rate; it's available for ExpressJS: https://www.npmjs.com/package/express-rate-limit; even the "login throttling" feature in Laravel uses a RateLimiter for the actual implementation).

If this component existed, then we don't need to add "login throttling" to the security component. Instead, developers needing this would limit the number of accesses to the /login path in a given period of time. It would also be very useful in APIs (e.g. API Platform) to restrict the number of requests to certain API paths.

@wouterj
Copy link
Member Author

wouterj commented Jun 29, 2020

I like the idea of creating an abstraction of the throttling behavior as a RateLimiting component. This can be used in a request listener to implement complex API rate limits and also in a security listener to add login throttling.

This will require reading a bit more about rate limiting functionalities and implementations. If anyone already has this knowledge and wants to contribute, feel free to do so in a new PR - I'll close this one then. Otherwise, I'll try to implement something in some weeks time. Unfortunately, GitHub doesn't allow me to switch PRs to a "draft" state.

@javiereguiluz
Copy link
Member

If this idea is considered, we may use the "RateLimiter" feature in other components if it makes sense: HttpClient (to limit the number of request to some or any host?); Messenger (to limit the number of messages sent/delivered or generated?).

@wouterj
Copy link
Member Author

wouterj commented Jul 9, 2020

Closing in favor of #37546

@wouterj wouterj closed this Jul 9, 2020
@wouterj wouterj deleted the issue-37266/login-throttling branch July 9, 2020 23:05
fabpot added a commit that referenced this pull request Sep 16, 2020
This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[RFC] Introduce a RateLimiter component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Refs #37444
| License       | MIT
| Doc PR        | tbd

Based on the discussions in #37444, I decided to write a general purpose RateLimiter component. This implementation uses the token bucket algorithm, inspired by the [Go's time/rate](https://github.com/golang/time/blob/3af7569d3a1e776fc2a3c1cec133b43105ea9c2e/rate/rate.go) library and the [PHP `bandwidth-throttle/token-bucket` package](https://github.com/bandwidth-throttle/token-bucket) (which is [unmaintained for years](bandwidth-throttle/token-bucket#19)).

### Usage

The component has two main methods:

* `Limiter::reserve(int $tokens, int $maxTime)`, allocates `$tokens` and returns a `Reservation` containing the wait time. Use this method if your process wants to wait before consuming the token.
* `Limiter::consume(int $tokens)`, checks if `$tokens` are available now and discards the reservation if that's not the case. Use this method if you want to skip when there are not enough tokens at this moment.

The component uses the Lock component to make sure it can be used in parallel processes.

Example:

```php
<?php

namespace App\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\RateLimiter\LimiterFactory;

class LimitListener
{
    private $limiterFactory;

    public function __construct(LimiterFactory $apiLimiterFactory)
    {
        $this->limiterFactory = $apiLimiterFactory;
    }

    public function __invoke(RequestEvent $event)
    {
        $ip = $event->getRequest()->getClientIp();

        $limiter = $this->limiterFactory->createLimiter(preg_replace('/[^a-zA-Z0-9]/', '-', $ip));
        if (!$limiter->consume()) {
            $event->setResponse(new Response('Too many requests', 429));
        }
    }
}
```

### Usefullness of the component

I think a generic rate limiter is usefull in quite some places:

* Add a login throttling feature in Symfony
* <s>Rate limiting outgoing API calls (e.g. HttpClient), to prevent hitting upstream API limits.</s> See #37471 (and https://blog.heroku.com/rate-throttle-api-client )
* Allowing users to easily implement API rate limits in their own Symfony-based APIs.

### State of the art

There are some rate limiting packages in PHP, but I think there is no precendent for this component:

* [`graham-campbell/throttle`](https://github.com/GrahamCampbell/Laravel-Throttle) is heavily relying on Laravel. It is however very popular, proofing there is a need for such feature
* [`nikolaposa/rate-limit`](https://github.com/nikolaposa/rate-limit) does not implement reservation of tokens and as such less feature complete. Also its architecture combines the rate limiter and storage, making it harder to implement different storages.

### Todo

If it is agreed that this component can bring something to Symfony, it needs some more love:

* [x] Add more tests
* [x] Integrate with the FrameworkBundle
* [x] Add sliding window implementation
* [x] Add integration with the Security component
* <s>Maybe add more storage implementations? I didn't want to duplicate storage functionalities already existing in the Lock and Cache component, thus I for now focused mostly on integrating the Cache adapters. But maybe a special Doctrine adapter makes sense?</s>

Commits
-------

67417a6 [RFC] Introduce a RateLimiter component
fabpot added a commit that referenced this pull request Sep 17, 2020
This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[Security] Added login throttling feature

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #37266
| License       | MIT
| Doc PR        | tbd

This "recreates" #37444 based on the RateLimiter component from #37546 <s>(commits are included in this branch atm)</s>.

Login throttling can be enabled on any user-based authenticator (thanks to the `UserBadge`) with this configuration:

```yaml
security:
    firewalls:
        default:
            # default limits to 5 login attempts per minute, the number can be configured via "max_attempts"
            login_throttling: ~

            # or you can define your own RateLimiter on framework.rate_limiter and configure it instead:
            login_throttling:
                limiter: login
```

Commits
-------

afdd805 [Security] Added login throttling feature
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login throttling
5 participants