[apparmor] [patch] Add SignalRule and SignalRuleset classes
Christian Boltz
apparmor at cboltz.de
Thu Nov 19 17:41:07 UTC 2015
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]
More information about the AppArmor
mailing list