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

[enh] Add fail2ban helpers #364

Open
wants to merge 5 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
7 participants
@JimboJoe
Copy link
Member

JimboJoe commented Sep 2, 2017

Problems

To enhance applications protection against hackers/spammers, etc., we can propose helpers to ease the creating of fail2ban jails.

Solution

Add ynh_add_fail2ban_config and ynh_remove_fail2ban_config helpers.
A successful implementation example is the piwigo app.

PR Status

Working, but opinion welcome!
And should be implemented in other applications to validate its principle.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Oct 6, 2017

As anyone tested those? I have no knowledge of fail2ban.

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented Oct 7, 2017

At the moment, only the (nearly official) Piwigo application implemented it. So maybe should we wait for a couple of other successful implementations to merge it...?

@alexAubin alexAubin added this to the 2.7.x (fixes) or 2.8 milestone Oct 7, 2017

@yunohost-bot yunohost-bot modified the milestones: 2.7.x (fixes) or 2.8, 3.x Nov 8, 2017

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 28, 2017

As far as I know, fail2ban's conf format changed in Stretch so that might affect these helpers :s

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Dec 26, 2017

At the moment, only the (nearly official) Piwigo application implemented it. So maybe should we wait for a couple of other successful implementations to merge it...?

Any update on that?

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented Dec 26, 2017

It's currently being implemented in Wordpress, and has evolved here.

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Jan 31, 2018

About this :

we should be careful that the fail2ban conf format changed a bit in stretch and is backward-incompatible. I don't know if these helpers are affected... but right now, the step in the stretch migration is to remove all existing fail2ban conf and regenerate it from scratch :s

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented May 10, 2018

Bumpitybump, is there any update on this ? Are the helpers stable enough to be considered merged in the core ? (Do they work on Stretch ?)

@alexAubin alexAubin changed the title Add fail2ban helpers [enh] Add fail2ban helpers May 10, 2018

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented May 19, 2018

I've updated the helpers to be on-par with the ones from Experimental_helpers.
I've tested again piwigo on Stretch and they work as advertised.
I understand you had to delete all existing fail2ban jails when migrating from Jessie to Stretch, so that means existing apps won't be protected by fail2ban until their next upgrade (which I find perfectly acceptable)?

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented May 19, 2018

I understand you had to delete all existing fail2ban jails when migrating from Jessie to Stretch, so that means existing apps won't be protected by fail2ban until their next upgrade

Yes indeed :s

@JimboJoe JimboJoe removed the work needed label May 19, 2018

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented May 21, 2018

This helper currently only works on Stretch...

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jun 11, 2018

Not a problem if it only works on Stretch, we could merged it only in stretch...

@alexAubin alexAubin changed the base branch from unstable to stretch-unstable Jun 17, 2018

@alexAubin alexAubin modified the milestones: 2.7.x, 3.1.x Jun 19, 2018

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Jun 30, 2018

We are now on stretch, shouldn't we merge this?

@alexAubin alexAubin modified the milestones: 3.1.x, 3.2.x Aug 15, 2018

alexAubin added some commits Aug 29, 2018

@alexAubin
Copy link
Member

alexAubin left a comment

Yolo LGTM, we should merge this at some point ¯\_(ツ)_/¯

@JimboJoe

This comment has been minimized.

Copy link
Member

JimboJoe commented Aug 29, 2018

Yes, but not before #520 gets merged 😉

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Oct 24, 2018

#520 got merged. Any update about this ? :D

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Oct 31, 2018

Bumpz D:

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Nov 23, 2018

I think it could be useful to have more flexibility about the config file. By example if we need to specify multiple regex or add some ingoreregex or add the "maxlines" option.

@alexAubin alexAubin modified the milestones: 3.3.x, 3.4.x Nov 23, 2018

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 27, 2018

Okay guys, I'm flagging this as inactive :/ ... It'd be really nice if we could have some progress on this one way or another. I don't think we need a 100% perfect solution to deploy this, considering that in the meantime some web apps may remain vulnerable to brute force.

@zamentur
Copy link
Contributor

zamentur left a comment

I approve this helper in this state. If some apps need more, it could be possible to make this helper better in future

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Dec 2, 2018

Let's merge in 3 days if no one disagree until then.

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Dec 4, 2018

Following today's meeting, this PR is postponed. Josue will have a look at integrating fail2ban in his apps, which requires to have several regex instead of just one. He'll iterate on the design.

@alexAubin alexAubin added postponed and removed opinion needed labels Dec 7, 2018

@alexAubin alexAubin modified the milestones: 3.4.x, 3.x Dec 10, 2018

finalfail2banjailconf="/etc/fail2ban/jail.d/$app.conf"
finalfail2banfilterconf="/etc/fail2ban/filter.d/$app.conf"
ynh_backup_if_checksum_is_different "$finalfail2banjailconf" 1
ynh_backup_if_checksum_is_different "$finalfail2banfilterconf" 1

This comment has been minimized.

@maniackcrudelis

maniackcrudelis Jan 4, 2019

Contributor

Is anyone know why is there a '1' as a second argument for this helper ?
I can't find any version of ynh_backup_if_checksum_is_different with a second argument.

This is crashing getopts, which is another problem to fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment