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

[Christian Boltz]

Hello,

Am Samstag, 29. November 2014 schrieb Steve Beattie:
> First off, let me say thanks for driving forward on this and not
> getting put off by my feedback.

I know I sometimes send out evil feedback, so I'm not surprised you take 
your chance for a revenge ;-)  And I'm also not surprised that my code 
isn't perfect, so I'm always happy if someone sends some improvements.

I'm more surprised that Kshitij doesn't take revenge ;-) - he's probably 
too busy with his exams.

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

That's an interesting question ;-)

You are right that the changes are quite big for a maintenance release. 

However there are some reasons that let me tend to include it in 2.9.x:
- as a side effect of the conversation to classes, we fix some bugs. 
  Maybe we even accidently ;-) fix some bugs we didn't even notice 
  before.
- the classes are fully covered by tests, which means the risk for 
  regressions is quite small (we can "only" break aa.py)
- we'll have a separate branch that we need to support with bugfixes if
  we don't include the rule classes in 2.9.x ;-)

> > The updated patch (v4) is attached.

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

Sounds like a good argument ;-)

Let me warn you that your __init__() also has a regression when compared 
with my set_* functions - imagine someone calls it with a raw_rule that 
completely differs from the other parameters, like

    cap_rule = CapabilityRule("chown", raw_rule="capability,")

The result of cap_rule.get_raw() will be a very permissive profile ;-) 
but all checks like is_equal() and is_covered() will assume cap_rule 
only allows chown.

(We could in theory check raw_rule with is_equal(), in practise - well, 
see next paragraph ;-)

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

Well, I expect that someone who calls our internal functions knows how 
to do this. I'm not too keen on writing "do not dry your dog in the 
microwave"-style checks and expect at least some sanity when internal 
functions are called ;-)

So if someone wants to shoot himself in the foot - hey, it's his foot, 
not mine *eg*

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

That's not a real difference for capabilities, but file rules can have 
such differences. (BTW: AFAIK Peter is playing with "path" vs. 
"boring_path" for more performance improvements.)

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

OK, when we come to log handling, we'll see which way works the best.

> A summary of the changes:
> 
> CapabilityRule/BaseRule
> - complete initialization of CapabilityRule in __init__(); drop
> _init_vars() 

Ack

> - use standard pythonic notions of using a superclass'
> __init__ function. 

I know it's more pythonic, but it's also harder to read ;-)

What about renaming __init__ to _init_common_vars() in BaseRule?

> - external user that want to specify the capability all rule should 
>   pass CapabilityRule.ALL as the argument to the initializer. 

Ack

Minor nitpicking (CapabilityRule __init__):

If cap_list is a string and contains multiple capabilities ("chown 
chmod"), you should split() it. To make things easier, you can just 
always split() it if it's a string.

Untested:

            if type(cap_list) == str:
-                self.capability = {cap_list}
+                cap_list = cap_list.split()
+
-            elif type(cap_list) == list and len(cap_list) > 0:
+            if type(cap_list) == list and len(cap_list) > 0:
                self.capability = set(cap_list)
            else:
                raise AppArmorBug('Passed unknown object to 
CapabilityRule: %s' % str(cap_list))

> - renamed inlinecomment to just comment

Ack

> - converted get_raw() and get_clean() to use depth=0 by default for
>   convenience

Good idea, even if it comes with the risk of "forgetting" to specify the 
depth parameter. Ack

> - get rid of set_log and set_param as not needed

Well, I'd say "not usable with your changed __init__()" ;-)
That said - Ack

> - move the implementation of set_raw to parser.py:parse_capability()
>   which returns a completed CapabilityRule object.

IMHO it would make sense to keep all capability-specific code in one 
file - rule/capability.py. I don't see a problem with having it outside 
the class, but I doubt having a parser.py with code for all rule types 
is a good idea. 

My experience from rewriting PostfixAdmin is: having everything specific 
to mailboxes in MailboxHandler turned out to be the best way, and allows 
to (ab?)use the basic infrastructure to manage anything, not only 
mailboxes and aliases. You could easily write a module to manage your 
collection of $whatever in some hours ;-))

BTW: please rename inlinecomment to comment and rawrule to raw_rule in 
parse_capability() to stay consistent.

> - rename parse_audit_allow to parse_modifiers and place in parser.py.

If you move parse_capability() to rule/capability.py, 
parse_audit_allow() should be in rule/__init__.py

> - add check to ensure the passed rule object is of CapabilityType for
>   is_covered and is_equal_localvars() 

Ack, good idea.

>   (would prefer to be just is_equal and call self.is_equal_modifiers 
>   within it).

The current way has the advantage that is_equal is always called, so you 
can't forget to call is_equal_modifiers ;-)

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

Ack

> BaseRuleset
> - drop add_raw, delete_raw, is_raw_covered, is_log_covered as not
>   necessary (see discussion above on is_log_covered)

Ack

> - rename add_obj to add, delete_obj to delete, is_obj_covered to
>   is_covered, as interacting via Rule objects is the primary behavior.

Ack

> - fixed up counting in delete() so argument to pop() wouldn't need to
>   be modified by -1.

Ack

> aa.py
> - convert to using new interfaces

Ack

> - drop parse_audit_allow and use parser.py:parse_modifiers() instead.

Ack, except that I'd move the function to rule/__init__.py

A note about test-capability.py - renaming inlinecomment to comment 
would be nice, but I tend to do this as separate patch to keep this 
patchset readable.

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

Especially for test-capability.py, meld turned out to be very useful 
because it highlights which parts of a line were changed.


Regards,

Christian Boltz
-- 
> > > Liegt nicht an meinem .spec.
> > Das sagt jeder ;-)
> Naja, aber ich zu Recht ;))
Sagt auch jeder ;-) *SCNR*
[> David Haller und Christian Boltz in fontlinge-devel]