more cases covered, start in repair distinguish operations, on demand flag etc

fail2ban/server/action.py

@@ -484,7 +484,7 @@ class CommandAction(ActionBase):
 		"""
 		return self._start()
 
-	def _start(self, family='', forceStart=False):
+	def _start(self, family=None, forceStart=False):
 		"""Executes the "actionstart" command.
 
 		Replace the tags in the action command with actions properties
@@ -496,7 +496,7 @@ class CommandAction(ActionBase):
 				return True
 		elif not forceStart and self.__started.get(family): # pragma: no cover - normally unreachable
 			return True
-		family = [family] if family != '' else self._families
+		family = [family] if family is not None else self._families
 		def _started(family, ret):
 			if ret:
 				self._operationExecuted('<actionstop>', family, None)
@@ -570,14 +570,14 @@ class CommandAction(ActionBase):
 		"""
 		return self._stop()
 
-	def _stop(self, family=''):
+	def _stop(self, family=None):
 		"""Executes the "actionstop" command.
 
 		Replaces the tags in the action command with actions properties
 		and executes the resulting command.
 		"""
 		# collect started families, if started on demand (conditional):
-		if not family:
+		if family is None:
 			family = [f for (f,v) in self.__started.iteritems() if v]
 			# if no started (on demand) actions:
 			if not family: return True
@@ -812,26 +812,27 @@ class CommandAction(ActionBase):
 			realCmd = Utils.buildShellCmd(realCmd, varsDict)
 		return realCmd
 
-	def _invariantCheck(self, family='', beforeRepair=None):
+	def _invariantCheck(self, family=None, beforeRepair=None, forceStart=True):
 		"""Executes a substituted `actioncheck` command.
 		"""
 		# for started action/family only (avoid check not started inet4 if inet6 gets broken):
-		if family != '' and family not in self.__started and not self.__started.get(''):
-			return True
+		if not forceStart and family is not None and family not in self.__started:
+			return 1
 		checkCmd = self._getOperation('<actioncheck>', family)
 		if not checkCmd or self.executeCmd(checkCmd, self.timeout):
-			return True
+			return 1
 		# if don't need repair/restore - just return:
 		if beforeRepair and not beforeRepair():
-			return False
+			return -1
 		self._logSys.error(
 			"Invariant check failed. Trying to restore a sane environment")
 		# try to find repair command, if exists - exec it:
 		repairCmd = self._getOperation('<actionrepair>', family)
 		if repairCmd:
 			if not self.executeCmd(repairCmd, self.timeout):
+				self.__started[family] = 0
 				self._logSys.critical("Unable to restore environment")
-				return False
+				return 0
 			self.__started[family] = 1
 		else:
 			# no repair command, try to restart action...
@@ -843,11 +844,11 @@ class CommandAction(ActionBase):
 				self._stop(family)
 			except RuntimeError: # bypass error in stop (if start/check succeeded hereafter).
 				pass
-			self._start(family)
-		if not self.executeCmd(checkCmd, self.timeout):
+			self._start(family, forceStart=forceStart or not self._startOnDemand)
+		if self.__started.get(family) and not self.executeCmd(checkCmd, self.timeout):
 			self._logSys.critical("Unable to restore environment")
-			return False
-		return True
+			return 0
+		return 1
 
 	def _processCmd(self, cmd, aInfo=None):
 		"""Executes a command with preliminary checks and substitutions.
@@ -876,7 +877,7 @@ class CommandAction(ActionBase):
 		# conditional corresponding family of the given ip:
 		try:
 			family = aInfo["family"]
-		except KeyError:
+		except (KeyError, TypeError):
 			family = ''
 
 		# invariant check:
@@ -887,8 +888,10 @@ class CommandAction(ActionBase):
 					self._logSys.error("Invariant check failed. Unban is impossible.")
 					return False
 				return True
-			ret = self._invariantCheck(family, _beforeRepair)
-			if not ret:
+			# check and repair if broken:
+			ret = self._invariantCheck(family, _beforeRepair, forceStart=(cmd != '<actionunban>'))
+			# if not sane (and not restored) return:
+			if ret != 1:
 				return False
 
 		# Replace static fields

fail2ban/tests/actionstestcase.py

@@ -228,8 +228,12 @@ class ExecuteActions(LogCaptureTestCase):
 			all=True, wait=True)
 
 		# check should fail (so cause stop/start):
