Merge branch 'gh-488-check-on-error'

ChangeLog

@@ -9,11 +9,20 @@ Fail2Ban: Changelog
 ver. 1.0.1-dev-1 (20??/??/??) - development nightly edition
 -----------
 
+### Compatibility:
+* to v.0.11:
+  - due to change of `actioncheck` behavior (gh-488), some actions can be incompatible as regards
+    the invariant check, if `actionban` or `actionunban` would not throw an error (exit code 
+    different from 0) in case of unsane environment.
+
 ### Fixes
 
 ### New Features
 
 ### Enhancements
+* `actioncheck` behavior is changed now (gh-488), so invariant check as well as restore or repair
+   of sane environment (in case of recognized unsane state) would only occur on action errors (e. g.
+   if ban or unban operations are exiting with other code as 0)
 
 
 ver. 0.11.1 (2020/01/11) - this-is-the-way

fail2ban/server/action.py

@@ -407,7 +407,7 @@ class CommandAction(ActionBase):
 		cmd = self.replaceTag(tag, self._properties,
 			conditional=('family='+family if family else ''),
 			cache=self.__substCache)
-		if '<' not in cmd or not family: return cmd
+		if not family or '<' not in cmd: return cmd
 		# replace family as dynamic tags, important - don't cache, no recursion and auto-escape here:
 		cmd = self.replaceDynamicTags(cmd, {'family':family})
 		return cmd
@@ -962,8 +962,11 @@ class CommandAction(ActionBase):
 		except (KeyError, TypeError):
 			family = ''
 
-		# invariant check:
+		repcnt = 0
-		if self.actioncheck:
+		while True:
+
+			# got some error, do invariant check:
+			if repcnt and self.actioncheck:
 				# don't repair/restore if unban (no matter):
 				def _beforeRepair():
 					if cmd == '<actionunban>' and not self._properties.get('actionrepair_on_unban'):
@@ -986,7 +989,11 @@ class CommandAction(ActionBase):
 			else:
 				realCmd = cmd
 
-		return self.executeCmd(realCmd, self.timeout)
+			# try execute command:
+			ret = self.executeCmd(realCmd, self.timeout)
+			repcnt += 1
+			if ret or repcnt > 1:
+				return ret
 
 	@staticmethod
 	def executeCmd(realCmd, timeout=60, **kwargs):

fail2ban/tests/actionstestcase.py

@@ -215,6 +215,9 @@ class ExecuteActions(LogCaptureTestCase):
 		# 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
+		# force errors via check in ban/unban:
+		act.actionban = "<actioncheck> ; " + act.actionban
+		act.actionunban = "<actioncheck> ; " + act.actionunban
 		self.__actions.start()
 		self.assertNotLogged("stdout: %r" % 'ip start')
 
@@ -292,6 +295,9 @@ class ExecuteActions(LogCaptureTestCase):
 		act['actionflush?family=inet6'] = act.actionflush + '; exit 1'
 		act.actionstart_on_demand = True
 		act.actionrepair_on_unban = True
+		# force errors via check in ban/unban:
+		act.actionban = "<actioncheck> ; " + act.actionban
+		act.actionunban = "<actioncheck> ; " + act.actionunban
 		self.__actions.start()
 		self.assertNotLogged("stdout: %r" % 'ip start')
 

fail2ban/tests/actiontestcase.py

@@ -305,8 +305,8 @@ class CommandActionTest(LogCaptureTestCase):
 		self.assertEqual(self.__action.actionstart, "touch '%s'" % tmp)
 		self.__action.actionstop = "rm -f '%s'" % tmp
 		self.assertEqual(self.__action.actionstop, "rm -f '%s'" % tmp)
-		self.__action.actionban = "echo -n"
+		self.__action.actionban = "<actioncheck> && echo -n"
-		self.assertEqual(self.__action.actionban, 'echo -n')
+		self.assertEqual(self.__action.actionban, "<actioncheck> && echo -n")
 		self.__action.actioncheck = "[ -e '%s' ]" % tmp
 		self.assertEqual(self.__action.actioncheck, "[ -e '%s' ]" % tmp)
 		self.__action.actionunban = "true"
@@ -316,6 +316,7 @@ class CommandActionTest(LogCaptureTestCase):
 		self.assertNotLogged('returned')
 		# no action was actually executed yet
 
+		# start on demand is false, so it should cause failure on first attempt of ban:
 		self.__action.ban({'ip': None})
 		self.assertLogged('Invariant check failed')
 		self.assertLogged('returned successfully')
@@ -365,12 +366,51 @@ class CommandActionTest(LogCaptureTestCase):
 		self.pruneLog('[phase 2]')
 		self.__action.actionstart = "touch '%s'" % tmp
 		self.__action.actionstop = "rm '%s'" % tmp
-		self.__action.actionban = """printf "%%%%b\n" <ip> >> '%s'""" % tmp
+		self.__action.actionban = """<actioncheck> && printf "%%%%b\n" <ip> >> '%s'""" % tmp
 		self.__action.actioncheck = "[ -e '%s' ]" % tmp
 		self.__action.ban({'ip': None})
 		self.assertLogged('Invariant check failed')
 		self.assertNotLogged('Unable to restore environment')
 
