Issue 29364214: Issue 2487 - Introduce fail2ban module (Closed)
DescriptionIssue 2487 - Introduce fail2ban module
Patch Set 1 #
Total comments: 36
Patch Set 2 : Issue 2487 - Introduce fail2ban module #
Total comments: 9
Patch Set 3 : #
Total comments: 26
Patch Set 4 : Issue 2487 - Introduce fail2ban module #
Total comments: 2
Patch Set 5 : Issue 2487 - Introduce fail2ban module #Patch Set 6 : Issue 2487 - Introduce fail2ban module #
Total comments: 26
Patch Set 7 : Issue 2487 - Introduce fail2ban module #
Total comments: 8
Patch Set 8 : For comments 18 and 19 #
Total comments: 2
Patch Set 9 : For comment 22 and 23 #
MessagesTotal messages: 26
Very promising ;-) https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:3: # Manage filter information and files for any custom filter we create Please use un-personalized text in documentation, i.e. "filter created" vs "filter we create" - or just "filter". And full sentences (including periods) are appreciated as well ;-) https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:9: # One per line A bit too specific. Something like "The regular expression applied by the filter." is more than enough. And what do you mean by "One per line"? https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:19: # }, The example is not valid Puppet code, a snippet at best, but even then not matching the fail2ban::filter type. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:22: $ensure = 'present', The $ensure parameter is not documented yet. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:28: if $failregex != undef { This condition does not make much sense in this context. What if $ensure = 'absent'? Nothing will happen. I think the better approach would be to fail() if $ensure = 'present' and $failregex == undef, and have the resource definition below the conditional statement. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:12: # ensure => 'latest') or remove Fluentd (ensure => 'absent' or 'purged') Fluentd? https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # } The example code is not properly indented. Also it should follow the same style practices as regular code within this repository. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:38: $package = {}, Please make sure to wrap all default arguments according to this pattern: $parameter = hiera('class::parameter', $default), This emulates Puppet 3 behavior and allows for i.e. designating parameter values as global keys within Hiera. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:50: /^(absent|purged|held)$/ => 'absent', By now I wouldn't consider a "held" package as "absent" any more. Usually one would mark a package that way if there is a customized local version available. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:55: if ($ensure == 'present') or ($service['ensure'] != undef) { Why checking for $service['ensure'] being defined? Shouldn't the default be totally fine? https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:62: # already in there, so the config file should be done appart. I don't really get this point, but I assume you meant "separately" when you wrote "appart"? https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:63: if jail_config != undef { The $jail_config defaults to an empty hash, so I doubt that this condition makes much sense here. Especially not if this "magic" with undef is not intended and/or documented accordingly. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:74: Service[$title] <~ Package[$title] Usually package updates imply reloading/restarting the associated service (if any), hence I am not sure if one would want to notify the service resource here (rather than just declaring an order relationship). https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:77: Package[$title] -> File['/etc/fail2ban/jail.local'] Since the file resource is just declared under certain circumstances, the dependency on the package can trigger an error if denoted in this fashion and position. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:10: bantime = <%= config['bantime']||= 3600 %> The key-by-key assignment is limiting the user/configuration quite a lot. One could iterate through the config hash and print existing key/value pairs instead.. https://codereview.adblockplus.org/29364214/diff/29364215/modules/private-stu... File modules/private-stub/hiera/base.yaml (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/private-stu... modules/private-stub/hiera/base.yaml:6: fail2ban: Please only include the class here, but do not configure actual filters etc. just yet.
I found white spaces! https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:8: # Adds jail.local to the default configuration of fail2ban Unnecessary white space at end of line. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:26: # logpath => '/var/log/nginx/access.log', Unnecessary white spaces at end of line. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:60: Unnecessary white spaces here. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:73: Unnecessary white spaces here.
https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:3: # Manage filter information and files for any custom filter we create On 2016/11/24 16:08:48, mathias wrote: > Please use un-personalized text in documentation, i.e. "filter created" vs > "filter we create" - or just "filter". And full sentences (including periods) > are appreciated as well ;-) Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:9: # One per line On 2016/11/24 16:08:48, mathias wrote: > A bit too specific. Something like "The regular expression applied by the > filter." is more than enough. And what do you mean by "One per line"? Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:19: # }, On 2016/11/24 16:08:48, mathias wrote: > The example is not valid Puppet code, a snippet at best, but even then not > matching the fail2ban::filter type. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:22: $ensure = 'present', On 2016/11/24 16:08:48, mathias wrote: > The $ensure parameter is not documented yet. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:28: if $failregex != undef { On 2016/11/24 16:08:48, mathias wrote: > This condition does not make much sense in this context. What if $ensure = > 'absent'? Nothing will happen. I think the better approach would be to fail() if > $ensure = 'present' and $failregex == undef, and have the resource definition > below the conditional statement. There can be cases where an already existing filter is in the folder so one might want to remove it. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:12: # ensure => 'latest') or remove Fluentd (ensure => 'absent' or 'purged') On 2016/11/24 16:08:48, mathias wrote: > Fluentd? Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # } On 2016/11/24 16:08:48, mathias wrote: > The example code is not properly indented. Also it should follow the same style > practices as regular code within this repository. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:38: $package = {}, On 2016/11/24 16:08:48, mathias wrote: > Please make sure to wrap all default arguments according to this pattern: > $parameter = hiera('class::parameter', $default), > > This emulates Puppet 3 behavior and allows for i.e. designating parameter values > as global keys within Hiera. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:50: /^(absent|purged|held)$/ => 'absent', On 2016/11/24 16:08:48, mathias wrote: > By now I wouldn't consider a "held" package as "absent" any more. Usually one > would mark a package that way if there is a customized local version available. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:55: if ($ensure == 'present') or ($service['ensure'] != undef) { On 2016/11/24 16:08:48, mathias wrote: > Why checking for $service['ensure'] being defined? Shouldn't the default be > totally fine? Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:62: # already in there, so the config file should be done appart. On 2016/11/24 16:08:48, mathias wrote: > I don't really get this point, but I assume you meant "separately" when you > wrote "appart"? Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:63: if jail_config != undef { On 2016/11/24 16:08:48, mathias wrote: > The $jail_config defaults to an empty hash, so I doubt that this condition makes > much sense here. Especially not if this "magic" with undef is not intended > and/or documented accordingly. One can have filters without activating them, so you can still create filters without activating them. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:74: Service[$title] <~ Package[$title] On 2016/11/24 16:08:48, mathias wrote: > Usually package updates imply reloading/restarting the associated service (if > any), hence I am not sure if one would want to notify the service resource here > (rather than just declaring an order relationship). Notify is a kind of relation, but I agree that when the package is updated or installed is handles that part, I'm removing this. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:77: Package[$title] -> File['/etc/fail2ban/jail.local'] On 2016/11/24 16:08:48, mathias wrote: > Since the file resource is just declared under certain circumstances, the > dependency on the package can trigger an error if denoted in this fashion and > position. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:10: bantime = <%= config['bantime']||= 3600 %> On 2016/11/24 16:08:48, mathias wrote: > The key-by-key assignment is limiting the user/configuration quite a lot. One > could iterate through the config hash and print existing key/value pairs > instead.. I agree but I couldn't find a way to make this happen for each filter that can be activated since the defaults must be written for each filter on this file. https://codereview.adblockplus.org/29364214/diff/29364215/modules/private-stu... File modules/private-stub/hiera/base.yaml (right): https://codereview.adblockplus.org/29364214/diff/29364215/modules/private-stu... modules/private-stub/hiera/base.yaml:6: fail2ban: On 2016/11/24 16:08:48, mathias wrote: > Please only include the class here, but do not configure actual filters etc. > just yet. Acknowledged.
Unless one specifies a specific port to ban, the iptables operation will fail.
Example configuration on vagrant host issues1:
classes:
fail2ban:
jail_config:
wordpress:
logpath: /var/log/nginx/access_log_trac
filters:
wordpress:
failregex:
- ^<HOST>.*\"WordPress\/.*
https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser...
File modules/downloadserver/files/known_hosts (left):
https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser...
modules/downloadserver/files/known_hosts:1: ssh.adblockplus.org,88.198.66.34
ecdsa-sha2-nistp256
AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBELZuMCPfLTL12kf+OBvKhYucuUwIAOB77sp1UFHgveHBko/23A3Ix1h1aIFOYDuJc63Jo2nkPyWJ/TRLdVsEuM=
Accidentally included in this patch set.
https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser...
File modules/downloadserver/manifests/init.pp (left):
https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser...
modules/downloadserver/manifests/init.pp:107:
Accidentally included in this patch set.
https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/ma...
File modules/fail2ban/manifests/init.pp (right):
https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/ma...
modules/fail2ban/manifests/init.pp:24: # jail_config => {
We should require setting a port here, and if not we should switch to using the
iptables-allports.conf action.
https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/ma...
modules/fail2ban/manifests/init.pp:67: file {'/etc/fail2ban/jail.local':
Like stated earlier, if no ports have been configured, we should set action to
use /etc/fail2ban/action.d/iptables-allports.conf instead of default action
(iptables).
https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/te...
File modules/fail2ban/templates/jail.erb (right):
https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/te...
modules/fail2ban/templates/jail.erb:6: port = <%= config['port'] ||= 'all' %>
'all' is not a valid port range for iptables, ref earlier comments. It leads to
this command failing (note that fail2ban does not print the reason in its log
for some reason):
root@issues1:~# iptables -I INPUT -p tcp -m multiport --dports all -j
fail2ban-wordpress
iptables v1.4.12: invalid port/service `all' specified
For you r comment about `all` not being a valid argument, there is this example in the `jail.conf` file where it show that it is in fact an actual option, so maybe I'm rendering wrong or something different. [xinetd-fail] enabled = false filter = xinetd-fail port = all banaction = iptables-multiport-log logpath = /var/log/daemon.log maxretry = 2 https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser... File modules/downloadserver/files/known_hosts (left): https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser... modules/downloadserver/files/known_hosts:1: ssh.adblockplus.org,88.198.66.34 ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBELZuMCPfLTL12kf+OBvKhYucuUwIAOB77sp1UFHgveHBko/23A3Ix1h1aIFOYDuJc63Jo2nkPyWJ/TRLdVsEuM= On 2016/11/25 16:23:29, f.nicolaisen wrote: > Accidentally included in this patch set. Yeah, gonna remove in next patch https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser... File modules/downloadserver/manifests/init.pp (left): https://codereview.adblockplus.org/29364214/diff/29364554/modules/downloadser... modules/downloadserver/manifests/init.pp:107: On 2016/11/25 16:23:29, f.nicolaisen wrote: > Accidentally included in this patch set. Yeah, gonna remove next patch as well https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:24: # jail_config => { On 2016/11/25 16:23:29, f.nicolaisen wrote: > We should require setting a port here, and if not we should switch to using the > iptables-allports.conf action. That is the actual motive for this kinda of configuration, to use whatever we feel like, see matze's comment on the jail.local template file, I still don't know how to make that work but then it would work flawlessly https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29364554/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:6: port = <%= config['port'] ||= 'all' %> On 2016/11/25 16:23:30, f.nicolaisen wrote: > 'all' is not a valid port range for iptables, ref earlier comments. It leads to > this command failing (note that fail2ban does not print the reason in its log > for some reason): > > root@issues1:~# iptables -I INPUT -p tcp -m multiport --dports all -j > fail2ban-wordpress > iptables v1.4.12: invalid port/service `all' specified Well, according to the examples provided by fail2ban and the actual templates on the jail.conf, this is an acceptable way of telling fail2ban to block all the ports, but this file should be a different template anyways.
> For you r comment about `all` not being a valid argument, there is this example > in the `jail.conf` file where it show that it is in fact an actual option, so > maybe I'm rendering wrong or something different. > > [xinetd-fail] > > enabled = false > filter = xinetd-fail > port = all > banaction = iptables-multiport-log > logpath = /var/log/daemon.log > maxretry = 2 > I think it must be a copy/paste error from the previous example, where the comment reads "port actually must be irrelevant but lets leave it all for some possible uses". Looking at the iptables-multiport-log action, it takes the values of port and passes it onto iptables' multiport --dports argument, which according to iptables manpage, allows up to 15 ports/port-range specifiers. The same manpage has no mention of any 'all' protocol. I guess we have to trust that the puppet-users here know how to configure fail2ban, but at least the example in our docs should provide a working setup. Either suggest port => http,https (default banaction is iptables-multiport), or suggest banaction iptables-allports. Perhaps even default to use iptables-allports, unless we can think of a reason not to.
On 2016/11/26 00:05:54, f.nicolaisen wrote: > I guess we have to trust that the puppet-users here know how to configure > fail2ban, but at least the example in our docs should provide a working setup. > Either suggest port => http,https (default banaction is iptables-multiport), or > suggest banaction iptables-allports. Perhaps even default to use > iptables-allports, unless we can think of a reason not to. By the way, if I try starting fail2ban with "port = ", it fails without a good error message in its log, just like with "port = all". If I try starting it with the port line missing completely, it fails with a big nice stack trace and an explanation from ConfigParser. So if we iterate over the config hash and populate jail.local like you/Matze mentioned, we will get the second behavior when people forget to specify ports with default banaction. Which is good.
It (still) works! I still found some documentation things to pick on, but LGTM functionality-wise as it stands. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:44: } No empty line at end of file https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:8: # Adds jail.local to the default configuration of fail2ban. "Provisions a jail.local adjacent to the default fail2ban configuration." https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:9: # By default it will have the following parameters: s/it/entries https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:17: # otherwise it will fail if there is no 'banaction' declared. I would formulate it like this: For the default banaction iptables-allports, the port parameter is not used and only set here for documentation purposes. Note that if 'banaction' is set to iptables-multiport, it requires that the 'port' parameter contains one or more comma-separated ports or protocols. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # logpath => '/var/log/nginx/access_log_hg', Remove trailing WS (white space) https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # logpath => '/var/log/nginx/access_log_hg', Maybe we should add banaction multiport: http,https in this example. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:50: $jail_config = {}, Now that I think about it, maybe we should call this one 'jail_configs'? Not sure. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:68: /^(absent|purged)$/ => 'absent', WS https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:78: WS https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:81: # of the filters, another thing to conside is the possibility of Typo: 'conside'. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:81: # of the filters, another thing to conside is the possibility of Split the sentences: "... filters. Another thing to..." https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:83: # passed. The whole above comment is a bit "loose" and undecided. Try instead to declare exactly how to do what. "Filters already present in the fail2ban distribution can also be activated. Configure deactivated filters so that ... (when is that a good idea?)." https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:98: } No empty line at end of file https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:9: <% end -%> I think if you remove the minus (-) above here you'll get a nice empty line between each jail configured in the jail.local file. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:10: <% end -%> Missing empty line for end of file.
https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:8: # Adds jail.local to the default configuration of fail2ban. On 2016/11/29 00:38:54, f.nicolaisen wrote: > "Provisions a jail.local adjacent to the default fail2ban configuration." Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:9: # By default it will have the following parameters: On 2016/11/29 00:38:54, f.nicolaisen wrote: > s/it/entries Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:17: # otherwise it will fail if there is no 'banaction' declared. On 2016/11/29 00:38:54, f.nicolaisen wrote: > I would formulate it like this: > For the default banaction iptables-allports, the port parameter is not used and > only set here for documentation purposes. Note that if 'banaction' is set to > iptables-multiport, it requires that the 'port' parameter contains one or more > comma-separated ports or protocols. Like it, gonna use your instead :) https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # logpath => '/var/log/nginx/access_log_hg', On 2016/11/29 00:38:54, f.nicolaisen wrote: > Remove trailing WS (white space) Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:36: # logpath => '/var/log/nginx/access_log_hg', On 2016/11/29 00:38:54, f.nicolaisen wrote: > Remove trailing WS (white space) Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:50: $jail_config = {}, On 2016/11/29 00:38:54, f.nicolaisen wrote: > Now that I think about it, maybe we should call this one 'jail_configs'? Not > sure. I think singular is ok since it is only one jail.local configuration file we are going to create, I tried using a jail.d folder in order to have more configurations but it doesn't work. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:68: /^(absent|purged)$/ => 'absent', On 2016/11/29 00:38:54, f.nicolaisen wrote: > WS Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:78: On 2016/11/29 00:38:54, f.nicolaisen wrote: > WS Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:81: # of the filters, another thing to conside is the possibility of On 2016/11/29 00:38:54, f.nicolaisen wrote: > Typo: 'conside'. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:83: # passed. On 2016/11/29 00:38:55, f.nicolaisen wrote: > The whole above comment is a bit "loose" and undecided. Try instead to declare > exactly how to do what. "Filters already present in the fail2ban distribution > can also be activated. Configure deactivated filters so that ... (when is that a > good idea?)." Well you can have filters for specific situations and then deactivate them. https://codereview.adblockplus.org/29364214/diff/29365536/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:98: } On 2016/11/29 00:38:54, f.nicolaisen wrote: > No empty line at end of file Acknowledged.
Just a couple of more nitpicks :) https://codereview.adblockplus.org/29364214/diff/29365564/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365564/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:39: # 'port' => 'https, http', lines not aligned? https://codereview.adblockplus.org/29364214/diff/29365564/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:86: # passed. I still don't understand when I should consider this possibility :) "so no conf is passed" is not clear grammar. Who is passing what conf where?
https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:7: # [*failregex*] Either we allow for passing a single string as well (the most common use-case probably, and easy to wrap into an array later, but a little more excessive documentation required) or we should re-name the parameter to reflect the list character (plural). https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:16: # fail2ban::filter => {'CVE-2013-0235': The "=>" here is invalid syntax, and the sub-levels below should be indented by 2wo spaces only. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:19: # '^.*\"WordPress\/.*<HOST>.*' Missing a comma here, after the string item. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:22: # } Another comment-line (hash-tag in the beginning, otherwise empty) should follow here. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:32: fail('Require an array of string[s] with the regex patterns to apply.') Those square brackets are not necessary. And the error message does not indicate the actual issue, namely that an empty array has been encountered ("array of strings" vs "array of one or more regular expressions", for example). https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:29: # Adds adittional filters to the filters.d folder. Another comment-line (hash-tag in the beginning, otherwise empty) should follow here. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:49: # } Another comment-line (hash-tag in the beginning, otherwise empty) should follow here. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:75: # Service resources don't properly support the concept of absence There is more than just a service resource taken care of below, so this comment should either be more inclusive or more abstract or absent. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:78: ensure_resource('service', $title, $service) What about the $hasrestart and $hasstatus parameters? With fail2ban, can't we set those to true by default? https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:86: if jail_config != undef { This condition should check for empty($jail_config) instead; undef even being recognized/supported is rather hidden magic. And then this conditional is IMHO not necessary anyway: It should be no problem to have the template iterate through an empty hash, always creating a jail.local file. That would also obsolete the second phrase in the commend above, including the "aslo" typo. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:97: Package[$title] -> File['/etc/fail2ban/jail.local'] A relationship declaring the Service[$title] being dependent on the Package[$title] is missing here. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:2: <% if !config['logpath'].empty? -%> Why this condition? And why just silently ignoring the current item if it does not evaluate to true? https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:4: filter = <%= name %> The filter should be an extra parameter of the current config item, only defaulting to the jail name. Otherwise one cannot re-use filter definitions, or utilize vanilla ones.
https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:7: # [*failregex*] On 2016/11/29 13:21:24, mathias wrote: > Either we allow for passing a single string as well (the most common use-case > probably, and easy to wrap into an array later, but a little more excessive > documentation required) or we should re-name the parameter to reflect the list > character (plural). Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:16: # fail2ban::filter => {'CVE-2013-0235': On 2016/11/29 13:21:24, mathias wrote: > The "=>" here is invalid syntax, and the sub-levels below should be indented by > 2wo spaces only. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:19: # '^.*\"WordPress\/.*<HOST>.*' On 2016/11/29 13:21:24, mathias wrote: > Missing a comma here, after the string item. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:22: # } On 2016/11/29 13:21:24, mathias wrote: > Another comment-line (hash-tag in the beginning, otherwise empty) should follow > here. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:32: fail('Require an array of string[s] with the regex patterns to apply.') On 2016/11/29 13:21:24, mathias wrote: > Those square brackets are not necessary. And the error message does not indicate > the actual issue, namely that an empty array has been encountered ("array of > strings" vs "array of one or more regular expressions", for example). Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:29: # Adds adittional filters to the filters.d folder. On 2016/11/29 13:21:24, mathias wrote: > Another comment-line (hash-tag in the beginning, otherwise empty) should follow > here. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:49: # } On 2016/11/29 13:21:25, mathias wrote: > Another comment-line (hash-tag in the beginning, otherwise empty) should follow > here. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:75: # Service resources don't properly support the concept of absence On 2016/11/29 13:21:25, mathias wrote: > There is more than just a service resource taken care of below, so this comment > should either be more inclusive or more abstract or absent. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:78: ensure_resource('service', $title, $service) On 2016/11/29 13:21:25, mathias wrote: > What about the $hasrestart and $hasstatus parameters? With fail2ban, can't we > set those to true by default? We can, indeed, set those params to true https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:86: if jail_config != undef { On 2016/11/29 13:21:25, mathias wrote: > This condition should check for empty($jail_config) instead; undef even being > recognized/supported is rather hidden magic. And then this conditional is IMHO > not necessary anyway: It should be no problem to have the template iterate > through an empty hash, always creating a jail.local file. That would also > obsolete the second phrase in the commend above, including the "aslo" typo. You are right, if we iterate over an empty param it won't matter since it just won't merge any configurations with the jail.conf file https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:97: Package[$title] -> File['/etc/fail2ban/jail.local'] On 2016/11/29 13:21:25, mathias wrote: > A relationship declaring the Service[$title] being dependent on the > Package[$title] is missing here. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... File modules/fail2ban/templates/jail.erb (right): https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:2: <% if !config['logpath'].empty? -%> On 2016/11/29 13:21:27, mathias wrote: > Why this condition? And why just silently ignoring the current item if it does > not evaluate to true? The path to the log file is necessary so fail2ban know where to look and I don't think there is a good default here, so if the path is not given fail2ban won't start, I'll add an error in order to let the user know. https://codereview.adblockplus.org/29364214/diff/29365577/modules/fail2ban/te... modules/fail2ban/templates/jail.erb:4: filter = <%= name %> On 2016/11/29 13:21:26, mathias wrote: > The filter should be an extra parameter of the current config item, only > defaulting to the jail name. Otherwise one cannot re-use filter definitions, or > utilize vanilla ones. Acknowledged.
Almost there ;-) https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:34: "to create a filter file.") Please do not personalized messages ("you"). There is also no necessity to be *that* verbose, but well. https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:37: file {"/etc/fail2ban/filter.d/$title.conf": Please use $name (and document that this parameter is used to compose the file name). https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:71: # Used as default $ensure parameter for most resources below It is not used as parameter anywhere, just in the condition. Simply removing the comment here should suffice though.
Forgot one point.. https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:53: $package = {}, Please consistently use hiera('fail2ban::PARAMETER', $DEFAULT), in order to mimic Puppet 3 behavior and allow for adjustments outside of the classes: hash.
https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... File modules/fail2ban/manifests/filter.pp (right): https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:34: "to create a filter file.") On 2016/12/01 09:28:30, mathias wrote: > Please do not personalized messages ("you"). There is also no necessity to be > *that* verbose, but well. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/filter.pp:37: file {"/etc/fail2ban/filter.d/$title.conf": On 2016/12/01 09:28:30, mathias wrote: > Please use $name (and document that this parameter is used to compose the file > name). Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:53: $package = {}, On 2016/12/01 09:32:44, mathias wrote: > Please consistently use hiera('fail2ban::PARAMETER', $DEFAULT), in order to > mimic Puppet 3 behavior and allow for adjustments outside of the classes: hash. Acknowledged. https://codereview.adblockplus.org/29364214/diff/29365873/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:71: # Used as default $ensure parameter for most resources below On 2016/12/01 09:28:30, mathias wrote: > It is not used as parameter anywhere, just in the condition. Simply removing the > comment here should suffice though. Acknowledged.
https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:7: # [*jail_config*] One last point: I'd like to rename this one to "jails". Background: One of the follow-ups for this ticket will be the introduction of a named type called fail2ban::jail (which will compose the target configuration file using the concat module). The idea is that having these parts as distinct resources allows for the introduction of a combined type (i.e. fail2ban::monitor or fail2ban::concern or something) that eases the creation of filers and jails in the most common form, where they have a 1:1 relationship (so one does not need to separate that over multiple configuration items). Does that sound reasonable?
On 2016/12/02 13:16:21, mathias wrote: > https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... > File modules/fail2ban/manifests/init.pp (right): > > https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... > modules/fail2ban/manifests/init.pp:7: # [*jail_config*] > One last point: I'd like to rename this one to "jails". Background: One of the > follow-ups for this ticket will be the introduction of a named type called > fail2ban::jail (which will compose the target configuration file using the > concat module). The idea is that having these parts as distinct resources allows > for the introduction of a combined type (i.e. fail2ban::monitor or > fail2ban::concern or something) that eases the creation of filers and jails in > the most common form, where they have a 1:1 relationship (so one does not need > to separate that over multiple configuration items). Does that sound reasonable? +1 from my side. I think f.lopez also had something like this concat in mind earlier.
https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... File modules/fail2ban/manifests/init.pp (right): https://codereview.adblockplus.org/29364214/diff/29366529/modules/fail2ban/ma... modules/fail2ban/manifests/init.pp:7: # [*jail_config*] On 2016/12/02 13:16:21, mathias wrote: > One last point: I'd like to rename this one to "jails". Background: One of the > follow-ups for this ticket will be the introduction of a named type called > fail2ban::jail (which will compose the target configuration file using the > concat module). The idea is that having these parts as distinct resources allows > for the introduction of a combined type (i.e. fail2ban::monitor or > fail2ban::concern or something) that eases the creation of filers and jails in > the most common form, where they have a 1:1 relationship (so one does not need > to separate that over multiple configuration items). Does that sound reasonable? Yeah sounds good to me, that was actually my first idea about the config file.
LGTM. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
