On Sun, Nov 30, 2014 at 12:45:49AM +0100, Christian Boltz wrote:
> 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 ;-)
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), it would be possible to
become confused about whether set_param() had already been called on
a partially initialized object.
> 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.
> > 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.
Yes, that was my point, and it's also true for dbus, unix domain
sockets, and others.
> (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.
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).
> > - 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).
> 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.
> > - 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. On the other,
this leads to the difficulty of sorting multi-line rules.
> > - 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, 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.
> BTW: please rename inlinecomment to comment and rawrule to raw_rule in
> parse_capability() to stay consistent.
Done.
> > - 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
Done.
> > (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 ;-)
I've left this as is.
> 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.
Done.
I'll send out version 5 of the patch set shortly, with my changes
flattened into yours.
--
Steve Beattie
<sbeat...@ubuntu.com>
http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
