From a544c5abac03dbc124708afbcd1912cbd9f2686a Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 8 Jul 2016 12:22:38 +0200 Subject: [PATCH 01/26] sshd-filter: recognized "Failed publickey for" now (gh-1477) + improved regexp (not anchored now to recognize all "Failed anything for ... from " ChangeLog entry added --- ChangeLog | 3 +++ config/filter.d/sshd.conf | 2 +- fail2ban/tests/files/logs/sshd | 3 ++- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index c4444e2a..f441b99b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -30,6 +30,9 @@ releases. - Extended failregex and test cases to handle ASSP V1 and V2 (gh-1494) * `filter.d/postfix-sasl.conf` - Allow for having no trailing space after 'failed:' (gh-1497) +* filter.d/sshd.conf + - recognized "Failed publickey for" (gh-1477); + - optimized failregex to match all of "Failed any-method for ... from " (gh-1479) ### New Features diff --git a/config/filter.d/sshd.conf b/config/filter.d/sshd.conf index eeb1518e..7003d1b2 100644 --- a/config/filter.d/sshd.conf +++ b/config/filter.d/sshd.conf @@ -20,7 +20,7 @@ _daemon = sshd failregex = ^%(__prefix_line)s(?:error: PAM: )?[aA]uthentication (?:failure|error|failed) for .* from ( via \S+)?\s*$ ^%(__prefix_line)s(?:error: PAM: )?User not known to the underlying authentication module for .* from \s*$ - ^%(__prefix_line)sFailed \S+ for .*? from (?: port \d*)?(?: ssh\d*)?(: (ruser .*|(\S+ ID \S+ \(serial \d+\) CA )?\S+ %(__md5hex)s(, client user ".*", client host ".*")?))?\s*$ + ^%(__prefix_line)sFailed \S+ for .*? from (?: port \d*)?(?: ssh\d*)?(?:: | (?!from )|$) ^%(__prefix_line)sROOT LOGIN REFUSED.* FROM \s*$ ^%(__prefix_line)s[iI](?:llegal|nvalid) user .* from \s*$ ^%(__prefix_line)sUser .+ from not allowed because not listed in AllowUsers\s*$ diff --git a/fail2ban/tests/files/logs/sshd b/fail2ban/tests/files/logs/sshd index 7baf4be7..a6b07222 100644 --- a/fail2ban/tests/files/logs/sshd +++ b/fail2ban/tests/files/logs/sshd @@ -118,6 +118,8 @@ Sep 29 17:15:02 spaceman sshd[12946]: Failed password for user from 127.0.0.1 po # failJSON: { "time": "2004-11-11T08:04:51", "match": true , "host": "127.0.0.1", "desc": "Injecting on username ssh 'from 10.10.1.1'@localhost" } Nov 11 08:04:51 redbamboo sshd[2737]: Failed password for invalid user from 10.10.1.1 from 127.0.0.1 port 58946 ssh2 +# failJSON: { "time": "2005-07-05T18:22:44", "match": true , "host": "127.0.0.1", "desc": "Failed publickey for ..." } +Jul 05 18:22:44 mercury sshd[4669]: Failed publickey for graysky from 127.0.0.1 port 37954 ssh2: RSA SHA256:v3dpapGleDaUKf$4V1vKyR9ZyUgjaJAmoCTcb2PLljI # failJSON: { "match": false } Nov 23 21:50:19 sshd[8148]: Disconnecting: Too many authentication failures for root [preauth] @@ -161,4 +163,3 @@ Apr 27 13:02:04 host sshd[29116]: Received disconnect from 1.2.3.4: 11: Normal S # Match sshd auth errors on OpenSUSE systems # failJSON: { "time": "2015-04-16T20:02:50", "match": true , "host": "222.186.21.217", "desc": "Authentication for user failed" } 2015-04-16T18:02:50.321974+00:00 host sshd[2716]: pam_unix(sshd:auth): authentication failure; logname= uid=0 euid=0 tty=ssh ruser= rhost=222.186.21.217 user=root - From 2c54f9046970046265f6f6f5d18cdca19e331940 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 18 Aug 2016 21:34:09 +0200 Subject: [PATCH 02/26] sshd-filter: better universal regexp, that matches more complex different injects, using conditional expressions (on username and auth-info section), see new test cases also. --- ChangeLog | 1 + config/filter.d/sshd.conf | 2 +- fail2ban/tests/files/logs/sshd | 4 ++++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index f441b99b..2bc060ee 100644 --- a/ChangeLog +++ b/ChangeLog @@ -33,6 +33,7 @@ releases. * filter.d/sshd.conf - recognized "Failed publickey for" (gh-1477); - optimized failregex to match all of "Failed any-method for ... from " (gh-1479) + - eliminated possible complex injections (on user-name resp. auth-info, see gh-1479) ### New Features diff --git a/config/filter.d/sshd.conf b/config/filter.d/sshd.conf index 7003d1b2..9a3d40f0 100644 --- a/config/filter.d/sshd.conf +++ b/config/filter.d/sshd.conf @@ -20,7 +20,7 @@ _daemon = sshd failregex = ^%(__prefix_line)s(?:error: PAM: )?[aA]uthentication (?:failure|error|failed) for .* from ( via \S+)?\s*$ ^%(__prefix_line)s(?:error: PAM: )?User not known to the underlying authentication module for .* from \s*$ - ^%(__prefix_line)sFailed \S+ for .*? from (?: port \d*)?(?: ssh\d*)?(?:: | (?!from )|$) + ^%(__prefix_line)sFailed \S+ for (?Pinvalid user )?(?P(?P\S+)|(?(cond_inv)(?:(?! from ).)*?|[^:]+)) from (?: port \d+)?(?: ssh\d*)?(?(cond_user):|(?:(?:(?! from ).)*)$) ^%(__prefix_line)sROOT LOGIN REFUSED.* FROM \s*$ ^%(__prefix_line)s[iI](?:llegal|nvalid) user .* from \s*$ ^%(__prefix_line)sUser .+ from not allowed because not listed in AllowUsers\s*$ diff --git a/fail2ban/tests/files/logs/sshd b/fail2ban/tests/files/logs/sshd index a6b07222..c53bbadb 100644 --- a/fail2ban/tests/files/logs/sshd +++ b/fail2ban/tests/files/logs/sshd @@ -117,6 +117,10 @@ Sep 29 17:15:02 spaceman sshd[12946]: Failed password for user from 127.0.0.1 po # failJSON: { "time": "2004-11-11T08:04:51", "match": true , "host": "127.0.0.1", "desc": "Injecting on username ssh 'from 10.10.1.1'@localhost" } Nov 11 08:04:51 redbamboo sshd[2737]: Failed password for invalid user from 10.10.1.1 from 127.0.0.1 port 58946 ssh2 +# failJSON: { "time": "2004-11-11T08:04:52", "match": true , "host": "127.0.0.1", "desc": "More complex injecting on username ssh 'test from 10.10.1.2 port 55555 ssh2'@localhost" } +Nov 11 08:04:52 redbamboo sshd[2737]: Failed password for invalid user test from 10.10.1.2 port 55555 ssh2 from 127.0.0.1 port 58946 ssh2 +# failJSON: { "time": "2004-11-11T08:04:52", "match": true , "host": "127.0.0.1", "desc": "More complex injecting on auth-info ssh test@localhost, auth-info: ' from 10.10.1.2 port 55555 ssh2'" } +Nov 11 08:04:52 redbamboo sshd[2737]: Failed password for invalid user test from 127.0.0.1 port 58946 ssh2: from 10.10.1.2 port 55555 ssh2 # failJSON: { "time": "2005-07-05T18:22:44", "match": true , "host": "127.0.0.1", "desc": "Failed publickey for ..." } Jul 05 18:22:44 mercury sshd[4669]: Failed publickey for graysky from 127.0.0.1 port 37954 ssh2: RSA SHA256:v3dpapGleDaUKf$4V1vKyR9ZyUgjaJAmoCTcb2PLljI From 84c3eb3e0ee08058d23cc81b7954813287dd29f8 Mon Sep 17 00:00:00 2001 From: sebres Date: Sat, 15 Oct 2016 14:53:08 +0200 Subject: [PATCH 03/26] filter.d/sendmail-reject.conf: double space (should be by missing dns-host only) Closes #1578 --- config/filter.d/sendmail-reject.conf | 2 +- fail2ban/tests/files/logs/sendmail-reject | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/config/filter.d/sendmail-reject.conf b/config/filter.d/sendmail-reject.conf index 93b8343c..20d3648e 100644 --- a/config/filter.d/sendmail-reject.conf +++ b/config/filter.d/sendmail-reject.conf @@ -23,7 +23,7 @@ _daemon = (?:(sm-(mta|acceptingconnections)|sendmail)) failregex = ^%(__prefix_line)s\w{14}: ruleset=check_rcpt, arg1=(?P<\S+@\S+>), relay=(\S+ )?\[\]( \(may be forged\))?, reject=(550 5\.7\.1 (?P=email)\.\.\. Relaying denied\. (IP name possibly forged \[(\d+\.){3}\d+\]|Proper authentication required\.|IP name lookup failed \[(\d+\.){3}\d+\])|553 5\.1\.8 (?P=email)\.\.\. Domain of sender address \S+ does not exist|550 5\.[71]\.1 (?P=email)\.\.\. (Rejected: .*|User unknown))$ ^%(__prefix_line)sruleset=check_relay, arg1=(?P\S+), arg2=, relay=((?P=dom) )?\[(\d+\.){3}\d+\]( \(may be forged\))?, reject=421 4\.3\.2 (Connection rate limit exceeded\.|Too many open connections\.)$ - ^%(__prefix_line)s\w{14}: rejecting commands from (\S+ )?\[\] due to pre-greeting traffic after \d+ seconds$ + ^%(__prefix_line)s\w{14}: rejecting commands from (\S* )?\[\] due to pre-greeting traffic after \d+ seconds$ ^%(__prefix_line)s\w{14}: (\S+ )?\[\]: ((?i)expn|vrfy) \S+ \[rejected\]$ ^(?P<__prefix>%(__prefix_line)s\w+: )<[^@]+@[^>]+>\.\.\. No such user here(?P=__prefix)from=<[^@]+@[^>]+>, size=\d+, class=\d+, nrcpts=\d+, bodytype=\w+, proto=E?SMTP, daemon=MTA, relay=\S+ \[\]$ diff --git a/fail2ban/tests/files/logs/sendmail-reject b/fail2ban/tests/files/logs/sendmail-reject index b326cf43..70d4dde6 100644 --- a/fail2ban/tests/files/logs/sendmail-reject +++ b/fail2ban/tests/files/logs/sendmail-reject @@ -40,6 +40,8 @@ Feb 19 18:01:50 batman sm-mta[78152]: ruleset=check_relay, arg1=[196.213.73.146] # failJSON: { "time": "2005-02-27T10:53:06", "match": true , "host": "209.15.212.253" } Feb 27 10:53:06 batman sm-mta[44307]: s1R9r60D044307: rejecting commands from [209.15.212.253] due to pre-greeting traffic after 0 seconds +# failJSON: { "time": "2005-02-27T10:53:07", "match": true , "host": "1.2.3.4" } +Feb 27 10:53:07 strange sm-mta[18001]: u9A0GtpL018001: rejecting commands from example.com [1.2.3.4] due to pre-greeting traffic after 6 seconds # failJSON: { "time": "2005-02-27T15:44:18", "match": true , "host": "41.204.78.137" } Feb 27 15:44:18 batman sm-mta[87838]: s1REiHdq087838: ruleset=check_rcpt, arg1=, relay=[41.204.78.137], reject=550 5.7.1 ... Relaying denied. IP name lookup failed [41.204.78.137] From 519e355bf2cb524775dfae4087c577f9292cf9a1 Mon Sep 17 00:00:00 2001 From: sebres Date: Sat, 15 Oct 2016 14:59:36 +0200 Subject: [PATCH 04/26] ChangeLog entry added --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 57737828..2a6b5733 100644 --- a/ChangeLog +++ b/ChangeLog @@ -36,6 +36,8 @@ releases. - Allow for having no trailing space after 'failed:' (gh-1497) * `filter.d/vsftpd.conf` - Optional reason part in message after FAIL LOGIN (gh-1543) +* `filter.d/sendmail-reject.conf` + - removed mandatory double space (if dns-host available, gh-1579) ### New Features From 4e252be76f1e7a307fb626af9636e745f93547ec Mon Sep 17 00:00:00 2001 From: "Serg G. Brester" Date: Tue, 25 Oct 2016 11:01:32 +0200 Subject: [PATCH 05/26] Update FILTERS closes #1591 --- FILTERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FILTERS b/FILTERS index 10113dfc..e114973a 100644 --- a/FILTERS +++ b/FILTERS @@ -227,7 +227,7 @@ Regular expressions (failregex, ignoreregex) assume that the date/time has been removed from the log line (this is just how fail2ban works internally ATM). If the format is like ' error 1.2.3.4 is evil' then you need to match -the < at the start so regex should be similar to '^<> is evil$' using +the <> at the start so regex should be similar to '^<> error is evil$' using where the IP/domain name appears in the log line. The following general rules apply to regular expressions: From 189e70d99cc38f09183c26b86cfd880039763314 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 18 Nov 2016 14:42:29 +0100 Subject: [PATCH 06/26] processLine etc. rewritten: - normalize calling parameters (persistent parameters moved from function arguments to filter member variables) - save last line as lambda instead of return it as string (lazy convert of process line tuple to string on demand, needed in fail2ban-regex only) --- fail2ban/client/fail2banregex.py | 6 +++++- fail2ban/server/filter.py | 28 +++++++++++++++++----------- fail2ban/tests/samplestestcase.py | 6 ++++-- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/fail2ban/client/fail2banregex.py b/fail2ban/client/fail2banregex.py index 4096ac4e..a5aad76f 100644 --- a/fail2ban/client/fail2banregex.py +++ b/fail2ban/client/fail2banregex.py @@ -247,6 +247,9 @@ class Fail2banRegex(object): self.raw = True if opts.raw else False if opts.usedns: self._filter.setUseDns(opts.usedns) + self._filter.returnRawHost = self.raw + self._filter.checkFindTime = False + self._filter.checkAllRegex = True def decode_line(self, line): return FileContainer.decode_line('', self.encoding, line) @@ -350,7 +353,8 @@ class Fail2banRegex(object): orgLineBuffer = self._filter._Filter__lineBuffer fullBuffer = len(orgLineBuffer) >= self._filter.getMaxLines() try: - line, ret = self._filter.processLine(line, date, checkAllRegex=True, returnRawHost=self.raw) + ret = self._filter.processLine(line, date) + line = self._filter.processedLine() for match in ret: # Append True/False flag depending if line was matched by # more than one regex diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index 9b1aefd6..2487b8f5 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -90,6 +90,12 @@ class Filter(JailThread): ## Error counter (protected, so can be used in filter implementations) ## if it reached 100 (at once), run-cycle will go idle self._errors = 0 + ## return raw host (host is not dns): + self.returnRawHost = False + ## check each regex (used for test purposes): + self.checkAllRegex = False + ## if true ignores obsolete failures (failure time < now - findTime): + self.checkFindTime = True ## Ticks counter self.ticks = 0 @@ -455,8 +461,7 @@ class Filter(JailThread): return False - def processLine(self, line, date=None, returnRawHost=False, - checkAllRegex=False, checkFindTime=False): + def processLine(self, line, date=None): """Split the time portion from log msg and return findFailures on them """ if date: @@ -476,14 +481,15 @@ class Filter(JailThread): else: tupleLine = (l, "", "", None) - return "".join(tupleLine[::2]), self.findFailure( - tupleLine, date, returnRawHost, checkAllRegex, checkFindTime) + # save last line (lazy convert of process line tuple to string on demand): + self.processedLine = lambda: "".join(tupleLine[::2]) + return self.findFailure(tupleLine, date) def processLineAndAdd(self, line, date=None): """Processes the line for failures and populates failManager """ try: - for element in self.processLine(line, date, checkFindTime=True)[1]: + for element in self.processLine(line, date): ip = element[1] unixTime = element[2] lines = element[3] @@ -539,10 +545,10 @@ class Filter(JailThread): # to find the logging time. # @return a dict with IP and timestamp. - def findFailure(self, tupleLine, date=None, returnRawHost=False, - checkAllRegex=False, checkFindTime=False): + def findFailure(self, tupleLine, date=None): failList = list() + returnRawHost = self.returnRawHost cidr = IPAddr.CIDR_UNSPEC if self.__useDns == "raw": returnRawHost = True @@ -577,7 +583,7 @@ class Filter(JailThread): timeText = self.__lastTimeText or "".join(tupleLine[::2]) date = self.__lastDate - if checkFindTime and date is not None and date < MyTime.time() - self.getFindTime(): + if self.checkFindTime and date is not None and date < MyTime.time() - self.getFindTime(): logSys.log(5, "Ignore line since time %s < %s - %s", date, MyTime.time(), self.getFindTime()) return failList @@ -598,7 +604,7 @@ class Filter(JailThread): # The ignoreregex matched. Remove ignored match. self.__lineBuffer = failRegex.getUnmatchedTupleLines() logSys.log(7, "Matched ignoreregex and was ignored") - if not checkAllRegex: + if not self.checkAllRegex: break else: continue @@ -641,7 +647,7 @@ class Filter(JailThread): ip = IPAddr(fid, IPAddr.CIDR_RAW) failList.append([failRegexIndex, ip, date, failRegex.getMatchedLines(), fail]) - if not checkAllRegex: + if not self.checkAllRegex: break else: ips = DNSUtils.textToIp(host, self.__useDns) @@ -649,7 +655,7 @@ class Filter(JailThread): for ip in ips: failList.append([failRegexIndex, ip, date, failRegex.getMatchedLines(), fail]) - if not checkAllRegex: + if not self.checkAllRegex: break except RegexException as e: # pragma: no cover - unsure if reachable logSys.error(e) diff --git a/fail2ban/tests/samplestestcase.py b/fail2ban/tests/samplestestcase.py index 31b1812e..55dffdb0 100644 --- a/fail2ban/tests/samplestestcase.py +++ b/fail2ban/tests/samplestestcase.py @@ -44,6 +44,9 @@ class FilterSamplesRegex(unittest.TestCase): def setUp(self): """Call before every test case.""" self.filter = Filter(None) + self.filter.returnRawHost = True + self.filter.checkAllRegex = True + self.filter.checkFindTime = False self.filter.active = True setUpMyTime() @@ -111,8 +114,7 @@ def testSampleRegexsFactory(name, basedir): else: faildata = {} - ret = self.filter.processLine( - line, returnRawHost=True, checkAllRegex=True)[1] + ret = self.filter.processLine(line) if not ret: # Check line is flagged as none match self.assertFalse(faildata.get('match', True), From c442569b63028c573389a673f7473a4731d43567 Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 21 Nov 2016 16:35:33 +0100 Subject: [PATCH 07/26] executeCmd: added possibility to select success return codes ignorecommand: both return codes (0, 1) are success codes now, so no errors will be logged + test cases extended to check this (and error case) --- fail2ban/server/action.py | 4 ++-- fail2ban/server/filter.py | 3 ++- fail2ban/server/utils.py | 12 +++++++----- fail2ban/tests/files/ignorecommand.py | 3 +++ fail2ban/tests/filtertestcase.py | 4 ++++ 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index 976adb20..62aa51dc 100644 --- a/fail2ban/server/action.py +++ b/fail2ban/server/action.py @@ -584,7 +584,7 @@ class CommandAction(ActionBase): return self.executeCmd(realCmd, self.timeout) @staticmethod - def executeCmd(realCmd, timeout=60): + def executeCmd(realCmd, timeout=60, **kwargs): """Executes a command. Parameters @@ -613,6 +613,6 @@ class CommandAction(ActionBase): _cmd_lock.acquire() try: - return Utils.executeCmd(realCmd, timeout, shell=True, output=False) + return Utils.executeCmd(realCmd, timeout, shell=True, output=False, **kwargs) finally: _cmd_lock.release() diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index 2487b8f5..d263c901 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -455,7 +455,8 @@ class Filter(JailThread): if self.__ignoreCommand: command = CommandAction.replaceTag(self.__ignoreCommand, { 'ip': ip } ) logSys.debug('ignore command: ' + command) - ret_ignore = CommandAction.executeCmd(command) + ret, ret_ignore = CommandAction.executeCmd(command, success_codes=(0, 1)) + ret_ignore = ret and ret_ignore == 0 self.logIgnoreIp(ip, log_ignore and ret_ignore, ignore_source="command") return ret_ignore diff --git a/fail2ban/server/utils.py b/fail2ban/server/utils.py index 6ed3d8fa..6d74d2db 100644 --- a/fail2ban/server/utils.py +++ b/fail2ban/server/utils.py @@ -110,7 +110,7 @@ class Utils(): return flags @staticmethod - def executeCmd(realCmd, timeout=60, shell=True, output=False, tout_kill_tree=True): + def executeCmd(realCmd, timeout=60, shell=True, output=False, tout_kill_tree=True, success_codes=(0,)): """Executes a command. Parameters @@ -178,7 +178,7 @@ class Utils(): if not popen: return False if not output else (False, stdout, stderr, retcode) - std_level = retcode == 0 and logging.DEBUG or logging.ERROR + std_level = logging.DEBUG if retcode in success_codes else logging.ERROR # if we need output (to return or to log it): if output or std_level >= logSys.getEffectiveLevel(): # if was timeouted (killed/terminated) - to prevent waiting, set std handles to non-blocking mode. @@ -208,8 +208,8 @@ class Utils(): popen.stderr.close() success = False - if retcode == 0: - logSys.debug("%-.40s -- returned successfully", realCmd) + if retcode in success_codes: + logSys.debug("%-.40s -- returned successfully %i", realCmd, retcode) success = True elif retcode is None: logSys.error("%-.40s -- unable to kill PID %i", realCmd, popen.pid) @@ -223,7 +223,9 @@ class Utils(): logSys.error("%-.40s -- returned %i", realCmd, retcode) if msg: logSys.info("HINT on %i: %s", retcode, msg % locals()) - return success if not output else (success, stdout, stderr, retcode) + if output: + return success, stdout, stderr, retcode + return success if len(success_codes) == 1 else (success, retcode) @staticmethod def wait_for(cond, timeout, interval=None): diff --git a/fail2ban/tests/files/ignorecommand.py b/fail2ban/tests/files/ignorecommand.py index 7011b51b..8c115006 100755 --- a/fail2ban/tests/files/ignorecommand.py +++ b/fail2ban/tests/files/ignorecommand.py @@ -1,5 +1,8 @@ #!/usr/bin/env fail2ban-python import sys +if len(sys.argv) != 2 or sys.argv[1] == "": + sys.stderr.write('usage: ignorecommand IP') + exit(10) if sys.argv[1] == "10.0.0.1": exit(0) exit(1) diff --git a/fail2ban/tests/filtertestcase.py b/fail2ban/tests/filtertestcase.py index 2b57ce47..a6f491dd 100644 --- a/fail2ban/tests/filtertestcase.py +++ b/fail2ban/tests/filtertestcase.py @@ -377,6 +377,10 @@ class IgnoreIP(LogCaptureTestCase): self.filter.setIgnoreCommand(sys.executable + ' ' + os.path.join(TEST_FILES_DIR, "ignorecommand.py ")) self.assertTrue(self.filter.inIgnoreIPList("10.0.0.1")) self.assertFalse(self.filter.inIgnoreIPList("10.0.0.0")) + self.assertLogged("returned successfully 0", "returned successfully 1", all=True) + self.pruneLog() + self.assertFalse(self.filter.inIgnoreIPList("")) + self.assertLogged("usage: ignorecommand IP", "returned 10", all=True) def testIgnoreCauseOK(self): ip = "93.184.216.34" From 701abfd250c7f2e68dd2d6f4490fec624ec8ade5 Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 21 Nov 2016 17:00:08 +0100 Subject: [PATCH 08/26] ChangeLog entry added + indentation fix (space-tab replacement) --- ChangeLog | 1 + fail2ban/tests/files/ignorecommand.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6d7ecf42..05a2e38f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -28,6 +28,7 @@ TODO: implementing of options resp. other tasks from PR #1346 * Pyinotify-backend: stability fix for sporadically errors in multi-threaded environment (without lock) * Fixed sporadically error in testCymruInfoNxdomain, because of unsorted values +* Misleading errors logged from ignorecommand in success case on retcode 1 (gh-1194) ### New Features * IPv6 support: diff --git a/fail2ban/tests/files/ignorecommand.py b/fail2ban/tests/files/ignorecommand.py index 8c115006..da48ff91 100755 --- a/fail2ban/tests/files/ignorecommand.py +++ b/fail2ban/tests/files/ignorecommand.py @@ -1,8 +1,8 @@ #!/usr/bin/env fail2ban-python import sys if len(sys.argv) != 2 or sys.argv[1] == "": - sys.stderr.write('usage: ignorecommand IP') - exit(10) + sys.stderr.write('usage: ignorecommand IP') + exit(10) if sys.argv[1] == "10.0.0.1": exit(0) exit(1) From 261f875748cba31369bdedca5add04059b1c4a5c Mon Sep 17 00:00:00 2001 From: sebres Date: Mon, 21 Nov 2016 19:00:53 +0100 Subject: [PATCH 09/26] Fixed sporadic tab-replacement (`\n\t` instead of `\n ` by word wrapping) in mime content of smtp-message in test cases, see https://github.com/fail2ban/fail2ban/pull/1410#issuecomment-262000804 --- fail2ban/tests/action_d/test_smtp.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fail2ban/tests/action_d/test_smtp.py b/fail2ban/tests/action_d/test_smtp.py index 5c8b1923..bfbf7b61 100644 --- a/fail2ban/tests/action_d/test_smtp.py +++ b/fail2ban/tests/action_d/test_smtp.py @@ -21,6 +21,7 @@ import os import smtpd import threading import unittest +import re import sys if sys.version_info >= (3, 3): import importlib @@ -38,7 +39,9 @@ class TestSMTPServer(smtpd.SMTPServer): self.peer = peer self.mailfrom = mailfrom self.rcpttos = rcpttos - self.data = data + self.org_data = data + # replace new line (with tab or space) for possible mime translations (word wrap): + self.data = re.sub(r"\n[\t ]", " ", data) class SMTPActionTest(unittest.TestCase): @@ -104,9 +107,9 @@ class SMTPActionTest(unittest.TestCase): self.assertEqual(self.smtpd.rcpttos, ["root"]) subject = "Subject: [Fail2Ban] %s: banned %s" % ( self.jail.name, aInfo['ip']) - self.assertIn(subject, self.smtpd.data.replace("\n", "")) - self.assertTrue( - "%i attempts" % aInfo['failures'] in self.smtpd.data) + self.assertIn(subject, self.smtpd.data) + self.assertIn( + "%i attempts" % aInfo['failures'], self.smtpd.data) self.action.matches = "matches" self.action.ban(aInfo) From 528a7a5abbf388def344387cd77f8f2fb54b532d Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 11:14:27 +0100 Subject: [PATCH 10/26] systemd service update: - starting service in normal mode (without forking) - does not restart if service exited normally (exit-code 0, e.g. stopped via fail2ban-client) - does not restart if service can not start (exit-code 255, e.g. wrong configuration, etc.) - service can be additionally started/stopped with commands (fail2ban-client, fail2ban-server) --- files/fail2ban.service | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/files/fail2ban.service b/files/fail2ban.service index e79faed1..5b661100 100644 --- a/files/fail2ban.service +++ b/files/fail2ban.service @@ -5,12 +5,13 @@ After=network.target iptables.service firewalld.service PartOf=iptables.service firewalld.service [Service] -Type=forking -ExecStart=/usr/bin/fail2ban-client -x start +Type=simple +ExecStart=/usr/bin/fail2ban-server -xf start ExecStop=/usr/bin/fail2ban-client stop ExecReload=/usr/bin/fail2ban-client reload PIDFile=/var/run/fail2ban/fail2ban.pid -Restart=always +Restart=on-failure +RestartPreventExitStatus=0 255 [Install] WantedBy=multi-user.target From 77dc5a334c0e1ba79482a7799fefef27b6df413e Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:19:09 +0100 Subject: [PATCH 11/26] really skips invalid jails (because of theirs wrong configuration) - server starts nevertheless, as long as one jail was successful configured; message about wrong jail configuration logged in client log (stdout, systemd journal etc.) and in server log as error --- fail2ban/client/jailreader.py | 134 ++++++++++++++++++--------------- fail2ban/client/jailsreader.py | 13 +++- fail2ban/server/transmitter.py | 3 + 3 files changed, 86 insertions(+), 64 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 9d01a693..df6c6664 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -118,70 +118,78 @@ class JailReader(ConfigReader): defsec = self._cfg.get_defaults() defsec["fail2ban_version"] = version - # Read first options only needed for merge defaults ('known/...' from filter): - self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) - if not self.__opts: - return False - - if self.isEnabled(): - # Read filter - if self.__opts["filter"]: - filterName, filterOpt = JailReader.extractOptions( - self.__opts["filter"]) - self.__filter = FilterReader( - filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) - ret = self.__filter.read() - # merge options from filter as 'known/...': - self.__filter.getOptions(self.__opts) - ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') - if not ret: - logSys.error("Unable to read the filter") - return False - else: - self.__filter = None - logSys.warning("No filter set for jail %s" % self.__name) + try: - # Read second all options (so variables like %(known/param) can be interpolated): - self.__opts = ConfigReader.getOptions(self, self.__name, opts) + # Read first options only needed for merge defaults ('known/...' from filter): + self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) if not self.__opts: - return False - - # cumulate filter options again (ignore given in jail): - if self.__filter: - self.__filter.getOptions(self.__opts) - - # Read action - for act in self.__opts["action"].split('\n'): - try: - if not act: # skip empty actions - continue - actName, actOpt = JailReader.extractOptions(act) - if actName.endswith(".py"): - self.__actions.append([ - "set", - self.__name, - "addaction", - actOpt.pop("actname", os.path.splitext(actName)[0]), - os.path.join( - self.getBaseDir(), "action.d", actName), - json.dumps(actOpt), - ]) - else: - action = ActionReader( - actName, self.__name, actOpt, - share_config=self.share_config, basedir=self.getBaseDir()) - ret = action.read() - if ret: - action.getOptions(self.__opts) - self.__actions.append(action) + raise ValueError("Init jail options failed") + + if self.isEnabled(): + # Read filter + if self.__opts["filter"]: + filterName, filterOpt = JailReader.extractOptions( + self.__opts["filter"]) + self.__filter = FilterReader( + filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) + ret = self.__filter.read() + # merge options from filter as 'known/...': + self.__filter.getOptions(self.__opts) + ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') + if not ret: + raise ValueError("Unable to read the filter %r" % filterName) + else: + self.__filter = None + logSys.warning("No filter set for jail %s" % self.__name) + + # Read second all options (so variables like %(known/param) can be interpolated): + self.__opts = ConfigReader.getOptions(self, self.__name, opts) + if not self.__opts: + raise ValueError("Read jail options failed") + + # cumulate filter options again (ignore given in jail): + if self.__filter: + self.__filter.getOptions(self.__opts) + + # Read action + for act in self.__opts["action"].split('\n'): + try: + if not act: # skip empty actions + continue + actName, actOpt = JailReader.extractOptions(act) + if actName.endswith(".py"): + self.__actions.append([ + "set", + self.__name, + "addaction", + actOpt.pop("actname", os.path.splitext(actName)[0]), + os.path.join( + self.getBaseDir(), "action.d", actName), + json.dumps(actOpt), + ]) else: - raise AttributeError("Unable to read action") - except Exception as e: - logSys.error("Error in action definition " + act) - logSys.debug("Caught exception: %s" % (e,)) - return False - if not len(self.__actions): - logSys.warning("No actions were defined for %s" % self.__name) + action = ActionReader( + actName, self.__name, actOpt, + share_config=self.share_config, basedir=self.getBaseDir()) + ret = action.read() + if ret: + action.getOptions(self.__opts) + self.__actions.append(action) + else: + raise AttributeError("Unable to read action") + except Exception as e: + logSys.debug("Caught exception: %s" % (e,)) + raise ValueError("Error in action definition %r" % e) + if not len(self.__actions): + logSys.warning("No actions were defined for %s" % self.__name) + + except ValueError as e: + e = str(e) + logSys.error(e) + if not self.__opts: + self.__opts = dict() + self.__opts['config-error'] = e + return False return True def convert(self, allow_no_files=False): @@ -195,6 +203,10 @@ class JailReader(ConfigReader): """ stream = [] + e = self.__opts.get('config-error') + if e: + stream.extend([['config-error', "Jail '%s' skipped, because of wrong configuration: %s" % (self.__name, e)]]) + return stream for opt, value in self.__opts.iteritems(): if opt == "logpath" and \ not self.__opts.get('backend', None).startswith("systemd"): diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 09725ec9..ec7baca7 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -66,7 +66,7 @@ class JailsReader(ConfigReader): sections = [ section ] # Get the options of all jails. - parse_status = True + parse_status = None for sec in sections: if sec == 'INCLUDES': continue @@ -77,11 +77,17 @@ class JailsReader(ConfigReader): ret = jail.getOptions() if ret: if jail.isEnabled(): + # at least one jail was successful: + parse_status = True # We only add enabled jails self.__jails.append(jail) else: logSys.error("Errors in jail %r. Skipping..." % sec) - parse_status = False + self.__jails.append(jail) + if parse_status is None: + parse_status = False + if parse_status is None: + parse_status = True return parse_status def convert(self, allow_no_files=False): @@ -103,7 +109,8 @@ class JailsReader(ConfigReader): stream.extend(jail.convert(allow_no_files=allow_no_files)) # Start jails for jail in self.__jails: - stream.append(["start", jail.getName()]) + if not jail.options.get('config-error'): + stream.append(["start", jail.getName()]) return stream diff --git a/fail2ban/server/transmitter.py b/fail2ban/server/transmitter.py index 2f5be043..ae1017b9 100644 --- a/fail2ban/server/transmitter.py +++ b/fail2ban/server/transmitter.py @@ -131,6 +131,9 @@ class Transmitter: return self.status(command[1:]) elif command[0] == "version": return version.version + elif command[0] == "config-error": + logSys.error(command[1]) + return None raise Exception("Invalid command") def __commandSet(self, command, multiple=False): From c6e8c700f7f712d9b5c93c125bdcefbef670c7ad Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:57:06 +0100 Subject: [PATCH 12/26] test cases fixed --- fail2ban/client/jailreader.py | 14 +++++++++----- fail2ban/tests/clientreadertestcase.py | 17 +++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index df6c6664..74072481 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -127,9 +127,11 @@ class JailReader(ConfigReader): if self.isEnabled(): # Read filter - if self.__opts["filter"]: - filterName, filterOpt = JailReader.extractOptions( - self.__opts["filter"]) + flt = self.__opts["filter"] + if flt: + filterName, filterOpt = JailReader.extractOptions(flt) + if not filterName: + raise ValueError("Invalid filter declaration %r" % flt) self.__filter = FilterReader( filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() @@ -157,6 +159,8 @@ class JailReader(ConfigReader): if not act: # skip empty actions continue actName, actOpt = JailReader.extractOptions(act) + if not actName: + raise ValueError("Invalid action declaration %r" % act) if actName.endswith(".py"): self.__actions.append([ "set", @@ -178,8 +182,8 @@ class JailReader(ConfigReader): else: raise AttributeError("Unable to read action") except Exception as e: - logSys.debug("Caught exception: %s" % (e,)) - raise ValueError("Error in action definition %r" % e) + logSys.debug("Caught exception: %s", e, exc_info=True) + raise ValueError("Error in action definition %r: %r" % (act, e)) if not len(self.__actions): logSys.warning("No actions were defined for %s" % self.__name) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index e68523c2..20d6ced1 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -193,13 +193,9 @@ class JailReaderTest(LogCaptureTestCase): self.assertTrue(jail.read()) self.assertFalse(jail.getOptions()) self.assertTrue(jail.isEnabled()) - self.assertLogged('Error in action definition joho[foo') - # This unittest has been deactivated for some time... - # self.assertLogged( - # 'Caught exception: While reading action joho[foo we should have got 1 or 2 groups. Got: 0') - # let's test for what is actually logged and handle changes in the future + self.assertLogged("Error in action definition 'joho[foo'") self.assertLogged( - "Caught exception: 'NoneType' object has no attribute 'endswith'") + "Caught exception: Invalid action declaration 'joho[foo'") if STOCK: def testStockSSHJail(self): @@ -496,7 +492,7 @@ class JailsReaderTest(LogCaptureTestCase): def testReadTestJailConf(self): jails = JailsReader(basedir=IMPERFECT_CONFIG, share_config=IMPERFECT_CONFIG_SHARE_CFG) self.assertTrue(jails.read()) - self.assertFalse(jails.getOptions()) + self.assertTrue(jails.getOptions()) self.assertRaises(ValueError, jails.convert) comm_commands = jails.convert(allow_no_files=True) self.maxDiff = None @@ -525,7 +521,12 @@ class JailsReaderTest(LogCaptureTestCase): ['start', 'emptyaction'], ['start', 'missinglogfiles'], ['start', 'brokenaction'], - ['start', 'parse_to_end_of_jail.conf'],])) + ['start', 'parse_to_end_of_jail.conf'], + ['config-error', + 'Jail \'brokenactiondef\' skipped, because of wrong configuration: Error in action definition \'joho[foo\': ValueError("Invalid action declaration \'joho[foo\'",)'], + ['config-error', + "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], + ])) self.assertLogged("Errors in jail 'missingbitsjail'. Skipping...") self.assertLogged("No file(s) found for glob /weapons/of/mass/destruction") From e52b47d8f52b9dd1f5704b62f21b3bf9c6c604b5 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:57:20 +0100 Subject: [PATCH 13/26] normalized log output (all jail parameters in filter are indented with 2 spaces) --- fail2ban/server/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index dcfe09f6..dd4c97c2 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -193,7 +193,7 @@ class Actions(JailThread, Mapping): def setBanTime(self, value): value = MyTime.str2seconds(value) self.__banManager.setBanTime(value) - logSys.info("Set banTime = %s" % value) + logSys.info(" banTime: %s" % value) ## # Get the ban time. From 4882093a41c031d79e3aeace447d762009f6528c Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 14:08:35 +0100 Subject: [PATCH 14/26] test cases extended: cover skipping invalid jail --- fail2ban/tests/fail2banclienttestcase.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fail2ban/tests/fail2banclienttestcase.py b/fail2ban/tests/fail2banclienttestcase.py index e212ff23..cec48cc3 100644 --- a/fail2ban/tests/fail2banclienttestcase.py +++ b/fail2ban/tests/fail2banclienttestcase.py @@ -768,6 +768,10 @@ class Fail2banServerTest(Fail2banClientServerBase): _write_action_cfg(actname="test-action2") _write_jail_cfg(enabled=[1], actions=[1,2]) + # append one wrong configured jail: + _write_file(pjoin(cfg, "jail.conf"), "a", "", "[broken-jail]", + "", "filter = broken-jail-filter", "enabled = true") + _write_file(test1log, "w", *((str(int(MyTime.time())) + " failure 401 from 192.0.2.1: test 1",) * 3)) _write_file(test2log, "w") _write_file(test3log, "w") @@ -786,6 +790,12 @@ class Fail2banServerTest(Fail2banClientServerBase): self.assertLogged( "stdout: '[test-jail1] test-action1: ** start'", "stdout: '[test-jail1] test-action2: ** start'", all=True) + + # broken jail was logged (in client and server log): + self.assertLogged( + "Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'. Skipping...", + "Jail 'broken-jail' skipped, because of wrong configuration", all=True) # enable both jails, 3 logs for jail1, etc... # truncate test-log - we should not find unban/ban again by reload: From 3e9852d4d2a7c73936a4cbaafd17bf13a0e412a6 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 14:56:54 +0100 Subject: [PATCH 15/26] code review, increase coverage --- fail2ban/client/jailreader.py | 24 +++++++++++++++--------- fail2ban/client/jailsreader.py | 10 ++-------- fail2ban/tests/clientreadertestcase.py | 18 ++++++++++++++---- fail2ban/tests/config/jail.conf | 8 ++++++++ 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 74072481..b63df5f1 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -122,8 +122,8 @@ class JailReader(ConfigReader): # Read first options only needed for merge defaults ('known/...' from filter): self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) - if not self.__opts: - raise ValueError("Init jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Init jail options failed") if self.isEnabled(): # Read filter @@ -131,7 +131,7 @@ class JailReader(ConfigReader): if flt: filterName, filterOpt = JailReader.extractOptions(flt) if not filterName: - raise ValueError("Invalid filter declaration %r" % flt) + raise JailDefError("Invalid filter definition %r" % flt) self.__filter = FilterReader( filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() @@ -139,15 +139,15 @@ class JailReader(ConfigReader): self.__filter.getOptions(self.__opts) ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') if not ret: - raise ValueError("Unable to read the filter %r" % filterName) + raise JailDefError("Unable to read the filter %r" % filterName) else: self.__filter = None logSys.warning("No filter set for jail %s" % self.__name) # Read second all options (so variables like %(known/param) can be interpolated): self.__opts = ConfigReader.getOptions(self, self.__name, opts) - if not self.__opts: - raise ValueError("Read jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Read jail options failed") # cumulate filter options again (ignore given in jail): if self.__filter: @@ -160,7 +160,7 @@ class JailReader(ConfigReader): continue actName, actOpt = JailReader.extractOptions(act) if not actName: - raise ValueError("Invalid action declaration %r" % act) + raise JailDefError("Invalid action definition %r" % act) if actName.endswith(".py"): self.__actions.append([ "set", @@ -180,14 +180,16 @@ class JailReader(ConfigReader): action.getOptions(self.__opts) self.__actions.append(action) else: - raise AttributeError("Unable to read action") + raise JailDefError("Unable to read action %r" % actName) + except JailDefError: + raise except Exception as e: logSys.debug("Caught exception: %s", e, exc_info=True) raise ValueError("Error in action definition %r: %r" % (act, e)) if not len(self.__actions): logSys.warning("No actions were defined for %s" % self.__name) - except ValueError as e: + except JailDefError as e: e = str(e) logSys.error(e) if not self.__opts: @@ -280,3 +282,7 @@ class JailReader(ConfigReader): val for val in optmatch.group(2,3,4) if val is not None][0] option_opts[opt.strip()] = value.strip() return option_name, option_opts + + +class JailDefError(Exception): + pass diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index ec7baca7..7d81d0a0 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -84,11 +84,8 @@ class JailsReader(ConfigReader): else: logSys.error("Errors in jail %r. Skipping..." % sec) self.__jails.append(jail) - if parse_status is None: - parse_status = False - if parse_status is None: - parse_status = True - return parse_status + if parse_status is None: parse_status = False + return True if parse_status != False else False def convert(self, allow_no_files=False): """Convert read before __opts and jails to the commands stream @@ -101,9 +98,6 @@ class JailsReader(ConfigReader): """ stream = list() - for opt in self.__opts: - if opt == "": - stream.append([]) # Convert jails for jail in self.__jails: stream.extend(jail.convert(allow_no_files=allow_no_files)) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 20d6ced1..33c217cd 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -193,9 +193,15 @@ class JailReaderTest(LogCaptureTestCase): self.assertTrue(jail.read()) self.assertFalse(jail.getOptions()) self.assertTrue(jail.isEnabled()) - self.assertLogged("Error in action definition 'joho[foo'") - self.assertLogged( - "Caught exception: Invalid action declaration 'joho[foo'") + self.assertLogged("Invalid action definition 'joho[foo'") + + def testJailFilterBrokenDef(self): + jail = JailReader('brokenfilterdef', basedir=IMPERFECT_CONFIG, + share_config=IMPERFECT_CONFIG_SHARE_CFG) + self.assertTrue(jail.read()) + self.assertFalse(jail.getOptions()) + self.assertTrue(jail.isEnabled()) + self.assertLogged("Invalid filter definition 'flt[test'") if STOCK: def testStockSSHJail(self): @@ -523,7 +529,11 @@ class JailsReaderTest(LogCaptureTestCase): ['start', 'brokenaction'], ['start', 'parse_to_end_of_jail.conf'], ['config-error', - 'Jail \'brokenactiondef\' skipped, because of wrong configuration: Error in action definition \'joho[foo\': ValueError("Invalid action declaration \'joho[foo\'",)'], + "Jail 'brokenactiondef' skipped, because of wrong configuration: Invalid action definition 'joho[foo'"], + ['config-error', + "Jail 'brokenfilterdef' skipped, because of wrong configuration: Invalid filter definition 'flt[test'"], + ['config-error', + "Jail 'missingaction' skipped, because of wrong configuration: Unable to read action 'noactionfileforthisaction'"], ['config-error', "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], ])) diff --git a/fail2ban/tests/config/jail.conf b/fail2ban/tests/config/jail.conf index bf1dea45..659e3fd3 100644 --- a/fail2ban/tests/config/jail.conf +++ b/fail2ban/tests/config/jail.conf @@ -27,10 +27,18 @@ logpath = /weapons/of/mass/destruction enabled = true action = joho[foo +[brokenfilterdef] +enabled = true +filter = flt[test + [brokenaction] enabled = true action = brokenaction +[missingaction] +enabled = true +action = noactionfileforthisaction + [missingbitsjail] enabled = true filter = catchallthebadies From fdac44ca589ff5d9de8afde3865022dc7f702858 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:08:44 +0100 Subject: [PATCH 16/26] introduced new option `-t` or `--test` to test configuration resp. start server only if configuration is clean (not skip wrong configured jails if option `-t` specified); --- fail2ban/client/configurator.py | 4 +-- fail2ban/client/fail2bancmdline.py | 33 ++++++++++++++++---- fail2ban/client/fail2banserver.py | 38 ++++++++++++------------ fail2ban/client/jailsreader.py | 13 ++++---- fail2ban/tests/fail2banclienttestcase.py | 30 +++++++++++++++++++ 5 files changed, 86 insertions(+), 32 deletions(-) diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index 4d28238f..e8472ac1 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -72,9 +72,9 @@ class Configurator: def getEarlyOptions(self): return self.__fail2ban.getEarlyOptions() - def getOptions(self, jail=None, updateMainOpt=None): + def getOptions(self, jail=None, updateMainOpt=None, ignoreWrong=True): self.__fail2ban.getOptions(updateMainOpt) - return self.__jails.getOptions(jail) + return self.__jails.getOptions(jail, ignoreWrong=ignoreWrong) def convertToProtocol(self): self.__streams["general"] = self.__fail2ban.convert() diff --git a/fail2ban/client/fail2bancmdline.py b/fail2ban/client/fail2bancmdline.py index 74236ab1..7d0eeead 100644 --- a/fail2ban/client/fail2bancmdline.py +++ b/fail2ban/client/fail2bancmdline.py @@ -47,6 +47,7 @@ class Fail2banCmdLine(): def __init__(self): self._argv = self._args = None self._configurator = None + self.cleanConfOnly = False self.resetConf() def resetConf(self): @@ -101,6 +102,7 @@ class Fail2banCmdLine(): output(" --logtarget |STDOUT|STDERR|SYSLOG") output(" --syslogsocket auto|") output(" -d dump configuration. For debugging") + output(" -t, --test test configuration (can be also specified with start parameters)") output(" -i interactive mode") output(" -v increase verbosity") output(" -q decrease verbosity") @@ -136,6 +138,9 @@ class Fail2banCmdLine(): self._conf[ o[2:] ] = opt[1] elif o == "-d": self._conf["dump"] = True + elif o == "-t" or o == "--test": + self.cleanConfOnly = True + self._conf["test"] = True elif o == "-v": self._conf["verbose"] += 1 elif o == "-q": @@ -173,8 +178,8 @@ class Fail2banCmdLine(): # Reads the command line options. try: - cmdOpts = 'hc:s:p:xfbdviqV' - cmdLongOpts = ['loglevel=', 'logtarget=', 'syslogsocket=', 'async', 'timeout=', 'help', 'version'] + cmdOpts = 'hc:s:p:xfbdtviqV' + cmdLongOpts = ['loglevel=', 'logtarget=', 'syslogsocket=', 'test', 'async', 'timeout=', 'help', 'version'] optList, self._args = getopt.getopt(self._argv[1:], cmdOpts, cmdLongOpts) except getopt.GetoptError: self.dispUsage() @@ -225,13 +230,30 @@ class Fail2banCmdLine(): logSys.info("Using pid file %s, [%s] logging to %s", self._conf["pidfile"], logging.getLevelName(llev), self._conf["logtarget"]) + readcfg = True if self._conf.get("dump", False): - ret, stream = self.readConfig() + if readcfg: + ret, stream = self.readConfig() + readcfg = False self.dumpConfig(stream) - return ret + if not self._conf.get("test", False): + return ret + + if self._conf.get("test", False): + if readcfg: + readcfg = False + ret, stream = self.readConfig() + if not ret: + raise ServerExecutionException("ERROR: test configuration failed") + # exit after test if no commands specified (test only): + if not len(self._args): + output("OK: configuration test is successful") + return ret # Nothing to do here, process in client/server return None + except ServerExecutionException: + raise except Exception as e: output("ERROR: %s" % (e,)) if verbose > 2: @@ -246,7 +268,8 @@ class Fail2banCmdLine(): try: self.configurator.Reload() self.configurator.readAll() - ret = self.configurator.getOptions(jail, self._conf) + ret = self.configurator.getOptions(jail, self._conf, + ignoreWrong=not self.cleanConfOnly) self.configurator.convertToProtocol() stream = self.configurator.getConfigStream() except Exception as e: diff --git a/fail2ban/client/fail2banserver.py b/fail2ban/client/fail2banserver.py index dfee34d2..006a02cf 100644 --- a/fail2ban/client/fail2banserver.py +++ b/fail2ban/client/fail2banserver.py @@ -144,27 +144,27 @@ class Fail2banServer(Fail2banCmdLine): return cli def start(self, argv): - # Command line options - ret = self.initCmdLine(argv) - if ret is not None: - return ret - - # Commands - args = self._args - - cli = None - # Just start: - if len(args) == 1 and args[0] == 'start' and not self._conf.get("interactive", False): - pass - else: - # If client mode - whole processing over client: - if len(args) or self._conf.get("interactive", False): - cli = self._Fail2banClient() - return cli.start(argv) - - # Start the server: server = None try: + # Command line options + ret = self.initCmdLine(argv) + if ret is not None: + return ret + + # Commands + args = self._args + + cli = None + # Just start: + if len(args) == 1 and args[0] == 'start' and not self._conf.get("interactive", False): + pass + else: + # If client mode - whole processing over client: + if len(args) or self._conf.get("interactive", False): + cli = self._Fail2banClient() + return cli.start(argv) + + # Start the server: from ..server.utils import Utils # background = True, if should be new process running in background, otherwise start in foreground # process will be forked in daemonize, inside of Server module. diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 7d81d0a0..cd3409b4 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -54,7 +54,7 @@ class JailsReader(ConfigReader): self.__jails = list() return ConfigReader.read(self, "jail") - def getOptions(self, section=None): + def getOptions(self, section=None, ignoreWrong=True): """Reads configuration for jail(s) and adds enabled jails to __jails """ opts = [] @@ -66,7 +66,7 @@ class JailsReader(ConfigReader): sections = [ section ] # Get the options of all jails. - parse_status = None + parse_status = 0 for sec in sections: if sec == 'INCLUDES': continue @@ -78,14 +78,15 @@ class JailsReader(ConfigReader): if ret: if jail.isEnabled(): # at least one jail was successful: - parse_status = True + parse_status |= 1 # We only add enabled jails self.__jails.append(jail) else: - logSys.error("Errors in jail %r. Skipping..." % sec) + logSys.error("Errors in jail %r.%s", sec, " Skipping..." if ignoreWrong else "") self.__jails.append(jail) - if parse_status is None: parse_status = False - return True if parse_status != False else False + # at least one jail was invalid: + parse_status |= 2 + return ((ignoreWrong and parse_status & 1) or not (parse_status & 2)) def convert(self, allow_no_files=False): """Convert read before __opts and jails to the commands stream diff --git a/fail2ban/tests/fail2banclienttestcase.py b/fail2ban/tests/fail2banclienttestcase.py index cec48cc3..e68d9779 100644 --- a/fail2ban/tests/fail2banclienttestcase.py +++ b/fail2ban/tests/fail2banclienttestcase.py @@ -675,6 +675,36 @@ class Fail2banServerTest(Fail2banClientServerBase): self.pruneLog() os.remove(pjoin(tmp, "f2b.sock")) + @with_tmpdir + @with_kill_srv + def testServerTestFailStart(self, tmp): + # started directly here, so prevent overwrite test cases logger with "INHERITED" + startparams = _start_params(tmp, logtarget="INHERITED") + cfg = pjoin(tmp, "config") + + # test configuration is correct: + self.pruneLog("[test-phase 0]") + self.execSuccess(startparams, "--test") + self.assertLogged("OK: configuration test is successful") + + # append one wrong configured jail: + _write_file(pjoin(cfg, "jail.conf"), "a", "", "[broken-jail]", + "", "filter = broken-jail-filter", "enabled = true") + + # first try test config: + self.pruneLog("[test-phase 0a]") + self.execFailed(startparams, "--test") + self.assertLogged("Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'.", + "ERROR: test configuration failed", all=True) + + # failed to start with test config: + self.pruneLog("[test-phase 0b]") + self.execFailed(startparams, "-t", "start") + self.assertLogged("Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'.", + "ERROR: test configuration failed", all=True) + @with_tmpdir def testKillAfterStart(self, tmp): try: From 8ed5b44bfd2256f8ffe8ad867b7cd9fc7cc0e467 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:38:32 +0100 Subject: [PATCH 17/26] no cover for sporadic executed (time-related) code pieces (just to prevent randomly increasing/decreasing of coverage) --- fail2ban/client/fail2banclient.py | 2 +- fail2ban/server/asyncserver.py | 2 +- fail2ban/server/utils.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fail2ban/client/fail2banclient.py b/fail2ban/client/fail2banclient.py index 007afd57..0a1ae4f1 100755 --- a/fail2ban/client/fail2banclient.py +++ b/fail2ban/client/fail2banclient.py @@ -125,7 +125,7 @@ class Fail2banClient(Fail2banCmdLine, Thread): if client: try : client.close() - except Exception as e: + except Exception as e: # pragma: no cover if showRet or self._conf["verbose"] > 1: logSys.debug(e) if showRet or c[0] == 'echo': diff --git a/fail2ban/server/asyncserver.py b/fail2ban/server/asyncserver.py index d1818d7a..9cc74658 100644 --- a/fail2ban/server/asyncserver.py +++ b/fail2ban/server/asyncserver.py @@ -241,7 +241,7 @@ class AsyncServer(asyncore.dispatcher): def _remove_sock(self): try: os.remove(self.__sock) - except OSError as e: + except OSError as e: # pragma: no cover if e.errno != errno.ENOENT: raise diff --git a/fail2ban/server/utils.py b/fail2ban/server/utils.py index 6d74d2db..57da495a 100644 --- a/fail2ban/server/utils.py +++ b/fail2ban/server/utils.py @@ -170,7 +170,7 @@ class Utils(): time.sleep(Utils.DEFAULT_SLEEP_INTERVAL) retcode = popen.poll() #logSys.debug("%s -- killed %s ", realCmd, retcode) - if retcode is None and not Utils.pid_exists(pgid): + if retcode is None and not Utils.pid_exists(pgid): # pragma: no cover retcode = signal.SIGKILL except OSError as e: stderr = "%s -- failed with %s" % (realCmd, e) From 7256a5cb8e0c37bca69fd684bd3e568a4cc46540 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:55:27 +0100 Subject: [PATCH 18/26] code review: back to previous code - no skipping in testReadTestJailConf --- fail2ban/tests/clientreadertestcase.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 33c217cd..5129df61 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -498,7 +498,7 @@ class JailsReaderTest(LogCaptureTestCase): def testReadTestJailConf(self): jails = JailsReader(basedir=IMPERFECT_CONFIG, share_config=IMPERFECT_CONFIG_SHARE_CFG) self.assertTrue(jails.read()) - self.assertTrue(jails.getOptions()) + self.assertFalse(jails.getOptions(ignoreWrong=False)) self.assertRaises(ValueError, jails.convert) comm_commands = jails.convert(allow_no_files=True) self.maxDiff = None @@ -537,7 +537,8 @@ class JailsReaderTest(LogCaptureTestCase): ['config-error', "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], ])) - self.assertLogged("Errors in jail 'missingbitsjail'. Skipping...") + self.assertLogged("Errors in jail 'missingbitsjail'.") + self.assertNotLogged("Skipping...") self.assertLogged("No file(s) found for glob /weapons/of/mass/destruction") if STOCK: From 1cd67ecaa260bdaa6e62383ed783d5910587c258 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 23 Nov 2016 22:03:12 +0100 Subject: [PATCH 19/26] automatically creates /var/run/fail2ban before start fail2ban (systems which /var/run/ is virtual resp. memory mount device) --- files/fail2ban.service | 1 + 1 file changed, 1 insertion(+) diff --git a/files/fail2ban.service b/files/fail2ban.service index 5b661100..1ef7b5e6 100644 --- a/files/fail2ban.service +++ b/files/fail2ban.service @@ -6,6 +6,7 @@ PartOf=iptables.service firewalld.service [Service] Type=simple +ExecStartPre=/bin/mkdir -p /var/run/fail2ban ExecStart=/usr/bin/fail2ban-server -xf start ExecStop=/usr/bin/fail2ban-client stop ExecReload=/usr/bin/fail2ban-client reload From 45174c5eaf1479a2b5fec594dce421364b5cfd36 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 24 Nov 2016 12:13:47 +0100 Subject: [PATCH 20/26] if fail2ban running as systemd-service, for logging to the systemd-journal, the `logtarget` could be set to STDOUT small fixes by logging in stdout (+ system targets also allowed in lowercase now) --- fail2ban/client/fail2bancmdline.py | 1 + fail2ban/server/server.py | 10 ++++++---- files/fail2ban.service | 2 ++ man/fail2ban-server.1 | 3 +++ man/jail.conf.5 | 4 +++- 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/fail2ban/client/fail2bancmdline.py b/fail2ban/client/fail2bancmdline.py index 74236ab1..c7c40120 100644 --- a/fail2ban/client/fail2bancmdline.py +++ b/fail2ban/client/fail2bancmdline.py @@ -274,6 +274,7 @@ class Fail2banCmdLine(): def exit(code=0): logSys.debug("Exit with code %s", code) # because of possible buffered output in python, we should flush it before exit: + logging.shutdown() sys.stdout.flush() sys.stderr.flush() # exit diff --git a/fail2ban/server/server.py b/fail2ban/server/server.py index efd47b09..313b6ee5 100644 --- a/fail2ban/server/server.py +++ b/fail2ban/server/server.py @@ -530,17 +530,19 @@ class Server: # @param target the logging target def setLogTarget(self, target): + # check reserved targets in uppercase, don't change target, because it can be file: + systarget = target.upper() with self.__loggingLock: # don't set new handlers if already the same # or if "INHERITED" (foreground worker of the test cases, to prevent stop logging): if self.__logTarget == target: return True - if target == "INHERITED": + if systarget == "INHERITED": self.__logTarget = target return True # set a format which is simpler for console use fmt = "%(asctime)s %(name)-24s[%(process)d]: %(levelname)-7s %(message)s" - if target == "SYSLOG": + if systarget == "SYSLOG": # Syslog daemons already add date to the message. fmt = "%(name)s[%(process)d]: %(levelname)s %(message)s" facility = logging.handlers.SysLogHandler.LOG_DAEMON @@ -559,9 +561,9 @@ class Server: "Syslog socket file: %s does not exists" " or is not a socket" % self.__syslogSocket) return False - elif target == "STDOUT": + elif systarget == "STDOUT": hdlr = logging.StreamHandler(sys.stdout) - elif target == "STDERR": + elif systarget == "STDERR": hdlr = logging.StreamHandler(sys.stderr) else: # Target should be a file diff --git a/files/fail2ban.service b/files/fail2ban.service index 1ef7b5e6..6eeba957 100644 --- a/files/fail2ban.service +++ b/files/fail2ban.service @@ -8,6 +8,8 @@ PartOf=iptables.service firewalld.service Type=simple ExecStartPre=/bin/mkdir -p /var/run/fail2ban ExecStart=/usr/bin/fail2ban-server -xf start +# if should be logged in systemd journal, use following line or set logtarget to stdout in fail2ban.local +# ExecStart=/usr/bin/fail2ban-server -xf --logtarget=stdout start ExecStop=/usr/bin/fail2ban-client stop ExecReload=/usr/bin/fail2ban-client reload PIDFile=/var/run/fail2ban/fail2ban.pid diff --git a/man/fail2ban-server.1 b/man/fail2ban-server.1 index de8ba6a4..09dcd65a 100644 --- a/man/fail2ban-server.1 +++ b/man/fail2ban-server.1 @@ -23,6 +23,9 @@ pidfile path logging level .HP \fB\-\-logtarget\fR |STDOUT|STDERR|SYSLOG +logging target +.br +Note. If fail2ban running as systemd-service, for logging to the systemd-journal, the logtarget could be set to STDOUT .HP \fB\-\-syslogsocket\fR auto| .TP diff --git a/man/jail.conf.5 b/man/jail.conf.5 index 51a00cdc..2e333e5a 100644 --- a/man/jail.conf.5 +++ b/man/jail.conf.5 @@ -130,7 +130,9 @@ The items that can be set are: verbosity level of log output: CRITICAL, ERROR, WARNING, NOTICE, INFO, DEBUG, TRACEDEBUG, HEAVYDEBUG or corresponding numeric value (50-5). Default: ERROR (equal 40) .TP .B logtarget -log target: filename, SYSLOG, STDERR or STDOUT. Default: STDERR +log target: filename, SYSLOG, STDERR or STDOUT. Default: STDOUT if not set in fail2ban.conf/fail2ban.local +.br +Note. If fail2ban running as systemd-service, for logging to the systemd-journal, the logtarget could be set to STDOUT .br Only a single log target can be specified. If you change logtarget from the default value and you are using logrotate -- also adjust or disable rotation in the From d908688b565fbc55fd63c1cf4ac305379afdfdca Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 24 Nov 2016 20:25:08 +0100 Subject: [PATCH 21/26] ChangeLog update --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 05a2e38f..ef2ce050 100644 --- a/ChangeLog +++ b/ChangeLog @@ -55,6 +55,8 @@ TODO: implementing of options resp. other tasks from PR #1346 banned in this jail, if option `--unban` specified - `unban --all` - unbans all IP addresses (in all jails and database) - `unban ... ` - unbans \ (in all jails and database) (see gh-1388) + - introduced new option `-t` or `--test` to test configuration resp. start server only + if configuration is clean (fails by wrong configured jails if option `-t` specified) * New command action parameter `actionrepair` - command executed in order to restore sane environment in error case of `actioncheck`. @@ -119,6 +121,10 @@ fail2ban-client set loglevel INFO - new replacement for `` in opposition to ``, for separate usage of 2 address groups only (regardless of `usedns`), `ip4` and `ip6` together, without host (dns) +* Misconfigured jails don't prevent fail2ban from starting, server starts + nevertheless, as long as one jail was successful configured (gh-1619) + Message about wrong jail configuration logged in client log (stdout, systemd + journal etc.) and in server log with error level * fail2ban-testcases: - `assertLogged` extended with parameter wait (to wait up to specified timeout, before we throw assert exception) + test cases rewritten using that From 308bba448ca7edfd46115b16d27ce18b2f2999c9 Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 24 Nov 2016 20:43:55 +0100 Subject: [PATCH 22/26] ChangeLog update --- ChangeLog | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ChangeLog b/ChangeLog index 05a2e38f..b07d2842 100644 --- a/ChangeLog +++ b/ChangeLog @@ -29,6 +29,16 @@ TODO: implementing of options resp. other tasks from PR #1346 environment (without lock) * Fixed sporadically error in testCymruInfoNxdomain, because of unsorted values * Misleading errors logged from ignorecommand in success case on retcode 1 (gh-1194) +* fail2ban.service - systemd service updated (gh-1618): + - starting service in normal mode (without forking) + - does not restart if service exited normally (exit-code 0, e.g. stopped via fail2ban-client) + - does not restart if service can not start (exit-code 255, e.g. wrong configuration, etc.) + - service can be additionally started/stopped with commands (fail2ban-client, fail2ban-server) + - automatically creates `/var/run/fail2ban` directory before start fail2ban + (systems with virtual resp. memory-based FS for `/var/run`), see gh-1531 + - if fail2ban running as systemd-service, for logging to the systemd-journal, + the `logtarget` could be set to STDOUT + - value `logtarget` for system targets allowed also in lowercase (stdout, stderr, syslog, etc.) ### New Features * IPv6 support: From 097970781cbe61160824c834ca5cb4a065c2a8f8 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 25 Nov 2016 16:53:46 +0100 Subject: [PATCH 23/26] filter/action (and its includes): substitution `%(param)s` may be used now (instead of ``) for init-values specified in jail-configs via `action[param1="...", param2=...]`; substitution `` should be used for dynamic interpolation only (todo: review configurations to replace it); --- fail2ban/client/configparserinc.py | 2 +- fail2ban/client/configreader.py | 32 ++++++++++++++++++++++++------ fail2ban/client/filterreader.py | 6 ++++-- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/fail2ban/client/configparserinc.py b/fail2ban/client/configparserinc.py index d6cda7f7..35fa7498 100644 --- a/fail2ban/client/configparserinc.py +++ b/fail2ban/client/configparserinc.py @@ -29,7 +29,7 @@ import re import sys from ..helpers import getLogger -if sys.version_info >= (3,2): # pragma: no cover +if sys.version_info >= (3,2): # SafeConfigParser deprecated from Python 3.2 (renamed to ConfigParser) from configparser import ConfigParser as SafeConfigParser, \ diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index 643cdf3a..caab67aa 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -28,13 +28,25 @@ import glob import os from ConfigParser import NoOptionError, NoSectionError -from .configparserinc import SafeConfigParserWithIncludes, logLevel +from .configparserinc import sys, SafeConfigParserWithIncludes, logLevel from ..helpers import getLogger # Gets the instance of the logger. logSys = getLogger(__name__) +# if sys.version_info >= (3,5): +# def _merge_dicts(x, y): +# return {**x, **y} +# else: +def _merge_dicts(x, y): + r = x + if y: + r = x.copy() + r.update(y) + return r + + class ConfigReader(): """Generic config reader class. @@ -127,9 +139,9 @@ class ConfigReader(): return self._cfg.options(*args) return {} - def get(self, sec, opt): + def get(self, sec, opt, raw=False, vars={}): if self._cfg is not None: - return self._cfg.get(sec, opt) + return self._cfg.get(sec, opt, raw, vars) return None def getOptions(self, *args, **kwargs): @@ -210,6 +222,8 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): def getOptions(self, sec, options, pOptions=None, shouldExist=False): values = dict() + if pOptions is None: + pOptions = {} for optname in options: if isinstance(options, (list,tuple)): if len(optname) > 2: @@ -218,15 +232,15 @@ class ConfigReaderUnshared(SafeConfigParserWithIncludes): (opttype, optname), optvalue = optname, None else: opttype, optvalue = options[optname] + if optname in pOptions: + continue try: if opttype == "bool": v = self.getboolean(sec, optname) elif opttype == "int": v = self.getint(sec, optname) else: - v = self.get(sec, optname) - if not pOptions is None and optname in pOptions: - continue + v = self.get(sec, optname, vars=pOptions) values[optname] = v except NoSectionError as e: if shouldExist: @@ -289,6 +303,12 @@ class DefinitionInitConfigReader(ConfigReader): return SafeConfigParserWithIncludes.read(self._cfg, self._file) def getOptions(self, pOpts): + # overwrite static definition options with init values, supplied as + # direct parameters from jail-config via action[xtra1="...", xtra2=...]: + if self._initOpts: + if not pOpts: + pOpts = dict() + pOpts = _merge_dicts(pOpts, self._initOpts) self._opts = ConfigReader.getOptions( self, "Definition", self._configOpts, pOpts) diff --git a/fail2ban/client/filterreader.py b/fail2ban/client/filterreader.py index 8b30f914..5e6b2b74 100644 --- a/fail2ban/client/filterreader.py +++ b/fail2ban/client/filterreader.py @@ -27,7 +27,7 @@ __license__ = "GPL" import os import shlex -from .configreader import DefinitionInitConfigReader +from .configreader import DefinitionInitConfigReader, _merge_dicts from ..server.action import CommandAction from ..helpers import getLogger @@ -50,7 +50,9 @@ class FilterReader(DefinitionInitConfigReader): return self.__file def getCombined(self): - combinedopts = dict(list(self._opts.items()) + list(self._initOpts.items())) + combinedopts = self._opts + if self._initOpts: + combinedopts = _merge_dicts(self._opts, self._initOpts) if not len(combinedopts): return {} opts = CommandAction.substituteRecursiveTags(combinedopts) From 65abc639cc7838beaf1f587eae74e98afc4a1473 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 25 Nov 2016 16:56:46 +0100 Subject: [PATCH 24/26] allow newline in extra init-parameters of action/filter (or interpolation of it), e. g. action[..., logpath="%(logpath)s"] --- fail2ban/client/jailreader.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index b63df5f1..d01064b2 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -43,13 +43,13 @@ logSys = getLogger(__name__) class JailReader(ConfigReader): # regex, to extract list of options: - optionCRE = re.compile("^((?:\w|-|_|\.)+)(?:\[(.*)\])?$") + optionCRE = re.compile(r"^([\w\-_\.]+)(?:\[(.*)\])?\s*$", re.DOTALL) # regex, to iterate over single option in option list, syntax: # `action = act[p1="...", p2='...', p3=...]`, where the p3=... not contains `,` or ']' # since v0.10 separator extended with `]\s*[` for support of multiple option groups, syntax # `action = act[p1=...][p2=...]` optionExtractRE = re.compile( - r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,\]]*))(?:,|\]\s*\[|$)') + r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,\]]*))(?:,|\]\s*\[|$)', re.DOTALL) def __init__(self, name, force_enable=False, **kwargs): ConfigReader.__init__(self, **kwargs) From a2af19c9f0c7ad42e733693f1cbf3d247cded582 Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 25 Nov 2016 19:06:34 +0100 Subject: [PATCH 25/26] fixed several actions, that could not work with jails using multiple logpath; additionally repaired execution in default shell (bad substitution by `${x//...}` executing in `/bin/sh`); added helper "action.d/helpers-common.conf", and `_grep_logs` part-command for actions needed grep logs from multiple log-files test cases: executing of some complex actions covered --- config/action.d/complain.conf | 20 ++++- config/action.d/helpers-common.conf | 13 +++ config/action.d/mail-whois-lines.conf | 29 ++++--- config/action.d/sendmail-geoip-lines.conf | 13 +-- config/action.d/sendmail-whois-lines.conf | 13 +-- fail2ban/tests/files/testcase01a.log | 4 + fail2ban/tests/servertestcase.py | 96 +++++++++++++++++++++-- 7 files changed, 160 insertions(+), 28 deletions(-) create mode 100644 config/action.d/helpers-common.conf create mode 100644 fail2ban/tests/files/testcase01a.log diff --git a/config/action.d/complain.conf b/config/action.d/complain.conf index 9247803e..e4ceb35f 100644 --- a/config/action.d/complain.conf +++ b/config/action.d/complain.conf @@ -28,6 +28,10 @@ # +[INCLUDES] + +before = helpers-common.conf + [Definition] # Option: actionstart @@ -54,10 +58,16 @@ actioncheck = # Tags: See jail.conf(5) man page # Values: CMD # -actionban = oifs=${IFS}; IFS=.;SEP_IP=( ); set -- ${SEP_IP}; ADDRESSES=$(dig +short -t txt -q $4.$3.$2.$1.abuse-contacts.abusix.org); IFS=${oifs} - IP= +actionban = oifs=${IFS}; + IFS=.; SEP_IP=( ); set -- ${SEP_IP}; ADDRESSES=$(dig +short -t txt -q $4.$3.$2.$1.abuse-contacts.abusix.org); + IFS=,; ADDRESSES=$(echo $ADDRESSES) + IFS=${oifs} + IP= if [ ! -z "$ADDRESSES" ]; then - (printf %%b "\n"; date '+Note: Local timezone is %%z (%%Z)'; grep -E '(^|[^0-9])([^0-9]|$)' ) | "Abuse from " ${ADDRESSES//,/\" \"} + ( printf %%b "\n"; date '+Note: Local timezone is %%z (%%Z)'; + printf %%b "\nLines containing failures of (max )\n"; + %(_grep_logs)s; + ) | "Abuse from " $ADDRESSES fi # Option: actionunban @@ -92,3 +102,7 @@ mailcmd = mail -s # mailargs = +# Number of log lines to include in the email +# +#grepmax = 1000 +#grepopts = -m diff --git a/config/action.d/helpers-common.conf b/config/action.d/helpers-common.conf new file mode 100644 index 00000000..7fa8e9e4 --- /dev/null +++ b/config/action.d/helpers-common.conf @@ -0,0 +1,13 @@ +[DEFAULT] + +# Usage: +# _grep_logs_args = 'test' +# (printf %%b "Log-excerpt contains 'test':\n"; %(_grep_logs)s; printf %%b "Log-excerpt contains 'test':\n") | mail ... +# +_grep_logs = logpath=""; grep -E %(_grep_logs_args)s $logpath | +_grep_logs_args = '(^|[^0-9])([^0-9]|$)' + +[Init] +greplimit = tail -n +grepmax = 1000 +grepopts = -m \ No newline at end of file diff --git a/config/action.d/mail-whois-lines.conf b/config/action.d/mail-whois-lines.conf index 6e39c605..cbd970c9 100644 --- a/config/action.d/mail-whois-lines.conf +++ b/config/action.d/mail-whois-lines.conf @@ -7,6 +7,7 @@ [INCLUDES] before = mail-whois-common.conf + helpers-common.conf [Definition] @@ -17,7 +18,7 @@ before = mail-whois-common.conf actionstart = printf %%b "Hi,\n The jail has been started successfully.\n Regards,\n - Fail2Ban"|mail -s "[Fail2Ban] : started on `uname -n`" + Fail2Ban" | -s "[Fail2Ban] : started on `uname -n`" # Option: actionstop # Notes.: command executed once at the end of Fail2Ban @@ -26,7 +27,7 @@ actionstart = printf %%b "Hi,\n actionstop = printf %%b "Hi,\n The jail has been stopped.\n Regards,\n - Fail2Ban"|mail -s "[Fail2Ban] : stopped on `uname -n`" + Fail2Ban" | -s "[Fail2Ban] : stopped on `uname -n`" # Option: actioncheck # Notes.: command executed once before each actionban command @@ -40,15 +41,18 @@ actioncheck = # Tags: See jail.conf(5) man page # Values: CMD # -actionban = printf %%b "Hi,\n + +_ban_mail_content = ( printf %%b "Hi,\n The IP has just been banned by Fail2Ban after attempts against .\n\n - Here is more information about :\n - `%(_whois_command)s`\n\n - Lines containing IP: in \n - `grep -E '(^|[^0-9])([^0-9]|$)' `\n\n + Here is more information about :\n" + %(_whois_command)s; + printf %%b "\nLines containing failures of (max )\n"; + %(_grep_logs)s; + printf %%b "\n Regards,\n - Fail2Ban"|mail -s "[Fail2Ban] : banned from `uname -n`" + Fail2Ban" ) +actionban = %(_ban_mail_content)s | "[Fail2Ban] : banned from `uname -n`" # Option: actionunban # Notes.: command executed when unbanning an IP. Take care that the @@ -60,6 +64,12 @@ actionunban = [Init] +# Option: mailcmd +# Notes.: Your system mail command. Is passed 2 args: subject and recipient +# Values: CMD +# +mailcmd = mail -s + # Default name of the chain # name = default @@ -74,4 +84,5 @@ logpath = /dev/null # Number of log lines to include in the email # -grepopts = -m 1000 +#grepmax = 1000 +#grepopts = -m diff --git a/config/action.d/sendmail-geoip-lines.conf b/config/action.d/sendmail-geoip-lines.conf index 2232642c..a5616e9f 100644 --- a/config/action.d/sendmail-geoip-lines.conf +++ b/config/action.d/sendmail-geoip-lines.conf @@ -7,6 +7,7 @@ [INCLUDES] before = sendmail-common.conf + helpers-common.conf [Definition] @@ -19,7 +20,7 @@ before = sendmail-common.conf # Tags: See jail.conf(5) man page # Values: CMD # -actionban = printf %%b "Subject: [Fail2Ban] : banned from `uname -n` +actionban = ( printf %%b "Subject: [Fail2Ban] : banned from `uname -n` Date: `LC_ALL=C date +"%%a, %%d %%h %%Y %%T %%z"` From: <> To: \n @@ -33,10 +34,11 @@ actionban = printf %%b "Subject: [Fail2Ban] : banned from `uname -n` Country:`geoiplookup -f /usr/share/GeoIP/GeoIP.dat "" | cut -d':' -f2-` AS:`geoiplookup -f /usr/share/GeoIP/GeoIPASNum.dat "" | cut -d':' -f2-` hostname: `host -t A 2>&1`\n\n - Lines containing IP: in \n - `grep -E '(^|[^0-9])([^0-9]|$)' `\n\n + Lines containing failures of \n"; + %(_grep_logs)s; + printf %%b "\n Regards,\n - Fail2Ban" | /usr/sbin/sendmail -f + Fail2Ban" ) | /usr/sbin/sendmail -f [Init] @@ -50,4 +52,5 @@ logpath = /dev/null # Number of log lines to include in the email # -grepopts = -m 1000 +#grepmax = 1000 +#grepopts = -m diff --git a/config/action.d/sendmail-whois-lines.conf b/config/action.d/sendmail-whois-lines.conf index 4156c947..e1c85928 100644 --- a/config/action.d/sendmail-whois-lines.conf +++ b/config/action.d/sendmail-whois-lines.conf @@ -7,6 +7,7 @@ [INCLUDES] before = sendmail-common.conf + helpers-common.conf [Definition] @@ -16,7 +17,7 @@ before = sendmail-common.conf # Tags: See jail.conf(5) man page # Values: CMD # -actionban = printf %%b "Subject: [Fail2Ban] : banned from `uname -n` +actionban = ( printf %%b "Subject: [Fail2Ban] : banned from `uname -n` Date: `LC_ALL=C date +"%%a, %%d %%h %%Y %%T %%z"` From: <> To: \n @@ -25,10 +26,11 @@ actionban = printf %%b "Subject: [Fail2Ban] : banned from `uname -n` attempts against .\n\n Here is more information about :\n `/usr/bin/whois || echo missing whois program`\n\n - Lines containing IP: in \n - `grep -E '(^|[^0-9])([^0-9]|$)' `\n\n + Lines containing failures of \n"; + %(_grep_logs)s; + printf %%b "\n Regards,\n - Fail2Ban" | /usr/sbin/sendmail -f + Fail2Ban" ) | /usr/sbin/sendmail -f [Init] @@ -42,4 +44,5 @@ logpath = /dev/null # Number of log lines to include in the email # -grepopts = -m 1000 +#grepmax = 1000 +#grepopts = -m diff --git a/fail2ban/tests/files/testcase01a.log b/fail2ban/tests/files/testcase01a.log new file mode 100644 index 00000000..203f0517 --- /dev/null +++ b/fail2ban/tests/files/testcase01a.log @@ -0,0 +1,4 @@ +Dec 31 11:55:01 [sshd] error: PAM: Authentication failure for test from 87.142.124.10 +Dec 31 11:55:02 [sshd] error: PAM: Authentication failure for test from 87.142.124.10 +Dec 31 11:55:03 [sshd] error: PAM: Authentication failure for test from 87.142.124.10 +Dec 31 11:55:04 [sshd] error: PAM: Authentication failure for test from 87.142.124.10 diff --git a/fail2ban/tests/servertestcase.py b/fail2ban/tests/servertestcase.py index 56c85e94..f8915b59 100644 --- a/fail2ban/tests/servertestcase.py +++ b/fail2ban/tests/servertestcase.py @@ -28,6 +28,7 @@ import unittest import time import tempfile import os +import re import sys import platform @@ -1609,31 +1610,114 @@ class ServerConfigReaderTests(LogCaptureTestCase): # wrap default command processor: action.executeCmd = self._executeCmd # test start : - logSys.debug('# === start ==='); self.pruneLog() + self.pruneLog('# === start ===') action.start() self.assertLogged(*tests['start'], all=True) # test ban ip4 : - logSys.debug('# === ban-ipv4 ==='); self.pruneLog() + self.pruneLog('# === ban-ipv4 ===') action.ban({'ip': IPAddr('192.0.2.1')}) self.assertLogged(*tests['ip4-check']+tests['ip4-ban'], all=True) self.assertNotLogged(*tests['ip6'], all=True) # test unban ip4 : - logSys.debug('# === unban ipv4 ==='); self.pruneLog() + self.pruneLog('# === unban ipv4 ===') action.unban({'ip': IPAddr('192.0.2.1')}) self.assertLogged(*tests['ip4-check']+tests['ip4-unban'], all=True) self.assertNotLogged(*tests['ip6'], all=True) # test ban ip6 : - logSys.debug('# === ban ipv6 ==='); self.pruneLog() + self.pruneLog('# === ban ipv6 ===') action.ban({'ip': IPAddr('2001:DB8::')}) self.assertLogged(*tests['ip6-check']+tests['ip6-ban'], all=True) self.assertNotLogged(*tests['ip4'], all=True) # test unban ip6 : - logSys.debug('# === unban ipv6 ==='); self.pruneLog() + self.pruneLog('# === unban ipv6 ===') action.unban({'ip': IPAddr('2001:DB8::')}) self.assertLogged(*tests['ip6-check']+tests['ip6-unban'], all=True) self.assertNotLogged(*tests['ip4'], all=True) # test stop : - logSys.debug('# === stop ==='); self.pruneLog() + self.pruneLog('# === stop ===') action.stop() self.assertLogged(*tests['stop'], all=True) + def _executeMailCmd(self, realCmd, timeout=60): + # replace pipe to mail with pipe to cat: + realCmd = re.sub(r'\)\s*\|\s*mail\b([^\n]*)', + r' echo mail \1 ) | cat', realCmd) + # replace abuse retrieving (possible no-network): + realCmd = re.sub(r'[^\n]+\bADDRESSES=\$\(dig\s[^\n]+', + 'ADDRESSES="abuse-1@abuse-test-server, abuse-2@abuse-test-server"', realCmd) + # execute action: + return _actions.CommandAction.executeCmd(realCmd, timeout=timeout) + + def testComplexMailActionMultiLog(self): + testJailsActions = ( + # mail-whois-lines -- + ('j-mail-whois-lines', + 'mail-whois-lines[' + 'name=%(__name__)s, grepopts="-m 1", grepmax=2, mailcmd="mail -s", ' + + # 2 logs to test grep from multiple logs: + 'logpath="' + os.path.join(TEST_FILES_DIR, "testcase01.log") + '\n' + + ' ' + os.path.join(TEST_FILES_DIR, "testcase01a.log") + '", ' + '_whois_command="echo \'-- information about --\'"' + ']', + { + 'ip4-ban': ( + 'The IP 87.142.124.10 has just been banned by Fail2Ban after', + '100 attempts against j-mail-whois-lines.', + 'Here is more information about 87.142.124.10 :', + '-- information about 87.142.124.10 --', + 'Lines containing failures of 87.142.124.10 (max 2)', + 'testcase01.log:Dec 31 11:59:59 [sshd] error: PAM: Authentication failure for kevin from 87.142.124.10', + 'testcase01a.log:Dec 31 11:55:01 [sshd] error: PAM: Authentication failure for test from 87.142.124.10', + ), + }), + # complain -- + ('j-complain-abuse', + 'complain[' + 'name=%(__name__)s, grepopts="-m 1", grepmax=2, mailcmd="mail -s",' + + # 2 logs to test grep from multiple logs: + 'logpath="' + os.path.join(TEST_FILES_DIR, "testcase01.log") + '\n' + + ' ' + os.path.join(TEST_FILES_DIR, "testcase01a.log") + '", ' + ']', + { + 'ip4-ban': ( + 'Lines containing failures of 87.142.124.10 (max 2)', + 'testcase01.log:Dec 31 11:59:59 [sshd] error: PAM: Authentication failure for kevin from 87.142.124.10', + 'testcase01a.log:Dec 31 11:55:01 [sshd] error: PAM: Authentication failure for test from 87.142.124.10', + # both abuse mails should be separated with space: + 'mail -s Abuse from 87.142.124.10 abuse-1@abuse-test-server abuse-2@abuse-test-server', + ), + }), + ) + server = TestServer() + transm = server._Server__transm + cmdHandler = transm._Transmitter__commandHandler + + for jail, act, tests in testJailsActions: + stream = self.getDefaultJailStream(jail, act) + + # for cmd in stream: + # print(cmd) + + # transmit jail to the server: + for cmd in stream: + # command to server: + ret, res = transm.proceed(cmd) + self.assertEqual(ret, 0) + + jails = server._Server__jails + + for jail, act, tests in testJailsActions: + # print(jail, jails[jail]) + for a in jails[jail].actions: + action = jails[jail].actions[a] + logSys.debug('# ' + ('=' * 50)) + logSys.debug('# == %-44s ==', jail + ' - ' + action._name) + logSys.debug('# ' + ('=' * 50)) + # wrap default command processor: + action.executeCmd = self._executeMailCmd + # test ban : + self.pruneLog('# === ban ===') + action.ban({'ip': IPAddr('87.142.124.10'), + 'failures': 100, + }) + self.assertLogged(*tests['ip4-ban'], all=True) From ec7bb0d6c9c06dcd9a09d8bd7338d9cfc4359a0d Mon Sep 17 00:00:00 2001 From: sebres Date: Fri, 25 Nov 2016 20:12:49 +0100 Subject: [PATCH 26/26] python 3x compatibility fix (positional arguments vs named arguments) --- fail2ban/client/configreader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fail2ban/client/configreader.py b/fail2ban/client/configreader.py index caab67aa..a72ca1e9 100644 --- a/fail2ban/client/configreader.py +++ b/fail2ban/client/configreader.py @@ -141,7 +141,7 @@ class ConfigReader(): def get(self, sec, opt, raw=False, vars={}): if self._cfg is not None: - return self._cfg.get(sec, opt, raw, vars) + return self._cfg.get(sec, opt, raw=raw, vars=vars) return None def getOptions(self, *args, **kwargs):