diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php index 81d2a1c3c3c2b..80d31a6dce1b6 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Security/Factory/LoginThrottlingFactory.php @@ -17,8 +17,10 @@ use Symfony\Component\DependencyInjection\ChildDefinition; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Reference; +use Symfony\Component\HttpFoundation\RateLimiter\RequestRateLimiterInterface; use Symfony\Component\RateLimiter\Limiter; use Symfony\Component\Security\Http\EventListener\LoginThrottlingListener; +use Symfony\Component\Security\Http\RateLimiter\DefaultLoginRateLimiter; /** * @author Wouter de Jong @@ -50,7 +52,7 @@ public function addConfiguration(NodeDefinition $builder) { $builder ->children() - ->scalarNode('limiter')->info('The name of the limiter that you defined under "framework.rate_limiter".')->end() + ->scalarNode('limiter')->info(sprintf('A service id implementing "%s".', RequestRateLimiterInterface::class))->end() ->integerNode('max_attempts')->defaultValue(5)->end() ->end(); } @@ -70,18 +72,27 @@ public function createAuthenticator(ContainerBuilder $container, string $firewal throw new \LogicException('You must either configure a rate limiter for "security.firewalls.'.$firewallName.'.login_throttling" or install symfony/framework-bundle:^5.2.'); } - FrameworkExtension::registerRateLimiter($container, $config['limiter'] = '_login_'.$firewallName, [ + $limiterOptions = [ 'strategy' => 'fixed_window', 'limit' => $config['max_attempts'], 'interval' => '1 minute', 'lock_factory' => 'lock.factory', 'cache_pool' => 'cache.app', - ]); + ]; + FrameworkExtension::registerRateLimiter($container, $localId = '_login_local_'.$firewallName, $limiterOptions); + + $limiterOptions['limit'] = 5 * $config['max_attempts']; + FrameworkExtension::registerRateLimiter($container, $globalId = '_login_global_'.$firewallName, $limiterOptions); + + $container->register($config['limiter'] = 'security.login_throttling.'.$firewallName.'.limiter', DefaultLoginRateLimiter::class) + ->addArgument(new Reference('limiter.'.$globalId)) + ->addArgument(new Reference('limiter.'.$localId)) + ; } $container ->setDefinition('security.listener.login_throttling.'.$firewallName, new ChildDefinition('security.listener.login_throttling')) - ->replaceArgument(1, new Reference('limiter.'.$config['limiter'])) + ->replaceArgument(1, new Reference($config['limiter'])) ->addTag('kernel.event_subscriber', ['dispatcher' => 'security.event_dispatcher.'.$firewallName]); return []; diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php index 4ba3735153561..3d0c6ddcb4f9e 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php @@ -118,7 +118,7 @@ ->abstract() ->args([ service('request_stack'), - abstract_arg('rate limiter'), + abstract_arg('request rate limiter'), ]) // Authenticators diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig index a137e0cb849ad..34ea19f2bde62 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/Bundle/FormLoginBundle/Resources/views/Login/login.html.twig @@ -4,6 +4,7 @@ {% if error %}
{{ error.messageKey }}
+
{{ error.messageKey|replace(error.messageData) }}
{% endif %}
diff --git a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php index c527935e00cba..f49cfa84c07e1 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/FormLoginTest.php @@ -108,7 +108,10 @@ public function testFormLoginRedirectsToProtectedResourceAfterLogin(array $optio $this->assertStringContainsString('You\'re browsing to path "/protected_resource".', $text); } - public function testLoginThrottling() + /** + * @dataProvider provideInvalidCredentials + */ + public function testLoginThrottling($username, $password) { if (!class_exists(LoginThrottlingListener::class)) { $this->markTestSkipped('Login throttling requires symfony/security-http:^5.2'); @@ -117,17 +120,23 @@ public function testLoginThrottling() $client = $this->createClient(['test_case' => 'StandardFormLogin', 'root_config' => 'login_throttling.yml', 'enable_authenticator_manager' => true]); $form = $client->request('GET', '/login')->selectButton('login')->form(); - $form['_username'] = 'johannes'; - $form['_password'] = 'wrong'; + $form['_username'] = $username; + $form['_password'] = $password; $client->submit($form); $client->followRedirect()->selectButton('login')->form(); - $form['_username'] = 'johannes'; - $form['_password'] = 'wrong'; + $form['_username'] = $username; + $form['_password'] = $password; $client->submit($form); $text = $client->followRedirect()->text(null, true); - $this->assertStringContainsString('Too many failed login attempts, please try again later.', $text); + $this->assertStringContainsString('Too many failed login attempts, please try again in 1 minute.', $text); + } + + public function provideInvalidCredentials() + { + yield 'invalid_password' => ['johannes', 'wrong']; + yield 'invalid_username' => ['wrong', 'wrong']; } public function provideClientOptions() diff --git a/src/Symfony/Component/HttpFoundation/CHANGELOG.md b/src/Symfony/Component/HttpFoundation/CHANGELOG.md index babb87c2b70ed..277efc4fae28a 100644 --- a/src/Symfony/Component/HttpFoundation/CHANGELOG.md +++ b/src/Symfony/Component/HttpFoundation/CHANGELOG.md @@ -8,6 +8,7 @@ CHANGELOG * added `HeaderUtils::parseQuery()`: it does the same as `parse_str()` but preserves dots in variable names * added `File::getContent()` * added ability to use comma separated ip addresses for `RequestMatcher::matchIps()` + * added `RateLimiter\RequestRateLimiterInterface` and `RateLimiter\AbstractRequestRateLimiter` 5.1.0 ----- diff --git a/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php b/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php new file mode 100644 index 0000000000000..430c150ca0934 --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php @@ -0,0 +1,59 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpFoundation\RateLimiter; + +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\RateLimiter\Limit; +use Symfony\Component\RateLimiter\LimiterInterface; +use Symfony\Component\RateLimiter\NoLimiter; + +/** + * An implementation of RequestRateLimiterInterface that + * fits most use-cases. + * + * @author Wouter de Jong + * + * @experimental in Symfony 5.2 + */ +abstract class AbstractRequestRateLimiter implements RequestRateLimiterInterface +{ + public function consume(Request $request): Limit + { + $limiters = $this->getLimiters($request); + if (0 === \count($limiters)) { + $limiters = [new NoLimiter()]; + } + + $minimalLimit = null; + foreach ($limiters as $limiter) { + $limit = $limiter->consume(1); + + if (null === $minimalLimit || $limit->getRemainingTokens() < $minimalLimit->getRemainingTokens()) { + $minimalLimit = $limit; + } + } + + return $minimalLimit; + } + + public function reset(): void + { + foreach ($this->getLimiters($request) as $limiter) { + $limiter->reset(); + } + } + + /** + * @return LimiterInterface[] a set of limiters using keys extracted from the request + */ + abstract protected function getLimiters(Request $request): array; +} diff --git a/src/Symfony/Component/HttpFoundation/RateLimiter/RequestRateLimiterInterface.php b/src/Symfony/Component/HttpFoundation/RateLimiter/RequestRateLimiterInterface.php new file mode 100644 index 0000000000000..21d7637710ba8 --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/RateLimiter/RequestRateLimiterInterface.php @@ -0,0 +1,32 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpFoundation\RateLimiter; + +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\RateLimiter\Limit; + +/** + * A special type of limiter that deals with requests. + * + * This allows to limit on different types of information + * from the requests. + * + * @author Wouter de Jong + * + * @experimental in Symfony 5.2 + */ +interface RequestRateLimiterInterface +{ + public function consume(Request $request): Limit; + + public function reset(): void; +} diff --git a/src/Symfony/Component/RateLimiter/NoLimiter.php b/src/Symfony/Component/RateLimiter/NoLimiter.php index 3dfbabac0fbb8..b5f5d5f68c4c6 100644 --- a/src/Symfony/Component/RateLimiter/NoLimiter.php +++ b/src/Symfony/Component/RateLimiter/NoLimiter.php @@ -25,7 +25,7 @@ final class NoLimiter implements LimiterInterface { public function consume(int $tokens = 1): Limit { - return new Limit(\INF, new \DateTimeImmutable(), true, 'no_limit'); + return new Limit(\INF, new \DateTimeImmutable(), true); } public function reset(): void diff --git a/src/Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php b/src/Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php index 297db1580b2b3..74696b22da873 100644 --- a/src/Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php +++ b/src/Symfony/Component/Security/Core/Exception/TooManyLoginAttemptsAuthenticationException.php @@ -19,11 +19,46 @@ */ class TooManyLoginAttemptsAuthenticationException extends AuthenticationException { + private $threshold; + + public function __construct(int $threshold = null) + { + $this->threshold = $threshold; + } + + /** + * {@inheritdoc} + */ + public function getMessageData(): array + { + return [ + '%minutes%' => $this->threshold, + ]; + } + /** * {@inheritdoc} */ public function getMessageKey(): string { - return 'Too many failed login attempts, please try again later.'; + return 'Too many failed login attempts, please try again '.($this->threshold ? 'in %minutes% minute'.($this->threshold > 1 ? 's' : '').'.' : 'later.'); + } + + /** + * {@inheritdoc} + */ + public function __serialize(): array + { + return [$this->threshold, parent::__serialize()]; + } + + /** + * {@inheritdoc} + */ + public function __unserialize(array $data): void + { + [$this->threshold, $parentData] = $data; + $parentData = \is_array($parentData) ? $parentData : unserialize($parentData); + parent::__unserialize($parentData); } } diff --git a/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php b/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php index 85b46733bf868..2b4954d71a538 100644 --- a/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php +++ b/src/Symfony/Component/Security/Http/EventListener/LoginThrottlingListener.php @@ -12,13 +12,12 @@ namespace Symfony\Component\Security\Http\EventListener; use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\RateLimiter\RequestRateLimiterInterface; use Symfony\Component\HttpFoundation\RequestStack; -use Symfony\Component\RateLimiter\Limiter; use Symfony\Component\Security\Core\Exception\TooManyLoginAttemptsAuthenticationException; +use Symfony\Component\Security\Core\Security; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\Security\Http\Event\CheckPassportEvent; -use Symfony\Component\Security\Http\Event\LoginSuccessEvent; /** * @author Wouter de Jong @@ -30,7 +29,7 @@ final class LoginThrottlingListener implements EventSubscriberInterface private $requestStack; private $limiter; - public function __construct(RequestStack $requestStack, Limiter $limiter) + public function __construct(RequestStack $requestStack, RequestRateLimiterInterface $limiter) { $this->requestStack = $requestStack; $this->limiter = $limiter; @@ -44,33 +43,18 @@ public function checkPassport(CheckPassportEvent $event): void } $request = $this->requestStack->getMasterRequest(); - $username = $passport->getBadge(UserBadge::class)->getUserIdentifier(); - $limiterKey = $this->createLimiterKey($username, $request); + $request->attributes->set(Security::LAST_USERNAME, $passport->getBadge(UserBadge::class)->getUserIdentifier()); - $limiter = $this->limiter->create($limiterKey); - if (!$limiter->consume()->isAccepted()) { - throw new TooManyLoginAttemptsAuthenticationException(); + $limit = $this->limiter->consume($request); + if (!$limit->isAccepted()) { + throw new TooManyLoginAttemptsAuthenticationException(ceil(($limit->getRetryAfter()->getTimestamp() - time()) / 60)); } } - public function onSuccessfulLogin(LoginSuccessEvent $event): void - { - $limiterKey = $this->createLimiterKey($event->getAuthenticatedToken()->getUsername(), $event->getRequest()); - $limiter = $this->limiter->create($limiterKey); - - $limiter->reset(); - } - public static function getSubscribedEvents(): array { return [ - CheckPassportEvent::class => ['checkPassport', 64], - LoginSuccessEvent::class => 'onSuccessfulLogin', + CheckPassportEvent::class => ['checkPassport', 2080], ]; } - - private function createLimiterKey($username, Request $request): string - { - return $username.$request->getClientIp(); - } } diff --git a/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php b/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php new file mode 100644 index 0000000000000..bf7735bc369b9 --- /dev/null +++ b/src/Symfony/Component/Security/Http/RateLimiter/DefaultLoginRateLimiter.php @@ -0,0 +1,47 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Http\RateLimiter; + +use Symfony\Component\HttpFoundation\RateLimiter\AbstractRequestRateLimiter; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\RateLimiter\Limiter; +use Symfony\Component\Security\Core\Security; + +/** + * A default login throttling limiter. + * + * This limiter prevents breadth-first attacks by enforcing + * a limit on username+IP and a (higher) limit on IP. + * + * @author Wouter de Jong + * + * @experimental in Symfony 5.2 + */ +final class DefaultLoginRateLimiter extends AbstractRequestRateLimiter +{ + private $globalLimiter; + private $localLimiter; + + public function __construct(Limiter $globalLimiter, Limiter $localLimiter) + { + $this->globalLimiter = $globalLimiter; + $this->localLimiter = $localLimiter; + } + + protected function getLimiters(Request $request): array + { + return [ + $this->globalLimiter->create($request->getClientIp()), + $this->localLimiter->create($request->attributes->get(Security::LAST_USERNAME).$request->getClientIp()), + ]; + } +} diff --git a/src/Symfony/Component/Security/Http/Tests/EventListener/LoginThrottlingListenerTest.php b/src/Symfony/Component/Security/Http/Tests/EventListener/LoginThrottlingListenerTest.php index 19fa57f04394c..28af3713fca18 100644 --- a/src/Symfony/Component/Security/Http/Tests/EventListener/LoginThrottlingListenerTest.php +++ b/src/Symfony/Component/Security/Http/Tests/EventListener/LoginThrottlingListenerTest.php @@ -24,6 +24,7 @@ use Symfony\Component\Security\Http\Event\CheckPassportEvent; use Symfony\Component\Security\Http\Event\LoginSuccessEvent; use Symfony\Component\Security\Http\EventListener\LoginThrottlingListener; +use Symfony\Component\Security\Http\RateLimiter\DefaultLoginRateLimiter; class LoginThrottlingListenerTest extends TestCase { @@ -34,17 +35,24 @@ protected function setUp(): void { $this->requestStack = new RequestStack(); - $limiter = new Limiter([ + $localLimiter = new Limiter([ 'id' => 'login', 'strategy' => 'fixed_window', 'limit' => 3, 'interval' => '1 minute', ], new InMemoryStorage()); + $globalLimiter = new Limiter([ + 'id' => 'login', + 'strategy' => 'fixed_window', + 'limit' => 6, + 'interval' => '1 minute', + ], new InMemoryStorage()); + $limiter = new DefaultLoginRateLimiter($globalLimiter, $localLimiter); $this->listener = new LoginThrottlingListener($this->requestStack, $limiter); } - public function testPreventsLoginWhenOverThreshold() + public function testPreventsLoginWhenOverLocalThreshold() { $request = $this->createRequest(); $passport = $this->createPassport('wouter'); @@ -59,21 +67,19 @@ public function testPreventsLoginWhenOverThreshold() $this->listener->checkPassport($this->createCheckPassportEvent($passport)); } - public function testSuccessfulLoginResetsCount() + public function testPreventsLoginWhenOverGlobalThreshold() { - $this->expectNotToPerformAssertions(); - $request = $this->createRequest(); - $passport = $this->createPassport('wouter'); + $passports = [$this->createPassport('wouter'), $this->createPassport('ryan')]; $this->requestStack->push($request); - for ($i = 0; $i < 3; ++$i) { - $this->listener->checkPassport($this->createCheckPassportEvent($passport)); + for ($i = 0; $i < 6; ++$i) { + $this->listener->checkPassport($this->createCheckPassportEvent($passports[$i % 2])); } - $this->listener->onSuccessfulLogin($this->createLoginSuccessfulEvent($passport)); - $this->listener->checkPassport($this->createCheckPassportEvent($passport)); + $this->expectException(TooManyLoginAttemptsAuthenticationException::class); + $this->listener->checkPassport($this->createCheckPassportEvent($passports[0])); } private function createPassport($username) diff --git a/src/Symfony/Component/Security/Http/composer.json b/src/Symfony/Component/Security/Http/composer.json index aff757cee05fb..20c310e741257 100644 --- a/src/Symfony/Component/Security/Http/composer.json +++ b/src/Symfony/Component/Security/Http/composer.json @@ -19,7 +19,7 @@ "php": ">=7.2.5", "symfony/deprecation-contracts": "^2.1", "symfony/security-core": "^5.2", - "symfony/http-foundation": "^4.4.7|^5.0.7", + "symfony/http-foundation": "^5.2", "symfony/http-kernel": "^5.2", "symfony/polyfill-php80": "^1.15", "symfony/property-access": "^4.4|^5.0"