-		self.pruneLog('[test-phase 1] simulate inconsistent env')
+		self.pruneLog('[test-phase 1a] simulate inconsistent irreparable env by unban')
 		act['actioncheck?family=inet6'] = act.actioncheck + '; exit 1'
+		self.__actions.removeBannedIP('2001:db8::1')
+		self.assertLogged('Invariant check failed. Unban is impossible.',
+			wait=True)
+		self.pruneLog('[test-phase 1b] simulate inconsistent irreparable env by flush')
 		self.__actions._Actions__flushBan()
 		self.assertLogged(
 			"stdout: %r" % 'ip flush inet4',
@@ -238,7 +242,7 @@ class ExecuteActions(LogCaptureTestCase):
 			'No flush occured, do consistency check',
 			'Invariant check failed. Trying to restore a sane environment',
 			"stdout: %r" % 'ip stop',  # same for both families
-			'Unable to restore environment',
+			'Failed to flush bans',
 			all=True, wait=True)
 
 		# check succeeds:
@@ -283,11 +287,12 @@ class ExecuteActions(LogCaptureTestCase):
 			all=True)
 
 	def testActionsConsistencyCheckDiffFam(self):
-		# same as testActionsConsistencyCheck, but different start/stop commands for both families
+		# same as testActionsConsistencyCheck, but different start/stop commands for both families and repair on unban
 		act = self.defaultAction({'start':' <family>', 'check':' <family>', 'flush':' <family>', 'stop':' <family>'})
 		# flush for inet6 is intentionally "broken" here - test no unhandled except and invariant check:
 		act['actionflush?family=inet6'] = act.actionflush + '; exit 1'
 		act.actionstart_on_demand = True
+		act.actionrepair_on_unban = True
 		self.__actions.start()
 		self.assertNotLogged("stdout: %r" % 'ip start')
 
@@ -301,8 +306,36 @@ class ExecuteActions(LogCaptureTestCase):
 			all=True, wait=True)
 
 		# check should fail (so cause stop/start):
-		self.pruneLog('[test-phase 1] simulate inconsistent env')
 		act['actioncheck?family=inet6'] = act.actioncheck + '; exit 1'
+		self.pruneLog('[test-phase 1a] simulate inconsistent irreparable env by unban')
+		self.__actions.removeBannedIP('2001:db8::1')
+		self.assertLogged('Invariant check failed. Trying to restore a sane environment',
+			"stdout: %r" % 'ip stop inet6',
+			all=True, wait=True)
+		self.assertNotLogged(
+			"stdout: %r" % 'ip start inet6', # start on demand (not on repair)
+			"stdout: %r" % 'ip stop inet4',  # family inet4 is not affected
+			"stdout: %r" % 'ip start inet4',
+			all=True)
+
+		self.pruneLog('[test-phase 1b] simulate inconsistent irreparable env by ban')
+		self.assertEqual(self.__actions.addBannedIP('2001:db8::1'), 1)
+		self.assertLogged('Invariant check failed. Trying to restore a sane environment',
+			"stdout: %r" % 'ip stop inet6',
+			"stdout: %r" % 'ip start inet6',
+			"stdout: %r" % 'ip check inet6',
+			'Unable to restore environment',
+			'Failed to execute ban',
+			all=True, wait=True)
+		self.assertNotLogged(
+			"stdout: %r" % 'ip stop inet4',  # family inet4 is not affected
+			"stdout: %r" % 'ip start inet4',
+			all=True)
+
+		act['actioncheck?family=inet6'] = act.actioncheck
+		self.assertEqual(self.__actions.addBannedIP('2001:db8::2'), 1)
+		act['actioncheck?family=inet6'] = act.actioncheck + '; exit 1'
+		self.pruneLog('[test-phase 1c] simulate inconsistent irreparable env by flush')
 		self.__actions._Actions__flushBan()
 		self.assertLogged(
 			"stdout: %r" % 'ip flush inet4',
@@ -311,7 +344,7 @@ class ExecuteActions(LogCaptureTestCase):
 			'No flush occured, do consistency check',
 			'Invariant check failed. Trying to restore a sane environment',
 			"stdout: %r" % 'ip stop inet6',
-			'Unable to restore environment',
+			'Failed to flush bans in jail',
 			all=True, wait=True)
 		# start/stop should be called for inet6 only:
 		self.assertNotLogged(