From ed4b5b8bdc8914e9d8dc68ca889f7c05ea194986 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 15 Jan 2020 13:06:21 +0100 Subject: [PATCH 1/2] change actioncheck behavior (on error only, gh-488) --- fail2ban/server/action.py | 55 +++++++++++++++++-------------- fail2ban/tests/actionstestcase.py | 6 ++++ fail2ban/tests/actiontestcase.py | 46 ++++++++++++++++++++++++-- fail2ban/tests/servertestcase.py | 21 +++++++++--- 4 files changed, 97 insertions(+), 31 deletions(-) diff --git a/fail2ban/server/action.py b/fail2ban/server/action.py index 5c817fc05..4df7217e1 100644 --- a/fail2ban/server/action.py +++ b/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,31 +962,38 @@ class CommandAction(ActionBase): except (KeyError, TypeError): family = '' - # invariant check: - if self.actioncheck: - # don't repair/restore if unban (no matter): - def _beforeRepair(): - if cmd == '' and not self._properties.get('actionrepair_on_unban'): - self._logSys.error("Invariant check failed. Unban is impossible.") - return False - return True - # check and repair if broken: - ret = self._invariantCheck(family, _beforeRepair, forceStart=(cmd != '')) - # if not sane (and not restored) return: - if ret != 1: - return False - - # Replace static fields - realCmd = self.replaceTag(cmd, self._properties, - conditional=('family='+family if family else ''), cache=self.__substCache) + repcnt = 0 + while True: - # Replace dynamical tags, important - don't cache, no recursion and auto-escape here - if aInfo is not None: - realCmd = self.replaceDynamicTags(realCmd, aInfo) - else: - realCmd = cmd + # got some error, do invariant check: + if repcnt and self.actioncheck: + # don't repair/restore if unban (no matter): + def _beforeRepair(): + if cmd == '' and not self._properties.get('actionrepair_on_unban'): + self._logSys.error("Invariant check failed. Unban is impossible.") + return False + return True + # check and repair if broken: + ret = self._invariantCheck(family, _beforeRepair, forceStart=(cmd != '')) + # if not sane (and not restored) return: + if ret != 1: + return False - return self.executeCmd(realCmd, self.timeout) + # Replace static fields + realCmd = self.replaceTag(cmd, self._properties, + 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: + realCmd = self.replaceDynamicTags(realCmd, aInfo) + else: + realCmd = cmd + + # 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): diff --git a/fail2ban/tests/actionstestcase.py b/fail2ban/tests/actionstestcase.py index a3c14a4b6..b324b2c4f 100644 --- a/fail2ban/tests/actionstestcase.py +++ b/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 = " ; " + act.actionban + act.actionunban = " ; " + 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 = " ; " + act.actionban + act.actionunban = " ; " + act.actionunban self.__actions.start() self.assertNotLogged("stdout: %r" % 'ip start') diff --git a/fail2ban/tests/actiontestcase.py b/fail2ban/tests/actiontestcase.py index 1a00c0406..2dab3ab11 100644 --- a/fail2ban/tests/actiontestcase.py +++ b/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.assertEqual(self.__action.actionban, 'echo -n') + self.__action.actionban = " && echo -n" + self.assertEqual(self.__action.actionban, " && 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" >> '%s'""" % tmp + self.__action.actionban = """ && printf "%%%%b\n" >> '%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 '" % 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' diff --git a/fail2ban/tests/servertestcase.py b/fail2ban/tests/servertestcase.py index 55e724556..71347c7e6 100644 --- a/fail2ban/tests/servertestcase.py +++ b/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 ===') From 8cbc1e0ebb91142a34756fd2f4416b08d773deaa Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 16 Jan 2020 16:51:27 +0100 Subject: [PATCH 2/2] ChangeLog (change actioncheck behavior) --- ChangeLog | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ChangeLog b/ChangeLog index 3f9c11b1a..73e0e0475 100644 --- a/ChangeLog +++ b/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