Auth: Add setting to disable username based brute force login protection #109152
Conversation
TheoBrigitte
requested review from
a team,
irenerl24 and
torkelo
as code owners
TheoBrigitte
requested review from
cinaglia and
linoman
and removed request for
a team
|
|
github-actions
bot
added
area/backend
type/docs
There was a problem hiding this comment.
Thanks for your contribution, @TheoBrigitte! I just have a minor comment to avoid unnecessary database calls. The PR should be good to go once that is addressed.
|
|
||
| func (s *Service) Validate(ctx context.Context, username string) (bool, error) { | ||
| if s.cfg.DisableBruteForceLoginProtection { | ||
| if s.cfg.DisableUsernameLoginProtection { |
There was a problem hiding this comment.
We should continue to skip the validation if s.cfg.DisableBruteForceLoginProtection is true to avoid unnecessary database calls.
| if s.cfg.DisableUsernameLoginProtection { | |
| if s.cfg.DisableBruteForceLoginProtection || s.cfg.DisableUsernameLoginProtection { |
For completeness, we would probably want to do the same for Service.ValidateIPAddress().
We could go even a step further and update Service.Add() to skip adding in the scenario where s.cfg.DisableBruteForceLoginProtection || (s.cfg.DisableBruteForceLoginProtection && s.cfg.DisableBruteForceLoginProtection). You're welcome to make that change if you wish, otherwise we can probably create a separate PR for it.
There was a problem hiding this comment.
That's a good point. I think you meant s.cfg.DisableBruteForceLoginProtection || (s.cfg.DisableUsernameLoginProtection && s.cfg.DisableIPAddressLoginProtection)
I added that.
There was a problem hiding this comment.
Heh, good catch. I did, indeed.
There was a problem hiding this comment.
@TheoBrigitte Looks great! I was wondering if you could add a couple of tests to exercise these new checks (for both Validate & ValidateIPAddress)?
I'd approved it before realizing that we needed to update the tests. I've removed my approval but will take another look soon. Thanks again for your contribution!
There was a problem hiding this comment.
I added couple of test cases to test all the settings combinations.
cinaglia
changed the title
cinaglia
dismissed
their stale review
Removing approval until tests are updated. Will check back again!
3 participants
What is this feature?
This PR adds a new setting in order to be able to disable the username based brute force login protection, which allow to use IP only brute force login protection.
Why do we need this feature?
The issue I am facing currently is due to brute force attacks trying to login in as admin user, which then results in the admin user account being blocked for 5 min. I would rather instead only have the attacker IP address being blocked and not the admin account.
Who is this feature for?
For DevOps people running and configuring Grafana.
Which issue(s) does this PR fix?:
Fixes #109138
Special notes for your reviewer:
Please check that: