From 3a179ec5d7121003683c16db77096247b0fe63f7 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 2 Dec 2015 19:51:29 +0100 Subject: [PATCH 1/7] small code review: (much pretty) handling of filename as key - FileFilter contains (ordered) dict of files (not list), as discussed in gh-1265 --- ChangeLog | 2 ++ fail2ban/server/filter.py | 46 +++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/ChangeLog b/ChangeLog index 20b708a8..225a8a8b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -59,6 +59,8 @@ ver. 0.9.4 (2015/XX/XXX) - wanna-be-released rest api and web interface (gh-1223) * Add *_backend options for services to allow distros to set the default backend per service, set default to systemd for Fedora as appropriate + * small improvement for better handling of many log files (gh-1265) + Thanks @kshetragia ver. 0.9.3 (2015/08/01) - lets-all-stay-friends ---------- diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index 65be0467..e0579c6e 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -38,6 +38,11 @@ from .failregex import FailRegex, Regex, RegexException from .action import CommandAction from ..helpers import getLogger +try: + from collections import OrderedDict +except ImportError: + OrderedDict = dict + # Gets the instance of the logger. logSys = getLogger(__name__) @@ -552,7 +557,7 @@ class FileFilter(Filter): def __init__(self, jail, **kwargs): Filter.__init__(self, jail, **kwargs) ## The log file path. - self.__logPath = [] + self.__logs = OrderedDict() self.setLogEncoding("auto") ## @@ -561,7 +566,7 @@ class FileFilter(Filter): # @param path log file path def addLogPath(self, path, tail = False): - if self.containsLogPath(path): + if path in self.__logs: logSys.error(path + " already exists") else: container = FileContainer(path, self.getLogEncoding(), tail) @@ -570,7 +575,7 @@ class FileFilter(Filter): lastpos = db.addLog(self.jail, container) if lastpos and not tail: container.setPos(lastpos) - self.__logPath.append(container) + self.__logs[path] = container logSys.info("Added logfile = %s" % path) self._addLogPath(path) # backend specific @@ -585,15 +590,16 @@ class FileFilter(Filter): # @param path the log file to delete def delLogPath(self, path): - for log in self.__logPath: - if log.getFileName() == path: - self.__logPath.remove(log) - db = self.jail.database - if db is not None: - db.updateLog(self.jail, log) - logSys.info("Removed logfile = %s" % path) - self._delLogPath(path) - return + try: + log = self.__logs.pop(path) + except KeyError: + return + db = self.jail.database + if db is not None: + db.updateLog(self.jail, log) + logSys.info("Removed logfile = %s" % path) + self._delLogPath(path) + return def _delLogPath(self, path): # pragma: no cover - overwritten function # nothing to do by default @@ -606,7 +612,7 @@ class FileFilter(Filter): # @return log file path def getLogPath(self): - return self.__logPath + return self.__logs.values() ## # Check whether path is already monitored. @@ -615,10 +621,7 @@ class FileFilter(Filter): # @return True if the path is already monitored else False def containsLogPath(self, path): - for log in self.__logPath: - if log.getFileName() == path: - return True - return False + return path in self.__logs ## # Set the log file encoding @@ -629,7 +632,7 @@ class FileFilter(Filter): if encoding.lower() == "auto": encoding = locale.getpreferredencoding() codecs.lookup(encoding) # Raise LookupError if invalid codec - for log in self.getLogPath(): + for log in self.__logs.itervalues(): log.setEncoding(encoding) self.__encoding = encoding logSys.info("Set jail log file encoding to %s" % encoding) @@ -643,10 +646,7 @@ class FileFilter(Filter): return self.__encoding def getFileContainer(self, path): - for log in self.__logPath: - if log.getFileName() == path: - return log - return None + return self.__logs.get(path, None) ## # Gets all the failure in the log file. @@ -698,7 +698,7 @@ class FileFilter(Filter): """Status of Filter plus files being monitored. """ ret = super(FileFilter, self).status(flavor=flavor) - path = [m.getFileName() for m in self.getLogPath()] + path = self.__logs.keys() ret.append(("File list", path)) return ret From 6ce7522d3cebbc6a3adcd066d3189f7cff40536c Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 2 Dec 2015 21:06:32 +0100 Subject: [PATCH 2/7] unordered (python 2.6) compatibility fix and coverage extended; --- fail2ban/tests/filtertestcase.py | 10 ++++++++++ fail2ban/tests/servertestcase.py | 20 ++++++++------------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/fail2ban/tests/filtertestcase.py b/fail2ban/tests/filtertestcase.py index 3674a574..69b6e5b5 100644 --- a/fail2ban/tests/filtertestcase.py +++ b/fail2ban/tests/filtertestcase.py @@ -859,6 +859,16 @@ class GetFailures(LogCaptureTestCase): self.filter.delLogPath(GetFailures.FILENAME_01) self.assertEqual(self.filter.getLogPath(),[]) + def testNoLogAdded(self): + self.filter.addLogPath(GetFailures.FILENAME_01, tail=True) + self.assertTrue(self.filter.containsLogPath(GetFailures.FILENAME_01)) + self.filter.delLogPath(GetFailures.FILENAME_01) + self.assertFalse(self.filter.containsLogPath(GetFailures.FILENAME_01)) + # and unknown (safety and cover) + self.assertFalse(self.filter.containsLogPath('unknown.log')) + self.filter.delLogPath('unknown.log') + + def testGetFailures01(self, filename=None, failures=None): filename = filename or GetFailures.FILENAME_01 failures = failures or GetFailures.FAILURES_01 diff --git a/fail2ban/tests/servertestcase.py b/fail2ban/tests/servertestcase.py index 86ffdb46..07e10c7d 100644 --- a/fail2ban/tests/servertestcase.py +++ b/fail2ban/tests/servertestcase.py @@ -113,19 +113,15 @@ class TransmitterBase(unittest.TestCase): self.assertEqual( self.transm.proceed(["get", jail, cmd]), (0, [])) for n, value in enumerate(values): - self.assertEqual( - self.transm.proceed(["set", jail, cmdAdd, value]), - (0, values[:n+1])) - self.assertEqual( - self.transm.proceed(["get", jail, cmd]), - (0, values[:n+1])) + ret = self.transm.proceed(["set", jail, cmdAdd, value]) + self.assertEqual((ret[0], sorted(ret[1])), (0, sorted(values[:n+1]))) + ret = self.transm.proceed(["get", jail, cmd]) + self.assertEqual((ret[0], sorted(ret[1])), (0, sorted(values[:n+1]))) for n, value in enumerate(values): - self.assertEqual( - self.transm.proceed(["set", jail, cmdDel, value]), - (0, values[n+1:])) - self.assertEqual( - self.transm.proceed(["get", jail, cmd]), - (0, values[n+1:])) + ret = self.transm.proceed(["set", jail, cmdDel, value]) + self.assertEqual((ret[0], sorted(ret[1])), (0, sorted(values[n+1:]))) + ret = self.transm.proceed(["get", jail, cmd]) + self.assertEqual((ret[0], sorted(ret[1])), (0, sorted(values[n+1:]))) def jailAddDelRegexTest(self, cmd, inValues, outValues, jail): cmdAdd = "add" + cmd From dd9d1912e88d09910ae8173a70d76c58afd9d3f1 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 2 Dec 2015 22:49:47 -0500 Subject: [PATCH 3/7] RF: Filter.getLogPaths -> getLogs Since it returns log containers not paths per se --- fail2ban/server/filter.py | 8 ++++---- fail2ban/server/filtergamin.py | 4 ++-- fail2ban/server/filterpoll.py | 4 ++-- fail2ban/server/server.py | 2 +- fail2ban/tests/filtertestcase.py | 8 ++++---- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index e0579c6e..ab4e4d5b 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -565,7 +565,7 @@ class FileFilter(Filter): # # @param path log file path - def addLogPath(self, path, tail = False): + def addLogPath(self, path, tail=False): if path in self.__logs: logSys.error(path + " already exists") else: @@ -607,11 +607,11 @@ class FileFilter(Filter): pass ## - # Get the log file path + # Get the log containers # - # @return log file path + # @return log containers - def getLogPath(self): + def getLogs(self): return self.__logs.values() ## diff --git a/fail2ban/server/filtergamin.py b/fail2ban/server/filtergamin.py index 1f51744b..e731a8e9 100644 --- a/fail2ban/server/filtergamin.py +++ b/fail2ban/server/filtergamin.py @@ -129,6 +129,6 @@ class FilterGamin(FileFilter): # Desallocates the resources used by Gamin. def __cleanup(self): - for path in self.getLogPath(): - self.monitor.stop_watch(path.getFileName()) + for log in self.getLogs(): + self.monitor.stop_watch(log.getFileName()) del self.monitor diff --git a/fail2ban/server/filterpoll.py b/fail2ban/server/filterpoll.py index 25c3e119..d0b37775 100644 --- a/fail2ban/server/filterpoll.py +++ b/fail2ban/server/filterpoll.py @@ -88,10 +88,10 @@ class FilterPoll(FileFilter): while self.active: if logSys.getEffectiveLevel() <= 6: logSys.log(6, "Woke up idle=%s with %d files monitored", - self.idle, len(self.getLogPath())) + self.idle, len(self.getLogs())) if not self.idle: # Get file modification - for container in self.getLogPath(): + for container in self.getLogs(): filename = container.getFileName() if self.isModified(filename): self.getFailures(filename) diff --git a/fail2ban/server/server.py b/fail2ban/server/server.py index 6d19544d..3e371945 100644 --- a/fail2ban/server/server.py +++ b/fail2ban/server/server.py @@ -212,7 +212,7 @@ class Server: filter_ = self.__jails[name].filter if isinstance(filter_, FileFilter): return [m.getFileName() - for m in filter_.getLogPath()] + for m in filter_.getLogs()] else: # pragma: systemd no cover logSys.info("Jail %s is not a FileFilter instance" % name) return [] diff --git a/fail2ban/tests/filtertestcase.py b/fail2ban/tests/filtertestcase.py index 69b6e5b5..39a1b352 100644 --- a/fail2ban/tests/filtertestcase.py +++ b/fail2ban/tests/filtertestcase.py @@ -853,11 +853,11 @@ class GetFailures(LogCaptureTestCase): def testTail(self): self.filter.addLogPath(GetFailures.FILENAME_01, tail=True) - self.assertEqual(self.filter.getLogPath()[-1].getPos(), 1653) - self.filter.getLogPath()[-1].close() - self.assertEqual(self.filter.getLogPath()[-1].readline(), "") + self.assertEqual(self.filter.getLogs()[-1].getPos(), 1653) + self.filter.getLogs()[-1].close() + self.assertEqual(self.filter.getLogs()[-1].readline(), "") self.filter.delLogPath(GetFailures.FILENAME_01) - self.assertEqual(self.filter.getLogPath(),[]) + self.assertEqual(self.filter.getLogs(), []) def testNoLogAdded(self): self.filter.addLogPath(GetFailures.FILENAME_01, tail=True) From 59da27b9f659badc2d28ada465eb575ebf4a651c Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 2 Dec 2015 22:53:28 -0500 Subject: [PATCH 4/7] ENH: add a check to testTail to assure correct test logic below it --- fail2ban/tests/filtertestcase.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fail2ban/tests/filtertestcase.py b/fail2ban/tests/filtertestcase.py index 39a1b352..40879b66 100644 --- a/fail2ban/tests/filtertestcase.py +++ b/fail2ban/tests/filtertestcase.py @@ -852,6 +852,8 @@ class GetFailures(LogCaptureTestCase): LogCaptureTestCase.tearDown(self) def testTail(self): + # There must be no containters registered, otherwise [-1] indexing would be wrong + self.assertEqual(self.filter.getLogs(), []) self.filter.addLogPath(GetFailures.FILENAME_01, tail=True) self.assertEqual(self.filter.getLogs()[-1].getPos(), 1653) self.filter.getLogs()[-1].close() From 48202f998d3ad672f8b56ae5e9d1854131d0a31c Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 2 Dec 2015 22:57:40 -0500 Subject: [PATCH 5/7] RF: prefer log over container in getLog and local variables Even though I have left FileContainer class name intact --- fail2ban/server/filter.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index ab4e4d5b..8f4f602a 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -569,13 +569,13 @@ class FileFilter(Filter): if path in self.__logs: logSys.error(path + " already exists") else: - container = FileContainer(path, self.getLogEncoding(), tail) + log = FileContainer(path, self.getLogEncoding(), tail) db = self.jail.database if db is not None: - lastpos = db.addLog(self.jail, container) + lastpos = db.addLog(self.jail, log) if lastpos and not tail: - container.setPos(lastpos) - self.__logs[path] = container + log.setPos(lastpos) + self.__logs[path] = log logSys.info("Added logfile = %s" % path) self._addLogPath(path) # backend specific @@ -645,7 +645,7 @@ class FileFilter(Filter): def getLogEncoding(self): return self.__encoding - def getFileContainer(self, path): + def getLog(self, path): return self.__logs.get(path, None) ## @@ -656,13 +656,13 @@ class FileFilter(Filter): # is created and is added to the FailManager. def getFailures(self, filename): - container = self.getFileContainer(filename) - if container is None: + log = self.getLog(filename) + if log is None: logSys.error("Unable to get failures in " + filename) return False # Try to open log file. try: - has_content = container.open() + has_content = log.open() # see http://python.org/dev/peps/pep-3151/ except IOError, e: logSys.error("Unable to open %s" % filename) @@ -683,15 +683,15 @@ class FileFilter(Filter): # start reading tested to be empty container -- race condition # might occur leading at least to tests failures. while has_content: - line = container.readline() + line = log.readline() if not line or not self.active: # The jail reached the bottom or has been stopped break self.processLineAndAdd(line) - container.close() + log.close() db = self.jail.database if db is not None: - db.updateLog(self.jail, container) + db.updateLog(self.jail, log) return True def status(self, flavor="basic"): From 6d984717b5f743d836840d851b1d0814f3da842a Mon Sep 17 00:00:00 2001 From: sebres Date: Sat, 12 Dec 2015 15:46:45 +0100 Subject: [PATCH 6/7] ordered dict replaced with dict + change log entry fix # Conflicts: # fail2ban/server/filter.py --- ChangeLog | 5 +++-- fail2ban/server/filter.py | 7 +------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index 225a8a8b..42fc0b38 100644 --- a/ChangeLog +++ b/ChangeLog @@ -59,8 +59,9 @@ ver. 0.9.4 (2015/XX/XXX) - wanna-be-released rest api and web interface (gh-1223) * Add *_backend options for services to allow distros to set the default backend per service, set default to systemd for Fedora as appropriate - * small improvement for better handling of many log files (gh-1265) - Thanks @kshetragia + * Performance improvements while monitoring large number of files (gh-1265). + Use associative array (dict) for monitored log files to speed up lookup + operations. Thanks @kshetragia ver. 0.9.3 (2015/08/01) - lets-all-stay-friends ---------- diff --git a/fail2ban/server/filter.py b/fail2ban/server/filter.py index 8f4f602a..2b354f7a 100644 --- a/fail2ban/server/filter.py +++ b/fail2ban/server/filter.py @@ -38,11 +38,6 @@ from .failregex import FailRegex, Regex, RegexException from .action import CommandAction from ..helpers import getLogger -try: - from collections import OrderedDict -except ImportError: - OrderedDict = dict - # Gets the instance of the logger. logSys = getLogger(__name__) @@ -557,7 +552,7 @@ class FileFilter(Filter): def __init__(self, jail, **kwargs): Filter.__init__(self, jail, **kwargs) ## The log file path. - self.__logs = OrderedDict() + self.__logs = dict() self.setLogEncoding("auto") ## From 5d6cead99694efae8710ab12cd68a924a83d7396 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sun, 13 Dec 2015 23:21:04 -0500 Subject: [PATCH 7/7] ENH: sshd filter -- match new "maximum auth attempts exceeded" (Closes #1269) --- ChangeLog | 2 ++ config/filter.d/sshd.conf | 1 + fail2ban/tests/files/logs/sshd | 3 +++ 3 files changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 42fc0b38..90cdae59 100644 --- a/ChangeLog +++ b/ChangeLog @@ -41,6 +41,8 @@ ver. 0.9.4 (2015/XX/XXX) - wanna-be-released rest api and web interface (gh-1223) - nginx-limit-req - ban hosts, that were failed through nginx by limit request processing rate (ngx_http_limit_req_module) + * sshd filter got new failregex to match "maximum authentication + attempts exceeded" (introduced in openssh 6.8) - Enhancements: * Do not rotate empty log files diff --git a/config/filter.d/sshd.conf b/config/filter.d/sshd.conf index 5fad2b32..180ac52a 100644 --- a/config/filter.d/sshd.conf +++ b/config/filter.d/sshd.conf @@ -33,6 +33,7 @@ failregex = ^%(__prefix_line)s(?:error: PAM: )?[aA]uthentication (?:failure|erro ^(?P<__prefix>%(__prefix_line)s)User .+ not allowed because account is locked(?P=__prefix)(?:error: )?Received disconnect from : 11: .+ \[preauth\]$ ^(?P<__prefix>%(__prefix_line)s)Disconnecting: Too many authentication failures for .+? \[preauth\](?P=__prefix)(?:error: )?Connection closed by \[preauth\]$ ^(?P<__prefix>%(__prefix_line)s)Connection from port \d+(?: on \S+ port \d+)?(?P=__prefix)Disconnecting: Too many authentication failures for .+? \[preauth\]$ + ^%(__prefix_line)s(error: )?maximum authentication attempts exceeded for .* from (?: port \d*)?(?: ssh\d*)? \[preauth\]$ ^%(__prefix_line)spam_unix\(sshd:auth\):\s+authentication failure;\s*logname=\S*\s*uid=\d*\s*euid=\d*\s*tty=\S*\s*ruser=\S*\s*rhost=\s.*$ ignoreregex = diff --git a/fail2ban/tests/files/logs/sshd b/fail2ban/tests/files/logs/sshd index 62204339..7baf4be7 100644 --- a/fail2ban/tests/files/logs/sshd +++ b/fail2ban/tests/files/logs/sshd @@ -148,6 +148,9 @@ Feb 12 04:09:18 localhost sshd[26713]: Connection from 115.249.163.77 port 51353 # failJSON: { "time": "2005-02-12T04:09:21", "match": true , "host": "115.249.163.77", "desc": "Multiline match with interface address" } Feb 12 04:09:21 localhost sshd[26713]: Disconnecting: Too many authentication failures for root [preauth] +# failJSON: { "time": "2004-11-23T21:50:37", "match": true , "host": "61.0.0.1", "desc": "New logline format as openssh 6.8 to replace prev multiline version" } +Nov 23 21:50:37 myhost sshd[21810]: error: maximum authentication attempts exceeded for root from 61.0.0.1 port 49940 ssh2 [preauth] + # failJSON: { "match": false } Apr 27 13:02:04 host sshd[29116]: User root not allowed because account is locked # failJSON: { "match": false }