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

[Christian Boltz]

Hello,

Am Mittwoch, 3. Dezember 2014 schrieb Steve Beattie:
> On Sun, Nov 30, 2014 at 12:45:49AM +0100, Christian Boltz wrote:

> I agree with that; however, I think the difference between the two
> situations (raw_rule not matching the passed cap_list versus calling
> set_param() twice) is that you have to go out of your way to set
> raw_rule to be different from the capability that represents in
> the same constructor call, whereas with some convoluted logic (and
> really, aa.py excels at convoluted logic), 

Indeed ;-) - but it has its roots in the Immunix::AppArmor module, so 
that is what you really should blame *g*

> it would be possible to
> become confused about whether set_param() had already been called on
> a partially initialized object.

OK, good argument ;-)

> > So if someone wants to shoot himself in the foot - hey, it's his
> > foot, not mine *eg*
> 
> That said, with your followup email, I think it's worth trying to do
> better with raw_rule handling. My intent is to make it easy to do
> the easy and correct things, and make you go out of your way to do
> the things that will likely cause bugs.

Ideally we would make it impossible to do things that cause bugs. 
However that would include making the internal variables really internal 
(aka "not available for external access") which isn't possible in python 
AFAIK (praise PHP for allowing that!). However, such private variables 
would make testing some corner cases (where I intentionally break the 
internal variables) harder ;-)

> > > 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.
> 
> I can see a relatively simple algorithm for handling matching log
> events with regex patterns: convert apparmor regex to python regex
> (caching if necessary) and then ask the python regex if the log event
> matches. With two regexs, I'd have to do some algorithm research
> (probably John has pointers).

aa-mergeprof already somehow[tm] has to compare two profiles (including 
file rules), which means doing exactly that. We should have a look at it 
when we come to implementing file rules - either we find the perfect way 
to do it in aa-mergeprof, or we find a (possible) bug in it ;-)  
(no, I did not check the code before writing this mail ;-)
 
> > > - use standard pythonic notions of using a superclass'
> > > __init__ function.
> > 
> > I know it's more pythonic, but it's also harder to read ;-)
> 
> It would be less annoying to read if we were python3 only, and could
> just do super().__init__(blah blah).

Argh, py version fun again. At least the py2 code still works with py3 
this time, which already sounds like a surprise ;-)

Anyway - having a function with another name in BaseRule would solve 
this problem in a less cryptic^Wpythonic way.

> > Minor nitpicking (CapabilityRule __init__):
> > 
> > If cap_list is a string and contains multiple capabilities ("chown
> > chmod"), you should split() it.
> 
> That's... not really an interface use case I'd considered; my thinking
> was if you want to give the constructor a group of capabilities, you
> give it a list.

That's the theory, yes. In practise someone gives you a string, and you 
don't know exactly what this string contains. Maybe he just parsed it 
with "capability (.*),"

My untested proposal should cover this without adding much code (same 
line count if you ignore the blank line) or overhead, so there's no 
reason not to split the string ;-)

> > > - 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
> 
> I'm really of two minds about get_raw and get_clean. On the one hand,
> the rules themselves know best how to format themselves.

Not really - at the moment, CapabilityRule and CapabilityRuleset don't 
know if they live inside a profile, a hat or an include (which all have 
different whitespace levels), so they don't know which depth is needed.

> On the other,
> this leads to the difficulty of sorting multi-line rules.

Not really, as long as you use the same depth for all rules you want to 
sort.

On the long(er) term, we could think about setting/changing the 
whitespace only for get_clean() and keep it unchanged in get_raw(). 
Let me warn you that this would mean doing quite some changes in aa.py 
(which does strip() all lines by default), so let's just keep it 
somewhere in the not-too-important section of the TODO list ;-)
 
> > > - 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 ;-))
> 
> When I first started re-arranging your patches, I originally had the
> parsing functions in __init__.py and capability.py, 


Funny enough that this is basically what we then discussed on IRC (see 
my follow-up mail)

> but then pushed them to the separate parser.py file.  
> I don't have a strong feeling one
> way or the other, but I'd note that one of the drawbacks of doing so
> was that the references to the functions were somewhat awkward; we'd
> either need to import each of the individual parsing functions, or do
> what I'd tried, which was to import capability into the name space
> (and reference capability.parse_raw()), but then that collided with a
> few of the local loop variables. A third way of coping with this I'd
> thought of was to hang the parse_raw function as a class method off
> of the Rule classes; I think I'll give that a try to see how it
> looks.

See the IRC discussion quoted in my follow-up mail:
Integrating the parse_raw function in CapabilityRule.__init__() is 
probably the best way - either directly or by calling an internal 
function.

Yes, that means some more imports are needed in capability.py - but it 
has the big advantage of having everything capability-related together 
in one file. (Well, except the regex, but let's ignore this detail for 
now ;-)

> I'll send out version 5 of the patch set shortly, with my changes
> flattened into yours.

Sounds good :-)

The remaining question is if we want to split the commit to know who 
broke which part *g* or if we just commit the merged patch with two 
authors mentioned.

Oh, and is one of us still allowed to send an Ack for a merged patch 
(which would mean acking partially self-written code), or do we need to 
force someone else to review it? ;-)


Regards,

Christian Boltz
-- 
Funktionieren Windows 98-Witze auch unter Windows 2000
oder braucht man da ein Update?