Merge pull request #1619 from sebres/_0.10/skip-wrong-jails

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 <IP> ... <IP>` - unbans \<IP\> (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 `<ADDR>` in opposition to `<HOST>`, 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

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()

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':

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 <FILE>|STDOUT|STDERR|SYSLOG")
 		output("    --syslogsocket auto|<FILE>")
 		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:

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.

fail2ban/client/jailreader.py

@@ -118,70 +118,84 @@ 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)
-			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)
+			# 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: # pragma: no cover
+				raise JailDefError("Init jail options failed")
+			
+			if self.isEnabled():
+				# Read filter
+				flt = self.__opts["filter"]
+				if flt:
+					filterName, filterOpt = JailReader.extractOptions(flt)
+					if not filterName:
+						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()
+					# 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 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: # pragma: no cover
+					raise JailDefError("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 not actName:
+							raise JailDefError("Invalid action definition %r" % 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 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 JailDefError 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 +209,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"):
@@ -264,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

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 = True
+		parse_status = 0
 		for sec in sections:
 			if sec == 'INCLUDES':
 				continue
@@ -77,12 +77,16 @@ class JailsReader(ConfigReader):
 			ret = jail.getOptions()
 			if ret:
 				if jail.isEnabled():
+					# at least one jail was successful:
+					parse_status |= 1
 					# We only add enabled jails
 					self.__jails.append(jail)
 			else:
-				logSys.error("Errors in jail %r. Skipping..." % sec)
-				parse_status = False
-		return parse_status
+				logSys.error("Errors in jail %r.%s", sec, " Skipping..." if ignoreWrong else "")
+				self.__jails.append(jail)
+				# 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
@@ -95,15 +99,13 @@ 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))
 		# 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
 

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.

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
 

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):

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)

fail2ban/tests/clientreadertestcase.py

@@ -193,13 +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')
-		# 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(
-			"Caught exception: 'NoneType' object has no attribute 'endswith'")
+		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):
@@ -496,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.assertFalse(jails.getOptions())
+		self.assertFalse(jails.getOptions(ignoreWrong=False))
 		self.assertRaises(ValueError, jails.convert)
 		comm_commands = jails.convert(allow_no_files=True)
 		self.maxDiff = None
@@ -525,8 +527,18 @@ class JailsReaderTest(LogCaptureTestCase):
 			 ['start', 'emptyaction'],
 			 ['start', 'missinglogfiles'],
 			 ['start', 'brokenaction'],
-			 ['start', 'parse_to_end_of_jail.conf'],]))
-		self.assertLogged("Errors in jail 'missingbitsjail'. Skipping...")
+			 ['start', 'parse_to_end_of_jail.conf'],
+			 ['config-error',
+				"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'"],
+			 ]))
+		self.assertLogged("Errors in jail 'missingbitsjail'.")
+		self.assertNotLogged("Skipping...")
 		self.assertLogged("No file(s) found for glob /weapons/of/mass/destruction")
 
 	if STOCK:

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

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:
@@ -768,6 +798,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 +820,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: