[apparmor] [patch 3/3] use capability rule class in aa.py and cleanprof.py

Sat Nov 29 10:52:03 UTC 2014

Hi Christian,

First off, let me say thanks for driving forward on this and not getting
put off by my feedback.

Second is that I think these changes are large enough to not be
acceptable for 2.9.1, and that we should branch off 2.9.x before
committing this patch set.

> The updated patch (v4) is attached.
> 
> Changes since v3:
> - document keys used in aa[profile][hat]
> - fix crash in write_capabilities()
> - improve resetting capability ruleset object in write_prior_segments()
>   and serialize_profile_from_old_profile() (includes correct handling 
>   for include rules "for free")
> 
> 
> The usual line statistics:
> v4-1-add-base-and-capability-rule-class.diff - 371 lines added, 0 removed
> v4-2-add-capability-rule-test.diff - 806 lines added, 0 removed
> v4-3-use-capability-rule-class.diff - 68 lines added, 112 removed

Attached is a patch that applies on top of your patch series. Consider
it the cumulative critique of your patch set; in particular it tries to
address what I consider to be the most problematic bit of your patches,
namely that CapabilityRule objects only get partially initialized,
as I'm convinced it will lead to bugs. It requires that callers to
the class know whether or not it has been completely set up or not,
and leads to other funny issues like "what happens when set_param()
is called twice"? (I know what would happen, but I don't know if
that's the behavior you intend or if an exception should be raised;
there's no test case for that situation.)

As for converting log events into Rule objects, I'm of two minds. One
the one hand, this is exactly what is done after applying this
patch set, but the conversion occurs outside of the CapabilityRule
class. On the other, comparing two capability rules is easy; detecting
if a generic log event is covered is a simpler problem than detecting
whether one rule is completely covered by another, when both rules can
contain variables and regular expressions. Log events have an explicit
object, and thus an is_covered() test needs to only see if a given
string is matched by a regex, not whether two regexs cover the same
set of strings, or that one is a subset of the other.  Thus, I've left
a bit of the code and the tests for log matching in but commented out.

A summary of the changes:

CapabilityRule/BaseRule
- complete initialization of CapabilityRule in __init__(); drop _init_vars()
  - use standard pythonic notions of using a superclass' __init__ function.
  - external user that want to specify the capability all rule should
    pass CapabilityRule.ALL as the argument to the initializer.
- renamed inlinecomment to just comment
- converted get_raw() and get_clean() to use depth=0 by default for
  convenience
- get rid of set_log and set_param as not needed
- move the implementation of set_raw to parser.py:parse_capability()
  which returns a completed CapabilityRule object.
- rename parse_audit_allow to parse_modifiers and place in parser.py.
- add check to ensure the passed rule object is of CapabilityType for
  is_covered and is_equal_localvars() (would prefer to be just
  is_equal and call self.is_equal_modifiers within it).

CapabilityRuleset
- drop new_rule() as its not (currently) needed; every location
  that wants a new rule has the context to know what type it is. If
  that becomes not the case, having a ruleset class method that
  points to the parser.py parse function for the rule type is the
  approach I was thinking of taking, to cope with the differing
  types of arguments that each rule's __init__ would need.

BaseRuleset
- drop add_raw, delete_raw, is_raw_covered, is_log_covered as not
  necessary (see discussion above on is_log_covered)
- rename add_obj to add, delete_obj to delete, is_obj_covered to
  is_covered, as interacting via Rule objects is the primary behavior.
- fixed up counting in delete() so argument to pop() wouldn't need to be
  modified by -1.

aa.py
- convert to using new interfaces
- drop parse_audit_allow and use parser.py:parse_modifiers() instead.

I've also attached all four patches folded together (as
folded_capability_rules.patch), to let people see what the resulting
added files look like. The total diffstat looks like:

 apparmor/aa.py              |  190    67   123     0 +++------
 apparmor/cleanprofile.py    |   15     2    13     0
 apparmor/parser.py          |   76    76     0     0 +++
 apparmor/rule/__init__.py   |  189   189     0     0 +++++++++
 apparmor/rule/capability.py |  119   119     0     0 ++++++
 test/test-capability.py     |  866   866     0     0 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 1319 insertions(+), 136 deletions(-)

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: utils-dont_leave_cap_class_half_initialized.patch
Type: text/x-diff
Size: 55696 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141129/867f897c/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: folded_capability_rules.patch
Type: text/x-diff
Size: 65568 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20141129/867f897c/attachment-0003.patch>
-------------- 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/20141129/867f897c/attachment-0001.pgp>