[apparmor] [patch 1/3] add base and capability rule classes
Christian Boltz
apparmor at cboltz.de
Thu Nov 27 20:06:10 UTC 2014
Hello,
Am Montag, 24. November 2014 schrieb Steve Beattie:
> On Sat, Nov 22, 2014 at 12:38:02AM +0100, Christian Boltz wrote:
> > > Mmm, I'm still not entirely convinced, but it feels like it again
> > > might be an artifact of the hasher and/or a lack of use of classes
> > > for profiles.
> >
> > Imagine aa.py will have something like this one day (for non-clean
> > mode):
> >
> > for default_write_order as rule_type:
> > data += aa[profile][hat][rule_type].get_raw()
> > aa[profile][hat][rule_type].delete_all_rules()
> >
> > If you tell me another way to implement the last line, I'll use it
> > ;-)
> If you're deadset on keeping the references to rulesets in a
> dict-style structure indexed by a string rule_type, I can think of at
> least a couple of ways:
I don't insist on keeping it as a dict ;-)
All I want is something that is allows to have the rule type as a
variable so that we can
a) loop over all rule types
b) have something like the match_includes() function from part 3 of my
patchset that works for all rule types (with the rule_type as
parameter)
I don't really care about the data type (being it dict, a class or
something else) as long as the above is possible ;-)
> 1) we already have a mapping from rule_types to print functions. I'm
> assuming get_raw()/get_clean() are replacements for those functions,
> but you could still retain a mapping to classes, such that
>
> a[profile][hat][rule_type] = ruleset_map[rule_type]()
>
> works.
>
> 2) you could grab the type of the existing ruleset and ask it for a
> new object:
>
> t = type(a[profile][hat][rule_type])
> a[profile][hat][rule_type] = t()
>
> There are surely other ways.
TMTOWTDI? We are talking about python, not perl! ;-)
Anyway, your proposal 2) looks good, is future-proof and even works with
['include'] :-) (which exploded with v3)
Thanks for the idea!
This also means I no longer need the delete_all_rules() function which
you loved so much ;-))
> > Does this interesting[tm] behaviour have a real-world usecase?
> > ("confuse developers" doesn't count ;-)
>
> Well, I simplified things in my explanation. If you assign to field,
> the contents are local to the instance. But if you modify what the
> field refers to (e.g. list.append()), it's going to change for all
> instances, because the instances only have a stored reference to
> the modified object (i.e. the only thing stored in the instance is
> a pointer to the modified object, not the object itself).
My question was more about what the python devs smoked^Wthought when
implementing this behaviour ;-)
Thanks for the details nevertheless!
> > > At sort of a meta level, I know in lots of instances we try to
> > > avoid
> > > making unnecessary changes to policy, yet I think there is value
> > > in
> > > normalizing policy as well, which you could consider separating
> > > out a
> > > multiple capability rule into multiple one-capability-per-rule
> > > rules
> > > as being a type of normalization.
> >
> > We have get_clean() and get_raw() to explicitely choose between
> > original rules and "cleaned up" rules, so that wouldn't be a
> > problem.
> >
> > The only problem would be the data type of the *Rule.get_clean() -
> > currently it returns a string, so for multiple rules we could return
> > a multi-line string (breaks sorting) or an array of lines (which
> > would mean a little behaviour change).
> >
> > AFAIK capability rules are the only rule type with this problem, so
> > I'd like to avoid having a solution that makes our life harder for
> > all other rule types just to cover a corner case.
>
> Other than maybe having to convert to having lists of lines, if you
> encapsulate the logic for this in the capability specific classes, I'm
> not sure how it makes things more difficult for other rule types.
> And, given how long dbus rules can be, I'd bet other rules classes
> will want the ability to return multiple lines.
Well, there's a difference - multiple lines vs. multiple rules ;-)
For multi-capability rules, the perfect solution would be to return two
(or more) independent lines, basically:
[
' capability audit_write,',
' capability chown,'
]
which also means the sort() in CapabilityRuleset {sh,c}ould sort them to
the right place.
However for dbus rules, you don't want to sort the lines individually
(only the whole rule).
This means we'll have to return the dbus rule as a string with some \n,
like:
' dbus send \n
bus=session \n
path=/com/example/path \n
interface=com.example.Interface '
So I'm still not sure if multi-capability lines are worth special
handling - and if we really want it, I'd go the \n way like for dbus
rules. This breaks sorting, but avoids that we need to add special
handling for a corner case in one rule type. (I promise to fix it in the
perfect way if we get a bugreport about the broken sorting from someone
who is not reading this mailinglist ;-)
(not implemented in the patch yet - that's something for a follow-up
patch)
> > > > +# The key for representing bare rules such as "capability," or
> > > > "file," +ALL = '\0ALL'
> > >
> > > I'm not so keen on having a magic dict key to represent the bare
> > > keyword rules; one way I'd thought about handling this was to have
> > > a keyword rule be represented by a subclass of the rule type; e.g.
> > > CapabilityAllRule as a subclass of CapabilityRule, and trying to
> > > encapsulate most of the differences within that subclass.
> >
> > As discussed on IRC, I'm not too keen on adding another class just
> > to
> > avoid two "if ALL in self.capabilitiy" branches.
> >
> > I changed it to use self.all_caps to avoid having a magic key, even
> > if that makes some places slightly more interesting than they would
> > be with the ALL magic key.
>
> Another option, if you converted to storing internally as sets,
> would be to have a locally defined (frozen) ALL set, that contains
> all the known capabilities. Again, you'd have the bonus of having
> set operations work correctly.
That sounds like making things more complicated ;-)
"All capabilities" should just mean that, without checking against a
list of known capabilities.)
> (Even better if libapparmor exported in one place a list of all the
> known capabilities that the python tools could use to construct the
> ALL set, as well as to ease checking whether a capability is valid or
> not.)
Can we first fix the parser build so that the list of capabilities is
complete even if compiled on an old kernel (bugreport exists) before
thinking about that? ;-)
> > I changed that, and also added init_vars() that is called by
> > __init__
> > (contains just "pass" in BaseRule/BaseRuleset) so that the child
> > classes don't need to re-implement or call the parent __init__ when
> > they need to add some variables.
>
> Can you rename this function as _init_vars() to indicate that it is
> internal to this class hierarchy, and not to be invoked by outsiders?
Done.
> > > I suspect we will want a rule equality test method.
> >
> > Yes. I added is_equal() (to BaseRule) which calls
> > is_equal_localvars() in CapabilityRule.
>
> Err. Why is it not just an overridable method? Pretty much every rule
> type is going to have it's own equality test, I'd bet, If there are
> common subchecks (audit, deny, allow; comments, maybe), have the base
> class make those available.
That's exactly what I did ;-) - is_equal() is in the base class and
checks things like audit and deny. After that, it calls
is_equal_localvars() in CapabilityRule to check the capability-specific
things.
This way has the advantage that we don't need to use super() in
CapabilityRules to do the base checks.
> > The question is how strict is_equal should be, for example
> > capability chown vs. allow capability chown
> > capability chown, vs capability chown, # comment
> >
> > Technically they are equal, so I disabled the checks for everything
> > that doesn't change the meaning of a rule ("allow" keyword and
> > comment - and also rawrule because it includes those)
>
> Hrm. I can see where both types of equality tests, logical and strict,
> would be useful. So a common pythonic idiom would be to take an
> additional named argument, e.g. strict=, that when set to False
> returns logical equality, and when True, returns a strict (comments,
> modifiers, etc.) equality test result. What the default should be
> would probably be best driven by what the more common need is.
Ah, the usual answer - "both of them" ;-)
I added the strict= parameter - False by default because I think users
typically are interested in behaviour, not in having exactly the same
comment ;-)
> > > > + def set_log(self, parsed_log_event):
> > > > + '''parse and store log event'''
> > > > +
> > > > + if parsed_log_event['operation'] != 'capable':
> > > > + raise AppArmorBug('Invalid capability log event -
> > > > not a
> > > > capability event') +
> > > > + if not parsed_log_event['name']:
> > > > + raise AppArmorBug('Invalid capability log event -
> > > > name
> > > > is empty') +
> > > > + self.capability = [ parsed_log_event['name'] ]
> > > > +
> > > > + self.logmode = parsed_log_event['aamode']
> > >
> > > What is logmode saved for?
> >
> > aa-logprof uses it, and the default answer (allow or deny) depends
> > on
> > complain vs. enforce mode. Oh, and it prints a nice headline like
> > "Changes in compline mode" ;-)
> >
> > That's exactly how the perl tools worked. The question is if we want
> > to keep this behaviour, or want to change it.
>
> Right. I don't have a problem with the behavior, per se. Rather
> than have a completely different set of _log() operations, I would
The *_log() operations make sense (they are still unused, but that will
change sooner or later).
> store the log event as a separate optional argument to __init__()
> (e.g. log_event=None) that is stored off as-is, and then any
> information that tools want to report about the log event that (first)
> generated the rule can be pulled directly from the log event the Rule
> hands back, if there is one to hand back.
Please allow me to disagree ;-)
I want to have log events as normal rule objects so that we can pass
them to is_covered() etc. without a need to "convert" them.
(That obviously needs changes in log handling/parsing, see my /dev/brain
for details ;-)
If we find out the tools need more log event details (besides the rule-
relevant information) than just complain vs. enforce mode, we can change
the storage to a self.log_details dict later.
> (I had a thought of supporting a list of log events, so that you could
> report statistics on how many events were covered by a given rule
> during a tooling run, but given how poorly we handle large log files
> already, keeping references to multiple events won't help with our
> memory usage. But we could perhaps add a counter.)
Indeed, adding a counter shouldn't be too expensive ;-)
Can you please remind me when we have the log handling converted to rule
objects?
The updated patch (v4) is attached. Changes since v3:
- BaseRule:
- rename init_vars() to _init_vars()
- add 'strict' parameter to is_equal()
- BaseRuleset:
- remove delete_all_rules() function and merge it into __init__()
- CapabilityRule:
- change self.capability to set() (based on the patch from Steve)
Regards,
Christian Boltz
--
Tatsächlich gibt es nach wie vor sogar OS, die auf systemd und
upstart initial verzichten. Kann natürlich sein, dass man dazu
die Yast-Hängematte verlassen muss.
[Tobias Crefeld in opensuse-de]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v4-1-add-base-and-capability-rule-class.diff
Type: text/x-patch
Size: 13364 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141127/a61d11da/attachment-0001.bin>
More information about the AppArmor
mailing list