[apparmor] [PATCH 1/3] utils: Basic support for signal rules

[Christian Boltz]

Hello,

Am Donnerstag, 3. April 2014 schrieb Tyler Hicks:
> Bug: https://bugs.launchpad.net/bugs/1300316
> 
> This patch does bare bones parsing of signal rules and stores the raw
> strings for writing them out later. It is meant to be a simple change
> to prevent aa.py from emitting a traceback when encountering signal
> rules.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>

In general,
Acked-By: Christian Boltz <apparmor at cboltz.de>

However,

> diff --git a/utils/test/test-regex_matches.py
> b/utils/test/test-regex_matches.py index 0b656cc..7096a50 100644
> --- a/utils/test/test-regex_matches.py
> +++ b/utils/test/test-regex_matches.py

> @@ -276,6 +283,88 @@ class AARegexFile(unittest.TestCase):
>          result = aa.RE_PROFILE_FILE_ENTRY.search(line)
>          self.assertFalse(result, 'RE_PROFILE_FILE_ENTRY unexpectedly
> matched "%s"' % line)
> 
> +class AARegexSignal(unittest.TestCase):
> +    '''Tests for RE_PROFILE_SIGNAL'''
> +
> +    def test_bare_signal_01(self):
> +        '''test '   signal,' '''
> +
> +        rule = 'signal,'
> +        line = '   %s' % rule
> +        result = aa.RE_PROFILE_SIGNAL.search(line)
> +        self.assertTrue(result, 'Couldn\'t find signal rule in "%s"'
> % line) +        parsed = result.groups()[2].strip()
> +        self.assertEqual(parsed, rule, 'Expected signal rule "%s",
> got "%s"' +                         % (rule, parsed))
> +
> +    def test_bare_signal_02(self):
> +        '''test '   audit signal,' '''
> +
> +        rule = 'signal,'
> +        line = '   audit %s' % rule
> +        result = aa.RE_PROFILE_SIGNAL.search(line)
> +        self.assertTrue(result, 'Couldn\'t find signal rule in "%s"'
> % line) 
> +        self.assertTrue(result.groups()[0], 'Couldn\'t find
> audit modifier in "%s"' % line) +        parsed =
> result.groups()[2].strip()
> +        self.assertEqual(parsed, rule, 'Expected signal rule "%s",
> got "%s"' +                         % (rule, parsed))
[... and some more test.*signal.* functions ...]

You have lots of duplicated code. I'd recommend to put everything into 
an array and loop over it.

Basically, you need the following information for each test:

- the rule you want to test, for example 'audit signal send'
- the expected 'audit' status: True or False (BTW: your tests never 
  check for audit == False in lines not containing "audit" - you would 
  get that "for free" if you loop over everything ;-)
- the expected 'deny' status: True or False
- the expected rule details, 'send' in my example

If you don't like arrays ;-) it might also be a good idea to store the 
testcases in an INI-formatted file, like we already do with 
regex_tests.ini.

> diff --git a/utils/test/test-signal_parse.py
> b/utils/test/test-signal_parse.py new file mode 100644
> index 0000000..392fb32
> --- /dev/null
> +++ b/utils/test/test-signal_parse.py
> @@ -0,0 +1,63 @@
> +#! /usr/bin/env python
> +# ------------------------------------------------------------------
> +#
> +#    Copyright (C) 2014 Canonical Ltd.
> +#
> +#    This program is free software; you can redistribute it and/or
> +#    modify it under the terms of version 2 of the GNU General Public
> +#    License published by the Free Software Foundation.
> +#
> +# ------------------------------------------------------------------
> +
> +import apparmor.aa as aa
> +import unittest
> +
> +class AAParseSignalTest(unittest.TestCase):
> +
> +    def _test_parse_signal_rule(self, rule):
> +        signal = aa.parse_signal_rule(rule)
> +        print(signal.serialize())
> +        self.assertEqual(rule, signal.serialize(),
> +                'signal object returned "%s", expected "%s"' %
> (signal.serialize(), rule)) +
> +    def test_parse_plain_signal_rule(self):
> +        self._test_parse_signal_rule('signal,')
> +
> +    def test_parse_receive_signal_rule(self):
> +        self._test_parse_signal_rule('signal (receive),')
> +
> +    def test_parse_send_signal_rule(self):
> +        self._test_parse_signal_rule('signal (send),')
> +
> +    def test_parse_send_receive_signal_rule(self):
> +        self._test_parse_signal_rule('signal (send receive),')
> +
> +    def test_parse_r_signal_rule(self):
> +        self._test_parse_signal_rule('signal r,')
> +
> +    def test_parse_w_signal_rule(self):
> +        self._test_parse_signal_rule('signal w,')
> +
> +    def test_parse_rw_signal_rule(self):
> +        self._test_parse_signal_rule('signal rw,')
> +
> +    def test_parse_set_1_signal_rule(self):
> +        self._test_parse_signal_rule('signal send set=("hup"),')
> +
> +    def test_parse_set_2_signal_rule(self):
> +        self._test_parse_signal_rule('signal (receive) set=kill,')
> +
> +    def test_parse_set_3_signal_rule(self):
> +        self._test_parse_signal_rule('signal w set=(quit int),')
> +
> +    def test_parse_peer_1_signal_rule(self):
> +        self._test_parse_signal_rule('signal receive peer=foo,')
> +
> +    def test_parse_peer_2_signal_rule(self):
> +        self._test_parse_signal_rule('signal (send receive)
> peer=/usr/bin/bar,') +
> +    def test_parse_peer_3_signal_rule(self):
> +        self._test_parse_signal_rule('signal wr set=(pipe, usr1)
> peer=/sbin/baz,') +

That's also SHOUTING for an array of rules to test, and a loop to check 
each rule ;-)

(Feel free to commit your patch as sent, and then do those changes as 
follow-up patch.)


Regards,

Christian Boltz
-- 
> > > Ein Update auf eine EIN JAHR alte Version?
> > Ich denke er hat einfach auf das geupdated, was bei Debian derzeit
> > als "aktuell" ausgeliefert wird...
> Ja, ist mir dann auch aufgegangen.
Immer diese "Debian-Hasser". :)
[>> nighthawk, >(>>) Ralf Hildebrandt und crandler in postfixbuch-users]