Oefenweb / ansible-fail2ban

Ansible role to set up fail2ban in Debian-like systems
MIT License
117 stars 55 forks source link

fix #36 #37

Closed Yannik closed 7 years ago

tersmitten commented 7 years ago

Nice addition, I would love to merge it. Can you revert line 107 and adjust the documentation (README)?

Yannik commented 7 years ago

@tersmitten About line 107: I do not think the current behaviour is useful at all. If the service is present in your ansible configuration, it should obviously be enabled, or it would not be present in your config or be manually disabled IMO. Anyone who feels it necessary to explicitly state that it is enabled (which seems to be a rare usecase) can still do so with my patch. Otherwise this seems like unnecessary bloat.

tersmitten commented 7 years ago

I understand yoou point, but if you specify:

fail2ban_services:
  - name: ssh
    enabled: false

jail.local will contain:

enabled = False

instead of

enabled = false
tersmitten commented 7 years ago

This works like a charm

enabled = {{ service.enabled | default(true) | bool | to_json }}

What do you think about that?

Yannik commented 7 years ago

I see your point! I have just tested this, and actually, True and False are interpreted correctly by fail2ban, so this issue is purely cosmetic :) Your suggestion is fine, but I feel that it is utterly unreadable. (I.e., you wouldn't guess why it's there without knowing). Perhaps there is a better way to achieve this? Or alternatively just leave it as-is, as it is just cosmetic.

Yannik commented 7 years ago

{{ service.enabled | default('true') | bool | lower }} seems a bit more readable. Other suggestions?

Yannik commented 7 years ago

I have just done some research regarding the README update: There is a jail defined in jail.conf for every filter shipped with fail2ban. This role is currently not able to add new filters. So, generally, you will always just enable and perhaps adjust one of the predefined jails. Ultimately, the only variable required for each fail2ban_services item is name. Due to these points, It seems most reasonable replace the current fail2ban_services documentation with one or more examples on how to enable/adjust existing jails and document the aforementioned behaviour.

tersmitten commented 7 years ago

This role is currently not able to add new filters

What about fail2ban_filterd_path (see README)?

tersmitten commented 7 years ago

Due to these points, It seems most reasonable replace the current fail2ban_services documentation with one or more examples on how to enable/adjust existing jails and document the aforementioned behaviour

Can you make the documentation look something like this?

* `supervisor_programs_present` [default: `{}`]: Program definitions
* `supervisor_programs_present.{n}` [required]: Program name
* `supervisor_programs_present.{n}.command` [required]: The command that will be run when this program is started
* `supervisor_programs_present.{n}.directory` [optional]: A directory to which supervisord should temporarily chdir before exec’ing the child
* `supervisor_programs_present.{n}.environment` [optional]: A list of key/value pairs that will be placed in the child process’ environment
* `supervisor_programs_present.{n}.autostart` [optional]: If true, this program will start automatically when supervisord is started
* `supervisor_programs_present.{n}.autorestart` [optional]: Whether the process will be autorestarted
* `supervisor_programs_present.{n}.startretries` [optional]: The number of serial failure attempts that supervisord will allow when attempting to start the program before giving up
Yannik commented 7 years ago

Okay, gotcha on the custom filters. Anyway, the only (always) required value is "name". Could you perhaps submit a concrete README draft (or merge this and commit it on your own).

tersmitten commented 7 years ago

I can fix the documentation, no worries