+	@with_tmpdir
+	def testExecuteActionCheckOnBanFailure(self, tmp):
+		tmp += '/fail2ban.test'
+		self.__action.actionstart = "touch '%s'; echo 'started ...'" % tmp
+		self.__action.actionstop = "rm -f '%s'" % tmp
+		self.__action.actionban = "[ -e '%s' ] && echo 'banned '<ip>" % tmp
+		self.__action.actioncheck = "[ -e '%s' ] && echo 'check ok' || { echo 'check failed'; exit 1; }" % tmp
+		self.__action.actionrepair = "echo 'repair ...'; touch '%s'" % tmp
+		self.__action.actionstart_on_demand = False
+		self.__action.start()
+		# phase 1: with repair;
+		# phase 2: without repair (start/stop), not on demand;
+		# phase 3: without repair (start/stop), start on demand.
+		for i in (1, 2, 3):
+			self.pruneLog('[phase %s]' % i)
+			# 1st time with success ban:
+			self.__action.ban({'ip': '192.0.2.1'})
+			self.assertLogged(
+				"stdout: %r" % 'banned 192.0.2.1', all=True)
+			self.assertNotLogged("Invariant check failed. Trying", 
+				"stdout: %r" % 'check failed',
+				"stdout: %r" % ('repair ...' if self.__action.actionrepair else 'started ...'),
+				"stdout: %r" % 'check ok', all=True)
+			# force error in ban:
+			os.remove(tmp)
+			self.pruneLog()
+			# 2nd time with fail recognition, success repair, check and ban:
+			self.__action.ban({'ip': '192.0.2.2'})
+			self.assertLogged("Invariant check failed. Trying",
+				"stdout: %r" % 'check failed',
+				"stdout: %r" % ('repair ...' if self.__action.actionrepair else 'started ...'),
+				"stdout: %r" % 'check ok', 
+				"stdout: %r" % 'banned 192.0.2.2', all=True)
+			# repeat without repair (stop/start), herafter enable on demand:
+			if self.__action.actionrepair:
+				self.__action.actionrepair = ""
+			elif not self.__action.actionstart_on_demand:
+				self.__action.actionstart_on_demand = True
+
 	@with_tmpdir
 	def testExecuteActionCheckRepairEnvironment(self, tmp):
 		tmp += '/fail2ban.test'

fail2ban/tests/servertestcase.py

@@ -2030,25 +2030,38 @@ class ServerConfigReaderTests(LogCaptureTestCase):
 				action.ban(aInfos['ipv4'])
 				if tests.get('ip4-start'): self.assertLogged(*tests.get('*-start', ())+tests['ip4-start'], all=True)
 				if tests.get('ip6-start'): self.assertNotLogged(*tests['ip6-start'], all=True)
-				self.assertLogged(*tests.get('ip4-check',())+tests['ip4-ban'], all=True)
+				self.assertLogged(*tests['ip4-ban'], all=True)
 				self.assertNotLogged(*tests['ip6'], all=True)
 				# test unban ip4 :
 				self.pruneLog('# === unban ipv4 ===')
 				action.unban(aInfos['ipv4'])
-				self.assertLogged(*tests.get('ip4-check',())+tests['ip4-unban'], all=True)
+				self.assertLogged(*tests['ip4-unban'], all=True)
 				self.assertNotLogged(*tests['ip6'], all=True)
 				# test ban ip6 :
 				self.pruneLog('# === ban ipv6 ===')
 				action.ban(aInfos['ipv6'])
 				if tests.get('ip6-start'): self.assertLogged(*tests.get('*-start', ())+tests['ip6-start'], all=True)
 				if tests.get('ip4-start'): self.assertNotLogged(*tests['ip4-start'], all=True)
-				self.assertLogged(*tests.get('ip6-check',())+tests['ip6-ban'], all=True)
+				self.assertLogged(*tests['ip6-ban'], all=True)
 				self.assertNotLogged(*tests['ip4'], all=True)
 				# test unban ip6 :
 				self.pruneLog('# === unban ipv6 ===')
 				action.unban(aInfos['ipv6'])
-				self.assertLogged(*tests.get('ip6-check',())+tests['ip6-unban'], all=True)
+				self.assertLogged(*tests['ip6-unban'], all=True)
 				self.assertNotLogged(*tests['ip4'], all=True)
+				# test invariant check (normally on demand in error case only):
+				if tests.get('ip4-check'):
+					self.pruneLog('# === check ipv4 ===')
+					action._invariantCheck(aInfos['ipv4']['family'])
+					self.assertLogged(*tests['ip4-check'], all=True)
+					if tests.get('ip6-check') and tests['ip6-check'] != tests['ip4-check']:
+						self.assertNotLogged(*tests['ip6-check'], all=True)
+				if tests.get('ip6-check'):
+					self.pruneLog('# === check ipv6 ===')
+					action._invariantCheck(aInfos['ipv6']['family'])
+					self.assertLogged(*tests['ip6-check'], all=True)
+					if tests.get('ip4-check') and tests['ip4-check'] != tests['ip6-check']:
+						self.assertNotLogged(*tests['ip4-check'], all=True)
 				# test flush for actions should supported this:
 				if tests.get('flush'):
 					self.pruneLog('# === flush ===')