[apparmor] [patch] add tests for aamode.py

[Steve Beattie]

On Fri, Nov 28, 2014 at 07:47:29PM +0100, Christian Boltz wrote:
> Am Donnerstag, 27. November 2014 schrieb Steve Beattie:
> > If we're going to raise an exception that nobody's going to check for,
> > wuld it be better to raise AppArmorBug, as that seems more
> > appropriate, that either there's a bug in our code or we got a dodgy
> > log message with a weird permission mode. Plus we can attach
> > information about why things went wrong. Patch follows:
> 
> So we replace an exception that nobody is going to check for with 
> another exception (with some additional details) that nobody is going to 
> check for? ;-)

Err? I thought we were going to at some point do some fancy
logging/handling of AppArmorBug exceptions, no? So while in the
short term, it doesn't make much difference except in the detailed
reporting, I thought getting things set up for where you wanted to
go in the future made sense.

> > Signed-off-by: Steve Beattie <steve at nxnw.org>
> > ---
> >  utils/apparmor/aamode.py  |    9 ++++++++-
> >  utils/test/test-aamode.py |    7 +++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > Index: b/utils/apparmor/aamode.py
> > ===================================================================
> > --- a/utils/apparmor/aamode.py
> > +++ b/utils/apparmor/aamode.py
> > @@ -12,6 +12,7 @@
> >  #
> >  #
> > ---------------------------------------------------------------------
> > - import re
> > +from apparmor.common import AppArmorBug
> > 
> >  def AA_OTHER(mode):
> >      other = set()
> > @@ -103,11 +104,17 @@ def split_log_mode(mode):
> >      other = ''
> > 
> >      if "::" in mode:
> > -        user, other = mode.split("::")
> > +        try:
> > +            user, other = mode.split("::")
> > +        except ValueError as e:
> > +            raise AppArmorBug("Got ValueError '%s' when splitting %s"
> > % (str(e), mode)) 
> 
> That gives us a better error message than implicitely raising a 
> ValueError, and isn't measurable in the execution time. So even if I 
> joked about it above, it's an improvement.
> 
> >      else:
> >          user = mode
> >          other = mode
> >      #print ('split_logmode:', user, mode)
> 
> Unrelated: please delete the "#print" line before commiting.

I didn't add it, but sure.

> > +    if ":" in user or ":" in other:
> > +        raise AppArmorBug("After splitting %s, user (%s) or other
> > (%s) contained ':' " % (mode, user, other)) 
> 
> This check makes aa-logprof slightly slower (~2% - 1:10 instead of 1:09 
> runtime for 20 aa-logprof runs on a 4 MB audit.log).
> 
> Valid data might be worth that - OTOH, the function doesn't check if 
> user or other contain unknown letters, numbers or even special chars 
> (splitting "yes!!11!!::no§42" will work successfully, at least until you 
> try to use the result ;-)
> 
> So I'm undecided if adding this check really makes sense - given that we 
> only check for ":" and allow any other strange character, it feels like 
> a drop in the ocean ;-)

Well, do we check for sanity on permissions anywhere else that would
detect if we got a wonky log entry? Somewhere we should ensure that we
don't write out garbage to a profile, right?

> >  def mode_contains(mode, subset):
> > Index: b/utils/test/test-aamode.py
> > ===================================================================
> > --- a/utils/test/test-aamode.py
> > +++ b/utils/test/test-aamode.py
> > @@ -12,6 +12,7 @@
> >  import unittest
> > 
> >  from apparmor.aamode import split_log_mode, sub_str_to_mode
> > +from apparmor.common import AppArmorBug
> > 
> >  class AamodeTest_split_log_mode(unittest.TestCase):
> >      def test_split_log_mode_1(self):
> > @@ -26,6 +27,12 @@ class AamodeTest_split_log_mode(unittest
> >          self.assertEqual(split_log_mode('r::w'), ('r', 'w'))
> >      def test_split_log_mode_6(self):
> >          self.assertEqual(split_log_mode('rw::rw'), ('rw', 'rw'))
> > +    def test_split_log_mode_invalid_1(self):
> > +        with self.assertRaises(AppArmorBug):
> > +            split_log_mode('r::w::r')
> > +    def test_split_log_mode_invalid_2(self):
> > +        with self.assertRaises(AppArmorBug):
> > +            split_log_mode('r:::r')
> 
> With or without the check for ":":
>     Acked-by: Christian Boltz <apparmor at cboltz.de>
> 
> (If you don't include the check for ":", you'll also have to remove 
> test_split_log_mode_invalid_2() test)

I'll drop it for now.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141129/841a8d0a/attachment.pgp>