search

OpenMediaVault-Plugin-Developers / openmediavault-fail2ban

11 stars 5 forks source link

Add possibility to choose custom chain #32

Open TheLastProject opened 2 years ago

TheLastProject commented 2 years ago

Fixes #31.

Currently untested, as I am still on OMV 5. When 6 becomes stable I will test this code and remove draft state from this PR.

ryecoaaron commented 2 years ago

The default entries will need a chain element added. And existing installs will need to be upgraded to add the chain element to each jail in the database. In order to add the proper chain, the default value in /etc/default/openmediavault will need to be checked to see if it is something other than INPUT.

TheLastProject commented 2 years ago

The default entries will need a chain element added. And existing installs will need to be upgraded to add the chain element to each jail in the database.

Do they though? Because there is still a default chain in srv/salt/omv/deploy/fail2ban/files/etc-fail2ban-jail_conf.j2. If chain isn't set it'll just stick to the default.

In order to add the proper chain, the default value in /etc/default/openmediavault will need to be checked to see if it is something other than INPUT.

I made sure to keep respecting the default in 6ce5926fe9bcc813b5ef828757554468dbd83ab4 until a new value is saved in fail2ban settings. It will break if you go to fail2ban settings and hit save without paying attention to the new default (which is now INPUT) as it will set your configuration default to INPUT. Preferably the settings field gets pre-filled with your current value, but I don't know if that is possible (this is my first attempt to contribute to OMV and given I'm still on OMV5 I can't really test).

ryecoaaron commented 2 years ago

Do they though?

Technically, no. Do I think a plugin should have a different xml structure for some jails? no. All that is require is a migration file. Here is one doing almost exactly the same thing - https://github.com/OpenMediaVault-Plugin-Developers/openmediavault-mergerfs/blob/main/usr/share/openmediavault/confdb/migrations.d/conf.service.mergerfs_6.0.9.sh. Just need to source the omv default flle and use the environment variable.

I don't know if that is possible (this is my first attempt to contribute to OMV and given I'm still on OMV5 I can't really test).

For most plugins, only the web interface component is different between OMV 5 and 6. The php, saltstack, database structure, etc usually requires no changes.

TheLastProject commented 2 years ago

Looking at your migration file, I think I misunderstood. I thought you were referring to explicitly writing chain = in each specific jail file despite there already being a chain = in the DEFAULTS file.

The migration should be implemented in c770a380982bd27de1aac2d02a8b8c739541ddfb now.

For most plugins, only the web interface component is different between OMV 5 and 6. The php, saltstack, database structure, etc usually requires no changes.

Yeah, I understand. I more meant that OMV 5 support was dropped in this plugin (old interface files removed) and for everyone's benefit it made little sense to target an older commit of this project, so I targeted master but I can't currently test it very well because it won't integrate in the OMV 5 UI. Once there is a stable release of OMV 6 I will upgrade my install, test this more thoroughly, fix any issues I notice and mark it ready for review.

ryecoaaron commented 2 years ago

While I don't use this plugin, I can test these changes and push a new build.

TheLastProject commented 2 years ago

Well, that part is up to you. If you don't mind waiting for OMV 6 to become stable (I don't) I can test it myself and fix any issues I may run into as I am interested in this feature for myself. I definitely do not want to put undue extra work on anyone :)