Merge pull request #2022 from sebres/0.11-fix-get-current-bans

0.11: fix get current bans (after upgrade DB)

ChangeLog

@@ -38,6 +38,8 @@ ver. 0.11.0-dev-0 (20??/??/??) - development nightly edition
 * purge database will be executed now (within observer).
 * restoring currently banned ip after service restart fixed 
   (now < timeofban + bantime), ignore old log failures (already banned)
+* upgrade database: update new created table `bips` with entries from table `bans` (allows restore
+  current bans after upgrade from version <= 0.10)
 
 ### New Features
 * Increment ban time (+ observer) functionality introduced.
@@ -50,6 +52,10 @@ ver. 0.11.0-dev-0 (20??/??/??) - development nightly edition
   Note: because ban-time is dynamic, it was removed from jail.conf as timeout argument (check jail.local).
 
 ### Enhancements
+* algorithm of restore current bans after restart changed: update the restored ban-time (and therefore 
+  end of ban) of the ticket with ban-time of jail (as maximum), for all tickets with ban-time greater
+  (or persistent); not affected if ban-time of the jail is unchanged between stop/start.
+
 
 
 ver. 0.10.3-dev-1 (20??/??/??) - development edition

fail2ban/server/database.py

@@ -378,18 +378,23 @@ def updateDb(self, cur, version):
 							"COMMIT;" % Fail2BanDb._CREATE_TABS['logs'])
 
 			if version < 3 and self._tableExists(cur, "bans"):
+				# set ban-time to -1 (note it means rather unknown, as persistent, will be fixed by restore):
 				cur.executescript("BEGIN TRANSACTION;"
-							"CREATE TEMPORARY TABLE bans_temp AS SELECT jail, ip, timeofban, 600 as bantime, 1 as bancount, data FROM bans;"
+							"CREATE TEMPORARY TABLE bans_temp AS SELECT jail, ip, timeofban, -1 as bantime, 1 as bancount, data FROM bans;"
 							"DROP TABLE bans;"
-							"%s;"
+							"%s;\n"
 							"INSERT INTO bans SELECT * from bans_temp;"
 							"DROP TABLE bans_temp;"
 							"COMMIT;" % Fail2BanDb._CREATE_TABS['bans'])
-			if version < 4:
+			if version < 4 and not self._tableExists(cur, "bips"):
 				cur.executescript("BEGIN TRANSACTION;"
-							"%s;"
+							"%s;\n"
 							"UPDATE fail2banDb SET version = 4;"
 							"COMMIT;" % Fail2BanDb._CREATE_TABS['bips'])
+				if self._tableExists(cur, "bans"):
+					cur.execute(
+							"INSERT OR REPLACE INTO bips(ip, jail, timeofban, bantime, bancount, data)"
+							"  SELECT ip, jail, timeofban, bantime, bancount, data FROM bans order by timeofban")
 
 			cur.execute("SELECT version FROM fail2banDb LIMIT 1")
 			return cur.fetchone()[0]
@@ -753,16 +758,32 @@ def _getCurrentBans(self, cur, jail = None, ip = None, forbantime=None, fromtime
 		return cur.execute(query, queryArgs)
 
 	@commitandrollback
-	def getCurrentBans(self, cur, jail = None, ip = None, forbantime=None, fromtime=None):
+	def getCurrentBans(self, cur, jail=None, ip=None, forbantime=None, fromtime=None,
+		correctBanTime=True
+	):
+		"""Reads tickets (with merged info) currently affected from ban from the database.
+		
+		There are all the tickets corresponding parameters jail/ip, forbantime,
+		fromtime (normally now).
+		
+		If correctBanTime specified (default True) it will fix the restored ban-time 
+		(and therefore endOfBan) of the ticket (normally it is ban-time of jail as maximum)
+		for all tickets with ban-time greater (or persistent).
+		"""
 		tickets = []
 		ticket = None
+		if correctBanTime is True:
+			correctBanTime = jail.actions.getBanTime() if jail is not None else None
 
 		for ticket in self._getCurrentBans(cur, jail=jail, ip=ip, 
 			forbantime=forbantime, fromtime=fromtime
 		):
 			# can produce unpack error (database may return sporadical wrong-empty row):
 			try:
 				banip, timeofban, bantime, bancount, data = ticket
+				# if persistent ban (or still unknown after upgrade), use current bantime of the jail:
+				if correctBanTime and (bantime == -1 or bantime > correctBanTime):
+					bantime = correctBanTime
 				# additionally check for empty values:
 				if banip is None or banip == "": # pragma: no cover
 					raise ValueError('unexpected value %r' % (banip,))

fail2ban/server/jail.py

@@ -263,7 +263,7 @@ def getBanTimeExtra(self, opt=None):
 			return self._banExtra.get(opt, None)
 		return self._banExtra
 
-	def restoreCurrentBans(self):
+	def restoreCurrentBans(self, correctBanTime=True):
 		"""Restore any previous valid bans from the database.
 		"""
 		try:
@@ -272,7 +272,9 @@ def restoreCurrentBans(self):
 				# use ban time as search time if we have not enabled a increasing:
 				if not self.getBanTimeExtra('increment'):
 					forbantime = self.actions.getBanTime()
-				for ticket in self.database.getCurrentBans(jail=self, forbantime=forbantime):
+				for ticket in self.database.getCurrentBans(jail=self, forbantime=forbantime,
+					correctBanTime=correctBanTime
+				):
 					try:
 						#logSys.debug('restored ticket: %s', ticket)
 						if self.filter.inIgnoreIPList(ticket.getIP(), log_ignore=True): continue

fail2ban/tests/databasetestcase.py

@@ -169,13 +169,20 @@ def testUpdateDb(self):
 
 			self.assertEqual(self.db.updateDb(Fail2BanDb.__version__), Fail2BanDb.__version__)
 			self.assertRaises(NotImplementedError, self.db.updateDb, Fail2BanDb.__version__ + 1)
+			# check current bans (should find exactly 1 ticket after upgrade):
+			tickets = self.db.getCurrentBans(fromtime=1388009242, correctBanTime=False)
+			self.assertEqual(len(tickets), 1)
+			self.assertEqual(tickets[0].getBanTime(), -1); # ban-time still unknown (normally updated from jail)
 		finally:
 			if self.db and self.db._dbFilename != ":memory:":
 				os.remove(self.db._dbBackupFilename)
 
 	def testUpdateDb2(self):
-		if Fail2BanDb is None or self.db.filename == ':memory:': # pragma: no cover
+		if Fail2BanDb is None: # pragma: no cover
 			return
+		self.db = None
+		if self.dbFilename is None: # pragma: no cover
+			_, self.dbFilename = tempfile.mkstemp(".db", "fail2ban_")
 		shutil.copyfile(
 			os.path.join(TEST_FILES_DIR, 'database_v2.db'), self.dbFilename)
 		self.db = Fail2BanDb(self.dbFilename)
@@ -195,6 +202,11 @@ def testUpdateDb2(self):
 		self.assertEqual(bans[1].getIP(), "1.2.3.8")
 		# updated ?
 		self.assertEqual(self.db.updateDb(Fail2BanDb.__version__), Fail2BanDb.__version__)
+		# check current bans (should find 2 tickets after upgrade):
+		self.jail = DummyJail(name='pam-generic')
+		tickets = self.db.getCurrentBans(jail=self.jail, fromtime=1417595494)
+		self.assertEqual(len(tickets), 2)
+		self.assertEqual(tickets[0].getBanTime(), 600)
 		# further update should fail:
 		self.assertRaises(NotImplementedError, self.db.updateDb, Fail2BanDb.__version__ + 1)
 		# clean:
@@ -380,7 +392,7 @@ def testGetBansMerged(self):
 			return
 		self.testAddJail()
 
-		jail2 = DummyJail()
+		jail2 = DummyJail(name='DummyJail-2')
 		self.db.addJail(jail2)
 
 		ticket = FailTicket("127.0.0.1", MyTime.time() - 40, ["abc\n"])
@@ -473,6 +485,13 @@ def testGetBansMerged(self):
 		tickets = self.db.getCurrentBans(jail=self.jail, forbantime=-1,
 			fromtime=MyTime.time() + MyTime.str2seconds("1year"))
 		self.assertEqual(len(tickets), 1)
+		self.assertEqual(tickets[0].getBanTime(), 600); # current jail ban time.
+		# change jail to persistent ban and try again:
+		self.jail.actions.setBanTime(-1)
+		tickets = self.db.getCurrentBans(jail=self.jail, forbantime=-1,
+			fromtime=MyTime.time() + MyTime.str2seconds("1year"))
+		self.assertEqual(len(tickets), 1)
+		self.assertEqual(tickets[0].getBanTime(), -1); # current jail ban time.
 
 	def testActionWithDB(self):
 		# test action together with database functionality

fail2ban/tests/dummyjail.py

@@ -36,10 +36,10 @@ def checkBan(self):
 class DummyJail(Jail):
 	"""A simple 'jail' to suck in all the tickets generated by Filter's
 	"""
-	def __init__(self, backend=None):
+	def __init__(self, name='DummyJail', backend=None):
 		self.lock = Lock()
 		self.queue = []
-		super(DummyJail, self).__init__(name='DummyJail', backend=backend)
+		super(DummyJail, self).__init__(name=name, backend=backend)
 		self.__db = None
 		self.__actions = DummyActions(self)
 
@@ -66,10 +66,6 @@ def getFailTicket(self):
 			except IndexError:
 				return False
 
-	@property
-	def name(self):
-		return "DummyJail #%s with %d tickets" % (id(self), len(self))
-
 	@property
 	def idle(self):
 		return False;

fail2ban/tests/observertestcase.py

@@ -263,23 +263,23 @@ def testBanTimeIncr(self):
 		)
 		# search currently banned and 1 day later (nothing should be found):
 		self.assertEqual(
-			self.db.getCurrentBans(forbantime=-24*60*60, fromtime=stime),
+			self.db.getCurrentBans(forbantime=-24*60*60, fromtime=stime, correctBanTime=False),
 			[]
 		)
 		# search currently banned one ticket for ip:
-		restored_tickets = self.db.getCurrentBans(ip=ip)
+		restored_tickets = self.db.getCurrentBans(ip=ip, correctBanTime=False)
 		self.assertEqual(
 			str(restored_tickets), 
 			('FailTicket: ip=%s time=%s bantime=20 bancount=2 #attempts=0 matches=[]' % (ip, stime + 15))
 		)
 		# search currently banned anywhere:
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(
 			str(restored_tickets),
 			('[FailTicket: ip=%s time=%s bantime=20 bancount=2 #attempts=0 matches=[]]' % (ip, stime + 15))
 		)
 		# search currently banned:
-		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime, correctBanTime=False)
 		self.assertEqual(
 			str(restored_tickets), 
 			('[FailTicket: ip=%s time=%s bantime=20 bancount=2 #attempts=0 matches=[]]' % (ip, stime + 15))
@@ -310,7 +310,7 @@ def testBanTimeIncr(self):
 		ticket2.incrBanCount()
 		self.db.addBan(jail, ticket2)
 		# search currently banned:
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 2)
 		self.assertEqual(
 			str(restored_tickets[0]),
@@ -321,7 +321,7 @@ def testBanTimeIncr(self):
 			'FailTicket: ip=%s time=%s bantime=%s bancount=1 #attempts=0 matches=[]' % (ip+'1', stime-24*60*60, 36*60*60)
 		)
 		# search out-dated (give another fromtime now is -18 hours):
-		restored_tickets = self.db.getCurrentBans(fromtime=stime-18*60*60)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime-18*60*60, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 3)
 		self.assertEqual(
 			str(restored_tickets[2]),
@@ -351,44 +351,44 @@ def testBanTimeIncr(self):
 		ticket.setBanTime(-1)
 		ticket.incrBanCount()
 		self.db.addBan(jail, ticket)
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 3)
 		self.assertEqual(
 			str(restored_tickets[2]),
 			'FailTicket: ip=%s time=%s bantime=%s bancount=1 #attempts=0 matches=[]' % (ip+'3', stime-36*60*60, -1)
 		)
 		# purge (nothing should be changed):
 		self.db.purge()
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 3)
 		# set short time and purge again:
 		ticket.setBanTime(600)
 		ticket.incrBanCount()
 		self.db.addBan(jail, ticket)
 		self.db.purge()
 		# this old ticket should be removed now:
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 2)
 		self.assertEqual(restored_tickets[0].getIP(), ip)
 
 		# purge remove 1st ip
 		self.db._purgeAge = -48*60*60
 		self.db.purge()
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		self.assertEqual(restored_tickets[0].getIP(), ip+'1')
 
 		# this should purge all bans, bips and logs - nothing should be found now
 		self.db._purgeAge = -240*60*60
 		self.db.purge()
-		restored_tickets = self.db.getCurrentBans(fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(fromtime=stime, correctBanTime=False)
 		self.assertEqual(restored_tickets, [])
 
 		# two separate jails :
 		jail1 = DummyJail(backend='polling')
 		jail1.database = self.db
 		self.db.addJail(jail1)
-		jail2 = DummyJail(backend='polling')
+		jail2 = DummyJail(name='DummyJail-2', backend='polling')
 		jail2.database = self.db
 		self.db.addJail(jail2)
 		ticket1 = FailTicket(ip, stime, [])
@@ -400,13 +400,13 @@ def testBanTimeIncr(self):
 		ticket2.setBanCount(1)
 		ticket2.incrBanCount()
 		self.db.addBan(jail2, ticket2)
-		restored_tickets = self.db.getCurrentBans(jail=jail1, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail1, fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		self.assertEqual(
 			str(restored_tickets[0]),
 			'FailTicket: ip=%s time=%s bantime=%s bancount=1 #attempts=0 matches=[]' % (ip, stime, 6000)
 		)
-		restored_tickets = self.db.getCurrentBans(jail=jail2, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail2, fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		self.assertEqual(
 			str(restored_tickets[0]),
@@ -424,13 +424,24 @@ def testBanTimeIncr(self):
 			self.assertEqual(row, (3, stime, 18000))
 			break
 		# test restoring bans from database:
-		jail1.restoreCurrentBans()
+		jail1.restoreCurrentBans(correctBanTime=False)
 		ticket = jail1.getFailTicket()
 		self.assertTrue(ticket.restored)
 		self.assertEqual(str(ticket), 
 			'FailTicket: ip=%s time=%s bantime=%s bancount=1 #attempts=0 matches=[]' % (ip, stime, 6000)
 		)
 		# jail2 does not restore any bans (because all ban tickets should be already expired: stime-6000):
+		jail2.restoreCurrentBans(correctBanTime=False)
+		self.assertEqual(jail2.getFailTicket(), False)
+		# test again, but now normally (with maximum ban-time of restored ticket allowed):
+		jail1.restoreCurrentBans()
+		ticket = jail1.getFailTicket()
+		self.assertTrue(ticket.restored)
+		# ticket restored, but it has new time = 600 (current ban-time of jail, as maximum):
+		self.assertEqual(str(ticket), 
+			'FailTicket: ip=%s time=%s bantime=%s bancount=1 #attempts=0 matches=[]' % (ip, stime, 600)
+		)
+		# jail2 does not restore any bans (because all ban tickets should be already expired: stime-6000):
 		jail2.restoreCurrentBans()
 		self.assertEqual(jail2.getFailTicket(), False)
 
@@ -478,7 +489,7 @@ def testObserver(self):
 		# add manually 4th times banned (added to bips - make ip bad):
 		ticket.setBanCount(4)
 		self.db.addBan(self.jail, ticket)
-		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime-120)
+		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime-120, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		# check again, new ticket, new failmanager:
 		ticket = FailTicket(ip, stime, [])
@@ -506,7 +517,7 @@ def testObserver(self):
 		self.assertEqual(ticket2.getBanCount(), 5)
 
 		# check prolonged in database also :
-		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		self.assertEqual(restored_tickets[0].getBanTime(), 160)
 		self.assertEqual(restored_tickets[0].getBanCount(), 5)
@@ -521,7 +532,7 @@ def testObserver(self):
 		self.assertTrue(jail.actions.checkBan())
 
 		obs.wait_empty(5)
-		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 1)
 		self.assertEqual(restored_tickets[0].getBanTime(), 320)
 		self.assertEqual(restored_tickets[0].getBanCount(), 6)
@@ -539,7 +550,7 @@ def testObserver(self):
 		self.assertFalse(jail.actions.checkBan())
 
 		obs.wait_empty(5)
-		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime)
+		restored_tickets = self.db.getCurrentBans(jail=jail, fromtime=stime, correctBanTime=False)
 		self.assertEqual(len(restored_tickets), 2)
 		self.assertEqual(restored_tickets[1].getBanTime(), -1)
 		self.assertEqual(restored_tickets[1].getBanCount(), 1)