Update the check_fail2ban script #157

Merged
merged 3 commits into from Apr 9, 2013

3 participants

@labynocle

Replace the check_fail2ban script by a new one which respects the Nagios specs (like status, output, perfdata, help...).

Also add a README which includes the content of f2ban.txt (which is now removed)

(PR according to a previous discussion)

labynocle added some commits Mar 26, 2013
@labynocle labynocle Replace the check_fail2ban script by a new one which respects the Nag…
…ios specs (like status, output, perfdata, help...).

Also add a README which includes the content of f2ban.txt (which is now removed)
d7d5228
@labynocle labynocle fix the script name to check_fail2ban everywhere c4d92fb
@yarikoptic yarikoptic and 1 other commented on an outdated diff Mar 27, 2013
files/nagios/check_fail2ban
#
-# please visit my website http://www.elchtest.eu or my personal WIKI http://wiki.elchtest.eu
+# ####################################################################
+
+# ####################################################################
+# GPL v3
@yarikoptic
Fail2Ban member
yarikoptic added a line comment Mar 27, 2013

minor: fail2ban is still under GPL >= 2, so it would be better if this piece also allows GPL v2, or do you have specific reservations?

@labynocle
labynocle added a line comment Mar 27, 2013

Good point!
Actually I left the copyright of my previous script (some kind of legacy :) ), but I have no reservation to change it in GPL v2.

At this point, I don't know the best practice: do I have to resend a new PR with the previous commits + a new one where I change the license?

@yarikoptic
Fail2Ban member
yarikoptic added a line comment Mar 29, 2013

just commit to the same branch and push -- usually that commit automagically gets added to the PR. If not -- let us know

@labynocle
labynocle added a line comment Apr 2, 2013

(I just pushed the version with a GPLv2 licence, hope it will be ok for you :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@coveralls

Changes Unknown when pulling 4473603 on labynocle:master into ** on fail2ban:master**.

View Details

@yarikoptic
Fail2Ban member

BTW is there a native nagios way to configure invocation of this script (eg env variables?)

I thought to ask myself and then Fabian Wenk also brought up similar ideas on the list: it is possible to run fail2ban in non-root mode, and we have such mode of operation available. So for ideal situation, it would be nice to disable sudo invocation... or may be provide the user to sudo to?

@labynocle

sudo is not mandatory actually.
I just note in the README file a way to run it if the nagios user has no permission to run fail2ban-client command.

So it's not necessary but usually the nagios user have only very restricted permission.

If you want I can update the README file to explain why you could have to use a sudo configuration, just let me know. :)

@yarikoptic
Fail2Ban member

I think I just misunderstood originally where I saw "sudo" ;) so it all looks good to me. I just hope that I (or someone else) gives it a try with a working nagios setup before we merge it

You might be interested to look at (or may be even refer in README?) one of the ways how fail2ban could be used for banning without root access: https://github.com/fail2ban/fail2ban/blob/HEAD/doc/run-rootless.txt
Debian distribution's init scripts even provide convenient settings/instructions on how to enable that:
http://github.com/fail2ban/fail2ban/blob/HEAD/debian/fail2ban.default

@yarikoptic yarikoptic commented on the diff Apr 9, 2013
files/nagios/check_fail2ban
+);
+
+print_usage() if ($help_value);
+print_version() if ($version_value);
+
+
+# Syntax check of your specified options
+# --------------------------------------
+
+print "DEBUG : fail2ban_client_path: $fail2ban_client_path\n" if ($verbose_value);
+if (($fail2ban_client_path eq "")) {
+ print $display.'- one or more following arguments are missing: fail2ban_client_path'."\n";
+ exit $ERRORS{"UNKNOWN"};
+}
+
+if(! -x $fail2ban_client_path) {
@yarikoptic
Fail2Ban member
yarikoptic added a line comment Apr 9, 2013

I will merge in a sec (if do not find anything else) but would be kind to extend check for if fail2ban-client is present there at all ;) ?

@labynocle
labynocle added a line comment Apr 9, 2013

Good point!
I can add it and it will become more easier for user to debug :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@yarikoptic yarikoptic commented on the diff Apr 9, 2013
files/nagios/check_fail2ban
-SECOND_CHECK=0
-STATE_OK=0
-STATE_CRITICAL=2
+### Test the connection to the fail2ban server
+my @command_output = `$fail2ban_cmd ping`;
+my $return_code = $?;
+if ($return_code) {
+ print $display.'CRITICAL - non-zero exit code during testing fail2ban-client ping, check if the server is running and if you have the good permissions';
@yarikoptic
Fail2Ban member
yarikoptic added a line comment Apr 9, 2013

shouldn't all print statements have trailing new line (\n)?

@yarikoptic
Fail2Ban member
yarikoptic added a line comment Apr 9, 2013

ok -- checked -- so within ' ' escape symbols are not treated, but they are within "", so IMHO for the best readability it is better just to have "CRITICAL - ...\n". Similar in other locations -- why not to have everything just within "" and even avoid using concatenation (with .) and use variables within the strings, e.g. "$display - the thresholds ... .\n"

or there is a catch? just wondering

@labynocle
labynocle added a line comment Apr 9, 2013

the \n is not necessary in this case.
Nagios removes the latest one (if it exists) for the display.
There is no real best practice about this, I cab add it if you want :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@yarikoptic yarikoptic commented on the diff Apr 9, 2013
files/nagios/check_fail2ban
+
+print_usage() if ($help_value);
+print_version() if ($version_value);
+
+
+# Syntax check of your specified options
+# --------------------------------------
+
+print "DEBUG : fail2ban_client_path: $fail2ban_client_path\n" if ($verbose_value);
+if (($fail2ban_client_path eq "")) {
+ print $display.'- one or more following arguments are missing: fail2ban_client_path'."\n";
+ exit $ERRORS{"UNKNOWN"};
+}
+
+if(! -x $fail2ban_client_path) {
+ print $display.' - '.$fail2ban_client_path.' is not executable by you'."\n";
@yarikoptic
Fail2Ban member
yarikoptic added a line comment Apr 9, 2013

I have no clue in perl -- so could you please educate me why you'."\n" instead of a simple you.\n'?

@labynocle
labynocle added a line comment Apr 9, 2013

Actualy the result is exactly the same, the difference is between ' the variables and special characters are not interpreted so it's just a bad habit from me :)

I can change the code to have the kind of print everywhere if you want

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@yarikoptic yarikoptic merged commit 4473603 into fail2ban:master Apr 9, 2013

1 check passed

Details default The Travis build passed
@yarikoptic
Fail2Ban member

yes -- if you could unify print statements appearance/functionality -- that would be great

Also -- I think it would be even better if you could craft a unittest which would basically just

  • probably derive from Transmitter test class in servertestcase.py which already starts a server
  • similar to other tests add some jail
  • invoke your script providing it with correct paths to the client and socket and testing all basic commands it provides.

if we had such a unittest -- we could have some assurance that things work as they should!

we might need to have perl installed for tests, but well -- that shouldn't be difficult ;)

@labynocle

Ok let me see what can I do with it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment