Re: [apparmor] [patch 1/3] Add NetworkRule and NetworkRuleset classes
Hello,
On Sat, Apr 18, 2015 at 2:49 AM, Christian Boltz wrote:
> Hello,
>
> Am Mittwoch, 15. April 2015 schrieb Kshitij Gupta:
> > On Wed, Apr 15, 2015 at 3:07 AM, Christian Boltz wrote:
> > > this patch adds utils/apparmor/rule/network.py with the NetworkRule
> > > and NetworkRuleset classes. These classes are meant to handle
> > > network rules.
> > >
> > > In comparison to the existing code in aa.py, relevant news are:
> > > - the keywords are checked against a list of allowed domains, types
> > > and>
> > > protocols (these lists are based on what the utils/vim/Makefile
> > > generates - on the long term an autogenerated file with the
> > > keywords
> > > for all rule types would be nice ;-)
> >
> > In theory you can have a script generate a python module that solely
> > contains these keywords, which can be imported in here. Sounds pretty
> > trivial ;-)
>
> We have something like that already for creating apparmor.vim (see
> utils/vim/Makefile) - but I'd describe the code that does that as
> "scary" ;-)
>
> Yes, it can be done, and it can even be done in a more sane way ;-)
>
> > > - there are variables for domain and type_or_protocol instead of
> > >
> > > first_param and second_param. (If someone is bored enough to map
> > > the
> > > protocol "shortcuts" to their expanded meaning, that shouldn't be
> > > too
> > > hard.)
> > >
> > > - (obviously) more readable code because we have everything at one
> > > place>
> > > now
> >
> > yay!
> >
> > > - some bugs are fixed along the way (for example, "network foo" will
> > > now>
> > > be kept, not "network foo bar" - see my last mail about
> > > write_net_rules() for details)
> > >
> > > [ 44-add-NetworkRule-and-NetworkRuleset-classes.diff ]
> > >
> > > === added file 'utils/apparmor/rule/network.py'
> > > --- utils/apparmor/rule/network.py 1970-01-01 00:00:00 +
> > > +++ utils/apparmor/rule/network.py 2015-04-14 20:47:40 +
>
> > > +# missing in manpage: 'unix', 'rds', 'llc', 'can', 'tipc',
> > > 'iucv', 'rxrpc', 'isdn', 'phonet', 'ieee802154', 'caif', 'alg',
> > > 'nfc', 'vsock'
> > > +
> >
> > comments from others on the missing types?
>
> That line should have gone into a bugreport or a patch for
> parser/apparmor.d.pod ;-)
>
> > > +network_type_keywords = ['stream', 'dgram', 'seqpacket', 'rdm',
> > 'raw',
> > > 'packet']
> > > +network_protocol_keywords = ['tcp', 'udp', 'icmp']
> > > +
> > > +
> > > +RE_NETWORK_DOMAIN = '(' + '|'.join(network_domain_keywords) + ')'
> > > +RE_NETWORK_TYPE = '(' + '|'.join(network_type_keywords) + ')'
> > > +RE_NETWORK_PROTOCOL = '(' + '|'.join(network_protocol_keywords) +
> > > ')'
> > neat!
>
> If we need it more often, it might even be worth a little function.
> We'll see...
>
> > > +RE_NETWORK_DETAILS = re.compile(
> > > +'^\s*(' +
> > > +'(?P' + RE_NETWORK_DOMAIN +
> > > ')(\s+(?P' + RE_NETWORK_TYPE + '|' +
> > > RE_NETWORK_PROTOCOL + '))?' + # domain, optional type or protocol
> >
> > exercising new/larger monitor privileges? ;-)
>
> Not really, it even fits on my laptop screen ;-)
>
> > can be split into two lines I think.
>
> Good idea.
>
> > +'|' + # or
> >
> > > +'(?P' + RE_NETWORK_PROTOCOL + ')' + # protocol
> > > only +')\s*$')
> >
> > The leading and trailing \s* become superfluous as __parse has:
> > rule_details = matches.group('details').strip()
>
> I know, but it doesn't hurt and helps in case someone uses that regex
> in another way one day ;-)
>
> > > +
> > > +
> > > +
> > > +
> > > +
> >
> > what is the standard for whitespace separation?
>
> I don't know if there's a standard ;-)
>
> > 7 seems a bit much.
>
> Agreed, I trimmed some of them.
>
> > > +else:
> > > +raise AppArmorBug('Passed unknown domain to
> > > NetworkRule: %s' % str(domain))
> >
> > str(domain) is unnecessary, as type of domain is known to be str.
>
> Indeed, it's superfluous inside an "elif type(domain) == str:" ;-)
>
> > > +else:
> > > +raise AppArmorBug('Passed unknown object to
> > > NetworkRule: %s' % str(domain))
>
> (but still needed here)
>
> > > +self.type_or_protocol = None
> > > +self.all_type_or_protocols = False
> > > +if type_or_protocol == NetworkRule.ALL:
> > > +self.all_type_or_protocols = True
> > > +elif type(type_or_protocol) == str:
> > > +if type_or_protocol in network_protocol_keywords:
> > > +self.type_or_protocol = type_or_protocol
> > > +elif type_or_protocol in network_type_keywords:
> > > +if self.all_domains:
> > > +raise AppArmorException('Passing type %s to
> > > NetworkRule without specifying a domain keyword is not allowed' %
> > > str(type_or_protocol))
> >
> > str(type_or_protocol) not required.
>
> Right.
>
> > +self.type_or_protocol = type_or_protocol
> >
> > > +else:
> > > +raise AppArmorBu
Re: [apparmor] [patch 1/3] Add NetworkRule and NetworkRuleset classes
Hello,
Am Mittwoch, 15. April 2015 schrieb Kshitij Gupta:
> On Wed, Apr 15, 2015 at 3:07 AM, Christian Boltz wrote:
> > this patch adds utils/apparmor/rule/network.py with the NetworkRule
> > and NetworkRuleset classes. These classes are meant to handle
> > network rules.
> >
> > In comparison to the existing code in aa.py, relevant news are:
> > - the keywords are checked against a list of allowed domains, types
> > and>
> > protocols (these lists are based on what the utils/vim/Makefile
> > generates - on the long term an autogenerated file with the
> > keywords
> > for all rule types would be nice ;-)
>
> In theory you can have a script generate a python module that solely
> contains these keywords, which can be imported in here. Sounds pretty
> trivial ;-)
We have something like that already for creating apparmor.vim (see
utils/vim/Makefile) - but I'd describe the code that does that as
"scary" ;-)
Yes, it can be done, and it can even be done in a more sane way ;-)
> > - there are variables for domain and type_or_protocol instead of
> >
> > first_param and second_param. (If someone is bored enough to map
> > the
> > protocol "shortcuts" to their expanded meaning, that shouldn't be
> > too
> > hard.)
> >
> > - (obviously) more readable code because we have everything at one
> > place>
> > now
>
> yay!
>
> > - some bugs are fixed along the way (for example, "network foo" will
> > now>
> > be kept, not "network foo bar" - see my last mail about
> > write_net_rules() for details)
> >
> > [ 44-add-NetworkRule-and-NetworkRuleset-classes.diff ]
> >
> > === added file 'utils/apparmor/rule/network.py'
> > --- utils/apparmor/rule/network.py 1970-01-01 00:00:00 +
> > +++ utils/apparmor/rule/network.py 2015-04-14 20:47:40 +
> > +# missing in manpage: 'unix', 'rds', 'llc', 'can', 'tipc',
> > 'iucv', 'rxrpc', 'isdn', 'phonet', 'ieee802154', 'caif', 'alg',
> > 'nfc', 'vsock'
> > +
>
> comments from others on the missing types?
That line should have gone into a bugreport or a patch for
parser/apparmor.d.pod ;-)
> > +network_type_keywords = ['stream', 'dgram', 'seqpacket', 'rdm',
> 'raw',
> > 'packet']
> > +network_protocol_keywords = ['tcp', 'udp', 'icmp']
> > +
> > +
> > +RE_NETWORK_DOMAIN = '(' + '|'.join(network_domain_keywords) + ')'
> > +RE_NETWORK_TYPE = '(' + '|'.join(network_type_keywords) + ')'
> > +RE_NETWORK_PROTOCOL = '(' + '|'.join(network_protocol_keywords) +
> > ')'
> neat!
If we need it more often, it might even be worth a little function.
We'll see...
> > +RE_NETWORK_DETAILS = re.compile(
> > +'^\s*(' +
> > +'(?P' + RE_NETWORK_DOMAIN +
> > ')(\s+(?P' + RE_NETWORK_TYPE + '|' +
> > RE_NETWORK_PROTOCOL + '))?' + # domain, optional type or protocol
>
> exercising new/larger monitor privileges? ;-)
Not really, it even fits on my laptop screen ;-)
> can be split into two lines I think.
Good idea.
> +'|' + # or
>
> > +'(?P' + RE_NETWORK_PROTOCOL + ')' + # protocol
> > only +')\s*$')
>
> The leading and trailing \s* become superfluous as __parse has:
> rule_details = matches.group('details').strip()
I know, but it doesn't hurt and helps in case someone uses that regex
in another way one day ;-)
> > +
> > +
> > +
> > +
> > +
>
> what is the standard for whitespace separation?
I don't know if there's a standard ;-)
> 7 seems a bit much.
Agreed, I trimmed some of them.
> > +else:
> > +raise AppArmorBug('Passed unknown domain to
> > NetworkRule: %s' % str(domain))
>
> str(domain) is unnecessary, as type of domain is known to be str.
Indeed, it's superfluous inside an "elif type(domain) == str:" ;-)
> > +else:
> > +raise AppArmorBug('Passed unknown object to
> > NetworkRule: %s' % str(domain))
(but still needed here)
> > +self.type_or_protocol = None
> > +self.all_type_or_protocols = False
> > +if type_or_protocol == NetworkRule.ALL:
> > +self.all_type_or_protocols = True
> > +elif type(type_or_protocol) == str:
> > +if type_or_protocol in network_protocol_keywords:
> > +self.type_or_protocol = type_or_protocol
> > +elif type_or_protocol in network_type_keywords:
> > +if self.all_domains:
> > +raise AppArmorException('Passing type %s to
> > NetworkRule without specifying a domain keyword is not allowed' %
> > str(type_or_protocol))
>
> str(type_or_protocol) not required.
Right.
> +self.type_or_protocol = type_or_protocol
>
> > +else:
> > +raise AppArmorBug('Passed unknown type_or_protocol
> > to NetworkRule: %s' % str(type_or_protocol))
>
> str(type_or_protocol) not required. #copy paste reviews
;-)
> > +def is_equal_localvars(self, rule_obj):
> > +'''compare if rule-specific variables are equal'''
> > +
> > +if not type(rule_obj) == NetworkR
Re: [apparmor] [patch 1/3] Add NetworkRule and NetworkRuleset classes
Hello,
On Wed, Apr 15, 2015 at 3:07 AM, Christian Boltz wrote:
> Hello,
>
> this patch adds utils/apparmor/rule/network.py with the NetworkRule and
> NetworkRuleset classes. These classes are meant to handle network rules.
>
> In comparison to the existing code in aa.py, relevant news are:
> - the keywords are checked against a list of allowed domains, types and
> protocols (these lists are based on what the utils/vim/Makefile
> generates - on the long term an autogenerated file with the keywords
> for all rule types would be nice ;-)
>
In theory you can have a script generate a python module that solely
contains these keywords, which can be imported in here. Sounds pretty
trivial ;-)
> - there are variables for domain and type_or_protocol instead of
> first_param and second_param. (If someone is bored enough to map the
> protocol "shortcuts" to their expanded meaning, that shouldn't be too
> hard.)
> - (obviously) more readable code because we have everything at one place
> now
>
yay!
> - some bugs are fixed along the way (for example, "network foo" will now
> be kept, not "network foo bar" - see my last mail about
> write_net_rules() for details)
>
>
>
> [ 44-add-NetworkRule-and-NetworkRuleset-classes.diff ]
>
> === added file 'utils/apparmor/rule/network.py'
> --- utils/apparmor/rule/network.py 1970-01-01 00:00:00 +
> +++ utils/apparmor/rule/network.py 2015-04-14 20:47:40 +
> @@ -0,0 +1,210 @@
> +#!/usr/bin/env python
> +# --
> +#Copyright (C) 2013 Kshitij Gupta
> +#Copyright (C) 2015 Christian Boltz
> +#
> +#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 as published by the Free Software Foundation.
> +#
> +#This program is distributed in the hope that it will be useful,
> +#but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +#GNU General Public License for more details.
> +#
> +# --
> +
> +import re
> +
> +from apparmor.regex import RE_PROFILE_NETWORK
> +from apparmor.common import AppArmorBug, AppArmorException
> +from apparmor.rule import BaseRule, BaseRuleset, parse_modifiers
> +
> +# setup module translations
> +from apparmor.translations import init_translation
> +_ = init_translation()
> +
> +
> +network_domain_keywords = [ 'unix', 'inet', 'ax25', 'ipx', 'appletalk',
> 'netrom', 'bridge', 'atmpvc', 'x25', 'inet6',
> + 'rose', 'netbeui', 'security', 'key',
> 'netlink', 'packet', 'ash', 'econet', 'atmsvc', 'rds', 'sna',
> + 'irda', 'pppox', 'wanpipe', 'llc', 'can',
> 'tipc', 'bluetooth', 'iucv', 'rxrpc', 'isdn', 'phonet',
> + 'ieee802154', 'caif', 'alg', 'nfc', 'vsock'
> ]
> +# missing in manpage: 'unix', 'rds', 'llc', 'can', 'tipc',
> 'iucv', 'rxrpc', 'isdn', 'phonet', 'ieee802154', 'caif', 'alg', 'nfc',
> 'vsock'
> +
>
comments from others on the missing types?
+network_type_keywords = ['stream', 'dgram', 'seqpacket', 'rdm', 'raw',
> 'packet']
> +network_protocol_keywords = ['tcp', 'udp', 'icmp']
> +
> +
> +RE_NETWORK_DOMAIN = '(' + '|'.join(network_domain_keywords) + ')'
> +RE_NETWORK_TYPE = '(' + '|'.join(network_type_keywords) + ')'
> +RE_NETWORK_PROTOCOL = '(' + '|'.join(network_protocol_keywords) + ')'
>
neat!
> +
> +RE_NETWORK_DETAILS = re.compile(
> +'^\s*(' +
> +'(?P' + RE_NETWORK_DOMAIN + ')(\s+(?P'
> + RE_NETWORK_TYPE + '|' + RE_NETWORK_PROTOCOL + '))?' + # domain, optional
> type or protocol
>
exercising new/larger monitor privileges? ;-)
can be split into two lines I think.
+'|' + # or
> +'(?P' + RE_NETWORK_PROTOCOL + ')' + # protocol only
> +')\s*$')
>
The leading and trailing \s* become superfluous as __parse has:
rule_details = matches.group('details').strip()
+
+
> +
> +
> +
> +
> +
>
what is the standard for whitespace separation? 7 seems a bit much.
+class NetworkRule(BaseRule):
> +'''Class to handle and store a single network rule'''
> +
> +# Nothing external should reference this class, all external users
> +# should reference the class field NetworkRule.ALL
> +class __NetworkAll(object):
> +pass
> +
>
+ALL = __NetworkAll
> +
> +def __init__(self, domain, type_or_protocol, audit=False, deny=False,
> allow_keyword=False,
> + comment='', log_event=None):
> +
> +'''
> + NETWORK RULE = 'network' [ [ DOMAIN [ TYPE | PROTOCOL ] ] | [
> PROTOCOL ] ] ','
> +'''
> +
> +super(NetworkRule, self).__init__(audit=audit, deny=deny,
> + allow_keyword=allow_keyword,
> + comment=comment,
> +
