[apparmor] [patch] cleanup aa-disable handling in tools.py

[Steve Beattie]

[Just commenting for the moment on the proposed patches.]

On Sat, Feb 28, 2015 at 02:09:30AM +0100, Christian Boltz wrote:
> You uncovered a bug without even noticing it ;-)
> 
> All functions in tools.py call apparmor.read_profiles() first.
> 
> All functions? No, there's a little Gaulish village ^W^W cmd_disable()
> function that doesn't do that, and therefore
> - doesn't error out when hitting a broken profile directory
> - doesn't find a profile if it doesn't use the default naming scheme
>   (for example /bin/true profile hiding in bin.false)
> 
> Let me propose another patch (for 2.9 and trunk):
> 
> [ tools-read-profiles-in-disable.diff ]

Acked-by: Steve Beattie <steve at nxnw.org> for both.

> === modified file 'utils/apparmor/tools.py'
> --- utils/apparmor/tools.py     2015-02-27 13:19:00 +0000
> +++ utils/apparmor/tools.py     2015-02-27 23:43:12 +0000
> @@ -125,6 +121,8 @@
>                      sys.exit(1)
>  
>      def cmd_disable(self):
> +        apparmor.read_profiles()
> +
>          for (program, profile) in self.get_next_to_profile():
>  
>              output_name = profile if program is None else program
> 
> Hint: it doesn't error out on non-existing files.
> 
> Here's another patch for trunk and 2.9:
> 
> [ load_include_not_found_error.diff ]
> 
> === modified file 'utils/apparmor/aa.py'
> --- utils/apparmor/aa.py        2015-02-20 20:36:55 +0000
> +++ utils/apparmor/aa.py        2015-02-28 00:25:00 +0000
> @@ -4390,6 +4398,8 @@
>          #If the include is a directory means include all subfiles
>          elif os.path.isdir(profile_dir + '/' + incfile):
>              load_includeslist += list(map(lambda x: incfile + '/' + x, os.listdir(profile_dir + '/' + incfile)))
> +        else:
> +            raise AppArmorException("Include file %s not found" % (profile_dir + '/' + incfile) )
>  
>      return 0

So, on the one hand, I'm a little leery of raising another exception
(that likely won't get handled and instead just gets dumped into the
users lap) as in Ubuntu, we had issues after doing the transition
to the python tools where the basic aa-{enforce,complain,disable}
commands failed because the tools didn't understand enough of the
language that had been added. Our goal should be to make the tools
less brittle, not more.

On the other hand, if the include file isn't there, the parser's
going to fail the load anyway (and we helpfully just throw another
back trace in that case, too :/ ). So as long as the tools and parser
are in agreement on what directories to look for for include files,
this should be a safe change. Given that:

Acked-by: Steve Beattie <steve at nxnw.org> for both trunk and 2.9.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- 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/20150306/4a98af15/attachment.pgp>