[apparmor] [patch 2/3] Add tests for NetworkRule and NetworkRuleset
Christian Boltz
apparmor at cboltz.de
Sun Apr 19 22:29:26 UTC 2015
Hello,
Am Sonntag, 19. April 2015 schrieb Kshitij Gupta:
> On Sat, Apr 18, 2015 at 2:51 AM, Christian Boltz wrote:
> > Am Dienstag, 14. April 2015 schrieb Christian Boltz:
> > > this patch adds utils/test/test-network.py with tests for
> > > NetworkRule
> > > and NetworkRuleset.
> > >
> > > The tests are hopefully self-explaining, so let me just mention
> > > the
> > > most important things:
> > > - I started to play with namedtuple, which looks very useful (see
>
> Interesting! Like a dictionary but without any overhead.
The important thing is that it allows to have named elements without the
need to repeat the names all the time. And that's something really
useful, especially when writing tests table-style to get sane line
lengths ;-)
That said - we have to thank Steve who gave a pointer to namedtuple some
weeks ago.
> > +import unittest
> > +from common_test import AATest, setup_all_loops
> > +from collections import namedtuple
>
> Nitpick: a non standard import surrounding apparmor specific import is
> odd. Probably could re-use some shuffling around/separation.
> quoting PEP 8 [1]:
> "Imports should be grouped in the following order:
>
> 1. standard library imports
> 2. related third party imports
> 3. local application/library specific imports
>
> You should put a blank line between each group of imports."
Hmm, that's an interesting question.
While I agree that those rules make sense for "normal" code, I'm not
sure if they make sense for our testcases.
Basically the order I'm using is:
1. libraries needed to setup and run the tests (sooner or later, all
tests will have those imports)
2. the things we want to test
3. additional libs needed for some tests (for example 're', even if
that vanished by moving test_parse_profile_invalid() to
test-baserule.py)
Inside the first group, changing the order according to PEP 8 can't
hurt, so I'm moving namedtuple a line upwards.
> > +from apparmor.rule.network import NetworkRule, NetworkRuleset
> > +from apparmor.rule import BaseRule, parse_modifiers
> > +from apparmor.common import AppArmorException, AppArmorBug
> > +from apparmor.logparser import ReadLog
> > +
> > +import re
> > +
> > +exp = namedtuple('exp', ['audit', 'allow_keyword', 'deny',
> > 'comment', + 'domain', 'all_domains', 'type_or_protocol',
> > 'all_type_or_protocols'])
>
> Didn't know about this. Pretty cool for readability.
Indeed :-) (as long as you add a comment as "headline" when using it)
> > +class NetworkTestParseFromLog(NetworkTest):
> > + def test_net_from_log(self):
> > + parser = ReadLog('', '', '', '', '')
> > + event = 'type=AVC msg=audit(1428699242.551:386):
> > apparmor="DENIED" operation="create" profile="/bin/ping" pid=10589
> > comm="ping" family="inet" sock_type="raw" protocol=1'
> > +
> > + parsed_event = parser.parse_event(event)
> > +
> > + self.assertEqual(parsed_event, {
> > + 'request_mask': None,
> > + 'denied_mask': None,
>
> you switched from set() to None between the two versions of this
> patch?
Yes, that's caused by the logparser.py r3009 changes, which moved the
code that parses request_mask and denied_mask to add_event_to_tree()
(and does it only for file events).
Sorry, I should have mentioned that in my v2 mail.
> > + self._compare_obj(obj, expected)
> > +
> > + self.assertEqual(obj.get_raw(1), ' network inet raw,')
>
> why obj.get_raw(1) and not assertEqual on obj.raw_rule? The depth code
> in get_raw() should be already be under tests elsewhere, seems
> redundant? look below: comment about parse_modifiers
I prefer to test against the public functions - obj.raw_rule is just an
implementation detail that could in theory change without breaking
something.
Besides that - in test_write_manually() obj.raw_rule is empty ;-)
> > +class InvalidNetworkInit(AATest):
> > + tests = [
> > + ([dict(), 'tcp' ] , AppArmorBug), # wrong
> > type for domain
> > + (['inet', dict() ] , AppArmorBug), # wrong
> > type for type_or_protocol
>
> Nitpick: would it be better/correct to use None instead of dict()
> here? using dict() or list() seems arbitrary to me (I maybe wrong)?
Sounds like a philosophical question - what is a correct invalid value? ;-)
Basically we can use everything that is not a string and not
NetworkRule.ALL as invalid parameter, so I just picked one.
Oh, and I just added the "None" variant as additional tests.
> > + def test_parse_modifiers_invalid(self):
> > + regex =
> > re.compile('^\s*(?P<audit>audit\s+)?(?P<allow>allow\s+|deny\s+|inval
> > id\s+)?') + matches = regex.search('audit invalid ')
> > +
> > + with self.assertRaises(AppArmorBug):
> > + parse_modifiers(matches)
>
> This test is redundant as its already present in test-capability. Also
> this test should instead be located somewhere where the tests for the
> base classes are located as parse_modifiers is defined with them.
Indeed. I removed it from test-network.py (which also means to remove
the import for parse_modifiers and re).
I just sent a separate patch to move it from test-capability.py to
test-baserule.py (the patch to add test-baserule.py is still pending,
reviewing it should be easy ;-)
> > + def test_write_manually(self):
> > + obj = NetworkRule('inet', 'stream', allow_keyword=True)
> > +
> > + expected = ' allow network inet stream,'
> > +
> > + self.assertEqual(expected, obj.get_clean(2), 'unexpected
> > clean rule')
> > + self.assertEqual(expected, obj.get_raw(2), 'unexpected raw
> > rule')
> As stated in rule/__init__.py, subclasses needed to implement
> get_clean so its fair that get_clean should have tests here, however,
> get_raw is not being implemented in capability or network rule
> classes and it seems redundant to have tests for the same at both
> places. I feel we can do away with tests for these methods in every
> rule class and have them centralised place (tests for the __init__
> stuff), unless a class overrides it. Opinions?
In theory, I agree - yes, it seems redundant.
In practise, I want to keep that safety net - and I also want to test
with all rule types to make sure we don't accidently break writing the
raw rule somewhere.
> In general the above patch looks good specially given the 100%
> coverage stat (will coverage % be effected if we move the get_raw
> stuff to some other place?).
Probably not, but as I said, I prefer not to move it.
BTW: Coverage stats are nice, but don't tell everything. Just as an
example - if you test a complex regex with one example, you'll have
100% coverage, but this doesn't mean that the regex is correct for all
usecases.
> Note: I've used to the existing capability rule tests/classes for
> reference given it went through many rounds of iteration before being
> committed.
Then you probably also noticed that I've used test-capability.py as
base for test-network.py - basically I started with
sed 's/^/# / ; s/capability/network/' < test-capability.py > test-network.py
and then did some more changes and removed the # again.
Of course, that's the simplified version - there are more differences
besides those replacements, and also some improvements like using
namedtuple.
> It wouldn't hurt to have second set of eyes look at this patch series.
>
> However, I'm willing to ack this patch set once Christian responds on
> the above queries.
:-)
> [1]- https://www.python.org/dev/peps/pep-0008/#imports
>
> Follow-up read:
> https://www.python.org/dev/peps/pep-0328/#guido-s-decision
That sounds like another can of worms... ;-)
That all said, here's the updated patch.
[ 45-add-tests-for-NetworkRule.diff ]
=== added file utils/test/test-network.py
--- utils/test/test-network.py 2015-04-20 00:15:46.001752247 +0200
+++ utils/test/test-network.py 2015-04-20 00:17:43.852948920 +0200
@@ -0,0 +1,421 @@
+#!/usr/bin/env python
+# ----------------------------------------------------------------------
+# Copyright (C) 2015 Christian Boltz <apparmor at cboltz.de>
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of version 2 of the GNU General Public
+# License as published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# ----------------------------------------------------------------------
+
+import unittest
+from collections import namedtuple
+from common_test import AATest, setup_all_loops
+
+from apparmor.rule.network import NetworkRule, NetworkRuleset
+from apparmor.rule import BaseRule
+from apparmor.common import AppArmorException, AppArmorBug
+from apparmor.logparser import ReadLog
+
+exp = namedtuple('exp', ['audit', 'allow_keyword', 'deny', 'comment',
+ 'domain', 'all_domains', 'type_or_protocol', 'all_type_or_protocols'])
+
+# --- tests for single NetworkRule --- #
+
+class NetworkTest(AATest):
+ def _compare_obj(self, obj, expected):
+ self.assertEqual(expected.allow_keyword, obj.allow_keyword)
+ self.assertEqual(expected.audit, obj.audit)
+ self.assertEqual(expected.domain, obj.domain)
+ self.assertEqual(expected.type_or_protocol, obj.type_or_protocol)
+ self.assertEqual(expected.all_domains, obj.all_domains)
+ self.assertEqual(expected.all_type_or_protocols, obj.all_type_or_protocols)
+ self.assertEqual(expected.deny, obj.deny)
+ self.assertEqual(expected.comment, obj.comment)
+
+class NetworkTestParse(NetworkTest):
+ tests = [
+ # rawrule audit allow deny comment domain all? type/proto all?
+ ('network,' , exp(False, False, False, '' , None , True , None , True )),
+ ('network inet,' , exp(False, False, False, '' , 'inet', False, None , True )),
+ ('network inet stream,' , exp(False, False, False, '' , 'inet', False, 'stream' , False)),
+ ('deny network inet stream, # comment' , exp(False, False, True , ' # comment' , 'inet', False, 'stream' , False)),
+ ('audit allow network tcp,' , exp(True , True , False, '' , None , True , 'tcp' , False)),
+ ]
+
+ def _run_test(self, rawrule, expected):
+ obj = NetworkRule.parse(rawrule)
+ self.assertEqual(rawrule.strip(), obj.raw_rule)
+ self._compare_obj(obj, expected)
+
+class NetworkTestParseInvalid(NetworkTest):
+ tests = [
+ ('network stream,' , AppArmorException), # domain missing
+ ('network foo,' , AppArmorException),
+ ('network foo bar,' , AppArmorException),
+ ('network foo tcp,' , AppArmorException),
+ ('network inet bar,' , AppArmorException),
+ ]
+
+ def _run_test(self, rawrule, expected):
+ with self.assertRaises(expected):
+ NetworkRule.parse(rawrule)
+
+class NetworkTestParseFromLog(NetworkTest):
+ def test_net_from_log(self):
+ parser = ReadLog('', '', '', '', '')
+ event = 'type=AVC msg=audit(1428699242.551:386): apparmor="DENIED" operation="create" profile="/bin/ping" pid=10589 comm="ping" family="inet" sock_type="raw" protocol=1'
+
+ parsed_event = parser.parse_event(event)
+
+ self.assertEqual(parsed_event, {
+ 'request_mask': None,
+ 'denied_mask': None,
+ 'error_code': 0,
+ 'family': 'inet',
+ 'magic_token': 0,
+ 'parent': 0,
+ 'profile': '/bin/ping',
+ 'protocol': 'icmp',
+ 'sock_type': 'raw',
+ 'operation': 'create',
+ 'resource': None,
+ 'info': None,
+ 'aamode': 'REJECTING',
+ 'time': 1428699242,
+ 'active_hat': None,
+ 'pid': 10589,
+ 'task': 0,
+ 'attr': None,
+ 'name2': None,
+ 'name': None,
+ })
+
+ obj = NetworkRule(parsed_event['family'], parsed_event['sock_type'], log_event=parsed_event)
+
+ # audit allow deny comment domain all? type/proto all?
+ expected = exp(False, False, False, '' , 'inet', False, 'raw' , False)
+
+ self._compare_obj(obj, expected)
+
+ self.assertEqual(obj.get_raw(1), ' network inet raw,')
+
+
+class NetworkFromInit(NetworkTest):
+ tests = [
+ # NetworkRule object audit allow deny comment domain all? type/proto all?
+ (NetworkRule('inet', 'raw', deny=True) , exp(False, False, True , '' , 'inet', False, 'raw' , False)),
+ (NetworkRule('inet', 'raw') , exp(False, False, False, '' , 'inet', False, 'raw' , False)),
+ (NetworkRule('inet', NetworkRule.ALL) , exp(False, False, False, '' , 'inet', False, None , True )),
+ (NetworkRule(NetworkRule.ALL, NetworkRule.ALL) , exp(False, False, False, '' , None , True , None , True )),
+ (NetworkRule(NetworkRule.ALL, 'tcp') , exp(False, False, False, '' , None , True , 'tcp' , False)),
+ ]
+
+ def _run_test(self, obj, expected):
+ self._compare_obj(obj, expected)
+
+
+class InvalidNetworkInit(AATest):
+ tests = [
+ # init params expected exception
+ (['inet', '' ] , AppArmorBug), # empty type_or_protocol
+ (['' , 'tcp' ] , AppArmorBug), # empty domain
+ ([' ', 'tcp' ] , AppArmorBug), # whitespace domain
+ (['inet', ' ' ] , AppArmorBug), # whitespace type_or_protocol
+ (['xyxy', 'tcp' ] , AppArmorBug), # invalid domain
+ (['inet', 'xyxy' ] , AppArmorBug), # invalid type_or_protocol
+ ([dict(), 'tcp' ] , AppArmorBug), # wrong type for domain
+ ([None , 'tcp' ] , AppArmorBug), # wrong type for domain
+ (['inet', dict() ] , AppArmorBug), # wrong type for type_or_protocol
+ (['inet', None ] , AppArmorBug), # wrong type for type_or_protocol
+ ([NetworkRule.ALL, 'stream'] , AppArmorException), # stream requires a domain
+ ]
+
+ def _run_test(self, params, expected):
+ with self.assertRaises(expected):
+ NetworkRule(params[0], params[1])
+
+ def test_missing_params_1(self):
+ with self.assertRaises(TypeError):
+ NetworkRule()
+
+ def test_missing_params_2(self):
+ with self.assertRaises(TypeError):
+ NetworkRule('inet')
+
+
+class InvalidNetworkTest(AATest):
+ def _check_invalid_rawrule(self, rawrule):
+ obj = None
+ with self.assertRaises(AppArmorException):
+ obj = NetworkRule(NetworkRule.parse(rawrule))
+
+ self.assertIsNone(obj, 'NetworkRule handed back an object unexpectedly')
+
+ def test_invalid_net_missing_comma(self):
+ self._check_invalid_rawrule('network') # missing comma
+
+ def test_invalid_net_non_NetworkRule(self):
+ self._check_invalid_rawrule('dbus,') # not a network rule
+
+ def test_empty_net_data_1(self):
+ obj = NetworkRule('inet', 'stream')
+ obj.domain = ''
+ # no domain set, and ALL not set
+ with self.assertRaises(AppArmorBug):
+ obj.get_clean(1)
+
+ def test_empty_net_data_2(self):
+ obj = NetworkRule('inet', 'stream')
+ obj.type_or_protocol = ''
+ # no type_or_protocol set, and ALL not set
+ with self.assertRaises(AppArmorBug):
+ obj.get_clean(1)
+
+
+class WriteNetworkTestAATest(AATest):
+ def _run_test(self, rawrule, expected):
+ obj = NetworkRule.parse(rawrule)
+ clean = obj.get_clean()
+ raw = obj.get_raw()
+
+ self.assertEqual(expected.strip(), clean, 'unexpected clean rule')
+ self.assertEqual(rawrule.strip(), raw, 'unexpected raw rule')
+
+ tests = [
+ # raw rule clean rule
+ (' network , # foo ' , 'network, # foo'),
+ (' audit network inet,' , 'audit network inet,'),
+ (' deny network inet stream,# foo bar' , 'deny network inet stream, # foo bar'),
+ (' deny network inet ,# foo bar' , 'deny network inet, # foo bar'),
+ (' allow network tcp ,# foo bar' , 'allow network tcp, # foo bar'),
+ ]
+
+ def test_write_manually(self):
+ obj = NetworkRule('inet', 'stream', allow_keyword=True)
+
+ expected = ' allow network inet stream,'
+
+ self.assertEqual(expected, obj.get_clean(2), 'unexpected clean rule')
+ self.assertEqual(expected, obj.get_raw(2), 'unexpected raw rule')
+
+
+class NetworkCoveredTest(AATest):
+ def _run_test(self, param, expected):
+ obj = NetworkRule.parse(self.rule)
+ check_obj = NetworkRule.parse(param)
+
+ self.assertEqual(obj.is_equal(check_obj), expected[0], 'Mismatch in is_equal, expected %s' % expected[0])
+ self.assertEqual(obj.is_equal(check_obj, True), expected[1], 'Mismatch in is_equal/strict, expected %s' % expected[1])
+
+ self.assertEqual(obj.is_covered(check_obj), expected[2], 'Mismatch in is_covered, expected %s' % expected[2])
+ self.assertEqual(obj.is_covered(check_obj, True, True), expected[3], 'Mismatch in is_covered/exact, expected %s' % expected[3])
+
+class NetworkCoveredTest_01(NetworkCoveredTest):
+ rule = 'network inet,'
+
+ tests = [
+ # rule equal strict equal covered covered exact
+ ('network,' , [ False , False , False , False ]),
+ ('network inet,' , [ True , True , True , True ]),
+ ('network inet, # comment' , [ True , False , True , True ]),
+ ('allow network inet,' , [ True , False , True , True ]),
+ ('network inet,' , [ True , False , True , True ]),
+ ('network inet stream,' , [ False , False , True , True ]),
+ ('network inet tcp,' , [ False , False , True , True ]),
+ ('audit network inet,' , [ False , False , False , False ]),
+ ('audit network,' , [ False , False , False , False ]),
+ ('network unix,' , [ False , False , False , False ]),
+ ('network tcp,' , [ False , False , False , False ]),
+ ('audit deny network inet,' , [ False , False , False , False ]),
+ ('deny network inet,' , [ False , False , False , False ]),
+ ]
+
+class NetworkCoveredTest_02(NetworkCoveredTest):
+ rule = 'audit network inet,'
+
+ tests = [
+ # rule equal strict equal covered covered exact
+ ( 'network inet,' , [ False , False , True , False ]),
+ ('audit network inet,' , [ True , True , True , True ]),
+ ( 'network inet stream,' , [ False , False , True , False ]),
+ ('audit network inet stream,' , [ False , False , True , True ]),
+ ( 'network,' , [ False , False , False , False ]),
+ ('audit network,' , [ False , False , False , False ]),
+ ('network unix,' , [ False , False , False , False ]),
+ ]
+
+
+class NetworkCoveredTest_03(NetworkCoveredTest):
+ rule = 'network inet stream,'
+
+ tests = [
+ # rule equal strict equal covered covered exact
+ ( 'network inet stream,' , [ True , True , True , True ]),
+ ('allow network inet stream,' , [ True , False , True , True ]),
+ ( 'network inet,' , [ False , False , False , False ]),
+ ( 'network,' , [ False , False , False , False ]),
+ ( 'network inet tcp,' , [ False , False , False , False ]),
+ ('audit network,' , [ False , False , False , False ]),
+ ('audit network inet stream,' , [ False , False , False , False ]),
+ ( 'network unix,' , [ False , False , False , False ]),
+ ( 'network,' , [ False , False , False , False ]),
+ ]
+
+class NetworkCoveredTest_04(NetworkCoveredTest):
+ rule = 'network,'
+
+ tests = [
+ # rule equal strict equal covered covered exact
+ ( 'network,' , [ True , True , True , True ]),
+ ('allow network,' , [ True , False , True , True ]),
+ ( 'network inet,' , [ False , False , True , True ]),
+ ( 'network inet6 stream,' , [ False , False , True , True ]),
+ ( 'network tcp,' , [ False , False , True , True ]),
+ ( 'network inet raw,' , [ False , False , True , True ]),
+ ('audit network,' , [ False , False , False , False ]),
+ ('deny network,' , [ False , False , False , False ]),
+ ]
+
+class NetworkCoveredTest_05(NetworkCoveredTest):
+ rule = 'deny network inet,'
+
+ tests = [
+ # rule equal strict equal covered covered exact
+ ( 'deny network inet,' , [ True , True , True , True ]),
+ ('audit deny network inet,' , [ False , False , False , False ]),
+ ( 'network inet,' , [ False , False , False , False ]), # XXX should covered be true here?
+ ( 'deny network unix,' , [ False , False , False , False ]),
+ ( 'deny network,' , [ False , False , False , False ]),
+ ]
+
+
+class NetworkCoveredTest_Invalid(AATest):
+ def test_borked_obj_is_covered_1(self):
+ obj = NetworkRule.parse('network inet,')
+
+ testobj = NetworkRule('inet', 'stream')
+ testobj.domain = ''
+
+ with self.assertRaises(AppArmorBug):
+ obj.is_covered(testobj)
+
+ def test_borked_obj_is_covered_2(self):
+ obj = NetworkRule.parse('network inet,')
+
+ testobj = NetworkRule('inet', 'stream')
+ testobj.type_or_protocol = ''
+
+ with self.assertRaises(AppArmorBug):
+ obj.is_covered(testobj)
+
+ def test_invalid_is_covered(self):
+ obj = NetworkRule.parse('network inet,')
+
+ testobj = BaseRule() # different type
+
+ with self.assertRaises(AppArmorBug):
+ obj.is_covered(testobj)
+
+ def test_invalid_is_equal(self):
+ obj = NetworkRule.parse('network inet,')
+
+ testobj = BaseRule() # different type
+
+ with self.assertRaises(AppArmorBug):
+ obj.is_equal(testobj)
+
+## --- tests for NetworkRuleset --- #
+
+class NetworkRulesTest(AATest):
+ def test_empty_ruleset(self):
+ ruleset = NetworkRuleset()
+ ruleset_2 = NetworkRuleset()
+ self.assertEqual([], ruleset.get_raw(2))
+ self.assertEqual([], ruleset.get_clean(2))
+ self.assertEqual([], ruleset_2.get_raw(2))
+ self.assertEqual([], ruleset_2.get_clean(2))
+
+ def test_ruleset_1(self):
+ ruleset = NetworkRuleset()
+ rules = [
+ 'network tcp,',
+ 'network inet,',
+ ]
+
+ expected_raw = [
+ 'network tcp,',
+ 'network inet,',
+ '',
+ ]
+
+ expected_clean = [
+ 'network inet,',
+ 'network tcp,',
+ '',
+ ]
+
+ for rule in rules:
+ ruleset.add(NetworkRule.parse(rule))
+
+ self.assertEqual(expected_raw, ruleset.get_raw())
+ self.assertEqual(expected_clean, ruleset.get_clean())
+
+ def test_ruleset_2(self):
+ ruleset = NetworkRuleset()
+ rules = [
+ 'network inet6 raw,',
+ 'allow network inet,',
+ 'deny network udp, # example comment',
+ ]
+
+ expected_raw = [
+ ' network inet6 raw,',
+ ' allow network inet,',
+ ' deny network udp, # example comment',
+ '',
+ ]
+
+ expected_clean = [
+ ' deny network udp, # example comment',
+ '',
+ ' allow network inet,',
+ ' network inet6 raw,',
+ '',
+ ]
+
+ for rule in rules:
+ ruleset.add(NetworkRule.parse(rule))
+
+ self.assertEqual(expected_raw, ruleset.get_raw(1))
+ self.assertEqual(expected_clean, ruleset.get_clean(1))
+
+
+class NetworkGlobTestAATest(AATest):
+ def setUp(self):
+ self.maxDiff = None
+ self.ruleset = NetworkRuleset()
+
+ def test_glob_1(self):
+ self.assertEqual(self.ruleset.get_glob('network inet,'), 'network,')
+
+ # not supported or used yet
+ # def test_glob_2(self):
+ # self.assertEqual(self.ruleset.get_glob('network inet raw,'), 'network inet,')
+
+ def test_glob_ext(self):
+ with self.assertRaises(AppArmorBug):
+ # get_glob_ext is not available for network rules
+ self.ruleset.get_glob_ext('network inet raw,')
+
+class NetworkDeleteTestAATest(AATest):
+ pass
+
+setup_all_loops(__name__)
+if __name__ == '__main__':
+ unittest.main(verbosity=2)
Regards,
Christian Boltz
--
[KDE3 vs. KDE4] My guess is the vocal minority will only be satisfied
when KDE4 gets dropped. We need to let them know that might happen
round about the release of KDE5.4 . [from a comment on
http://blogs.warwick.ac.uk/bweber/entry/opensuse_110_kde4/]
More information about the AppArmor
mailing list