[apparmor] [patch 1/3] add base and capability rule classes

[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:

  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.

> 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).

I still stand by the following recommendation:

> > Generally we should probably avoid Class Attributes unless they are
> > something immutable across the class.

even if it's not strictly necessary for things that hold simple values
(e.g. True/False) where assignment would occur.

> OK, that means all variables (except can_glob and can_glob_ext) should 
> be initialized in __init__.

Yes.

> > > > > utils/apparmor/rule/capability.py:
> > > > > 
> > > > > class capability_rule(base_rule):
> > > > >     '''Class to handle and store a single capability rule'''
> > > > >     
> > > > >     def __init__(self):
> > > > >         self.capability = []
> > > > 
> > > > Why is it a list, for a single capability?
> > > 
> > > Because someone had the idea to allow
> > > 
> > >     capability chown chgrp audit_write,
> > > 
> > > in the profiles ;-)
> > > 
> > > Until now, the utils just parsed this as
> > > 
> > >     "capability"     "chown chgrp audit_write"
> > > 
> > > but that makes finding duplicates quite hard ;-)
> > 
> > Mmm, right. A suggestion would be to make it a set() instead of a
> > list, to force elimination of duplicates.
> 
> Might be an idea, but then we need to ensure not to accidently create 
> keys in it (see hasher ;-)

Hrm, I don't understand the accidentally created key issue you're
raising.

In python, a set() is very similar to a list, except that it guarantees
uniqueness amongst its member elements (which means that there's an
additional constraint that the elements have to be hashable). As a
bonus, you also have common set operations available, like issubset(),
issuperset(), union(), intersection(), etc. These may be really helpful
for dealing with capability rules that contain multiple capabilities.

See https://docs.python.org/3/library/stdtypes.html#set for details.

> > 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.

> > > +# 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.

(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.)

> 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?

> > 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.

> 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.

> > > +            for cap_obj in self.rules:
> > > +                if inccaps.covered_obj(cap_obj, True, True):
> > > +                    self.delete_obj(cap_obj)
> > > +                    deleted.append(cap_obj)
> > > +
> > > +        return len(deleted)
> > 
> > Would it make sense from a UI perspective to return the deleted list,
> > which could then be displayed if requested?
> 
> Good question - aa-logprof displays "xx rules deleted" when adding an 
> include that covers some of the existing rules, and at the end, we have 
> (V)iew changes which compares the old and new profile.
> 
> The only place where knowing the deleted list would make sense is 
> aa-mergeprof in 3-way mode, but we don't have 3-way yet. Besides that, 
> we'll need a "real diff" for 3-way merge and not delete_duplicates.
> 
> So I'd say lets keep len(deleted) for now and change it when we have a 
> real usecase.

Okay. It just felt odd to construct a list of deleted rules and then
only return their length, throwing the list away.

> > > +    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
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.

(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.)

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141124/820f737f/attachment.pgp>