Issue 2487 - Introduce fail2ban module

modules/fail2ban/manifests/init.pp

 # == Class: fail2ban
 #
 # Create and maintain fail2ban (http://www.fail2ban.org/) setups.
 #
 # == Parameters:
 #
-# [*jail_config*]
-#   Adds jail.local to the default configuration of fail2ban 

[f.nicolaisen, 2016/11/25 15:09:09]

Unnecessary white space at end of line.

+# [*jails*]
+#   Provisions a jail.local adjacent to the default configuration.
+#   By default entries will have the following parameters:
+#     'enabled' => 'true',
+#     'port' => 'all',
+#     'maxretry' => 6,
+#     'banaction' => 'iptables-allports',
+#     'bantime' => 3600,
+#
+#   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.
 #
 # [*package*]
 #   Overwrite the default package options, to fine-tune the target version (i.e.
-#   ensure => 'latest') or remove Fluentd (ensure => 'absent' or 'purged')

[mathias, 2016/11/24 16:08:48]

Fluentd?

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

+#   ensure => 'latest') or remove fail2ban (ensure => 'absent' or 'purged')
 #
 # [*service*]
 #   Overwrite the default service options.
 #
 # [*filters*]
-#   Adds adittional filters to the filters.d folder
+#   Adds adittional filters to the filters.d folder.
+#
 # === Examples:
 #
-# class {'fail2ban':
-#   package => {ensure => 'present',},
-#   service => {},
-#   jail_config => {
-#     'wordpress' => {
-#       logpath => '/var/log/nginx/access.log',        

[f.nicolaisen, 2016/11/25 15:09:09]

Unnecessary white spaces at end of line.

-#     }
-#   },
-#   filters => {
-#     'wordpress' => {
-#       failregex => [
-#         '^<HOST>.*\"WordPress\/.*',
-#       ],
-#     }
-#   },
-# }

[mathias, 2016/11/24 16:08:48]

The example code is not properly indented. Also it should follow the same style
practices as regular code within this repository.

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

+#  class {'fail2ban':
+#    package => {ensure => 'present',},
+#    service => {},
+#    jails => {
+#      'CVE-2013-0235' => {
+#        'logpath' => '/var/log/nginx/access_log_hg',
+#        'banaction' => 'iptables-multiport',
+#        'port' => 'https, http',
+#      }
+#    },
+#    filters => {
+#      'CVE-2013-0235' => {
+#        regexes => [
+#          '^<HOST>.*\"WordPress\/.*',
+#        ],
+#      }
+#    },
+#  }
+#
 class fail2ban (
-  $package = {},

[mathias, 2016/11/24 16:08:48]

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.

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

-  $service = {},
-  $jail_config = {},
-  $filters = {},
+  $package = hiera('fail2ban::package', {}),
+  $service = hiera('fail2ban::service', {}),
+  $jails = hiera('fail2ban::jails', {}),
+  $filters = hiera('fail2ban::filters', {}),
 ) {
 
   include stdlib
 
+  $jail_default = {
+    'enabled' => 'true',
+    'port' => 'all',
+    'maxretry' => 6,
+    'banaction' => 'iptables-allports',
+    'bantime' => 3600,
+  }
+
   ensure_resource('package', $title, $package)
 
-  # Used as default $ensure parameter for most resources below
   $ensure = getparam(Package[$title], 'ensure') ? {
-    /^(absent|purged|held)$/ => 'absent',

[mathias, 2016/11/24 16:08:48]

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.

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

+    /^(absent|purged)$/ => 'absent',
     default => 'present',
   }
 
-  # Service resources don't properly support the concept of absence
-  if ($ensure == 'present') or ($service['ensure'] != undef) {

[mathias, 2016/11/24 16:08:48]

Why checking for $service['ensure'] being defined? Shouldn't the default be
totally fine?

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

+  if ($ensure == 'present') {
 
-    ensure_resource('service', $title, $service)
+    ensure_resource('service', $title, merge({
+      hasrestart => true,
+      hasstatus => true,
+    }, $service))
+
     # See modules/fail2ban/manifests/filter.pp
     create_resources('fail2ban::filter', $filters)
-    

[f.nicolaisen, 2016/11/25 15:09:09]

Unnecessary white spaces here.

-    # According to the docs one can also enable filters that are
-    # already in there, so the config file should be done appart.

[mathias, 2016/11/24 16:08:48]

I don't really get this point, but I assume you meant "separately" when you
wrote "appart"?

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

-    if jail_config != undef {

[mathias, 2016/11/24 16:08:48]

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. 

[f.lopez, 2016/11/25 15:13:49]

One can have filters without activating them, so you can still create filters
without activating them.

-      file {'/etc/fail2ban/jail.local':
-        ensure => present,
-        group => 'root',
-        mode => '0644',
-        owner => 'root',
-        content => template("fail2ban/jail.erb"),
-        notify => Service[$title],
-      }
+
+    file {'/etc/fail2ban/jail.local':
+      ensure => present,
+      group => 'root',
+      mode => '0644',
+      owner => 'root',
+      content => template("fail2ban/jail.erb"),
+      notify => Service['fail2ban'],
+      require => Package['fail2ban'],
     }
-    

[f.nicolaisen, 2016/11/25 15:09:09]

Unnecessary white spaces here.

+
+    Package[$title] -> File['/etc/fail2ban/jail.local']
     Service[$title] <~ Package[$title]

[mathias, 2016/11/24 16:08:48]

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).

[f.lopez, 2016/11/25 15:13:49]

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.

   }
 
-  Package[$title] -> File['/etc/fail2ban/jail.local']

[mathias, 2016/11/24 16:08:48]

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.

[f.lopez, 2016/11/25 15:13:49]

Acknowledged.

+}
 
-
-}