avoid unhandled exception during flush, better invariant check (and repair), avoid repair by unban/stop etc...

config/jail.conf

@@ -861,7 +861,8 @@ filter       = apache-pass[knocking_url="%(knocking_url)s"]
 logpath      = %(apache_access_log)s
 blocktype    = RETURN
 returntype   = DROP
-action       = %(action_)s[blocktype=%(blocktype)s, returntype=%(returntype)s]
+action       = %(action_)s[blocktype=%(blocktype)s, returntype=%(returntype)s,
+                        actionstart_on_demand=True, actionrepair_on_unban=True]
 bantime      = 1h
 maxretry     = 1
 findtime     = 1

fail2ban/client/actionreader.py

@@ -44,6 +44,7 @@ class ActionReader(DefinitionInitConfigReader):
 		"actionreload": ["string", None],
 		"actioncheck": ["string", None],
 		"actionrepair": ["string", None],
+		"actionrepair_on_unban": ["string", None],
 		"actionban": ["string", None],
 		"actionunban": ["string", None],
 		"norestored": ["string", None],
@@ -78,7 +79,7 @@ class ActionReader(DefinitionInitConfigReader):
 		opts = self.getCombined(
 			ignore=CommandAction._escapedTags | set(('timeout', 'bantime')))
 		# type-convert only after combined (otherwise boolean converting prevents substitution):
-		for o in ('norestored', 'actionstart_on_demand'):
+		for o in ('norestored', 'actionstart_on_demand', 'actionrepair_on_unban'):
 			if opts.get(o):
 				opts[o] = self._convert_to_boolean(opts[o])
 		

fail2ban/client/jailreader.py

@@ -33,7 +33,7 @@ from .configreader import ConfigReaderUnshared, ConfigReader
 from .filterreader import FilterReader
 from .actionreader import ActionReader
 from ..version import version
-from ..helpers import getLogger, extractOptions, splitwords
+from ..helpers import getLogger, extractOptions, splitWithOptions, splitwords
 
 # Gets the instance of the logger.
 logSys = getLogger(__name__)
@@ -164,21 +164,15 @@ class JailReader(ConfigReader):
 				self.__filter.getOptions(self.__opts)
 		
 			# Read action
-			prevln = ''
+			for act in splitWithOptions(self.__opts["action"]):
-			actlst = self.__opts["action"].split('\n')
-			for n, act in enumerate(actlst):
 				try:
+					act = act.strip()
 					if not act:			  # skip empty actions
 						continue
 					# join with previous line if needed (consider possible new-line):
-					if prevln: act = prevln + '\n' + act
 					actName, actOpt = extractOptions(act)
 					prevln = ''
 					if not actName:
