Merge pull request #484 from grooverdan/more-more-tests

BF/TST: fix internals of jailreader and add test cases

MANIFEST

@@ -242,3 +242,7 @@ files/fail2ban-tmpfiles.conf
 files/fail2ban.service
 files/ipmasq-ZZZzzz_fail2ban.rul
 files/gen_badbots
+testcases/config/jail.conf
+testcases/config/fail2ban.conf
+testcases/config/filter.d/simple.conf
+testcases/config/action.d/brokenaction.conf

client/configreader.py

@@ -112,7 +112,7 @@ class ConfigReader(SafeConfigParserWithIncludes):
 			except NoSectionError, e:
 				# No "Definition" section or wrong basedir
 				logSys.error(e)
-				values[option[1]] = option[2]
+				return False
 			except NoOptionError:
 				if not option[2] is None:
 					logSys.warn("'%s' not defined in '%s'. Using default one: %r"

client/jailreader.py

@@ -35,7 +35,7 @@ logSys = logging.getLogger("fail2ban.client.config")
 
 class JailReader(ConfigReader):
 	
-	actionCRE = re.compile("^((?:\w|-|_|\.)+)(?:\[(.*)\])?$")
+	actionCRE = re.compile("^([\w_.-]+)(?:\[(.*)\])?$")
 	
 	def __init__(self, name, force_enable=False, **kwargs):
 		ConfigReader.__init__(self, **kwargs)
@@ -54,7 +54,7 @@ class JailReader(ConfigReader):
 		return ConfigReader.read(self, "jail")
 	
 	def isEnabled(self):
-		return self.__force_enable or self.__opts["enabled"]
+		return self.__force_enable or ( self.__opts and self.__opts["enabled"] )
 
 	@staticmethod
 	def _glob(path):
@@ -64,12 +64,10 @@ class JailReader(ConfigReader):
 		"""
 		pathList = []
 		for p in glob.glob(path):
-			if not os.path.exists(p):
-				logSys.warning("File %s doesn't even exist, thus cannot be monitored" % p)
-			elif not os.path.lexists(p):
-				logSys.warning("File %s is a dangling link, thus cannot be monitored" % p)
-			else:
+			if os.path.exists(p):
 				pathList.append(p)
+			else:
+				logSys.warning("File %s is a dangling link, thus cannot be monitored" % p)
 		return pathList
 
 	def getOptions(self):
@@ -86,18 +84,24 @@ class JailReader(ConfigReader):
 				["string", "filter", ""],
 				["string", "action", ""]]
 		self.__opts = ConfigReader.getOptions(self, self.__name, opts)
+		if not self.__opts:
+			return False
 		
 		if self.isEnabled():
 			# Read filter
-			self.__filter = FilterReader(self.__opts["filter"], self.__name,
-										 basedir=self.getBaseDir())
-			ret = self.__filter.read()
-			if ret:
-				self.__filter.getOptions(self.__opts)
+			if self.__opts["filter"]:
+				self.__filter = FilterReader(self.__opts["filter"], self.__name,
+											 basedir=self.getBaseDir())
+				ret = self.__filter.read()
+				if ret:
+					self.__filter.getOptions(self.__opts)
+				else:
+					logSys.error("Unable to read the filter")
+					return False
 			else:
-				logSys.error("Unable to read the filter")
-				return False
-			
+				self.__filter = None
+				logSys.warn("No filter set for jail %s" % self.__name)
+		
 			# Read action
 			for act in self.__opts["action"].split('\n'):
 				try:
@@ -165,7 +169,8 @@ class JailReader(ConfigReader):
 					# Do not send a command if the rule is empty.
 					if regex != '':
 						stream.append(["set", self.__name, "addignoreregex", regex])
-		stream.extend(self.__filter.convert())
+		if self.__filter:
+			stream.extend(self.__filter.convert())
 		for action in self.__actions:
 			stream.extend(action.convert())
 		stream.insert(0, ["add", self.__name, backend])
@@ -175,12 +180,16 @@ class JailReader(ConfigReader):
 	def splitAction(action):
 		m = JailReader.actionCRE.match(action)
 		d = dict()
-		mgroups = m.groups()
+		try:
+			mgroups = m.groups()
+		except AttributeError:
+			raise ValueError("While reading action %s we should have got 1 or "
+							 "2 groups. Got: 0" % action)
 		if len(mgroups) == 2:
 			action_name, action_opts = mgroups
-		elif len(mgroups) == 1:
+		elif len(mgroups) == 1: # pragma: nocover - unreachable - .* on second group always matches
 			action_name, action_opts = mgroups[0], None
-		else:
+		else: # pragma: nocover - unreachable - regex only can capture 2 groups
 			raise ValueError("While reading action %s we should have got up to "
 							 "2 groups. Got: %r" % (action, mgroups))
 		if not action_opts is None:

client/jailsreader.py

@@ -60,6 +60,7 @@ class JailsReader(ConfigReader):
 			sections = [ section ]
 
 		# Get the options of all jails.
+		parse_status = True
 		for sec in sections:
 			jail = JailReader(sec, basedir=self.getBaseDir(),
 							  force_enable=self.__force_enable)
@@ -71,8 +72,8 @@ class JailsReader(ConfigReader):
 					self.__jails.append(jail)
 			else:
 				logSys.error("Errors in jail %r. Skipping..." % sec)
-				return False
-		return True
+				parse_status = False
+		return parse_status
 
 	def convert(self, allow_no_files=False):
 		"""Convert read before __opts and jails to the commands stream

man/jail.conf.5

@@ -63,6 +63,12 @@ Comments: use '#' for comment lines and ';' (following a space) for inline comme
 .SH DEFAULT
 The following options are applicable to all jails. Their meaning is described in the default \fIjail.conf\fR file.
 .TP
+\fBfilter\fR 
+.TP
+\fBlogpath\fR 
+.TP
+\fBaction\fR 
+.TP
 \fBignoreip\fR 
 .TP
 \fBbantime\fR
@@ -74,6 +80,10 @@ The following options are applicable to all jails. Their meaning is described in
 \fBbackend\fR
 .TP
 \fBusedns\fR
+.TP
+\fBfailregex\fR
+.TP
+\fBignoreregex\fR
 
 
 .SH "ACTION FILES"

testcases/clientreadertestcase.py

@@ -27,6 +27,7 @@ from client.configreader import ConfigReader
 from client.jailreader import JailReader
 from client.jailsreader import JailsReader
 from client.configurator import Configurator
+from utils import LogCaptureTestCase
 
 class ConfigReaderTest(unittest.TestCase):
 
@@ -106,7 +107,31 @@ option = %s
 		self.assertEqual(self._getoption(), 1)
 
 
-class JailReaderTest(unittest.TestCase):
+class JailReaderTest(LogCaptureTestCase):
+
+	def testJailActionEmpty(self):
+		jail = JailReader('emptyaction', basedir=os.path.join('testcases','config'))
+		self.assertTrue(jail.read())
+		self.assertTrue(jail.getOptions())
+		self.assertTrue(jail.isEnabled())
+		self.assertTrue(self._is_logged('No filter set for jail emptyaction'))
+		self.assertTrue(self._is_logged('No actions were defined for emptyaction'))
+
+	def testJailActionFilterMissing(self):
+		jail = JailReader('missingbitsjail', basedir=os.path.join('testcases','config'))
+		self.assertTrue(jail.read())
+		self.assertFalse(jail.getOptions())
+		self.assertTrue(jail.isEnabled())
+		self.assertTrue(self._is_logged("Found no accessible config files for 'filter.d/catchallthebadies' under testcases/config"))
+		self.assertTrue(self._is_logged('Unable to read the filter'))
+
+	def testJailActionBrokenDef(self):
+		jail = JailReader('brokenactiondef', basedir=os.path.join('testcases','config'))
+		self.assertTrue(jail.read())
+		self.assertFalse(jail.getOptions())
+		self.assertTrue(jail.isEnabled())
+		self.assertTrue(self._is_logged('Error in action definition joho[foo'))
+		self.assertTrue(self._is_logged('Caught exception: While reading action joho[foo we should have got 1 or 2 groups. Got: 0'))
 
 	def testStockSSHJail(self):
 		jail = JailReader('ssh-iptables', basedir='config') # we are running tests from root project dir atm
@@ -114,33 +139,111 @@ class JailReaderTest(unittest.TestCase):
 		self.assertTrue(jail.getOptions())
 		self.assertFalse(jail.isEnabled())
 		self.assertEqual(jail.getName(), 'ssh-iptables')
+		jail.setName('ssh-funky-blocker')
+		self.assertEqual(jail.getName(), 'ssh-funky-blocker')
 
 	def testSplitAction(self):
 		action = "mail-whois[name=SSH]"
 		expected = ['mail-whois', {'name': 'SSH'}]
 		result = JailReader.splitAction(action)
 		self.assertEqual(expected, result)
+
+		self.assertEqual(['mail.who_is', {}], JailReader.splitAction("mail.who_is"))
+		self.assertEqual(['mail.who_is', {'a':'cat', 'b':'dog'}], JailReader.splitAction("mail.who_is[a=cat,b=dog]"))
+		self.assertEqual(['mail--ho_is', {}], JailReader.splitAction("mail--ho_is"))
+
+		self.assertEqual(['mail--ho_is', {}], JailReader.splitAction("mail--ho_is['s']"))
+		self.assertTrue(self._is_logged("Invalid argument ['s'] in ''s''"))
+
+		self.assertEqual(['mail', {'a': ','}], JailReader.splitAction("mail[a=',']"))
 		
+		self.assertRaises(ValueError, JailReader.splitAction ,'mail-how[')
+
+
 	def testGlob(self):
 		d = tempfile.mkdtemp(prefix="f2b-temp")
 		# Generate few files
 		# regular file
-		open(os.path.join(d, 'f1'), 'w').close()
+		f1 = os.path.join(d, 'f1')
+		open(f1, 'w').close()
 		# dangling link
-		os.symlink('nonexisting', os.path.join(d, 'f2'))
+		
+		f2 = os.path.join(d, 'f2')
+		os.symlink('nonexisting',f2)
 
 		# must be only f1
-		self.assertEqual(JailReader._glob(os.path.join(d, '*')), [os.path.join(d, 'f1')])
+		self.assertEqual(JailReader._glob(os.path.join(d, '*')), [f1])
 		# since f2 is dangling -- empty list
-		self.assertEqual(JailReader._glob(os.path.join(d, 'f2')), [])
+		self.assertEqual(JailReader._glob(f2), [])
+		self.assertTrue(self._is_logged('File %s is a dangling link, thus cannot be monitored' % f2))
+		self.assertEqual(JailReader._glob(os.path.join(d, 'nonexisting')), [])
+		os.remove(f1)
+		os.remove(f2)
+		os.rmdir(d)
 
-class JailsReaderTest(unittest.TestCase):
+class JailsReaderTest(LogCaptureTestCase):
 
 	def testProvidingBadBasedir(self):
 		if not os.path.exists('/XXX'):
 			reader = JailsReader(basedir='/XXX')
 			self.assertRaises(ValueError, reader.read)
 
+	def testReadTestJailConf(self):
+		jails = JailsReader(basedir=os.path.join('testcases','config'))
+		self.assertTrue(jails.read())
+		self.assertFalse(jails.getOptions())
+		self.assertRaises(ValueError, jails.convert)
+		comm_commands = jails.convert(allow_no_files=True)
+		self.maxDiff = None
+		self.assertEqual(sorted(comm_commands),
+			sorted([['add', 'emptyaction', 'auto'],
+			 ['set', 'emptyaction', 'usedns', 'warn'],
+			 ['set', 'emptyaction', 'maxretry', 3],
+			 ['set', 'emptyaction', 'findtime', 600],
+			 ['set', 'emptyaction', 'bantime', 600],
+			 ['add', 'special', 'auto'],
+			 ['set', 'special', 'usedns', 'warn'],
+			 ['set', 'special', 'maxretry', 3],
+			 ['set', 'special', 'addfailregex', '<IP>'],
+			 ['set', 'special', 'findtime', 600],
+			 ['set', 'special', 'bantime', 600],
+			 ['add', 'missinglogfiles', 'auto'],
+			 ['set', 'missinglogfiles', 'usedns', 'warn'],
+			 ['set', 'missinglogfiles', 'maxretry', 3],
+			 ['set', 'missinglogfiles', 'findtime', 600],
+			 ['set', 'missinglogfiles', 'bantime', 600],
+			 ['set', 'missinglogfiles', 'addfailregex', '<IP>'],
+			 ['add', 'brokenaction', 'auto'],
+			 ['set', 'brokenaction', 'usedns', 'warn'],
+			 ['set', 'brokenaction', 'maxretry', 3],
+			 ['set', 'brokenaction', 'findtime', 600],
+			 ['set', 'brokenaction', 'bantime', 600],
+			 ['set', 'brokenaction', 'addfailregex', '<IP>'],
+			 ['set', 'brokenaction', 'addaction', 'brokenaction'],
+			 ['set',
+			  'brokenaction',
+			  'actionban',
+			  'brokenaction',
+			  'hit with big stick <ip>'],
+			 ['set', 'brokenaction', 'actionstop', 'brokenaction', ''],
+			 ['set', 'brokenaction', 'actionstart', 'brokenaction', ''],
+			 ['set', 'brokenaction', 'actionunban', 'brokenaction', ''],
+			 ['set', 'brokenaction', 'actioncheck', 'brokenaction', ''],
+			 ['add', 'parse_to_end_of_jail.conf', 'auto'],
+			 ['set', 'parse_to_end_of_jail.conf', 'usedns', 'warn'],
+			 ['set', 'parse_to_end_of_jail.conf', 'maxretry', 3],
+			 ['set', 'parse_to_end_of_jail.conf', 'findtime', 600],
+			 ['set', 'parse_to_end_of_jail.conf', 'bantime', 600],
+			 ['set', 'parse_to_end_of_jail.conf', 'addfailregex', '<IP>'],
+			 ['start', 'emptyaction'],
+			 ['start', 'special'],
+			 ['start', 'missinglogfiles'],
+			 ['start', 'brokenaction'],
+			 ['start', 'parse_to_end_of_jail.conf'],]))
+		self.assertTrue(self._is_logged("Errors in jail 'missingbitsjail'. Skipping..."))
+		self.assertTrue(self._is_logged("No file(s) found for glob /weapons/of/mass/destruction"))
+
+
 	def testReadStockJailConf(self):
 		jails = JailsReader(basedir='config') # we are running tests from root project dir atm
 		self.assertTrue(jails.read())		  # opens fine
@@ -153,6 +256,7 @@ class JailsReaderTest(unittest.TestCase):
 		# We should not "read" some bogus jail
 		old_comm_commands = comm_commands[:]   # make a copy
 		self.assertFalse(jails.getOptions("BOGUS"))
+		self.assertTrue(self._is_logged("No section: 'BOGUS'"))
 		# and there should be no side-effects
 		self.assertEqual(jails.convert(), old_comm_commands)
 

testcases/config/action.d/brokenaction.conf

@@ -0,0 +1,4 @@
+
+[Definition]
+
+actionban = hit with big stick <ip>

testcases/config/fail2ban.conf

@@ -0,0 +1,5 @@
+[Definition]
+
+#         3 = INFO
+loglevel = 3
+

testcases/config/filter.d/simple.conf

@@ -0,0 +1,4 @@
+
+[Definition]
+
+failregex = <IP>

testcases/config/jail.conf

@@ -0,0 +1,33 @@
+
+[DEFAULT]
+filter = simple
+logpath = /non/exist
+
+[emptyaction]
+enabled = true
+filter =
+action =
+
+[special]
+failregex = <IP>
+ignoreregex =
+ignoreip =
+
+[missinglogfiles]
+logpath = /weapons/of/mass/destruction
+
+[brokenactiondef]
+enabled = true
+action = joho[foo
+
+[brokenaction]
+enabled = true
+action = brokenaction
+
+[missingbitsjail]
+filter = catchallthebadies
+action = thefunkychickendance
+
+[parse_to_end_of_jail.conf]
+enabled = true
+action =