[apparmor] [patch] Raise AppArmorBug on unknown request_mask in logparser.py

[Steve Beattie]

On Thu, Jan 07, 2016 at 03:21:38PM +0100, Christian Boltz wrote:
> Am Donnerstag, 7. Januar 2016 schrieb Steve Beattie:
> > For trunk/upcoming 2.11, I proposed that we use the following python 3
> > only syntax:
> ...
> > +                except AppArmorException as e:
> > +                    # Drop the original AppArmorException by passing
> > None as the parent exception 
> > +                    raise
> > AppArmorBug('%(msg)s\n\nThis error was caused by the log
> > line:\n%(logline)s' % +                            {'msg': e.value,
> > 'logline': line}) from None 
> 
> Nice, I didn't know the "from None" (or "from other_exception") syntax 
> yet.
> 
> > This results in the following:
> ...
> > Which is what I think you desire. 
> 
> Yes, that's much better than two backtraces.
> 
> In this special case, it's superfluous to display a backtrace at all 
> because the relevant information (the log line) is in the exception 
> message, but it's probably not worth special handling.

I agree, though I think it *could* be done in apparmor.fail by
having the AppArmorBug class take an extra initializer option that
displays tracebacks or not, which apparmor.fail.handle_exception()
would then honor.

> > But it also means we need to get
> > serious about making the code base python3 only (e.g. use pyflakes3
> > instead of pyflakes, etc.).
> > 
> > What do you think?
> 
> I tend to add another version check ;-)
> 
> My plan is:
> - officially deprecate (but still support) py2 in 2.11 - and also give 
>   people some time to switch from aa-status --enabled to aa-enabled
>   (BTW: You didn't commit your aa-status patch to make apparmor.fail 
>   optional yet.)
> - no longer worry about py2 in 2.12
> 
> That said, here's the updated patch:
> 
> 
> [ 38-more-useful-logparser-failure-reports.diff ]
> 
> === modified file ./utils/apparmor/logparser.py
> --- utils/apparmor/logparser.py 2016-01-07 14:53:52.853018485 +0100
> +++ utils/apparmor/logparser.py 2016-01-07 15:11:58.033710237 +0100
> @@ -17,7 +17,7 @@
>  import sys
>  import time
>  import LibAppArmor
> -from apparmor.common import AppArmorException, open_file_read, DebugLogger
> +from apparmor.common import AppArmorException, AppArmorBug, open_file_read, DebugLogger
>  
>  from apparmor.aamode import validate_log_mode, log_str_to_mode, hide_log_mode, AA_MAY_EXEC
>  
> @@ -413,7 +413,17 @@
>              event = self.parse_log_record(line)
>              #print(event)
>              if event:
> -                self.add_event_to_tree(event)
> +                try:
> +                    self.add_event_to_tree(event)
> +                except AppArmorException as e:
> +                    ex_msg = ('%(msg)s\n\nThis error was caused by the log line:\n%(logline)s' %
> +                            {'msg': e.value, 'logline': line})
> +                    if sys.version_info < (3, 0):
> +                        raise AppArmorBug(ex_msg)
> +                    else:
> +                        # Drop the original AppArmorException by passing None as the parent exception
> +                        raise AppArmorBug(ex_msg) from None
> +
>          self.LOG.close()
>          self.logmark = ''
>          return self.log
> 
> 
> Since this patch should work with py2 and py3, I propose it for trunk,
> 2.10 and 2.9.

NACK, this version won't work with python2 as it doesn't understand the
python3 from Exception syntax, which it still attempts to parse even if
it won't invoke it:

  $ PYTHON_PATH=. python2 aa-logprof -f <(echo 'Dec 11 10:24:07 gw-dc01 kernel: [2214272.912766] type=1400 audit(1449822247.549:21251): apparmor="ALLOWED" operation="file_inherit" profile="/usr/sbin/smbd" name="/foo/bar" pid=7112 comm="nsupdate" requested_mask="foo" denied_mask="foo" fsuid=0 ouid=0')
  Traceback (most recent call last):
    File "aa-logprof", line 18, in <module>
      import apparmor.aa as apparmor
    File "/home/steve/bzr/apparmor-master/utils/apparmor/aa.py", line 29, in <module>
      import apparmor.logparser
    File "/home/steve/bzr/apparmor-master/utils/apparmor/logparser.py", line 410
      raise AppArmorBug(ex_msg) from None
                                   ^
  SyntaxError: invalid syntax

In order to make this work, you'd need to some kind of conditional
import based on python major version that defined the functionality of a
(e.g.) my_raise_from() function. Which is kind of ugly.

(It also does not look like there's a from __future__ import foo that
would get the accepted 'raise from' syntax enabled in 2.7.)

> As I wrote above, I'll replace \n in ex_msg with spaces in 2.9 because
> it can't handle \n in exception messages.

Yeah, that part's fine, ugly traceback with log message embedded is
better than one without.

-- 
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/20160108/691111bb/attachment-0001.pgp>