Skip to content
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

Bugfix: BruteForceDetection check for whitelist ip before blacklist #16348

Closed

Conversation

markoboy
Copy link

A fix for the BruteForceDetection module on Login/Security plugin. When the configs are set to disable login from all ips but never block some ips, the user gets blocked from being able to login because the first check is done on the blacklist instead of the whitelist to allow the user to login from the whitelist ips.

Check this post for your reference:
https://forum.matomo.org/t/cannot-login-because-of-blocked-ip/35446/5

@Findus23 Findus23 added the Needs Review PRs that need a code review label Aug 26, 2020
@tsteur
Copy link
Member

tsteur commented Aug 26, 2020

@markoboy @Findus23 I wonder if you are maybe more after the login whitelist feature? https://matomo.org/faq/how-to/faq_25543/

For the brute force blacklist it was meant to have a priority say some IP gets blocked from time to time and you want to block that specific IP forever.

@markoboy
Copy link
Author

@tsteur what about this page https://matomo.org/faq/troubleshooting/faq_32758/

It says 'Never block these IPs from logging in', that's how I was not able to log in... I added the above on the configs although I was still unable to log in...

As it is implied that these ips will never get blocked shouldn't they be checked first?

@tsteur
Copy link
Member

tsteur commented Aug 26, 2020

I see. We might need to change the wording. I guess the blacklist generally wasn't meant as a "Block all" kind of feature. For this case the login whitelist feature be better suited.

I'm basically saying this to avoid later someone coming like "I have whitelisted a range of IPs but want to blacklist specific IPs and the blacklist should be checked first".

@tsteur
Copy link
Member

tsteur commented Sep 3, 2020

@mattab any thoughts on this?

@mattab
Copy link
Member

mattab commented Sep 8, 2020

@tsteur Sounds good to me to merge

Thanks for creating the PR @markoboy

I'm basically saying this to avoid later someone coming like "I have whitelisted a range of IPs but want to blacklist specific IPs and the blacklist should be checked first".

👍

@tsteur
Copy link
Member

tsteur commented Sep 8, 2020

@mattab you are saying sounds good to merge but then giving a thumbs up on my indication not to merge.

Note that changing this would also break existing configurations and in above described use case clearly the wrong feature is used as mentioned. The correct feature to be used is definitely https://matomo.org/faq/how-to/faq_25543/ so I'd be rather keen to close the PR since it allows more flexibility overall I would say.

@mattab
Copy link
Member

mattab commented Sep 8, 2020

@tsteur I see what you mean now, I agree it's better not to merge and instead update wording. So would we just need to change the wording in the settings from 'Never block these IPs from logging in' to the actual meaning like 'Only allow these IPs to login' or so?

@markoboy could you use this Login Whitelist IPs feature instead?

@tsteur
Copy link
Member

tsteur commented Sep 9, 2020

Thanks for creating the PR @markoboy as mentioned in the comments we generally prefer not changing the behaviour and rather explaining it better see #16411

We generally prefer to rather block an IP than to allow it. And changing the meaning of it would break the meaning for existing users and it is maybe a more safe approach to rather block the IP when we have an extra feature where it's possible to only allow specific IPs. Thanks for this again!

@tsteur tsteur closed this Sep 9, 2020
@mattab mattab added this to the 4.0.0 milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants