Merge pull request #670 from kwirk/reban-once-per-ip

BF: On jail restart reinstatement of bans, fetch one ticket per IP

ChangeLog

@@ -17,6 +17,7 @@ ver. 0.9.1 (2014/xx/xx) - better, faster, stronger
    * journalmatch for recidive incorrect PRIORITY
    * loglevel couldn't be changed in fail2ban.conf
    * Handle case when no sqlite library is available for persistent database
+   * Only reban once per IP from database on fail2ban restart
 
 - New features:
 

fail2ban/server/database.py

@@ -361,7 +361,10 @@ class Fail2BanDb(object):
 		ticket : BanTicket
 			Ticket of the ban to be added.
 		"""
-		self._bansMergedCache = {}
+		try:
+			del self._bansMergedCache[(ticket.getIP(), jail)]
+		except KeyError:
+			pass
 		#TODO: Implement data parts once arbitrary match keys completed
 		cur.execute(
 			"INSERT INTO bans(jail, ip, timeofban, data) VALUES(?, ?, ?, ?)",
@@ -383,7 +386,7 @@ class Fail2BanDb(object):
 		if ip is not None:
 			query += " AND ip=?"
 			queryArgs.append(ip)
-		query += " ORDER BY timeofban"
+		query += " ORDER BY ip, timeofban"
 
 		return cur.execute(query, queryArgs)
 
@@ -412,7 +415,7 @@ class Fail2BanDb(object):
 			tickets[-1].setAttempt(data['failures'])
 		return tickets
 
-	def getBansMerged(self, ip, jail=None, **kwargs):
+	def getBansMerged(self, ip=None, jail=None, bantime=None):
 		"""Get bans from the database, merged into single ticket.
 
 		This is the same as `getBans`, but bans merged into single
@@ -430,22 +433,44 @@ class Fail2BanDb(object):
 
 		Returns
 		-------
-		Ticket
+		list or Ticket
-			Single ticket representing bans stored in database.
+			Single ticket representing bans stored in database per IP
+			in a list. When `ip` argument passed, a single `Ticket` is
+			returned.
 		"""
-		cacheKey = ip if jail is None else "%s|%s" % (ip, jail.name)
+		if bantime is None:
-		if cacheKey in self._bansMergedCache:
+			cacheKey = (ip, jail)
-			return self._bansMergedCache[cacheKey]
+			if cacheKey in self._bansMergedCache:
-		matches = []
+				return self._bansMergedCache[cacheKey]
-		failures = 0
+
-		for ip, timeofban, data in self._getBans(ip=ip, jail=jail, **kwargs):
+		tickets = []
-			#TODO: Implement data parts once arbitrary match keys completed
+		ticket = None
-			matches.extend(data['matches'])
+
-			failures += data['failures']
+		results = list(self._getBans(ip=ip, jail=jail, bantime=bantime))
-		ticket = FailTicket(ip, timeofban, matches)
+		if results:
-		ticket.setAttempt(failures)
+			prev_banip = results[0][0]
-		self._bansMergedCache[cacheKey] = ticket
+			matches = []
-		return ticket
+			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 bantime is None:
+			self._bansMergedCache[cacheKey] = tickets if ip is None else ticket
+		return tickets if ip is None else ticket
 
 	@commitandrollback
 	def purge(self, cur):

fail2ban/server/jail.py

@@ -211,7 +211,7 @@ class Jail:
 		self.actions.start()
 		# Restore any previous valid bans from the database
 		if self.database is not None:
-			for ticket in self.database.getBans(
+			for ticket in self.database.getBansMerged(
 				jail=self, bantime=self.actions.getBanTime()):
 				if not self.filter.inIgnoreIPList(ticket.getIP()):
 					self.__queue.put(ticket)

fail2ban/tests/databasetestcase.py

@@ -190,16 +190,16 @@ class DatabaseTest(unittest.TestCase):
 		jail2 = DummyJail()
 		self.db.addJail(jail2)
 
-		ticket = FailTicket("127.0.0.1", 10, ["abc\n"])
+		ticket = FailTicket("127.0.0.1", MyTime.time() - 40, ["abc\n"])
 		ticket.setAttempt(10)
 		self.db.addBan(self.jail, ticket)
-		ticket = FailTicket("127.0.0.1", 20, ["123\n"])
+		ticket = FailTicket("127.0.0.1", MyTime.time() - 30, ["123\n"])
 		ticket.setAttempt(20)
 		self.db.addBan(self.jail, ticket)
-		ticket = FailTicket("127.0.0.2", 30, ["ABC\n"])
+		ticket = FailTicket("127.0.0.2", MyTime.time() - 20, ["ABC\n"])
 		ticket.setAttempt(30)
 		self.db.addBan(self.jail, ticket)
-		ticket = FailTicket("127.0.0.1", 40, ["ABC\n"])
+		ticket = FailTicket("127.0.0.1", MyTime.time() - 10, ["ABC\n"])
 		ticket.setAttempt(40)
 		self.db.addBan(jail2, ticket)
 
@@ -220,7 +220,15 @@ class DatabaseTest(unittest.TestCase):
 			id(ticket),
 			id(self.db.getBansMerged("127.0.0.1", jail=self.jail)))
 
-		newTicket = FailTicket("127.0.0.1", 40, ["ABC\n"])
+		newTicket = FailTicket("127.0.0.2", MyTime.time() - 20, ["ABC\n"])
+		ticket.setAttempt(40)
+		# Add ticket, but not for same IP, so cache still valid
+		self.db.addBan(self.jail, newTicket)
+		self.assertEqual(
+			id(ticket),
+			id(self.db.getBansMerged("127.0.0.1", jail=self.jail)))
+
+		newTicket = FailTicket("127.0.0.1", MyTime.time() - 10, ["ABC\n"])
 		ticket.setAttempt(40)
 		self.db.addBan(self.jail, newTicket)
 		# Added ticket, so cache should have been cleared
@@ -228,6 +236,22 @@ class DatabaseTest(unittest.TestCase):
 			id(ticket),
 			id(self.db.getBansMerged("127.0.0.1", jail=self.jail)))
 
+		tickets = self.db.getBansMerged()
+		self.assertEqual(len(tickets), 2)
+		self.assertEqual(
+			sorted(list(set(ticket.getIP() for ticket in tickets))),
+			sorted([ticket.getIP() for ticket in tickets]))
+
+		tickets = self.db.getBansMerged(jail=jail2)
+		self.assertEqual(len(tickets), 1)
+
+		tickets = self.db.getBansMerged(bantime=25)
+		self.assertEqual(len(tickets), 2)
+		tickets = self.db.getBansMerged(bantime=15)
+		self.assertEqual(len(tickets), 1)
+		tickets = self.db.getBansMerged(bantime=5)
+		self.assertEqual(len(tickets), 0)
+
 	def testPurge(self):
 		if Fail2BanDb is None: # pragma: no cover
 			return