[apparmor] [patch] aa.py: split off parse_profile_start() from parse_profile_data() and add tests
Christian Boltz
apparmor at cboltz.de
Sun Mar 1 18:12:05 UTC 2015
Hello,
this patch moves the code for parsing the profile start ("/foo {") from
aa.py parse_profile_data() to a separate function parse_profile_start().
Most of the changes are just moving around code, with some small
exceptions:
- instead of handing over profile_data to parse_profile_start() to
modify it, it sets two variables (pps_set_profile and
pps_set_hat_external) as part of its return value, which are then
used in parse_profile_data() to set the flags in profile_data.
- existing_profiles[profile] = file is executed later, which means
it used the strip_quotes() version of profile now
- whitespace / tab level changes
The patch also adds some tests for the parse_profile_start() function.
At the risk of shocking you - since this is a preparation for a bugfix,
I propose this patch for 2.9 and trunk.
I'm also ok with commiting only to trunk if someone else backports it
to 2.9 when all fixes are in. However, let me warn you that this might
be more difficult than commiting to 2.9 instantly ;-)
Notes / background:
Maybe you followed the discussion on #apparmor today, and saw the note
that aa-logprof fails for "profile foo /bin/foo {" style profiles from
one of my other patches.
I found the reason today: RE_PROFILE_START does something else than you
might expect :-/
This means we need quite some changes to get it fixed, and I'd like to
have tests for the current behaviour first to make sure the fixes don't
break too much ;-)
This also means parse_profile_start() will see quite some changes to
get "profile foo /bin/foo {" working.
My plan is to make RE_PROFILE_START even more complex *eg* and wrap
it in a function that gives profile, attachment, flags etc. as result.
BTW: Here's working testcase for the current RE_PROFILE_START.
Just look at the match groups - especially the matches 1, 3 and 6 which
are used in the code. It should be obvious what is broken and why ;-)
(patch for test-regex_matches.py on request, but I doubt it makes sense
to officially document how broken RE_PROFILE_START is ;-)
class AARegexProfileStart(unittest.TestCase):
'''Tests for RE_PROFILE_START'''
def setUp(self):
self.regex = aa.RE_PROFILE_START
tests = [
('/bin/foo ', False), # no {
('profile {', False), # no attachment
# match groups: 0 1 2 3 4 5 6 7
(' /foo {', ('/foo', '/foo', None, None, None, None, None, None )),
(' "/foo" {', ('"/foo"', '/foo', None, None, None, None, None, None )),
(' profile /foo {', ('profile /foo', None, 'profile /foo', '/foo', None, None, None, None )),
(' profile "/foo" {', ('profile "/foo"', None, 'profile "/foo"', '/foo', None, None, None, None )),
(' profile foo /foo {', ('profile foo /foo', None, 'profile foo /foo', 'foo /foo', None, None, None, None )),
(' profile "foo" "/foo" {', ('profile "foo" "/foo"', None, 'profile "foo" "/foo"', 'foo" "/foo', None, None, None, None )),
(' /foo (complain) {', ('/foo', '/foo', None, None, '(complain)', None, 'complain', None )),
(' /foo flags=(complain) {',('/foo', '/foo', None, None, 'flags=(complain)', 'flags=', 'complain', None )),
(' /foo (complain) { # x', ('/foo', '/foo', None, None, '(complain)', None, 'complain', '# x')),
# RE_PROFILE_START = re.compile('^\s*("?(/.+?)"??|(profile\s+"?(.+?)"??))\s+((flags=)?\((.+)\)\s+)?\{' + RE_EOL)
]
[ split-off-parse_profile_start-and-add-tests.diff ]
--- test-aa.py---nur-get-profile-flags 2015-03-01 17:37:24.789364744 +0100
+++ test-aa.py 2015-03-01 18:41:33.062731404 +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
+from apparmor.aa import check_for_apparmor, get_profile_flags, is_skippable_file, parse_profile_start
from apparmor.common import AppArmorException
class AaTestWithTempdir(unittest.TestCase):
@@ -147,6 +147,45 @@ class AaTest_is_skippable_file(unittest.
def test_skippable_13(self):
self.assertTrue(is_skippable_file('README'))
+class AaTest_parse_profile_start(unittest.TestCase):
+ def _parse(self, line, profile, hat):
+ return parse_profile_start(line, 'somefile', 1, profile, hat)
+ # (profile, hat, flags, in_contained_hat, pps_set_profile, pps_set_hat_external)
+
+ def test_parse_profile_start_01(self):
+ result = self._parse('/foo {', None, None)
+ expected = ('/foo', '/foo', None, False, False, False)
+ self.assertEqual(result, expected)
+
+ def test_parse_profile_start_02(self):
+ result = self._parse('/foo (complain) {', None, None)
+ expected = ('/foo', '/foo', 'complain', False, False, False)
+ self.assertEqual(result, expected)
+
+ def test_parse_profile_start_03(self):
+ result = self._parse('profile foo /foo {', None, None) # named profile
+ expected = ('foo /foo', 'foo /foo', None, False, False, False) # XXX yes, that's what happens with the current code :-/
+ self.assertEqual(result, expected)
+
+ def test_parse_profile_start_04(self):
+ result = self._parse('profile /foo {', '/bar', '/bar') # child profile
+ expected = ('/bar', '/foo', None, True, True, False)
+ self.assertEqual(result, expected)
+
+ def test_parse_profile_start_05(self):
+ result = self._parse('/foo//bar {', None, None) # external hat
+ expected = ('/foo', 'bar', None, False, False, True)
+ self.assertEqual(result, expected)
+
+
+ def test_parse_profile_start_invalid_01(self):
+ with self.assertRaises(AppArmorException):
+ result = self._parse('/foo {', '/bar', '/bar') # child profile without profile keyword
+
+ def test_parse_profile_start_invalid_02(self):
+ with self.assertRaises(AttributeError): # XXX does this need error handling in parse_profile_start?
+ result = self._parse('xy', '/bar', '/bar') # not a profile start
+
if __name__ == '__main__':
unittest.main(verbosity=2)
=== modified file 'utils/apparmor/aa.py'
--- utils/apparmor/aa.py 2015-02-20 20:36:55 +0000
+++ utils/apparmor/aa.py 2015-03-01 17:42:11 +0000
@@ -2632,6 +2642,49 @@
for p in profile_data.keys():
profiles[p] = deepcopy(profile_data[p])
+
+def parse_profile_start(line, file, lineno, profile, hat):
+ print (line)
+ print(" profile %s hat %s file %s" % (profile, hat, file))
+ matches = RE_PROFILE_START.search(line).groups()
+
+ pps_set_profile = False
+ pps_set_hat_external = False
+
+ if profile:
+ #print(profile, hat)
+ if profile != hat or not matches[3]:
+ raise AppArmorException(_('%(profile)s profile in %(file)s contains syntax errors in line: %(line)s.') % { 'profile': profile, 'file': file, 'line': lineno + 1 })
+ # Keep track of the start of a profile
+ if profile and profile == hat and matches[3]:
+ # local profile
+ hat = matches[3]
+ in_contained_hat = True
+ pps_set_profile = True
+ else:
+ if matches[1]:
+ profile = matches[1]
+ else:
+ profile = matches[3]
+ #print(profile)
+ if len(profile.split('//')) >= 2:
+ profile, hat = profile.split('//')[:2]
+ else:
+ hat = None
+ in_contained_hat = False
+ if hat:
+ pps_set_hat_external = True
+ else:
+ hat = profile
+
+ flags = matches[6]
+
+ profile = strip_quotes(profile)
+ if hat:
+ hat = strip_quotes(hat)
+
+ return (profile, hat, flags, in_contained_hat, pps_set_profile, pps_set_hat_external)
+
def parse_profile_data(data, file, do_include):
profile_data = hasher()
profile = None
@@ -2655,41 +2708,15 @@
lastline = None
# Starting line of a profile
if RE_PROFILE_START.search(line):
- matches = RE_PROFILE_START.search(line).groups()
-
- if profile:
- #print(profile, hat)
- if profile != hat or not matches[3]:
- raise AppArmorException(_('%(profile)s profile in %(file)s contains syntax errors in line: %(line)s.') % { 'profile': profile, 'file': file, 'line': lineno + 1 })
- # Keep track of the start of a profile
- if profile and profile == hat and matches[3]:
- # local profile
- hat = matches[3]
- in_contained_hat = True
+ (profile, hat, flags, in_contained_hat, pps_set_profile, pps_set_hat_external) = parse_profile_start(line, file, lineno, profile, hat, profile_data)
+ if pps_set_profile:
profile_data[profile][hat]['profile'] = True
- else:
- if matches[1]:
- profile = matches[1]
- else:
- profile = matches[3]
- #print(profile)
- if len(profile.split('//')) >= 2:
- profile, hat = profile.split('//')[:2]
- else:
- hat = None
- in_contained_hat = False
- if hat:
- profile_data[profile][hat]['external'] = True
- else:
- hat = profile
+ if pps_set_hat_external:
+ profile_data[profile][hat]['external'] = True
+
# Profile stored
existing_profiles[profile] = file
- flags = matches[6]
-
- profile = strip_quotes(profile)
- if hat:
- hat = strip_quotes(hat)
# save profile name and filename
profile_data[profile][hat]['name'] = profile
profile_data[profile][hat]['filename'] = file
Regards,
Christian Boltz
--
Ähhh, so ein Quark. Egal was du rauchst, es ist zuviel.
[David Haller in opensuse-de]
More information about the AppArmor
mailing list