From 2b38d46fb5a6a73a27f2ff4bb04394a8b77e1a53 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 23 Sep 2014 19:57:55 +0200 Subject: [PATCH] actions: bug fix in lambdas in checkBan, because getBansMerged could return None (purge resp. asynchronous addBan), make the logic all around more stable; test cases: extended with test to check action together with database functionality (ex.: to verify lambdas in checkBan); database: getBansMerged should work within lock, using reentrant lock (cause call of getBans inside of getBansMerged); --- fail2ban/server/actions.py | 53 +++++++++++--- fail2ban/server/database.py | 71 ++++++++++--------- fail2ban/tests/databasetestcase.py | 23 +++++- .../tests/files/action.d/action_checkainfo.py | 14 ++++ 4 files changed, 118 insertions(+), 43 deletions(-) create mode 100644 fail2ban/tests/files/action.d/action_checkainfo.py diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index 15a16e99..8a1d8ec9 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -246,6 +246,44 @@ class Actions(JailThread, Mapping): logSys.debug(self._jail.name + ": action terminated") return True + def __getBansMerged(self, mi, idx): + """Helper for lamda to get bans merged once + + This function never returns None for ainfo lambdas - always a ticket (merged or single one) + and prevents any errors through merging (to guarantee ban actions will be executed). + [TODO] move merging to observer - here we could wait for merge and read already merged info from a database + + Parameters + ---------- + mi : dict + initial for lambda should contains {ip, ticket} + idx : str + key to get a merged bans : + 'all' - bans merged for all jails + 'jail' - bans merged for current jail only + + Returns + ------- + BanTicket + merged or self ticket only + """ + if idx in mi: + return mi[idx] if mi[idx] is not None else mi['ticket'] + try: + jail=self._jail + ip=mi['ip'] + mi[idx] = None + if idx == 'all': + mi[idx] = jail.database.getBansMerged(ip=ip) + elif idx == 'jail': + mi[idx] = jail.database.getBansMerged(ip=ip, jail=jail) + except Exception as e: + logSys.error( + "Failed to get %s bans merged, jail '%s': %s", + idx, jail.name, e, + exc_info=logSys.getEffectiveLevel()<=logging.DEBUG) + return mi[idx] if mi[idx] is not None else mi['ticket'] + def __checkBan(self): """Check for IP address to ban. @@ -272,16 +310,13 @@ class Actions(JailThread, Mapping): aInfo["time"] = bTicket.getTime() aInfo["matches"] = "\n".join(bTicket.getMatches()) btime = bTicket.getBanTime(self.__banManager.getBanTime()) - # [todo] move merging to observer - here we could read already merged info from database (faster); + # retarded merge info via twice lambdas : once for merge, once for matches/failures: if self._jail.database is not None: - aInfo["ipmatches"] = lambda jail=self._jail: "\n".join( - jail.database.getBansMerged(ip=ip).getMatches()) - aInfo["ipjailmatches"] = lambda jail=self._jail: "\n".join( - jail.database.getBansMerged(ip=ip, jail=jail).getMatches()) - aInfo["ipfailures"] = lambda jail=self._jail: \ - jail.database.getBansMerged(ip=ip).getAttempt() - aInfo["ipjailfailures"] = lambda jail=self._jail: \ - jail.database.getBansMerged(ip=ip, jail=jail).getAttempt() + mi4ip = lambda idx, self=self, mi={'ip':ip, 'ticket':bTicket}: self.__getBansMerged(mi, idx) + aInfo["ipmatches"] = lambda: "\n".join(mi4ip('all').getMatches()) + aInfo["ipjailmatches"] = lambda: "\n".join(mi4ip('jail').getMatches()) + aInfo["ipfailures"] = lambda: mi4ip('all').getAttempt() + aInfo["ipjailfailures"] = lambda: mi4ip('jail').getAttempt() if btime != -1: bendtime = aInfo["time"] + btime diff --git a/fail2ban/server/database.py b/fail2ban/server/database.py index 8fdadbd6..8227dea5 100644 --- a/fail2ban/server/database.py +++ b/fail2ban/server/database.py @@ -27,7 +27,7 @@ import sqlite3 import json import locale from functools import wraps -from threading import Lock +from threading import RLock from .mytime import MyTime from .ticket import FailTicket @@ -138,7 +138,7 @@ class Fail2BanDb(object): def __init__(self, filename, purgeAge=24*60*60, outDatedFactor=3): try: - self._lock = Lock() + self._lock = RLock() self._db = sqlite3.connect( filename, check_same_thread=False, detect_types=sqlite3.PARSE_DECLTYPES) @@ -397,6 +397,10 @@ class Fail2BanDb(object): del self._bansMergedCache[(ticket.getIP(), jail)] except KeyError: pass + try: + del self._bansMergedCache[(ticket.getIP(), None)] + except KeyError: + pass #TODO: Implement data parts once arbitrary match keys completed cur.execute( "INSERT INTO bans(jail, ip, timeofban, bantime, bancount, data) VALUES(?, ?, ?, ?, ?, ?)", @@ -496,40 +500,41 @@ class Fail2BanDb(object): in a list. When `ip` argument passed, a single `Ticket` is returned. """ - cacheKey = None - if bantime is None or bantime < 0: - cacheKey = (ip, jail) - if cacheKey in self._bansMergedCache: - return self._bansMergedCache[cacheKey] + with self._lock: + cacheKey = None + if bantime is None or bantime < 0: + cacheKey = (ip, jail) + if cacheKey in self._bansMergedCache: + return self._bansMergedCache[cacheKey] - tickets = [] - ticket = None + tickets = [] + ticket = None - results = list(self._getBans(ip=ip, jail=jail, bantime=bantime)) - if results: - prev_banip = results[0][0] - matches = [] - failures = 0 - for banip, timeofban, data in results: - #TODO: Implement data parts once arbitrary match keys completed - if banip != prev_banip: - ticket = FailTicket(prev_banip, prev_timeofban, matches) - ticket.setAttempt(failures) - tickets.append(ticket) - # Reset variables - prev_banip = banip - matches = [] - failures = 0 - matches.extend(data['matches']) - failures += data['failures'] - prev_timeofban = timeofban - ticket = FailTicket(banip, prev_timeofban, matches) - ticket.setAttempt(failures) - tickets.append(ticket) + results = list(self._getBans(ip=ip, jail=jail, bantime=bantime)) + if results: + prev_banip = results[0][0] + matches = [] + failures = 0 + for banip, timeofban, data in results: + #TODO: Implement data parts once arbitrary match keys completed + if banip != prev_banip: + ticket = FailTicket(prev_banip, prev_timeofban, matches) + ticket.setAttempt(failures) + tickets.append(ticket) + # Reset variables + prev_banip = banip + matches = [] + failures = 0 + matches.extend(data['matches']) + failures += data['failures'] + prev_timeofban = timeofban + ticket = FailTicket(banip, prev_timeofban, matches) + ticket.setAttempt(failures) + tickets.append(ticket) - if cacheKey: - self._bansMergedCache[cacheKey] = tickets if ip is None else ticket - return tickets if ip is None else ticket + if cacheKey: + self._bansMergedCache[cacheKey] = tickets if ip is None else ticket + return tickets if ip is None else ticket @commitandrollback def getBan(self, cur, ip, jail=None, forbantime=None, overalljails=None, fromtime=None): diff --git a/fail2ban/tests/databasetestcase.py b/fail2ban/tests/databasetestcase.py index fef7dcae..5a06a301 100644 --- a/fail2ban/tests/databasetestcase.py +++ b/fail2ban/tests/databasetestcase.py @@ -32,18 +32,21 @@ import shutil from ..server.filter import FileContainer from ..server.mytime import MyTime from ..server.ticket import FailTicket +from ..server.actions import Actions from .dummyjail import DummyJail try: from ..server.database import Fail2BanDb except ImportError: Fail2BanDb = None +from .utils import LogCaptureTestCase TEST_FILES_DIR = os.path.join(os.path.dirname(__file__), "files") -class DatabaseTest(unittest.TestCase): +class DatabaseTest(LogCaptureTestCase): def setUp(self): """Call before every test case.""" + super(DatabaseTest, self).setUp() if Fail2BanDb is None and sys.version_info >= (2,7): # pragma: no cover raise unittest.SkipTest( "Unable to import fail2ban database module as sqlite is not " @@ -55,6 +58,7 @@ class DatabaseTest(unittest.TestCase): def tearDown(self): """Call after every test case.""" + super(DatabaseTest, self).tearDown() if Fail2BanDb is None: # pragma: no cover return # Cleanup @@ -267,6 +271,23 @@ class DatabaseTest(unittest.TestCase): tickets = self.db.getBansMerged(bantime=-1) self.assertEqual(len(tickets), 2) + def testActionWithDB(self): + # test action together with database functionality + self.testAddJail() # Jail required + self.jail.database = self.db; + actions = Actions(self.jail) + actions.add( + "action_checkainfo", + os.path.join(TEST_FILES_DIR, "action.d/action_checkainfo.py"), + {}) + ticket = FailTicket("1.2.3.4") + ticket.setAttempt(5) + ticket.setMatches(['test', 'test']) + self.jail.putFailTicket(ticket) + actions._Actions__checkBan() + self.assertTrue(self._is_logged("ban ainfo %s, %s, %s, %s" % (True, True, True, True))) + + def testPurge(self): if Fail2BanDb is None: # pragma: no cover return diff --git a/fail2ban/tests/files/action.d/action_checkainfo.py b/fail2ban/tests/files/action.d/action_checkainfo.py new file mode 100644 index 00000000..eec9cc85 --- /dev/null +++ b/fail2ban/tests/files/action.d/action_checkainfo.py @@ -0,0 +1,14 @@ + +from fail2ban.server.action import ActionBase + +class TestAction(ActionBase): + + def ban(self, aInfo): + self._logSys.info("ban ainfo %s, %s, %s, %s", + aInfo["ipmatches"] != '', aInfo["ipjailmatches"] != '', aInfo["ipfailures"] > 0, aInfo["ipjailfailures"] > 0 + ) + + def unban(self, aInfo): + pass + +Action = TestAction