[apparmor] GSoC review r54

Christian Boltz apparmor at cboltz.de
Thu Sep 5 16:21:44 UTC 2013


Hello,

the review for r54 is attached.

One interesting question about globbing:
    /**/ -> /**/  (hmm, or would /** be correct? good question...)

Opinions?


Regards,

Christian Boltz
-- 
Super-PC von IBM - der erste 486er im Test
[Titelseite der Chip 8/1989]
-------------- next part --------------
------------------------------------------------------------
revno: 54
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Fri 2013-08-30 03:54:31 +0530
message:
  Fixes from review 52-53, merging cleanprof into apparmor/tools.py corrected
  enforce() and complain() to create/remove symlinks to force-complain/disable
  subdirs. Wrote some tests for globbing methods, segregated glob-path and
  glob-path-with-extension into methods in aa.py


=== modified file 'Testing/aa_test.py'
--- Testing/aa_test.py	2013-08-11 13:00:01 +0000
+++ Testing/aa_test.py	2013-08-29 22:24:31 +0000
@@ -5,13 +5,68 @@
 import apparmor.aa
 import apparmor.logparser
 
-#from apparmor.aa import parse_event
-
 class Test(unittest.TestCase):
+    
+    def setUp(self):
+        self.MODE_TEST = {'x': apparmor.aamode.AA_MAY_EXEC, 

# good, but test_string_to_modes() still has its own set of MODE_TEST
# (and basically does nothing because the actual test is commented)

     def test_loadinclude(self):
         apparmor.aa.loadincludes()
     
+    def test_path_globs(self):
+        globs = {

# please also test for
# /** -> /**
# /**/ -> /**/  (hmm, or would /** be correct? good question...)
# /usr/bin/foo**bar -> /usr/bin/**
# /usr/bin/foo*bar -> /usr/bin/*
# /usr/bin/*foo* -> /usr/bin/*

+                 }
+        for path in globs.keys():
+            self.assertEqual(apparmor.aa.glob_path(path), globs[path], 'Unexpected glob generated for path: %s'%path)

# printing the actual glob result would be helpful

+    def test_path_withext_globs(self):
+        globs = {

# please also test for
# /usr/*foo*.bar -> /usr/*.bar
# /usr/fo*oo.bar -> /usr/*.bar

+        for path in globs.keys():
+            self.assertEqual(apparmor.aa.glob_path_withext(path), globs[path], 'Unexpected glob generated for path: %s'%path)

# printing the actual glob result would be helpful


=== added file 'Testing/minitools_test.py'
--- Testing/minitools_test.py	1970-01-01 00:00:00 +0000
+++ Testing/minitools_test.py	2013-08-29 22:24:31 +0000

+    def setUp(self):
+        #copy the local profiles to the test directory
+        shutil.copytree('/etc/apparmor.d/', './profiles/')

# you should take the profiles from bzr or the tarball
# (taking them from /etc/ will give you user-modified profiles and therefore random test "failures")
# for now (until your tools are in the official apparmor bzr), I'd say go for the path '../profiles/apparmor.d/' 
# and expect users to symlink it to the profile directory of a recent bzr trunk checkout

=== modified file 'Testing/severity_test.py'
--- Testing/severity_test.py	2013-07-28 02:53:46 +0000
+++ Testing/severity_test.py	2013-08-29 22:24:31 +0000
@@ -7,6 +8,10 @@
+    def setUp(self):
+        #copy the local profiles to the test directory
+        shutil.copytree('/etc/apparmor.d/', './profiles/')

# you should take the profiles from bzr or the tarball
# (taking them from /etc/ will give you user-modified profiles and therefore random test "failures")
# for now (until your tools are in the official apparmor bzr), I'd say go for the path '../profiles/apparmor.d/' 
# and expect users to symlink it to the profile directory of a recent bzr trunk checkout

=== added file 'Tools/aa-mergeprof.py'
# still empty

=== modified file 'Tools/aa-unconfined.py'
--- Tools/aa-unconfined.py	2013-08-25 18:53:59 +0000
+++ Tools/aa-unconfined.py	2013-08-29 22:24:31 +0000
@@ -47,19 +47,19 @@
         pname = '(%s)'%pname
     else:
         pname = ''
+    regex_interpreter = re.compile('^(/usr)?/bin/(python|perl|bash|dash|sh)$')

# better, but I'd still prefer to have the list of shells/scripting languages at one place

=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-08-25 18:53:59 +0000
+++ apparmor/aa.py	2013-08-29 22:24:31 +0000
@@ -226,16 +226,46 @@
+def set_complain(filename, program, ):
+    """Sets the profile to complain mode"""
+    UI_Info('Setting %s to complain mode.' % program)
+    create_symlink('force-complain', filename)
+    change_profile_flags(filename, 'complain', True)

# creating a symlink _and_ changing the flags is superfluous - one of them is enough
# (IIRC we decided to default to using symlinks, and make it a commandline and/or config option)

+def delete_symlink(subdir, filename):
+    path = filename
+    link = re.sub('^%s'%profile_dir, '%s/%s'%(profile_dir, subdir), path)
+    if link != path and os.path.islink(link):
+        os.remove(link)

# a warning if link is not a symlink would be nice

+def create_symlink(subdir, filename):
+    path = filename
+    bname = os.path.basename(filename)
+    if not bname:
+        raise AppArmorException(_('Unable to find basename for %s.')%filename)
+    link = re.sub('^%s'%profile_dir, '%s/%s'%(profile_dir, subdir), path)
+    link = link + '/%s'%bname
+    if not os.path.exists(link):

# a warning if link exists, but is not a symlink, would be nice

+        try:
+            os.symlink(filename, link)
+        except:
+            raise AppArmorException('Could not create %s symlink to %s.'%(link, filename))
     
@@ -2426,7 +2457,7 @@
 def is_skippable_dir(path):
-    if path in ['disable', 'cache', 'force-complain', 'lxc']:
+    if re.search('(disable|cache|force-complain|lxc)', path):

# your regex also matches /etc/apparmor.d/bin.do-not-disable-me ;-)
# ^...$ is probably wrong here, but /.../ should work

         return True
     return False
 
=== modified file 'apparmor/tools.py'
--- apparmor/tools.py	2013-08-25 18:53:59 +0000
+++ apparmor/tools.py	2013-08-29 22:24:31 +0000
@ -41,46 +44,72 @@
                 which = apparmor.which(p)
                 if which:
                     program = apparmor.get_full_path(which)
-                               
-            if os.path.exists(program):
+            
+            if not os.path.exists(program):
+                apparmor.UI_Info(_('The given program cannot be found, please try with the fully qualified path name of the program: '))
+                program = apparmor.UI_GetString('', '')

# that's funny ;-)

#   # python3 aa-complain /sbin/syslogd
#	The given program cannot be found, please try with the fully qualified path name of the program:

# hint: you should only ask if program is not a fully qualified path ;-)

+                        # One simply does not walk in here!
+                        raise apparmor.AppArmorException('Unknown tool.')

# printing the tool name could make debugging easier ;-)

       

vim:ft=diff


More information about the AppArmor mailing list