WIP: __fail2ban new type #379

Draft
smwltr wants to merge 1 commit from smwltr:fail2ban into master
3 changed files with 76 additions and 0 deletions
Showing only changes of commit 5e09834811 - Show all commits

View file

@ -0,0 +1,36 @@
#!/bin/sh
#
# 2016 Simon Walter (simon at explicit dot technology)
#
# This file is part of cdist.
#
# cdist is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# cdist is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with cdist. If not, see <http://www.gnu.org/licenses/>.
#
config_file="/etc/fail2ban/jail.conf"
if [ -f "$__object/parameter/enable-services" ]; then
enable_services="$(cat "$__object/parameter/enable-services")"
else
enable_services="$__object_id"
fi
services="$(echo $enable_services | sed -e 's/,/ /g')"
for service in $services
do
echo "[$(tput setaf 6)info$(tput sgr 0)] Enabling fail2ban for $service..." >&2
cat << EOF
perl -i -pe 'BEGIN{undef $/;} s/\[$service\].*[\n]*enabled.*=.*\n/\[$service\]\n\nenabled = true\n/g' $config_file
Review

I see two problems here:

  • The configuration should be generated and installed in the manifest, not in gencode-remote.
  • You should not use perl in core cdist types - we only assume POSIX sh on the remote hosts.
I see two problems here: * The configuration should be generated and installed in the manifest, not in gencode-remote. * You should not use `perl` in core cdist types - we only assume POSIX sh on the remote hosts.
EOF
done
echo "service fail2ban restart"

View file

@ -0,0 +1,39 @@
#!/bin/sh
#
# 2016 Simon Walter (simon at explicit dot technology)
#
# This file is part of cdist.
#
# cdist is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# cdist is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with cdist. If not, see <http://www.gnu.org/licenses/>.
#
os=$(cat "$__global/explorer/os")
case "$os" in
debian|ubuntu|devuan)
__package iptables --state present
require=__package/iptables __package fail2ban --state present
;;
centos)
__package epel-release --state present
require=__package/epel-release __package iptables --state present
require=__package/epel-release __package fail2ban --state present
;;
*)
echo "Your operating system ($os) is currently untested for ${__type##*/}." >&2
Review

Just say that the type does not support $os and exit.

Just say that the type does not support `$os` and exit.
Review

I'll need to make a testing set up for this. It's most likely that other OSes can use the same code, but I just haven't tested it.

I'll need to make a testing set up for this. It's most likely that other OSes can use the same code, but I just haven't tested it.
Review

Well, we only ship code that is known to be working :-)

Well, we only ship code that is known to be working :-)
echo "If it works, please add it." >&2
__package fail2ban --state present
;;
esac
Review

You also have to make sure that the fail2ban service starts on boot.

You also have to make sure that the fail2ban service starts on boot.
Review

Is that typically what we do?

Or do we expect the user to use __start_on_boot?

Maybe I should rather fail when there is no fail2ban package installed. Then this type would be only for configuring the services that you want fail2ban to monitor (jails).

I could make an opiton and default to start it. Or I could add __start_on_boot and __package as an example in the man page?

What fits more with the cdist design?

Is that typically what we do? Or do we expect the user to use `__start_on_boot`? Maybe I should rather fail when there is no fail2ban package installed. Then this type would be only for configuring the services that you want fail2ban to monitor (jails). I could make an opiton and default to start it. Or I could add `__start_on_boot` and `__package` as an example in the man page? What fits more with the cdist design?
Review

We usually install and enable (i.e. start_on_boot) the service within the type. Please run both __package and __start_on_boot in your type.

We usually install and enable (i.e. start_on_boot) the service within the type. Please run both `__package` and `__start_on_boot` in your type.
Review

IMO, forcing something other than the default is not my style, but if that is standard cdist style, so be it.

IMO, forcing something other than the default is not my style, but if that is standard cdist style, so be it.

View file

@ -0,0 +1 @@
enable-services
Review

I find this parameter confusing - could you rename it? e.g. jail-service ?

I find this parameter confusing - could you rename it? e.g. `jail-service` ?
Review

Would it be enable-jail? I suppose we need disable-jail too. Or?

Would it be `enable-jail`? I suppose we need `disable-jail` too. Or?
Review

The type could generate configuration for the services specified in --jail-service and remove anything that isn't provided. You could make --jail-service an optional_multiple parameter.

Also mind that if we need to add per-service/jail configuration, a second __fail2ban_jail service will be needed (so that every jailed service can have its own configuration).

The type could generate configuration for the services specified in `--jail-service` and remove anything that isn't provided. You could make `--jail-service` an optional_multiple parameter. Also mind that if we need to add per-service/jail configuration, a second __fail2ban_jail service will be needed (so that every jailed service can have its own configuration).
Review

IIUC, that would mean that cdist team be responsible for shipping working tested config for fail2ban.

Do you think that is out of scope? Have you seen how many jails are in the default config?

That's the reason why all this type does is allow you to enable a jail that is already in the config.

Before I remove the perl code, I'd like to understand this more. If we are just dis/enabling jails from the config, I can use sed. But generating a config for fail2ban requires much more code and maintenance. It doesn't make sense to me.

WDYT?

IIUC, that would mean that cdist team be responsible for shipping working tested config for fail2ban. Do you think that is out of scope? Have you seen how many jails are in the default config? That's the reason why all this type does is allow you to enable a jail that is already in the config. Before I remove the perl code, I'd like to understand this more. If we are just dis/enabling jails from the config, I can use sed. But generating a config for fail2ban requires much more code and maintenance. It doesn't make sense to me. WDYT?
Review

IT would be a generic type to deploy a file in /etc/fail2ban/jail.d/myjail.conf (on debian at least). It doesn't have to support every configuration parameter, but should allow for custom configuration.

Its simplest form could looks like:

 __fail2ban_jail myjail --config <<- EOF
[apache-overflows]
enabled  = true
port     = http,https
filter   = apache-overflows
logpath  = /var/log/apache*/*error.log
maxretry = 2
EOF

I would also run fail2ban-client --test or similar in gencode-remote before reloading the configuration.

IT would be a generic type to deploy a file in /etc/fail2ban/jail.d/myjail.conf (on debian at least). It doesn't have to support every configuration parameter, but should allow for custom configuration. Its simplest form could looks like: ``` __fail2ban_jail myjail --config <<- EOF [apache-overflows] enabled = true port = http,https filter = apache-overflows logpath = /var/log/apache*/*error.log maxretry = 2 EOF ``` I would also run ` fail2ban-client --test` or similar in gencode-remote before reloading the configuration.