[apparmor] [PATCH v2] utils: Basic support for file prefix in path rules

[Christian Boltz]

Hello,

Am Donnerstag, 3. April 2014 schrieb Tyler Hicks:
> Bug: https://bugs.launchpad.net/bugs/1295346
> 
> Add the ability to read and write path rules containing the file
> prefix. This also includes bare "file," rules.
> 
> The ALL global is updated to include a preceding NUL char to eliminate
> possibilities of a real file path colliding with the ALL global.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> ---
>  utils/apparmor/aa.py             | 151
> +++++++++++++++++++++++++++++++++++++--
> utils/test/test-regex_matches.py | 124
> ++++++++++++++++++++++++++++++++ 2 files changed, 270 insertions(+),
> 5 deletions(-)
> 
> diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py
> index 03e28b1..f56d50b 100644
> --- a/utils/apparmor/aa.py
> +++ b/utils/apparmor/aa.py

>+RE_PROFILE_FILE_ENTRY = re.compile('^\s*(audit\s+)?(allow\s+|deny\s+)?
(owner\s+)?file(?:\s+([\"@/].*?)\s+(\S+)(\s+->\s*(.*?))?)?\s*,\s*(#.*)?
$')
> RE_PROFILE_PATH_ENTRY = re.compile('^\s*(audit\s+)?(allow\s+|deny\s+)?
(owner\s+)?([\"@/].*?)\s+(\S+)(\s+->\s*(.*?))?\s*,\s*(#.*)?$')

I don't like that patch too much - basically you copied and pasted the 
path handling, and added the "file" prefix (including bare "file") to 
the copy.

This is a maintenance hell ;-)


Counter-proposal:

RE_PROFILE_PATH_ENTRY = re.compile('^\s*(audit\s+)?(allow\s+|deny\s+)?
(owner\s+)?(file\s+)?([\"@/].*?)\s+(\S+)(\s+->\s*(.*?))?\s*,\s*(#.*)?$')

Note that I only added   (file\s+)?   to the regex so that it will cover 
all file rules except the bare "file," rule.
(Also note this will change the matches numbering ;-)

Also add a regex for bare "file," rule:

RE_PROFILE_BARE_FILE_ENTRY = re.compile('^\s*(audit\s+)?(allow\s+|
deny\s+)?(owner\s+)?file\s*,\s*(#.*)?$')

and then add some (quite simple) code to handle bare file rules - which 
will be much shorter than what you copied ;-)



BTW: Looking at the regex, we should probably switch to something like

RE_AUDIT_DENY = '(audit\s+)?(allow\s+|deny\s+)?'
RE_EOL = '\s*,\s*(#.*)?$'

RE_PROFILE_PATH_ENTRY = re.compile('^\s*' + RE_AUDIT_DENY + 
'(owner\s+)?(file\s+)?([\"@/].*?)\s+(\S+)(\s+->\s*(.*?))?' + RE_EOL)

This would make the important parts of the regexes more readable, and 
also avoid copy&paste errors for common regex parts.

(create-apparmor.vim does something similar already ;-)

> --- a/utils/test/test-regex_matches.py
> +++ b/utils/test/test-regex_matches.py

> @@ -154,6 +157,125 @@ class AARegexCapability(unittest.TestCase):
>          result = aa.RE_PROFILE_CAP.search(line)
>          self.assertFalse(result, 'Found unexpected capability rule in
> "%s"' % line)
> 
> +class AARegexPath(unittest.TestCase):
> +    '''Tests for RE_PROFILE_PATH_ENTRY'''
> +
> +    def test_simple_path_01(self):

As already written several times today, having all tests in an array 
would be a good idea ;-)


Regards,

Christian Boltz
-- 
> Blöde Frage: Platte/Partition voll?  ->  df -h
Tatsächlich. Erklärt mich nicht für blöd, das war auch mein 1.Gedanke,
habe mich dann aber dummerweise bei der Ausgabe von "df -Th" verguckt.
Die root-Partition ist wirklich voll gelaufen. Oh, wie ärgerlich.
Aber vielen Dank für die blöde Frage! ;-)
[> Christian Boltz und Jürgen Pabst in suse-linux]