From 77dc5a334c0e1ba79482a7799fefef27b6df413e Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:19:09 +0100 Subject: [PATCH 1/9] really skips invalid jails (because of theirs wrong configuration) - server starts nevertheless, as long as one jail was successful configured; message about wrong jail configuration logged in client log (stdout, systemd journal etc.) and in server log as error --- fail2ban/client/jailreader.py | 134 ++++++++++++++++++--------------- fail2ban/client/jailsreader.py | 13 +++- fail2ban/server/transmitter.py | 3 + 3 files changed, 86 insertions(+), 64 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 9d01a693..df6c6664 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -118,70 +118,78 @@ class JailReader(ConfigReader): defsec = self._cfg.get_defaults() defsec["fail2ban_version"] = version - # Read first options only needed for merge defaults ('known/...' from filter): - self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) - if not self.__opts: - return False - - if self.isEnabled(): - # Read filter - if self.__opts["filter"]: - filterName, filterOpt = JailReader.extractOptions( - self.__opts["filter"]) - self.__filter = FilterReader( - filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) - ret = self.__filter.read() - # merge options from filter as 'known/...': - self.__filter.getOptions(self.__opts) - ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') - if not ret: - logSys.error("Unable to read the filter") - return False - else: - self.__filter = None - logSys.warning("No filter set for jail %s" % self.__name) + try: - # Read second all options (so variables like %(known/param) can be interpolated): - self.__opts = ConfigReader.getOptions(self, self.__name, opts) + # Read first options only needed for merge defaults ('known/...' from filter): + self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) if not self.__opts: - return False - - # cumulate filter options again (ignore given in jail): - if self.__filter: - self.__filter.getOptions(self.__opts) - - # Read action - for act in self.__opts["action"].split('\n'): - try: - if not act: # skip empty actions - continue - actName, actOpt = JailReader.extractOptions(act) - if actName.endswith(".py"): - self.__actions.append([ - "set", - self.__name, - "addaction", - actOpt.pop("actname", os.path.splitext(actName)[0]), - os.path.join( - self.getBaseDir(), "action.d", actName), - json.dumps(actOpt), - ]) - else: - action = ActionReader( - actName, self.__name, actOpt, - share_config=self.share_config, basedir=self.getBaseDir()) - ret = action.read() - if ret: - action.getOptions(self.__opts) - self.__actions.append(action) + raise ValueError("Init jail options failed") + + if self.isEnabled(): + # Read filter + if self.__opts["filter"]: + filterName, filterOpt = JailReader.extractOptions( + self.__opts["filter"]) + self.__filter = FilterReader( + filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) + ret = self.__filter.read() + # merge options from filter as 'known/...': + self.__filter.getOptions(self.__opts) + ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') + if not ret: + raise ValueError("Unable to read the filter %r" % filterName) + else: + self.__filter = None + logSys.warning("No filter set for jail %s" % self.__name) + + # Read second all options (so variables like %(known/param) can be interpolated): + self.__opts = ConfigReader.getOptions(self, self.__name, opts) + if not self.__opts: + raise ValueError("Read jail options failed") + + # cumulate filter options again (ignore given in jail): + if self.__filter: + self.__filter.getOptions(self.__opts) + + # Read action + for act in self.__opts["action"].split('\n'): + try: + if not act: # skip empty actions + continue + actName, actOpt = JailReader.extractOptions(act) + if actName.endswith(".py"): + self.__actions.append([ + "set", + self.__name, + "addaction", + actOpt.pop("actname", os.path.splitext(actName)[0]), + os.path.join( + self.getBaseDir(), "action.d", actName), + json.dumps(actOpt), + ]) else: - raise AttributeError("Unable to read action") - except Exception as e: - logSys.error("Error in action definition " + act) - logSys.debug("Caught exception: %s" % (e,)) - return False - if not len(self.__actions): - logSys.warning("No actions were defined for %s" % self.__name) + action = ActionReader( + actName, self.__name, actOpt, + share_config=self.share_config, basedir=self.getBaseDir()) + ret = action.read() + if ret: + action.getOptions(self.__opts) + self.__actions.append(action) + else: + raise AttributeError("Unable to read action") + except Exception as e: + logSys.debug("Caught exception: %s" % (e,)) + raise ValueError("Error in action definition %r" % e) + if not len(self.__actions): + logSys.warning("No actions were defined for %s" % self.__name) + + except ValueError as e: + e = str(e) + logSys.error(e) + if not self.__opts: + self.__opts = dict() + self.__opts['config-error'] = e + return False return True def convert(self, allow_no_files=False): @@ -195,6 +203,10 @@ class JailReader(ConfigReader): """ stream = [] + e = self.__opts.get('config-error') + if e: + stream.extend([['config-error', "Jail '%s' skipped, because of wrong configuration: %s" % (self.__name, e)]]) + return stream for opt, value in self.__opts.iteritems(): if opt == "logpath" and \ not self.__opts.get('backend', None).startswith("systemd"): diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 09725ec9..ec7baca7 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -66,7 +66,7 @@ class JailsReader(ConfigReader): sections = [ section ] # Get the options of all jails. - parse_status = True + parse_status = None for sec in sections: if sec == 'INCLUDES': continue @@ -77,11 +77,17 @@ class JailsReader(ConfigReader): ret = jail.getOptions() if ret: if jail.isEnabled(): + # at least one jail was successful: + parse_status = True # We only add enabled jails self.__jails.append(jail) else: logSys.error("Errors in jail %r. Skipping..." % sec) - parse_status = False + self.__jails.append(jail) + if parse_status is None: + parse_status = False + if parse_status is None: + parse_status = True return parse_status def convert(self, allow_no_files=False): @@ -103,7 +109,8 @@ class JailsReader(ConfigReader): stream.extend(jail.convert(allow_no_files=allow_no_files)) # Start jails for jail in self.__jails: - stream.append(["start", jail.getName()]) + if not jail.options.get('config-error'): + stream.append(["start", jail.getName()]) return stream diff --git a/fail2ban/server/transmitter.py b/fail2ban/server/transmitter.py index 2f5be043..ae1017b9 100644 --- a/fail2ban/server/transmitter.py +++ b/fail2ban/server/transmitter.py @@ -131,6 +131,9 @@ class Transmitter: return self.status(command[1:]) elif command[0] == "version": return version.version + elif command[0] == "config-error": + logSys.error(command[1]) + return None raise Exception("Invalid command") def __commandSet(self, command, multiple=False): From c6e8c700f7f712d9b5c93c125bdcefbef670c7ad Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:57:06 +0100 Subject: [PATCH 2/9] test cases fixed --- fail2ban/client/jailreader.py | 14 +++++++++----- fail2ban/tests/clientreadertestcase.py | 17 +++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index df6c6664..74072481 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -127,9 +127,11 @@ class JailReader(ConfigReader): if self.isEnabled(): # Read filter - if self.__opts["filter"]: - filterName, filterOpt = JailReader.extractOptions( - self.__opts["filter"]) + flt = self.__opts["filter"] + if flt: + filterName, filterOpt = JailReader.extractOptions(flt) + if not filterName: + raise ValueError("Invalid filter declaration %r" % flt) self.__filter = FilterReader( filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() @@ -157,6 +159,8 @@ class JailReader(ConfigReader): if not act: # skip empty actions continue actName, actOpt = JailReader.extractOptions(act) + if not actName: + raise ValueError("Invalid action declaration %r" % act) if actName.endswith(".py"): self.__actions.append([ "set", @@ -178,8 +182,8 @@ class JailReader(ConfigReader): else: raise AttributeError("Unable to read action") except Exception as e: - logSys.debug("Caught exception: %s" % (e,)) - raise ValueError("Error in action definition %r" % e) + logSys.debug("Caught exception: %s", e, exc_info=True) + raise ValueError("Error in action definition %r: %r" % (act, e)) if not len(self.__actions): logSys.warning("No actions were defined for %s" % self.__name) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index e68523c2..20d6ced1 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -193,13 +193,9 @@ class JailReaderTest(LogCaptureTestCase): self.assertTrue(jail.read()) self.assertFalse(jail.getOptions()) self.assertTrue(jail.isEnabled()) - self.assertLogged('Error in action definition joho[foo') - # This unittest has been deactivated for some time... - # self.assertLogged( - # 'Caught exception: While reading action joho[foo we should have got 1 or 2 groups. Got: 0') - # let's test for what is actually logged and handle changes in the future + self.assertLogged("Error in action definition 'joho[foo'") self.assertLogged( - "Caught exception: 'NoneType' object has no attribute 'endswith'") + "Caught exception: Invalid action declaration 'joho[foo'") if STOCK: def testStockSSHJail(self): @@ -496,7 +492,7 @@ class JailsReaderTest(LogCaptureTestCase): def testReadTestJailConf(self): jails = JailsReader(basedir=IMPERFECT_CONFIG, share_config=IMPERFECT_CONFIG_SHARE_CFG) self.assertTrue(jails.read()) - self.assertFalse(jails.getOptions()) + self.assertTrue(jails.getOptions()) self.assertRaises(ValueError, jails.convert) comm_commands = jails.convert(allow_no_files=True) self.maxDiff = None @@ -525,7 +521,12 @@ class JailsReaderTest(LogCaptureTestCase): ['start', 'emptyaction'], ['start', 'missinglogfiles'], ['start', 'brokenaction'], - ['start', 'parse_to_end_of_jail.conf'],])) + ['start', 'parse_to_end_of_jail.conf'], + ['config-error', + 'Jail \'brokenactiondef\' skipped, because of wrong configuration: Error in action definition \'joho[foo\': ValueError("Invalid action declaration \'joho[foo\'",)'], + ['config-error', + "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], + ])) self.assertLogged("Errors in jail 'missingbitsjail'. Skipping...") self.assertLogged("No file(s) found for glob /weapons/of/mass/destruction") From e52b47d8f52b9dd1f5704b62f21b3bf9c6c604b5 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 13:57:20 +0100 Subject: [PATCH 3/9] normalized log output (all jail parameters in filter are indented with 2 spaces) --- fail2ban/server/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fail2ban/server/actions.py b/fail2ban/server/actions.py index dcfe09f6..dd4c97c2 100644 --- a/fail2ban/server/actions.py +++ b/fail2ban/server/actions.py @@ -193,7 +193,7 @@ class Actions(JailThread, Mapping): def setBanTime(self, value): value = MyTime.str2seconds(value) self.__banManager.setBanTime(value) - logSys.info("Set banTime = %s" % value) + logSys.info(" banTime: %s" % value) ## # Get the ban time. From 4882093a41c031d79e3aeace447d762009f6528c Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 14:08:35 +0100 Subject: [PATCH 4/9] test cases extended: cover skipping invalid jail --- fail2ban/tests/fail2banclienttestcase.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/fail2ban/tests/fail2banclienttestcase.py b/fail2ban/tests/fail2banclienttestcase.py index e212ff23..cec48cc3 100644 --- a/fail2ban/tests/fail2banclienttestcase.py +++ b/fail2ban/tests/fail2banclienttestcase.py @@ -768,6 +768,10 @@ class Fail2banServerTest(Fail2banClientServerBase): _write_action_cfg(actname="test-action2") _write_jail_cfg(enabled=[1], actions=[1,2]) + # append one wrong configured jail: + _write_file(pjoin(cfg, "jail.conf"), "a", "", "[broken-jail]", + "", "filter = broken-jail-filter", "enabled = true") + _write_file(test1log, "w", *((str(int(MyTime.time())) + " failure 401 from 192.0.2.1: test 1",) * 3)) _write_file(test2log, "w") _write_file(test3log, "w") @@ -786,6 +790,12 @@ class Fail2banServerTest(Fail2banClientServerBase): self.assertLogged( "stdout: '[test-jail1] test-action1: ** start'", "stdout: '[test-jail1] test-action2: ** start'", all=True) + + # broken jail was logged (in client and server log): + self.assertLogged( + "Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'. Skipping...", + "Jail 'broken-jail' skipped, because of wrong configuration", all=True) # enable both jails, 3 logs for jail1, etc... # truncate test-log - we should not find unban/ban again by reload: From 3e9852d4d2a7c73936a4cbaafd17bf13a0e412a6 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 14:56:54 +0100 Subject: [PATCH 5/9] code review, increase coverage --- fail2ban/client/jailreader.py | 24 +++++++++++++++--------- fail2ban/client/jailsreader.py | 10 ++-------- fail2ban/tests/clientreadertestcase.py | 18 ++++++++++++++---- fail2ban/tests/config/jail.conf | 8 ++++++++ 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/fail2ban/client/jailreader.py b/fail2ban/client/jailreader.py index 74072481..b63df5f1 100644 --- a/fail2ban/client/jailreader.py +++ b/fail2ban/client/jailreader.py @@ -122,8 +122,8 @@ class JailReader(ConfigReader): # Read first options only needed for merge defaults ('known/...' from filter): self.__opts = ConfigReader.getOptions(self, self.__name, opts1st, shouldExist=True) - if not self.__opts: - raise ValueError("Init jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Init jail options failed") if self.isEnabled(): # Read filter @@ -131,7 +131,7 @@ class JailReader(ConfigReader): if flt: filterName, filterOpt = JailReader.extractOptions(flt) if not filterName: - raise ValueError("Invalid filter declaration %r" % flt) + raise JailDefError("Invalid filter definition %r" % flt) self.__filter = FilterReader( filterName, self.__name, filterOpt, share_config=self.share_config, basedir=self.getBaseDir()) ret = self.__filter.read() @@ -139,15 +139,15 @@ class JailReader(ConfigReader): self.__filter.getOptions(self.__opts) ConfigReader.merge_section(self, self.__name, self.__filter.getCombined(), 'known/') if not ret: - raise ValueError("Unable to read the filter %r" % filterName) + raise JailDefError("Unable to read the filter %r" % filterName) else: self.__filter = None logSys.warning("No filter set for jail %s" % self.__name) # Read second all options (so variables like %(known/param) can be interpolated): self.__opts = ConfigReader.getOptions(self, self.__name, opts) - if not self.__opts: - raise ValueError("Read jail options failed") + if not self.__opts: # pragma: no cover + raise JailDefError("Read jail options failed") # cumulate filter options again (ignore given in jail): if self.__filter: @@ -160,7 +160,7 @@ class JailReader(ConfigReader): continue actName, actOpt = JailReader.extractOptions(act) if not actName: - raise ValueError("Invalid action declaration %r" % act) + raise JailDefError("Invalid action definition %r" % act) if actName.endswith(".py"): self.__actions.append([ "set", @@ -180,14 +180,16 @@ class JailReader(ConfigReader): action.getOptions(self.__opts) self.__actions.append(action) else: - raise AttributeError("Unable to read action") + raise JailDefError("Unable to read action %r" % actName) + except JailDefError: + raise except Exception as e: logSys.debug("Caught exception: %s", e, exc_info=True) raise ValueError("Error in action definition %r: %r" % (act, e)) if not len(self.__actions): logSys.warning("No actions were defined for %s" % self.__name) - except ValueError as e: + except JailDefError as e: e = str(e) logSys.error(e) if not self.__opts: @@ -280,3 +282,7 @@ class JailReader(ConfigReader): val for val in optmatch.group(2,3,4) if val is not None][0] option_opts[opt.strip()] = value.strip() return option_name, option_opts + + +class JailDefError(Exception): + pass diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index ec7baca7..7d81d0a0 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -84,11 +84,8 @@ class JailsReader(ConfigReader): else: logSys.error("Errors in jail %r. Skipping..." % sec) self.__jails.append(jail) - if parse_status is None: - parse_status = False - if parse_status is None: - parse_status = True - return parse_status + if parse_status is None: parse_status = False + return True if parse_status != False else False def convert(self, allow_no_files=False): """Convert read before __opts and jails to the commands stream @@ -101,9 +98,6 @@ class JailsReader(ConfigReader): """ stream = list() - for opt in self.__opts: - if opt == "": - stream.append([]) # Convert jails for jail in self.__jails: stream.extend(jail.convert(allow_no_files=allow_no_files)) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 20d6ced1..33c217cd 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -193,9 +193,15 @@ class JailReaderTest(LogCaptureTestCase): self.assertTrue(jail.read()) self.assertFalse(jail.getOptions()) self.assertTrue(jail.isEnabled()) - self.assertLogged("Error in action definition 'joho[foo'") - self.assertLogged( - "Caught exception: Invalid action declaration 'joho[foo'") + self.assertLogged("Invalid action definition 'joho[foo'") + + def testJailFilterBrokenDef(self): + jail = JailReader('brokenfilterdef', basedir=IMPERFECT_CONFIG, + share_config=IMPERFECT_CONFIG_SHARE_CFG) + self.assertTrue(jail.read()) + self.assertFalse(jail.getOptions()) + self.assertTrue(jail.isEnabled()) + self.assertLogged("Invalid filter definition 'flt[test'") if STOCK: def testStockSSHJail(self): @@ -523,7 +529,11 @@ class JailsReaderTest(LogCaptureTestCase): ['start', 'brokenaction'], ['start', 'parse_to_end_of_jail.conf'], ['config-error', - 'Jail \'brokenactiondef\' skipped, because of wrong configuration: Error in action definition \'joho[foo\': ValueError("Invalid action declaration \'joho[foo\'",)'], + "Jail 'brokenactiondef' skipped, because of wrong configuration: Invalid action definition 'joho[foo'"], + ['config-error', + "Jail 'brokenfilterdef' skipped, because of wrong configuration: Invalid filter definition 'flt[test'"], + ['config-error', + "Jail 'missingaction' skipped, because of wrong configuration: Unable to read action 'noactionfileforthisaction'"], ['config-error', "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], ])) diff --git a/fail2ban/tests/config/jail.conf b/fail2ban/tests/config/jail.conf index bf1dea45..659e3fd3 100644 --- a/fail2ban/tests/config/jail.conf +++ b/fail2ban/tests/config/jail.conf @@ -27,10 +27,18 @@ logpath = /weapons/of/mass/destruction enabled = true action = joho[foo +[brokenfilterdef] +enabled = true +filter = flt[test + [brokenaction] enabled = true action = brokenaction +[missingaction] +enabled = true +action = noactionfileforthisaction + [missingbitsjail] enabled = true filter = catchallthebadies From fdac44ca589ff5d9de8afde3865022dc7f702858 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:08:44 +0100 Subject: [PATCH 6/9] introduced new option `-t` or `--test` to test configuration resp. start server only if configuration is clean (not skip wrong configured jails if option `-t` specified); --- fail2ban/client/configurator.py | 4 +-- fail2ban/client/fail2bancmdline.py | 33 ++++++++++++++++---- fail2ban/client/fail2banserver.py | 38 ++++++++++++------------ fail2ban/client/jailsreader.py | 13 ++++---- fail2ban/tests/fail2banclienttestcase.py | 30 +++++++++++++++++++ 5 files changed, 86 insertions(+), 32 deletions(-) diff --git a/fail2ban/client/configurator.py b/fail2ban/client/configurator.py index 4d28238f..e8472ac1 100644 --- a/fail2ban/client/configurator.py +++ b/fail2ban/client/configurator.py @@ -72,9 +72,9 @@ class Configurator: def getEarlyOptions(self): return self.__fail2ban.getEarlyOptions() - def getOptions(self, jail=None, updateMainOpt=None): + def getOptions(self, jail=None, updateMainOpt=None, ignoreWrong=True): self.__fail2ban.getOptions(updateMainOpt) - return self.__jails.getOptions(jail) + return self.__jails.getOptions(jail, ignoreWrong=ignoreWrong) def convertToProtocol(self): self.__streams["general"] = self.__fail2ban.convert() diff --git a/fail2ban/client/fail2bancmdline.py b/fail2ban/client/fail2bancmdline.py index 74236ab1..7d0eeead 100644 --- a/fail2ban/client/fail2bancmdline.py +++ b/fail2ban/client/fail2bancmdline.py @@ -47,6 +47,7 @@ class Fail2banCmdLine(): def __init__(self): self._argv = self._args = None self._configurator = None + self.cleanConfOnly = False self.resetConf() def resetConf(self): @@ -101,6 +102,7 @@ class Fail2banCmdLine(): output(" --logtarget |STDOUT|STDERR|SYSLOG") output(" --syslogsocket auto|") output(" -d dump configuration. For debugging") + output(" -t, --test test configuration (can be also specified with start parameters)") output(" -i interactive mode") output(" -v increase verbosity") output(" -q decrease verbosity") @@ -136,6 +138,9 @@ class Fail2banCmdLine(): self._conf[ o[2:] ] = opt[1] elif o == "-d": self._conf["dump"] = True + elif o == "-t" or o == "--test": + self.cleanConfOnly = True + self._conf["test"] = True elif o == "-v": self._conf["verbose"] += 1 elif o == "-q": @@ -173,8 +178,8 @@ class Fail2banCmdLine(): # Reads the command line options. try: - cmdOpts = 'hc:s:p:xfbdviqV' - cmdLongOpts = ['loglevel=', 'logtarget=', 'syslogsocket=', 'async', 'timeout=', 'help', 'version'] + cmdOpts = 'hc:s:p:xfbdtviqV' + cmdLongOpts = ['loglevel=', 'logtarget=', 'syslogsocket=', 'test', 'async', 'timeout=', 'help', 'version'] optList, self._args = getopt.getopt(self._argv[1:], cmdOpts, cmdLongOpts) except getopt.GetoptError: self.dispUsage() @@ -225,13 +230,30 @@ class Fail2banCmdLine(): logSys.info("Using pid file %s, [%s] logging to %s", self._conf["pidfile"], logging.getLevelName(llev), self._conf["logtarget"]) + readcfg = True if self._conf.get("dump", False): - ret, stream = self.readConfig() + if readcfg: + ret, stream = self.readConfig() + readcfg = False self.dumpConfig(stream) - return ret + if not self._conf.get("test", False): + return ret + + if self._conf.get("test", False): + if readcfg: + readcfg = False + ret, stream = self.readConfig() + if not ret: + raise ServerExecutionException("ERROR: test configuration failed") + # exit after test if no commands specified (test only): + if not len(self._args): + output("OK: configuration test is successful") + return ret # Nothing to do here, process in client/server return None + except ServerExecutionException: + raise except Exception as e: output("ERROR: %s" % (e,)) if verbose > 2: @@ -246,7 +268,8 @@ class Fail2banCmdLine(): try: self.configurator.Reload() self.configurator.readAll() - ret = self.configurator.getOptions(jail, self._conf) + ret = self.configurator.getOptions(jail, self._conf, + ignoreWrong=not self.cleanConfOnly) self.configurator.convertToProtocol() stream = self.configurator.getConfigStream() except Exception as e: diff --git a/fail2ban/client/fail2banserver.py b/fail2ban/client/fail2banserver.py index dfee34d2..006a02cf 100644 --- a/fail2ban/client/fail2banserver.py +++ b/fail2ban/client/fail2banserver.py @@ -144,27 +144,27 @@ class Fail2banServer(Fail2banCmdLine): return cli def start(self, argv): - # Command line options - ret = self.initCmdLine(argv) - if ret is not None: - return ret - - # Commands - args = self._args - - cli = None - # Just start: - if len(args) == 1 and args[0] == 'start' and not self._conf.get("interactive", False): - pass - else: - # If client mode - whole processing over client: - if len(args) or self._conf.get("interactive", False): - cli = self._Fail2banClient() - return cli.start(argv) - - # Start the server: server = None try: + # Command line options + ret = self.initCmdLine(argv) + if ret is not None: + return ret + + # Commands + args = self._args + + cli = None + # Just start: + if len(args) == 1 and args[0] == 'start' and not self._conf.get("interactive", False): + pass + else: + # If client mode - whole processing over client: + if len(args) or self._conf.get("interactive", False): + cli = self._Fail2banClient() + return cli.start(argv) + + # Start the server: from ..server.utils import Utils # background = True, if should be new process running in background, otherwise start in foreground # process will be forked in daemonize, inside of Server module. diff --git a/fail2ban/client/jailsreader.py b/fail2ban/client/jailsreader.py index 7d81d0a0..cd3409b4 100644 --- a/fail2ban/client/jailsreader.py +++ b/fail2ban/client/jailsreader.py @@ -54,7 +54,7 @@ class JailsReader(ConfigReader): self.__jails = list() return ConfigReader.read(self, "jail") - def getOptions(self, section=None): + def getOptions(self, section=None, ignoreWrong=True): """Reads configuration for jail(s) and adds enabled jails to __jails """ opts = [] @@ -66,7 +66,7 @@ class JailsReader(ConfigReader): sections = [ section ] # Get the options of all jails. - parse_status = None + parse_status = 0 for sec in sections: if sec == 'INCLUDES': continue @@ -78,14 +78,15 @@ class JailsReader(ConfigReader): if ret: if jail.isEnabled(): # at least one jail was successful: - parse_status = True + parse_status |= 1 # We only add enabled jails self.__jails.append(jail) else: - logSys.error("Errors in jail %r. Skipping..." % sec) + logSys.error("Errors in jail %r.%s", sec, " Skipping..." if ignoreWrong else "") self.__jails.append(jail) - if parse_status is None: parse_status = False - return True if parse_status != False else False + # at least one jail was invalid: + parse_status |= 2 + return ((ignoreWrong and parse_status & 1) or not (parse_status & 2)) def convert(self, allow_no_files=False): """Convert read before __opts and jails to the commands stream diff --git a/fail2ban/tests/fail2banclienttestcase.py b/fail2ban/tests/fail2banclienttestcase.py index cec48cc3..e68d9779 100644 --- a/fail2ban/tests/fail2banclienttestcase.py +++ b/fail2ban/tests/fail2banclienttestcase.py @@ -675,6 +675,36 @@ class Fail2banServerTest(Fail2banClientServerBase): self.pruneLog() os.remove(pjoin(tmp, "f2b.sock")) + @with_tmpdir + @with_kill_srv + def testServerTestFailStart(self, tmp): + # started directly here, so prevent overwrite test cases logger with "INHERITED" + startparams = _start_params(tmp, logtarget="INHERITED") + cfg = pjoin(tmp, "config") + + # test configuration is correct: + self.pruneLog("[test-phase 0]") + self.execSuccess(startparams, "--test") + self.assertLogged("OK: configuration test is successful") + + # append one wrong configured jail: + _write_file(pjoin(cfg, "jail.conf"), "a", "", "[broken-jail]", + "", "filter = broken-jail-filter", "enabled = true") + + # first try test config: + self.pruneLog("[test-phase 0a]") + self.execFailed(startparams, "--test") + self.assertLogged("Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'.", + "ERROR: test configuration failed", all=True) + + # failed to start with test config: + self.pruneLog("[test-phase 0b]") + self.execFailed(startparams, "-t", "start") + self.assertLogged("Unable to read the filter 'broken-jail-filter'", + "Errors in jail 'broken-jail'.", + "ERROR: test configuration failed", all=True) + @with_tmpdir def testKillAfterStart(self, tmp): try: From 8ed5b44bfd2256f8ffe8ad867b7cd9fc7cc0e467 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:38:32 +0100 Subject: [PATCH 7/9] no cover for sporadic executed (time-related) code pieces (just to prevent randomly increasing/decreasing of coverage) --- fail2ban/client/fail2banclient.py | 2 +- fail2ban/server/asyncserver.py | 2 +- fail2ban/server/utils.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fail2ban/client/fail2banclient.py b/fail2ban/client/fail2banclient.py index 007afd57..0a1ae4f1 100755 --- a/fail2ban/client/fail2banclient.py +++ b/fail2ban/client/fail2banclient.py @@ -125,7 +125,7 @@ class Fail2banClient(Fail2banCmdLine, Thread): if client: try : client.close() - except Exception as e: + except Exception as e: # pragma: no cover if showRet or self._conf["verbose"] > 1: logSys.debug(e) if showRet or c[0] == 'echo': diff --git a/fail2ban/server/asyncserver.py b/fail2ban/server/asyncserver.py index d1818d7a..9cc74658 100644 --- a/fail2ban/server/asyncserver.py +++ b/fail2ban/server/asyncserver.py @@ -241,7 +241,7 @@ class AsyncServer(asyncore.dispatcher): def _remove_sock(self): try: os.remove(self.__sock) - except OSError as e: + except OSError as e: # pragma: no cover if e.errno != errno.ENOENT: raise diff --git a/fail2ban/server/utils.py b/fail2ban/server/utils.py index 6d74d2db..57da495a 100644 --- a/fail2ban/server/utils.py +++ b/fail2ban/server/utils.py @@ -170,7 +170,7 @@ class Utils(): time.sleep(Utils.DEFAULT_SLEEP_INTERVAL) retcode = popen.poll() #logSys.debug("%s -- killed %s ", realCmd, retcode) - if retcode is None and not Utils.pid_exists(pgid): + if retcode is None and not Utils.pid_exists(pgid): # pragma: no cover retcode = signal.SIGKILL except OSError as e: stderr = "%s -- failed with %s" % (realCmd, e) From 7256a5cb8e0c37bca69fd684bd3e568a4cc46540 Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Nov 2016 17:55:27 +0100 Subject: [PATCH 8/9] code review: back to previous code - no skipping in testReadTestJailConf --- fail2ban/tests/clientreadertestcase.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fail2ban/tests/clientreadertestcase.py b/fail2ban/tests/clientreadertestcase.py index 33c217cd..5129df61 100644 --- a/fail2ban/tests/clientreadertestcase.py +++ b/fail2ban/tests/clientreadertestcase.py @@ -498,7 +498,7 @@ class JailsReaderTest(LogCaptureTestCase): def testReadTestJailConf(self): jails = JailsReader(basedir=IMPERFECT_CONFIG, share_config=IMPERFECT_CONFIG_SHARE_CFG) self.assertTrue(jails.read()) - self.assertTrue(jails.getOptions()) + self.assertFalse(jails.getOptions(ignoreWrong=False)) self.assertRaises(ValueError, jails.convert) comm_commands = jails.convert(allow_no_files=True) self.maxDiff = None @@ -537,7 +537,8 @@ class JailsReaderTest(LogCaptureTestCase): ['config-error', "Jail 'missingbitsjail' skipped, because of wrong configuration: Unable to read the filter 'catchallthebadies'"], ])) - self.assertLogged("Errors in jail 'missingbitsjail'. Skipping...") + self.assertLogged("Errors in jail 'missingbitsjail'.") + self.assertNotLogged("Skipping...") self.assertLogged("No file(s) found for glob /weapons/of/mass/destruction") if STOCK: From d908688b565fbc55fd63c1cf4ac305379afdfdca Mon Sep 17 00:00:00 2001 From: sebres Date: Thu, 24 Nov 2016 20:25:08 +0100 Subject: [PATCH 9/9] ChangeLog update --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 05a2e38f..ef2ce050 100644 --- a/ChangeLog +++ b/ChangeLog @@ -55,6 +55,8 @@ TODO: implementing of options resp. other tasks from PR #1346 banned in this jail, if option `--unban` specified - `unban --all` - unbans all IP addresses (in all jails and database) - `unban ... ` - unbans \ (in all jails and database) (see gh-1388) + - introduced new option `-t` or `--test` to test configuration resp. start server only + if configuration is clean (fails by wrong configured jails if option `-t` specified) * New command action parameter `actionrepair` - command executed in order to restore sane environment in error case of `actioncheck`. @@ -119,6 +121,10 @@ fail2ban-client set loglevel INFO - new replacement for `` in opposition to ``, for separate usage of 2 address groups only (regardless of `usedns`), `ip4` and `ip6` together, without host (dns) +* Misconfigured jails don't prevent fail2ban from starting, server starts + nevertheless, as long as one jail was successful configured (gh-1619) + Message about wrong jail configuration logged in client log (stdout, systemd + journal etc.) and in server log with error level * fail2ban-testcases: - `assertLogged` extended with parameter wait (to wait up to specified timeout, before we throw assert exception) + test cases rewritten using that