Looks good. Acked-by: Kshitij Gupta <[email protected]>
On Thu, Nov 19, 2015 at 11:11 PM, Christian Boltz <[email protected]> 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 <[email protected]> > > > [ 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 > [email protected] > Modify settings or unsubscribe at: > https://lists.ubuntu.com/mailman/listinfo/apparmor > -- Regards, Kshitij Gupta
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
