[apparmor] review r93..95

[Christian Boltz]

Hello,

the review for r93..95 of the new profile tools is attached.

Your commits fixed several bugs, but there are also cases where you 
replaced one bug with another ;-)

I also answered your question about trace handling in the aa-* tools, 
even if my answer where to handle it won't be too surprising ;-)


Regards,

Christian Boltz
-- 
Well, I guess, Stephan knows very well, what the fuzz is about: it's
about hundreds of patches, which will have to be regenerated, done as
an employment-creation measure for this lazy gang of packagers. 
[Hans-Peter Jansen in opensuse-packaging]
-------------- next part --------------
------------------------------------------------------------
revno: 93
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Sat 2014-02-01 06:14:05 +0530
message:
  Some bugfixes for UIYesNo to deny invalid keys, fix autodep when creating new profiles


=== modified file 'Tools/aa-audit'
--- Tools/aa-audit	2013-10-21 21:36:23 +0000
+++ Tools/aa-audit	2014-02-01 00:44:05 +0000


#[merged review for aa-audit r93..95]
#
# r95 message:
#   Fixed the sample --trace feature. Opinions on using it? and should it be implemented in every tool separately?

# not shocking users with a backtrace is a good idea.

# maybe you should only "hide" AppArmor exceptions (they could be called "expected", and the error message in them is usually enough)
# and still display other ("unexpected") exceptions (because we want to know where something fails unexpectedly)

# This should of course ;-) be handled in a central place (tools.py?) instead of adding it to every aa-*



=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-12-19 21:42:58 +0000
+++ apparmor/aa.py	2014-02-01 00:44:05 +0000
@ -418,7 +418,8 @@
                 local_profile[hat]['flags'] = 'complain'
 
     created.append(localfile)
-
+    changed.append(localfile)

# looks good, but...

# python Tools/aa-genprof ~cb/linuxtag/apparmor/scripts/hello 
# Traceback (most recent call last):
#   File "Tools/aa-genprof", line 102, in <module>
#     apparmor.autodep(program)
#   File "/usr/lib/python2.7/site-packages/apparmor/aa.py", line 569, in autodep
#     profile_data = create_new_profile(pname)
#   File "/usr/lib/python2.7/site-packages/apparmor/aa.py", line 421, in create_new_profile
#     changed.append(localfile)
# AttributeError: 'dict' object has no attribute 'append'

# hint (aa.py around line 90)
# changed = dict()
# created = []

@@ -865,34 +869,42 @@
 
 def build_x_functions(default, options, exec_toggle):
     ret_list = []
+    fallback_toggle = False
     if exec_toggle:
         if 'i' in options:
             ret_list.append('CMD_ix')
             if 'p' in options:
                 ret_list.append('CMD_pix')
-                ret_list.append('CMD_EXEC_IX_OFF')
+                fallback_toggle = True
-            elif 'c' in options:
+            if 'c' in options:
                 ret_list.append('CMD_cix')
-                ret_list.append('CMD_EXEC_IX_OFF')
+                fallback_toggle = True
-            elif 'n' in options:
+            if 'n' in options:
                 ret_list.append('CMD_nix')
+                fallback_toggle = True
+            if fallback_toggle:
                 ret_list.append('CMD_EXEC_IX_OFF')

# placing CMD_EXEC_IX_OFF here has a small disadvantage - it will be offered left of CMD_ux (but should be right of it)

-        elif 'u' in options:
+        if 'u' in options:
             ret_list.append('CMD_ux')



=== modified file 'apparmor/common.py'
--- apparmor/common.py	2013-12-29 09:42:30 +0000
+++ apparmor/common.py	2014-02-01 00:44:05 +0000
@@ -201,9 +201,16 @@
 
+def user_perm(prof_dir):

# the function name is not too helpful
# what about "is_writeable()"?
# (or "must_be_writeable()" and let it raise an exception if prof_dir is not writeable, see below)

+    if not os.access(prof_dir, os.R_OK):

# [should check for W_OK, already fixed in r94]

+        sys.stdout.write("Cannot write to profile directory.\n" +
+                         "Please run as a user with appropriate permissions." )

# please make the error message translatable
# I'd also mention prof_dir in the message.
# To make the function useable for everything (not just profile dir), change the message to   "Cannot write to %s"




 class DebugLogger(object):
     def __init__(self, module_name=__name__):
...           
+            try:
+                logging.basicConfig(filename=self.logfile, level=self.debug_level,
+                                    format='%(asctime)s - %(name)s - %(message)s\n')
+            except OSError:
# looks good, but fails with py2
# -> use "except IOError:" instead, that works with py2 and py3

+                # Unable to open the default logfile, so create a temporary logfile and tell use about it
# ... tell use_r_ about it
+                import tempfile
+                templog = tempfile.NamedTemporaryFile('w', prefix='apparmor', suffix='.log' ,delete=False)
+                sys.stdout.write("\nCould not open: %s\nLogging to: %s\n"%(self.logfile, templog.name))

# printing another \n would be nice
               
=== modified file 'apparmor/tools.py'
--- apparmor/tools.py	2013-12-29 09:42:30 +0000
+++ apparmor/tools.py	2014-02-01 00:44:05 +0000
@@ -41,6 +43,9 @@
             if not os.path.isdir(apparmor.profile_dir):
                 raise apparmor.AppArmorException("%s is not a directory." %self.profiledir)
 
+        if not user_perm(apparmor.profile_dir):
+            raise apparmor.AppArmorException("Cannot write to profile directory: %s." %(apparmor.profile_dir))

# user_perm() already prints an error message, so you'll have two at the end...
# you should decide where to print the error message, and maybe raise the exception at the same place that prints the error message

# if you decide that user_perm() should only check the permission without printing any error message,
# this might be one of the rare cases where a separate function doesn't make sense ;-)



vim:ft=diff