Skip to content

[Security] Added login throttling feature #38204

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 17, 2020
Merged

[Security] Added login throttling feature #38204

merged 1 commit into from
Sep 17, 2020

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Sep 15, 2020 •
edited
Loading

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 (commits are included in this branch atm).

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

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

@wouterj wouterj requested a review from jderusse as a code owner September 15, 2020 19:37
@wouterj wouterj mentioned this pull request Sep 15, 2020
Merged
4 tasks
@wouterj wouterj force-pushed the rate-limiter-security branch 6 times, most recently from 730a56d to 586894b Compare September 16, 2020 12:54
weaverryan
weaverryan reviewed Sep 16, 2020
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice :).

Due to the architecture, I understand why there is config needed under framework and security. However, could we avoid this? From a user's perspective, I think:

I need login throttling!

And I would expect that I can enable that easily in security.yaml. Could we allow the user to create a "rate limiter" inline in the security config? If we need to, we could make it less configurable (and if you need a more configurable limiter, then you could define it properly under framework and reference that limiter from here).

@wouterj wouterj force-pushed the rate-limiter-security branch 5 times, most recently from df93b39 to 477ab43 Compare September 16, 2020 14:14
@wouterj wouterj force-pushed the rate-limiter-security branch from 477ab43 to a039c10 Compare September 16, 2020 15:37
@wouterj
Copy link
Member Author

wouterj commented Sep 16, 2020

This PR is now ready to merge imho.

Based on suggestions from @weaverryan, the SecurityBundle now configures a "default" limiter for login throttling. A user can override this by configuring the limiter setting. This required some changes in the RateLimiter configuration - which is probably now a bit easier to understand and less magic than before.

@wouterj wouterj force-pushed the rate-limiter-security branch from a039c10 to e8981b2 Compare September 16, 2020 15:56
fabpot
fabpot approved these changes Sep 16, 2020
View reviewed changes
@wouterj wouterj force-pushed the rate-limiter-security branch from e8981b2 to c75533c Compare September 16, 2020 16:14
javiereguiluz
javiereguiluz reviewed Sep 16, 2020
View reviewed changes
@wouterj wouterj force-pushed the rate-limiter-security branch from c75533c to 94340c1 Compare September 16, 2020 19:16
@fabpot fabpot force-pushed the rate-limiter-security branch from 94340c1 to afdd805 Compare September 17, 2020 05:36
@fabpot
Copy link
Member

fabpot commented Sep 17, 2020

Thank you @wouterj.

@fabpot fabpot mentioned this pull request Sep 17, 2020
Closed
@fabpot fabpot merged commit 237d91f into symfony:master Sep 17, 2020
@wouterj wouterj deleted the rate-limiter-security branch September 17, 2020 11:00
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@bobvandevijver bobvandevijver mentioned this pull request Oct 10, 2020
Merged
fabpot added a commit that referenced this pull request Oct 11, 2020
@fabpot
bug #38507 [Bug] Fix RateLimiter framework configuration (bobvandevij…
d1f2faf 
…ver)

This PR was merged into the 5.x branch.

Discussion
----------

[Bug] Fix RateLimiter framework configuration

| Q             | A
| ------------- | ---
| Branch?       | 5.x for features <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |  - <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | -

Small mistake in the rate limiter configuration, instead of unsetting the `storage_service` option the never existing `storage` option was unset, resulting into an application error when trying to use a Limiter in your application.

The exception was:
```
Uncaught PHP Exception: The option "storage_service" does not exist. Defined options are: "id", "interval", "limit", "rate", "strategy"."
```

This was introduced in #38204, so a highlight for @wouterj to check this :)

Commits
-------

b360320 [Bug] Fix RateLimiter framework configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@javiereguiluz javiereguiluz javiereguiluz left review comments

@weaverryan weaverryan weaverryan left review comments

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Login throttling
5 participants
@wouterj @fabpot @javiereguiluz @weaverryan @carsonbot