[apparmor] [patch] add tests for set_profile_flags() (and some fun)

[Christian Boltz]

Hello,

this patch adds various tests for set_profile_flags, and documents 
various interesting[tm] things I discovered while writing the tests (see 
the inline comments for details).


The most interesting[tm] thing I found is:
    regex_hat_flag = re.compile('^([a-z]*)\s+([A-Z]*)\s*(#.*)?$') 

I found a match, that, well, probably shouldn't match ;-)
    regex_hat_flag.search('   ')
(yes, really!)

oh, and it does _NOT_ match for real hat definitions like '  ^hat {'

You want more fun? Take this profile

/foo {
dbus
  ,
}

Now run set_profile_flags() on it and, voila:

/foo flags=(complain) {
dbus flags=(complain) {
     ,
 }

Funny, eh?


After so much fun, I also have some tests:


[ 15-add-tests-for-set_profile_flags.diff ]

--- utils/test/test-aa.py       2015-03-05 23:16:18.971117710 +0100
+++ utils/test/test-aa.py       2015-03-05 23:14:41.790946858 +0100
@@ -15,7 +15,7 @@ import shutil
 import tempfile
 from common_test import write_file
 
-from apparmor.aa import check_for_apparmor, get_profile_flags, is_skippable_file, parse_profile_start, serialize_parse_profile_start
+from apparmor.aa import check_for_apparmor, get_profile_flags, set_profile_flags, is_skippable_file, parse_profile_start, serialize_parse_profile_start
 from apparmor.common import AppArmorException, AppArmorBug
 
 class AaTestWithTempdir(unittest.TestCase):
@@ -104,6 +104,100 @@ class AaTest_get_profile_flags(AaTestWit
         with self.assertRaises(AppArmorException):
             self._test_get_flags('/no-such-profile flags=(complain)', 'complain')
 
+class AaTest_set_profile_flags(AaTestWithTempdir):
+    def _test_set_flags(self, old_profile_header, flags, whitespace = '', comment = '', expected_flags = '@- at -@', check_new_flags = True, profile_name='/foo'):
+        self.file = write_file(self.tmpdir, 'profile', '%s%s {%s\n}\n' % (whitespace, old_profile_header, comment))
+        set_profile_flags(self.file, profile_name, flags)
+        if check_new_flags:
+            newflags = get_profile_flags(self.file, profile_name)
+            if expected_flags == '@- at -@':
+                expected_flags = flags
+            self.assertEqual(expected_flags, newflags)
+
+    # tests that actually don't change the flags
+    def test_set_flags_nochange_01(self):
+        self._test_set_flags('/foo', None)
+    def test_set_flags_nochange_02(self):
+        self._test_set_flags('/foo (  complain  )', '  complain  ', whitespace='   ')
+    def test_set_flags_nochange_03(self):
+        self._test_set_flags('/foo (complain)', 'complain')
+    def test_set_flags_nochange_04(self):
+        self._test_set_flags('/foo flags=(complain)', 'complain')
+    def test_set_flags_nochange_05(self):
+        self._test_set_flags('/foo flags=(complain,  audit)', 'complain,  audit', whitespace='     ')
+    def test_set_flags_nochange_06(self):
+        self._test_set_flags('/foo flags=(complain,  audit)', 'complain,  audit', whitespace='     ', comment=' # a comment')
+        # XXX check if the comment is kept
+    def test_set_flags_nochange_07(self):
+        self._test_set_flags('/foo flags=(complain,  audit)', 'complain,  audit', whitespace='     ', comment='\n\n# a comment')
+        # XXX check if the comment is kept
+    def test_set_flags_nochange_08(self):
+        self._test_set_flags('profile /foo flags=(complain)', 'complain')
+    def test_set_flags_nochange_09(self):
+        self._test_set_flags('profile xy /foo flags=(complain)', 'complain', profile_name='xy /foo') # XXX profile_name should be 'xy'
+        #newflags = get_profile_flags(self.file, 'xy /foo')
+        #self.assertEqual(newflags, 'complain')
+
+    # tests that change the flags
+    def test_set_flags_01(self):
+        self._test_set_flags('/foo', 'audit')
+    def test_set_flags_02(self):
+        self._test_set_flags('/foo (  complain  )', 'audit ', whitespace='  ')
+    def test_set_flags_04(self):
+        self._test_set_flags('/foo (complain)', 'audit')
+    def test_set_flags_05(self):
+        self._test_set_flags('/foo flags=(complain)', 'audit')
+    def test_set_flags_06(self):
+        self._test_set_flags('/foo flags=(complain,  audit)', None, whitespace='    ')
+    def test_set_flags_07(self):
+        self._test_set_flags('/foo flags=(complain,  audit)', '', expected_flags=None)
+    def test_set_flags_08(self):
+        with self.assertRaises(AppArmorException):
+            # XXX '  ' should not make the profile invalid
+            self._test_set_flags('/foo flags=(complain,  audit)', '  ', expected_flags=None)
+    def test_set_flags_09(self):
+        self._test_set_flags('profile /foo flags=(complain)', 'audit')
+    def test_set_flags_10(self):
+        self._test_set_flags('profile xy /foo flags=(complain)', 'audit', profile_name='xy /foo') # XXX profile_name should be 'xy'
+
+    # XXX regex_hat_flag in set_profile_flags() is totally broken - it matches for '   ' and '  X ', but doesn't match for ' ^foo {'
+    # oh, it matches on a line like 'dbus' and changes it to 'dbus flags=(...)' if there's no leading whitespace (and no comma)
+    #def test_set_flags_hat_01(self):
+    #    self._test_set_flags('  ^hat {', 'audit')
+
+
+    # XXX current code/regex doesn't check for empty flags
+    #def test_set_flags_invalid_01(self):
+    #    with self.assertRaises(AppArmorBug):
+    #        self._test_set_flags('/foo ()', None, check_new_flags=False)
+    #def test_set_flags_invalid_02(self):
+    #    with self.assertRaises(AppArmorBug):
+    #        self._test_set_flags('/foo flags=()', None, check_new_flags=False)
+    #def test_set_flags_invalid_03(self):
+    #    with self.assertRaises(AppArmorException):
+    #        self._test_set_flags('/foo (  )', '  ', check_new_flags=False)
+
+    def test_set_flags_other_profile(self):
+        # test behaviour if the file doesn't contain the specified /foo profile
+
+        # XXX this silently fails - should it raise an exception instead?
+        self._test_set_flags('/no-such-profile flags=(complain)', 'audit', check_new_flags=False)
+
+        # the file should still not contain a profile for /foo
+        with self.assertRaises(AppArmorException):
+            newflags = get_profile_flags(self.file, '/foo')
+
+        # nothing should change for /no-such-profile
+        newflags = get_profile_flags(self.file, '/no-such-profile')
+        self.assertEqual(newflags, 'complain')
+
+    def test_set_flags_file_not_found(self):
+        # XXX this exits silently
+        # the easiest solution would be to drop the
+        #         if os.path.isfile(prof_filename):
+        # check and let open_file_read raise an exception
+        set_profile_flags('%s/file-not-found' % self.tmpdir, '/foo', 'audit')
+
 
 class AaTest_is_skippable_file(unittest.TestCase):
     def test_not_skippable_01(self):




Regards,

Christian Boltz
-- 
Key Signing Party? Was ist denn das?
Kultiges Zusammensitzen und gemeinsames Murmeln magischer Zahlen.
-- Gert Döring, FdI 95