-						# consider possible new-line, so repeat with joined next line's:
-						if n < len(actlst) - 1:
-							prevln = act
-							continue
 						raise JailDefError("Invalid action definition %r" % act)
 					if actName.endswith(".py"):
 						self.__actions.append([

fail2ban/helpers.py

@@ -336,6 +336,9 @@ OPTION_CRE = re.compile(r"^([^\[]+)(?:\[(.*)\])?\s*$", re.DOTALL)
 # `action = act[p1=...][p2=...]`
 OPTION_EXTRACT_CRE = re.compile(
 	r'([\w\-_\.]+)=(?:"([^"]*)"|\'([^\']*)\'|([^,\]]*))(?:,|\]\s*\[|$)', re.DOTALL)
+# split by new-line considering possible new-lines within options [...]:
+OPTION_SPLIT_CRE = re.compile(
+	r'(?:[^\[\n]+(?:\s*\[\s*(?:[\w\-_\.]+=(?:"[^"]*"|\'[^\']*\'|[^,\]]*)\s*(?:,|\]\s*\[)?\s*)*\])?\s*|[^\n]+)(?=\n\s*|$)', re.DOTALL)
 
 def extractOptions(option):
 	match = OPTION_CRE.match(option)
@@ -352,6 +355,9 @@ def extractOptions(option):
 			option_opts[opt.strip()] = value.strip()
 	return option_name, option_opts
 
+def splitWithOptions(option):
+	return OPTION_SPLIT_CRE.findall(option)
+
 #
 # Following facilities used for safe recursive interpolation of
 # tags (<tag>) in tagged options.
@@ -386,8 +392,7 @@ def substituteRecursiveTags(inptags, conditional='',
 	"""
 	#logSys = getLogger("fail2ban")
 	tre_search = TAG_CRE.search
-	# copy return tags dict to prevent modifying of inptags:
+	tags = inptags
-	tags = inptags.copy()
 	# init:
 	ignore = set(ignore)
 	done = set()
@@ -449,6 +454,9 @@ def substituteRecursiveTags(inptags, conditional='',
 				# check still contains any tag - should be repeated (possible embedded-recursive substitution):
 				if tre_search(value):
 					repFlag = True
+				# copy return tags dict to prevent modifying of inptags:
+				if id(tags) == id(inptags):
+					tags = inptags.copy()
 				tags[tag] = value
 			# no more sub tags (and no possible composite), add this tag to done set (just to be faster):
 			if '<' not in value: done.add(tag)

fail2ban/server/action.py

@@ -50,6 +50,7 @@ allowed_ipv6 = True
 # capture groups from filter for map to ticket data:
 FCUSTAG_CRE = re.compile(r'<F-([A-Z0-9_\-]+)>'); # currently uppercase only
 
+COND_FAMILIES = ('inet4', 'inet6')
 CONDITIONAL_FAM_RE = re.compile(r"^(\w+)\?(family)=")
 
 # Special tags:
@@ -363,8 +364,8 @@ class CommandAction(ActionBase):
 		self.__properties = dict(
 			(key, getattr(self, key))
 			for key in dir(self)
-			if not key.startswith("_") and not callable(getattr(self, key)))
+			if not key.startswith("_") and not callable(getattr(self, key))
-		#
+		)
 		return self.__properties
 
 	@property
@@ -372,8 +373,13 @@ class CommandAction(ActionBase):
 		return self.__substCache
 
 	def _getOperation(self, tag, family):
+		# be sure family is enclosed as conditional value (if not overwritten in action):
+		if family and self._hasCondSection:
+			if 'family' not in self._properties and 'family?family='+family not in self._properties:
+				self._properties['family?family='+family] = family
+		# replace operation tag (interpolate all values):
 		return self.replaceTag(tag, self._properties,
-			conditional=('family=' + family), cache=self.__substCache)
+			conditional=('family='+family if family else ''), cache=self.__substCache)
 
 	def _executeOperation(self, tag, operation, family=[]):
 		"""Executes the operation commands (like "actionstart", "actionstop", etc).
@@ -400,7 +406,17 @@ class CommandAction(ActionBase):
 			raise RuntimeError("Error %s action %s/%s: %r" % (operation, self._jail, self._name, e))
 		return res
 
-	COND_FAMILIES = ('inet4', 'inet6')
+	@property
+	def _hasCondSection(self):
+		v = self._properties.get('__hasCondSection')
+		if v is None:
+			v = False
+			for n in self._properties:
+				if CONDITIONAL_FAM_RE.match(n):
+					v = True
+					break
+			self._properties['__hasCondSection'] = v
+		return v
 
 	@property
 	def _startOnDemand(self):
@@ -409,11 +425,7 @@ class CommandAction(ActionBase):
 		if v is not None:
 			return v
 		# not set - auto-recognize (depending on conditional):
-		v = False
+		v = self._hasCondSection
-		for n in self._properties:
-			if CONDITIONAL_FAM_RE.match(n):
-				v = True
-				break
 		self._properties['actionstart_on_demand'] = v
 		return v
 
@@ -514,6 +526,17 @@ class CommandAction(ActionBase):
 		"""
 		return self._executeOperation('<actionreload>', 'reloading')
 
+	def consistencyCheck(self, beforeRepair=None):
+		"""Executes the invariant check with repair if expected (conditional).
+		"""
+		ret = True
+		# for each started family:
+		if self.actioncheck:
+			for (family, started) in self.__started.iteritems():
+				if started and not self._invariantCheck(family, beforeRepair):
+					ret &= False
+		return ret
+
 	@staticmethod
 	def escapeTag(value):
 		"""Escape characters which may be used for command injection.
@@ -705,7 +728,40 @@ class CommandAction(ActionBase):
 			realCmd = Utils.buildShellCmd(realCmd, varsDict)
 		return realCmd
 
-	def _processCmd(self, cmd, aInfo=None, conditional=''):
+	def _invariantCheck(self, family='', beforeRepair=None):
+		"""Executes a substituted `actioncheck` command.
+		"""
+		checkCmd = self._getOperation('<actioncheck>', family)
+		if not checkCmd or self.executeCmd(checkCmd, self.timeout):
+			return True
+		# if don't need repair/restore - just return:
+		if beforeRepair and not beforeRepair():
+			return False
+		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._logSys.critical("Unable to restore environment")
+				return False
+		else:
+			# no repair command, try to restart action...
+			# [WARNING] TODO: be sure all banactions get a repair command, because
+			#    otherwise stop/start will theoretically remove all the bans,
+			#    but the tickets are still in BanManager, so in case of new failures
+			#    it will not be banned, because "already banned" will happen.
+			try:
+				self.stop()
+			except RuntimeError: # bypass error in stop (if start/check succeeded hereafter).
+				pass
+			self.start()
+		if not self.executeCmd(checkCmd, self.timeout):
+			self._logSys.critical("Unable to restore environment")
+			return False
+		return True
+
+	def _processCmd(self, cmd, aInfo=None):
 		"""Executes a command with preliminary checks and substitutions.
 
 		Before executing any commands, executes the "check" command first
@@ -730,47 +786,26 @@ class CommandAction(ActionBase):
 			return True
 
 		# conditional corresponding family of the given ip:
-		if conditional == '':
+		try:
-			conditional = 'family=inet4'
+			family = aInfo["family"]
-			if allowed_ipv6:
+		except KeyError:
-				try:
+			family = ''
-					ip = aInfo["ip"]
-					if ip and asip(ip).isIPv6:
-						conditional = 'family=inet6'
-				except KeyError:
-					pass
 
-		checkCmd = self.replaceTag('<actioncheck>', self._properties, 
+		# invariant check:
-			conditional=conditional, cache=self.__substCache)
+		if self.actioncheck:
-		if checkCmd:
+			# don't repair/restore if unban (no matter):
-			if not self.executeCmd(checkCmd, self.timeout):
+			def _beforeRepair():
-				self._logSys.error(
+				if cmd == '<actionunban>' and not self._properties.get('actionrepair_on_unban'):
-					"Invariant check failed. Trying to restore a sane environment")
+					self._logSys.error("Invariant check failed. Unban is impossible.")
-				# try to find repair command, if exists - exec it:
-				repairCmd = self.replaceTag('<actionrepair>', self._properties, 
-					conditional=conditional, cache=self.__substCache)
-				if repairCmd:
-					if not self.executeCmd(repairCmd, self.timeout):
-						self._logSys.critical("Unable to restore environment")
-						return False
-				else:
-					# no repair command, try to restart action...
-					# [WARNING] TODO: be sure all banactions get a repair command, because
-					#    otherwise stop/start will theoretically remove all the bans,
-					#    but the tickets are still in BanManager, so in case of new failures
-					#    it will not be banned, because "already banned" will happen.
-					try:
-						self.stop()
-					except RuntimeError: # bypass error in stop (if start/check succeeded hereafter).
-						pass
-					self.start()
-				if not self.executeCmd(checkCmd, self.timeout):
-					self._logSys.critical("Unable to restore environment")
 					return False
+				return True
+			ret = self._invariantCheck(family, _beforeRepair)
+			if not ret:
+				return False
 
 		# Replace static fields
 		realCmd = self.replaceTag(cmd, self._properties, 
-			conditional=conditional, cache=self.__substCache)
+			conditional=('family='+family if family else ''), cache=self.__substCache)
 
 		# Replace dynamical tags, important - don't cache, no recursion and auto-escape here
 		if aInfo is not None:

fail2ban/server/actions.py

@@ -160,8 +160,8 @@ class Actions(JailThread, Mapping):
 				delacts = OrderedDict((name, action) for name, action in self._actions.iteritems()
 					if name not in self._reload_actions)
 				if len(delacts):
-					# unban all tickets using remove action only:
+					# unban all tickets using removed actions only:
-					self.__flushBan(db=False, actions=delacts)
+					self.__flushBan(db=False, actions=delacts, stop=True)
 					# stop and remove it:
 					self.stopActions(actions=delacts)
 				delattr(self, '_reload_actions')
@@ -326,7 +326,7 @@ class Actions(JailThread, Mapping):
 					self.__checkUnBan(bancnt if bancnt and bancnt < self.unbanMaxCount else self.unbanMaxCount)
 				cnt = 0
 		
-		self.__flushBan()
+		self.__flushBan(stop=True)
 		self.stopActions()
 		return True
 
@@ -494,7 +494,7 @@ class Actions(JailThread, Mapping):
 				cnt, self.__banManager.size(), self._jail.name)
 		return cnt
 
-	def __flushBan(self, db=False, actions=None):
+	def __flushBan(self, db=False, actions=None, stop=False):
 		"""Flush the ban list.
 
 		Unban all IP address which are still in the banning list.
@@ -513,11 +513,26 @@ class Actions(JailThread, Mapping):
 		# first we'll execute flush for actions supporting this operation:
 		unbactions = {}
 		for name, action in (actions if actions is not None else self._actions).iteritems():
-			if hasattr(action, 'flush') and action.actionflush:
+			try:
-				logSys.notice("[%s] Flush ticket(s) with %s", self._jail.name, name)
+				if hasattr(action, 'flush') and (not isinstance(action, CommandAction) or action.actionflush):
-				action.flush()
+					logSys.notice("[%s] Flush ticket(s) with %s", self._jail.name, name)
-			else:
+					action.flush()
-				unbactions[name] = action
+				else:
+					unbactions[name] = action
+			except Exception as e:
+				logSys.error("Failed to flush bans in jail '%s' action '%s': %s",
+					self._jail.name, name, e,
+					exc_info=logSys.getEffectiveLevel()<=logging.DEBUG)
+				logSys.info("No flush occured, do consistency check")
+				def _beforeRepair():
+					if stop:
+						self._logSys.error("Invariant check failed. Flush is impossible.")
+						return False
+					return True
+				if not hasattr(action, 'consistencyCheck') or action.consistencyCheck(_beforeRepair):
+					# fallback to single unbans:
+					logSys.info("unban tickets each individualy")
+					unbactions[name] = action
 		actions = unbactions
 		# flush the database also:
 		if db and self._jail.database is not None:

fail2ban/tests/actionstestcase.py

@@ -44,20 +44,20 @@ class ExecuteActions(LogCaptureTestCase):
 		super(ExecuteActions, self).setUp()
 		self.__jail = DummyJail()
 		self.__actions = Actions(self.__jail)
-		self.__tmpfile, self.__tmpfilename  = tempfile.mkstemp()
 
 	def tearDown(self):
 		super(ExecuteActions, self).tearDown()
-		os.remove(self.__tmpfilename)
 
-	def defaultActions(self):
+	def defaultAction(self):
 		self.__actions.add('ip')
-		self.__ip = self.__actions['ip']
+		act = self.__actions['ip']
-		self.__ip.actionstart = 'echo ip start 64 >> "%s"' % self.__tmpfilename
+		act.actionstart = 'echo ip start'
-		self.__ip.actionban = 'echo ip ban <ip> >> "%s"' % self.__tmpfilename
+		act.actionban = 'echo ip ban <ip>'
-		self.__ip.actionunban = 'echo ip unban <ip> >> "%s"' % self.__tmpfilename
+		act.actionunban = 'echo ip unban <ip>'
-		self.__ip.actioncheck = 'echo ip check <ip> >> "%s"' % self.__tmpfilename
+		act.actioncheck = 'echo ip check'
-		self.__ip.actionstop = 'echo ip stop >> "%s"' % self.__tmpfilename
+		act.actionflush = 'echo ip flush <family>'
+		act.actionstop = 'echo ip stop'
+		return act
 
 	def testActionsAddDuplicateName(self):
 		self.__actions.add('test')
@@ -89,13 +89,12 @@ class ExecuteActions(LogCaptureTestCase):
 		self.assertLogged('Ban 192.0.2.3')
 
 	def testActionsOutput(self):
-		self.defaultActions()
+		self.defaultAction()
 		self.__actions.start()
-		with open(self.__tmpfilename) as f:
+		self.assertLogged("stdout: %r" % 'ip start', wait=True)
-			self.assertTrue( Utils.wait_for(lambda: (f.read() == "ip start 64\n"), 3) )
-
 		self.__actions.stop()
 		self.__actions.join()
+		self.assertLogged("stdout: %r" % 'ip flush', "stdout: %r" % 'ip stop')
 		self.assertEqual(self.__actions.status(),[("Currently banned", 0 ),
                ("Total banned", 0 ), ("Banned IP list", [] )])
 
@@ -211,3 +210,30 @@ class ExecuteActions(LogCaptureTestCase):
 
 		self.assertLogged('Unbanned 30, 0 ticket(s)')
 		self.assertNotLogged('Unbanned 50, 0 ticket(s)')
+
+	@with_alt_time
+	def testActionsConsistencyCheck(self):
+		# flush is broken - test no unhandled except and invariant check:
+		act = self.defaultAction()
+		setattr(act, 'actionflush?family=inet6', 'echo ip flush <family>; exit 1')
+		act.actionstart_on_demand = True
+		self.__actions.start()
+		self.assertNotLogged("stdout: %r" % 'ip start')
+
+		self.assertEqual(self.__actions.addBannedIP('192.0.2.1'), 1)
+		self.assertEqual(self.__actions.addBannedIP('2001:db8::1'), 1)
+		self.assertLogged('Ban 192.0.2.1', 'Ban 2001:db8::1',
+			"stdout: %r" % 'ip start',
+			"stdout: %r" % 'ip ban 192.0.2.1',
+			"stdout: %r" % 'ip ban 2001:db8::1',
+			all=True, wait=True)
+
+		self.__actions._Actions__flushBan()
+		self.assertLogged('Failed to flush bans',
+			'No flush occured, do consistency check',
+			"stdout: %r" % 'ip ban 192.0.2.1',
+			all=True, wait=True)
+
+		self.__actions.stop()
+		self.__actions.join()
+

fail2ban/tests/clientreadertestcase.py

@@ -31,7 +31,7 @@ import unittest
 from ..client.configreader import ConfigReader, ConfigReaderUnshared, \
 	DefinitionInitConfigReader, NoSectionError
 from ..client import configparserinc
-from ..client.jailreader import JailReader, extractOptions
+from ..client.jailreader import JailReader, extractOptions, splitWithOptions
 from ..client.filterreader import FilterReader
 from ..client.jailsreader import JailsReader
 from ..client.actionreader import ActionReader, CommandAction
@@ -778,7 +778,7 @@ class JailsReaderTest(LogCaptureTestCase):
 
 			# somewhat duplicating here what is done in JailsReader if
 			# the jail is enabled
-			for act in actions.split('\n'):
+			for act in splitWithOptions(actions):
 				actName, actOpt = extractOptions(act)
 				self.assertTrue(len(actName))
 				self.assertTrue(isinstance(actOpt, dict))

fail2ban/tests/files/action.d/action_modifyainfo.py

@@ -12,4 +12,9 @@ class TestAction(ActionBase):
         del aInfo['ip']
         self._logSys.info("%s unban deleted aInfo IP", self._name)
 
+    def flush(self):
+        # intended error to cover no unhandled exception occurs in flash
+        # as well as unbans are done individually after errored flush.
+        raise ValueError("intended error")
+
 Action = TestAction