[apparmor] [2/3] add testsuite for capability rule class
Christian Boltz
apparmor at cboltz.de
Fri Nov 21 23:38:08 UTC 2014
Hello,
Am Donnerstag, 20. November 2014 schrieb Steve Beattie:
> On Sat, Nov 15, 2014 at 11:44:15PM +0100, Christian Boltz wrote:
> > this patch adds a testsuite for the capability rule class (including
> > the base class). Since I'm writing these classes test-driven, the
> > testsuite covers 100% of the rule/*.py code :-)
> >
> > Note that the testsuite also documents two known issues (commented
> > out):
> >
> > a)
> > If you use covered_raw('capability foo bar,') and your profile has
> > "capability foo, capability bar,", covered_raw will not detect it.
> > The reason for this is that "capability foo, capability bar," is
> > split over two capability_rule objects internally.
> >
> > However it works the other way round - if the profile has
> > "capability
> > foo bar", covered_raw("capability foo") and covered_raw("capability
> > foo bar",) and even "covered_raw("capability bar foo,") will find
> > it.
> >
> > That's a corner case and the only problem it can cause is a
> > superfluous line in a (hand-modified, we don't write
> > multi-capability lines) profile, so I'm not sure if it's worth
> > fixing it
>
> There's also the bit I raised in the earlier patch, that asking to
> delete 'capability foo' will delete the entire 'capability foo bar'
> rule. Or a 'capability,' rule.
I added some tests to cover this.
> > b)
> > When deleting duplicates, "capability," will delete all "capability
> > foo," rules. However it doesn't delete "allow capability foo,"
> > rules.
> > (I didn't check the reason for this yet.)
> >
> > (The included good news is that "capability," is now recogniced as
> > "all capabilities" :-)
> >
> > === added file 'utils/test/test-capability.py'
> > --- utils/test/test-capability.py 1970-01-01 00:00:00 +0000
> > +++ utils/test/test-capability.py 2014-11-15 21:48:16 +0000
> > @@ -0,0 +1,657 @@
> > +#!/usr/bin/env python
> > +#
> > -------------------------------------------------------------------
> > --- +# Copyright (C) 2014 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 apparmor.rule.capability import capability_rule,
> > capability_rules +from apparmor.aa import ALL
> > +from apparmor.common import AppArmorException, AppArmorBug, hasher
> > +from apparmor.logparser import ReadLog
> > +
> > +import re
> > +
> > +# --- tests for single capability_rule --- #
>
> Can you add:
>
> def setUp(self):
> self.maxDiff = None
>
> to each of the classes. so that when the comparison of two large
> objects fails, we see the entire diff in the failure output?
Good idea, done.
> > +class CapabilityTest(unittest.TestCase):
> > + def test_cap_deny_sys_admin(self):
> > + self._compare_obj_with_rawrule(" deny capability
> > sys_admin, # some comment", { + 'allow_keyword':
> > False,
> > + 'deny': True,
> > + 'audit': False,
> > + 'capability': [ 'sys_admin' ],
> > + 'inlinecomment': " # some comment",
> > + })
>
> Don't you want a test for something like
> 'capability sys_admin dac_override,'?
Good idea, test_cap_multi() added.
> > + def test_cap_from_log(self):
> > + parser = ReadLog('', '', '', '', '')
>
> Blech to the ReadLog() interface. But that's a different patch set,
> hopefully.
;-)
> > +class CapabilityRulesCoveredTest(unittest.TestCase):
> > + def test_collection_covered_raw(self):
> > + col = capability_rules()
> > + rules = [
> > + 'capability chown,',
> > + 'capability setuid setgid,',
> > + 'allow capability sys_admin,',
> > + 'audit capability kill,',
> > + 'deny capability chgrp, # example comment',
> > + ]
> > +
> > + for rule in rules:
> > + col.add_raw(rule)
> > +
> > + self.assertTrue(col.covered_raw('capability chown,'))
> > + self.assertTrue(col.covered_raw('capability sys_admin,'))
> > + self.assertTrue(col.covered_raw('allow capability
> > sys_admin,')) + self.assertTrue(col.covered_raw('capability
> > setuid,')) + self.assertTrue(col.covered_raw('allow
> > capability setgid,')) +
> > self.assertTrue(col.covered_raw('capability setgid setuid,')) +
> > # self.assertTrue(col.covered_raw('capability sys_admin
> > chown,')) # fails because it is split over two rule objects
> > internally + self.assertTrue(col.covered_raw('capability
> > kill,')) +
> > + self.assertFalse(col.covered_raw('deny capability chown,'))
> > + self.assertFalse(col.covered_raw('deny capability
> > sys_admin,')) + self.assertFalse(col.covered_raw('deny
> > capability sys_admin chown,')) +
> > self.assertFalse(col.covered_raw('deny capability setgid,')) +
> > self.assertFalse(col.covered_raw('deny capability kill,')) +
> > + self.assertFalse(col.covered_raw('audit capability
> > chown,'))
> > + self.assertFalse(col.covered_raw('audit capability
> > sys_admin,')) + self.assertFalse(col.covered_raw('audit
> > capability sys_admin chown,')) +
> > self.assertFalse(col.covered_raw('audit capability setgid,')) +
> > self.assertTrue(col.covered_raw('audit capability kill,')) +
> > + self.assertTrue(col.covered_raw('deny capability chgrp,'))
> > + self.assertFalse(col.covered_raw('audit deny capability
> > chgrp,')) + self.assertFalse(col.covered_raw('audit
> > capability chgrp,'))
> Since they're all independent, can you break these apart into
> individual test functions, and move the set up of 'col' into the setUp
> function (especially since its the same between the covered_raw and
> covered_raw_2 tests)? That way, if something changes in the
> capability ruleset implementation that causes breakage, you'll know
> which if the asserts that are triggered, not just the first one to
> get hit.
>
> (If you're concerned about the time cost of setting up col
> for each test case, you could use setUpClass and tearDownClass
> https://docs.python.org/2.7/library/unittest.html#setupclass-and-teard
> ownclass but it removes some of the independence between the
> testcases.)
I care more about superfluous and duplicated code ;-)
We discussed this on IRC already some days ago, so I'll follow the
majority and add lots of (IMHO) superfluous
def test_collection_is_raw_covered_XX(self):
lines ;-)
> > + def test_collection_covered_raw_2(self):
> > + col = capability_rules()
> > + rules = [
> > + 'capability chown,',
> > + 'capability setuid setgid,',
> > + 'allow capability sys_admin,',
> > + 'audit capability kill,',
> > + 'deny capability chgrp, # example comment',
> > + ]
> > +
> > + for rule in rules:
> > + col.add_raw(rule)
> > +
> > + event_base = 'type=AVC msg=audit(1415403814.628:662):
> > apparmor="ALLOWED" operation="capable" profile="/bin/ping"
> > pid=15454 comm="ping" capability=13 capname="%s"' +
> > + parser = ReadLog('', '', '', '', '')
> > +
> > +
> > self.assertFalse(col.covered_log(parser.parse_event(event_base%'net
> > _raw'))) +
> > self.assertTrue(col.covered_log(parser.parse_event(event_base%'chow
> > n'))) +
> > self.assertTrue(col.covered_log(parser.parse_event(event_base%'sys_
> > admin'))) +
> > self.assertTrue(col.covered_log(parser.parse_event(event_base%'kill
> > '))) +
> > self.assertFalse(col.covered_log(parser.parse_event(event_base%'chg
> > rp'))) +
> > self.assertTrue(col.covered_log(parser.parse_event(event_base%'chgr
> > p'), False)) # ignores allow/deny
> And similarly split up here.
Done.
> Shouldn't there also be a set of tests for col.covered_obj?
covered_raw and covered_log both use it (after converting their
parameter to a rule object), so I don't think a separate test is needed.
> > +class CapabilityGlobTest(unittest.TestCase):
> > + def test_glob(self):
> > + col = capability_rules()
>
> nit: move 'col = capability_rules()' into setUp().
Done.
> > +class CapabilityDeleteTest(unittest.TestCase):
>
> > + def _setup_testcol(self):
> This should just be setUp(). You'll need to convert all references of
> col to self.col, but then you won't need to manually call the setup
> function at the beginning of each testcase.
Done.
I also renamed "col" to "ruleset" to have an easier to understand
name ;-)
> > + def test_delete_duplicates(self):
> > + col = self._setup_testcol()
> > +
> > + inc = capability_rules()
> > + rules = [
> > + 'capability chown,',
> > + 'deny capability chgrp, # example comment',
> > + ]
> > +
> > + for rule in rules:
> > + inc.add_raw(rule)
>
> Seeing stuff like the above makes me want to have the __init__
> function for rulesets take an option raw_ruleset argument (perhaps
> obj_ and log_ as well), that automatically adds the rules for you, so
> that you'd just write something like:
>
> inc = CapabilityRuleset(raw_ruleset=[
> 'capability chown,',
> 'deny capability chgrp, # example comment',
> ])
As long as we only need it for tests, I don't like that idea too much,
and prefer to use the same method we also use in production code.
Especially because we have several possible input types, which would
make __init__ interesting[tm] to read.
The updated patch (v2) is attached.
Regards,
Christian Boltz
--
PostfixAdmin developer
Infos and Download: http://postfixadmin.sf.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2-add-capability-rule-test.diff
Type: text/x-patch
Size: 27886 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141122/a4de277d/attachment-0001.bin>
More information about the AppArmor
mailing list