[apparmor] [PATCH] utils: Handle the safe/unsafe change_profile exec modes

[Christian Boltz]

Hello,

(just answering on your comments and deleting everything you didn't 
comment on)

Am Montag, 27. Juni 2016, 17:10:26 CEST schrieb Tyler Hicks:
> On 06/27/2016 03:27 PM, Christian Boltz wrote:
> > Am Samstag, 25. Juni 2016, 15:59:06 CEST schrieb Tyler Hicks:

> >>      def is_covered_localvars(self, other_rule):
> >>          '''check if other_rule is covered by this rule object'''
> >> 
> >> +        if self.execmode != other_rule.execmode:
> >> +            return False
> > 
> > The default execmode is 'safe', so 'safe' and '' / None should be
> > considered as equal.
> 
> That's not always true. The default is 'safe' on very new kernels that
> support stacking. In all other cases, the 'safe' is dropped and the
> default kernel behavior is 'unsafe'.

Yes, things are downgraded to 'unsafe' behaviour', but that happens with 
or without an explicit 'safe' keyword, so 'safe' and None are still the 
same ;-)

> >> @@ -150,10 +170,12 @@ class ChangeProfileRule(BaseRule):
> >>          return True
> >>      
> >>      def logprof_header_localvars(self):
> >> +        execmode_txt        = logprof_value_or_all(self.execmode,
> >> 
> >>   None)
> > 
> > Same again ;-)
> > 
> > Also, did you really test this in all variants? I'd guess not
> > specifying an exec mode will result in
> > 
> >     Exec Mode:
> > or even
> > 
> >     Exec Mode: None
> 
> 'Exec Mode: None' is what is printed. There are even tests below that
> prove it.

Right, I noticed it when reading the test changes.

Note that you are including a type conversation here (None -> 'None'), 
and that this 'None' is not translatable.

I'd say not displaying the execmode if it isn't explicitely set is 
probably the easiest option ;-)  If you don't like this, displaying 
something like "ExecMode: default" might be an option.

> I didn't want to bubble up a default execmode behavior in the python
> utils because, as mentioned above, the default behavior is confusing.
> My preference is for the tools to mostly ignore safe/unsafe and let
> the parser and kernel figure it out.
> 
> If a policy author has explicitly chosen safe or unsafe, then the
> Python utils should preserve that decision.

They'll do that ;-)

> > 'safe' / 'unsafe' would be better choices ;-)
> 
> I disagree unless we want to duplicate the parser's logic of checking
> the kernel features and deciding what the default execmode is.

OK, good point ;-)

> >> --- a/utils/test/test-parser-simple-tests.py
> >> +++ b/utils/test/test-parser-simple-tests.py

> >> +    # The tools don't detect conflicting change_profile exec modes
> >> +    'change_profile/onx_conflict_unsafe1.sd',
> >> +    'change_profile/onx_conflict_unsafe2.sd',
> > 
> > OK for now, but having conflict detection would be nice.
> 
> Yikes. I'd hope not. After writing this patch, I've come to the
> conclusion that the utils are reimplementing way too much of what the
> parser is already doing. 

I know, and I'll happily use libapparmor if it provides something useful 
for the tools. I'm not really talking about parsing the profiles (because 
returning a proper *Rule object from libapparmor would be 
interesting[tm], but a way to replace is_covered() would really be nice 
because this is one of the more interesting code parts ;-)

OTOH, I found several documentation bugs during my work on the tools, so 
the reimplementation of parser functions can't be too bad ;-)

If you really want to avoid duplication, that would mean to rewrite the 
tools in C or C++ - and I slightly ;-) doubt we want to do that.

> This patch took way too long to essentially
> rewrite in Python what I've already developed and tested in the
> parser.

Just curious - how long did you need for it?
(I'll tell you about my estimate afterwards ;-)


Regards,

Christian Boltz
-- 
Auch wenn da nix sein KANN und Du lieber neue Parameter einbaust. Tust
Du MIR bitte mal den Gefallen und liest Du wenigstens EINMAL Deine
main.cf auf komische Umbrüche und Einträge hin durch? Nur mir zuliebe,
bitte. Ich weiß, ist natürlich Unsinn. Machst Du es trotzdem?
[Peer Heinlein in postfixbuch-users]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160628/bc6e2317/attachment.pgp>