[apparmor] [patch] Add SignalRule and SignalRuleset classes
Kshitij Gupta
kgupta8592 at gmail.com
Thu Nov 19 20:24:30 UTC 2015
Looks good.
Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>
On Thu, Nov 19, 2015 at 11:11 PM, Christian Boltz <apparmor at cboltz.de>
wrote:
> Hello,
>
> [scroll down for an add-on patch that addresses Kshitij's comments]
>
> Am Donnerstag, 19. November 2015 schrieb Kshitij Gupta:
> > On Fri, Oct 23, 2015 at 6:30 PM, Christian Boltz wrote:
> > > this patch adds the SignalRule and SignalRuleset classes
>
> > > [ 07-add-SignalRule-and-SignalRuleset.diff ]
> > >
> > > === modified file ./utils/apparmor/rule/signal.py
> > > --- utils/apparmor/rule/signal.py 2015-10-23
> > > 01:17:21.579245521 +0200 +++ utils/apparmor/rule/signal.py
> > > 2015-10-23 01:08:01.149132984 +0200 @@ -0,0 +1,300 @@
> ...
> > > + 'io', 'pwr', 'sys', 'emt', 'exists']
> > > +RE_SIGNAL_REALTIME =
> > > re.compile('^rtmin\+0*([0-9]|[12][0-9]|3[0-2])$') #
> > > rtmin+0..rtmin+32, number may have leading zeros
> >
> > I do not like this regex.
> > Its far too complicated for when its only saying: rtmin+x such that x
> > maybe 2digit and is in [0,32] and possibly return x. Plus Its
> > confusing whether rtmin+032 is allowed or not (regex suggests it is).
>
> It is allowed. Even rtmin+000000000000000000000000000000000000000000032
> is allowed ;-)
>
> > Maybe its easier(more readably) done in a function with some int() and
> > boundary tests.
>
> We have more interesting regexes, and that one is not too terrible IMHO.
>
> With your proposal, the regex would probably be ^rtmin\+([0-9]+)$ and a
> boundary check at another place. I slightly doubt that makes the code
> more readable ;-)
>
> > > +joint_access_keyword = '\s*(' + '|'.join(access_keywords) + ')\s*'
> >
> > Better[tm] written as:
> > joint_access_keyword = ''\s*(%s)\s*' % ('|'.join(access_keywords))
>
> If it would be the only regex in the file, I'd fully agree. However the
> other regexes nearby are composed using + to keep them readable.
> Therefore I tend to keep using + also here ;-)
>
> > > +RE_ACCESS_KEYWORDS = ( joint_access_keyword + # one of the
> > > access_keyword or
> > > + '|' +
> > > # or
> > > + '\(' + joint_access_keyword + '(' +
> > > '(\s|,)+' + joint_access_keyword + ')*' + '\)' # one or more
> > > signal_keyword in (...) + )
> >
> > Thats some beast!
>
> Thanks to the syntax of signal rules - I don't like some details, but
> obviously have to support all of them. (You missed some funny rants
> about that on IRC ;-)
>
> > > allow_keyword=allow_keyword, +
> > > comment=comment, +
> > > log_event=log_event) +
> > > + self.access, self.all_accesss, unknown_items =
> > > check_and_split_list(access, access_keywords, SignalRule.ALL,
> > > 'SignalRule', 'access')
> >
> > all_accesss (three s's) ;-)
> > I hope that was intentional.
>
> Not really, but at least it's consistent across signal.py (thanks
> autocompletion!) and therefore "just" a strange variable name without
> causing bugs.
>
> Nevertheless, I'll remove the middle s.
>
> > > + if RE_SIGNAL_REALTIME.match(item):
> > > + self.signal.add(item)
> > > + else:
> > > + raise AppArmorException('Passed unknown signal
> > > keyword to SignalRule: %s' % item)
> >
> > Missing _().
> > AppArmorExceptions are expected to have translations while for
> > AppArmorBug we dont, right?
>
> Right, I'll change that (here and some lines below)
>
> > > + if details.group('access'):
> > > + access = details.group('access')
> > > + access = ' '.join(access.split(',')) # split by
> > > ',' or whitespace
> >
> > Is it expected to split strings separated by a , or whitespace? This
> > part will only split strings separated by comma.
> > It can't do both(which the comment confused me to believe until I read
> > on and saw the space split ;-) ).
>
> Actually the comment describes what the code does, but I changed the
> code after writing the comment ;-)
>
> This line splits at the comma and re-joins with a space as delimiter.
> After looking at it again, .replace() should be enough (and probably
> faster). Also, replacing the comma and split() should be done nearby.
>
> I'll change that to a single line so that the comment fits again ;-)
>
> > > + if access.startswith('(') and access.endswith(')'):
> > > + access = access[1:-1]
> > > + access = access.split()
> >
> > There's the space separated split.
>
> Yes, after the (sometimes optional) "(...)" wrapper was removed.
> And that's also the place where the comma split/replacement should and
> will happen.
>
> > > + if not self.all_peers:
> > > + if other_rule.all_peers:
> > > + return False
> > > + if other_rule.peer != self.peer: # XXX use AARE
> > > + return False
> > > +
> > > + # still here? -> then it is covered
> >
> > code seems surprised seeing one here ;-)
>
> Well, it had enough changes to hit a "return False" in all the checks
> above ;-) - and the comment is needed to explain the "return True" in
> the next line.
>
> > > + return True
>
>
> > > === modified file ./utils/test/test-signal.py
> > > --- utils/test/test-signal.py 2015-10-23 01:17:35.102452075 +0200
> > > +++ utils/test/test-signal.py 2015-10-23 13:29:07.054183515 +0200
> > > @@ -0,0 +1,576 @@
> ...
> > > + ('deny signal send,' , [ False , False
> > > , False , False ]),
> > > + ]
> > > +
> > > +
> > > +
> > > +
> >
> > Too much white-space
>
> Indeed. I'll remove whitespace here and at some more places.
>
>
> > The tests mostly look nice and comprehensive(I've superficially
> > looked at them). Yay for the awesome coverage!
> >
> > Looks good.
> >
> > I can understand the patch being pending for so long due to its sheer
> > size.
>
> Come on, signal.py has only 300 lines - that's not too much for a
> totally new feature ;-) but yes, I know it needs more review time than
> a one-line change.
>
>
>
> Here's the patch to address your comments.
> Since you have already acked those changes in advance in your next mail,
> I'll assume it's acked if nobody complains within a day ;-)
>
>
> [patch] Some adjustments for SignalRule based on Kshitij's comments
>
> - add a missing comment to explain RE_SIGNAL_DETAILS
> - rename self.all_accesss to self.all_access (also in the tests)
> - make all AppArmorExceptions translatable
> - make splitting the access keywords one line to fit the comment
> - remove some superfluous empty lines from test-signal.py
>
>
> I'll commit this together with 07-add-SignalRule-and-SignalRuleset.diff
>
> (Pre-)Acked-by: Kshitij Gupta <kgupta8592 at gmail.com>
>
>
> [ 22-signal-rule-adjustments.diff ]
>
> === modified file ./utils/apparmor/rule/signal.py
> --- utils/apparmor/rule/signal.py 2015-11-19 17:42:26.325879118 +0100
> +++ utils/apparmor/rule/signal.py 2015-11-19 18:06:46.376422794 +0100
> @@ -53,7 +53,7 @@
> '^' +
> '(\s+(?P<access>' + RE_ACCESS_KEYWORDS + '))?' + # optional access
> keyword(s)
> '(?P<signal>' + '(\s+(' + RE_SIGNAL_KEYWORDS + '))+' + ')?' + #
> optional signal set(s)
> - '(\s+(peer=' + RE_PROFILE_NAME % 'peer' + '))?' +
> + '(\s+(peer=' + RE_PROFILE_NAME % 'peer' + '))?' + # optional peer
> '\s*$')
>
>
> @@ -80,9 +80,9 @@
> comment=comment,
> log_event=log_event)
>
> - self.access, self.all_accesss, unknown_items =
> check_and_split_list(access, access_keywords, SignalRule.ALL, 'SignalRule',
> 'access')
> + self.access, self.all_access, unknown_items =
> check_and_split_list(access, access_keywords, SignalRule.ALL, 'SignalRule',
> 'access')
> if unknown_items:
> - raise AppArmorException('Passed unknown access keyword to
> SignalRule: %s' % ' '.join(unknown_items))
> + raise AppArmorException(_('Passed unknown access keyword to
> SignalRule: %s') % ' '.join(unknown_items))
>
> self.signal, self.all_signals, unknown_items =
> check_and_split_list(signal, signal_keywords, SignalRule.ALL, 'SignalRule',
> 'signal')
> if unknown_items:
> @@ -90,7 +90,7 @@
> if RE_SIGNAL_REALTIME.match(item):
> self.signal.add(item)
> else:
> - raise AppArmorException('Passed unknown signal
> keyword to SignalRule: %s' % item)
> + raise AppArmorException(_('Passed unknown signal
> keyword to SignalRule: %s') % item)
>
> self.peer = None
> self.all_peers = False
> @@ -129,10 +129,9 @@
>
> if details.group('access'):
> access = details.group('access')
> - access = ' '.join(access.split(',')) # split by ',' or
> whitespace
> if access.startswith('(') and access.endswith(')'):
> access = access[1:-1]
> - access = access.split()
> + access = access.replace(',', ' ').split() # split by ','
> or whitespace
> else:
> access = SignalRule.ALL
>
> @@ -162,7 +161,7 @@
>
> space = ' ' * depth
>
> - if self.all_accesss:
> + if self.all_access:
> access = ''
> elif len(self.access) == 1:
> access = ' %s' % ' '.join(self.access)
> @@ -192,7 +191,7 @@
> def is_covered_localvars(self, other_rule):
> '''check if other_rule is covered by this rule object'''
>
> - if not other_rule.access and not other_rule.all_accesss:
> + if not other_rule.access and not other_rule.all_access:
> raise AppArmorBug('No access specified in other signal rule')
>
> if not other_rule.signal and not other_rule.all_signals:
> @@ -201,8 +200,8 @@
> if not other_rule.peer and not other_rule.all_peers:
> raise AppArmorBug('No peer specified in other signal rule')
>
> - if not self.all_accesss:
> - if other_rule.all_accesss:
> + if not self.all_access:
> + if other_rule.all_access:
> return False
> if other_rule.access != self.access:
> return False
> @@ -229,7 +228,7 @@
> raise AppArmorBug('Passed non-signal rule: %s' %
> str(rule_obj))
>
> if (self.access != rule_obj.access
> - or self.all_accesss != rule_obj.all_accesss):
> + or self.all_access != rule_obj.all_access):
> return False
>
> if (self.signal != rule_obj.signal
> @@ -245,7 +244,7 @@
> return True
>
> def logprof_header_localvars(self):
> - if self.all_accesss:
> + if self.all_access:
> access = _('ALL')
> else:
> access = ' '.join(sorted(self.access))
> === modified file ./utils/test/test-signal.py
> --- utils/test/test-signal.py 2015-11-19 17:42:26.329879090 +0100
> +++ utils/test/test-signal.py 2015-11-19 17:46:14.472311782 +0100
> @@ -25,7 +25,7 @@
> _ = init_translation()
>
> exp = namedtuple('exp', ['audit', 'allow_keyword', 'deny', 'comment',
> - 'access', 'all_accesss', 'signal', 'all_signals', 'peer',
> 'all_peers'])
> + 'access', 'all_access', 'signal', 'all_signals', 'peer',
> 'all_peers'])
>
> # --- tests for single SignalRule --- #
>
> @@ -39,7 +39,7 @@
> self.assertEqual(expected.peer, obj.peer.regex)
> else:
> self.assertEqual(expected.peer, obj.peer)
> - self.assertEqual(expected.all_accesss, obj.all_accesss)
> + self.assertEqual(expected.all_access, obj.all_access)
> self.assertEqual(expected.all_signals, obj.all_signals)
> self.assertEqual(expected.all_peers, obj.all_peers)
> self.assertEqual(expected.deny, obj.deny)
> @@ -125,7 +125,6 @@
>
> self.assertEqual(obj.get_raw(1), ' signal send set=term
> peer=/usr/bin/pulseaudio///usr/lib/pulseaudio/pulse/gconf-helper,')
>
> -
> class SignalFromInit(SignalTest):
> tests = [
> # SignalRule object
> audit allow deny comment access all? signal
> all? peer all?
> @@ -140,7 +139,6 @@
> def _run_test(self, obj, expected):
> self._compare_obj(obj, expected)
>
> -
> class InvalidSignalInit(AATest):
> tests = [
> # init params expected exception
> @@ -177,7 +175,6 @@
> with self.assertRaises(TypeError):
> SignalRule('r', 'int')
>
> -
> class InvalidSignalTest(AATest):
> def _check_invalid_rawrule(self, rawrule):
> obj = None
> @@ -309,7 +306,6 @@
> ('signal receive,' , [ False , False ,
> False , False ]),
> ]
>
> -
> class SignalCoveredTest_03(SignalCoveredTest):
> rule = 'signal send set=quit,'
>
> @@ -437,9 +433,6 @@
> ('deny signal send,' , [ False , False
> , False , False ]),
> ]
>
> -
> -
> -
> class SignalCoveredTest_Invalid(AATest):
> def test_borked_obj_is_covered_1(self):
> obj = SignalRule.parse('signal send peer=/foo,')
>
>
>
>
> Regards,
>
> Christian Boltz
> --
> Jaaaa! Und am besten den Rest des Desktops auch noch mit DHTML
> nachprogrammieren, ach was sag ich, JavaScript ist ja /die/ Sprache,
> um das ganze Betriebsystem neu zu entwickeln.
> [Ratti in fontlinge-devel]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
>
--
Regards,
Kshitij Gupta
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20151120/22d976b5/attachment-0001.html>
More information about the AppArmor
mailing list