-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add Fail2ban support #65
Conversation
It's always nice to have a quick look at the UI of the software you're going to install :)
To make sure any older version will get fail2ban support
This is needed for fail2ban helpers
Changed the base branch to master, as it is an high priority issue, we can't wait for testing to be merged. |
I fixed the regex conf, at first I did not understand how to configure it correctly. I tried a fresh install + removal, it works. Fail2ban seems to be active: after 5 failed login attempts, the login page doesn't load. It's ready for review. |
Adding fail2ban is a good improvement, not a security issue needed to be quickly fixed. |
Ok, only upgrade from this version fails… I found the issue: I fixed the regex syntax in the install script, but not in the upgrade one |
This allow empty username (not possible, but may still block some extra brute force spammers) and username with spaces
This allow empty username (not possible, but may still block some extra brute force spammers) and username with spaces
Upgrade is stille failing on the CI.
|
But we don't have the log for fail2ban… :( |
Isn't the problem reproducible with package_check in your environment? |
How do I use it ? |
As an app maintainer, you will love package_check! Have a look at the README, you can install it on your test server/VM. It's been developed mainly by @maniackcrudelis, and it's what produces the results of the CI. |
I don't have any VM or test server for that :/ |
Then you'll be interested by this forum post 😉 |
Ok, I tried again on my main server (backup + remove old wallabag + install master + upgrade to this branch), the error is:
I fixed that in the install script, but not in the upgrade script 😅 |
By the way, more relevant link here to use the CI available for app packagers 😉 |
CI is succeeding right now. Let's merge ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Can be merged in 3 days (if @maniackcrudelis confirms his code review after late changes) |
Can be merged in 3 days. |
Thanks @maniackcrudelis for the improvements :) |
Problem
See Set-up fail2ban rules #37
Solution
The number of retry is set to 5, I think it allows users to make a reasonable number of errors while restricting brute force possibilities.
Warning : this PR will drop support for version older than 3.5, in particular Yunohost 2.7 (Debian 8).
PR Status
Validation
Minor decision
When the PR is marked as ready to merge, you have to wait for 3 days before really merging it.